On Thu, Oct 31, 2024 at 6:59 PM Yangyu Chen <c...@cyyself.name> wrote:
>
>
>
> > 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.
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
