[PATCH] D15712: [ELF] - implemented --eh-frame-hdr command line option.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:767
@@ -764,2 +766,3 @@
 
 template <class ELFT>
+EhFrameHdr<ELFT>::EhFrameHdr()
----------------
Please add a short comment with the URL to the Ian's blog article.

================
Comment at: ELF/OutputSections.cpp:771
@@ +770,3 @@
+                              llvm::ELF::SHF_ALLOC) {
+  this->Header.sh_size = 12;
+}
----------------
Is this value correct? (If correct, it needs a comment.)

================
Comment at: ELF/OutputSections.cpp:774
@@ +773,3 @@
+
+template <class ELFT>
+typename EhFrameHdr<ELFT>::uintX_t
----------------
This function needs a brief comment.

================
Comment at: ELF/OutputSections.cpp:783-791
@@ +782,11 @@
+  uintX_t PcRel;
+  switch (S) {
+  case dwarf::DW_EH_PE_udata2:
+  case dwarf::DW_EH_PE_sdata2:
+    PcRel = read16<E>(F.PCRel);
+    break;
+  case dwarf::DW_EH_PE_udata4:
+  case dwarf::DW_EH_PE_sdata4:
+    PcRel = read32<E>(F.PCRel);
+    break;
+  case dwarf::DW_EH_PE_udata8:
----------------
I'd make this a function so that you write

  case ...:
    return read16<E>(F.PCRel);

instead of

  case ...:
    PcRel = read16<E>(F.PCRel);
    break;

================
Comment at: ELF/OutputSections.cpp:829
@@ +828,3 @@
+
+  for (auto I = PcToOffset.begin(), F = PcToOffset.end(); I != F; ++I) {
+    // The first four bytes are an offset to the initial PC value for the FDE.
----------------
Why don't you use range-based for loop?

================
Comment at: ELF/OutputSections.cpp:860
@@ +859,3 @@
+template <class ELFT> void EhFrameHdr<ELFT>::reserveFde() {
+  this->Header.sh_size += 8;
+}
----------------
Ah, OK, so sh_size is initialized to 12 and change in size as you add new FDEs. You want to write about that as a comment.

================
Comment at: ELF/OutputSections.cpp:1032
@@ -931,2 +1031,3 @@
 
 template <class ELFT>
+static bool readByte(ArrayRef<uint8_t> &D, uint8_t *R = nullptr) {
----------------
Add a comment.

  // Read a byte and advance D by one byte.

I think you don't need to template this function.

================
Comment at: ELF/OutputSections.cpp:1133
@@ +1132,3 @@
+
+template <class ELFT> static bool skipLeb128(ArrayRef<uint8_t> &D) {
+  while (!D.empty()) {
----------------
You can remove template <class ELFT>. Please move this just below readByte.

================
Comment at: ELF/OutputSections.cpp:1183
@@ +1182,3 @@
+        Out<ELFT>::EhFrameHdr->Alive = false;
+        warning("corrupted or unsupported CIE information");
+      }
----------------
Isn't this a fatal error?

================
Comment at: ELF/OutputSections.h:450
@@ +449,3 @@
+
+  bool Alive = false;
+
----------------
Alive -> Live.

================
Comment at: test/ELF/eh-frame-hdr-no-out.s:2
@@ +1,3 @@
+// REQUIRES: x86
+// RUN: ld.lld --eh-frame-hdr %p/Inputs/invalid-cie-version2.elf -o %t
+// RUN: llvm-readobj -file-headers -s -section-data -program-headers -symbols %t | FileCheck %s --check-prefix=NOHDR
----------------
How did you create the binary file? Can you use an assembler to create that file?


http://reviews.llvm.org/D15712