Page MenuHomePhabricator

[ELF] - Linkerscript: disallow discarding COMMON.
ClosedPublic

Authored by grimar on Aug 8 2017, 7:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Aug 8 2017, 7:37 AM
davide added a subscriber: davide.Aug 8 2017, 8:45 AM

What's the use case for discarding common symbols?
Also, when you say "ignores such scripts" you mean that gold accepts this syntax and doesn't discard the common?

grimar added a comment.Aug 8 2017, 8:50 AM

What's the use case for discarding common symbols?

I do not know about real use case, I worked on a D36466 and tried to discard COMMON during work on that
patch to see what -Map will contain, but LLD crashed here :)
We can probably restrict discarding COMMON instead, though it will be 1 line fix too.

Also, when you say "ignores such scripts" you mean that gold accepts this syntax and doesn't discard the common?

Yes.

ruiu edited edge metadata.Aug 9 2017, 7:14 AM

I'm lukewarm about this change. Such linker script doesn't make sense, so doing something for such nonsense seems to be just increasing the complexity of the code. You might still want to fix a crash bug, but attempting to do "the right thing" for this doesn't seem to worth it.

grimar added a comment.Aug 9 2017, 7:17 AM
In D36468#836637, @ruiu wrote:

I'm lukewarm about this change. Such linker script doesn't make sense, so doing something for such nonsense seems to be just increasing the complexity of the code. You might still want to fix a crash bug, but attempting to do "the right thing" for this doesn't seem to worth it.

Ok, I'll restrict discarding COMMON and update the diff then.

grimar updated this revision to Diff 110394.Aug 9 2017, 8:00 AM
grimar retitled this revision from [ELF] - Allow discard COMMON from linkerscript. to [ELF] - Linkerscript: disallow discarding COMMON..
grimar edited the summary of this revision. (Show Details)
ruiu accepted this revision.Aug 9 2017, 9:44 AM

LGTM

This revision is now accepted and ready to land.Aug 9 2017, 9:44 AM
This revision was automatically updated to reflect the committed changes.