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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
Comment at: lld/ELF/InputFiles.cpp:324
 template <class ELFT>
-ArrayRef<typename ObjFile<ELFT>::Elf_Word>
+std::pair<typename ObjFile<ELFT>::Elf_Word, ArrayRef<typename ObjFile<ELFT>::Elf_Word>>
 ObjFile<ELFT>::getShtGroupEntries(const Elf_Shdr &Sec) {
----------------
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?




================
Comment at: lld/ELF/InputFiles.cpp:329
       CHECK(Obj.template getSectionContentsAsArray<Elf_Word>(&Sec), this);
-  if (Entries.empty() || Entries[0] != GRP_COMDAT)
+  if (Entries.empty() || (Entries[0] != GRP_COMDAT && Entries[0] != 0)) {
     fatal(toString(this) + ": unsupported SHT_GROUP format");
----------------
LLD coding style tends to favour not including { and } for a single statement if.


================
Comment at: lld/ELF/InputFiles.cpp:442
     case SHT_GROUP: {
-      // De-duplicate section groups by their signatures.
-      StringRef Signature = getShtGroupSignature(ObjSections, Sec);
-      bool IsNew = ComdatGroups.insert(CachedHashStringRef(Signature)).second;
-      this->Sections[I] = &InputSection::Discarded;
-
-      // We only support GRP_COMDAT type of group. Get the all entries of the
+      // We only support default and GRP_COMDAT type of group. Get the all entries of the
       // section here to let getShtGroupEntries to check the type early for us.
----------------
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.


================
Comment at: lld/ELF/InputFiles.cpp:449
+
+      bool IsNew;
+      if(Flag == 0) {
----------------
IsNew is associated with the hash table. Perhaps change the name to AddGroupMembers?


================
Comment at: lld/ELF/InputFiles.cpp:450
+      bool IsNew;
+      if(Flag == 0) {
+        IsNew = true;
----------------
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;
}
```


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56437