| Message ID | CAFULd4bj2PTRvDbbRYwdxjGHc7fETB1-8jV1GW8Zio2Ae_Vdpw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 15, 2015 at 8:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > Hello! > > As mentioned in the PR, ix86_valid_target_attribute_tree creates > temporary copies of current options strings and saves *pointers* to > these copies with build_target_option_node. A couple of lines below, > these temporary copies are freed, leaving dangling pointers in the > saved structure. > > Use xstrndup to create permanent copy of string on the heap. This will > however create a small leak, as this copy is never deallocated. > > There is no test infrastructure to check for memory errors, so there > is no testcase added. > > 2015-09-15 Uros Bizjak <ubizjak@gmail.com> > > PR target/67484 > * config/i386/i386.c (ix86_valid_target_attribute_tree): > Use xstrdup to copy option_strings to opts->x_ix86_arch_string and > opts->x_ix86_tune_string. > > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. > > I'll wait a couple of days for possible comments on the above solution. I thought we have a custom destructor for target_option_node. Ah, no, that was for target_globals. I suppose we could add one to cl_target_option as well. Note that currently the strings are not GTY((skip)) so it seems we expect ggc allocated strings there? Which means the xstrdup in ix86_valid_target_attribute_inner_p should be ggc_strdup? Richard. > Uros.
On Wed, Sep 16, 2015 at 10:45 AM, Richard Biener <richard.guenther@gmail.com> wrote: >> As mentioned in the PR, ix86_valid_target_attribute_tree creates >> temporary copies of current options strings and saves *pointers* to >> these copies with build_target_option_node. A couple of lines below, >> these temporary copies are freed, leaving dangling pointers in the >> saved structure. >> >> Use xstrndup to create permanent copy of string on the heap. This will >> however create a small leak, as this copy is never deallocated. >> >> There is no test infrastructure to check for memory errors, so there >> is no testcase added. >> >> 2015-09-15 Uros Bizjak <ubizjak@gmail.com> >> >> PR target/67484 >> * config/i386/i386.c (ix86_valid_target_attribute_tree): >> Use xstrdup to copy option_strings to opts->x_ix86_arch_string and >> opts->x_ix86_tune_string. >> >> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. >> >> I'll wait a couple of days for possible comments on the above solution. > > I thought we have a custom destructor for target_option_node. Ah, no, > that was for target_globals. I suppose we could add one to cl_target_option > as well. Note that currently the strings are not GTY((skip)) so it seems > we expect ggc allocated strings there? Which means the xstrdup in > ix86_valid_target_attribute_inner_p should be ggc_strdup? This is a bit over my knowledge of option processing, but please note that the only function that performs non-recursive call to ix86_valid_target_attribute_inner_p also frees the strings, allocated by mentioned function. Uros.
I see in gtype-desc.c:
void
gt_ggc_mx_cl_target_option (void *x_p)
{
struct cl_target_option * const x = (struct cl_target_option *)x_p;
if (ggc_test_and_set_mark (x))
{
gt_ggc_m_S ((*x).x_ix86_arch_string);
gt_ggc_m_S ((*x).x_ix86_recip_name);
gt_ggc_m_S ((*x).x_ix86_tune_ctrl_string);
gt_ggc_m_S ((*x).x_ix86_tune_memcpy_strategy);
gt_ggc_m_S ((*x).x_ix86_tune_memset_strategy);
gt_ggc_m_S ((*x).x_ix86_tune_string);
}
so it certainly does not expect heap allocated strings in
ix86_arch_string and friends.
Richard.
On Wed, Sep 16, 2015 at 10:59 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Sep 16, 2015 at 10:45 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>
>>> As mentioned in the PR, ix86_valid_target_attribute_tree creates
>>> temporary copies of current options strings and saves *pointers* to
>>> these copies with build_target_option_node. A couple of lines below,
>>> these temporary copies are freed, leaving dangling pointers in the
>>> saved structure.
>>>
>>> Use xstrndup to create permanent copy of string on the heap. This will
>>> however create a small leak, as this copy is never deallocated.
>>>
>>> There is no test infrastructure to check for memory errors, so there
>>> is no testcase added.
>>>
>>> 2015-09-15 Uros Bizjak <ubizjak@gmail.com>
>>>
>>> PR target/67484
>>> * config/i386/i386.c (ix86_valid_target_attribute_tree):
>>> Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
>>> opts->x_ix86_tune_string.
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>
>>> I'll wait a couple of days for possible comments on the above solution.
>>
>> I thought we have a custom destructor for target_option_node. Ah, no,
>> that was for target_globals. I suppose we could add one to cl_target_option
>> as well. Note that currently the strings are not GTY((skip)) so it seems
>> we expect ggc allocated strings there? Which means the xstrdup in
>> ix86_valid_target_attribute_inner_p should be ggc_strdup?
>
> This is a bit over my knowledge of option processing, but please note
> that the only function that performs non-recursive call to
> ix86_valid_target_attribute_inner_p also frees the strings, allocated
> by mentioned function.
>
> Uros.
And it is initialized via
void
cl_target_option_save (struct cl_target_option *ptr, struct gcc_options *opts)
{
if (targetm.target_option.save)
targetm.target_option.save (ptr, opts);
ptr->x_recip_mask = opts->x_recip_mask;
ptr->x_ix86_isa_flags = opts->x_ix86_isa_flags;
ptr->x_ix86_fpmath = opts->x_ix86_fpmath;
ptr->x_target_flags = opts->x_target_flags;
}
which uses a target hook to copy from gcc_options to cl_target_options...
(what a maze), and ix86_function_specific_save also plain copies the
pointers.
Richard.
On Wed, Sep 16, 2015 at 11:08 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> I see in gtype-desc.c:
>
> void
> gt_ggc_mx_cl_target_option (void *x_p)
> {
> struct cl_target_option * const x = (struct cl_target_option *)x_p;
> if (ggc_test_and_set_mark (x))
> {
> gt_ggc_m_S ((*x).x_ix86_arch_string);
> gt_ggc_m_S ((*x).x_ix86_recip_name);
> gt_ggc_m_S ((*x).x_ix86_tune_ctrl_string);
> gt_ggc_m_S ((*x).x_ix86_tune_memcpy_strategy);
> gt_ggc_m_S ((*x).x_ix86_tune_memset_strategy);
> gt_ggc_m_S ((*x).x_ix86_tune_string);
> }
>
> so it certainly does not expect heap allocated strings in
> ix86_arch_string and friends.
>
> Richard.
>
> On Wed, Sep 16, 2015 at 10:59 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Wed, Sep 16, 2015 at 10:45 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>
>>>> As mentioned in the PR, ix86_valid_target_attribute_tree creates
>>>> temporary copies of current options strings and saves *pointers* to
>>>> these copies with build_target_option_node. A couple of lines below,
>>>> these temporary copies are freed, leaving dangling pointers in the
>>>> saved structure.
>>>>
>>>> Use xstrndup to create permanent copy of string on the heap. This will
>>>> however create a small leak, as this copy is never deallocated.
>>>>
>>>> There is no test infrastructure to check for memory errors, so there
>>>> is no testcase added.
>>>>
>>>> 2015-09-15 Uros Bizjak <ubizjak@gmail.com>
>>>>
>>>> PR target/67484
>>>> * config/i386/i386.c (ix86_valid_target_attribute_tree):
>>>> Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
>>>> opts->x_ix86_tune_string.
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>>
>>>> I'll wait a couple of days for possible comments on the above solution.
>>>
>>> I thought we have a custom destructor for target_option_node. Ah, no,
>>> that was for target_globals. I suppose we could add one to cl_target_option
>>> as well. Note that currently the strings are not GTY((skip)) so it seems
>>> we expect ggc allocated strings there? Which means the xstrdup in
>>> ix86_valid_target_attribute_inner_p should be ggc_strdup?
>>
>> This is a bit over my knowledge of option processing, but please note
>> that the only function that performs non-recursive call to
>> ix86_valid_target_attribute_inner_p also frees the strings, allocated
>> by mentioned function.
>>
>> Uros.
Index: config/i386/i386.c =================================================================== --- config/i386/i386.c (revision 227777) +++ config/i386/i386.c (working copy) @@ -5080,12 +5080,14 @@ ix86_valid_target_attribute_tree (tree args, /* If we are using the default tune= or arch=, undo the string assigned, and use the default. */ if (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]) - opts->x_ix86_arch_string = option_strings[IX86_FUNCTION_SPECIFIC_ARCH]; + opts->x_ix86_arch_string + = xstrdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]); else if (!orig_arch_specified) opts->x_ix86_arch_string = NULL; if (option_strings[IX86_FUNCTION_SPECIFIC_TUNE]) - opts->x_ix86_tune_string = option_strings[IX86_FUNCTION_SPECIFIC_TUNE]; + opts->x_ix86_tune_string + = xstrdup (option_strings[IX86_FUNCTION_SPECIFIC_TUNE]); else if (orig_tune_defaulted) opts->x_ix86_tune_string = NULL;
Hello! As mentioned in the PR, ix86_valid_target_attribute_tree creates temporary copies of current options strings and saves *pointers* to these copies with build_target_option_node. A couple of lines below, these temporary copies are freed, leaving dangling pointers in the saved structure. Use xstrndup to create permanent copy of string on the heap. This will however create a small leak, as this copy is never deallocated. There is no test infrastructure to check for memory errors, so there is no testcase added. 2015-09-15 Uros Bizjak <ubizjak@gmail.com> PR target/67484 * config/i386/i386.c (ix86_valid_target_attribute_tree): Use xstrdup to copy option_strings to opts->x_ix86_arch_string and opts->x_ix86_tune_string. Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. I'll wait a couple of days for possible comments on the above solution. Uros.