[PATCH] D37626: [ELF] Scan .eh_frame sections precisely in order to eliminate unused LSDAs and personality routines.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Sorry for the belated response. This patch seemed a bit too complicated to me, and I was thinking if there's a better way of doing it.

So, there are two tricky things in garbage-collecting .eh_frame section contents:

1. .eh_frame sections consists of multiple section pieces, and each piece can be alive or dead. This is different from a regular section because a regular section is alive or dead as a whole.
2. .eh_frame sections have reverse dependency to .text sections. Usually, if a section A has a relocation pointing to B, then B is alive if A is alive. However, for .eh_frame section pieces, this liveness propagates backwards.

To deal with the above problems, you scan all .eh_frame section pieces in this patch. So, our algorithm with this patch is no longer a simple graph traversal algorithm. I think that's the reason why it feels a bit too complicated to me. But I think if you add data members to InputSection for reverse dependencies, you can keep it as a simple graph traversal.

How about this?

1. Add a new member, EhSectionPieces vector, to InputSection. It holds the input section's FDEs and CIEs if exists. At the beginning of markLive, scan all .eh_frame sections to initialize the vectors.
2. To represent edges between InputSections through .eh_frame section pieces, explicitly add these dependencies to InputSection::DependentSections. For example, if a function has .eh_frame pieces, its personality function is added to its DependentSection. You can initialize it at the beginning of markLive.

After doing the above two things, all dependencies are explicitly represented, and all you have to do is to traverse the graph, make .eh_frame pieces alive, and visit DependentSections as usual. I think this is a cleaner way of doing garbage-collecting.



================
Comment at: ELF/InputSection.h:276
+  uint32_t Size : 8 * sizeof(uint32_t) - 1;
+  unsigned Live : 1;
   unsigned FirstRelocation;
----------------
If `unsigned` is not `uint32_t`, a padding would be created between Size and Live, so you should make Live uint32_t.

Also, `sizeof(uint32_t)` is 32, so `8 * sizeof(uint32_t) - 1` is always just 31.

  uint32_t Size : 31;
  uint32_t Live : 1;


================
Comment at: ELF/MarkLive.cpp:125
+
+  const uint32_t CieOffset = Fde.InputOff + 4 - ID;
+
----------------
We do not sprinkle `const` that much in lld if a local variable is obviously not mutated.


================
Comment at: ELF/MarkLive.cpp:131
+
+  auto Found = std::lower_bound(Cies.begin(), Cies.end(), CieOffset,
+                                [](EhSectionPiece *Cie, uint32_t Offset) {
----------------
We usually call this `It`.


================
Comment at: ELF/MarkLive.cpp:145
+                    const EhSectionPiece &Fde) {
+  const typename ELFT::uint FdeEnd = Fde.InputOff + Fde.Size;
+  // The first relocation is known to point to the described function,
----------------
`const typename ELFT::uint` -> uint64_t

You can always use `uint64_t`  to represent a value that can be either 32 or 64 bit.


================
Comment at: ELF/MarkLive.cpp:148-150
+  for (unsigned I = Fde.FirstRelocation + 1, N = Rels.size(); I < N; ++I) {
+    const RelTy &Rel = Rels[I];
+    if (Rel.r_offset >= FdeEnd)
----------------
This loop can be written as

  for (unsigned I = Fde.FirstRelocation + 1, N = Rels.size(); I < N && Rels[I].r_offset < FdeEnd; ++I)
      resolveReloc<ELFT>(EH, Rels[I], Fn);


================
Comment at: ELF/MarkLive.cpp:164
+                    const EhSectionPiece &Cie) {
+  const unsigned CieFirstRelI = Cie.FirstRelocation;
+  if (CieFirstRelI == (unsigned)-1)
----------------
I wouldn't assign it to a temporary local variable.


================
Comment at: ELF/MarkLive.cpp:165-166
+  const unsigned CieFirstRelI = Cie.FirstRelocation;
+  if (CieFirstRelI == (unsigned)-1)
+    return;
+
----------------
I still doubt if this check makes sense. If we visit an invalid CIE that doesn't point to anything and mark it alive, then that means we are going to emit an invalid CIE to the output file, no? It seems it is an error condition.


================
Comment at: ELF/MarkLive.cpp:269-271
+  // The flag is set if an executable section is added to the queue.
+  // We scan .eh_frame sections only if the flag is set.
+  bool ExecInQueue = false;
----------------
Isn't this a premature optimization? I wouldn't add this unless it is proved to be actually effective.


https://reviews.llvm.org/D37626