[PATCH] D58047: [LLD][ELF][ARM] Synthesise missing .ARM.exidx sections.

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 6 inline comments as done.
peter.smith added a comment.

Thanks for the comments. I will upload another diff in a few minutes.



================
Comment at: ELF/SyntheticSections.h:937
+// all InputSections are sorted.
+// Normal entry for a known Section; IsSentinel = false, LinkSec = Section.
+class ARMExidxSyntheticSection : public SyntheticSection {
----------------
grimar wrote:
> Could you extend the comment? Without reading the patch description it
> is probably not clear what is EXIDX_CANTUNWIND entry and why
> normal and terminating entries are required.
Sure have done so. This was a valuable exercise as I remembered the precise reason we had to have a terminating sentinel (libunwind bug).


================
Comment at: ELF/SyntheticSections.h:949
+  // We only need to represent a single EXIDX_CANTUNWIND entry.
+  uint8_t Data[8] = {0, 0, 0, 0,  // PREL31 to target
+                     1, 0, 0, 0}; // EXIDX_CANTUNWIND
----------------
grimar wrote:
> Could it be inlined in the constructor?
> 
> ```
>   RawData = { ... };
> ```
> 
> `getSize()` seems could either return a constant or `RawData.size()` then it seems.
Not easily as RawData is a member of a base class. I could add another constructor for SyntheticSection that forwards Data to InputSection but I'm not sure it is worth it. Given that all sections will get the same value I've made it a static.


================
Comment at: ELF/Writer.cpp:1483
+        }
+        return false;
+      };
----------------
grimar wrote:
> This would probably look simpler a bit if was inversed + early return was used:
> 
> ```
> if (!IS->Live || IS->kind() == InputSectionBase::Synthetic ||
>             !(IS->Flags & SHF_EXECINSTR) || !IS->getSize())
> return false;
> ...
> ```
> 
> Also, so you need `IS->Live`? I would expect input sections included in output sections to be live.
I do as I'm iterating through InputSections and not the contents of the OutputSection. As it happens I found that adding the Sections at this point breaks LinkerScript Symbol assignment. I've moved the addition to the same place as the existing sentinel and I've made a modification to arm-exidx-sentinel-and-assignment.s so that it fails if I don't.


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

https://reviews.llvm.org/D58047