[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,1 +766,3 @@
 
+//--eh-frame-hdr option tells linker to construct a header for all the .eh_frame
+// sections. This header is placed to a section named .eh_frame_hdr and also to
----------------
Add a space before "--".

================
Comment at: ELF/OutputSections.cpp:778-779
@@ +777,4 @@
+EhFrameHdr<ELFT>::EhFrameHdr()
+    : OutputSectionBase<ELFT>(".eh_frame_hdr", llvm::ELF::SHT_PROGBITS,
+                              llvm::ELF::SHF_ALLOC) {
+  // This is initial size without counting FDE`s entries descriptors size.
----------------
Remove "llvm::ELF::".

================
Comment at: ELF/OutputSections.cpp:780
@@ +779,3 @@
+                              llvm::ELF::SHF_ALLOC) {
+  // This is initial size without counting FDE`s entries descriptors size.
+  // Its a 4 bytes of header + pointer to the contents of the .eh_frame section
----------------
` -> '

================
Comment at: ELF/OutputSections.cpp:781
@@ +780,3 @@
+  // This is initial size without counting FDE`s entries descriptors size.
+  // Its a 4 bytes of header + pointer to the contents of the .eh_frame section
+  // + the number of FDE pointers in the table.
----------------
Its -> It's

================
Comment at: ELF/OutputSections.cpp:794
@@ +793,3 @@
+EhFrameHdr<ELFT>::getFdePc(uintX_t EhVA,
+                           typename const EhFrameHdr<ELFT>::FdeData &F) {
+  const endianness E = ELFT::TargetEndianness;
----------------
Can you remove "typename EhFrameHdr<ELFT>::"?

================
Comment at: ELF/OutputSections.cpp:800
@@ +799,3 @@
+  uintX_t FdeOff = EhVA + F.Off + 8;
+  switch (S) {
+  case dwarf::DW_EH_PE_udata2:
----------------
switch (F.enc & 0xF)

================
Comment at: ELF/OutputSections.cpp:831
@@ +830,3 @@
+  write32<E>(Buf + 8, this->FdeList.size());
+
+  // InitialPC -> Offset in .eh_frame,
----------------
It is better to advance Buf here.

  Buf += 12;

================
Comment at: ELF/OutputSections.cpp:851
@@ +850,3 @@
+void EhFrameHdr<ELFT>::assignEhFrame(EHOutputSection<ELFT> *Sec) {
+  if (this->Sec && this->Sec != Sec) {
+    warning("multiple .eh_frame sections not supported for .eh_frame_hdr");
----------------
Isn't parameter Sec be non-null? If it is, you can just do

  if (this->Sec != Sec)

================
Comment at: ELF/OutputSections.cpp:1122-1124
@@ +1121,5 @@
+    case 'R':
+      if (!readByte(D, &FdeEnc))
+        return false;
+      break;
+    case 'P': {
----------------
We can exit from this function as soon as we read data, no?

  return readByte(D, &Cie.FdeEncoding));

================
Comment at: ELF/OutputSections.cpp:1136
@@ +1135,3 @@
+      D = D.slice(EncSize);
+      break;
+    }
----------------
Ditto

================
Comment at: ELF/OutputSections.h:436
@@ -433,1 +435,3 @@
 
+template <class ELFT> class EhFrameHdr final : public OutputSectionBase<ELFT> {
+  typedef OutputSectionBase<ELFT> Base;
----------------
Please add a comment to this class.

================
Comment at: ELF/OutputSections.h:437
@@ +436,3 @@
+template <class ELFT> class EhFrameHdr final : public OutputSectionBase<ELFT> {
+  typedef OutputSectionBase<ELFT> Base;
+  typedef typename llvm::object::ELFFile<ELFT>::uintX_t uintX_t;
----------------
You can remove this typedef.


http://reviews.llvm.org/D15712