nasa / fpp

F Prime Prime: A modeling language for F Prime
https://fprime.jpl.nasa.gov
Apache License 2.0
49 stars 31 forks source link

Autocoder Generates Incorrect Code #369

Closed LeStarch closed 10 months ago

LeStarch commented 10 months ago

The FPP autocoder generates code that can fail to compile in various scenarios.

  1. Usages of BUILD_UT. BUILD_UT is not guaranteed to be set and thus #if BUILD_UT should be replaced with #if (defined(BUILD_UT) && BUILD_UT)
  2. There is a flawed string import when:
    
    #if FW_SERIALIZABLE_TO_STRING
    #include <Fw/Types/String.hpp>
    #endif
This should read

if FW_SERIALIZABLE_TO_STRING || (defined(BUILD_UT) && BUILD_UT)

include <Fw/Types/String.hpp>

endif


3. `FW_SERIALIZABLE_TO_STRING_BUFFER_SIZE` is only defined by FW_SERIALIZABLE_TO_STRING and not `BUILD_UT` thus its usages are only valid in the when `FW_SERIALIZABLE_TO_STRING=1`

This would all be resolved by: https://github.com/fprime-community/fpp/issues/303, however; this adds the added constraint that projects must set `FW_SERIALIZABLE_TO_STRING` when running UTs.  Doing this is impractical when they wish the release to be without `FW_SERIALIZABLE_TO_STRING` as there is only one configuration file.
bocchino commented 10 months ago

BUILD_UT is not guaranteed to be set and thus #if BUILD_UT should be replaced with #if (defined(BUILD_UT) && BUILD_UT)

If BUILD_UT is not defined, then #if BUILD_UT is compiled as #if 0, isn't it? So what's the problem?

Also: the use of #if X is pervasive in the generated code. If there is something wrong with this pattern then we should reconsider it everywhere, not just for BUILD_UT.

Doing this is impractical when they wish the release to be without FW_SERIALIZABLE_TO_STRING as there is only one configuration file.

Wouldn't #define FW_SERIALIZABLE_TO_STRING BUILD_UT work?

bocchino commented 10 months ago

More generally, I have always wondered why we say #if X instead of #ifdef X for on/off flags. I think #ifdef X is more common. So maybe we should consistently use #ifdef X for binary flags and #if X only where X can have more than two values. Again, this is a pervasive issue in F Prime generated code, not just a BUILD_UT issue.

If we did it this way, then we could write

#ifdef BUILD_UT
#define FW_SERIALIZABLE_TO_STRING
#endif

to turn on serializable strings in UTs only.

LeStarch commented 10 months ago

#if BUILD_UT when BUILD_UT is not defined results in an error BUILD_UT not defined.

bocchino commented 10 months ago

What is causing this error? If I compile the following code

#if BUILD_UT
int x;
#endif

with g++ -c -Wall -Wextra -Wpedantic -Werror --std=c++11 I get no such error.

bocchino commented 10 months ago

This causes the error:

g++ -c -Wundef -Werror

As discussed, the fix is to replace all occurrences of #if BUILD_UT in the generated code with #ifdef BUILD_UT, since BUILD_UT is an on/off switch.

bocchino commented 10 months ago

Closing in favor of #374 and https://github.com/nasa/fprime/issues/2467.