This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Implemented --no-dynamic-linker option
ClosedPublic

Authored by grimar on Feb 22 2017, 8:57 AM.

Diff Detail

Event Timeline

grimar created this revision.Feb 22 2017, 8:57 AM
ruiu edited edge metadata.Feb 22 2017, 9:36 AM

We do not add .interp if no -dynamic-linker is given. So, do you really need -no-dynamic-linker?

ruiu added a comment.Feb 22 2017, 9:39 AM

Also, if both -foo and -no-foo are given, the last one usually wins. But in this patch -no-dynamic-linker always wins no matter where that is in the command line. That is incosnsitent and confusing.

In D30258#683639, @ruiu wrote:

We do not add .interp if no -dynamic-linker is given. So, do you really need -no-dynamic-linker?

No, as I wrote in description for now for x86/x64 kernel I am looking into, I do not really need it,
they ignore everything except PT_LOAD segments.
For PPC it seems they explicitly fail if PT_INTERP is there (according to code I see).
So I implemented because "was there" and it was trivial.

ruiu added a comment.Feb 22 2017, 9:49 AM

I wouldn't add this unless it is implemented in the same way as other -no-foo options. Basically you don't need to edit anything but Driver.cpp.

grimar updated this revision to Diff 89474.Feb 23 2017, 12:35 AM
  • Addressed review comment.
ruiu accepted this revision.Feb 23 2017, 11:24 AM

LGTM with the following changes.

ELF/Driver.cpp
586

As you can see the code is roughly grouped in this function. Please keep it consistent. Probably you need to move the line here.

ELF/Options.td
152

Remove this blank line to keep the style consistent.

This revision is now accepted and ready to land.Feb 23 2017, 11:24 AM
grimar added inline comments.Feb 23 2017, 11:47 PM
ELF/Driver.cpp
586

Ok, I did not move it there intentionally because we have

Config->Discard = getDiscardOption(Args);

at its place which confused me, probably I had to.

But aside of this I find this grouping is confusing. It mixes real and "calculated" options.
I mean:

  • OFormatBinary is ok, just corresponds to OPT_oformat.
  • SectionStartMap is different, there is no such option, it uses OPT_section_start/OPT_Ttext/OPT_Tdata/OPT_Tbss
  • SortSection and Target2 are ok, because corresponds to OPT_sort_section/OPT_target2
  • UnresolvedSymbols depends on OPT_noinhibit_exec/OPT_error_undef/OPT_warn_undef/OPT_no_undefined/OPT_unresolved_symbols/OPT_z

General grouping is also not ideal IMO.

What I would suggest is:

  • Do not split Args.hasArg/getString/getInteger groups and sort options alphabetically. That probably much more useful.

I think code readers usually search option implementation by name and not by its type which is impossible to remember.

  • Move OFormatBinary, SortSection, Target2 back to that list, because them are regular options connected with single command line parameter which equals to their variable name.
  • Keep only SectionStartMap and UnresolvedSymbols outside in separate group.
ELF/Options.td
152

Blank line is intentional. Doesn't absence of blank line here

def no_export_dynamic: F<"no-export-dynamic">;
def no_fatal_warnings: F<"no-fatal-warnings">;

is a violation of style ? I think for regular options (not aliases and not ignored)
we always used blank line to separate them.

This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Feb 24 2017, 12:38 AM
ELF/Options.td
152

Looks like these two options have no empty separating line because does not
have HelpText ? That is inconsistent styling then.
At least because LTO options has HelpText but also have no blank lines.

I think it worth to add help text for all options that are active (not ignored).
For ignored options probably makes sence to put "Option is ignored" or something
as help text too.

I added HelpText for no_dynamic_linker before commiting.

ruiu added inline comments.Feb 24 2017, 10:26 AM
ELF/Driver.cpp
586

Maybe. Let me think about that. Some options depend on other options, but from this code it is not very easy to see.

ELF/Options.td
152

This change was inconsistent in this local context. The entire context might be inconsistent, but it is not good to make more it more inconsistent by making an exception inside an exception (if it is exceptional).