[PATCH v8 04/19] gas: implement parsing of object attributes v2

[Jan Beulich]

On 27.08.2025 17:17, Matthieu Longo wrote:
> On 2025-08-06 16:05, Jan Beulich wrote:
>> On 06.08.2025 16:02, Matthieu Longo wrote:
>>> On 2025-07-31 15:40, Jan Beulich wrote:
>>>> On 15.07.2025 13:39, Matthieu Longo wrote:
>>>>> --- a/bfd/elf-attrs.c
>>>>> +++ b/bfd/elf-attrs.c
>>>>> @@ -255,8 +255,152 @@ bfd_elf_set_obj_attr_contents (bfd *abfd, bfd_byte *buffer, bfd_vma size)
>>>>>      write_obj_attr_section_v1 (abfd, buffer, size);
>>>>>    }
>>>>>    
>>>>> +/* The first two tags in gnu-testing namespace are known, and so have a name and
>>>>> +   can be initialized to the default value ('0' or NULL) depending on the
>>>>> +   encoding specified on the subsection.  Any tags above 1 will be considered
>>>>> +   unknown, so will be default initialized in the same way but its status will
>>>>> +   be set to obj_attr_subsection_v2_unknown.  */
>>>>> +static const obj_attr_info_t known_tags_gnu_testing[] =
>>>>> +{
>>>>> +  {
>>>>> +    .tag = {"GNUTestTag_0", .value = {
>>>>> +      .val.u32 = 0,
>>>>> +      .vtype = VALUE_U32
>>>>> +    }},
>>>>> +    .default_value = {.val.u64 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
>>>>> +    .encoding = OA_ENC_ULEB128,
>>>>> +  },
>>>>> +  {
>>>>> +    .tag = {"GNUTestTag_1", .value = {
>>>>> +      .val.u32 = 1,
>>>>> +      .vtype = VALUE_U32
>>>>> +    }},
>>>>> +    .default_value = {.val.u64 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
>>>>> +    .encoding = OA_ENC_ULEB128,
>>>>> +  },
>>>>> +};
>>>>
>>>> I went to look at the doc referenced by [2] in the cover letter, but I couldn't
>>>> find anything there about the known-ness of these two tags.
>>>>
>>>
>>> Those two tags were added for testing purpose only, they are not defined
>>> in the specifications. Ideally they should only be available for testing
>>> via a runtime flag, or maybe developer mode and disabled for release.
>>>
>>> Richard Earnshaw and I discussed this issue off-line previously, he
>>> agreed that it is not ideal to do a release build with those.
>>>
>>> However, there is neither existing define to check whether the project
>>> was compiled in maintainer mode or release mode, nor a runtime flag for
>>> a testing mode. We decided to postpone this discussion later as it would
>>> involve introducing new flags or add a new define for the build mode.
>>> This would extend the scope of the patch series again, and we certainly
>>> don't want that given the size of this patch series.
>>
>> In a recent patch of mine I had the need to tell release builds from debug
>> ones, and I came up with the check you can now find in gas'es
>> perform_an_assembly_pass(). See commit 7b40f4c6587c ("gas: add a means to
>> programmatically determine the assembler version").
>>
> 
> I tried to address this concern as a part of 
> https://inbox.sourceware.org/binutils/20250814151629.174026-1-matthieu.longo@arm.com/.
> 
> I am not sure how to progress on this after the concern you raised on 
> potential conflicts with some distro's versioning, and that I cannot 
> rely on either gas or ld to determine the release mode.

I've replied there with another (not entirely nice, but hopefully workable)
suggestion.

>>>>> +/* Extract an integer literal from the input.
>>>>> +   Anything matched by O_constant is considered an integer literal (see the
>>>>> +   usage of O_constant in expr.c to see all the matches.  */
>>>>
>>>> But an expression the value of is known only at the end of assembly is
>>>> deliberately not permitted?
>>
>> This was left unaddressed (unless ...
> 
> Yes, it is not permitted.

Hmm. May at least want calling out somewhere.

>>>>> +  if (exp.X_op != O_constant)
>>>>> +    {
>>>>> +      as_bad (_("invalid value, expected an integer literal"));
>>>>> +      goto bad;
>>>>> +    }
>>>>> +
>>>>> +  int64_t val = exp.X_add_number;
>>>>
>>>> Any reason not to use offsetT here (and wherever else applicable)?
>>>
>>> offsetT lets think that we are dealing with an offset here whereas we
>>> are not. This type makes the intent of the code more obscure, so I
>>> preferred to use int64_t (offsetT -> bfd_signed_vma -> int64_t).
>>
>> But hidden type conversions can be problematic, too. We use offsetT and
>> its siblings in many places where it's not offsets we're dealing with.
> 
> When you say "hidden type conversion", do you mean that it would not be 
> obvious that a conversion happened if one day for instance, someone was 
> changing offsetT to be unsigned, or being a structure, or something 
> similar ?
> 
> I could add an explicit cast as below to really make it clear that a 
> conversion could potentially happen:
>    int64_t val = (int64_t) exp.X_add_number;
> 
> Using offsetT (a higher abstraction level type) in this part of the code 
> does not really make sense as I mentioned previously. The code for OAv2 
> is dealing with raw integers, strings, then up to the backend to 
> interpret them how it needs.
> 
> What would the real benefit of using offsetT be here ?

That in a future where we want to support 128-bit targets this code wouldn't
silently break (or at least be at risk of breaking). If you absolutely want
to use int64_t, you will need to deal with possible truncation issues, even
though they're entirely theoretical as of now.

>>>>> +/* Parse an argument, and set its type accordingly depending on the input
>>>>> +   value, and the constraints on the expected argument.  */
>>>>> +static bool
>>>>> +obj_attr_parse_arg (arg_token_t expected_ttype,
>>>>> +		    bool (*match_identifier) (char c),
>>>>> +		    bool resolve_identifier,
>>>>> +		    arg_t *arg_out)
>>>>> +{
>>>>> +  const arg_token_t low_ttype = (expected_ttype & LT_MASK);
>>>>> +
>>>>> +  /* Note: symbol look-up for string literals is not available.  */
>>>>> +  if (((low_ttype & STRING) && *input_line_pointer == '"')
>>>>> +      || !(low_ttype & ~STRING))
>>>>> +    return extract_string_literal (arg_out);
>>>>> +
>>>>> +  if (((low_ttype & (UNSIGNED_INTEGER | SIGNED_INTEGER))
>>>>> +      && look_like_integer_literal (input_line_pointer))
>>>>> +      || !(low_ttype & ~(UNSIGNED_INTEGER | SIGNED_INTEGER)))
>>>>> +    return extract_integer_literal (arg_out, (low_ttype & UNSIGNED_INTEGER));
>>>>
>>>> What if someone wants to use an equate?
>>>
>>> What do you mean ?
>>> Something like the below ?
>>>     .aeabi_attribute (123 + 546), ~(2-1)
>>
>> That's expressions, which - yes - also ought to work imo. What I meant
>> with the comment is
>>
>>      .equ x, 1
>>      .eqv y, 2
>>      .aeabi_attribute x, y
>>
>> (as one of the most simple examples).
> 
> The equate is already supported. See the test 
> gas/testsuite/gas/aarch64/build-attributes/ba-1.s in patch 
> "Serialization and dumping tests of Object Attributes v2" as an example.

Hmm, okay, then I must still not have sufficiently grokked how this parsing
works.

Jan