diff mbox series

[v3,05/11] RISC-V: Implement TARGET_COMPARE_VERSION_PRIORITY and TARGET_OPTION_FUNCTION_VERSIONS

Message ID tencent_678E26B89931CE21728D777CFA825CDA1408@qq.com
State New
Headers show
Series RISC-V: Add Function Multi-Versioning support | expand

Commit Message

Yangyu Chen Oct. 24, 2024, 7:11 a.m. UTC
This patch implements TARGET_COMPARE_VERSION_PRIORITY and
TARGET_OPTION_FUNCTION_VERSIONS for RISC-V.

The TARGET_COMPARE_VERSION_PRIORITY is implemented to compare the
priority of two function versions based on the rules defined in the
RISC-V C-API Doc PR #85:

https://github.com/riscv-non-isa/riscv-c-api-doc/pull/85/files#diff-79a93ca266139524b8b642e582ac20999357542001f1f4666fbb62b6fb7a5824R721

If multiple versions have equal priority, we select the function with
the most number of feature bits generated by
riscv_minimal_hwprobe_feature_bits. When it comes to the same number of
feature bits, we diff two versions and select the one with the least
significant bit set. Since a feature appears earlier in the feature_bits
might be more important to performance.

The TARGET_OPTION_FUNCTION_VERSIONS is implemented to check whether the
two function versions are the same. This Implementation reuses the code
in TARGET_COMPARE_VERSION_PRIORITY and check it returns 0, which means
the equal priority.

Co-Developed-by: Hank Chang <hank.chang@sifive.com>

gcc/ChangeLog:

	* config/riscv/riscv-target-attr.cc
	(riscv_target_attr_parser::update_settings): never free the
	arch string.
	* config/riscv/riscv.cc
	(parse_features_for_version): New function.
	(compare_fmv_features): New function.
	(riscv_compare_version_priority): New function.
	(riscv_common_function_versions): New function.
	(TARGET_COMPARE_VERSION_PRIORITY): Implement it.
	(TARGET_OPTION_FUNCTION_VERSIONS): Implement it.
	* config/riscv/riscv.h (TARGET_HAS_FMV_TARGET_ATTRIBUTE): Define
	it to 0 to use "target_version" for function versioning.
---
 gcc/config/riscv/riscv-target-attr.cc |   4 -
 gcc/config/riscv/riscv.cc             | 127 ++++++++++++++++++++++++++
 gcc/config/riscv/riscv.h              |   2 +
 3 files changed, 129 insertions(+), 4 deletions(-)

Comments

Kito Cheng Oct. 31, 2024, 9:32 a.m. UTC | #1
TARGET_HAS_FMV_TARGET_ATTRIBUTE should be a separate patch? it seems
not related to TARGET_OPTION_FUNCTION_VERSIONS and
TARGET_COMPARE_VERSION_PRIORITY?


