Fix #832, add "handler" feature to utassert stub API

[jphickey]

Describe the contribution
Adds the concept of a "handler" function to UT assert. A handler is basically the custom logic that exists between the hook function and the return to the stub caller. In current UT stubs, this is hard coded, and it generally comprises setting output parameters and translating return values as needed.

This concept adds the basic plumbing to allow the handler to be configured just like a hook function already does. The difference
is that the handler is directly responsible for setting all outputs.

This also includes a script to auto-generate stub functions that match this pattern. Given an API header file, the script extracts
the declarations, and generates a source file with stub definitionsthat rely on a separate handler to deal with the needed outputs.

Lastly, all existing stubs in OSAL will be updated to use the auto-generated stub logic from the script, created directly from the C header. This ensures that stubs will match the FSW implementation. Note - one header in FSW (os-shared-printf.h) was broken into two parts, to improve the ability to do this.

Fixes #832

Testing performed
Build and sanity check CFE
Run all unit tests, ensure all tests pass. Confirm that unmodified coverage test scripts are passing as-is, and coverage is still 100%.

Expected behavior changes
This change only affects coverage testing with stubs, and should be fully backward compatible with existing tests.
Once a stub is regenerated via the auto-generator script:

• Stub is exactly in sync with FSW implementation
• any return type handled (not just int32)
• all args passed through context
• handler can be completely overridden by test case when desired, such that test case can have 100% control over the stub call.

System(s) tested on
Ubuntu 20.04
RTEMS 4.11

Additional context
The "handler" function uses a different hook function prototype than the hook functions do. The latter is kept the same for backward compatibility with existing tests.

The intent is to address a few long-standing shortcomings in the design of the hook function. In particular, hook function design assumes/requires that the function returns int32. While most do, some do not, and this is a major issue if the goal is to completely replace the handler associated with a stub.

In this design there is now a dedicated data buffer to hold the return value, which can be of any data type, and a macro that the handler (or hook) implementation can use to fill/set that return value. However the framework will automatically populate that buffer from the int32 status code if the hook/handler did not populate it, which was what ~90% of stub functions did in their "post-hook" logic. This simplifies things in that those functions don't need any handler at all unless they had other work to do.

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

[jphickey]

This can be reviewed now. For review purposes I split this into multiple commits. The main API addition is in the first commit, which only affects a few files. This should be the first focus of review.

The second commit is the only one that touched FSW, and the goal was to split up "Impl" headers where they were grouped in a problematic way for scripting. It just splits a header file into two.

The remainder of commits focus on fixing/updating all the existing stubs to use this new pattern. Existing "post-hook" logic from the original stub is moved to a default handler function, and the stub itself is now generated directly from the C header using the generator script.

Note that no part of this PR changes a single test case. All changes are transparent to existing tests (migrating the "special sauce" from being in the stub itself into a default handler preserves all existing behavior so all tests still work as they always have).

[jphickey]

Slight addendum to my above statement about not affecting existing tests - there was a direct ref to the "ArgPtr" member of the context in the sample_lib test, which actually did break because auto-generated stub registers this differently. See nasa/sample_lib#60

In reality all hooks should use the provided macro - it is much more robust for several reasons.

[asgibson]

There is a bit to get through here, but I will start on it; reviewing the stub generation is where I will begin.

I do have a couple questions about the Testing performed

Confirm that unmodified coverage test scripts are passing as-is, and coverage is still 100%.

Is that after the change is implemented or before? Were any tests performed to show use cases where the stub logic is completely replaced? I'm sure I will get there at some point, just curious if there are any current results.

[jphickey]

Is that after the change is implemented or before

Both. This is one reason I pushed as separate commits. This way the first commit would also individually have CI run against it to get the green check mark, showing that the first commit alone does pass CI.

Basically you can take only the first commit and run all tests, they still pass, and still show same coverage level. (i.e. with only the new stub API and internal utassert stub impl changes added, but no actual stub changes, nothing breaks). You can then add the remainder of commits which update all the OSAL stubs to use the generated/split (stub+default handler) patterns, and run tests again. Still passes, with same coverage levels.

Were any tests performed to show use cases where the stub logic is completely replaced?

As this being a new feature there are no current test cases in OSAL or CFE that make use of the handler replacement. I did test it by specifically changing an ES test case to call UT_SetHandlerFunction() in order to confirm that this works and the default handler was not used. So I can confirm that the override concept basically works. I was hoping you'd provide some tests that will actually utilize it.

Regarding the generated stubs, they all follow a very basic pattern. Here is a random example:

/*
 * ----------------------------------------------------
 * Generated stub function for OS_BinSemCreate()
 * ----------------------------------------------------
 */
int32 OS_BinSemCreate(osal_id_t *sem_id, const char *sem_name, uint32 sem_initial_value, uint32 options)
{
    UT_GenStub_SetupReturnBuffer(OS_BinSemCreate, int32);

    UT_GenStub_AddParam(OS_BinSemCreate, osal_id_t *, sem_id);
    UT_GenStub_AddParam(OS_BinSemCreate, const char *, sem_name);
    UT_GenStub_AddParam(OS_BinSemCreate, uint32, sem_initial_value);
    UT_GenStub_AddParam(OS_BinSemCreate, uint32, options);

    UT_GenStub_Execute(OS_BinSemCreate, Basic, UT_DefaultHandler_OS_BinSemCreate);

    return UT_GenStub_GetReturnValue(OS_BinSemCreate, int32);
}

