Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ language implementation (first steps) #236

Merged
merged 12 commits into from
Mar 4, 2022

Conversation

asmfreak
Copy link
Contributor

@asmfreak asmfreak commented Feb 22, 2022

This PR is my attempt to create a working modern-looking (de)serialization layer for UAVCAN v1. This work is aimed at #91 progress.

Steps:

  • Update googletest to a new version as my installation of Arch Linux is already on a newer compiler (GCC 11), which generates a new warning, which crashes build with -Werror.
  • Translate C support library to C++
    • Basic bit patterns
    • Bits and integers (de)serialization
    • Floats (de)serialization
  • Translate serialization to C++ (with tests!)
    • Primitive types
    • Composite types
    • Const arrays
    • Variable arrays
    • Unions
  • Translate deserialization to C++ (with tests!)
    • Primitive types
    • Composite types
    • Const arrays
    • Variable arrays
    • Unions
  • Polyfill std::span with NUNAVUT_ASSERTs to minimize needed code.
  • Decide on how to polyfill std::expected.
  • Remove debug-only headers.

@thirtytwobits
Copy link
Member

Ack. Sorry about the linter error. Looks like something upstream change. Let me fix this then you can rebase and get your change in.

@asmfreak asmfreak changed the title Update googletest to work on GCC 11 C++ language implementation (first steps) Feb 23, 2022
@asmfreak
Copy link
Contributor Author

This commit intoduces (de)serialization of primitive types.

It also introduces an additional debug header - magic_enum - to be used in debugging return codes and pretty printing.

I think, it should be deleted before this implementation goes to main branch.

@thirtytwobits, can you please look into this? I hust want to know, if I'm doing the right thing.

@thirtytwobits
Copy link
Member

Sorry I didn't have time to look at this today. One thing that would help greatly is to fix the little linter errors and whatnot so we can get past this build stage and get to the important tests.

asmfreak added 4 commits March 1, 2022 09:10
It is using span-lite for spans of bytes and tl-expected for a Rust-like Result.
Both are a temporal solution until a better or an alternative solution is found.

Signed-off-by: delphi <[email protected]>
This commit intoduces (de)serialization of primitive types.
It also introduces an additional debug header - magic_enum - to
be used in debugging return codes and pretty printing.
I think, it should be deleted before this implementation goes to
main branch.
Signed-off-by: delphi <[email protected]>

Signed-off-by: delphi <[email protected]>
Copy link
Member

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general the direction this PR is going is problematic for safety-critical applications where every line of code increases certification burdens and where any reliance on the C++ standard library can block adoption of Nunavut if a given safety-critical implementation does not provide the support we require.

The ideal for this code is C++14 with:

  • no reliance on the standard library
  • no use of dynamic memory
  • no use of RTTI or dynamic_cast
  • no memory aliasing
  • no use of macros
  • no use of templates
  • no difference in APIs for code that uses -fno-exceptions and code that does.

We will not achieve that ideal but for every place we deviate we have to have adequate justification and provide work arounds to ensure compatibility with more esoteric toolchains like IAR, Green Hills INTEGRITY, VxWorks, Deos, FreeRTOS, etc.

Deeper Discussion

Here's an example of my assertion that we have to provide C++14 as minimal support:

https://www.ghs.com/news/20180227_ew_compiler2018.html

In 2018, Green Hills first added support for C++14. While it is now four years later the aviation industry moves slowly to acquire technology and uses this technology for long stretches of time. It is possible that aviation projects, still years from completion today, are using a 2018 version of INTEGRITY and will continue to do so for the lifespan of the airframe which, as we know, can span decades.

While we have added support for variant we did so because it could be trivially emulated in C++14. Similarly, our variable_length_array utility provides a consistent API regardless of exception handling in less than 500 lines of code (and far fewer Effective-Software-Lines-Of-Code). I did not expect to add any more fancy utilities after variant and hope we can build serialization logic that is primitive and dependency free even if it doesn't use the shiny new concepts available in C++20. This should be achievable since applications aren't burdened by any ugly internals we come up with as they depend only with the datatypes and the serialization APIs. Perhaps, once we have a version that is optimized for simplicity and compatibility, we can add C++20 as a new language type that is targeted at modern C++ systems, whatever those might be.

Post Script

We value the effort and willingness to contribute to this project. I hope you will see this as a challenge rather than an impediment. We ask you to dig deep into the logic and sculpt away everything extraneous from the syntax while still using C++ to an advantage over C. If you succeed you'll understand why the newer constructs were created and won't take them for granted when you have C++20 available as a tool.

src/nunavut/lang/cpp/support/tl_expected.hpp Outdated Show resolved Hide resolved
src/nunavut/lang/cpp/__init__.py Outdated Show resolved Hide resolved
#include "span_lite.hpp"
#include "tl_expected.hpp"
#ifdef __cpp_lib_bit_cast
#include <bit>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is available in C++14. Please omit if not.

Copy link
Contributor Author

