Re: [PATCH] aarch64: allow adding/removing just feature flags via .arch_extension

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 03.11.14 at 18:44, <marcus.shawcroft@gmail.com> wrote:
> On 24 October 2014 13:40, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>  static int
>> -aarch64_parse_features (char *str, const aarch64_feature_set **opt_p)
>> +aarch64_parse_features (char *str, const aarch64_feature_set **opt_p,
>> +                       char *ext)
> 
> The new parameter here is being used as a boolean flag, represent it
> with a bfd_boolean rather than a char *.

While I was expecting a comment to that effect, I had the slight hope
to get things accepted as is: This being an internal helper function,
using the parameter as local variable at the same time seems more
efficient to me. But if you insist, I'll have to convert it back.

>> @@ -7267,16 +7270,18 @@ aarch64_parse_features (char *str, const
>>    while (str != NULL && *str != 0)
>>      {
>>        const struct aarch64_option_cpu_value_table *opt;
>> -      char *ext;
>>        int optlen;
>>
>> -      if (*str != '+')
>> +      if (!ext)
>>         {
>> -         as_bad (_("invalid architectural extension"));
>> -         return 0;
>> -       }
>> +         if (*str != '+')
>> +           {
>> +             as_bad (_("invalid architectural extension"));
>> +             return 0;
>> +           }
>>
>> -      str++;
>> +         str++;
>> +       }
>>        ext = strchr (str, '+');
> 
> The logic here is odd. I don;t think we want this in the case when ext
> != NULL.  The effect is that:
> 
> .arch_extension +crc
> 
> ...will give a "missing extension" rather than an "unknown extension"
> error message.

Which is quite right: There is an extension name missing ahead of the
+ character. The other error message would be right too (on the
grounds that no extension name starts with +).

Jan