[PATCH] D40849: CodeGen: support an extension to pass linker options on ELF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:106-107
+    Streamer.EmitIntValue(5, 4);                           // namesz
+    Streamer.EmitIntValue(Descriptor.size() + 5, 4);       // descsz
+    Streamer.EmitIntValue(ELF::NT_LLVM_LINKER_OPTIONS, 4); // type
+    Streamer.EmitBytes("LLVM");                            // name
----------------
jhenderson wrote:
> I don't like the magic number 5 here. I know it's the length of the vendor string in the first instance, but it would be easy for somebody to miss updating this if the vendor name changes. Perhaps you could do something like:
> ```
> const char *Name = "LLVM";
> Streamer.EmitIntValue(sizeof(Name), 4);
> ...
> Streamer.EmitBytes(Name);
> ```
> What's the purpose of the "+5" in the descsz field? I assume this is to do with the additional null and version fields, and if so, it would be good for this to be explained somehow.
Sure, I can extract that name.  Sure, I can add a comment for the size calculation.


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:109-110
+    Streamer.EmitBytes("LLVM");                            // name
+    Streamer.EmitIntValue(0, 1);                           // null
+    Streamer.EmitValueToAlignment(4);                      // desc
+    Streamer.EmitIntValue(1, 4);                           // version
----------------
jhenderson wrote:
> I'm confused as to why you have to explicitly write a null character here instead of the code automatically using .asciz (like it does for the option string).
I wish that it would add the trailing null, but it just does not.  This happens in the other backends as well where we are forced to write the trailing null.


================
Comment at: test/tools/llvm-readobj/elf-directives.test:23-24
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         ''
+  - Name:            .note.linker-options
----------------
jhenderson wrote:
> I don't think you need AddressAlign or Content fields here. In fact, do you need the section at all?
No, I suppose I don't need the section at all.  I just did a simple obj2yaml.


================
Comment at: test/tools/llvm-readobj/elf-directives.test:29-32
+  - Name:            .note.GNU-stack
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         ''
----------------
jhenderson wrote:
> Is this important to the test?
No, `.note.GNU-stack` is not needed for this test.


================
Comment at: test/tools/llvm-readobj/elf-directives.test:33-38
+Symbols:
+  Local:
+    - Name:            elf-linker-options.ll
+      Type:            STT_FILE
+DynamicSymbols:
+...
----------------
jhenderson wrote:
> Do you need Symbols/DynamicSymbols at all?
Nope, the `.file` symbol is not needed in this test either.


Repository:
  rL LLVM

https://reviews.llvm.org/D40849