On 28/10/2019 21:52, Bernhard Reutner-Fischer wrote:
> On Mon, 28 Oct 2019 11:53:06 +1100
> Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote:
>
>> On Wed, 23 Oct 2019 at 23:07, Richard Biener <richard.guent...@gmail.com>
>> wrote:
>
>>> Did you try this with multiple assembler options? I see you stream
>>> them as -Wa,-mfpu=xyz,-mthumb but then compare the whole
>>> option strings so a mismatch with -Wa,-mthumb,-mfpu=xyz would be
>
> indeed, i'd have expected some kind of sorting, but i don't see it in
> the proposed patch?
Why? If the options interact with each other, then sorting could change
the meaning. We could only sort the options if we knew that couldn't
happen. For a trivial example,
-mcpu=zzz -mcpu=xxx
would override the zzz with xxx, but sorting would switch them around.
And this is just a trivial case, if the options interact but have
different names then you've no idea what must happen unless you are GAS;
and we don't want to build such knowledge into GCC.
So preserver the options, in the order they were given based on the
standard expectations: namely that options on the command line will
override anything built in to the compiler itself.
R.
>
>>> diagnosed. If there's a spec induced -Wa option do we get to see
>>> that as well? I can imagine -march=xyz enabling a -Wa option
>>> for example.
>>>
>>> + *collect_as = XNEWVEC (char, strlen (args_text) + 1);
>>> + strcpy (*collect_as, args_text);
>>>
>>> there's strdup. Btw, I'm not sure why you don't simply leave
>>> the -Wa option in the merged options [individually] and match
>>> them up but go the route of comparing strings and carrying that
>>> along separately. I think that would be much better.
>>
>> Is attached patch which does this is OK?
>
>> + obstack_init (&collect_obstack);
>> + obstack_grow (&collect_obstack, "COLLECT_AS_OPTIONS=",
>> + sizeof ("COLLECT_AS_OPTIONS=") - 1);
>> + obstack_grow (&collect_obstack, "-Wa,", strlen ("-Wa,"));
>
> Why don't you grow once, including the "-Wa," ?
>
>> +/* Append options OPTS from -Wa, options to ARGV_OBSTACK. */
>> +
>> +static void
>> +append_compiler_wa_options (obstack *argv_obstack,
>> + struct cl_decoded_option *opts,
>> + unsigned int count)
>> +{
>> + static const char *collect_as;
>> + for (unsigned int j = 1; j < count; ++j)
>> + {
>> + struct cl_decoded_option *option = &opts[j];
>
> Instead of the switch below, why not just
>
> if (option->opt_index != OPT_Wa_)
> continue;
>
> here?
>
>> + if (j == 1)
>> + collect_as = NULL;
>
> or at least here?
>
> (why's collect_as static in the first place? wouldn't that live in the parent
> function?)
>
>> + const char *args_text = option->orig_option_with_args_text;
>> + switch (option->opt_index)
>> + {
>> + case OPT_Wa_:
>> + break;
>> + default:
>> + continue;
>> + }
