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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few suggestions about the code/style are below.



================
Comment at: ELF/SyntheticSections.h:937
+// all InputSections are sorted.
+// Normal entry for a known Section; IsSentinel = false, LinkSec = Section.
+class ARMExidxSyntheticSection : public SyntheticSection {
----------------
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.


================
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
----------------
Could it be inlined in the constructor?

```
  RawData = { ... };
```

`getSize()` seems could either return a constant or `RawData.size()` then it seems.


================
Comment at: ELF/Writer.cpp:1479
+          for (InputSection *Dep : IS->DependentSections)
+            if (Dep->Type & (SHT_ARM_EXIDX))
+              return false;
----------------
```
Dep->Type & SHT_ARM_EXIDX
```


================
Comment at: ELF/Writer.cpp:1483
+        }
+        return false;
+      };
----------------
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.


================
Comment at: ELF/Writer.cpp:1490
+            Sec->addSection(make<ARMExidxSyntheticSection>(IS));
+      }
+    }
----------------
You do not need to wrap `for` in curly bracers.


================
Comment at: ELF/Writer.cpp:1518
         // Remember it here so that we don't have to find it again.
-        Sentinel->Highest = Sections[Sections.size() - 2]->getLinkOrderDep();
+        assert(Sentinel->IsSentinel == true);
+        Sentinel->LinkSec = Sections[Sections.size() - 2]->getLinkOrderDep();
----------------
```
assert(Sentinel->IsSentinel);
```


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

https://reviews.llvm.org/D58047