OSAL does not build cleanly with conversion warnings enabled

[jphickey]

Is your feature request related to a problem? Please describe. OSAL does not build cleanly if -Wconversion warnings are enabled. In the CFE builds this warning is neither enabled nor disabled so it is left with the compiler default. Most gcc cross toolchains currently used disable it by default, but with the toolchain provided in VxWorks 7 this is enabled by default, so this difference becomes apparent.

Describe the solution you'd like First needs CCB discussion as to whether we want to be "conversion clean".

Downside is that it requires a bunch of extra type casting for things that would normally work implicitly without issue - which makes code ugly - and the casts can become outtdated/stale if the underlying type changes and that doesn't always generate a compiler message but can cause problems due to multiple conversions and/or changing for equality of wrong types. So unnecessary extra type casts can be a real risk to behavior, not just readability.

Upside is that every now and then it will identify a truncation or sign conversion issue that might be a real problem.

Once decided one way or the other, we should explicitly set the -Wconversion or -Wno-conversion setting in CFE so that it is consistent and not dependent on compiler default.

Additional context Originally identified in #599 - split to separate issue (not limited to just VxWorks 7 - that's just what brought it up)

Requester Info Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Ping @klystron78

[jphickey]

Marked as topic for CCB discussion. Thus far we have not built any platform with -Wconversion out of the box. Need to decide whether we support this or not.

[skliper]

There has also been stakeholder interest, in that some coding standards have a rule that is satisfied by supporting it. @tngo67

[jphickey]

For discussion point - and one reason I recommend against sprinkling casts everywhere - consider this code. Note this does compile cleanly using -Wconversion. It's not exactly the same issue but a good example of how forcing the type can sometimes actually break the code.

int main(int argc, char *argv[])
{
    int32_t value = -1;

    if (value == 0xFFFFFFFF)
    {
        printf("Test = true\n");
    }
    else
    {
        printf("Test = false\n");
    }

    if (value == 0xFFFFFFFFL)
    {
        printf("TestL = true\n");
    }
    else
    {
        printf("TestL = false\n");
    }

    if (value == 0xFFFFFFFFUL)
    {
        printf("TestUL = true\n");
    }
    else
    {
        printf("TestUL = false\n");
    }

    return EXIT_SUCCESS;
}

When run on a 32-bit x86 CPU one gets:

Test = true
TestL = true
TestUL = true

But the same code run on a 64-bit x86 CPU one gets:

Test = true
TestL = false
TestUL = false

So - only the first one - with NO forced types, instead relying on the intentional implicit type promotion rules documented in the C standard - works consistently across platforms. Where the type was forced the comparison result is inconsistent.

[astrogeco]

CCB 2020-12-09

• Customer also interested in addressing this
• We should explicitly specify the flags and not be at the mercy of the compiler
• Can we get a count of how many changes we'd have to make?
• IDEA: add clang tests to continuous integration

[skliper]

Warning counts when using -Wconversion: Linux = 401 VxWorks = 242

[skliper]

Discussed w/ stakeholder and the consensus was to continue historical practice of spot checking with this flag to identify/resolve real issues, but we are not required to compile cleanly with this flag enabled. To close this out we can explicitly add -Wno-conversion, and include the review of warnings with it enabled as part of the development/release process.