On Thu, Oct 24, 2024 at 3:13 PM Yangyu Chen <cyy@cyyself.name> wrote:
>
> This patch implements TARGET_COMPARE_VERSION_PRIORITY and
> TARGET_OPTION_FUNCTION_VERSIONS for RISC-V.
>
> The TARGET_COMPARE_VERSION_PRIORITY is implemented to compare the
> priority of two function versions based on the rules defined in the
> RISC-V C-API Doc PR #85:
>
> https://github.com/riscv-non-isa/riscv-c-api-doc/pull/85/files#diff-79a93ca266139524b8b642e582ac20999357542001f1f4666fbb62b6fb7a5824R721
>
> If multiple versions have equal priority, we select the function with
> the most number of feature bits generated by
> riscv_minimal_hwprobe_feature_bits. When it comes to the same number of
> feature bits, we diff two versions and select the one with the least
> significant bit set. Since a feature appears earlier in the feature_bits
> might be more important to performance.
>
> The TARGET_OPTION_FUNCTION_VERSIONS is implemented to check whether the
> two function versions are the same. This Implementation reuses the code
> in TARGET_COMPARE_VERSION_PRIORITY and check it returns 0, which means
> the equal priority.
>
> Co-Developed-by: Hank Chang <hank.chang@sifive.com>
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv-target-attr.cc
>         (riscv_target_attr_parser::update_settings): never free the
>         arch string.
>         * config/riscv/riscv.cc
>         (parse_features_for_version): New function.
>         (compare_fmv_features): New function.
>         (riscv_compare_version_priority): New function.
>         (riscv_common_function_versions): New function.
>         (TARGET_COMPARE_VERSION_PRIORITY): Implement it.
>         (TARGET_OPTION_FUNCTION_VERSIONS): Implement it.
>         * config/riscv/riscv.h (TARGET_HAS_FMV_TARGET_ATTRIBUTE): Define
>         it to 0 to use "target_version" for function versioning.
> ---
>  gcc/config/riscv/riscv-target-attr.cc |   4 -
>  gcc/config/riscv/riscv.cc             | 127 ++++++++++++++++++++++++++
>  gcc/config/riscv/riscv.h              |   2 +
>  3 files changed, 129 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
> index 087fbae77b0..4c85ad60b72 100644
> --- a/gcc/config/riscv/riscv-target-attr.cc
> +++ b/gcc/config/riscv/riscv-target-attr.cc
> @@ -239,10 +239,6 @@ riscv_target_attr_parser::update_settings (struct gcc_options *opts) const
>      {
>        std::string local_arch = m_subset_list->to_string (true);
>        const char* local_arch_str = local_arch.c_str ();
> -      struct cl_target_option *default_opts
> -       = TREE_TARGET_OPTION (target_option_default_node);
> -      if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
> -       free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));
>        opts->x_riscv_arch_string = xstrdup (local_arch_str);
>
>        riscv_set_arch_by_subset_list (m_subset_list, opts);
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 3ac40234345..947864fc3a6 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -12574,6 +12574,127 @@ riscv_c_mode_for_floating_type (enum tree_index ti)
>    return default_mode_for_floating_type (ti);
>  }
>
> +/* This parses the attribute arguments to target_version in DECL and modifies
> +   the feature mask and priority required to select those targets.  */
> +static void
> +parse_features_for_version (tree decl,
> +                           struct riscv_feature_bits &res,
> +                           int &priority)
> +{
> +  tree version_attr = lookup_attribute ("target_version",
> +                                       DECL_ATTRIBUTES (decl));
> +  if (version_attr == NULL_TREE)
> +    {
> +      res.length = 0;
> +      priority = 0;
> +      return;
> +    }
> +
> +  const char *version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
> +                                                   (version_attr)));
> +  gcc_assert (version_string != NULL);
> +  if (strcmp (version_string, "default") == 0)
> +    {
> +      res.length = 0;
> +      priority = 0;
> +      return;
> +    }
> +  struct cl_target_option cur_target;
> +  cl_target_option_save (&cur_target, &global_options,
> +                        &global_options_set);
> +  /* Always set to default option before parsing "arch=+..."  */
> +  struct cl_target_option *default_opts
> +       = TREE_TARGET_OPTION (target_option_default_node);
> +  cl_target_option_restore (&global_options, &global_options_set,
> +                           default_opts);
> +
> +  riscv_process_target_attr (version_string,
> +                            DECL_SOURCE_LOCATION (decl));
> +
> +  priority = global_options.x_riscv_fmv_priority;
> +  const char *arch_string = global_options.x_riscv_arch_string;
> +  bool parse_res
> +    = riscv_minimal_hwprobe_feature_bits (arch_string, &res,
> +                                         DECL_SOURCE_LOCATION (decl));
> +  gcc_assert (parse_res);
> +
> +  if (arch_string != default_opts->x_riscv_arch_string)
> +    free (CONST_CAST (void *, (const void *) arch_string));
> +
> +  cl_target_option_restore (&global_options, &global_options_set,
> +                           &cur_target);
> +}
> +
> +/* Compare priorities of two feature masks.  Return:
> +     1: mask1 is higher priority
> +    -1: mask2 is higher priority
> +     0: masks are equal.
> +   Since riscv_feature_bits has total 128 bits to be used as mask, when counting
> +   the total 1s in the mask, the 1s in group1 needs to multiply a weight.  */
> +static int
> +compare_fmv_features (const struct riscv_feature_bits &mask1,
> +                     const struct riscv_feature_bits &mask2,
> +                     int prio1, int prio2)
> +{
> +  unsigned length1 = mask1.length, length2 = mask2.length;
> +  /* 1.  Compare length, for length == 0 means default version, which should be
> +        the lowest priority).  */
> +  if (length1 != length2)
> +    return length1 > length2 ? 1 : -1;
> +  /* 2.  Compare the priority.  */
> +  if (prio1 != prio2)
> +    return prio1 > prio2 ? 1 : -1;
> +  /* 3.  Compare the total number of 1s in the mask.  */
> +  unsigned pop1 = 0, pop2 = 0;
> +  for (unsigned i = 0; i < length1; i++)
> +    {
> +      pop1 += __builtin_popcountll (mask1.features[i]);
> +      pop2 += __builtin_popcountll (mask2.features[i]);
> +    }
> +  if (pop1 != pop2)
> +    return pop1 > pop2 ? 1 : -1;
> +  /* 4.  Compare the mask bit by bit order.  */
> +  for (unsigned i = 0; i < length1; i++)
> +    {
> +      unsigned long long xor_mask = mask1.features[i] ^ mask2.features[i];
> +      if (xor_mask == 0)
> +       continue;
> +      return TEST_BIT (mask1.features[i], __builtin_ctzll (xor_mask)) ? 1 : -1;
> +    }
> +  /* 5.  If all bits are equal, return 0.  */
> +  return 0;
> +}
> +
> +/* Compare priorities of two version decls.  Return:
> +     1: mask1 is higher priority
> +    -1: mask2 is higher priority
> +     0: masks are equal.  */
> +int
> +riscv_compare_version_priority (tree decl1, tree decl2)
> +{
> +  struct riscv_feature_bits mask1, mask2;
> +  int prio1, prio2;
> +
> +  parse_features_for_version (decl1, mask1, prio1);
> +  parse_features_for_version (decl2, mask2, prio2);
> +
> +  return compare_fmv_features (mask1, mask2, prio1, prio2);
> +}
> +
> +/* This function returns true if FN1 and FN2 are versions of the same function,
> +   that is, the target_version attributes of the function decls are different.
> +   This assumes that FN1 and FN2 have the same signature.  */
> +
> +bool
> +riscv_common_function_versions (tree fn1, tree fn2)
> +{
> +  if (TREE_CODE (fn1) != FUNCTION_DECL
> +      || TREE_CODE (fn2) != FUNCTION_DECL)
> +    return false;
> +
> +  return riscv_compare_version_priority (fn1, fn2) != 0;
> +}
> +
>  /* On riscv we have an ABI defined safe buffer.  This constant is used to
>     determining the probe offset for alloca.  */
>
> @@ -12948,6 +13069,12 @@ riscv_stack_clash_protection_alloca_probe_range (void)
>  #undef TARGET_C_MODE_FOR_FLOATING_TYPE
>  #define TARGET_C_MODE_FOR_FLOATING_TYPE riscv_c_mode_for_floating_type
>
> +#undef TARGET_COMPARE_VERSION_PRIORITY
> +#define TARGET_COMPARE_VERSION_PRIORITY riscv_compare_version_priority
> +
> +#undef TARGET_OPTION_FUNCTION_VERSIONS
> +#define TARGET_OPTION_FUNCTION_VERSIONS riscv_common_function_versions
> +
>  struct gcc_target targetm = TARGET_INITIALIZER;
>
>  #include "gt-riscv.h"
> diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> index 2ff9c1024f3..8a8b08b6b51 100644
> --- a/gcc/config/riscv/riscv.h
> +++ b/gcc/config/riscv/riscv.h
> @@ -1303,4 +1303,6 @@ extern void riscv_remove_unneeded_save_restore_calls (void);
>     the target attribute.  */
>  #define TARGET_CLONES_ATTR_SEPARATOR '#'
>
> +#define TARGET_HAS_FMV_TARGET_ATTRIBUTE 0
> +
>  #endif /* ! GCC_RISCV_H */
> --
> 2.45.2
>
Kito Cheng Oct. 31, 2024, 10:14 a.m. UTC | #2
> diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
> index 087fbae77b0..4c85ad60b72 100644
> --- a/gcc/config/riscv/riscv-target-attr.cc
> +++ b/gcc/config/riscv/riscv-target-attr.cc
> @@ -239,10 +239,6 @@ riscv_target_attr_parser::update_settings (struct gcc_options *opts) const
>      {
>        std::string local_arch = m_subset_list->to_string (true);
>        const char* local_arch_str = local_arch.c_str ();
> -      struct cl_target_option *default_opts
> -       = TREE_TARGET_OPTION (target_option_default_node);
> -      if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
> -       free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));

