[PATCH v9 12/19] Merge of Object Attributes v2 during linkage (generic logic)
Jan Beulich
jbeulich@suse.com
Fri Nov 14 07:46:32 GMT 2025
On 13.11.2025 18:49, Matthieu Longo wrote:
> On 31/10/2025 11:44, Jan Beulich wrote:
>> On 01.09.2025 18:56, Matthieu Longo wrote:
>>> --- a/bfd/elf-attrs.c
>>> +++ b/bfd/elf-attrs.c
>>> @@ -18,6 +18,108 @@
>>> Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
>>> MA 02110-1301, USA. */
>>>
>>> +/* Design note regarding the merge of Object Attributes v2 during linkage
>>> +
>>> + Entry point: _bfd_elf_link_setup_build_attributes
>>> +
>>> + This patch adds all the generic logic to the linker to process OAv2.
>>> + The linker is an "advanced" consumer of OAv2. After parsing, it deduplicates
>>> + them, merge them, detect any compatibility issues, and finally translate them
>>> + to GNU properties.
>>
>> Nit: "merges", "detects", "translates" (also in the commit message).
>>
>
> Fixed.
>
>> As to the translation to GNU properties: Is the same information then recorded
>> in two different forms in the final binary? Or are the attributes dropped, and
>> only the properties kept?
>>
>
> The same information is recorded in two different forms, if the GNU
> property has a OAv2 equivalence, or if a OAv2 has a GNU property
> equivalence.
>
> GNU properties are used by the runtime linker and there is no plan to
> migrate to OAv2 in my understanding because:
> 1. the look-up is faster.
> 2. backward compatibility.
>
>> Could this translation, at the very least, be split off of this
>> overly large patch?
>
> Done, but in my opinion, the benefit of it for the review is really
> minimal, and it makes the commit message even worse, because now,
> instead of having a whole description in one place, it is split in two.
Both to this and ...
>>> + ** Overall design
>>> +
>>> + The OAv2 processing pipeline follows a map-reduce pattern. Obviously, the
>>> + actual processing in GNU ld is not multi-threaded, and the operations are not
>>> + necessarily executed directly one after another.
>>> +
>>> + * Phase 1, map: successive per-file operations applied on the list of
>>> + compatible input objects.
>>> + 1. Parsing of the OAv2 section's data (also used by objcopy).
>>> + 2. Translation of relevant GNU properties to OAv2. This is required for the
>>> + backward-compatibility with input objects only marked using GNU
>>> + properties.
>>> + 3. Sorting of the subsections and object attributes. Further operations
>>> + rely on the ordering to perform some optimization in the processing of
>>> + the data.
>>> + 4. Deduplication of subsections and object attributes, and detection of any
>>> + conflict between duplicated subsections or tags.
>>> + 5. Translation of relevant OAv2 to GNU properties for a forward
>>> + -compatibility with the GNU properties merge.
>>> +
>>> + * Phase 2, reduce: OAv2 in input objects are merged together.
>>> + 1. Gathering of "frozen" values (=coming from the command-line arguments)
>>> + into a virtual read-only list of subsections and attributes.
>>> + 2. Merging of OAv2 from an input file and the frozen input.
>>> + 3. Merging of the results of step 2 together. Since the OAv2 merge is
>>> + commutative and associative, it can be implemented as a reduce.
>>> + However, GNU ld implements it as an accumulate because it does not
>>> + support multithreading.
>>> + Notes: the two merge phases also perform a marking of unsupported/invalid
>>> + subsections and attributes. This marking can be used for debugging, and
>>> + also more practically to drop unsupported optional subsections from the
>>> + output.
>>> +
>>> + * Phase 3, finalization of the output.
>>> + 1. Pruning of the unsupported/invalid subsections and attributes.
>>> + 2. Serialization of OAv2 data (also used by objcopy).
>>> + Notes:
>>> + - There is no translation of the merged OAv2 to GNU properties at this
>>> + stage, as the GNU properties merge has already all the information that
>>> + were translated in step 5 of stage 1.
>>> + - The GNU properties are currently required as the runtime linker does
>>> + not understand OAv2 yet.
>>> + - Phase 3 should also include a compatibility check between the final
>>> + merge result of the current link unit and input shared objects. I opted
>>> + for postponing this compatibility check, and GNU properties merge will
>>> + take care of it as it already does.
>>
>> The splitting into three phases also looks as if they could be boundaries at
>> which the patch could be split.
>
> I am really not convinced about this split.
> Unless you consider it mandatory to move forward, I prefer to abstain
> from it.
... this - splitting isn't mandatory, but as you have seen, I gave up reviewing
changes to one of the files here at some point. Solely reading through all of
your replies here has already taken excessively long, and I'm yet to write
replies where ones are needed. The bigger a change, the more likely that it'll
sit for a long time, since to review it one needs to find a big enough chunk of
time. Reviewing piecemeal is possible in theory, but doing a lot of reviews I
find that quite undesirable. IOW it is in your own interest to try and present
your work in manageable chunks.
>>> @@ -424,6 +526,100 @@ bfd_elf_set_obj_attr_contents (bfd *abfd, bfd_byte *buffer, bfd_vma size)
>>> abort ();
>>> }
>>>
>>> +/* Structure storing the result of a search in the list of input BFDs.
>>> + - the pointer to the BFD.
>>> + - the pointer to the section containing the object attributes. */
>>> +typedef struct
>>> +{
>>> + bfd *pbfd;
>>> + bool has_build_attributes;
>>> + asection *sec;
>>> +} bfd_search_result_t;
>>> +
>>> +/* Checks whether a BFD contains object attributes, and if so search for the
>>> + relevant section storing them. */
>>> +static bool
>>> +bfd_has_build_attributes (bfd *abfd, bfd_search_result_t *res)
>>> +{
>>> + if (elf_obj_attr_subsections (abfd).size == 0)
>>> + return false;
>>> + res->has_build_attributes = true;
>>> +
>>> + const char *sec_name = get_elf_backend_data (abfd)->obj_attrs_section;
>>> + if ((res->sec = bfd_get_section_by_name (abfd, sec_name)) == NULL)
>>> + return false;
>>> + return true;
>>> +}
>>
>> This is an odd interface: The sole caller sets res->pbfd, just to pass in the
>> same pointer. Why would the filling of the structure not be done solely here?
>>
>
> Indeed I can move the filling of the structure inside
> bfd_has_build_attributes(). I moved res->pbfd inside
> bfd_has_build_attributes and added comments to explain better what this
> boolean means.
>
> /* Search for the first input object file containing object attributes.
> If no such object is found, PBFD points to the last object file that
> could have contained object attributes. HAS_OBJECT_ATTRIBUTES allows
> to distinguish the cases when PBFD contains or does not contain object
> attributes. If no candidate file is found, PBFD will stay NULL. */
> static bfd_search_result_t
> bfd_linear_find_first_with_obj_attrs (const struct bfd_link_info *info)
> ...
>
>> Further, is has_build_attributes actually necessary as a separate field? Can't
>> pbfd (being NULL or non-NULL) fulfill its purpose?
>>
>
> When pbfd is NULL, it means that no candidate file to host an OAv2
> section was found.
> Has_object_attributes means that the file contains object attributes
> that are not necessarily part of an OAv2 section (sec might be NULL).
> For instance, they could have been added by the translation of GNU
> properties to OAv2.
>
>> Finally (I think I had mentioned this before, but I may be misremembering and
>> it was in another context): Going solely be section name is, imo, problematic.
>> ELF has section types for a reason.
>>
>
> 1. How does GNU Properties handle the case of several sections with the
> expected type, but with the same expected name, or different names ?
>
> Regarding the parsing of GNU properties, if several sections with
> NT_GNU_PROPERTY_TYPE_0 are met, all of them are deserialized and their
> content is appended to the list of GNU properties for this BFD input.
> Regarding the merge of those properties,
> _bfd_elf_link_setup_gnu_properties() does not seem to harmonize them
> before merging them. However, elf_merge_gnu_property_list() seems to
> handle their merge. I might be wrong, I find this code quite difficult
> to follow to be honest.
> In _bfd_elf_link_setup_gnu_properties(), the GNU properties section is
> searched only by name. The section type is never checked. If it does not
> find one with the expected name, it creates a new one.
For anything that's pre-existing we've lost anyway. I would hope though
that we could learn from past mistakes. See how e.g. SFrame has gained
its own section type, lack of which was one of the points I made during
review.
> 2. What about Object Attributes ?
>
> There was no mention of such cases in the Object Attributes
> specification, so I asked the author.
>
> He confirmed that he could not find such cases in either the 32-bit or
> 64-bit ABI that says anything about multiple .ARM.attributes sections.
> The tools are expected to only ever produced one section so this case
> never came up in practice. The closest thing the spec does say is that
> multiple instances of the same attribute within a subsection are not
> permitted. GNU as already handle this case to detect such duplication,
> check whether the attribute values are contradictory and emit an error,
> or if they match, deduplicate the value.
>
> When it comes down to intent. Build Attributes are defined per
> relocatable object so there is no need, or benefit, for there to be more
> than one. If attributes applied only to an individual section, then it
> could make sense to have a case where we have multiple .ARM.attributes
> sections, each describing the attributes for just one .text section. But
> this case is not supported by the current specification.
>
> 3. What will LLVM handle such an exotic object ?
>
> LLD will parse every .ARM.attributes sections it sees, but it will only
> "process" the last one, earlier ones will get ignored. This is more a
> quirk of the implementation rather than intent. In truth it is assumed
> that there would always be one .ARM.attributes section.
>
> 4. What does the author propose for such a case ?
>
> Each object file can contain at most one .ARM.attributes section. What a
> tool does when it encounters more than one in a single object is Q-o-I.
> For example it could give an error message.
> Making an object with more than one .ARM.attributes sections would be
> hard with GNU and LLVM as. Both GNU and Clang integrated assembler
> directives will only produce 1 section, and ld is expected to produce 1
> "merged" output. For this reason, it could make it awkward to test such
> cases.
Iirc objcopy can be used to insert pretty much arbitrary sections. What
I'm unsure about is whether one can control their types.
> I propose the following change to the existing implementation to clarify
> this situation, and document what GNU ld does.
> Please let me know if you think that it is clear enough.
I'm sorry (applicable both here and below): While I understand the good
intention behind presenting such incremental diff-s, they're of pretty
limited use to me. (Part of the problem being that you mailer badly
line-wraps things, btw.) I will want to see the next version of the patch,
even if this means another reviewing round afterwards.
>>> +/* Returns True if the given BFD is an ELF object with the target backend
>>> + machine code, non-dynamic (i.e. not a shared library), non-executable, and
>>> + has sections. False otherwise.
>>> + Note: this function is a convenient encapsulation of the predicate used to
>>> + search for objects containing object attributes in the list of BFDs. */
>>> +static bool
>>> +elf_may_contain_obj_attrs (struct bfd_link_info *info,
>>> + bfd *bed)
>>
>> "bed" stands for "backend data", like e.g. used ...
>>
>
> Renamed to abfd in the new revision.
>
>>> +{
>>> + const struct elf_backend_data *output_bed
>>> + = get_elf_backend_data (info->output_bfd);
>>
>> ... here. Please let's not use misleading variable names.
>
> and output_bed to output_bfd_bed.
Hmm, I don't complain about output_bed, and imo the new name is worse than
the original one.
>>> +{
>>> + asection *sec;
>>> + const char *sec_name = get_elf_backend_data (ebfd)->obj_attrs_section;
>>> + sec = bfd_make_section_with_flags (ebfd,
>>> + sec_name,
>>> + (SEC_READONLY
>>> + | SEC_HAS_CONTENTS
>>> + | SEC_DATA));
>>> + if (sec == NULL)
>>> + info->callbacks->fatal (_("%P: failed to create %s section\n"), sec_name);
>>> +
>>> + unsigned align
>>> + = (get_elf_backend_data (info->output_bfd)->s->elfclass == ELFCLASS64
>>> + ? 3
>>> + : 2);
>>
>> What in the format requires different alignment for 32- vs 64-bit ELF?
>>
>
> This is a copy-paste from _bfd_elf_link_create_gnu_property_sec() in
> bfd/elf-properties.c It is part of the original patch that introduced
> the feature.
>
> v1:
> https://inbox.sourceware.org/binutils/20210620225029.390239-1-hjl.tools@gmail.com/
> v2:
> https://inbox.sourceware.org/binutils/20210622235715.2813205-1-hjl.tools@gmail.com/
> v3:
> https://inbox.sourceware.org/binutils/20210624132411.1993105-1-hjl.tools@gmail.com/
>
> I don't know the original reason for it, and there was no discussion on
> this specific detail.
In such a case please at least add a comment calling out the unclarity.
>>> +/* Determine which merge policy will be applied to SUBSEC. The GNU policy are
>>> + detected from the name of the subsection. It should follow the following
>>> + pattern: "gnu-testing-XXXXXX-MERGE-<POLICY>".
>>> + Return one of the known merge policy if recognised, UNSUPPORTED otherwise. */
>>> +static gnu_testing_merge_policy
>>> +gnu_testing_merge_subsection (const char *subsec_name)
>>> +{
>>> + if (! gnu_testing_namespace (subsec_name))
>>> + return SUBSECTION_TESTING_MERGE_UNSUPPORTED;
>>> +
>>> + size_t subsec_name_len = strlen (subsec_name);
>>> + if (strcmp ("-MERGE-AND", subsec_name + subsec_name_len - 10) == 0)
>>> + return SUBSECTION_TESTING_MERGE_AND_POLICY;
>>> + else if (strcmp ("-MERGE-OR", subsec_name + subsec_name_len - 9) == 0)
>>> + return SUBSECTION_TESTING_MERGE_OR_POLICY;
>>> + else if (strcmp ("-MERGE-ADD", subsec_name + subsec_name_len - 10) == 0)
>>> + return SUBSECTION_TESTING_MERGE_ADD_POLICY;
>>> + else
>>> + return SUBSECTION_TESTING_MERGE_UNSUPPORTED;
>>> +}
>>> +
>>> +/* Merge policy Integer-AND: apply bitwise AND between REF and RHS. */
>>> +obj_attr_v2_merge_result
>>> +obj_attr_v2_merge_policy_AND (struct bfd_link_info *info ATTRIBUTE_UNUSED,
>>
>> I agree with the use of the term "policy" further up, but the functions are
>> actors, not policies, so I don't think their name should include the word.
>
> I removed "_policy" from the functions name.
>
>> As a non-static function this may also again want to gain _bfd_ or bfd_ as
>> a prefix? (Why is it non-static anyway? The sole caller looks to liver further
>> down in this same file.)
>
> The caller is in bfd/elfxx-aarch64.c
> See patch 14/19.
>
> I disagree on adding the prefix _bfd_ to this function, as it is
> internal to bfd (i.e. either used in elf-attr.c or a backend handler).
> However, splitting elf-bfd.h between public and private functions might
> help to clarify the situation, but this is out of scope of this patch
> series.
As to out of scope - absolutely. You may have seen though that meanwhile I
did some work to that effect. Things aren't globally consistent yet, but
the general guideline would be to have exported functions prefixed bfd_,
have extern but libbfd-internal functions prefixed _bfd_ (no matter that
this violates the C naming rules), and static functions without either of
the prefixes.
>>> +/* Report missing required attribute with key TAG in subsection SREF. */
>>> +static void
>>> +report_missing_required_obj_attr (struct bfd_link_info *info,
>>> + bfd *abfd,
>>> + obj_attr_subsection_v2 *s_ref,
>>> + obj_attr_tag_t tag)
>>> +{
>>> + const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>>> + const char *tag_s = obj_attr_v2_tag_to_string (bed, s_ref->name, tag);
>>> + info->callbacks->einfo (
>>> + _("%X%pB: error: missing required object attribute '%s' in subsection "
>>> + "'%s'\n"), abfd, tag_s, s_ref->name);
>>
>> Imo better:
>>
>> info->callbacks->einfo (
>> _("%X%pB: error: missing required object attribute '%s' in subsection '%s'\n"),
>> abfd, tag_s, s_ref->name);
>>
>> Splitting format strings should be avoided, unless the lines get really long (but
>> then the diagnostic text likely is too verbose anyway).
>>
>
> Ok, fixed in the next revision.
>
> What is the general recommendation when the string goes beyond the limit
> of 80 characters ?
Rule of thumb: First try to find more terse wording (while keeping things
understandable of course). Then accept going somewhat beyond 80. Then split
messages into multiple (output) lines (after all the printed messages also
should grow excessively long, or people may have trouble reading them), e.g.
by having a "core" error message and an "auxiliary" addendum.
>>> +/* Report required attribute A_ABFD mismatching with A_REF. */
>>> +static void
>>> +report_mismatching_required_obj_attr (struct bfd_link_info *info,
>>> + bfd *ref_bfd,
>>> + bfd *abfd,
>>> + obj_attr_subsection_v2 *s_ref,
>>> + obj_attr_v2 *a_ref,
>>> + obj_attr_v2 *a_abfd)
>>> +{
>>> + const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>>> + const char* tag_s = obj_attr_v2_tag_to_string (bed, s_ref->name, a_ref->tag);
>>> + if (s_ref->encoding == OA_ENC_ULEB128)
>>> + {
>>> + info->callbacks->einfo (
>>> + _("%X%pB, %pB: error: mismatching values 0x%x and 0x%x for "
>>
>> %#x
>
> See your previous comment below in v8.
> https://inbox.sourceware.org/binutils/99e171d4-64e6-4cfe-9c77-f47791373eed@suse.com/
Hmm, I really need to fix that. I'm going to repeat that comment again and
again. What I said there still applies - having printf()-like functions
which don't really behave printf()-like is simply problematic.
>>> +/* Merge case 3: S_ABFD does not have a S_REF equivalent.
>>> + 1. Create a new default-initialized S_REF subsection.
>>> + 2. Merge S_ABFD into S_REF.
>>> + 3. Insert S_REF into REF. */
>>> +static bool
>>> +handle_subsection_additional (struct bfd_link_info *info,
>>> + bfd *ref_bfd, bfd *abfd,
>>> + obj_attr_subsection_v2 *s_ref_next,
>>> + obj_attr_subsection_v2 *s_abfd,
>>> + obj_attr_subsection_v2 *s_frozen)
>>
>> There's nothing here that REF in the comment matches.
>>
>> (Giving up here.)
>>
>
> The comment starts by stating that there is not equivalent S_REF for
> S_ABFD, i.e. S_REF does not appear in the input parameters.
>
> What about the following phrasing ? Is it clearer ?
>
> Merge case 3: S_ABFD does not have an equivalent subsection in the
> current merge result.
> 1. Create a new default-initialized subsection S_REF from S_ABFD's
> properties.
> 2. Merge S_ABFD into S_REF.
> 3. Insert S_REF into the current merge result right before S_REF_NEXT.
Yes, thanks.
>>> --- a/bfd/elf-attrs.h
>>> +++ b/bfd/elf-attrs.h
>>> @@ -55,6 +55,16 @@ typedef union obj_attr_value_v2 {
>>> const char* string_val;
>>> } obj_attr_value_v2;
>>>
>>> +typedef enum obj_attr_v2_status
>>> +{
>>> + /* An attribute that is unknown to the linker, and so cannot be merged. */
>>> + obj_attr_v2_unknown = 0,
>>> + /* An attribute that was reported as corrupted. */
>>> + obj_attr_v2_corrupted,
>>> + /* A valid attribute. */
>>> + obj_attr_v2_ok,
>>> +} obj_attr_v2_status;
>>
>> Iirc already on an earlier version I asked that typedef-s please either be
>> omitted or be identified as such (by a _t suffix).
>
> Do you mean only for the enums or everything, even structs ?
Everything of course. What use would it be to limit things to only some of the
typedef-s?
>>> @@ -153,3 +179,36 @@ obj_attr_v2_find_known_by_tag (const struct elf_backend_data *,
>>> extern const char *
>>> obj_attr_v2_tag_to_string (const struct elf_backend_data *, const char*,
>>> obj_attr_tag_t);
>>> +
>>> +enum obj_attr_v2_merge_result_reason
>>> +{
>>> + /* Default: everything is ok. */
>>> + MERGE_OK = 0,
>>> + /* The result value of the merge is the same as REF. */
>>> + SAME_VALUE_AS_REF,
>>> + /* No implementation of a merge for this attribute exists. */
>>> + UNSUPPORTED,
>>> + /* The merge failed, an error message should be logged. */
>>> + ERROR,
>>> +};
>>
>> These identifiers would be fine if they lived in a .c file. For them to live in
>> a header, I think they need some disambiguating prefix. I also assume there's
>> no dependency anywhere that would require MERGE_OK to explictly have value 0
>> assigned (which it would get anyway if the "= 0" was dropped)?
>
> What about OAv2_MERGE_STATUS_(OK, SAME_VALUE_AS_REF, UNSUPPORTED, ERROR) ?
Much better, perhaps a little long now. Omit the STATUS part of the names?
>>> --- a/bfd/elf-bfd.h
>>> +++ b/bfd/elf-bfd.h
>>> @@ -1662,6 +1662,26 @@ struct elf_backend_data
>>> /* The size of the array of known subsections. */
>>> const size_t obj_attr_v2_known_subsections_size;
>>>
>>> + /* Translate the relevant GNU properties to object attributes v2. */
>>> + void (*translate_relevant_gnu_props_to_obj_attrs) (bfd *,
>>> + elf_property_list *);
>>> +
>>> + /* Translate the relevant object attributes v2 to GNU properties. */
>>> + void (*translate_relevant_obj_attrs_to_gnu_props) (bfd *,
>>> + obj_attr_subsection_v2 *);
>>
>> What is "relevant" intended to convey in the names?
>
> "Relevant" in this context means object attributes that have GNU
> properties equivalents.
>
> What about this ? Is it clearer ?
> /* Translate object attributes that have GNU property equivalents. */
>
> And a similar phrasing for the reciprocal function:
>
> /* Translate GNU properties that have object attributes v2 equivalents. */
The comments are better, but I still see no reason for "relevant" in the
names. What can't be translated simply can't be translated. That doesn't
need saying in already long-ish identifiers.
Jan
More information about the Binutils
mailing list