@asmfreak asmfreak Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for bitcast.
All of the type punning (used in many places throughout support library), which may be ok (not an expert) in C, is undefined behaviour in C++. Sadly, there is no easy and safe conversion from float/double* or uint32/64_t* to uint8_t*. If I understand correctlly in C++ <20 for integers the only and defined way is to use bit shifts. I'm not really sure about a defined way for floating point numbers.
In C++20, there is std::bitcast to fix this problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the conversion routines defined for the C support library should be UB-safe or nearly so. They do not rely on type punning. The floating-point conversion functions assume that the platform is IEEE 754-compliant, which is verified at compilation time, so I assume that to be safe, too. C++20 really does have many neat features that make one's life easier but as Scott stated above, it is not usable at the moment, so let us please focus on C++14-compatible solutions for now.

@@ -0,0 +1,1945 @@
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is entirely too much code for Nunavut. We should do something more primitive to keep the line count as low as possible.

Copy link
Contributor Author

@asmfreak asmfreak Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use something like this? Is it too big?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you insist and if Scott okays it, we could define a custom implementation of span fine-tuned for our purposes. It is hardly going to take over fifty lines of code so might be acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two things to consider are:

  1. We cannot accept any dead code. All lines of code provided by Nunavut support headers shall be used by some valid DSDL type. If we have a header we want to pull in and we end up only using 10% of it we should modify the header to elide the unneeded parts.
  2. Some certification regimes require exhaustive testing of all code. Any utility that makes it difficult to provide exhaustive tests on a running target is to be avoided. (We are pretty loose, in this constraint, with C++ templates but that is perhaps a tipping point where a project may want to move to C).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll just replace both expected and span with minimalistic replacements, which are much easier to control. Maybe even throw span away and integrate it's functionality into bitspan.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thirtytwobits I'm pretty sure that span is going to be used throughout unless one is not going to serialize/deserialize any DSDL (we already have an option to omit serialization code completely, perhaps it should also control the emission of the span class).

@asmfreak can we not define expected simply as using Expected = variant<Success, Error>;? I know that it lacks the sugar but see above.

Copy link
Contributor Author

@asmfreak asmfreak Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit wasteful, plus we don't actually have variant in C++14. We are generating the type for it. And since we don't need that many Result types (VoidResult, Result with bitspan, Result with number of bytes in it) we can optimize the tag away, while maintaining a nice interface.
In case of the void result, we just check if the value is zero.
In case of result with number of bytes in it, we can use MSB of size_t to store status. On 32 bit platform it gives us 2**31 - 2 GB - of max message size. I don't think, it is practically viable to send messages that big. Practically speaking, we are bound by bits/bytes conversion on 32 bit platforms - we can only (de)serialize messages of 2**29 bytes long - 3 bits worth of address (shift) is lost in bit offset.
Result with bitspan can also salvage some bits from bitspan, which can be used as error flag. And all three types can be trivially implemented in just 10 lines of code. And all of them will be essentially zero-cost.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be too keen to dig deep into bit-level optimizations. Let us please avoid that lest we end up with an assembly-style mindset where performance comes above all. I do understand the motivation but often performance-driven design comes at the cost of functional correctness: either because it nudges one to cut corners or because the implementation becomes unnecessarily complicated, potentially causing defects to creep in. What we should be focused on is an implementation that is 1. minimalistic (see rationale provided by Scott) and 2. trivial (this means that bit splitting is probably a no-go).

@pavel-kirienko
Copy link
Member

@asmfreak maybe for testing purposes it's better to rely on the C serialization as a reference implementation instead of copy-pasting the existing test cases? This way it should be easier to implement and require less effort to maintain.

@pavel-kirienko
Copy link
Member

You force-push a lot. This is okay but not very useful because we use squash merging -- all your commits will be rewritten into one regardless of the pre-merge history.

@asmfreak
Copy link
Contributor Author

asmfreak commented Mar 3, 2022

I wanted to clean out vendored libraries and used git filter-repo on my branch, hence the new commits.

it's better to rely on the C serialization as a reference implementation instead of copy-pasting the existing test cases?'

@pavel-kirienko I'd like to do that. I wanted to add back-and-forth serialization-deserialization through C implementation like the one with regulated.basics.Primitive.0.1.

@asmfreak
Copy link
Contributor Author

asmfreak commented Mar 3, 2022

Can we maybe merge this PR, if @thirtytwobits sees it as ok for a proof-of-concept? And I can continue to work on other tasks (see the initial message of this PR) in smaller PRs?

@thirtytwobits
Copy link
Member

@asmfreak , I'm totally good with that. This is why we have the "experimental languages" flag, to keep this work from building up in forks before it gets finalized. I just need you to bump the patch number in version.py if you don't mind. I haven't had time to create a proper "don't publish" workflow for commits.

@asmfreak
Copy link
Contributor Author

asmfreak commented Mar 4, 2022

Do you want me to add support for something like bumpversion?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.6% 80.6% Coverage
0.0% 0.0% Duplication

@thirtytwobits thirtytwobits merged commit 89e2f20 into OpenCyphal:main Mar 4, 2022
@thirtytwobits
Copy link
Member

Do you want me to add support for something like bumpversion?

Is this something that would automatically update the version number with each commit? If so then yes please.

This was referenced Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants