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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:764
@@ +763,3 @@
+                              SHF_ALLOC) {
+  // It`s a 4 bytes of header + pointer to the contents of the .eh_frame section
+  // + the number of FDE pointers in the table.
----------------
ruiu wrote:
> ` -> '
Sorry, I dont get what you want me to do here ?

================
Comment at: ELF/OutputSections.cpp:844
@@ +843,3 @@
+template <class ELFT>
+void EhFrameHdr<ELFT>::addFde(uint8_t Enc, size_t Off, uint8_t *PCRel) {
+  if (Live && (Enc & 0xF0) == dwarf::DW_EH_PE_datarel) {
----------------
ruiu wrote:
> So, if Sec is nonnull but this->Sec can be null,
> 
>  - If this->Sec == nullptr, (this->Sec != Sec) can never be true (because Sec is not null), no warning is displayed.
>  - If this->Sec != nullptr, (this->Sec != Sec) is true only when somebody tries to assign another one.
> 
> But this is a minor point. Leave it as is if you think yours more readable.
```
if (this->Sec != Sec) {
 warning("multiple .eh_frame sections not supported for .eh_frame_hdr");
...
```
So initially (this->Sec != Sec) is always true, because this->Sec is null and Sec is not. Them are always not equal at first run. Warning **is** displayed, but it should not.


================
Comment at: ELF/OutputSections.cpp:1111-1113
@@ +1110,5 @@
+      if (!readByte(D, &FdeEnc))
+        return false;
+      break;
+    case 'P': {
+      uint8_t Enc;
----------------
ruiu wrote:
> I'm pretty sure that that is better if this code is just for checking consistency. We generally don't want to read any data unless it is absolutely necessary to link stuff. It is compiler's responsibility to feed consistent file to the linker, so the linker doesn't have to verify all bits. Doing less is important for performance.
Ok, done.


http://reviews.llvm.org/D15712