Compiler optimization removes normalization for Cdr::serialize(bool)

[mirkomorati]

After way too long debugging a Fast-DDS subscriber that kept returning RETCODE_NO_DATA, I traced the problem back to assigning an uninitialized bool field to the published data type. The value had garbage in memory, and in a release build that garbage byte ends up written verbatim onto the wire.

I think that the issue is in Cdr::serialize(bool), the normalization step gets optimized away by the compiler in release builds.

Looking at the source:

Fast-CDR/src/cpp/Cdr.cpp

Lines 816 to 837 in a9bd70c

	Cdr& Cdr::serialize(
	const bool bool_t)
	{
	if (((end_ - offset_) >= sizeof(uint8_t)) || resize(sizeof(uint8_t)))
	{
	// Save last datasize.
	last_data_size_ = sizeof(uint8_t);
	if (bool_t)
	{
	offset_++ << static_cast<uint8_t>(1);
	}
	else
	{
	offset_++ << static_cast<uint8_t>(0);
	}
	return *this;
	}
	throw NotEnoughMemoryException(NotEnoughMemoryException::NOT_ENOUGH_MEMORY_MESSAGE_DEFAULT);
	}

The release build compiles it down to a single mov %bl, (%rax).
This is the release version decompiled for reference:

/* eprosima::fastcdr::Cdr::serialize(bool) */

Cdr * __thiscall eprosima::fastcdr::Cdr::serialize(Cdr *this,bool bool_t)

{
  char cVar1;
  long lVar2;

  // Omit exception handling

  *(undefined8 *)(this + 0x88) = 1;
  *(long *)(this + 0x98) = lVar2 + 1;
  *(bool *)lVar2 = bool_t; // Here we assign directly bool_t
  return this;
}

I know this is technically UB on my side, but right now that check is effectively a no-op.

This looks like the same issue addressed in #155, is there a better way to approach it?

I thought about introducing a function like the following:

bool force_bool(uint8_t value) {
    return !!value;
}

Then the serialize method would be:

Cdr& Cdr::serialize(
        const bool bool_t)
{
    if (((end_ - offset_) >= sizeof(uint8_t)) || resize(sizeof(uint8_t)))
    {
        // Save last datasize.
        last_data_size_ = sizeof(uint8_t);

        offset_++ << force_bool(static_cast<uint8_t>(bool_t));

        return *this;
    }

    throw NotEnoughMemoryException(NotEnoughMemoryException::NOT_ENOUGH_MEMORY_MESSAGE_DEFAULT);
}

