This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] aarch64: allow adding/removing just feature flags via .arch_extension
- From: "Jan Beulich" <JBeulich at suse dot com>
- To: "Marcus Shawcroft" <marcus dot shawcroft at gmail dot com>
- Cc: "Marcus Shawcroft" <marcus dot shawcroft at arm dot com>, "Richard Earnshaw" <rearnsha at arm dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 04 Nov 2014 07:47:09 +0000
- Subject: Re: [PATCH] aarch64: allow adding/removing just feature flags via .arch_extension
- Authentication-results: sourceware.org; auth=none
- References: <544A64EC0200007800041F06 at mail dot emea dot novell dot com> <CAFqB+Pzm9=VAvC25LAMgP8zicU4kmq6rvWEdEdtOZeCnam5_kQ at mail dot gmail 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