> On Oct 31, 2024, at 18:14, Kito Cheng <kito.ch...@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/>
```
