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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D40849#953984, @compnerd wrote:

> Yes, gABI specifies pointer sized alignment, but that is not what currently occurs.  Everything is 4-byte aligned due to a mis-interpretation of the specification long ago, and now it has fossilized into the reality (yay for compatibility!).


I'm going to drop my concern here, since other note sections do this. Also, it looks like the direction of conversation is going in the direction of creating a new section type for 64-bit notes.

> Thinking more about this, while Im still not a fan of the split argument proposal, I think that is something that isn't needed for this part of the implementation.  That will be a separate change to clang which will actually process the pragma and generate the associated metadata.  This implementation should be able to accommodate both.

How would you distinguish between a key/value pair versus two separate options in this case?



================
Comment at: include/llvm/BinaryFormat/ELF.h:1384
+  NT_LLVM_LINKER_OPTIONS = 0x2302,
+};
+
----------------
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?


================
Comment at: test/Feature/elf-linker-options.ll:6
+
+!0 = !{!"spaced", !"option"}
+!1 = !{!"nospace"}
----------------
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.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3464-3469
+    uint32_t ID;
+    const char *Name;
+  } Notes[] = {
+    {ELF::NT_LLVM_LINKER_OPTIONS, "NT_LLVM_LINKER_OPTIONS (linker options)"},
+  };
+
----------------
compnerd wrote:
> jhenderson wrote:
> > Why not make this a proper map of some kind (e.g. DenseMap), so that you can use a straightforward lookup?
> It was the pattern that I had started when I was adding support for other note types in `llvm-readobj`.  This is just following that pattern.  Is a `DenseMap` really better than the linear lookup for a very small number of items (<8)?  If so, I can do that in a subsequent change.
Ok, if this is the common pattern, that's fine. I think that maps just look a little nicer to construct, since you don't need to define a new struct.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3473
+      return std::string(Note.Name);
+
+  std::string string;
----------------
I don't think you need to explicitly convert to a string here.


Repository:
  rL LLVM

https://reviews.llvm.org/D40849