From c8801514d63aaa6ad78ee62930ca5160fb52f74a Mon Sep 17 00:00:00 2001 From: Ulf Magnusson Date: Thu, 14 Jun 2018 16:47:21 +0200 Subject: Fix incorrectly ordered properties for some nested multi.def. symbols _propagate_deps() visits menu nodes roughly breadth-first, meaning properties on symbols and choices defined in multiple locations could end up in the wrong order when copied from the menu node for some unlucky if/menu nestings. Fix it by moving the menu-node-to-symbol/choice property copying in _finalize_tree() so that it's guaranteed to happen in definition order. This bug was introduced by commit 63a4418 ("Record which MenuNode has each property"). --- kconfiglib.py | 97 ++++++++++++++++++++++++++------------------ tests/Kdepcopy | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ testsuite.py | 30 +++++++++++++- 3 files changed, 213 insertions(+), 39 deletions(-) create mode 100644 tests/Kdepcopy diff --git a/kconfiglib.py b/kconfiglib.py index 6abb4e6..af5415d 100644 --- a/kconfiglib.py +++ b/kconfiglib.py @@ -2097,7 +2097,7 @@ class Kconfig(object): def _parse_properties(self, node): # Parses and adds properties to the MenuNode 'node' (type, 'prompt', # 'default's, etc.) Properties are later copied up to symbols and - # choices in a separate pass after parsing, in _propagate_deps(). + # choices in a separate pass after parsing, in _copy_deps_to_sc(). # # An older version of this code added properties directly to symbols # and choices instead of to their menu nodes (and handled dependency @@ -2536,17 +2536,28 @@ class Kconfig(object): if node.item == MENU: visible_if = self._make_and(visible_if, node.visibility) + # Propagate the menu node's dependencies to each child menu node. + # + # The recursive _finalize_tree() calls assume that the current + # "level" in the tree has already had dependencies propagated. This + # makes e.g. implicit submenu creation, which needs to look ahead, + # easier to implement. self._propagate_deps(node, visible_if) + # Finalize the children cur = node.list while cur: self._finalize_tree(cur, visible_if) cur = cur.next elif isinstance(node.item, Symbol): - # The menu node is a symbol. See if we can create an implicit menu - # rooted at it and finalize each child in that menu if so, like for - # the choice/menu/if case above. + # Add the node's non-node-specific properties (defaults, ranges, + # etc.) to the Symbol + self._copy_deps_to_sc(node) + + # See if we can create an implicit menu rooted at the Symbol and + # finalize each child menu node in that menu if so, like for the + # choice/menu/if case above cur = node while cur.next and _auto_menu_dep(node, cur.next): # This also makes implicit submenu creation work recursively, @@ -2572,17 +2583,12 @@ class Kconfig(object): # Empty choices (node.list None) are possible, so this needs to go # outside if isinstance(node.item, Choice): + # Add the node's non-node-specific properties to the choice + self._copy_deps_to_sc(node) _finalize_choice(node) def _propagate_deps(self, node, visible_if): - # This function combines two tasks: - # - # 1) Copy properties from menu nodes to symbols and choices - # - # 2) Propagate dependencies from 'if' and 'depends on' to all - # properties - # - # See _parse_properties() as well. + # Propagates 'node's dependencies to its child menu nodes # If the parent node holds a Choice, we use the Choice itself as the # parent dependency. This makes sense as the value (mode) of the choice @@ -2605,15 +2611,6 @@ class Kconfig(object): if isinstance(cur.item, (Symbol, Choice)): sc = cur.item - # See the Symbol class docstring - sc.direct_dep = self._make_or(sc.direct_dep, dep) - - # TODO: Profile this code and see if the 'if's are worthwhile. - # Another potential optimization would be to assign the lists - # from the MenuNode directly instead of using extend() in cases - # where a symbol/choice only has a single MenuNode (the - # majority of cases). - # Propagate 'visible if' dependencies to the prompt if cur.prompt: cur.prompt = (cur.prompt[0], @@ -2624,45 +2621,69 @@ class Kconfig(object): if cur.defaults: cur.defaults = [(default, self._make_and(cond, dep)) for default, cond in cur.defaults] - sc.defaults.extend(cur.defaults) # Propagate dependencies to ranges if cur.ranges: cur.ranges = [(low, high, self._make_and(cond, dep)) for low, high, cond in cur.ranges] - sc.ranges.extend(cur.ranges) # Propagate dependencies to selects if cur.selects: cur.selects = [(target, self._make_and(cond, dep)) for target, cond in cur.selects] - sc.selects.extend(cur.selects) - - # Modify the reverse dependencies of the selected symbol - for target, cond in cur.selects: - target.rev_dep = self._make_or( - target.rev_dep, - self._make_and(sc, cond)) # Propagate dependencies to implies if cur.implies: cur.implies = [(target, self._make_and(cond, dep)) for target, cond in cur.implies] - sc.implies.extend(cur.implies) - - # Modify the weak reverse dependencies of the implied - # symbol - for target, cond in cur.implies: - target.weak_rev_dep = self._make_or( - target.weak_rev_dep, - self._make_and(sc, cond)) cur = cur.next + def _copy_deps_to_sc(self, node): + # Copies properties from the menu node 'node' up to its + # contained symbol or choice. + # + # This can't be rolled into _propagate_deps(), because that function + # traverses the menu tree roughly breadth-first order, meaning + # properties on symbols and choices defined in multiple locations could + # end up in the wrong order. + + # Symbol or choice + sc = node.item + + # See the Symbol class docstring + sc.direct_dep = self._make_or(sc.direct_dep, node.dep) + + if node.defaults: + sc.defaults.extend(node.defaults) + + if node.ranges: + sc.ranges.extend(node.ranges) + + if node.selects: + sc.selects.extend(node.selects) + + # Modify the reverse dependencies of the selected symbol + for target, cond in node.selects: + target.rev_dep = self._make_or( + target.rev_dep, + self._make_and(sc, cond)) + + if node.implies: + sc.implies.extend(node.implies) + + # Modify the weak reverse dependencies of the implied + # symbol + for target, cond in node.implies: + target.weak_rev_dep = self._make_or( + target.weak_rev_dep, + self._make_and(sc, cond)) + + # # Misc. # diff --git a/tests/Kdepcopy b/tests/Kdepcopy new file mode 100644 index 0000000..689bcc3 --- /dev/null +++ b/tests/Kdepcopy @@ -0,0 +1,125 @@ +# We verify that the properties below end up in definition order + +config MULTIDEF + bool + default A + default B + select AA + imply AA + +if FOO + +config MULTIDEF + default C + default D + select BB + imply BB + +if BAR + +config MULTIDEF + default E + default F + select CC + imply CC + +menu "menu" + +config MULTIDEF + default G + default H + select DD + imply DD + +config MULTIDEF + default I + default J + select EE + imply EE + +endmenu + +config MULTIDEF + default K + default L + select FF + imply FF + +config MULTIDEF + default M + default N + select GG + imply GG + +endif + +config MULTIDEF + default O + default P + select HH + select II + imply HH + imply II + +endif + +config MULTIDEF + default Q + default R + select JJ + imply JJ + + +# Same test with choices involved + +config MULTIDEF_CHOICE + bool + select A + +choice + bool "choice" + +config MULTIDEF_CHOICE + bool "multidef choice" + select B + +endchoice + +config MULTIDEF_CHOICE + bool + select C + + +# Same test with ranges involved + +config MULTIDEF_RANGE + int + range A _ + +menu "menu" + +config MULTIDEF_RANGE + int + range B _ + +if FOO + +config MULTIDEF_RANGE + int + range C _ + +endif + +config MULTIDEF_RANGE + int + range D _ + +endmenu + +config MULTIDEF_RANGE + int + range E _ + +config MULTIDEF_RANGE + int + range F _ diff --git a/testsuite.py b/testsuite.py index 5c12bc0..c773b99 100644 --- a/testsuite.py +++ b/testsuite.py @@ -1022,7 +1022,7 @@ g verify_deps(c.syms["JUST_DEPENDS_ON_REFS"].nodes[0], "A", "B") verify_deps(c.syms["LOTS_OF_REFS"].nodes[0], - *(chr(n) for n in range(ord('A'), ord('Z') + 1))) + *(chr(n) for n in range(ord("A"), ord("Z") + 1))) verify_deps(c.syms["INT_REFS"].nodes[0], "A", "B", "C", "D", "E", "F", "G", "H", "y") @@ -1945,6 +1945,34 @@ g verify_is_normal_choice_symbol("WS9") + print("Testing multi.def. property copying") + + c = Kconfig("Kconfiglib/tests/Kdepcopy", warn=False) + + def verify_props(desc, props, prop_names): + actual = [prop[0].name for prop in props] + expected = prop_names.split() + + verify(actual == expected, + "Wrong {} properties, expected '{}', got '{}'" + .format(desc, expected, actual)) + + verify_props("default", c.syms["MULTIDEF"].defaults, + "A B C D E F G H I J K L M N O P Q R") + + verify_props("select", c.syms["MULTIDEF"].selects, + "AA BB CC DD EE FF GG HH II JJ") + + verify_props("imply", c.syms["MULTIDEF"].selects, + "AA BB CC DD EE FF GG HH II JJ") + + verify_props("select", c.syms["MULTIDEF_CHOICE"].selects, + "A B C") + + verify_props("range", c.syms["MULTIDEF_RANGE"].ranges, + "A B C D E F") + + print("\nAll selftests passed\n" if all_passed else "\nSome selftests failed\n") -- cgit v1.2.3