Could you give a little more context why you decide to remove those logics?

>        opts->x_riscv_arch_string = xstrdup (local_arch_str);
>
>        riscv_set_arch_by_subset_list (m_subset_list, opts);
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 3ac40234345..947864fc3a6 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -12574,6 +12574,127 @@ riscv_c_mode_for_floating_type (enum tree_index ti)
>    return default_mode_for_floating_type (ti);
>  }
>
> +/* This parses the attribute arguments to target_version in DECL and modifies
> +   the feature mask and priority required to select those targets.  */
> +static void
> +parse_features_for_version (tree decl,
> +                           struct riscv_feature_bits &res,
> +                           int &priority)
> +{
> +  tree version_attr = lookup_attribute ("target_version",
> +                                       DECL_ATTRIBUTES (decl));
> +  if (version_attr == NULL_TREE)
> +    {
> +      res.length = 0;
> +      priority = 0;
> +      return;
> +    }
> +
> +  const char *version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
> +                                                   (version_attr)));
> +  gcc_assert (version_string != NULL);
> +  if (strcmp (version_string, "default") == 0)
> +    {
> +      res.length = 0;
> +      priority = 0;
> +      return;
> +    }
> +  struct cl_target_option cur_target;
> +  cl_target_option_save (&cur_target, &global_options,
> +                        &global_options_set);
> +  /* Always set to default option before parsing "arch=+..."  */
> +  struct cl_target_option *default_opts
> +       = TREE_TARGET_OPTION (target_option_default_node);
> +  cl_target_option_restore (&global_options, &global_options_set,
> +                           default_opts);
> +
> +  riscv_process_target_attr (version_string,
> +                            DECL_SOURCE_LOCATION (decl));

