Fix 4320: add an optional argument list string to the configure CheckFunc method

[jcbrill]

Add an optional argument list string to the configure CheckFunc method so that the generated function argument list matches the function's prototype when including a header file.

Fixes #4320.

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated CHANGES.txt (and read the README.rst)
• I have updated the appropriate documentation

[jcbrill]

@mwichmann documentation note fragment from #4320

If you include a header which provides a prototype declaration for the function being checked, the compiler may perform a more rigorous type check to match the prototype. If this causes unexpected failures, use funcargs to supply arguments to match the prototype.

Tests test/Configure/config-h.py and SCons/SConfTests.py pass in the following configurations:

• Windows 10 64-bit (AMD64), VS2022 [PC]
• Windows 11 64-bit (ARM64), VS2022 [PC]
• Windows 10 32-bit (x86), VS2019 [VM]
• Ubuntu 64-bit (AMD64) [WSL2]

[bdbaddog]

Ideally we'd add a test which actually populates funcargs.

Also the docs for CheckFunc() will need to be updated.
And lastly a blurb in CHANGES.txt/ RELEASE.txt

[mwichmann]

ah... moving slowly here. I thought this already existed, just wasn't being called.

[jcbrill]

Ideally we'd add a test which actually populates funcargs.

This was added to SConfTests.py which changes the prototype and populates funcargs:

 r = sconf.CheckFunc('strcpy', header='/* header */ char *strcpy(char *dest, char *src);', funcargs='"", ""')
 assert r, "did not find strcpy"

[jcbrill]

ah... moving slowly here. I thought this already existed, just wasn't being called.

Nope. Just trying to sneak a simple solution under the radar.

[bdbaddog]

empty string funcargs != populates IMHO.. ;)

[jcbrill]

'"", ""' != empty string:

/* header */ char *strcpy(char *dest, char *src);
...
strcpy("", "");

[bdbaddog]

ok. Needs doc update, CHANGES, RELEASE updates. Then it should be mergable.

[bdbaddog]

I've added a blurb for CHANGES.txt, RELEASE.txt and updated the manpage entry for CheckFunc()

[mwichmann]

This addition should go after the existing example (and maybe wants its own example?). Also notice that further down it basically says "don't do that" about what this change is enabling (that is, including a standard header). We'll want to think about what that should say now. Also include a version notice.

[bdbaddog]

Updated text. Please take another pass at this?

[mwichmann]

Not seeing anything different. Is github interface messing up again?

[jcbrill]

The test case was modified to do exactly what the documentation note says should not be done with the header argument:

Note: if header is supplied, it should not include the standard header file that declares function_name, and it should include a dummy prototype similar to the default case. Compilers reject builds where a function call does not match the declared prototype as happens if the "real" header is included, and modern compilers are now rejecting implicit function declarations.

With modern compilers/preprocessors (inline function declarations, rewritting/remapping declarations), I'm not sure how the stupid simple case for "printf" could be resolved without including the actual header.

The symbol for "printf" is only exported for one target platform (amd64). However, the source code for other targets (arm64, x86) certainly allows the "printf" symbol to be resolved in the source code by remapping to another function and not exporting "printf".

What exactly is the intent and semantics of what configure's CheckFunc is attempting to achieve?

1. that the function symbol can be imported from the library (e.g., winapi GetProcAddress), or
2. that the symbol can be safely used in source code.

The msvc "printf" satisfies both 1 and 2 only for the the amd64 dlls and satisfies only 2 for the other platform dlls.

The documentation note seems to imply 1 above. If that is the case, perhaps the test for "printf" should be removed or another stdlib function chosen that is likely to always be exported.

It seems far fetched in practice that a stdio function would be used without actually including the appropriate header file.

Either way, it doesn't hurt to have the ability to modify the generated signature.

[mwichmann]

The point was to remember to update the docs, not walk back on the new functionality. Don't want to have the case where it says Do Not Do X and then Here's How To Do X.

[mwichmann]

Here's automake on the issue:

If you include any header files, be sure to call the functions relevant to them with the correct number of arguments, even if they are just 0, to avoid compilation errors due to prototypes. GCC version 2 has internal prototypes for several functions that it automatically inlines; for example, memcpy. To avoid errors when checking for them, either pass them the correct number of arguments or redeclare them with a different return type (such as char).

[mwichmann]

What exactly is the intent and semantics of what configure's CheckFunc is attempting to achieve?

1. that the function symbol can be _imported_ from the library (e.g., winapi GetProcAddress), or

2. that the symbol can be safely used in _source code_.

The msvc "printf" satisfies both 1 and 2 only for the the amd64 dlls and satisfies only 2 for the other platform dlls.

The documentation note seems to imply 1 above. If that is the case, perhaps the test for "printf" should be removed or another stdlib function chosen that is likely to always be exported.

It seems far fetched in practice that a stdio function would be used without actually including the appropriate header file.

For explanation, these functions mirror things GNU autoconf does, and it's not very sophisticated. We make sure a function "exists" by trying to compile a program using it and not getting a link error (or, for that matter, a compile error); we're not going further than that. It's mostly from the era of not very stable APIs - glibc 2.10 has a new function that 2.9 didn't have, so if it's there use it, else use a local reimplementation (the User Guide has such an example). I don't personally see a lot of cases of needing to do this kind of thing these days, while I do see code that checks for linkable libraries - like you might include a block of code in the program if a particular crypto library is available, etc. It does depend on (potentially) accumulated information - it doesn't ask at all what library you might need to link against the symbol, so earlier CheckLib* calls should have been done, that allows you to accumulate found libraries into LIBS, which will then be used when linking a CheckFunc test.

