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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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
----------------
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.


================
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
----------------
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).


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


================
Comment at: test/tools/llvm-readobj/elf-directives.test:29-32
+  - Name:            .note.GNU-stack
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         ''
----------------
Is this important to the test?


================
Comment at: test/tools/llvm-readobj/elf-directives.test:33-38
+Symbols:
+  Local:
+    - Name:            elf-linker-options.ll
+      Type:            STT_FILE
+DynamicSymbols:
+...
----------------
Do you need Symbols/DynamicSymbols at all?


Repository:
  rL LLVM

https://reviews.llvm.org/D40849