Is it possible to just use DECL_FUNCTION_SPECIFIC_TARGET (decl) here
rather than parse and apply that globally?
Yangyu Chen Oct. 31, 2024, 10:52 a.m. UTC | #3
> On Oct 31, 2024, at 18:14, Kito Cheng <kito.cheng@gmail.com> wrote:
> 
>> diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
>> index 087fbae77b0..4c85ad60b72 100644
>> --- a/gcc/config/riscv/riscv-target-attr.cc
>> +++ b/gcc/config/riscv/riscv-target-attr.cc
>> @@ -239,10 +239,6 @@ riscv_target_attr_parser::update_settings (struct gcc_options *opts) const
>>     {
>>       std::string local_arch = m_subset_list->to_string (true);
>>       const char* local_arch_str = local_arch.c_str ();
>> -      struct cl_target_option *default_opts
>> -       = TREE_TARGET_OPTION (target_option_default_node);
>> -      if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
>> -       free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));
> 
> Could you give a little more context why you decide to remove those logics?
> 

That's because when we have target_version features, the riscv_arch_string
may need to be stored in the DECL_FUNCTION_SPECIFIC_TARGET. If we
free the arch string, it may have use-after-free bugs. I came across
this bug even without ASAN.

I know that if we didn't free it, it might cause a memory leak. But
for GCC, it’s OK, I think. It's also complex to add something like
std::shared_ptr or std::string to the global options.

If this explanation is OK, I will add a comment about this in this
source file.