Sorry for rambling on if all this was clear...

[bdbaddog]

@mwichmann @jcbrill - just pushed updates here which I think cover @mwichmann 's feedback?

[mwichmann]

Having said "I don't see this" I went an looked at a big project, and indeed it does do one of these:

    # Glibc 2.25+, OpenBSD 5.5+ and FreeBSD 11.0+ offer explicit_bzero, a secure way to zero memory
    if conf.CheckFunc('explicit_bzero'):
        conf.env.SetConfigHeaderDefine("CONFIG_HAVE_EXPLICIT_BZERO")

[mwichmann]

Here's a possible alternative (I think it's okay as is, I was just thinking of how I would have explained this... This would combine the two paragraphs into the new single para, and retains some of what was in the note originally.)

<para>
If <parameter>header</parameter> is supplied,
it should <emphasis>not</emphasis>
include the standard header file that declares
<parameter>function_name</parameter>,
and it <emphasis>should</emphasis> include a
dummy prototype similar to the default case.
If this is not possible,
the optional <parameter>funcargs</parameter>
argument can be used to specify a string containing
an argument list with the same number of arguments as
in the prototype (they can just be 0 values or empty strings).
Modern C/C++ compilers reject implicit function declarations,
and may also reject function calls that don't look
like they match the protoype.
</para>

[jcbrill]

I rather like this change.

Perhaps wordsmithing the following:

can be used to specify a string containing
an argument list with the same number of arguments as
in the prototype (they can just be 0 values or empty strings).

To something along the lines of:

can be used to specify a string containing
an argument list with the same number and type of arguments and as
in the prototype (they can simply be constant values of the correct type).

[mwichmann]

I rather like this change.

Perhaps wordsmithing the following:

that's fine by me (although apparently the types don't actually matter).

[jcbrill]

(although apparently the types don't actually matter)

I would not have expected that. If a prototype is present, I would have expected type checking (taking into account some types are automatically coerced) and possible compiler errors rather than linker errors, at least for c.

[jcbrill]

Tests run on ubuntu/wsl2 and windows/pc.

Both of these tests will fail (passing a float where a char* is expected):

r = sconf.CheckFunc('strcpy', header='/* header */ char *strcpy(char *dest, char *src);', funcargs='0.0, ""')
assert r, "type failure prototype"

r = sconf.CheckFunc('strcpy', header='#include <string.h>', funcargs='0.0, ""')
assert r, "type failures include header"

However, both of these tests will pass (passing an int where a char* is expected):

r = sconf.CheckFunc('strcpy', header='/* header */ char *strcpy(char *dest, char *src);', funcargs='0, ""')
assert r, "type failure prototype"

r = sconf.CheckFunc('strcpy', header='#include <string.h>', funcargs='0, ""')
assert r, "type failures include header"

Which is what I now suspect you were alluding to in the statement above.

Zero can used/coerced into integers, floats, and (apparently) addresses which covers pointers and strings.

I wouldn't be surprised if there were warnings generated but am too lazy to test.

Perhaps the original text covers most all scenarios.

Edit:
funcargs='0, 0' (i.e., strcpy(0, 0);) passes as well for both ubuntu/windows.

[mwichmann]

Interested in a small expansion to the unit tests? These all pass for me on Linux but might be pushing the envelope too hard on MSVC, don't know yet (a .txt due to GitHub's asinine assumptions about allowable file suffixes)

moretests.txt

[jcbrill]

Sorry for rambling on if all this was clear...

As always, I appreciate the information.

I'm stuck on viewing the original issue as a "false negative" which was the motivation for the intent question. I need to get over it, bigger fish to fry and all that.

Interested in a small expansion to the unit tests? These all pass for me on Linux but might be pushing the envelope too hard on MSVC, don't know yet (a .txt due to GitHub's asinine assumptions about allowable file suffixes)

Always.

The patch was applied and two tests were run using a minimal test file consisting of:

1. ./test/Configure/config-h.py
2. ./SCons/SConfTests.py

The two tests pass for the following configurations:

• Windows 10 [64-bit amd64 pc], VS2022
• Windows 11 [64-bit ARM64 pc] VS2022
• Windows 10 [32-bit x86 vm] VS2019
• Ubuntu 20.04 [64-bit amd64 wsl2] N/A

Due to time limitations, I have not run the full test suite locally.

Happy to push if desired.

[mwichmann]

Before you push, need to also fix the manpage problem, which is that an added line is not contained as expected:

diff --git a/doc/man/scons.xml b/doc/man/scons.xml
index e2e6a57fb..91e8dd7da 100644
--- a/doc/man/scons.xml
+++ b/doc/man/scons.xml
@@ -3993,7 +3993,9 @@ Note: if <parameter>header</parameter> is supplied,
   by using the <parameter>funcargs</parameter>.
 </para>
 
+<para>
 <emphasis>Changed in version 4.7.0: added the <parameter>funcargs</parameter>.</emphasis>
+</para>
 
 <para>Returns a boolean indicating success or failure.</para>
   </listitem>

[jcbrill]

I believe it was applied as expected. Let me know if it was botched.

[jcbrill]

Man page documentation change modified from @mwichmann suggestion above (plain text):

If header is supplied, it should not include the standard header file that declares function_name and it should include a dummy prototype similar to the default case. If this is not possible, the optional funcargs argument can be used to specify a string containing an argument list with the same number and type of arguments as the prototype. The arguments can simply be constant values of the correct type. Modern C/C++ compilers reject implicit function declarations and may also reject function calls whose arguments are not type compatible with the prototype.

No additional work is planned.