Page MenuHomePhabricator

Support blank flag in SHT_GROUP sections for ELF
Needs ReviewPublic

Authored by serge-sans-paille on Tue, Jan 8, 6:39 AM.



It is possible to use group sections outside of COMDAT, but that's unsupported by lld. This makes lld unusuable

This is a tentative patch that just use the section content, without the duplication logic implied by COMDAT.

Diff Detail

rLLD LLVM Linker

Event Timeline

ruiu added a comment.Tue, Jan 8, 8:58 AM

Why are you trying to do this? It is intentional that only GRP_COMDAT is supported. That's the only flag defined by the ELF spec as you can see here:

Other flags may be added in the future, but we'd implement them when they are added. Blindly handling all the undefined flags as if they were GRP_COMDAT is simply wrong. We should reject unknown flags.

For some background, the static glibc.a in the latest (unreleased) build of Fedora contains these non-comdat group sections.

There are 2 types of group, one called (unlikely is spelled wrongly in the object files) and one called The .text.unlikely and sections usually come about when the gcc optimizations like -freorder-blocks-and-partition and -freorder-functions are used. However I've never seen them generate an ELF group before, I can't for the life of me reproduce with trunk gcc, and I can't find them in the source code. Looking at the objects in the glibc-static-2.28.9000-28.fc30.i686.rpm I can see a with these contents:


I think that the sections are not yet part of gcc trunk. There is a plugin mentioned in so it is possible that these groups are only used when this plugin is used. As this is glibc then it would make lld unusable on the latest Fedora distributions. It would be good to try and find out why these groups are being created and whether it is down to the gcc plugin mentioned in the Watermark link.

I've not had a chance to look over the code in fine detail yet. At a quick glance:

  • I think the approach of just loading the sections is fine.
  • I suggest writing the test in assembler as we tend to use that unless an object can't be built using it. It would need .section, "axG", @progbits, .group to create the non comdat group.

Thanks @psmith for the context. I just found the culprit line in gas assembler:

But as mentionned by @ruiu, I cannot find any evidence of this being a standard behavior :-/ For instance MC/ELFObjectWriter.cpp unconditionnaly write all group sections as COMDAT.

I can confirm that the group sections are coming from the annobin tool.
From the man page:

When gcc compiles code with the -ffunction-sections option active it will place each function into its own section.  When the annobin attach option is active the plugin will attempt to attach the function section to a group containing the notes and relocations for the function.  In that way, if the linker decides to discard the function, it will also know that it should discard the notes and relocations as well.

The default is to enable attach, but the inverse option is available in case the host assembler does not support the .attach_to_group pseudo-op.  If this feature is disabled then note generation for function sections will not work properly.

From this appears to be on by default when building Redhat packages.

Both gold and ld.bfd appear to take into account groups when doing garbage collection. As it stands the patch would permit programs to link correctly, but if --gc-sections were on then LLD's implementation wouldn't throw away the sections (they are not SHF_ALLOC so are always marked live). This would lead to the tools that read the build information potentially giving incorrect results for some programs. How much of a problem this is would depend on the program, and how widely the watermark system is deployed.

grimar added a subscriber: grimar.Wed, Jan 9, 7:20 AM

I agree with Rui. My understanding is that groups in ELF were designed specifically to support C++ templates and inline functions which is why COMDAT groups are the only allowed kind.
Interestingly, I have been having a somewhat related discussion on this review:
Could the annobin tool use SHF_LINK_ORDER instead?

As a follow up to this review (and the original bug), a thread started on the binutils mlist:

A possible reading of the standard is that flags are additive and the absence of flag - i.e. 0, is a valid value.

The wording of the standard is unfortunate. This has been brought up and clarified (that COMDATs are the only legal groups currently) on the list previously.

@bd1976llvm interesting. Unfortunately I failed to find this thread :-/

@bd1976llvm interesting. Unfortunately I failed to find this thread :-/

What I think is happening here is that a mechanism, SHF_LINK_ORDER, was introduced into ELF for marking sections as associated with other sections for the purpose of garbage collection (dead/unused section removal). However, about the same time the GUI toolchain started using non-COMDAT groups in ELF to accomplish the same goal. In order to resolve this we will either need to get agreement from GNU to use SHF_LINK_ORDER instead or we will need to implement support for non-COMDAT ELF groups in LLVM in order to be compatible.

@ruiu given you answer to the thread above, I understand we can go on with that review. Anything you're expecting in addition to the current patch?

I've had a look at the code in a bit more detail and have left some comments. I think we should state somewhere, either as a comment or in a commit message about how we handle garbage collection/icf for Section Groups. As it stands LLD marks all non-SHF_ALLOC sections as live so the primary use case for the non-comdat section groups (ensure removal of function removes all the build notes associated with it) isn't going to happen.


We are getting more than just the entries now. Looking at how this function is now used (no assumption that the group is comdat), it might be better to inline this code in initializeSections so that the determination of type of group and any comment explaining the support can be in one place?


LLD coding style tends to favour not including { and } for a single statement if.


If getShtGroupEntries were inlined here we wouldn't need to mention the type of groups we support here.

Another thing we probably should mention is to what degree we support non-comdat groups. We unconditionally add the members, but we don't honour the garbage collection/icf behaviour implied by the specification for non-SHF_ALLOC sections in the group.


