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.
Both done. <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > > +/* Parse a C++-26 annotation list. > > + > > + annotation-list: > > + annotation ... [opt] > > + annotation-list , annotation ... [opt] > > + > > + annotation: > > + = constant-expression */ > > + > > +static tree > > +cp_parser_annotation_list (cp_parser *parser) > > +{ > > + tree attributes = NULL_TREE; > > + > > + while (true) > > + { > > + cp_token *token = cp_lexer_peek_token (parser->lexer); > > + location_t loc = token->location; > > + if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) > > + { > > + cp_lexer_consume_token (parser->lexer); > > + tree annotation > > + = cp_parser_constant_expression (parser, > > + /*allow_non_constant_p=*/false, > > + /*non_constant_p=*/nullptr, > > + /*strict_p=*/true); > > + if (annotation == error_mark_node) > > + break; > > + auto suppression > > + = make_temp_override (suppress_location_wrappers, 0); > > Why here rather than before parsing? Done. > > + annotation = maybe_wrap_with_location (annotation, loc); > > ...so this wouldn't be needed? Done in <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > > + annotation = build_tree_list (NULL_TREE, annotation); > > + if (cp_lexer_next_token_is (parser->lexer, CPP_ELLIPSIS)) > > + { > > + cp_lexer_consume_token (parser->lexer); > > + annotation = make_pack_expansion (annotation); > > + if (annotation == error_mark_node) > > + break; > > + } > > + attributes = tree_cons (build_tree_list (internal_identifier, > > + annotation_identifier), > > + annotation, attributes); > > + if (processing_template_decl) > > + ATTR_IS_DEPENDENT (attributes) = 1; > > This seems redundant with the is_late_template_attribute change. Removed in <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > ...> + tree name = get_attribute_name (attr); > > + if (is_attribute_p ("annotation ", name)) > > This pattern seems common enough to justify adding annotation_p (attr). Done in <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > > @@ -31925,7 +32826,27 @@ cp_parser_std_attribute_list (cp_parser *parser, > > tree attr_ns) > > while (true) > > { > > - location_t loc = cp_lexer_peek_token (parser->lexer)->location; > > + token = cp_lexer_peek_token (parser->lexer); > > + location_t loc = token->location; > > + if (token->type == CPP_EQ) > > + { > > + error_at (loc, "mixing annotations and attributes in the same list"); > > + cp_lexer_consume_token (parser->lexer); > > + tree annotation > > + = cp_parser_constant_expression (parser, > > + /*allow_non_constant_p=*/false, > > + /*non_constant_p=*/nullptr, > > + /*strict_p=*/true); > > + if (annotation == error_mark_node) > > + break; > > + if (cp_lexer_next_token_is (parser->lexer, CPP_ELLIPSIS)) > > + cp_lexer_consume_token (parser->lexer); > > + token = cp_lexer_peek_token (parser->lexer); > > + if (token->type != CPP_COMMA) > > + break; > > + cp_lexer_consume_token (parser->lexer); > > + continue; > > + } > > If we're going to do the normal parsing in this erroneous case, let's factor > it out of cp_parser_annotation_list so we can use it here as well. Done in <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > > @@ -12766,7 +12820,11 @@ instantiate_class_template (tree type) > > if (base == error_mark_node) > > continue; > > - base_list = tree_cons (access, base, base_list); > > + tree acc = access; > > + if (annotations) > > + acc = build_tree_list (access, idx == 0 ? annotations > > + : copy_list (annotations)); > > The copy_list could use a comment; I take it that we want the annotations > for each element of a base-specifier pack-expansion to have annotations > distinct from those of the other elements? Comment added in <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > > @@ -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? Resolved in another thread. > > @@ -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. Done in <https://forge.sourceware.org/marek/gcc/commit/f6bab77b339cb1b5b7390d119ce63feec4b28b02> > > @@ -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? Should be done in an incremental patch by using attribute_value_equal. I'm not finding that patch though. > > @@ -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? It doesn't seem to be needed anymore. Removed in <https://forge.sourceware.org/marek/gcc/commit/5687b6ef43f67712178bfc68a1e1dcbdd55d0577> > > for (i = 0; i < n; i++) > > { > > tree arg = CALL_EXPR_ARG (function, i);> @@ -7368,6 +7368,13 @@ > > build_new_op (const op_location_t &loc, enum > tree_code code, int flags, > > case LE_EXPR: > > case EQ_EXPR: > > case NE_EXPR: > > + if (!arg1_type || !arg2_type) > > + { > > + /* Something is very wrong. Perhaps we're trying to compare > > + type nodes. Make sure we've complained. */ > > + gcc_assert (seen_error ()); > > + return error_mark_node; > > + } > > This seems like a temporary workaround that doesn't belong in trunk. Removed in <https://forge.sourceware.org/marek/gcc/commit/bf3019d9a78de0946f711a1100fd45822720d088> > > + /* 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? Still WIP, being discussed in <https://gcc.gnu.org/pipermail/gcc-patches/2025-December/702936.html> > > @@ -12563,6 +12755,10 @@ potential_constant_expression_1 (tree t, bool > > want_rval, bool strict, bool now, > > case TU_LOCAL_ENTITY: > > return false; > > + /* A splice expression is dependent, so not constant. */ > > + case SPLICE_EXPR: > > + return false; > > A TEMPLATE_PARM_INDEX is also dependent, but it is potentially-constant > because it will be constant after substitution. The same seems to apply to > splices; I would expect this to return true. Fixed in <https://forge.sourceware.org/marek/gcc/commit/663ef75f770f1a19034c7534e22c5210f5ebe848> > > +/* For every DECL_EXPR check if it declares a consteval-only variable and > > + if so, overwrite it with a no-op. The point here is not to leak > > + consteval-only variables into the middle end. */ > > + > > +static tree > > +wipe_consteval_only_r (tree *stmt_p, int *, void *) > > +{ > > + if (TREE_CODE (*stmt_p) == DECL_EXPR) > > + { > > + tree d = DECL_EXPR_DECL (*stmt_p); > > + if (VAR_P (d) && consteval_only_p (d)) > > + /* Wipe the DECL_EXPR so that it doesn't get into gimple. */ > > + *stmt_p = build1 (NOP_EXPR, void_type_node, integer_zero_node); > > Why not void_node? Likewise for the other clobbers in fold_immediate. I think I copied that from somewhere. Changed in <https://forge.sourceware.org/marek/gcc/commit/1997a08f0ab5f80ae678a1c7c485db36b41b2930> > > +/* Helper macro to set SPLICE_EXPR_EXPRESSION_P. */ > > +#define SET_SPLICE_EXPR_EXPRESSION_P(NODE) \ > > + (SPLICE_EXPR_EXPRESSION_P (TREE_CODE (NODE) == SPLICE_EXPR \ > > + ? NODE : TREE_OPERAND (NODE, 0)) = true) > > What are these helpers for? What is the node that we expect to have a > splice in op0? These are to handle dependent_splice_p trees: either [:T:] or [:T:]<arg>, and I wanted one macro for both. The second one is a TEMPLATE_ID_EXPR. > > @@ -5514,6 +5582,15 @@ cxx_init_decl_processing (void) > > /* Create the `std' namespace. */ > > push_namespace (get_identifier ("std")); > > std_node = current_namespace; > > + if (flag_reflection) > > + { > > + /* Note that we haven't initialized void_type_node yet, so > > + std_meta_node will be initially typeless; its type will be > > + set a little later in init_reflection. */ > > + push_namespace (get_identifier ("meta"), /*inline*/false); > > + std_meta_node = current_namespace; > > + pop_namespace (); > > + } > > ??? std_meta_node is a namespace, which are always typeless. This was an ICE in is_std_substitution on: if (!(TYPE_LANG_SPECIFIC (type) && TYPE_TEMPLATE_INFO (type))) where decl=namespace_decl meta and type was null due to the ordering issue mentioned in the comment. make_namespace uses void_type_node to build a namespace_decl, so all other namespaces have a non-null type. > > @@ -9808,6 +9918,20 @@ cp_finish_decl (tree decl, tree init, bool > > init_const_expr_p, > > record_types_used_by_current_var_decl (decl); > > } > > + /* CWG 3115: Every function of consteval-only type shall be an > > + immediate function. */ > > + if (TREE_CODE (decl) == FUNCTION_DECL > > + && !DECL_IMMEDIATE_FUNCTION_P (decl) > > + && consteval_only_p (decl) > > + /* But if the function can be escalated, merrily we roll along. */ > > + && !immediate_escalating_function_p (decl)) > > + { > > + error_at (DECL_SOURCE_LOCATION (decl), > > + "function of consteval-only type must be declared %qs", > > + "consteval"); > > + return; > > + } > > @@ -20471,6 +20641,16 @@ finish_function (bool inline_p) > > unused_but_set_errorcount = errorcount; > > } > > + /* CWG 3115: Every function of consteval-only type shall be an immediate > > + function. */ > > + if (!DECL_IMMEDIATE_FUNCTION_P (fndecl) > > + && consteval_only_p (fndecl) > > + && !immediate_escalating_function_p (fndecl) > > + && !is_std_allocator_allocate (fndecl)) > > + error_at (DECL_SOURCE_LOCATION (fndecl), > > + "function of consteval-only type must be declared %qs", > > + "consteval"); > > Do we need this in both places? I think so, one is for fn decls, the other one for fn defs. > > @@ -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? Comments that this is about multiple virtual inheritance were added. > > + c = BINFO_INHERITANCE_CHAIN (c); > > + tpvis = type_visibility (BINFO_TYPE (c)); > > + *walk_subtrees = 0; > > + break; > > + case REFLECT_DATA_MEMBER_SPEC: > > + /* For data member description determine visibility > > + from the type. */ > > + tpvis = type_visibility (TREE_VEC_ELT (r, 0)); > > + *walk_subtrees = 0; > > + break; > > + case REFLECT_PARM: > > + /* For function parameter reflection determine visibility > > + based on parent_of. */ > > + tpvis = expr_visibility (DECL_CONTEXT (r)); > > + *walk_subtrees = 0; > > + break; > > + case REFLECT_OBJECT: > > + c = get_base_address (r); > > + if ((VAR_P (c) && decl_function_context (c)) > > + || TREE_CODE (c) == PARM_DECL) > > + { > > + /* Make reflect_object of block scope variables > > + subobjects local. */ > > + tpvis = VISIBILITY_ANON; > > + *walk_subtrees = 0; > > + } > > We just stopped treating local declarations as VISIBILITY_ANON in > r16-5024-gf062a6b7985fce -- for this to differ from that, we need more > rationale. E.g. in template <int N, decltype(^^::) I> void foo () { } static void bar () { int v = 42; foo <101, ^^v> (); // #1 } #1 shouldn't be exported. If I follow r16-5024, decl_linkage will give me lk_none for 'v', as it should (it's a var at block scope), but type_visibility of its type says VISIBILITY_DEFAULT > > + break; > > + case REFLECT_ANNOTATION: > > + /* Annotations are always local to the TU. */ > > + tpvis = VISIBILITY_ANON; > > + *walk_subtrees = 0; > > + break; > > + default: > > + if (TYPE_P (r)) > > + { > > + tpvis = type_visibility (r); > > + *walk_subtrees = 0; > > + break; > > + } > > + if ((VAR_P (r) && decl_function_context (r)) > > + || TREE_CODE (r) == PARM_DECL) > > + { > > + /* Block scope variables are local to the TU. */ > > + tpvis = VISIBILITY_ANON; > > + *walk_subtrees = 0; > > + break; > > + } > > As above, and they should share code, perhaps by the REFLECT_OBJECT case > falling through to the default? Adjusted in <https://forge.sourceware.org/marek/gcc/commit/65b8fb9ba50912fccd7b0931218b04302ac5af8a> > > @@ -5130,6 +5219,12 @@ no_linkage_error (tree decl) > > /* An imported decl is ok. */ > > return; > > + /* Metafunctions are magic and should be considered defined even though > > + they have no bodies. ??? This can't be checked in decl_defined_p; > > + we'd get redefinition errors for some of our metafunctions. */ > > + if (TREE_CODE (decl) == FUNCTION_DECL && metafunction_p (decl)) > > + return; > > Why would we complain about metafunctions without this change? This was due to: #include <meta> using namespace std::meta; template<typename T> consteval bool roundtrip (T value) { return extract<T>(reflect_constant (value)) == value; } static_assert (roundtrip ([] {})); where no_linkage_error complained: .../libstdc++-v3/include/meta:380:22: error: ‘consteval std::meta::info std::meta::reflect_constant(_Tp) [with _Tp = <lambda()>; info = std::meta::info]’, declared using local type ‘<lambda()>’, is used but never defined [-fpermissive] 380 | consteval info reflect_constant(_Tp); reflect_constant et al aren't defined but "hijacked" and processed in constexpr. > > @@ -352,6 +352,10 @@ wrapup_global_declaration_2 (tree decl) > > || (VAR_P (decl) && DECL_HAS_VALUE_EXPR_P (decl))) > > return false; > > + /* Compile-only variables are not to be written out. */ > > + if (lang_hooks.compile_only_p (decl)) > > + return false; > > Why don't we just set DECL_EXTERNAL instead of adding a hook? Ah lovely. Langhook removed in <https://forge.sourceware.org/marek/gcc/commit/a5b522cd5fecdb987e8874849e4a4d75fe2591ab> With DECL_EXTERNAL we have to be careful not to set it too early because that results in "non-constant" errors. But setting it in make_rtl_for_nonlocal_decl seems to work. > Not done, but this seems enough for a first pass. Thanks a lot. I won't post a new series until the comments in <https://gcc.gnu.org/pipermail/gcc-patches/2025-December/702950.html> are addressed as well. Marek
