[PATCH v8 05/19] gas: use common code for object attribute v1 & v2 parsing

[Matthieu Longo]

On 2025-08-19 17:44, Richard Earnshaw (lists) wrote:
> On 08/08/2025 12:58, Jan Beulich wrote:
>> On 15.07.2025 13:39, Matthieu Longo wrote:
>>> Since the previous patch added all the code to be able to parse both
>>> OAv1 and OAv2 directives, this patch switches OAv1 to use this common
>>> code.
>>> Additionally to the common code in obj-elf.c, the following backends
>>> using a custom object attribute directive were impacted.
>>>    - ARC
>>>    - Arm
>>>    - m68k
>>>    - PowerPC
>>>    - RISC-V
>>>    - TI C6X
>>> A parsing test for Arm had to be adapted to the error messages of the
>>> new parser.
>>>
>>> The gas and ld test suites were successfully run for the following
>>> backends: S390, ARC, Arm, CSky, m68k, msp430, PowerPC, TI C6X, RISC-V,
>>> AArch64, MIPS, SPARC.
>>> ---
>>>   gas/config/obj-elf-attr.c           | 116 ----------------------------
>>>   gas/config/obj-elf-attr.h           |   5 --
>>>   gas/config/obj-elf.c                |   5 --
>>>   gas/config/tc-arc.c                 |   2 +-
>>>   gas/config/tc-arm.c                 |   2 +-
>>>   gas/config/tc-m68k.c                |   2 +-
>>>   gas/config/tc-ppc.c                 |   2 +-
>>>   gas/config/tc-riscv.c               |   2 +-
>>>   gas/config/tc-tic6x.c               |   2 +-
>>>   gas/testsuite/gas/arm/attr-syntax.d |   6 +-
>>>   10 files changed, 11 insertions(+), 133 deletions(-)
>>
>> Such a diffstat is close to being okay all by itself. There's just one part
>> I don't understand:
>>
>>> --- a/gas/testsuite/gas/arm/attr-syntax.d
>>> +++ b/gas/testsuite/gas/arm/attr-syntax.d
>>> @@ -1,4 +1,8 @@
>>>   #source: attr-syntax.s
>>>   #notarget: *-*-pe
>>>   #as:
>>> -#error: :1: Error: Attribute name not recognised: made_up_tag.*:3: Error: expected <tag> , <value>.*:5: Error: expected <tag> , <value>
>>
>> This looks odd, and so do ...
> 
> I think this is because the re matches the entire stderr buffer against the regexp, and newline characters are part of that buffer.
> ".*" here matches an arbitrary amount of text, including newlines, so a message on line 2 would be silently skipped, for example.  But we're deleting that line, so this doesn't matter too much.
> 
>>
>>> +#error: \A[^\n]*\.s: Assembler messages:\n
>>> +#error: [^\n]*\.s:[0-9]+: Error: unknown identifier 'made_up_tag'\n
>>> +#error: [^\n]*\.s:[0-9]+: Error: could not parse attribute tag\n
>>> +#error: [^\n]*\.s:[0-9]+: Error: unexpected comma before parameter 1\n
>>> +#error: [^\n]*\.s:[0-9]+: Error: missing comma after parameter 1
> 
> Technically, this line should end with \Z if you're using this format, so that we consume the entire buffer.
> 

Fixed in the next revision.

>>
>> ... all the trailing \n here. Would this better be converted to the more
>> common #error_output: form?
> 
> There's precedent for this type of form in, eg binutils/testsuite/binutils-all/mips/mips-reginfo.d and gas/testsuite/gas/aarch64/illegal-sve2-aes.d (though the latter does not explicitly match newlines).
> 
> Error_output is certainly more powerful, and is, perhaps a little clearer.  But we could make the text clearer here as well, with
> little loss of accuracy.  I think
> 
> #error: \A[^\n]+: Assembler messages:
> #error: \n[^\n]+: Error: unknown identifier 'made_up_tag'
> #error: \n[^\n]+: Error: could not parse attribute tag
> #error: \n[^\n]+: Error: unexpected comma before parameter 1
> #error: \n[^\n]+: Error: missing comma after parameter 1\Z
> 
> is much easier to read and comprehend, and should be quite enough to ensure that the output does not meaningfully change.  Note that the framework concatenates all these lines into a single string, so it doesn't matter that we put the \n on the following line; we still end up with
> 
> \A[^\n]+: Assembler messages:\n[^\n]+: Error: unk[...<omitted>]\n[^\n]+: Error: missing comma after parameter 1\Z
> 
> as the regex.
> 
> R.
> 
>>
>> Jan
> 

Followed your recommendation for all the new tests. Fixed in the next 
revision.

Matthieu