This compiles to the following (as before, I'm showing the decompilation from the library compiled in release mode):

/* eprosima::fastcdr::force_bool(unsigned char) */

bool eprosima::fastcdr::force_bool(uchar param_1)

{
  return param_1 != '\0';
}

/* eprosima::fastcdr::Cdr::serialize(bool) */

Cdr * __thiscall eprosima::fastcdr::Cdr::serialize(Cdr *this,bool bool_t)

{
  undefined1 uVar1;
  char cVar2;
  undefined1 *puVar3;
  
  // Omit exception handling

  *(undefined8 *)(this + 0x88) = 1;
  *(undefined1 **)(this + 0x98) = puVar3 + 1;
  uVar1 = force_bool(bool_t);
  *puVar3 = uVar1;
  return this;
}

---

Version used:

• Fast-CDR v2.3.0
• GCC 11.5.0

[MiguelCompany]

How about changing if (bool_t) into if (!!bool_t) ?

[mirkomorati]

How about changing if (bool_t) into if (!!bool_t) ?

Tried it, the compiler still optimizes the function and assigns bool_t directly.
Also if force_bool is defined as inline the compiler can optimize the call away.

[MiguelCompany]

if (static_cast<uint8_t>(bool_t) != 0x00) ?

[mirkomorati]

Still getting the same assembly.

[MiguelCompany]

This is the best I've found playing in godbolt.org:

    volatile uint8_t n = static_cast<uint8_t>(bool_t);
    if (n != 0)

That said, it seems logical that the compiler assumes that a bool is correctly initialized,

§4.7/4 from the C++ 11 or 14 Standard, §7.8/4 from the C++ 17 Standard, §7.3.9/2 from the 20 Standard says (Integral Conversion)

If the source type is bool, the value false is converted to zero and the value true is converted to one.

[mirkomorati]

That said, it seems logical that the compiler assumes that a bool is correctly initialized

Yeah, I can't blame the compiler for doing its job 😄 That's totally a fault on my side for sneaking UB in the code (if you're wondering yes, -fsanitize=undefined does pick up the problem).

Still if the function needs to provide strong guarantees, the current behavior is not working as expected.

What about this instead of introducing volatile? The compiler optimizes std::memcpy out but it cannot make assumptions on the uint8_t, so it has to test the value.

uint8_t raw_bool_t;
std::memcpy(&raw_bool_t, &bool_t, 1);
offset_++ << (raw_bool_t != 0u ? uint8_t{1} : uint8_t{0});

Assembly diff

libfastcdr.so.2.3.0:     file format elf64-x86-64             | libfastcdr.so.2.3.0-fix:     file format elf64-x86-64
    afcc:       49 c7 84 24 88 00 00    movq   $0x1,0x88(%r12 |     afcc:       84 db                   test   %bl,%bl
    afd3:       00 01 00 00 00                                |     afce:       48 8d 50 01             lea    0x1(%rax),%rdx
    afd8:       48 8d 50 01             lea    0x1(%rax),%rdx |     afd2:       49 c7 84 24 88 00 00    movq   $0x1,0x88(%r12
    afdc:       49 89 94 24 98 00 00    mov    %rdx,0x98(%r12 |     afd9:       00 01 00 00 00 
    afe3:       00                                            |     afde:       49 89 94 24 98 00 00    mov    %rdx,0x98(%r12
    afe4:       88 18                   mov    %bl,(%rax)     |     afe5:       00 
    afe6:       48 83 c4 08             add    $0x8,%rsp      |     afe6:       0f 95 00                setne  (%rax)
    afea:       4c 89 e0                mov    %r12,%rax      |     afe9:       48 83 c4 08             add    $0x8,%rsp
    afed:       5b                      pop    %rbx           |     afed:       4c 89 e0                mov    %r12,%rax
    afee:       41 5c                   pop    %r12           |     aff0:       5b                      pop    %rbx
    aff0:       c3                      retq                  |     aff1:       41 5c                   pop    %r12
    aff1:       0f 1f 80 00 00 00 00    nopl   0x0(%rax)      |     aff3:       c3                      retq   
                                                              >     aff4:       0f 1f 40 00             nopl   0x0(%rax)

[MiguelCompany]

This might be the case for GCC 11.5, but in the latest one (GCC 16.1), it does the full optimization and just copies the input untouched.

We should also consider testing with other architectures, like ARM.

Link to my Compiler Explorer playground: https://godbolt.org/z/b1Ehf9nGv

[MiguelCompany]

The following shows interesting results:

    volatile uint8_t* raw_bool_t = reinterpret_cast<volatile uint8_t*>(&bool_t);
    if (*raw_bool_t != 0)

GCC 11.5

f(bool):
        xor     eax, eax
        test    dil, dil
        setne   al
        ret

GCC 16.1

f(bool):
        mov     BYTE PTR [rsp-1], dil
        xor     eax, eax
        cmp     BYTE PTR [rsp-1], 0
        setne   al
        ret

[mirkomorati]

You're right, I was considering only my use case.
I feel like we are reaching for some equivalent of the DoNotOptimize function as in Google Benchmark. I tried writing a launder_bool function that uses it, here is the compiler explorer: https://godbolt.org/z/v9o8vd8Yo
Seems to me the compiler is able to optimize better, avoiding themov as in the GCC 16 case.

I wonder if this normalization is worth paying for unconditionally in a release build. A well-behaved user will only pay the (small) price. Maybe keeping it behind a FASTCDR_STRICT_BOOL compiler flag might be a good idea?

[MiguelCompany]

Nice trick, but it fails to build as-is in MSVC. Including some missing parts from the Google Benchmark code you reference should do.

I agree to having a compiler flag (and cmake option) that allows for the optimization to be an opt-in.

[mirkomorati]

Yes I naively avoided adding the MSVC part in the godbolt example. If you have suggestions on where to add the to-be-adapted code, and if you don't mind, I would like to work on the PR with the changes.
Do you think that it would be a good idea to also add a test case for the flag usage?

[MiguelCompany]

I would add a helpers folder under src/cpp for the DoNotOptimize helper.

Please check whether serialize_array also needs to be changed.

It would definitely be a good idea to have test cases for both.