[PATCH v1 03/14] gas: implement parsing of object attributes v2

[Jan Beulich]

On 28.04.2025 13:56, Matthieu Longo wrote:
> On 2025-04-25 09:28, Jan Beulich wrote:
>> On 24.04.2025 18:27, Matthieu Longo wrote:
>>> On 2025-03-28 14:24, Jan Beulich wrote:
>>>> On 21.03.2025 18:14, Matthieu Longo wrote:
>>>>> --- a/gas/config/obj-elf.c
>>>>> +++ b/gas/config/obj-elf.c
>>>>> @@ -41,6 +41,10 @@
>>>>>    #include "elf/alpha.h"
>>>>>    #endif
>>>>>    
>>>>> +#ifdef TC_AARCH64
>>>>> +#include "elf/aarch64.h"
>>>>> +#endif
>>>>
>>>> Why is this needed? It's imo bat practice to include arch-specific files here,
>>>> even if ...
>>>>
>>>
>>> Removed in the next revision.
>>>
>>>>>    #ifdef TC_MIPS
>>>>>    #include "elf/mips.h"
>>>>>    #endif
>>>>
>>>> ... context on both sides demonstrates that it was done before.
>>>>
>>>> For most of the new code following in this file, I'm again not overly happy to
>>>> see so much dead code added for non-Arm64. Can't interested tc-*.h files have a
>>>> #define added controlling whether this code is actually to be compiled?
>>>>
>>>
>>> I refactored the code in the next revision, and shared the parsing code
>>> between OAv1 and OAv2. So TC_* defines should not be necessary anymore.
>>>
>>> There is actually a lot of code added here for object attributes (both
>>> v1 and v2). I am wondering if we could not move it to another file:
>>> obj-attr.c
>>> What do you think ?
>>
>> Generally I like splitting things up into smaller files. Splitting up here
>> may additionally make it easier to omit the code altogether for targets not
>> using it. So - yes, please, if it's reasonably possible to arrange for.
>>
> 
> Split done in the next revision.
> 
>>>>> +  char* val = xmemdup0 (s, i);
>>>>> +  arg_out->optional_set = true;
>>>>> +  arg_out->type = STRING_VALUE;
>>>>> +  arg_out->value.string = val;
>>>>
>>>> STRING_VALUE further up was used for string literals. How come it identifies
>>>> what looks to be an identifier here? If it's an identifier, why would you not
>>>> use one of the existing primitives to parse that, e.g. get_symbol_name()?
>>>
>>> get_symbol_name seems to do too much for what I needed, specifically the
>>> handling of double quotes.
>>
>> You could reject the double-quotes case after the function returned? Else
>> there may be other suitable parsing functions already available? If
>> absolutely nothing suitable is there, perhaps something wants adding, to
>> avoid too much open-coding here?
> 
> I tried to use get_symbol_name, and it treats '-' as a delimiter, which 
> is not correct for the name of subsections.

I fear there are more issues here then. If you can supply me with a legitimate
use including a dash (or whatever else non-symbol-char), I may be able to point
that out better using such an example than in abstract terms. (Summary up
front: Such kind-of-identifiers may need quoting anyway.)

Jan