>>       opts->x_riscv_arch_string = xstrdup (local_arch_str);
>> 
>>       riscv_set_arch_by_subset_list (m_subset_list, opts);
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 3ac40234345..947864fc3a6 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -12574,6 +12574,127 @@ riscv_c_mode_for_floating_type (enum tree_index ti)
>>   return default_mode_for_floating_type (ti);
>> }
>> 
>> +/* This parses the attribute arguments to target_version in DECL and modifies
>> +   the feature mask and priority required to select those targets.  */
>> +static void
>> +parse_features_for_version (tree decl,
>> +                           struct riscv_feature_bits &res,
>> +                           int &priority)
>> +{
>> +  tree version_attr = lookup_attribute ("target_version",
>> +                                       DECL_ATTRIBUTES (decl));
>> +  if (version_attr == NULL_TREE)
>> +    {
>> +      res.length = 0;
>> +      priority = 0;
>> +      return;
>> +    }
>> +
>> +  const char *version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
>> +                                                   (version_attr)));
>> +  gcc_assert (version_string != NULL);
>> +  if (strcmp (version_string, "default") == 0)
>> +    {
>> +      res.length = 0;
>> +      priority = 0;
>> +      return;
>> +    }
>> +  struct cl_target_option cur_target;
>> +  cl_target_option_save (&cur_target, &global_options,
>> +                        &global_options_set);
>> +  /* Always set to default option before parsing "arch=+..."  */
>> +  struct cl_target_option *default_opts
>> +       = TREE_TARGET_OPTION (target_option_default_node);
>> +  cl_target_option_restore (&global_options, &global_options_set,
>> +                           default_opts);
>> +
>> +  riscv_process_target_attr (version_string,
>> +                            DECL_SOURCE_LOCATION (decl));
> 
> Is it possible to just use DECL_FUNCTION_SPECIFIC_TARGET (decl) here
> rather than parse and apply that globally?

For DECL_FUNCTION_SPECIFIC_TARGET, the answer is no.

That's because the function may not be versioned when calling from
TARGET_OPTION_FUNCTION_VERSIONS.

You can check this by applying this assert and running the test in
the final patch to see the stack dump:

```diff
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 20c6bf4f2e4..1b39c33c518 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -12692,6 +12692,7 @@ parse_features_for_version (tree decl,
                            struct riscv_feature_bits &res,
                            int &priority)
 {
+  gcc_assert (decl != NULL && DECL_FUNCTION_VERSIONED (decl));
   tree version_attr = lookup_attribute ("target_version",
                                        DECL_ATTRIBUTES (decl));
   if (version_attr == NULL_TREE)
```

```console
test2.cpp:5:10: internal compiler error: in parse_features_for_version, at config/riscv/riscv.cc:12695
    5 | void foo() {}
      |          ^
0x30dd120 internal_error(char const*, ...)
        ../.././gcc/gcc/diagnostic-global-context.cc:518
0xd2ca07 fancy_abort(char const*, int, char const*)
        ../.././gcc/gcc/diagnostic.cc:1693
0xc5713d parse_features_for_version
        ../.././gcc/gcc/config/riscv/riscv.cc:12695
0x1a9b51f riscv_compare_version_priority(tree_node*, tree_node*)
        ../.././gcc/gcc/config/riscv/riscv.cc:12790
0x1a9b69a riscv_common_function_versions(tree_node*, tree_node*)
        ../.././gcc/gcc/config/riscv/riscv.cc:12808
0x1a9b69a riscv_common_function_versions(tree_node*, tree_node*)
        ../.././gcc/gcc/config/riscv/riscv.cc:12801
0xdf1a9e decls_match(tree_node*, tree_node*, bool)
        ../.././gcc/gcc/cp/decl.cc:1213
0xe1d5bd find_last_decl
        ../.././gcc/gcc/cp/decl2.cc:1752
0xe1d5bd cplus_decl_attributes(tree_node**, tree_node*, int)
        ../.././gcc/gcc/cp/decl2.cc:1888 <http://decl2.cc:1888/>
```
Kito Cheng Oct. 31, 2024, 1:26 p.m. UTC | #4
On Thu, Oct 31, 2024 at 6:59 PM Yangyu Chen <cyy@cyyself.name> wrote:
>
>
>
> > On Oct 31, 2024, at 18:14, Kito Cheng <kito.cheng@gmail.com> wrote:
> >
> >> diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
> >> index 087fbae77b0..4c85ad60b72 100644
> >> --- a/gcc/config/riscv/riscv-target-attr.cc
> >> +++ b/gcc/config/riscv/riscv-target-attr.cc
> >> @@ -239,10 +239,6 @@ riscv_target_attr_parser::update_settings (struct gcc_options *opts) const
> >>     {
> >>       std::string local_arch = m_subset_list->to_string (true);
> >>       const char* local_arch_str = local_arch.c_str ();
> >> -      struct cl_target_option *default_opts
> >> -       = TREE_TARGET_OPTION (target_option_default_node);
> >> -      if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
> >> -       free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));
> >
> > Could you give a little more context why you decide to remove those logics?
> >
>
> That's because when we have target_version features, the riscv_arch_string
> may need to be stored in the DECL_FUNCTION_SPECIFIC_TARGET. If we
> free the arch string, it may have use-after-free bugs. I came across
> this bug even without ASAN.
>
> I know that if we didn't free it, it might cause a memory leak. But
> for GCC, it’s OK, I think. It's also complex to add something like
> std::shared_ptr or std::string to the global options.
>
> If this explanation is OK, I will add a comment about this in this
> source file.

