Fix #1189, correct compiler warnings

[jphickey]

Describe the contribution
Corrects some (false) warnings about use before init.
Also corrects an alignment warning and removes unneeded union type (union type is not the best way to deal with this situation).

Fixes #1189

Testing performed
Build and sanity check CFE for Raspberry Pi (older GCC w/strict alignment) and native (64-bit)
Run all unit tests

Expected behavior changes
None, just compiler warning fixes

System(s) tested on
Ubuntu 20.04 (native)
Raspbian (cross compiled; gcc 4.9.3)

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Also confirmed that this builds cleanly on CentOS 7.9 with "gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)" and BUILDTYPE=release

[jphickey]

Suggesting fast track as it deals with warnings currently in main branch....

[skliper]

What about having CFE_SB_TransmitMsgValidate actually set RouteId (actually done by the underlying GetRouteId call)? Then it fixes everywhere the GetRouteId call is used.

[jphickey]

We can do that too, but I see it as a separate problem ...

1. A caller should never rely on/use output values from a function that returned non-success
2. A function should always set its output buffers to something, even when it returns non-success

I see (1) as the main issue, and (2) as the backup/failsafe. I have no objection to doing both, I just see it as a secondary concern. I can add it though.

[skliper]

In virtually every case I've seen that the static analyzer or compiler warning for possible uninitialized variable comes up 1 is met by the caller by either returning or checking status prior to using what is returned. 2 then is both good practices and squashes the warning. Is initializing the variable in this case really required where 1 is already satisfied by logic?

This seems even more useless when the value being initialized is not checked explicitly before used, and would cascade into other issues down the line if there WAS a possible way to get to that code. Basically, if you are checking for status == SUCCESS but depend on RouteId being valid why not check instead (or in addition if both cases need to be true) for RouteId being valid (that also squashes the warning).

[skliper]

Note for the case where the code IS checking for RouteId to be valid, I definitely agree explicit initialization is the way to go. I just struggle with justifying initialization if it's never accessed based on failure logic. Feels cleaner to me (and would be easier for me to maintain) when initialization is done locally when it matters (and squash errors via setting outputs in the underlying APIs when it doesn't matter).

[jphickey]

@skliper I added this in commit bf8f481 however I have my concerns/reservations - there is a chance that the original passed-in buffers themselves are NULL, where the status code becomes CFE_SB_BAD_ARGUMENT ... this will then try to write a value to the buffer. Could add NULL checks on each, but then this is just getting overly complicated at that point.

[skliper]

Or if:

cFE/fsw/cfe-core/src/sb/cfe_sb_api.c

Lines 1536 to 1541 in bf8f481

	/* check input parameter */
	if (MsgPtr == NULL)
	{
	PendingEventID = CFE_SB_SEND_BAD_ARG_EID;
	Status = CFE_SB_BAD_ARGUMENT;
	}

Is really the sweet spot for checking, why not just add the else and set the outputs to defaults here instead of at the end? Then it avoids setting them for the CFE_SB_BAD_ARGUMENT case.

[jphickey]

I mean, anything is possible, if you want to do that. Each output could be individually checked for NULL and assigned a default if not null. If it helps meet a checkbox on some review procedure, that's well and good - but it doesn't make sense in the overall design of the software, it is just unnecessary extra complexity for no actual benefit (and cpu cycles burnt in code that actually does execute rather frequently in CFE)

I'm just taking a more holistic view of the software. This is an internal helper function, called only from the two public "TransmitMsg" variants, in a specific scenario, to consolidate the common logic.

The only reason that MsgPtr is explicitly checked is because of the fact that (for some reason?) public APIs are all supposed to accept and detect a NULL pointer input. This assumes the role of checking that input.

For the other 3 - those are outputs to stack variables. They are stack variables in the parent - never NULL. This is not a public API - they aren't going to be randomly called with arbitrary values in a general-purpose sense. Again, the role of this function is to consolidate common code between CFE_SB_TransmitMsg and CFE_SB_TransmitBuffer, initializing local stack variables on behalf of the caller, such that the code is not repeated between the two.

By definition, any NULL pointer check here is impossible to get at runtime - thereby making it dead code. The way I see it this is very similar to the recent case in OSAL where a check for an impossible condition was removed due to the code before it. This is the exact same thing, just in a helper function. The helper is not invoked with NULL args, so any check for NULL args is immediately dead/unreachable code.

[skliper]

It was you're comment about "there is a chance that the original passed-in buffers themselves are NULL, where the status code becomes CFE_SB_BAD_ARGUMENT" that triggered my comments... so can they be NULL or not? If not, the current implementation is fine (I already approved that one when I first saw it.) If they can be NULL, then check for it. Alternatively, just set the passed in pointers as the else of the MsgPtr check. Are all 3 of these possible approaches really not acceptable and the only option is to not initialize these values? I still don't follow why there is any opposition? Maybe contact me for a chat?

[jphickey]

Sorry about that - that was only my initial knee-jerk before looking closer and seeing that the function really only does need to handle MsgPtr being null and not the others. To allow the others to be null would be pointless.

[skliper]

Copy. And I'm fine w/ how it's currently just checking MsgPtr (probably would benefit from a comment to that affect, but implementation is fine) based on the background you provided.

[jphickey]

Note - although I originally requested a fast track on this one - that was only for the initial commit (54adb9f) that directly fixed the compiler warnings without really changing other code.

Perhaps we should fast track the original change so that main compiles cleanly, then create a new ticket to argue over the virtues (or not) of adding complexity to internal helper functions?

[jphickey]

I reverted this PR back to its original change - just the real warning fixes - no change to actual implementation of CFE_SB_TransmitMsgValidate().

I recommend merging this as-is - it fixes the warning in #1189 without breaking anything and has minimal impact.

I created a new issue #1198 to further the discussion about whether internal helper functions need to separately validate their inputs (e.g. handle NULLs) or force outputs. This is a more complex topic and probably not something to implement in a fast bug fix.

[astrogeco]

CCB 2021-03-03 APPROVED

• Learned about "best practices" for aliasing and alignment rules and their effects on compiler warnings

[jphickey]

For reference here is the link to stack overflow discussion on the topic: https://stackoverflow.com/questions/66427774/best-practices-for-object-oriented-patterns-with-strict-aliasing-and-strict-alig

The key point is that the union-of-pointers or pointer-to-pointer approach violates strict aliasing not on the data itself but the pointers. A derived struct may alias a base struct and vice versa, but pointers to those structs are a distinct type and the aliasing rules apply to them too. When the compiler complained of strict aliasing rule violations, it wasn't about the data object, it was about the pointer to the data object.