[PATCH] D24728: [ARM][LLD] Add support for .ARM.exidx sections

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputFiles.cpp:250-253
@@ +249,6 @@
+
+// Find and mark InputSections that we do not want to combine into another
+// OutputSection when doing a relocatable link.
+template <class ELFT>
+void elf::ObjectFile<ELFT>::findRelocatableOrphanSections() {
+  if (Config->EMachine != EM_ARM)
----------------
This function returns true if we do not want to merge a section under --relocation option. I'm wondering if it should always return true for anything. In other word, we are able to not merge any section at all if --relocation is given. It'll change the current behavior, but I don't think it would do any harm.

================
Comment at: ELF/MarkLive.cpp:239
@@ -238,1 +238,3 @@
           Enqueue({Sec, 0});
+        if (Sec->Name.startswith(".ARM.exidx"))
+          Enqueue({Sec, 0});
----------------
You want to add `S.startswith(".ARM.exidx") to `isReserved` instead.

================
Comment at: ELF/OutputSections.cpp:882
@@ +881,3 @@
+  const typename ELFT::Shdr *Hdr = S->getSectionHdr();
+  if ((Hdr->sh_flags & SHF_LINK_ORDER) && Hdr->sh_link != 0) {
+    return S->getFile()->getSections()[Hdr->sh_link];
----------------
nit: remove {}

================
Comment at: ELF/OutputSections.cpp:890
@@ -879,1 +889,3 @@
   uint32_t Type = this->Header.sh_type;
+  if (this->Header.sh_flags & SHF_LINK_ORDER && !this->Sections.empty()) {
+    auto *D = getLinkOrderDependency(this->Sections.front());
----------------
Please add () for `&` because I (and probably a lot of people) don't memorize which has higher precedence, & or &&.

================
Comment at: ELF/OutputSections.cpp:890
@@ -879,1 +889,3 @@
   uint32_t Type = this->Header.sh_type;
+  if (this->Header.sh_flags & SHF_LINK_ORDER && !this->Sections.empty()) {
+    auto *D = getLinkOrderDependency(this->Sections.front());
----------------
ruiu wrote:
> Please add () for `&` because I (and probably a lot of people) don't memorize which has higher precedence, & or &&.
Can you please add a comment what this new code is for? (I think I know it now but probably it is a bit mysterious for first time readers.)

================
Comment at: ELF/Writer.cpp:1030
@@ -1025,1 +1029,3 @@
 
+// PT_ARM_EXIDX is the ARM EHABI equivalent of PT_GNU_EH_FRAME
+  if (ARMExidx.First)
----------------
Indentation

================
Comment at: ELF/Writer.cpp:1317
@@ -1308,1 +1316,3 @@
 
+static void ARMExidxEntryPrelToAbs(uint8_t* Loc, uint64_t EntryVA) {
+  uint64_t Addr = Target->getImplicitAddend(Loc, R_ARM_PREL31) + EntryVA;
----------------
Can you run clang-format-diff on this patch? A function comment would be appreciated.

================
Comment at: ELF/Writer.cpp:1336
@@ +1335,3 @@
+
+static void SortARMExidx(uint8_t* Buf, uint64_t OutSecVA, uint64_t Size) {
+
----------------
Please add a function comment.

================
Comment at: ELF/Writer.cpp:1339-1340
@@ +1338,4 @@
+  struct ARMExidxEntry {
+    uint32_t Target;
+    uint32_t Action;
+  };
----------------
I think you want to use `ulittle32_t` so that it will work even on big-endian machines.

================
Comment at: ELF/Writer.cpp:1372-1373
@@ +1371,4 @@
+  if (ARMExidx && !Config->Relocatable) {
+    auto OS = dyn_cast<OutputSection<ELFT>>(ARMExidx);
+    if (OS)
+      SortARMExidx(Buf + OS->getFileOff(),
----------------
  if (auto *OS = dyn_cast<OutputSection<ELFT>>(ARMEidx))


https://reviews.llvm.org/D24728