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

[James Henderson via Phabricator via llvm-commits]

jhenderson added subscribers: ruiu, rafael, jhenderson.
jhenderson added a reviewer: jhenderson.
jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Context missing. The SHT_NOTE section has a well-defined format, which this doesn't appear to conform to. If you want this to conform to a NOTE section format, you'll need to change the struct somehow. See http://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section. You could just make this non-NOTE (e.g. SHT_PROGBITS), and have the linker rely on only the name (in which case I'd just go with name it ".linker_options", or similar).

As for the format, can I suggest a couple of changes:

1. Make the version field a ULEB, rather than a .long, to save space. Given we're using variable length strings, there's no particular reason to have fixed width fields here, I'd say.
2. Assuming the "Reserved" field is meant to be the "null string", why not have an empty string as the last array member, rather than a uin32_t? Or a count/length field in the struct, which says how many options there are (or how big the total size is)? (Am I right in understanding the purpose of the "Reserved" field?)

We have a similar structure in our proprietary linker. One of the things it allows us to do is insert libraries immediately after the object file on the command-line. Where is it expected that the linker adds these command-lines? If immediately after the object file, what is the linker expected to do in -r links, where the sections contents would normally be concatenated?

Could you please add comments in the code, describing the structure and its purpose. Also, how are you planning on documenting this more widely?

I've also added @ruiu and @rafael to the subscribers, for their input from an LLD perspective.



================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:103
+        Directive.push_back(' ');
+        Directive.append(cast<MDString>(Option)->getString());
+      }
----------------
compnerd wrote:
> jakehehrlich wrote:
> > Shouldn't the strings be separated by null characters instead?
> No, this is the space for the "spaced option".  See the test :-).
.ascii directives, as emitted here, are not null terminated. You need to use .asciz. As things stand the output will look like " spaced option nospace" if you look at the binary, which means the linker won't know how to split it up.


Repository:
  rL LLVM

https://reviews.llvm.org/D40849