Page MenuHomePhabricator

Improve --warn-symbol-ordering.
AcceptedPublic

Authored by ruiu on Mar 7 2018, 9:30 AM.

Details

Summary

I originally tried to simplify code and then noticed that lld doesn't
do what it tells to the user by warn(). It says "unable to order
discarded symbol" but it actually can for sections eliminated by ICF.
With this patch, lld doesn't sort such sections.

Event Timeline

ruiu created this revision.Mar 7 2018, 9:30 AM
jhenderson accepted this revision.Mar 8 2018, 2:11 AM
jhenderson added a reviewer: espindola.
jhenderson added a subscriber: espindola.

LGTM (with nit). Having thought about it, I agree with this comment by @espindola:

I am OK with warning on both cases, but it feels like that would be a
separate warning. A non existing symbol showing up in the order file
probably means there is something wrong with how that files was
created. Two symbols that can't be independently moved because they are
in the same section (because of ICF or not) seems like a normal event in
most cases.

I also think it would be fairly simple to add - just detect if the priority was the default or not prior to changing it (though we'd need more info to get a better warning message. I don't mind this being spun off into a separate discussion.

P.S. Rafael - Can you confirm which of the two usernames you'd like us to use, please?

lld/ELF/Writer.cpp
1062

clang-format?

This revision is now accepted and ready to land.Mar 8 2018, 2:11 AM

Rui Ueyama via Phabricator <reviews@reviews.llvm.org> writes:

+ auto *Sec = dyn_cast_or_null<InputSectionBase>(cast<Defined>(Sym)->Section);
+ if (!Sec) {
+ Warning("unable to order absolute symbol");
+ continue;

}

This warning will now fire for some linker created symbols like
_GLOBAL_OFFSET_TABLE_, no?

Cheers,
Rafael

James Henderson via Phabricator <reviews@reviews.llvm.org> writes:

jhenderson added a reviewer: espindola.
jhenderson accepted this revision.
jhenderson added a subscriber: espindola.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM (with nit). Having thought about it, I agree with this comment by @espindola:

I am OK with warning on both cases, but it feels like that would be a

separate warning. A non existing symbol showing up in the order file
probably means there is something wrong with how that files was
created. Two symbols that can't be independently moved because they are
in the same section (because of ICF or not) seems like a normal event in
most cases.

I also think it would be fairly simple to add - just detect if the priority was the default or not prior to changing it (though we'd need more info to get a better warning message. I don't mind this being spun off into a separate discussion.

P.S. Rafael - Can you confirm which of the two usernames you'd like us to use, please?

The "espindola" account. I am terribly sorry for the noise. I wanted to
convert the older account to not use a google sign in but could not find
out how to do it.

Cheers,
Rafael

espindola added inline comments.Mar 27 2018, 8:42 PM
lld/ELF/Writer.cpp
1063

This will now fire on symbols the linker defines relative to an output section, no?

jhenderson added inline comments.Mar 28 2018, 1:53 AM
lld/ELF/Writer.cpp
1063

That would be a good thing, right? Attempting to order a symbol not associated with an OutputSection (whether linker-defined or not) would be impossible.

ruiu added inline comments.Mar 28 2018, 2:27 PM
lld/ELF/Writer.cpp
1063

Yeah, I think this is a desired behavior.

espindola added inline comments.Mar 28 2018, 3:53 PM
lld/ELF/Writer.cpp
1063

Warning would be good, calling it an absolute symbol in the warning is the issue.

> > This will now fire on symbols the linker defines relative to an output section, no?
> >
> That would be a good thing, right? Attempting to order a symbol not associated with an OutputSection (whether linker-defined or not) would be impossible.
Yeah, I think this is a desired behavior.

Warning would be good, calling it an absolute symbol in the warning is the issue.

The attached patch on top of your patch shows the
problem. _GLOBAL_OFFSET_TABLE_ is not an absolute symbol.

jhenderson added inline comments.Mar 29 2018, 1:27 AM
lld/ELF/Writer.cpp
1063

Ah, I see what you mean. Perhaps update the message to "absolute or linker-defined symbol"?

This wouldn't be strictly true, since if the linker were to define an input section-relative symbol it could reorder it (do we ever do this?), but should be clear enough.

espindola accepted this revision.Mar 29 2018, 9:02 AM
espindola added inline comments.
lld/ELF/Writer.cpp
1063

True. I was thinking about having two different messages, but just rewording is sufficient.

LGTM with the error updated.

Rui, are you still working on this?