CDRIVER-3632 Address pedantic warnings for function pointer conversions

[eramongodb]

Conversion between object pointers and function pointers is undefined behavior, though permitted as a "common extension" by the C standard (see N1570 J.5.7 "Function pointer casts").

This commit defines a new TestSuite_AddFullWithTestFn macro as a convenient utility that defines an intermediate TestFuncCtx object through which a given function pointer is passed via the ctx parameter without incurring function pointer <-> object pointer conversions.

---

Converting between object pointers and function pointers is undefined behavior per N1570 (C11 standard draft). It is however specified as a "common extension". As also noted by POSIX.1-2008:

Note that conversion from a void * pointer to a function pointer as in:

    fptr = (int (*)(int))dlsym(handle, "my_function");

is not defined by the ISO C standard.

This PR introduces a new TestSuite_AddFullWithTestFn macro as a conforming workaround. This utility stores the provided test function pointer _test_fn in an intermediate statically-allocated TestFnCtx object. A pointer to this intermediate object is then passed as the ctx parameter to _TestSuite_AddFull instead of the original test function pointer:

{
   static TestFnCtx ctx; // Note: variable is scoped to avoid local conflicts.
   ctx.test_fn = _test_fn;
   _TestSuite_AddFull (_suite, _name, _func, _dtor, &ctx, __VA_ARGS__, NULL);
}

An alternative solution that was considered is dynamically allocating the TestFuncCtx objects instead:

TestFnCtx *ctx = malloc (sizeof (TestFnCtx));

ctx->test_fn = (TestFunc) (_test_fn);
ctx->dtor = _dtor;

_TestSuite_AddFull (_suite, _name, _func, _TestSuite_TestFnCtxDtor, ctx, __VA_ARGS__, NULL);

This would necessitate an additional data member and function to correctly invoke the user-provided _dtor function when provided and free the TestFnCtx object:

void _TestSuite_TestFnCtxDtor (void *ctx) {
   TestFuncDtor dtor = ((TestFnCtx *) ctx)->dtor;
   if (dtor) { dtor (ctx); }
   free (ctx);
}

The binary size of test-libmongoc when built with the static solution vs. dynamic solution does not seem to make a compelling argument to favor one solution over the other. Some numbers when building on WSL Ubuntu 20.04 x86_64 using CMake-provided (version 3.16.3) default compiler arguments for Debug and Release mode:

Size (Bytes)	Debug (GCC 9)	Release (GCC 9)	Debug (Clang 10)	Release (Clang 10)
Static	8,666,040	3,585,632	8,063,480	3,253,640
Dynamic	8,666,680	3,586,520	8,062,272	3,252,328

Therefore I am in favor of the simpler "static" solution as currently proposed over the alternative "dynamic" solution.

[rcsanchez97]

LGTM!

Thank for figuring out how to address the last few warnings that I didn't manage to get to. I appreciate learning this interesting bit about how function pointers work.

[kevinAlbs]

LGTM. I have a nit question about the name TestSuite_AddFullWithTestFn, but nothing to block merging. If you have a counter proposal, feel free to re-request review.

This makes reading the output of compiles much easier!

Converting between object pointers and function pointers is undefined behavior per N1570 (C11 standard draft). It is however specified as a "common extension". As also noted by POSIX.1-2008:

Note that conversion from a void * pointer to a function pointer as in:

    fptr = (int (*)(int))dlsym(handle, "my_function");

is not defined by the ISO C standard.

Thank you for the references. I had not realized use of dlsym requires an undefined cast.

The binary size of test-libmongoc when built with the static solution vs. dynamic solution does not seem to make a compelling argument to favor one solution over the other. Some numbers when building on WSL Ubuntu 20.04 x86_64 using CMake-provided (version 3.16.3) default compiler arguments for Debug and Release mode:

Size (Bytes) Debug (GCC 9) Release (GCC 9) Debug (Clang 10) Release (Clang 10)
Static 8,666,040 3,585,632 8,063,480 3,253,640
Dynamic 8,666,680 3,586,520 8,062,272 3,252,328
Therefore I am in favor of the simpler "static" solution as currently proposed over the alternative "dynamic" solution.

Oh very cool! Thank you for the thorough investigation. For some context, AFAIK the binary size of test-libmongoc (or libbson / libmongoc) has not been a concern when evaluating changes. I see value in tracking the size of libbson / libmongoc since it would directly impact users. I have not seen user reports about increase of binary size.

[kevinAlbs]

Oops, I spoke too soon before checking the attached Evergreen tasks. Can you investigate the ASAN related failures. They appear related to these changes: https://spruce.mongodb.com/task/mongo_c_driver_asan_ubuntu_test_asan_latest_sharded_auth_nosasl_openssl_patch_d2838038aa7d1808b872ea7bd39df33db5bba364_611adc29a4cf4779ee240418_21_08_16_21_44_11/logs?execution=0

[eramongodb]

Preliminary results of Evergreen tests seem to suggest the relevant ASan test failures have been addressed. The failures were identified as due to a missing cast in a test now registered with TestSuite_AddFullWithTestFn.

[kevinAlbs]

LGTM! Offline we looked through test failures that and believe that these are not introduced by changes in this PR related to CDRIVER-4111 and CDRIVER-4134.

@eramongodb is rerunning tests that are believed to be flaky; if the result of that uncovers anything that requires additional changes, feel free to re-request review from me.

[eramongodb]

Rerun of failed tests appears to have flushed out unexpected test failures relative to base commit, confirming they were indeed flaky.