Thanks for the explanation, then let's keep the original code and wrap
it with `#if 0` and leave some comments there :)

>
> >>       opts->x_riscv_arch_string = xstrdup (local_arch_str);
> >>
> >>       riscv_set_arch_by_subset_list (m_subset_list, opts);
> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> >> index 3ac40234345..947864fc3a6 100644
> >> --- a/gcc/config/riscv/riscv.cc
> >> +++ b/gcc/config/riscv/riscv.cc
> >> @@ -12574,6 +12574,127 @@ riscv_c_mode_for_floating_type (enum tree_index ti)
> >>   return default_mode_for_floating_type (ti);
> >> }
> >>
> >> +/* This parses the attribute arguments to target_version in DECL and modifies
> >> +   the feature mask and priority required to select those targets.  */
> >> +static void
> >> +parse_features_for_version (tree decl,
> >> +                           struct riscv_feature_bits &res,
> >> +                           int &priority)
> >> +{
> >> +  tree version_attr = lookup_attribute ("target_version",
> >> +                                       DECL_ATTRIBUTES (decl));
> >> +  if (version_attr == NULL_TREE)
> >> +    {
> >> +      res.length = 0;
> >> +      priority = 0;
> >> +      return;
> >> +    }
> >> +
> >> +  const char *version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
> >> +                                                   (version_attr)));
> >> +  gcc_assert (version_string != NULL);
> >> +  if (strcmp (version_string, "default") == 0)
> >> +    {
> >> +      res.length = 0;
> >> +      priority = 0;
> >> +      return;
> >> +    }
> >> +  struct cl_target_option cur_target;
> >> +  cl_target_option_save (&cur_target, &global_options,
> >> +                        &global_options_set);
> >> +  /* Always set to default option before parsing "arch=+..."  */
> >> +  struct cl_target_option *default_opts
> >> +       = TREE_TARGET_OPTION (target_option_default_node);
> >> +  cl_target_option_restore (&global_options, &global_options_set,
> >> +                           default_opts);
> >> +
> >> +  riscv_process_target_attr (version_string,
> >> +                            DECL_SOURCE_LOCATION (decl));
> >
> > Is it possible to just use DECL_FUNCTION_SPECIFIC_TARGET (decl) here
> > rather than parse and apply that globally?
>
> For DECL_FUNCTION_SPECIFIC_TARGET, the answer is no.
>
> That's because the function may not be versioned when calling from
> TARGET_OPTION_FUNCTION_VERSIONS.
>
> You can check this by applying this assert and running the test in
> the final patch to see the stack dump:

