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

[Richard Earnshaw (lists)]

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.

> 
> ... 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