On Fri, Nov 28, 2025 at 08:25:05AM +0530, Jason Merrill wrote: > On 11/15/25 6:04 AM, Marek Polacek wrote: > > This patch contains the middle-end, libcpp/, c-family/, and cp/ bits > > (besides reflect.cc and the gperf bits). > > > +/* In TREE_VALUE of an attribute this means the attribute argument > > + is never equal to different attribute argument with the same value. */ > > +#define ATTR_UNIQUE_VALUE_P(NODE) (TREE_LIST_CHECK > > (NODE)->base.protected_flag) > > The word "value" seems superfluous here; it's the attribute that's unique, > not the value. OTOH, I see why it's convenient to put this flag on the > value TREE_LIST rather than the attribute TREE_LIST, but that means we can > only set it for attributes with arguments, and this directly affects > attribute_value_equal, so I guess it's OK. But let's drop "argument" from > the comment. And also mention that the flag implies order sensitivity, > since that doesn't follow from uniqueness. > > We need an update to the BINFO_BASE_ACCESSES comment for the additional > information you're adding.
Thanks, started adding changes to https://forge.sourceware.org/marek/gcc/pulls/112 (mostly for the annotation stuff so far). > > @@ -715,14 +715,20 @@ attribute_takes_identifier_p (const_tree attr_id) > > { > > const struct attribute_spec *spec = lookup_attribute_spec (attr_id); > > if (spec == NULL) > > - /* Unknown attribute that we'll end up ignoring, return true so we > > - don't complain about an identifier argument. */ > > - return true; > > + { > > + /* Unknown attribute that we'll end up ignoring, return true so we > > + don't complain about an identifier argument. Except C++ > > + annotations. */ > > + if (c_dialect_cxx () && id_equal (attr_id, "annotation ")) > > + return false; > > This seems unreachable, since lookup_attribute_spec should succeed for > annotations? Unfortunately it is not. ATTR_ID here is just the attribute name, so lookup_attribute_spec attempts to find it in the gnu namespace rather than in the "internal " namespace in which it is present, and fails. In order to make it work I guess is_late_template_attribute would need to pass TREE_PURPOSE (attr) instead of name to attribute_takes_identifier_p, similarly tsubst_attribute pass TREE_PURPOSE (t) instead of get_attribute_name (t), and attribute_takes_identifier_p would then need to do if (TREE_CODE (attr_id) == TREE_LIST) attr_id = TREE_VALUE (attr_id); for the targetm.* call at the end (which still expects just IDENTIFIER_NODE. Similarly maybe is_late_template_attribute should again call lookup_attribute_spec on TREE_PURPOSE (attr) rather than name and then the if (annotation_p (attr)) return true; stuff could be done after the if (!spec) return false; checking. > > @@ -1758,10 +1789,25 @@ merge_attributes (tree a1, tree a2) > > if (a == NULL_TREE) > > { > > a1 = copy_node (a2); > > - TREE_CHAIN (a1) = attributes; > > - attributes = a1; > > + if (a2_unique_value_p) > > + { > > + /* Make sure not to reverse order of a2 chain > > + added before attributes. */ > > + *pa = a1; > > + pa = &TREE_CHAIN (a1); > > + } > > + else > > + { > > + TREE_CHAIN (a1) = attributes; > > + attributes = a1; > > + } > > } > > } > > + if (a2_unique_value_p && a3) > > + { > > + *pa = attributes; > > + attributes = a3; > > + } > > Since we need to preserve order in some cases, why not do it in all cases? > The *pa approach doesn't seem any less efficient than the old code. The pick longest list and hang on the other list to it is more efficient if we can reorder. You're right that in this particular hunk if normal attributes (rather than annotations) are order independent it shouldn't matter which way we choose and so in theory could handle it always as if a2_unique_value_p was true. But I'm afraid that in real-world the order of attributes is not that unimportant, while some attribute users walk all attributes and handle all their values, I'm afraid tons of places just ask for the first one and use its value. Sure, it is not that obvious for users what exactly is chosen as first (for all attributes on the same decl first is usually the last one, for multiple decls it depends on the number of attributes on each and what is the same between those lists), but still such a change could break what used to work fine for years one way or another. If they add in annotations, guess it is unavoidable that things change, but otherwise it is not needed. > > @@ -957,20 +957,23 @@ decl_attributes (tree *node, tree attributes, int > > flags, > > if (!no_add_attrs) > > { > > tree old_attrs; > > - tree a; > > + tree a = NULL_TREE; > > if (DECL_P (*anode)) > > old_attrs = DECL_ATTRIBUTES (*anode); > > else > > old_attrs = TYPE_ATTRIBUTES (*anode); > > - for (a = find_same_attribute (attr, old_attrs); > > - a != NULL_TREE; > > - a = find_same_attribute (attr, TREE_CHAIN (a))) > > - { > > - if (simple_cst_equal (TREE_VALUE (a), args) == 1) > > - break; > > - } > > + if (!args > > + || TREE_CODE (args) != TREE_LIST > > + || !ATTR_UNIQUE_VALUE_P (args)) > > + for (a = find_same_attribute (attr, old_attrs); > > + a != NULL_TREE; > > + a = find_same_attribute (attr, TREE_CHAIN (a))) > > + { > > + if (simple_cst_equal (TREE_VALUE (a), args) == 1) > > Pre-existing issue, but it seems like a problem that this doesn't handle the > special cases of attribute_value_equal? So, shall it use attribute_value_equal instead? > > @@ -402,7 +402,7 @@ build_call_a (tree function, int n, tree *argarray) > > /* Don't pass empty class objects by value. This is useful > > for tags in STL, which are used to control overload resolution. > > We don't need to handle other cases of copying empty classes. */ > > - if (!decl || !fndecl_built_in_p (decl)) > > + if (!decl || (!fndecl_built_in_p (decl) && !metafunction_p (decl))) > > Why is this needed for metafunctions? Will defer to Marek, from git blame it fixed some ICE on g++.dg/reflect/reflect_constant{3,4,5}.C tests. > > + /* Some metafunctions aren't dependent just on their arguments, but also > > + on various other dependencies, e.g. has_identifier on a function > > parameter > > + reflection can change depending on further declarations of > > corresponding > > + function, is_complete_type depends on type definitions and template > > + specializations in between the calls, define_aggregate even defines > > + class types, etc. Thus, we need to arrange for calls which call > > + at least some metafunctions to be non-cacheable, because their > > behavior > > + might not be the same. Until we figure out which exact metafunctions > > + need this and which don't, do it for all of them. */ > > + bool metafns_called; > > Maybe instead we need more calls to clear_cv_and_fold_caches? But when? During completion of any type, any redeclaration of a function, and many other conditions which can change behavior of some metafunctions? Any additions of decls to namespaces would be another change, etc. On the other side, do we to clear the caches in all such spots even when none of the metafunctions have been used in anything that has been cached? > > @@ -2900,6 +2917,70 @@ min_vis_expr_r (tree *tp, int */*walk_subtrees*/, > > void *data) > > tpvis = type_visibility (DECL_CONTEXT (t)); > > break; > > + case REFLECT_EXPR: > > + tree r, c; > > + r = REFLECT_EXPR_HANDLE (t); > > + switch (REFLECT_EXPR_KIND (t)) > > + { > > + case REFLECT_BASE: > > + /* For direct base class relationship, determine visibility > > + from both D and B types. */ > > + tpvis = type_visibility (BINFO_TYPE (r)); > > + if (tpvis > *vis_p) > > + *vis_p = tpvis; > > + c = r; > > + while (BINFO_INHERITANCE_CHAIN (c)) > > Why does this need to loop? > > > + c = BINFO_INHERITANCE_CHAIN (c); The current patchset records direct base relationship reflection as REFLECT_BASE with a TREE_BINFO as REFLECT_EXPR_HANDLE. That looked to me best as it is an existing tree, instead of having to create say a TREE_VEC with 2 elements or TREE_LIST, where one operand of that would be the derived class and the other the base class. The base class is immediately accessible from the TREE_BINFO, it is BINFO_TYPE (REFLECT_EXPR_HANDLE (t)). The derived class is usually BINFO_TYPE (BINFO_INHERITANCE_CHAIN (REFLECT_EXPR_HANDLE (t))), but not always, sometimes there is longer BINFO_INHERITANCE_CHAIN chain before reaching the final one (which has NULL BINFO_INHERITANCE_CHAIN). There are testcases that test that in the testsuite. Jakub
