The 08/07/2025 15:44, Richard Sandiford wrote:
> <alfie.richa...@arm.com> writes:
> > From: Alfie Richards <alfie.richa...@arm.com>
> >
> > This patch introduces the TARGET_CHECK_TARGET_CLONE_VERSION hook
> > which is used to determine if a target_clones version string parses.
> >
> > The hook has a flag to enable emitting diagnostics.
> >
> > This is as specified in the Arm C Language Extension. The purpose of this
> > is to be able to ignore invalid versions to allow some portability of code
> > using target_clones attributes.
> >
> > Currently this is only properly implemented for the Aarch64 backend.
> >
> > For riscv which is the only other backend which uses target_version
> > semantics a partial implementation is present, where this hook is used
> > to check parsing, in which errors will be emitted on a failed parse
> > rather than warnings. A refactor of the riscv parsing logic would be
> > required to enable this functionality fully.
> >
> > This fixes PR 118339 where parse failures could cause ICE in Aarch64.
> >
> > gcc/ChangeLog:
> >
> > PR target/118339
> > * target.def: Add check_target_clone_version hook.
> > * tree.cc (get_clone_attr_versions): Add filter and location argument.
> > (get_clone_versions): Add filter argument.
> > * tree.h (get_clone_attr_versions): Add filter and location argument.
> > (get_clone_versions): Add filter argument.
> > * config/aarch64/aarch64.cc (aarch64_check_target_clone_version):
> > New function
> > (TARGET_CHECK_TARGET_CLONE_VERSION): New define.
> > * config/riscv/riscv.cc (riscv_check_target_clone_version):
> > New function.
> > (TARGET_CHECK_FUNCTION_CLONE_VERSION): New define.
> > * doc/tm.texi: Regenerated.
> > * doc/tm.texi.in: Add documentation for new hook.
> > * hooks.h (hook_stringslice_locationt_boolean_false): New function.
> > * hooks.cc (hook_stringslice_locationt_boolean_false): New function.
> >
> > gcc/c-family/ChangeLog:
> >
> > * c-attribs.cc (handle_target_clones_attribute): Update to use new hook.
> > ---
> > gcc/c-family/c-attribs.cc | 24 ++++++++++++----
> > gcc/config/aarch64/aarch64.cc | 53 +++++++++++++++++++++++++++++++++++
> > gcc/config/riscv/riscv.cc | 22 +++++++++++++++
> > gcc/doc/tm.texi | 5 ++++
> > gcc/doc/tm.texi.in | 2 ++
> > gcc/hooks.cc | 8 ++++++
> > gcc/hooks.h | 3 ++
> > gcc/target.def | 9 ++++++
> > gcc/tree.cc | 15 ++++++++--
> > gcc/tree.h | 15 +++++++---
> > 10 files changed, 144 insertions(+), 12 deletions(-)
> >
> > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> > index a0d832b5e05..ce31fa2fcaf 100644
> > --- a/gcc/c-family/c-attribs.cc
> > +++ b/gcc/c-family/c-attribs.cc
> > @@ -6238,12 +6238,26 @@ handle_target_clones_attribute (tree *node, tree
> > name, tree ARG_UNUSED (args),
> > }
> > }
> >
> > - auto_vec<string_slice> versions = get_clone_attr_versions (args,
> > NULL);
> > -
> > - if (versions.length () == 1)
> > + int num_defaults = 0;
> > + auto_vec<string_slice> versions = get_clone_attr_versions
> > + (args,
> > + &num_defaults,
> > + DECL_SOURCE_LOCATION (*node),
> > + false);
> > +
> > + for (auto v : versions)
> > + targetm.check_target_clone_version
> > + (v, DECL_SOURCE_LOCATION (*node), true);
> > +
> > + /* Lone target_clones version is always ignored for target attr
> > semantics.
> > + Only ignore under target_version semantics if it is a default
> > + version. */
> > + if (versions.length () == 1
> > + && (TARGET_HAS_FMV_TARGET_ATTRIBUTE || num_defaults == 1))
> > {
> > - warning (OPT_Wattributes,
> > - "single %<target_clones%> attribute is ignored");
> > + if (TARGET_HAS_FMV_TARGET_ATTRIBUTE)
> > + warning (OPT_Wattributes,
> > + "single %<target_clones%> attribute is ignored");
> > *no_add_attrs = true;
> > }
> > else
> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > index 44b14fc6f45..2029a851396 100644
> > --- a/gcc/config/aarch64/aarch64.cc
> > +++ b/gcc/config/aarch64/aarch64.cc
> > @@ -31807,6 +31807,56 @@ aarch64_expand_fp_spaceship (rtx dest, rtx op0,
> > rtx op1, rtx hint)
> > }
> > }
> >
> > +/* Checks if the function version string STR parses correctly.
> > + If it is an invalid string, returns true to reject the string, otherwise
> > + returns false. */
> > +
> > +bool
> > +aarch64_check_target_clone_version (string_slice str,
> > + location_t loc,
> > + bool emit_diag)
>
> Sorry to be awkward, but how about passing a location_t * instead
> of separate location_t and bool parameters, with nullptr meaning no
> diagnostic? I know that isn't what I suggested last time, but seeing
> the interface in action, it seemed a bit clunky that a location was
> required even when a diagnostic wasn't.
>
> I realise that'll involve more changes to the RISC-V code. I'm happy
> to do that if you'd prefer, since it's a sort of feature creep.
I changed the RISC-V hook to use UNKNOWN_LOCATION if loc is NULL.
I think this should be okay for now? It should never actually get
to the code path with an error where loc is NULL as the error emitted
for an invalid version string at handle_target_clones_attributs
stops compilation so it never gets to the later uses of this funciton.
>
> > +{
> > + str = str.strip ();
> > +
> > + if (str == "default")
> > + return true;
> > +
> > + enum aarch_parse_opt_result parse_res;
> > + auto isa_flags = aarch64_asm_isa_flags;
> > + std::string invalid_extension;
> > +
> > + parse_res
> > + = aarch64_parse_fmv_features (str, &isa_flags, NULL,
> > &invalid_extension);
> > +
> > + if (!emit_diag)
> > + return parse_res == AARCH_PARSE_OK;
> > +
> > + switch (parse_res)
> > + {
> > + case AARCH_PARSE_OK:
> > + return true;
> > + case AARCH_PARSE_MISSING_ARG:
> > + warning_at (loc, OPT_Wattributes,
> > + "empty string not valid for a %<target_clones%> version");
> > + break;
> > + case AARCH_PARSE_INVALID_FEATURE:
> > + warning_at (loc, OPT_Wattributes,
> > + "invalid feature modifier %qs in version %qB for "
> > + "%<target_clones%> attribute",
> > + invalid_extension.c_str (), &str);
> > + break;
> > + case AARCH_PARSE_DUPLICATE_FEATURE:
> > + warning_at (loc, OPT_Wattributes,
> > + "duplicate feature modifier %qs in version %qB for "
> > + "%<target_clones%> attribute",
> > + invalid_extension.c_str (), &str);
> > + break;
> > + default:
> > + gcc_unreachable ();
> > + }
> > + return false;
>
> I think for this kind of thing it's better to make the cases return false,
> to remove the default, and to put the gcc_unreachable () after the switch
> statement. That way we'll get a warning if there's a missing case.
>
> > +}
> > +
> > /* Target-specific selftests. */
> >
> > #if CHECKING_P
> > @@ -32664,6 +32714,9 @@ aarch64_libgcc_floating_mode_supported_p
> > #undef TARGET_OPTION_FUNCTION_VERSIONS
> > #define TARGET_OPTION_FUNCTION_VERSIONS aarch64_common_function_versions
> >
> > +#undef TARGET_CHECK_TARGET_CLONE_VERSION
> > +#define TARGET_CHECK_TARGET_CLONE_VERSION
> > aarch64_check_target_clone_version
> > +
> > #undef TARGET_COMPARE_VERSION_PRIORITY
> > #define TARGET_COMPARE_VERSION_PRIORITY aarch64_compare_version_priority
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 38fce5519da..90fd7d95c57 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -14153,6 +14153,25 @@ riscv_common_function_versions (tree fn1, tree fn2)
> > return riscv_compare_version_priority (fn1, fn2) != 0;
> > }
> >
> > +/* Checks if the function version specifying string STR parses correctly.
> > + If it is an invalid string, currently emits a diagnostic at LOC.
> > + Always returns false. */
> > +
> > +bool
> > +riscv_verify_target_clone_version (string_slice str, location_t loc)
> > +{
> > + struct riscv_feature_bits mask;
> > + int prio;
> > +
> > + /* Currently it is not possible to parse without emitting errors on
> > failure
> > + so do not reject on a failed parse, as this would then emit two
> > + diagnostics. Instead let errors be emitted which will halt
> > + compilation. */
> > + parse_features_for_version (str, loc, mask, prio);
> > +
> > + return true;
> > +}
> > +
> > /* Implement TARGET_MANGLE_DECL_ASSEMBLER_NAME, to add function
> > multiversioning
> > suffixes. */
> >
> > @@ -15785,6 +15804,9 @@ synthesize_and (rtx operands[3])
> > #undef TARGET_COMPARE_VERSION_PRIORITY
> > #define TARGET_COMPARE_VERSION_PRIORITY riscv_compare_version_priority
> >
> > +#undef TARGET_VERIFY_TARGET_CLONE_VERSION
> > +#define TARGET_VERIFY_TARGET_CLONE_VERSION riscv_check_target_clone_version
> > +
>
> s/VERIFY/CHECK/ in the macro name and s/verify/check/ in the function
> definition. The interface above is missing the bool parameter.
>
> > #undef TARGET_OPTION_FUNCTION_VERSIONS
> > #define TARGET_OPTION_FUNCTION_VERSIONS riscv_common_function_versions
> >
> > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> > index 4d4e676aadf..e91a1550ff8 100644
> > --- a/gcc/doc/tm.texi
> > +++ b/gcc/doc/tm.texi
> > @@ -12279,6 +12279,11 @@ function version at run-time for a given set of
> > function versions.
> > body must be generated.
> > @end deftypefn
> >
> > +@deftypefn {Target Hook} bool TARGET_CHECK_TARGET_CLONE_VERSION
> > (string_slice @var{str}, location_t @var{loc}, @var{bool})
> > +This hook is used to check if a version specified in a @code{target_clones}
> > +annotation is valid. @var{str} is the version to be considered.
> > +@end deftypefn
> > +
> > @deftypefn {Target Hook} bool TARGET_PREDICT_DOLOOP_P (class loop
> > *@var{loop})
> > Return true if we can predict it is possible to use a low-overhead loop
> > for a particular loop. The parameter @var{loop} is a pointer to the loop.
> > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> > index 1a51ad54817..eaab62652a7 100644
> > --- a/gcc/doc/tm.texi.in
> > +++ b/gcc/doc/tm.texi.in
> > @@ -7887,6 +7887,8 @@ to by @var{ce_info}.
> >
> > @hook TARGET_GENERATE_VERSION_DISPATCHER_BODY
> >
> > +@hook TARGET_CHECK_TARGET_CLONE_VERSION
> > +
> > @hook TARGET_PREDICT_DOLOOP_P
> >
> > @hook TARGET_HAVE_COUNT_REG_DECR_P
> > diff --git a/gcc/hooks.cc b/gcc/hooks.cc
> > index 76cb593103d..640759881d3 100644
> > --- a/gcc/hooks.cc
> > +++ b/gcc/hooks.cc
> > @@ -585,3 +585,11 @@ hook_optmode_mode_uhwi_none (machine_mode, unsigned
> > HOST_WIDE_INT)
> > {
> > return opt_machine_mode ();
> > }
> > +
> > +/* Generic hook that takes a string_slice and a locations and returns
> > false. */
> > +
> > +bool
> > +hook_stringslice_locationt_bool_true (string_slice, location_t, bool)
> > +{
> > + return true;
> > +}
> > diff --git a/gcc/hooks.h b/gcc/hooks.h
> > index e95bd11aca8..038598499a6 100644
> > --- a/gcc/hooks.h
> > +++ b/gcc/hooks.h
> > @@ -137,4 +137,7 @@ extern const char
> > *hook_constcharptr_int_const_tree_const_tree_null (int, const_
> >
> > extern opt_machine_mode hook_optmode_mode_uhwi_none (machine_mode,
> > unsigned HOST_WIDE_INT);
> > +
> > +extern bool hook_stringslice_locationt_bool_true (string_slice,
> > location_t, bool);
> > +
> > #endif
> > diff --git a/gcc/target.def b/gcc/target.def
> > index 5dd8f253ef6..96643612fb5 100644
> > --- a/gcc/target.def
> > +++ b/gcc/target.def
> > @@ -2606,6 +2606,15 @@ version at run-time. @var{decl} is one version from
> > a set of semantically\n\
> > identical versions.",
> > tree, (void *decl), NULL)
> >
> > +/* Target hook is used to ignore certain versions specified in a
> > target_clones
> > + annoration. STR is the version to be considered. */
> > +DEFHOOK
> > +(check_target_clone_version ,
> > + "This hook is used to check if a version specified in a
> > @code{target_clones}\n\
> > +annotation is valid. @var{str} is the version to be considered.",
>
> The other parameters should be documented too, as well as the return value.
>
> > + bool, (string_slice str, location_t loc, bool),
> > + hook_stringslice_locationt_bool_true)
> > +
> > /* Returns a code for a target-specific builtin that implements
> > reciprocal of a target-specific function, or NULL_TREE if not
> > available. */
> > DEFHOOK
> > diff --git a/gcc/tree.cc b/gcc/tree.cc
> > index 2335a6993b9..a50c2f5163b 100644
> > --- a/gcc/tree.cc
> > +++ b/gcc/tree.cc
> > @@ -15398,7 +15398,10 @@ get_attr_nonstring_decl (tree expr, tree *ref)
> > ARGLIST. DEFAULT_COUNT is incremented for each default version found.
> > */
> >
> > auto_vec<string_slice>
> > -get_clone_attr_versions (const tree arglist, int *default_count)
> > +get_clone_attr_versions (const tree arglist,
> > + int *default_count,
> > + location_t loc,
> > + bool filter)
>
> The function comment should document FILTER.
>
> > {
> > gcc_assert (TREE_CODE (arglist) == TREE_LIST);
> > auto_vec<string_slice> versions;
> > @@ -15414,6 +15417,9 @@ get_clone_attr_versions (const tree arglist, int
> > *default_count)
> > string_slice attr = string_slice::tokenize (&str, separators);
> > attr = attr.strip ();
> >
> > + if (filter && !targetm.check_target_clone_version (attr, loc, false))
> > + continue;
> > +
> > if (attr == "default" && default_count)
> > (*default_count)++;
> > versions.safe_push (attr);
> > @@ -15426,13 +15432,16 @@ get_clone_attr_versions (const tree arglist, int
> > *default_count)
> > the target_clone attribute from DECL. DEFAULT_COUNT is incremented for
> > each
> > default version found. */
> > auto_vec<string_slice>
> > -get_clone_versions (const tree decl, int *default_count)
> > +get_clone_versions (const tree decl, int *default_count, bool filter)
>
> Same here.
>
> Thanks,
> Richard
>
> > {
> > tree attr = lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl));
> > if (!attr)
> > return auto_vec<string_slice> ();
> > tree arglist = TREE_VALUE (attr);
> > - return get_clone_attr_versions (arglist, default_count);
> > + return get_clone_attr_versions (arglist,
> > + default_count,
> > + DECL_SOURCE_LOCATION (decl),
> > + filter);
> > }
> >
> > /* If DECL has a target_version attribute, returns a string_slice
> > containing the
> > diff --git a/gcc/tree.h b/gcc/tree.h
> > index eadf0603066..c239b3505e5 100644
> > --- a/gcc/tree.h
> > +++ b/gcc/tree.h
> > @@ -7091,10 +7091,17 @@ extern tree get_attr_nonstring_decl (tree, tree * =
> > NULL);
> > Returns an invalid string_slice if no attribute is present. */
> > extern string_slice get_target_version (const tree);
> > /* Returns a vector of the version strings from a target_clones attribute
> > on
> > - a decl. Can also record the number of default versions found. */
> > -extern auto_vec<string_slice> get_clone_versions (const tree, int * =
> > NULL);
> > + a decl. Can also record the number of default versions found.
> > + Use bool to control whether or not the results should
> > + be filtered with TARGET_CHECK_TARGET_CLONE_VERSION. */
> > +extern auto_vec<string_slice> get_clone_versions
> > + (const tree,int * = NULL, bool = true);
> > /* Returns a vector of the version strings from a target_clones attribute
> > - directly. */
> > -extern auto_vec<string_slice> get_clone_attr_versions (const tree, int *);
> > + directly. Additionally takes a location for potential diagnostics to be
> > + emitted for and a bool to control whether or not the results should
> > + be filtered with TARGET_CHECK_TARGET_CLONE_VERSION. */
> > +extern auto_vec<string_slice> get_clone_attr_versions (const tree, int *,
> > + location_t loc,
> > + bool = true);
> >
> > #endif /* GCC_TREE_H */
--
Alfie Richards