So sad, ok, let keep as it
Yangyu Chen Nov. 1, 2024, 7:51 p.m. UTC | #5
On 10/31/24 21:26, Kito Cheng wrote:
> On Thu, Oct 31, 2024 at 6:59 PM Yangyu Chen <cyy@cyyself.name> wrote:
>>> On Oct 31, 2024, at 18:14, Kito Cheng <kito.cheng@gmail.com> wrote:
>>>
>>>> diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
>>>> index 087fbae77b0..4c85ad60b72 100644
>>>> --- a/gcc/config/riscv/riscv-target-attr.cc
>>>> +++ b/gcc/config/riscv/riscv-target-attr.cc
>>>> @@ -239,10 +239,6 @@ riscv_target_attr_parser::update_settings (struct gcc_options *opts) const
>>>>      {
>>>>        std::string local_arch = m_subset_list->to_string (true);
>>>>        const char* local_arch_str = local_arch.c_str ();
>>>> -      struct cl_target_option *default_opts
>>>> -       = TREE_TARGET_OPTION (target_option_default_node);
>>>> -      if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
>>>> -       free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));
>>>
>>> Could you give a little more context why you decide to remove those logics?
>>>
>>
>> That's because when we have target_version features, the riscv_arch_string
>> may need to be stored in the DECL_FUNCTION_SPECIFIC_TARGET. If we
>> free the arch string, it may have use-after-free bugs. I came across
>> this bug even without ASAN.
>>
>> I know that if we didn't free it, it might cause a memory leak. But
>> for GCC, it’s OK, I think. It's also complex to add something like
>> std::shared_ptr or std::string to the global options.
>>
>> If this explanation is OK, I will add a comment about this in this
>> source file.
> 
> Thanks for the explanation, then let's keep the original code and wrap
> it with `#if 0` and leave some comments there :)
> 

Update: I found this use-after-free bug no longer happens after I 
restored default_opts to global_options in the function 
parse_features_for_version in this patch. These lines were just added 
when I was debugging this UAF bug. Since they are no longer needed, I 
will drop these lines in the next revision.
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
index 087fbae77b0..4c85ad60b72 100644
--- a/gcc/config/riscv/riscv-target-attr.cc
+++ b/gcc/config/riscv/riscv-target-attr.cc
@@ -239,10 +239,6 @@  riscv_target_attr_parser::update_settings (struct gcc_options *opts) const
     {
       std::string local_arch = m_subset_list->to_string (true);
       const char* local_arch_str = local_arch.c_str ();
-      struct cl_target_option *default_opts
-	= TREE_TARGET_OPTION (target_option_default_node);
-      if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
-	free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));
       opts->x_riscv_arch_string = xstrdup (local_arch_str);
 
       riscv_set_arch_by_subset_list (m_subset_list, opts);
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 3ac40234345..947864fc3a6 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -12574,6 +12574,127 @@  riscv_c_mode_for_floating_type (enum tree_index ti)
   return default_mode_for_floating_type (ti);
 }
 
