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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm going to bring up the encoding again of this section. It's perfectly reasonable for users to have non-ASCII characters in their path (imagine e.g. Japanese developers), and whilst the back-end probably doesn't care (I assume the front-end will convert to whatever encoding we choose), readobj will print garbage if fed e.g. a UTF-8 string with non-ASCII characters. I'm happy to defer this point to not block this stage going in, since I don't think we have agreement on this yet (I'd support UTF-8 FWIW), but I think it needs to be addressed at the point when front-end support is being added.

Otherwise, I'm happy with this, barring the small suggestions I've made.



================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:99
+    auto *S =
+        C.getELFSection(".linker-options", ELF::SHT_PROGBITS, ELF::SHF_EXCLUDE);
+
----------------
It would be good to add a comment documenting the section format (and purpose?) here, even though it is simple.


================
Comment at: test/Feature/elf-linker-options.ll:6-8
+!1 = !{!"path", !"/Users/compnerd/DerivedData/binutils-gdb-linux-x86_64/libiberty"}
+!2 = !{!"rpath", !"/var/empty"}
+
----------------
Suggestion: Add a space to one of these to show that spaces are not special in any way.


================
Comment at: test/tools/llvm-readobj/elf-linker-options.ll:6-7
+!0 = !{!"lib", !"iberty"}
+!1 = !{!"path", !"/Users/compnerd/DerivedData/binutils-gdb-linux-x86_64/libiberty"}
+!2 = !{!"rpath", !"/var/empty"}
+
----------------
As with the other test, I'd suggest adding a space to one of these.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4091
+      StringRef Value = StringRef(reinterpret_cast<const char *>(P) + Key.size() + 1);
+
+      W.printString(llvm::StringSwitch<StringRef>(Key)
----------------
clang-format?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4095
+                        .Case("path", "library search path")
+                        .Default(Key),
+                    Value);
----------------
I got the impression that there was a bit of a debate about whether the library search path option should be allowed or not, so it may be a good idea to remove this one for now. I think the "lib" option is uncontroversial though.


Repository:
  rL LLVM

https://reviews.llvm.org/D40849