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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:844
@@ +843,3 @@
+void EhFrameHdr<ELFT>::addFde(uint8_t Enc, size_t Off, uint8_t *PCRel) {
+  if (Live && (Enc & 0xF0) == dwarf::DW_EH_PE_datarel) {
+    error("DW_EH_PE_datarel encoding unsupported for FDEs by .eh_frame_hdr");
----------------
You can remove {}.

================
Comment at: ELF/OutputSections.cpp:1008-1009
@@ -908,4 +1007,4 @@
 template <class ELFT>
-EHOutputSection<ELFT>::EHOutputSection(StringRef Name, uint32_t Type,
-                                       uintX_t Flags)
-    : OutputSectionBase<ELFT>(Name, Type, Flags) {}
+EHOutputSection<ELFT>::EHOutputSection(StringRef Name, uint32_t sh_type,
+                                       uintX_t sh_flags)
+    : OutputSectionBase<ELFT>(Name, sh_type, sh_flags) {
----------------
Please use Type and Flags instead of sh_type and sh_flags because of the LLVM coding style.

================
Comment at: ELF/OutputSections.cpp:1031-1033
@@ -929,1 +1030,5 @@
 
+static void cieParseError() {
+  error("corrupted or unsupported CIE information");
+}
+
----------------
Do we need this? If this function is called only at a few places, I'd just inline it.

================
Comment at: ELF/OutputSections.cpp:1059
@@ +1058,3 @@
+  default:
+    llvm_unreachable("Unknown encoding");
+  case dwarf::DW_EH_PE_absptr:
----------------
This should be error() instead of llvm_reachable() because it is logically reachable if an input is corrupted.

================
Comment at: ELF/OutputSections.cpp:1086
@@ +1085,3 @@
+  uint8_t Version = readByte(D);
+  Check(Version == 1 || Version == 3);
+
----------------
This is probably better.

  if (Version != 1 && Version != 3)
    error("FDE version 1 or 3 expected, but got " + Version);

================
Comment at: ELF/OutputSections.cpp:1094
@@ +1093,3 @@
+  // Code alignment factor should always be 1 for .eh_frame.
+  Check(readByte(D) == 1);
+  // Skip data alignment factor
----------------
  if (readByte(D) != 1)
    error(".eh_frame alignment must be 1")

================
Comment at: ELF/OutputSections.cpp:1100-1105
@@ +1099,8 @@
+  // byte. In CIE version 3 this is an unsigned LEB128.
+  if (Version == 1)
+    readByte(D);
+  else if (Version == 3)
+    skipLeb128(D);
+  else
+    cieParseError();
+
----------------
At this point it is guaranteed that Version is either 1 or 3, so

  if (Version == 1)
    readByte(D);
  else
    skipLeb128(D);

================
Comment at: ELF/OutputSections.cpp:1117
@@ +1116,3 @@
+      // DW_EH_PE_aligned not yet supported.
+      Check((Enc & 0xf0) != dwarf::DW_EH_PE_aligned);
+      unsigned EncSize = getSizeForEncoding<ELFT>(Enc);
----------------
More specific error message would be appreciated.

================
Comment at: ELF/OutputSections.cpp:1130
@@ +1129,3 @@
+    default:
+      cieParseError();
+    }
----------------
More specific error message would be good. (error("Unknown XXX") or something like that.)

================
Comment at: ELF/OutputSections.h:288
@@ -287,3 +287,3 @@
 template <class ELFT> struct Cie : public EHRegion<ELFT> {
   Cie(EHInputSection<ELFT> *S, unsigned Index);
   std::vector<EHRegion<ELFT>> Fdes;
----------------
Is unsigned the right type here? If the linker runs on a 32-bit computer, it is 32-bit, but can it link a large 64-bit program?

================
Comment at: ELF/OutputSections.h:444
@@ +443,3 @@
+// This section contains a lookup table for quick binary search of FDEs.
+// Detailed info about internals can be found in Ian Lance Taylor`s blog:
+// http://www.airs.com/blog/archives/460 (".eh_frame")
----------------
Replace ` with ' (in general for propositions.)


http://reviews.llvm.org/D15712