[PATCH] D70020: [lld] Better support for group semantic wrt. notes

[Peter Smith via Phabricator via llvm-commits]

peter.smith added reviewers: peter.smith, MaskRay, grimar, bd1976llvm.
peter.smith added a comment.

Some additional conetxt. IIRC we originally discussed this as part of D56437 <https://reviews.llvm.org/D56437> . The specific part of annobin was the feature:

  "attach""no-attach"
  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.

In D56437 <https://reviews.llvm.org/D56437> there were some objections (https://reviews.llvm.org/D56437#1359385) to treating members of a COMDAT group in a keep-all/discard-all way. The non-gc impacting part of D56437 <https://reviews.llvm.org/D56437> was deferred to later. I guess this is it.

I don't have any objections. Adding some more reviewer that were involved in D56437 <https://reviews.llvm.org/D56437>.



================
Comment at: lld/ELF/InputFiles.cpp:552
+      // Save group entries for some post-processing.
+      groups.push_back(entries);
+
----------------
At this point we don't know whether the group has been accepted or not. Would it be possible to move this into the braces of the following (line 568)
```
      if (isNew) {
        if (config->relocatable)
          this->sections[i] = createInputSection(sec);
        continue;
      }
``` 
Possibly renaming it to selectedGroups? 

Given that a large number of COMDAT groups will be discarded in a large C++ program it is probably worth only scanning selected ones for SHT_NOTES sections. 


================
Comment at: lld/ELF/InputFiles.cpp:618
+
+  // The spec requires group entries to be inter-dependent, i.e. it's a
+  // discard-all or keep-all strategy. Implement that behavior for notes, so
----------------
I'd word the comment to mention why we are deviating from the specification here. Perhaps something like:
The ELF specification states "If the linker decides to remove the section group, it must remove all members of the group." The annobin tool places SHT_NOTES sections into groups, relying on the linker garbage collection to remove them if the group is removed. As LLD does not garbage collect non SHF_ALLOC sections we make the notes dependent on the non-SHT_NOTE group sections so that we can garbage collect them.


================
Comment at: lld/ELF/InputFiles.cpp:632
+    }
+    for (uint32_t secIndex : entries.slice(1)) {
+      if (!this->sections[secIndex] ||
----------------
I think you can skip this loop if notes is empty. The majority of groups won't have any SHT_NOTES sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70020