The pattern is very simple, with the goal being to:

• capture the details about return value, make a buffer of the correct type/size to hold it
• capture every passed in parameter, along with the name and correct type/size
• execute the hook/handler (as every stub always has)
• return the correct value from the buffer created earlier.

The main benefit of scripting this sort of thing is that every function stub is now exactly the same, no special sauce are added to any of them, and everything is done exactly the same way in all of them, because a script generated them all. It greatly minimizes the risk that any stub becomes "special" in any way - any the special sauce must be put into the handler, not the script-generated stub function. And thus it can be overridden, always.

[asgibson]

A sample from my first run on a cf header:

/*
 * ----------------------------------------------------
 * Generated stub function for CF_Chunks_ComputeGaps()
 * ----------------------------------------------------
 */
uint32 CF_Chunks_ComputeGaps(const chunks_t *chunks, index_t max_gaps,
                             chunk_size_t total, chunk_offset_t start,
                             compute_gap_fn_t compute_gap_fn, void *opaque) {
  UT_GenStub_SetupReturnBuffer(CF_Chunks_ComputeGaps, uint32);

  UT_GenStub_AddParam(CF_Chunks_ComputeGaps, const chunks_t *, chunks);
  UT_GenStub_AddParam(CF_Chunks_ComputeGaps, index_t, max_gaps);
  UT_GenStub_AddParam(CF_Chunks_ComputeGaps, chunk_size_t, total);
  UT_GenStub_AddParam(CF_Chunks_ComputeGaps, chunk_offset_t, start);
  UT_GenStub_AddParam(CF_Chunks_ComputeGaps, compute_gap_fn_t, compute_gap_fn);
  UT_GenStub_AddParam(CF_Chunks_ComputeGaps, void *, opaque);

  UT_GenStub_Execute(CF_Chunks_ComputeGaps, Basic, NULL);

  return UT_GenStub_GetReturnValue(CF_Chunks_ComputeGaps, uint32);
}

Putting those stubs in their place! Ya, Basic!

[asgibson]

Built all cf stub files that have associated headers (looks good!), will move on to trying them out on Monday.

[asgibson]

What if I have functions that do not appear in a header, but are accessed through an extern? (can be done by hand, just curious)

extern int CF_CFDP_RecvMd(transaction_t *t);
extern int CF_CFDP_RecvFd(transaction_t *t);
extern int CF_CFDP_RecvEof(void);
extern int CF_CFDP_RecvAck(void);
extern int CF_CFDP_RecvFin(void);
extern int CF_CFDP_RecvNak(int *num_segment_requests);

[jphickey]

What if I have functions that do not appear in a header, but are accessed through an extern?

This script does assume / require that functions needing stubs are prototyped in a separate header. I believe this is also required by most coding standards (i.e. that any function that serves as an interface between source units should be declared in a separate header file).

Local helper functions are an exception (i.e. functions used only within the same unit) but by definition those shouldn't need stubs, either.

My recommendation is that if you have functions which are defined and one source file and referred to in another, that you move the declaration to a common header (it can be a private header, that's fine, as long as its a header).

[jphickey]

BTW - as far as my previous comment goes - I totally acknowledge that the generator script currently does exactly this (extern right in .c file) for the default handler functions. But since it isn't FSW I was betting on this being OK, since it is simpler to do. It wouldn't be that hard to make it use a separate header though.

[astrogeco]

CCB:2021-04-21 APPROVED with changes

• First commit is the "big one"
• Adds a new "handler" function that is similar to the "hook" function
• Problem with the original design of hooks: they assume a int32 output
• Largely backwards compatible: didn't have to change any unit tests
• Can we rename "expected return" to "expected type"? "Expected return" It is a buffer that creates a buffer to hold the value. The expectation is that the handler will produce "some value" not a specific one. There is no verification on that value itself.

[asgibson]

I have setup and run the same experiment as I did for #839 and all tests completed as expected which verifies that the "do-no-harm" approach is intact; however, there is a caveat here. Current stubs must be transmogrified into the default hook functions if the behaviors they exhibit are to be preserved. The experiment added default hooks for two cFE API functions CFE_MSG_GetSize and CFE_MSG_GetMsgId, but the work was done by hand without any automation. It would seem that having the ability to automate the creation of default hooks from the current stubs will be a necessity.

That being said, the design appears to work well in the small simulation I have run. 14 unit tests were run without any updates on nasa/cFS@c58824d and all the tests performed as expected. cFE was updated with the two stub changes and OSAL was updated to https://github.com/jphickey/osal/tree/fix-832-stub-handler-replacements. Tests were run again, unaltered, and received the same results as before the update (no harm!). The seven experimental tests were then updated to use the new handler functionality while the control tests still used the hook functionality. The 7 experiment tests correctly changed behavior and the 7 control tests stayed static (stub control enabled!).

I will concur with the update once the conflict is resolved.

[jphickey]

Rebased to latest main branch, should merge clean now.