IsNew is associated with the hash table. Perhaps change the name to AddGroupMembers?


Could do

IsNew = true;
if (Flag !=0) {
        // De-duplicate comdat section groups by their signatures.
        StringRef Signature = getShtGroupSignature(ObjSections, Sec);
        IsNew = ComdatGroups.insert(CachedHashStringRef(Signature)).second;
        this->Sections[I] = &InputSection::Discarded;
ruiu added a comment.Mon, Jan 14, 4:41 PM

Given the discussion at the generic-abi mailing list, I'm fine with handling a group with no flags. However, I'd like you to add more code to this patch so that a group with no flags is actually handled as a group by garbage collector. Do you mind if I ask you do that?

@ruiu I'm not that fmailiar with ldd codebase but I'll happily give it a try!

ruiu added a comment.Tue, Jan 15, 11:27 AM

I *believe* that the following strategy seems like a good way to implement the feature (but I didn't try it personally so if you find it unable to work, take another path):

Every input section have a list of DependentSections. Usually the list is empty, but if the list is not empty, the GC handles it as edges when doing the mark-sweep garbage collection.

I think you can represent a group with DependentSections. If one section in a group is live, all sections in the same group are live. You can pick the first section (an arbitrary section) as a "leader" in a group and add all sections in the group to its DependentSection, and add the leader to every member's DependentSection.

I strongly feel that we shouldn't implement this. This will tie groups and --gc-sections together when they are orthogonal features; e.g. COMDAT groups will *also* have to be considered by --gc-sections as a single unit. The ability to strip individual sections form COMDAT groups is an important feature. I will add similar comment to the abi list. I'm surprised that gold implements this behavior, back in 2017 gold and bfd-ld disagreed on this.

serge-sans-paille marked 4 inline comments as done.

Added rough support for dependencies withing a group.

The test case I'm interested in focuses on SHT_NOTE within the same group as text sections.
This delays the pruning of SHF_GROUP flags until later into the linker processing, because SHT_NOTE were considered as live by default, but what we want is to consider notes *not* within a group as live.

This aprt of the code is somehow clumsy, I'm eager to read your feedbacks.

serge-sans-paille marked an inline comment as done.Wed, Jan 16, 1:52 AM

Given the comments from bd1976llvm about the intention vs the wording of the ELF specification, and the restriction in this patch of the behaviour to non-comdat groups (assuming intentional) then I think we should take a step back and decide why we want to make the change and what the requirements are. I think that if we are taking the annobin interpretation of the specification with respect to garbage collection and what to be strictly compliant then we should apply the behaviour to all groups. Another interpretation is that only use case for non-comdat groups is to affect garbage collection so we can handle them differently to comdat groups; given the only use case I've seen in many years is annobin then this could be low risk without adding group member squared extra dependencies for all comdat groups. Alternatively if we just want to support annobin then there may be a better way of doing this without involving groups. There is a relocation from each SHT_NOTE section to the code section that it describes. It could be possible to handle these in the same way as .ARM.exidx sections by making the SHT_NOTE section from the group dependent on the code section it relocates against. If we did that then you'd only need to preserve the SHF_GROUP flag on the SHT_NOTES section and wouldn't need to make all the sections in the group depend on each other.


This could be defined closer to its first use.


I think this comment belongs on line 472


If I understand it correctly we are adding dependencies between sections that are members of a group for non-comdat groups only? Whatever the interpretation of the spec is with respect to garbage collection the behaviour would apply equally to both comdat and non-comdat groups.


The ELF spec says that a SHT_GROUP section must come before any section that is a member of that group so I think all the references will be forward.

179 ↗(On Diff #181995)

typo withing -> within

274 ↗(On Diff #181995)

LLD doesn't use {} for a single statement loop body.

309 ↗(On Diff #181995)

I think this would unconditionally discard any SHT_NOTE section that wasn't part of a group as there would likely be no section to make it live. You'd probably need !(IsNote && IsGroupMember)

310 ↗(On Diff #181995)

No {} here either.

95 ↗(On Diff #181995)

It is true that we should only ever see SHF_GROUP in object files, type ET_REL (such as those produced by Config->Relocatable).

I think that this would only clear the SHF_GROUP flag from the first Input Section in the OutputSection. If there happened to be more than InputSection the SHF_GROUP would get added on line 125. This would need to go prior to line 89.

Will be worth test cases to make sure that SHF_GROUP flag isn't propagated into an OutputSection for executables and shared libraries.

Partial support of SHT_GROUP without flag

This does *not* implement full SHT_GROUP semantic, yet it is a simple step forward:
Sections within a group are still considered valid, but they do not behave as
specified by the standard in case of garbage collection.

@psmith considering your remark and the one from @bd1976llvm I 'd rather split the patch in two. This implements basic and partial support for SHT_GROUP. Instead of raising an error, it just acepts the sections as regular sections, and does not implement the dependency part. We can settle on a better support for annobin in another patch, but at least with this patch lld can be used as a normal linker on fedora.

I'm happy to split the patch into basic support and consider how we support GC separately.

emilio added a subscriber: emilio.Thu, Jan 17, 8:09 AM