[PATCH] arm: ignore inapplicable .arch=no...

[Richard Earnshaw (lists)]

On 06/06/2025 09:54, Jan Beulich wrote:
> On 03.06.2025 17:20, Richard Earnshaw wrote:
>> On 30/04/2025 15:20, Jan Beulich wrote:
>>> On 30.04.2025 16:10, Richard Earnshaw (lists) wrote:
>>>> On 30/04/2025 14:34, Jan Beulich wrote:
>>>>> Unlike for command line options, where a base architecture needs to be
>>>>> provided explicitly, the .arch directive doesn't have such a
>>>>> requirement. Therefore it is odd that disabling of an inapplicable
>>>>> extension isn't silently ignored; claiming "not allowed for the current
>>>>> base architecture" is at best misleading. Simply skip the error path,
>>>>> leveraging that in such a case ARM_CLEAR_FEATURE() is (supposed to be)
>>>>> a no-op.
>>>>>
>>>>> --- a/gas/config/tc-arm.c
>>>>> +++ b/gas/config/tc-arm.c
>>>>> @@ -32792,7 +32792,7 @@ s_arm_arch_extension (int ignored ATTRIB
>>>>>   	      break;
>>>>>   	  }
>>>>>   
>>>>> -	if (i == nb_allowed_archs)
>>>>> +	if (i == nb_allowed_archs && adding_value)
>>>>>   	  {
>>>>>   	    as_bad (_("architectural extension `%s' is not allowed for the "
>>>>>   		      "current base architecture"), name);
>>>>
>>>>
>>>> Sorry, I don't follow the logic you have in mind.  Trying to enable, for example, MVE on an a-profile architecture is completely meaningless and should be diagnosed as an error in the source file.
>>>
>>> Of course; the change here is about disabling of extensions.
>>>
>>>> Can you give an example of what you're trying to permit here?
>>>
>>> The example I noticed this was with ".arch nocrypto" under -march=all. I
>>> simply don't think this needs diagnosing. And if there was a diagnostic,
>>> I think it shouldn't say "crypto", kind of suggesting that's being turned
>>> on, not off.
>>>
>>> Jan
>>
>> Sorry for dropping the ball on this.
>>
>> I agree the message is misleading.  But you've also changed the 
>> subsequent logic here as well, and I'm not sure if that would be 100% safe.
>>
>> Perhaps a compromise would be a tsktsk along the lines:
>>
>> 	disabling feature `%s' has no effect on the current base architecture.
>>
>> so:
>>
>>    if (i == nb_allowed_archs)
>>      {
>>        if (adding_value)
>>          as_bad (_("architectural extension `%s' is not allowed for the "
>>                    "current base architecture"), name);
>>        else
>>          as_tsktsk (_"(disabling feature `%s' has no effect on the "
>>                        "current base architecture"), name);
>>        break;
>>      }
> 
> I'll do as you say, noting that I dislike as_tsktsk(), not the least because
> of there not being a way to suppress it (without changing the source). Since
> you suggested this alternative, I'll take it as implicit approval to commit
> in that shape. Please shout (soon) in case that's not appropriate.
> 
> Jan

That's fine. Thanks.

Perhaps the option that suppresses warnings should suppress tsktsk as well.