+/* This parses the attribute arguments to target_version in DECL and modifies
+   the feature mask and priority required to select those targets.  */
+static void
+parse_features_for_version (tree decl,
+			    struct riscv_feature_bits &res,
+			    int &priority)
+{
+  tree version_attr = lookup_attribute ("target_version",
+					DECL_ATTRIBUTES (decl));
+  if (version_attr == NULL_TREE)
+    {
+      res.length = 0;
+      priority = 0;
+      return;
+    }
+
+  const char *version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
+						    (version_attr)));
+  gcc_assert (version_string != NULL);
+  if (strcmp (version_string, "default") == 0)
+    {
+      res.length = 0;
+      priority = 0;
+      return;
+    }
+  struct cl_target_option cur_target;
+  cl_target_option_save (&cur_target, &global_options,
+			 &global_options_set);
+  /* Always set to default option before parsing "arch=+..."  */
+  struct cl_target_option *default_opts
+	= TREE_TARGET_OPTION (target_option_default_node);
+  cl_target_option_restore (&global_options, &global_options_set,
+			    default_opts);
+
+  riscv_process_target_attr (version_string,
+			     DECL_SOURCE_LOCATION (decl));
+
+  priority = global_options.x_riscv_fmv_priority;
+  const char *arch_string = global_options.x_riscv_arch_string;
+  bool parse_res
+    = riscv_minimal_hwprobe_feature_bits (arch_string, &res,
+					  DECL_SOURCE_LOCATION (decl));
+  gcc_assert (parse_res);
+
+  if (arch_string != default_opts->x_riscv_arch_string)
+    free (CONST_CAST (void *, (const void *) arch_string));
+
+  cl_target_option_restore (&global_options, &global_options_set,
+			    &cur_target);
+}
+
+/* Compare priorities of two feature masks.  Return:
+     1: mask1 is higher priority
+    -1: mask2 is higher priority
+     0: masks are equal.
+   Since riscv_feature_bits has total 128 bits to be used as mask, when counting
+   the total 1s in the mask, the 1s in group1 needs to multiply a weight.  */
+static int
+compare_fmv_features (const struct riscv_feature_bits &mask1,
+		      const struct riscv_feature_bits &mask2,
+		      int prio1, int prio2)
+{
+  unsigned length1 = mask1.length, length2 = mask2.length;
+  /* 1.  Compare length, for length == 0 means default version, which should be
+	 the lowest priority).  */
+  if (length1 != length2)
+    return length1 > length2 ? 1 : -1;
+  /* 2.  Compare the priority.  */
+  if (prio1 != prio2)
+    return prio1 > prio2 ? 1 : -1;
+  /* 3.  Compare the total number of 1s in the mask.  */
+  unsigned pop1 = 0, pop2 = 0;
+  for (unsigned i = 0; i < length1; i++)
+    {
+      pop1 += __builtin_popcountll (mask1.features[i]);
+      pop2 += __builtin_popcountll (mask2.features[i]);
+    }
+  if (pop1 != pop2)
+    return pop1 > pop2 ? 1 : -1;
+  /* 4.  Compare the mask bit by bit order.  */
+  for (unsigned i = 0; i < length1; i++)
+    {
+      unsigned long long xor_mask = mask1.features[i] ^ mask2.features[i];
+      if (xor_mask == 0)
+	continue;
+      return TEST_BIT (mask1.features[i], __builtin_ctzll (xor_mask)) ? 1 : -1;
+    }
+  /* 5.  If all bits are equal, return 0.  */
+  return 0;
+}
+
+/* Compare priorities of two version decls.  Return:
+     1: mask1 is higher priority
+    -1: mask2 is higher priority
+     0: masks are equal.  */
+int
+riscv_compare_version_priority (tree decl1, tree decl2)
+{
+  struct riscv_feature_bits mask1, mask2;
+  int prio1, prio2;
+
+  parse_features_for_version (decl1, mask1, prio1);
+  parse_features_for_version (decl2, mask2, prio2);
+
+  return compare_fmv_features (mask1, mask2, prio1, prio2);
+}
+
+/* This function returns true if FN1 and FN2 are versions of the same function,
+   that is, the target_version attributes of the function decls are different.
+   This assumes that FN1 and FN2 have the same signature.  */
+
+bool
+riscv_common_function_versions (tree fn1, tree fn2)
+{
+  if (TREE_CODE (fn1) != FUNCTION_DECL
+      || TREE_CODE (fn2) != FUNCTION_DECL)
+    return false;
+
+  return riscv_compare_version_priority (fn1, fn2) != 0;
+}
+
 /* On riscv we have an ABI defined safe buffer.  This constant is used to
    determining the probe offset for alloca.  */
 
@@ -12948,6 +13069,12 @@  riscv_stack_clash_protection_alloca_probe_range (void)
 #undef TARGET_C_MODE_FOR_FLOATING_TYPE
 #define TARGET_C_MODE_FOR_FLOATING_TYPE riscv_c_mode_for_floating_type
 
+#undef TARGET_COMPARE_VERSION_PRIORITY
+#define TARGET_COMPARE_VERSION_PRIORITY riscv_compare_version_priority
+
+#undef TARGET_OPTION_FUNCTION_VERSIONS
+#define TARGET_OPTION_FUNCTION_VERSIONS riscv_common_function_versions
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-riscv.h"
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 2ff9c1024f3..8a8b08b6b51 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -1303,4 +1303,6 @@  extern void riscv_remove_unneeded_save_restore_calls (void);
    the target attribute.  */
 #define TARGET_CLONES_ATTR_SEPARATOR '#'
 
+#define TARGET_HAS_FMV_TARGET_ATTRIBUTE 0
+
 #endif /* ! GCC_RISCV_H */