Incorrect assumption for enum size for STM32 platform

[Rendagar]

Intro

I believe I found a bug regarding enums in the current generator for my specific configuration. If I understand the code correctly it assumes that the size of an enum is always 4 bytes, which is not the case. I'll elaborate on my investigation below.

Platform

Some information regarding the platform I'm using.

Platform	STM32H742
Compiler	arm-none-eabi-gcc
Optimization	-Os
Client DDS (on STM)	Micro-XRCE-dds (v2.0.1)
Host DDS (Linux)	FastDDS (v2.5.1)

My investigation

I have a project which was running fine until we changed an IDL file and re-generated the code for it. The original code was generated quite some time ago so the code changed quite a bit.

The symptom we found was that suddenly a call to uxr_prepare_output_stream was failing to allocate a buffer (that is what I understood a return value of 0 means according to this documentation).

By applying the changes made to the code 1-by-1 I traced the error to the definition of an enum which is used in a dds message. In the new code the enum is defined like this (just an example, not the actual values):

typedef enum MyEnum {
   MY_VALUE
} MyEnum;

While in the original code we had:

enum {
  MY_VALUE
}
typedef uint32_t MyEnum;   // Note: Explicit type of MyEnum is missing in the new code.

Interestingly MyEnum has nothing to do with the output stream that was failing, but that output stream was the first to write after a message containing MyEnum was received.

I found 2 workarounds which lead me to believe that the reception of a topic containing MyEnum is breaking the internal memory management. These are

Workaround 1: Use the old enum definition again

Going back to the typedef uint32_t MyEnum. This change allowed the code to run correctly again.

Workaround 2: Use -fno-short-enums

Adding the -fno-short-enums compiler flag also seems to fix this issue.

Diving into godbolt to investigate seems to confirm that the GCC versions x.y.1 (which have no associated platform) default to a size of 1, unless -fno-short-enums is used (see the comment in the godbolt code for information about the windows). I've also checked with gcc version x.y.0 (linux) to show that when compiling for linux the compiler does indeed default to a size of 4 bytes.

Btw, my assumption that the generator assumes an enum if 4 bytes is due to the following piece of code that microxrceddsgen generated for the topic containing MyEnum.

uint32_t MyState_size_of_topic(const MyState* topic, uint32_t size)
{
    uint32_t previousSize = size;
        size += ucdr_alignment(size, 4) + 4;

    return size - previousSize;
}

[pablogs9]

Hello @Rendagar:

• Could you share your IDL file so I can replicate it?
• Which is the previous (the one that works) and the newer version?

[Rendagar]

Sorry @pablogs9 , I completely missed your response. I can't share the original IDL's but I've stripped 1 of the definitions that is causing problems down to the absolute minimum.

Note 1: the number of STATE_'s in MyEnum was chosen arbitrarily, having more of less makes no difference.

Note 2: The message on which uxr_prepare_output_stream fails (immediately after receiving a message containing MyState) does not seem to matter.

enum MyEnum {
  STATE_A,
  STATE_B,
  STATE_C
};

struct MyState {
  MyEnum state;
};

So with the current generator (where things go wrong) this results in the following code in the .h file.

typedef enum MyEnum
{
    STATE_A,
    STATE_B,
    STATE_C
} MyEnum;

typedef struct MyState
{
    MyEnum state;
} MyState;

While previously the following code was used (which works)

enum
{
    STATE_A,
    STATE_B,
    STATE_C
};
typedef uint32_t MyEnum;

typedef struct MyState
{
    MyEnum state;
} MyState;

[pablogs9]

Do you have the version number of the old version and the new version?

I'm gonna try to replicate it.

[Rendagar]

Ah yes, that would have been smart of me to add :facepalm:

The version I'm currently using is:

$ git show
commit b4190682216992eaf1f9b99fc4b54a21c72847ff (HEAD -> master, tag: v2.0.1, origin/master, origin/HEAD)
Merge: 29c0b2c 2b68016
Author: Antonio Cuadros <49162117+Acuadros95@users.noreply.github.com>
Date:   Mon Jul 10 11:29:12 2023 +0200

    Release v2.0.1

and it is build on the following platform

$ uname -a
Linux ####### 5.15.0-84-generic #93~20.04.1-Ubuntu SMP Wed Sep 6 16:15:40 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

The previous version is a bit more difficult to track down. I asked people working on the project at that time. Based on how long ago it is it is likely that this would have been v1.1.0 or earlier. Looking at the commit logs it must have been before commit b67aceb90.

[pablogs9]

Here my found:

1. Enum support were introduced in https://github.com/eProsima/Micro-XRCE-DDS-Gen/pull/43
2. Before this PR Micro XRCE-Gen does not generate any code for Enums, it was reported here: https://github.com/eProsima/Micro-XRCE-DDS-Gen/issues/37
   • This leads me to the following question: is there any possibility that your team modified the generated sources to include this Enum support for your use case?
3. Check if this solves the issue: https://github.com/eProsima/Micro-XRCE-DDS-Gen/pull/69

[Rendagar]

Yes the types where manually added at that time. As where ucdr_serialize_X() and ucdr_deserialize_X() functions that where also not generated yet a that time.

As of that commit enums can indeed be generated by the tool and manual intervention is no longer required, I don't think there is a problem there. The problem seems to me (as demo'd in the godbolt page I linked) that the serializer and deserializer generated assume an enum value is 4 bytes (which is true for most platforms) but that for the gcc compiler with no specific platform the enum defaults to 1 byte.

[pablogs9]

The problem seems to me (as demo'd in the godbolt page I linked) that the serializer and deserializer generated assume an enum value is 4 bytes (which is true for most platforms) but that for the gcc compiler with no specific platform the enum defaults to 1 byte.

Sure, but with https://github.com/eProsima/Micro-XRCE-DDS-Gen/pull/69 the enum members of the generated structures are forced to 4 Bytes (as CDR standard determines) independently of the compiler.

Could test that it solves your issue?

[Rendagar]

Oh that's quick. Currently I'm working from home and don't have a test setup available but I'll get back to you when I have had a chance to check this at work.

[Rendagar]

I've found some time to check the changes and this indeed seems to solve the crash I had :+1:

@pablogs9 I don't know if you are also looking for feedback on the change but I was wondering if it is not neater (introduces less indirection) to use something like uint32_t $member.name$; on line 125 instead of creating an in-between type with _cdr added? As there no direct link (for the compiler at least) between the enum and it's use in a struct anyway I'm not sure what the additional typedef is adding.

[pablogs9]

Thanks for testing the change!

I've just merged https://github.com/eProsima/Micro-XRCE-DDS-Gen/pull/69, so we can close this issue.

Do not hesitate to reopen if you have any other issues.