From 38261a8202c848d309c0ff0a6b53383e4cce7d2f Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Thu, 7 Nov 2024 07:16:55 -0700 Subject: [PATCH 1/3] Improve C scanner conditional inclusion 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 #4623). Signed-off-by: Mats Wichmann --- CHANGES.txt | 3 ++ RELEASE.txt | 3 ++ SCons/Scanner/C.py | 69 ++++++++++++++++++++++++++++++++--------- SCons/Scanner/CTests.py | 12 +++++++ 4 files changed, 73 insertions(+), 14 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index aa29180f5a..6411fcb56b 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -167,6 +167,9 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER their values taken from the default in the variable description (if a variable was set to the same value as the default in one of the input sources, it is not included in this list). + - The C scanner now does (limited) macro replacement on the values in + CPPDEFINES, to improve results of conditional source file inclusion + (issue #4523). RELEASE 4.8.1 - Tue, 03 Sep 2024 17:22:20 -0700 diff --git a/RELEASE.txt b/RELEASE.txt index aa3e3c15db..93e137f4d8 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -150,6 +150,9 @@ FIXES python ninja package version 1.11.1.2 changed the location and previous logic no longer worked. +- The C scanner now does (limited) macro replacement on the values in + CPPDEFINES, to improve results of conditional source file inclusion + (issue #4523). IMPROVEMENTS ------------ diff --git a/SCons/Scanner/C.py b/SCons/Scanner/C.py index aafe0d9a56..ad332b8ad6 100644 --- a/SCons/Scanner/C.py +++ b/SCons/Scanner/C.py @@ -28,6 +28,7 @@ add_scanner() for each affected suffix. """ +from typing import Dict import SCons.Node.FS import SCons.cpp import SCons.Util @@ -66,31 +67,69 @@ def read_file(self, file) -> str: return '' def dictify_CPPDEFINES(env) -> dict: - """Returns CPPDEFINES converted to a dict. - - This should be similar to :func:`~SCons.Defaults.processDefines`. - Unfortunately, we can't do the simple thing of calling that routine and - passing the result to the dict() constructor, because it turns the defines - into a list of "name=value" pairs, which the dict constructor won't - consume correctly. Also cannot just call dict on CPPDEFINES itself - it's - fine if it's stored in the converted form (currently deque of tuples), but - CPPDEFINES could be in other formats too. - - So we have to do all the work here - keep concepts in sync with - ``processDefines``. + """Return CPPDEFINES converted to a dict for preprocessor emulation. + + The concept is similar to :func:`~SCons.Defaults.processDefines`: + turn the values stored in an internal form in ``env['CPPDEFINES']`` + into one usable in a specific context - in this case the cpp-like + work the C/C++ scanner will do. We can't reuse ``processDefines`` + output as that's a list of strings for the command line. We also can't + pass the ``CPPDEFINES`` variable directly to the ``dict`` constructor, + as SCons allows it to be stored in several different ways - it's only + after ``Append`` and relatives has been called we know for sure it will + be a deque of tuples. + + Since the result here won't pass through a real preprocessor, simulate + some of the macro replacement that would take place if it did, or some + conditional inclusions might come out wrong. A bit of an edge case, but + does happen (GH #4623). See 6.10.5 in the C standard and 15.6 in the + C++ standard). + + .. versionchanged:: NEXT_RELEASE + Simple macro replacement added. """ + def _replace(mapping: Dict) -> Dict: + """Simplistic macro replacer for dictify_CPPDEFINES. + + *mapping* is scanned for any value that is the same as a key in + the dict, and is replaced by the value of that key; the process + is repeated. This is a cheap approximation of the C preprocessor's + macro replacement rules with no smarts - it doesn't "look inside" + the values, so only triggers on object-like macros, not on + function-like macros, and will not work on complex values, + e.g. a value like ``(1UL << PR_MTE_TCF_SHIFT)`` would not have + ``PR_MTE_TCF_SHIFT`` replaced if it was also in ``CPPDEFINES``, + but rather left as-is for the scanner to do comparisons against. + + Args: + mapping: a dictionary representing macro names and replacements. + + Returns: + a dictionary with substitutions made. + """ + old_ns = mapping + ns = {} + while True: + ns = {k: old_ns[v] if v in old_ns else v for k, v in old_ns.items()} + if old_ns == ns: + break + old_ns = ns + return ns + cppdefines = env.get('CPPDEFINES', {}) result = {} if cppdefines is None: return result if SCons.Util.is_Tuple(cppdefines): + # single macro defined in a tuple try: return {cppdefines[0]: cppdefines[1]} except IndexError: return {cppdefines[0]: None} if SCons.Util.is_Sequence(cppdefines): + # multiple (presumably) macro defines in a deque, list, etc. for c in cppdefines: if SCons.Util.is_Sequence(c): try: @@ -107,9 +146,10 @@ def dictify_CPPDEFINES(env) -> dict: else: # don't really know what to do here result[c] = None - return result + return _replace(result) if SCons.Util.is_String(cppdefines): + # single macro define in a string try: name, value = cppdefines.split('=') return {name: value} @@ -117,7 +157,8 @@ def dictify_CPPDEFINES(env) -> dict: return {cppdefines: None} if SCons.Util.is_Dict(cppdefines): - return cppdefines + # already in the desired form + return _replace(result) return {cppdefines: None} diff --git a/SCons/Scanner/CTests.py b/SCons/Scanner/CTests.py index 6860a10cef..8355937671 100644 --- a/SCons/Scanner/CTests.py +++ b/SCons/Scanner/CTests.py @@ -572,6 +572,18 @@ def runTest(self) -> None: expect = {"STRING": "VALUE", "UNVALUED": None} self.assertEqual(d, expect) + with self.subTest("CPPDEFINES with macro replacement"): + env = DummyEnvironment( + CPPDEFINES=[ + ("STRING", "VALUE"), + ("REPLACEABLE", "RVALUE"), + ("RVALUE", "AVALUE"), + ] + ) + d = SCons.Scanner.C.dictify_CPPDEFINES(env) + expect = {"STRING": "VALUE", "REPLACEABLE": "AVALUE", "RVALUE": "AVALUE"} + self.assertEqual(d, expect) + if __name__ == "__main__": unittest.main() From 0cb87375ad87f3d6cc77e1fdcf0e819bad400b54 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Wed, 27 Nov 2024 10:39:12 -0700 Subject: [PATCH 2/3] Further tweak CPPDEFINES repleacement in scanner Replacement is now limited to five passes, to avoid going into an endless loop in pathlogical cases where there are three or more macros that circularly refer to each other. No error is reported in this case. Replacement is now only done for the otional C Conditional scanner, not for the classical C scanner. Signed-off-by: Mats Wichmann --- CHANGES.txt | 7 +++++ RELEASE.txt | 13 ++++++--- SCons/Scanner/C.py | 60 ++++++++++++++++++++++++----------------- SCons/Scanner/CTests.py | 2 +- 4 files changed, 53 insertions(+), 29 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 6411fcb56b..2609355a97 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -170,6 +170,13 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - The C scanner now does (limited) macro replacement on the values in CPPDEFINES, to improve results of conditional source file inclusion (issue #4523). + - The (optional) C Conditional Scanner now does limited macro + replacement on the contents of CPPDEFINES, to improve finding deps + that are conditionally included. Previously replacement was only + done on macro definitions found in the file being scanned. + Only object-like macros are replaced (not function-like), and + only on a whole-word basis; recursion is limited to five levels + and does not error out if that limit is reached (issue #4523). RELEASE 4.8.1 - Tue, 03 Sep 2024 17:22:20 -0700 diff --git a/RELEASE.txt b/RELEASE.txt index 93e137f4d8..da8f74d3e8 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -144,15 +144,20 @@ FIXES not designed to work for the root user. - Make sure unknown variables from a Variables file are recognized - as such (issue #4645) + as such. Previously only unknowns from the command line were + recognized (issue #4645). - Update ninja tool to use ninja.BIN_DIR to find pypi packaged ninja binary. python ninja package version 1.11.1.2 changed the location and previous logic no longer worked. -- The C scanner now does (limited) macro replacement on the values in - CPPDEFINES, to improve results of conditional source file inclusion - (issue #4523). +- The (optional) C Conditional Scanner now does limited macro + replacement on the contents of CPPDEFINES, to improve finding deps + that are conditionally included. Previously replacement was only + done on macro definitions found in the file being scanned. + Only object-like macros are replaced (not function-like), and + only on a whole-word basis; recursion is limited to five levels + and does not error out if that limit is reached (issue #4523). IMPROVEMENTS ------------ diff --git a/SCons/Scanner/C.py b/SCons/Scanner/C.py index ad332b8ad6..e111778c97 100644 --- a/SCons/Scanner/C.py +++ b/SCons/Scanner/C.py @@ -29,6 +29,7 @@ """ from typing import Dict + import SCons.Node.FS import SCons.cpp import SCons.Util @@ -66,12 +67,12 @@ def read_file(self, file) -> str: self.missing.append((file, self.current_file)) return '' -def dictify_CPPDEFINES(env) -> dict: +def dictify_CPPDEFINES(env, replace: bool = False) -> dict: """Return CPPDEFINES converted to a dict for preprocessor emulation. The concept is similar to :func:`~SCons.Defaults.processDefines`: turn the values stored in an internal form in ``env['CPPDEFINES']`` - into one usable in a specific context - in this case the cpp-like + into one needed for a specific context - in this case the cpp-like work the C/C++ scanner will do. We can't reuse ``processDefines`` output as that's a list of strings for the command line. We also can't pass the ``CPPDEFINES`` variable directly to the ``dict`` constructor, @@ -79,47 +80,50 @@ def dictify_CPPDEFINES(env) -> dict: after ``Append`` and relatives has been called we know for sure it will be a deque of tuples. - Since the result here won't pass through a real preprocessor, simulate - some of the macro replacement that would take place if it did, or some - conditional inclusions might come out wrong. A bit of an edge case, but - does happen (GH #4623). See 6.10.5 in the C standard and 15.6 in the - C++ standard). + If requested (*replace* is true), simulate some of the macro + replacement that would take place if an actual preprocessor ran, + to avoid some conditional inclusions comeing out wrong. A bit + of an edge case, but does happen (GH #4623). See 6.10.5 in the C + standard and 15.6 in the C++ standard). + + Args: + replace: if true, simulate macro replacement .. versionchanged:: NEXT_RELEASE - Simple macro replacement added. + Simple macro replacement added, and *replace* arg to enable it. """ def _replace(mapping: Dict) -> Dict: """Simplistic macro replacer for dictify_CPPDEFINES. - *mapping* is scanned for any value that is the same as a key in - the dict, and is replaced by the value of that key; the process - is repeated. This is a cheap approximation of the C preprocessor's + Scan *mapping* for a value that is the same as a key in the dict, + and replace with the value of that key; the process is repeated a few + times, but not forever in case someone left a case that can't be + fully resolved. This is a cheap approximation of the preprocessor's macro replacement rules with no smarts - it doesn't "look inside" the values, so only triggers on object-like macros, not on - function-like macros, and will not work on complex values, - e.g. a value like ``(1UL << PR_MTE_TCF_SHIFT)`` would not have - ``PR_MTE_TCF_SHIFT`` replaced if it was also in ``CPPDEFINES``, - but rather left as-is for the scanner to do comparisons against. + function-like macros, and will not work on complex values, e.g. + a value like ``(1UL << PR_MTE_TCF_SHIFT)`` would not have + ``PR_MTE_TCF_SHIFT`` replaced if that was also a key in ``CPPDEFINES``. Args: mapping: a dictionary representing macro names and replacements. Returns: - a dictionary with substitutions made. + a dictionary with replacements made. """ old_ns = mapping - ns = {} - while True: + loops = 0 + while loops < 5: # don't recurse forever in case there's circular data ns = {k: old_ns[v] if v in old_ns else v for k, v in old_ns.items()} if old_ns == ns: break old_ns = ns + loops += 1 return ns cppdefines = env.get('CPPDEFINES', {}) - result = {} - if cppdefines is None: - return result + if not cppdefines: + return {} if SCons.Util.is_Tuple(cppdefines): # single macro defined in a tuple @@ -130,6 +134,7 @@ def _replace(mapping: Dict) -> Dict: if SCons.Util.is_Sequence(cppdefines): # multiple (presumably) macro defines in a deque, list, etc. + result = {} for c in cppdefines: if SCons.Util.is_Sequence(c): try: @@ -146,7 +151,9 @@ def _replace(mapping: Dict) -> Dict: else: # don't really know what to do here result[c] = None - return _replace(result) + if replace: + return _replace(result) + return(result) if SCons.Util.is_String(cppdefines): # single macro define in a string @@ -158,7 +165,9 @@ def _replace(mapping: Dict) -> Dict: if SCons.Util.is_Dict(cppdefines): # already in the desired form - return _replace(result) + if replace: + return _replace(cppdefines) + return cppdefines return {cppdefines: None} @@ -177,7 +186,9 @@ def __init__(self, name, variable) -> None: def __call__(self, node, env, path=()): cpp = SConsCPPScanner( - current=node.get_dir(), cpppath=path, dict=dictify_CPPDEFINES(env) + current=node.get_dir(), + cpppath=path, + dict=dictify_CPPDEFINES(env, replace=True), ) result = cpp(node) for included, includer in cpp.missing: @@ -190,6 +201,7 @@ def __call__(self, node, env, path=()): def recurse_nodes(self, nodes): return nodes + def select(self, node): return self diff --git a/SCons/Scanner/CTests.py b/SCons/Scanner/CTests.py index 8355937671..b0fdb566e2 100644 --- a/SCons/Scanner/CTests.py +++ b/SCons/Scanner/CTests.py @@ -580,7 +580,7 @@ def runTest(self) -> None: ("RVALUE", "AVALUE"), ] ) - d = SCons.Scanner.C.dictify_CPPDEFINES(env) + d = SCons.Scanner.C.dictify_CPPDEFINES(env, replace=True) expect = {"STRING": "VALUE", "REPLACEABLE": "AVALUE", "RVALUE": "AVALUE"} self.assertEqual(d, expect) From 3cda270b4fa252743e615711c0f91de1a4f66e32 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Fri, 13 Dec 2024 15:01:39 -0700 Subject: [PATCH 3/3] Switch scanner CPPDEFINES replacement algorithm Now using the "modified" approach from the PR discussion: unroll the dict comprehension and as we process replacements keep track if changes were made, rather than doing the relatively more expensive dict-vs-dict comparison at the end of each loop. Signed-off-by: Mats Wichmann --- CHANGES.txt | 3 --- SCons/Scanner/C.py | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 2609355a97..9bf0edfc53 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -167,9 +167,6 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER their values taken from the default in the variable description (if a variable was set to the same value as the default in one of the input sources, it is not included in this list). - - The C scanner now does (limited) macro replacement on the values in - CPPDEFINES, to improve results of conditional source file inclusion - (issue #4523). - The (optional) C Conditional Scanner now does limited macro replacement on the contents of CPPDEFINES, to improve finding deps that are conditionally included. Previously replacement was only diff --git a/SCons/Scanner/C.py b/SCons/Scanner/C.py index e111778c97..1d7e101e4e 100644 --- a/SCons/Scanner/C.py +++ b/SCons/Scanner/C.py @@ -114,8 +114,19 @@ def _replace(mapping: Dict) -> Dict: old_ns = mapping loops = 0 while loops < 5: # don't recurse forever in case there's circular data - ns = {k: old_ns[v] if v in old_ns else v for k, v in old_ns.items()} - if old_ns == ns: + # this was originally written as a dict comprehension, but unrolling + # lets us add a finer-grained check for whether another loop is + # needed, rather than comparing two dicts to see if one changed. + again = False + ns = {} + for k, v in old_ns.items(): + if v in old_ns: + ns[k] = old_ns[v] + if not again and ns[k] != v: + again = True + else: + ns[k] = v + if not again: break old_ns = ns loops += 1