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

[George Rimar via llvm-commits]

grimar added inline comments.

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

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

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

================
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.
----------------
ruiu wrote:
> Why don't you use range-based for loop?
Interesting question. Fixed.

================
Comment at: ELF/OutputSections.cpp:860
@@ +859,3 @@
+template <class ELFT> void EhFrameHdr<ELFT>::reserveFde() {
+  this->Header.sh_size += 8;
+}
----------------
ruiu wrote:
> 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.
Added 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) {
----------------
ruiu wrote:
> Add a comment.
> 
>   // Read a byte and advance D by one byte.
> 
> I think you don't need to template this function.
Done.

================
Comment at: ELF/OutputSections.cpp:1133
@@ +1132,3 @@
+
+template <class ELFT> static bool skipLeb128(ArrayRef<uint8_t> &D) {
+  while (!D.empty()) {
----------------
ruiu wrote:
> You can remove template <class ELFT>. Please move this just below readByte.
Ah, yes, initially I did it as class member and forgot to remove after making static.
Fixed.

================
Comment at: ELF/OutputSections.cpp:1183
@@ +1182,3 @@
+        Out<ELFT>::EhFrameHdr->Alive = false;
+        warning("corrupted or unsupported CIE information");
+      }
----------------
ruiu wrote:
> Isn't this a fatal error?
Thats a question. I think it is not. Linker here just should merge the .eh_frame sections in the way it can. It does not need to support any new versions for example to do that. But .eh_frame_hdr can`t be generated then without knowing internals.
Since my patch is about generating eh_frame_hdr then its correct for it to be Live=false with warning I think.

================
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
----------------
ruiu wrote:
> How did you create the binary file? Can you use an assembler to create that file?
I dont know any way to generate corrupted CIE or CIE with specified version using assembler.
That is done by compiler itself I believe.
So I just took sample and then used hex editor to manually change the version.


http://reviews.llvm.org/D15712