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

[George Rimar via llvm-commits]

grimar added inline comments.

================
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) {
----------------
ruiu wrote:
> Please use Type and Flags instead of sh_type and sh_flags because of the LLVM coding style.
Hmm not sure how that happend. Fixed.

================
Comment at: ELF/OutputSections.cpp:1031-1033
@@ -929,1 +1030,5 @@
 
+static void cieParseError() {
+  error("corrupted or unsupported CIE information");
+}
+
----------------
ruiu wrote:
> Do we need this? If this function is called only at a few places, I'd just inline it.
Now when there are few more specific errors introduced there is probably no need in that.

================
Comment at: ELF/OutputSections.cpp:1059
@@ +1058,3 @@
+  default:
+    llvm_unreachable("Unknown encoding");
+  case dwarf::DW_EH_PE_absptr:
----------------
ruiu wrote:
> This should be error() instead of llvm_reachable() because it is logically reachable if an input is corrupted.
Yes, nasty copypast issue :( I think I took it from llvm\tools\llvm-objdump\MachODump.cpp or llvm\lib\MC\MCDwarf.cpp where getSizeForEncoding() have the same one..

================
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
----------------
ruiu wrote:
>   if (readByte(D) != 1)
>     error(".eh_frame alignment must be 1")
Changed message to "CIE code alignment must be 1". As CIE also has data alignment factor.

================
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;
----------------
ruiu wrote:
> 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?
Its constructed from 
unsigned Index = S->Offsets.size();
...
Cie<ELFT> C(S, Index);
So it seems more correct would be to use size_t instead of unsigned. But I guess its not possible in real life to have overflow here (its hard to imagine).


http://reviews.llvm.org/D15712