[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: include/llvm/BinaryFormat/ELF.h:1384
+  NT_LLVM_LINKER_OPTIONS = 0x2302,
+};
+
----------------
jhenderson wrote:
> compnerd wrote:
> > jhenderson wrote:
> > > This number seems a bit arbitrary. Any particular reason for this value specifically?
> > No, there is no logical reasoning for this value.  Would another value be preferable?
> Assuming this is the first LLVM-vendor note, maybe 1? If not, what are the other note type values?
I used `1` originally as well.  However, the note values themselves are not valued by owner, but in a global pool.  We have values from various places like `0x53494749` which is if the `siginfo_t` is present in the core file that the ELF file represents.  Or did I manage to confuse myself when I looked at the values?


================
Comment at: test/Feature/elf-linker-options.ll:6
+
+!0 = !{!"spaced", !"option"}
+!1 = !{!"nospace"}
----------------
jhenderson wrote:
> ruiu wrote:
> > Imagine that these two strings are pathnames.  Pathnames can contain any character other than \0. That mean you can't distinguish "spaced options" from "spaced" and "options, which is bad. You should allow only one string.
> The way I see it, this is intended to be interpreted as a single option. However, I think this demonstrates why a key-value pair is important.
Yes, this is a single value.  " spaced option".  I don't remember what the issue was with trying to merge the entire spaced option into a single string.  But, that would simplify the logic here.  I think that we should go ahead and expect that the frontend will generate a single entry for that.  The key/value wouldn't help with this if the spaces were removed in the MDNode itself.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3473
+      return std::string(Note.Name);
+
+  std::string string;
----------------
jhenderson wrote:
> I don't think you need to explicitly convert to a string here.
True, the implicit constructor would work.  But, it is nicer to be explicit I think.


Repository:
  rL LLVM

https://reviews.llvm.org/D40849