SConsCPPConditionalScanner fails evaluate nested defines #4623

relfock · 2024-10-30T16:44:07Z

The SConsCPPConditionalScanner fails to evaluate a #if pre-processor directive to True if the following defines are passed:
env['CPPDEFINES'] = ['A=1', 'B=C', 'C=1']

and if I have a code with the following line:
main.c contains this

#if A==B
#include "header1.h"
#endif
int main()
{
return 0;
}

SConstruct:

import SCons.Scanner.C

env = Environment()

env['CPPPATH'] = [Dir('.')]
env['CPPDEFINES'] = [('A',1),('B', 'C'), ('C',1)]

expected_deps = [File('header1.h')]

c_scanner = SCons.Scanner.C.CConditionalScanner()
deps = c_scanner(File('main.c'), env, env['CPPPATH'])

# Compare deps with expected_deps
if deps == expected_deps:
    print("Dependencies match expected dependencies.")
else:
    raise("!!! Dependencies do not match expected dependencies !!!")

The SConsCPPConditionalScanner seems to treat the value of B define as a string instead of another define thus resulting in the evaluation of #if False instead of True.

Link to SCons Users thread discussing your issue. https://discord.com/channels/571796279483564041/1301211142915362918
Version of SCons 4.8.1
Version of Python 3.10.12
How you installed SCons snap install scons
What Platform are you on? (Linux/Windows and which version) Ubuntu 22.04.3 LTS
How to reproduce your issue? See attached SCons file
How you invoke scons (The command line you're using "scons --flags some_arguments") scons

The text was updated successfully, but these errors were encountered:

mwichmann · 2024-10-30T17:49:10Z

I cleaned the example up a little bit, and yes, I can see the same thing happen.

The SConsCPPConditionalScanner seems to treat the value of B define as a string instead of another define

Well, yes. Internally, it converts the CPPDEFINES to a dict to match against, and that conversion gives {'A': 1, 'B': 'C', 'C': 1}. Then it takes each right-hand-side of the tuple it's made out of recognized preprocessor directives, makes it a Python expression, and does an eval of the expression against a namespace containing that dict. I think the conditional scanner would have to specialize PreProcessor.eval_expression to have any chance of knowing to substitute values at this level, as the base class one doesn't know about this.

@ivankravets any thoughts? I think you knew this code fairly well...

mwichmann · 2024-10-30T19:24:40Z

A quick hack shows that one level of substitution on CPPDEFINES is easy enough and "fixes" that specific problem. Not entirely convinced this is "the right thing to do".

$ scons -Q
DEBUG: dep= header1.h
Dependencies match expected dependencies.
scons: `.' is up to date.

relfock · 2024-10-31T11:06:07Z

There is another issue tightly coupled with this one: The SConsCPPConditionalScanner doesn't do macro expansion properly (or maybe doesn't do it at all ?). For example, if I have the following code, it fails to find header1.h as a dependency when I have this in SConstruct:

env['CPPDEFINES'] = ['FEATURE_A_ENABLED=1']

and with this main.c file:

/**
 * @brief Macro for checking if the specified identifier is defined and it has
 *        a non-zero value.
 *
 * Normally, preprocessors treat all undefined identifiers as having the value
 * zero. However, some tools, like static code analyzers, can issue a warning
 * when such identifier is evaluated. This macro gives the possibility to suppress
 * such warnings only in places where this macro is used for evaluation, not in
 * the whole analyzed code.
 */
#define IS_ENABLED(config_macro) _IS_ENABLED1(config_macro)
/* IS_ENABLED() helpers */

/* This is called from IS_ENABLED(), and sticks on a "_XXXX" prefix,
 * it will now be "_XXXX1" if config_macro is "1", or just "_XXXX" if it's
 * undefined.
 *   ENABLED:   IS_ENABLED2(_XXXX1)
 *   DISABLED   IS_ENABLED2(_XXXX)
 */
#define _IS_ENABLED1(config_macro) _IS_ENABLED2(_XXXX##config_macro)

/* Here's the core trick, we map "_XXXX1" to "_YYYY," (i.e. a string
 * with a trailing comma), so it has the effect of making this a
 * two-argument tuple to the preprocessor only in the case where the
 * value is defined to "1"
 *   ENABLED:    _YYYY,    <--- note comma!
 *   DISABLED:   _XXXX
 */
#define _XXXX1 _YYYY,

/* Then we append an extra argument to fool the gcc preprocessor into
 * accepting it as a varargs macro.
 *                         arg1   arg2  arg3
 *   ENABLED:   IS_ENABLED3(_YYYY,    1,    0)
 *   DISABLED   IS_ENABLED3(_XXXX 1,  0)
 */
#define _IS_ENABLED2(one_or_two_args) _IS_ENABLED3(one_or_two_args 1, 0)

/* And our second argument is thus now cooked to be 1 in the case
 * where the value is defined to 1, and 0 if not:
 */
#define _IS_ENABLED3(ignore_this, val, ...) val

#if IS_ENABLED(FEATURE_A_ENABLED)
#include "header1.h"
#endif

int main()
{
}

mwichmann · 2024-10-31T11:36:54Z

It doesn't do any macro expansion on the contents of CPPDEFINES. That's not limited to the "new" conditional scanner.

relfock · 2024-10-31T13:33:25Z

Will it do macro expansion if a define is inside the file instead, e.g #define FEATURE_A_ENABLED 1 in the source file ?
Also, what do you mean by saying "new" conditional scanner ?

mwichmann · 2024-10-31T13:37:17Z

Just terminology - the conditional scanner is much newer.

I haven't really thought about what happens to defines in the file, I think that works - I think that was the intent of the conditional scanner in the first place.

relfock · 2024-10-31T13:57:53Z

Thanks for clarifying.
I just tried it, putting #define FEATURE_A_ENABLED 1 in the same main.c file and removing it from CPPDEFINES , and that didn't work either :/

mwichmann · 2024-10-31T14:15:50Z

I don't seem to get the same results, this works out for me even with the old scanner:

$ cat SConstruct
env = Environment(CPPDEFINES='FEATURE_A_ENABLED=1')
env.Program('test.c')
$ scons -Qn --tree=all
gcc -o test.o -c -DFEATURE_A_ENABLED=1 test.c
gcc -o test test.o
+-.
  +-SConstruct2
  +-header1.h
  +-test
  | +-test.o
  | | +-test.c
  | | +-header1.h  <====
  | | +-/bin/gcc
  | +-/bin/gcc
  +-test.c
  +-test.o
    +-test.c
    +-header1.h  <====
    +-/bin/gcc
$

bdbaddog · 2024-10-31T14:35:31Z

I'd argue overcomplicated pre-processor logic to determine defines and includes might be better replaced with logic in your SConstruct.
That said the preprocessing c scanner has never been a full c pre-processor implementation.
So I'd expect that once you get beyond fairly simple preprocessor logic, it's not going to work presently.

relfock · 2024-10-31T14:37:31Z

Yes, the old one works since it always picks all includes ignoring any pre-processor stuff. My experiment was with conditional scanner. My expectation is that if in the code I have #define FEATURE_A_ENABLED 0 it should not pick header1.h as a dependency and vice versa. With the test below, it always picks header1.h as a dependency, which means it did not expand the macro properly and always evaluated the #if IS_ENABLED(FEATURE_A_ENABLED) as 1 instead of 0.

$ cat SConstruct
import SCons.Scanner.C

env = Environment()

env['CPPPATH'] = [Dir('.')]

expected_deps = []

c_scanner = SCons.Scanner.C.CConditionalScanner()
deps = c_scanner(File('main.c'), env, env['CPPPATH'])

print("deps: ", [d.get_path() for d in deps])
print("expected_deps: ", [d.get_path() for d in expected_deps])

# Compare deps with expected_deps
if deps == expected_deps:
    print("Dependencies match expected dependencies.")
else:
    raise("!!! Dependencies do not match expected dependencies !!!")

$ cat main.c
#define FEATURE_A_ENABLED 0

/**
 * @brief Macro for checking if the specified identifier is defined and it has
 *        a non-zero value.
 *
 * Normally, preprocessors treat all undefined identifiers as having the value
 * zero. However, some tools, like static code analyzers, can issue a warning
 * when such identifier is evaluated. This macro gives the possibility to suppress
 * such warnings only in places where this macro is used for evaluation, not in
 * the whole analyzed code.
 */
#define IS_ENABLED(config_macro) _IS_ENABLED1(config_macro)
/* IS_ENABLED() helpers */

/* This is called from IS_ENABLED(), and sticks on a "_XXXX" prefix,
 * it will now be "_XXXX1" if config_macro is "1", or just "_XXXX" if it's
 * undefined.
 *   ENABLED:   IS_ENABLED2(_XXXX1)
 *   DISABLED   IS_ENABLED2(_XXXX)
 */
#define _IS_ENABLED1(config_macro) _IS_ENABLED2(_XXXX##config_macro)

/* Here's the core trick, we map "_XXXX1" to "_YYYY," (i.e. a string
 * with a trailing comma), so it has the effect of making this a
 * two-argument tuple to the preprocessor only in the case where the
 * value is defined to "1"
 *   ENABLED:    _YYYY,    <--- note comma!
 *   DISABLED:   _XXXX
 */
#define _XXXX1 _YYYY,

/* Then we append an extra argument to fool the gcc preprocessor into
 * accepting it as a varargs macro.
 *                         arg1   arg2  arg3
 *   ENABLED:   IS_ENABLED3(_YYYY,    1,    0)
 *   DISABLED   IS_ENABLED3(_XXXX 1,  0)
 */
#define _IS_ENABLED2(one_or_two_args) _IS_ENABLED3(one_or_two_args 1, 0)

/* And our second argument is thus now cooked to be 1 in the case
 * where the value is defined to 1, and 0 if not:
 */
#define _IS_ENABLED3(ignore_this, val, ...) val

#if IS_ENABLED(FEATURE_A_ENABLED)
#include "header1.h"
#endif

int main()
{
}

$ scons -Qn --tree=all
deps:  ['header1.h']
expected_deps:  []
TypeError: exceptions must derive from BaseException:
  File "/hfv/test_battery/scons_bug/SConstruct", line 19:
    raise("!!! Dependencies do not match expected dependencies !!!")

mwichmann · 2024-10-31T14:41:15Z

if in the code I have #define FEATURE_A_ENABLED 0 it should not pick header1.h as a dependency

ah, I see. yes, setting it to 0 still picks the header.

relfock · 2024-10-31T14:43:27Z

@bdbaddog : Sure, I just wanted to know what currently is possible / supported before I dive and implement an alternative solution for non supported part. I really like the SCons pre-processor stuff since it is way faster then calling gcc's -MM, -MD to get dependency list and manipulate with it.

relfock · 2024-11-01T10:16:33Z

Another issue, and this time it's pretty simple and silly :) I still can't understand what kind of bug can cause this.
The issue is that if any define which has letter L (upper case) and has at least one digit before it, the pre-processor fails to evaluate the expression to True, it's always False. If I change L to any other character, lets say with M or K or any other (didn't check everything though) it starts to work, even changing it to a lower case l makes it work. Any thoughts ?

$ cat SConstruct

import SCons.Scanner.C

env = Environment()

env['CPPPATH'] = [Dir('.')]
env['CPPDEFINES'] = ['A1L']


expected_deps = [File('header1.h')]

c_scanner = SCons.Scanner.C.CConditionalScanner()
deps = c_scanner(File('main.c'), env, env['CPPPATH'])

print("deps: ", [d.get_path() for d in deps])
print("expected_deps: ", [d.get_path() for d in expected_deps])

# Compare deps with expected_deps
if deps == expected_deps:
    print("Dependencies match expected dependencies.")
else:
    raise("!!! Dependencies do not match expected dependencies !!!")

$ cat main.c

#if defined (A1L)
#include "header1.h"
#endif

int main()
{
}

$ scons -Q
deps:  []
expected_deps:  ['header1.h']
TypeError: exceptions must derive from BaseException:
  File "/home/andrey/scons_bug/SConstruct", line 21:
    raise("!!! Dependencies do not match expected dependencies !!!")

mwichmann · 2024-11-01T15:03:55Z

Pretty sure the magic is there's a digit followed by L or UL - that would be C syntax for numbers. To confirm, try something ending with L that doesn't have a digit in it. I'll go look at the regexes presently.

mwichmann · 2024-11-01T15:13:23Z

And indeed that wasn't too hard to find...

    [r'(\d+)(?:L|UL)?',  '\\1'],

That will turn the A1L into A1 in the preprocessor directives extracted from the file (that happens in SCons.cpp.CPP_to_Python).

relfock · 2024-11-01T15:14:55Z

I just confirmed as well that without any digit in it, it does work. Also ending with UL and ULL prefixed with a digit, doesn't work too, so you're spot on ;)

bdbaddog · 2024-11-04T16:01:17Z

Hmm. so the symbol needs to be all numeric followed by L or UL for this transformation to be correct?

mwichmann · 2024-11-04T16:41:14Z

I don't know about correct, but for it to take place, yes. The intent it to recognize numeric strings like 0x123L and 123UL. Whether it should also be recognized when it's part of a string that does not otherwise look numeric is a question - maybe a tweak to the regexes is in order.

relfock · 2024-11-04T17:51:44Z

Here are some examples in which case scons fails to evaluate:

TEST1L
TEST1L_ABC
TEST236UL
ABC2745ULL
ABC89UL_XYZ
ABC54ULL_HF

All these are valid C/C++ define names but SCons fails to deal with them.

So basically any string which contains a number followed by L / UL / ULL, and it doesn't matter where in the string.

mwichmann · 2024-11-04T18:14:13Z

Wonder if it's good enough to just add beginning whitespace (and maybe ending space-or-newline) to the regexes? Or will that mess up other things?

    [r'(0x[0-9A-Fa-f]+)(?:L|UL)?',  '\\1'],
    [r'(\d+)(?:L|UL)?',  '\\1'],

bdbaddog · 2024-11-04T18:28:08Z

I think this should fix it.. ^(\d+)(?:L|UL)? ?

…ef x123UL

bdbaddog · 2024-11-05T01:45:40Z

Fix for this UL/L issue in #4624

relfock · 2024-11-05T13:23:19Z

I think this should fix it.. ^(\d+)(?:L|UL)? ?

This will not match the following:
1234U
1234ULL

bdbaddog · 2024-11-05T17:22:54Z

^(\d+)(?:L|UL)?

Uh.. Yes it does. Check the PR. #4624 I added a test to cover x1233l x1234UL, and left the other 123l, 124ul.. which was working. Are you checking via https://regex101.com/ ? Check in the match information section of the page.

relfock · 2024-11-05T18:34:10Z

It matches 1234UL and 1234L but doesn't match 1234U or 1234ULL which are also valid to be matched isn't it ?

bdbaddog · 2024-11-05T18:39:47Z

Please don't post screenshots.. Also move these comments over to the PR.. #4624
I see the issue now. That shouldn't be hard to fix.

bdbaddog · 2024-11-05T19:17:33Z

@relfock - updated code in PR. Please take a look and comment there.

Simplistic macro replacement is now done on the contents of CPPDEFINES, to improve accuracy of conditional inclusion as compared to the real preprocessor (ref: issue SCons#4623). Signed-off-by: Mats Wichmann <[email protected]>

jcbrill · 2024-11-11T02:00:20Z

My expectation is that if in the code I have #define FEATURE_A_ENABLED 0 it should not pick header1.h as a dependency and vice versa.

I can confirm that the macro expansion for #if (and possibly other statements) is not recursive. Only the first expansion is performed. In this case, evaluation results in a non empty string (the expanded string containing another macro) which is True causing the inclusion of the header.

The #include statement itself does perform/attempt a recursive evaluation.

jcbrill · 2024-11-11T02:06:47Z

@relfock See #4629 (comment) for some notes concerning the SCons C preprocessor implementation.

Disclaimer: it is possible that some of the information presented may be erroneous. Maintain a healthy skepticism.

relfock · 2024-11-11T13:34:03Z

@jcbrill : Thank you for the notes and fixes for the possible ones. I'll let you know if I see any other issue which is not listed in your 'Known/Potential issues' notes.

@bdbaddog: Seems like most of the issues described in this issue has been fixed. Should we close this issue now or you would like to keep it open for the recursive expansion of the macro issue ?

Simplistic macro replacement is now done on the contents of CPPDEFINES, to improve accuracy of conditional inclusion as compared to the real preprocessor (ref: issue SCons#4623). Signed-off-by: Mats Wichmann <[email protected]>

bdbaddog · 2024-11-12T01:05:31Z

@relfock - can you create an enhancement request for any remaining issues, and then we'll close this one?

Simplistic macro replacement is now done on the contents of CPPDEFINES, to improve accuracy of conditional inclusion as compared to the real preprocessor (ref: issue SCons#4623). Signed-off-by: Mats Wichmann <[email protected]>

bdbaddog added the scanner label Oct 30, 2024

bdbaddog added a commit to bdbaddog/scons that referenced this issue Nov 5, 2024

Fix Issue SCons#4623. SConsCPPConditionalScanner misinterpreting #ifd…

a711d13

…ef x123UL

bdbaddog mentioned this issue Nov 5, 2024

Fix Issue with SConsCPPConditionalScanner misinterpreting ifdefs suffixed with L or UL #4624

Closed

3 tasks

mwichmann mentioned this issue Nov 7, 2024

Improve C scanner conditional inclusion #4626

Open

3 tasks

jcbrill mentioned this issue Nov 10, 2024

Fix Issue with SConsCPPConditionalScanner misinterpreting ifdefs suffixed with L or UL [Alternate Implementation] #4629

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SConsCPPConditionalScanner fails evaluate nested defines #4623

SConsCPPConditionalScanner fails evaluate nested defines #4623

relfock commented Oct 30, 2024 •

edited by mwichmann

Loading

mwichmann commented Oct 30, 2024 •

edited

Loading

mwichmann commented Oct 30, 2024

relfock commented Oct 31, 2024 •

edited

Loading

mwichmann commented Oct 31, 2024

relfock commented Oct 31, 2024 •

edited

Loading

mwichmann commented Oct 31, 2024

relfock commented Oct 31, 2024 •

edited

Loading

mwichmann commented Oct 31, 2024

bdbaddog commented Oct 31, 2024

relfock commented Oct 31, 2024

mwichmann commented Oct 31, 2024

relfock commented Oct 31, 2024

relfock commented Nov 1, 2024 •

edited

Loading

mwichmann commented Nov 1, 2024

mwichmann commented Nov 1, 2024

relfock commented Nov 1, 2024 •

edited

Loading

bdbaddog commented Nov 4, 2024

mwichmann commented Nov 4, 2024 •

edited

Loading

relfock commented Nov 4, 2024 •

edited

Loading

mwichmann commented Nov 4, 2024

bdbaddog commented Nov 4, 2024

bdbaddog commented Nov 5, 2024

relfock commented Nov 5, 2024

bdbaddog commented Nov 5, 2024

relfock commented Nov 5, 2024

bdbaddog commented Nov 5, 2024

bdbaddog commented Nov 5, 2024

jcbrill commented Nov 11, 2024

jcbrill commented Nov 11, 2024

relfock commented Nov 11, 2024

bdbaddog commented Nov 12, 2024

SConsCPPConditionalScanner fails evaluate nested defines #4623

SConsCPPConditionalScanner fails evaluate nested defines #4623

Comments

relfock commented Oct 30, 2024 • edited by mwichmann Loading

mwichmann commented Oct 30, 2024 • edited Loading

mwichmann commented Oct 30, 2024

relfock commented Oct 31, 2024 • edited Loading

mwichmann commented Oct 31, 2024

relfock commented Oct 31, 2024 • edited Loading

mwichmann commented Oct 31, 2024

relfock commented Oct 31, 2024 • edited Loading

mwichmann commented Oct 31, 2024

bdbaddog commented Oct 31, 2024

relfock commented Oct 31, 2024

mwichmann commented Oct 31, 2024

relfock commented Oct 31, 2024

relfock commented Nov 1, 2024 • edited Loading

mwichmann commented Nov 1, 2024

mwichmann commented Nov 1, 2024

relfock commented Nov 1, 2024 • edited Loading

bdbaddog commented Nov 4, 2024

mwichmann commented Nov 4, 2024 • edited Loading

relfock commented Nov 4, 2024 • edited Loading

mwichmann commented Nov 4, 2024

bdbaddog commented Nov 4, 2024

bdbaddog commented Nov 5, 2024

relfock commented Nov 5, 2024

bdbaddog commented Nov 5, 2024

relfock commented Nov 5, 2024

bdbaddog commented Nov 5, 2024

bdbaddog commented Nov 5, 2024

jcbrill commented Nov 11, 2024

jcbrill commented Nov 11, 2024

relfock commented Nov 11, 2024

bdbaddog commented Nov 12, 2024

relfock commented Oct 30, 2024 •

edited by mwichmann

Loading

mwichmann commented Oct 30, 2024 •

edited

Loading

relfock commented Oct 31, 2024 •

edited

Loading

relfock commented Oct 31, 2024 •

edited

Loading

relfock commented Oct 31, 2024 •

edited

Loading

relfock commented Nov 1, 2024 •

edited

Loading

relfock commented Nov 1, 2024 •

edited

Loading

mwichmann commented Nov 4, 2024 •

edited

Loading

relfock commented Nov 4, 2024 •

edited

Loading