[PATCH v8 12/19] Merge of Object Attributes v2 during linkage (generic logic)

[Jan Beulich]

On 01.09.2025 15:53, Matthieu Longo wrote:
> On 2025-09-01 08:37, Jan Beulich wrote:
>> On 29.08.2025 18:37, Matthieu Longo wrote:
>>> On 2025-08-22 10:50, Jan Beulich wrote:
>>>> On 15.07.2025 13:39, Matthieu Longo wrote:
>>>>> +/* Merge object attributes from object file ABFD into REF_BFD.  */
>>>>> +static bool
>>>>> +oav2_subsections_merge (struct bfd_link_info *info, bfd *ref_bfd, bfd *abfd)
>>>>> +{
>>>>> +  bool success = true;
>>>>> +  obj_attr_subsection_list *out_frozen_subsecs
>>>>> +    = &elf_obj_attr_subsections (info->output_bfd);
>>>>> +  obj_attr_subsection_list *abfd_subsecs = &elf_obj_attr_subsections (abfd);
>>>>> +  obj_attr_subsection_list *ref_subsecs = &elf_obj_attr_subsections (ref_bfd);
>>>>> +
>>>>> +  obj_attr_subsection_v2 *s_frozen_first = out_frozen_subsecs->first;
>>>>> +  obj_attr_subsection_v2 *s_abfd = abfd_subsecs->first;
>>>>> +  obj_attr_subsection_v2 *s_ref = ref_subsecs->first;
>>>>> +
>>>>> +  /* Translate object attributes from abfd to GNU properties if they have an
>>>>> +     equivalence.  */
>>>>> +  _bfd_elf_translate_relevant_obj_attrs_to_gnu_props (abfd);
>>>>
>>>> How does this call fit here? Below you're only dealing with attributes,
>>>> afaict.
>>>
>>> ABFD is an input file. It might have either GNU properties only, object
>>> attributes only, or both.
>>>
>>> Before the merge of object attributes occur, we need to make sure that
>>> the GNU properties are translated to their object attributes equivalents
>>> (if they exists).
>>>
>>> In the same way, before the merge of GNU properties, we need to make
>>> sure that the object attributes are translated to their GNU properties
>>> equivalents (if they exists).
>>>
>>> In this implementation, the merge of object attributes always occurs
>>> before the merge of GNU properties.
>>
>> May I then ask that you extend the comment some, to mention not only what
>> is done, but also why it's done here?
>>
> 
> ABFD is an input file that may contain GNU properties, object 
> attributes, or both.  Before merging object attributes, we must first 
> translate any GNU properties into their equivalent object attributes (if 
> such equivalents exist) since they may not already be present.
> 
> Note: object attributes are always merged before GNU properties. 
> Ideally, there would be a single internal representation, with an 
> abstraction level flexible enough to capture both GNU properties and 
> object attributes without loss.  In such a design, merge order would be 
> irrelevant, and translation would occur only at the I/O boundaries 
> during deserialization of GNU properties and object attributes.
> 
> 
> Is this clear enough ?

Yes; it's maybe a little too verbose now.

>>>>> +      else /* (subsec->encoding == NTBS)  */
>>>>> +	{
>>>>> +	  if (strcmp (a->vals.string_val, a->next->vals.string_val) != 0)
>>>>
>>>> Code in obj_attr_v2_tag_merge_ADD() suggests that vals.string_val can be NULL,
>>>> in which case it would be invalid to pass to strcmp().
>>>
>>> oav2_file_scope_merge_subsections(), which calls oav2_compact_tags(), is
>>> called before oav2_merge_attrs().
>>> It compacts the content of subsections before the merge process starts,
>>> because the merge process has for the following requirements on an input
>>> object file:
>>> - the subsections have to be sorted in alphabetical order.
>>> - the attributes inside a subsection have to be sorted in ascending order.
>>> - the subsections and their attributes have to be unique.
>>>
>>> The value of a string attribute can only be NULL if this attribute was
>>> created by the merge process.
>>> Since this code runs before the merge process, no worry about the string
>>> being NULL and strcmp() can work assuming that the string will never be
>>> NULL.
>>
>> As that's far from obvious here, may I ask that you add a comment and maybe
>> also an assertion?
> 
> What about this ?
> 
> /* For NTBS encoding, ensure that the string value of the attributes is 
>   not NULL.
>     Note: a string attribute can only be NULL if it was created during 
> the merge process.  Since tag compaction occurs before merging begins, 
> this assertion guarantees that the function is never called in a context 
> where the assumption does not hold.  */
>    BFD_ASSERT
>      (subsec->encoding == OA_ENC_ULEB128 ||
>       (a->val.string_val != NULL && a->next->val.string_val != NULL));

Looks okay.

Jan