[PATCH] D56437: Support blank flag in SHT_GROUP sections for ELF

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
Comment at: lld/ELF/InputFiles.cpp:399
 
+  SmallVector<std::pair<uint32_t, uint32_t>, 8> GroupDependencies;
+
----------------
This could be defined closer to its first use.


================
Comment at: lld/ELF/InputFiles.cpp:451
 
-      // If it is a new section group, we want to keep group members.
       // Group leader sections, which contain indices of group members, are
       // discarded because they are useless beyond this point. The only
----------------
I think this comment belongs on line 472


================
Comment at: lld/ELF/InputFiles.cpp:455
       // object files, we want to pass through basically everything.
-      if (IsNew) {
+      else {
+        for (uint32_t SecIndex : Entries.slice(1)) {
----------------
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.


================
Comment at: lld/ELF/InputFiles.cpp:461
+	  // Model the sections inter-dependencies though DependentSections.
+	  // Unfortunately, some references are forward references, so apply them later.
+          for (uint32_t GroupSecIndex : Entries.slice(1)) {
----------------
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. 


================
Comment at: lld/ELF/MarkLive.cpp:179
+  case SHT_NOTE:
+    // Notes withing a group can be dismissed if the group is not live.
+    return !(Sec->Flags & SHF_GROUP);
----------------
typo withing -> within


================
Comment at: lld/ELF/MarkLive.cpp:274
     // If -gc-sections is missing, no sections are removed.
-    for (InputSectionBase *Sec : InputSections)
+    for (InputSectionBase *Sec : InputSections) {
       Sec->Live = true;
----------------
LLD doesn't use {} for a single statement loop body.


================
Comment at: lld/ELF/MarkLive.cpp:309
     bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);
-    if (!IsAlloc && !IsLinkOrder && !IsRel)
+    bool IsNote = Sec->Type == SHT_NOTE;
+    if (!IsAlloc && !IsLinkOrder && !IsRel && !IsNote) {
----------------
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)


================
Comment at: lld/ELF/MarkLive.cpp:310
+    bool IsNote = Sec->Type == SHT_NOTE;
+    if (!IsAlloc && !IsLinkOrder && !IsRel && !IsNote) {
       Sec->Live = true;
----------------
No {} here either.


================
Comment at: lld/ELF/OutputSections.cpp:95
     Entsize = IS->Entsize;
+    // Group flag is no longer relevant at that point (?), at least for notes.
+    // FIXME: This is definitievly not the right place to do that.
----------------
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. 


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56437/new/

https://reviews.llvm.org/D56437