Page MenuHomePhabricator

[ELF] Error if -Ttext-segment is specified
ClosedPublic

Authored by MaskRay on Nov 19 2019, 2:55 PM.

Details

Summary

In GNU ld, -Ttext sets the address of the .text section and -Ttext-segment sets the address of the text segment (RX).

gold only supports the -Ttext-segment semantic and treats -Ttext as an alias for -Ttext-segment.

lld only supports the -Ttext semantic and treats -Ttext-segment as an
alias for -Ttext. The text segment will be assigned to an address less
than the specified -Ttext-segment value.

This patch drops the -Ttext-segment alias.

The text segment is traditionally the first segment. Users who specify
-Ttext-segment may actually want to specify --image-base, the lld way to
express this. Unfortunately currently this is supported by GNU ld's
COFF port but not by its ELF port. gold does not support this option.
With -z separate-code, the behavior of GNU ld -Ttext-segment is weird (see https://sourceware.org/bugzilla/show_bug.cgi?id=25207)

rL289827 introduced the alias for linking qemu's non-pie user mode
binaries. As explained previously, this actually assigns the text
segment to an address less than 0x60000000. I feel that a better fix is
on the qemu side:
https://lists.nongnu.org/archive/html/qemu-devel/2019-11/msg02480.html

Event Timeline

MaskRay created this revision.Nov 19 2019, 2:55 PM
MaskRay edited the summary of this revision. (Show Details)Nov 19 2019, 3:28 PM
ruiu added inline comments.Nov 19 2019, 5:56 PM
lld/ELF/Driver.cpp
1015–1016

Could you expand the comment so that you explain a reasoning just as you explained it in the commit message?

MaskRay updated this revision to Diff 230192.Nov 19 2019, 9:22 PM

Expand a comment

MaskRay marked an inline comment as done.Nov 19 2019, 9:22 PM
ruiu accepted this revision.Nov 19 2019, 9:28 PM

LGTM

lld/ELF/Options.td
62

I'd remove HelpText so that it is recognized but doesn't appear in the --help message, unless you actually try to use the option. We have other unsupported options, and showing only this one seems a bit odd to me.

This revision is now accepted and ready to land.Nov 19 2019, 9:28 PM
MaskRay updated this revision to Diff 230194.Nov 19 2019, 9:37 PM
MaskRay marked an inline comment as done.

Remove HelpText from Ttext_segment.

grimar accepted this revision.Nov 20 2019, 12:18 AM

LGTM with a test.

lld/ELF/Driver.cpp
1020

-Ttext-segment->--Ttext-segment?
(here and in the comment)

I'd also suggest to add a test for this error. At least testing errors is useful for people who wants to search for a test for a particular option to see if it is supported and how.

MaskRay marked an inline comment as done.Nov 20 2019, 12:44 AM
MaskRay added inline comments.
lld/ELF/Driver.cpp
1020

-Ttext-segment is mix cased and documented in GNU ld as a one-dash option. Options that are documented as double-dash ones are exclusively lower case only. So I think we probably should just use -Ttext-segment.

MaskRay updated this revision to Diff 230312.Nov 20 2019, 12:21 PM

Forgot to git add a test...

This revision was automatically updated to reflect the committed changes.
troyj added a subscriber: troyj.Feb 4 2020, 9:50 AM

Unfortunately currently this is supported by GNU ld's COFF port but not by its ELF port. gold does not support this option.

Just picked up this commit downstream today and likely will be reverting it locally. I think it was a bad decision to remove an option like this when the expected replacement isn't supported by the other linkers yet. Some of us have environments where not everything is aware of the different nuances between bfd/gold/lld and so scripts supply common options to all three, which generally tends to work for most common options. As such, switching to --image-base isn't an option until the other linkers also support it.

Unfortunately currently this is supported by GNU ld's COFF port but not by its ELF port. gold does not support this option.

Just picked up this commit downstream today and likely will be reverting it locally. I think it was a bad decision to remove an option like this when the expected replacement isn't supported by the other linkers yet. Some of us have environments where not everything is aware of the different nuances between bfd/gold/lld and so scripts supply common options to all three, which generally tends to work for most common options. As such, switching to --image-base isn't an option until the other linkers also support it.

@troyj I think it was a good decision. As the description says, -Ttext and -Ttext-segment are different. gold incorrectly treats them the same. lld incorrectly did that too, with a different interpretation. D70468 made it clear that -Ttext-segment is not supported. If you are happy with the previous behavior, s/-Ttext-segment/-Ttext/

troyj added a comment.Feb 4 2020, 10:41 AM

I appreciate your point of view, but it's not very pragmatic given how "baked in" an old option like this is across existing environments. I have reverted downstream and we will continue to support the option ourselves until it can be phased out with less of an impact.

MaskRay added a comment.EditedFeb 4 2020, 11:18 AM

I appreciate your point of view, but it's not very pragmatic given how "baked in" an old option like this is across existing environments. I have reverted downstream and we will continue to support the option ourselves until it can be phased out with less of an impact.

I appreciate your feedback. In some cases there are still subtle differences among linkers. For this -Ttext-segment we faced a decision whether we should silently interpret an option incorrectly or leave it as an hard error. We chose the latter. I know the documentation says lld aims to be a drop-in placement of GNU linkers, but in my opinion in some cases we really need downstream users to do some necessary refactorings to adopt lld. This is like adopting clang. If a project enables -Werror and clang happens to report more diagnostics. That project may have to do some adaptations. Clang developers will assuredly consider the positive rate when deciding whether an option should be enabled by default. This may be more so for closed source software.

I remember that Peter Smith once said we probably need to consider a deprecation mechanism. For some more common options, if we eventually want to make some semantic changes, we should to consider that. For this -Ttext-segment, however, I don't think making it warning first then upgrading it to an error in a later release is worth the effort.

troyj added a comment.Feb 4 2020, 11:58 AM

For this -Ttext-segment we faced a decision whether we should silently interpret an option incorrectly or leave it as an hard error.

Understood, but since the incorrect interpretation was silently "working," it is necessary for us to remove this change until the option is no longer used anywhere.

I know the documentation says lld aims to be a drop-in placement of GNU linkers, but in my opinion in some cases we really need downstream users to do some necessary refactorings to adopt lld. This is like adopting clang.

It's not quite the same. We have adopted Clang, but Clang (and GNU) provide a -fuse-ld option, which means that the choice of linker is actually exposed to the user. We prefer to use lld as our default because we can more easily modify its source as necessary (such as in this case), but in practice the reason that our users sometimes need to select a specific linker is to workaround bugs in bfd/gold/lld. Normally the bugs present themselves as relocation errors that appear with only one of the three linkers. Therefore, the users need to be able to swap between the different linkers and any option that is not supported by all linkers requires additional effort -- either educating the user to be aware and change their options, or having scripts that automatically detect the active linker and hide the differences. Either takes time to implement and needs to be implemented before we leave the hard error in place.

I remember that Peter Smith once said we probably need to consider a deprecation mechanism

Yes, that would have been appreciated here.

MaskRay added a comment.EditedFeb 4 2020, 12:26 PM

For this -Ttext-segment we faced a decision whether we should silently interpret an option incorrectly or leave it as an hard error.

Understood, but since the incorrect interpretation was silently "working," it is necessary for us to remove this change until the option is no longer used anywhere.

It was "working" for your use case but "incorrect" in some others (qemu -Ttext-segment (Richard Henderson did more refactoring to qemu's configure and this kind of stuff is completed deleted now), an internal ld.so like program I debugged).

I know the documentation says lld aims to be a drop-in placement of GNU linkers, but in my opinion in some cases we really need downstream users to do some necessary refactorings to adopt lld. This is like adopting clang.

It's not quite the same. We have adopted Clang, but Clang (and GNU) provide a -fuse-ld option, which means that the choice of linker is actually exposed to the user. We prefer to use lld as our default because we can more easily modify its source as necessary (such as in this case), but in practice the reason that our users sometimes need to select a specific linker is to workaround bugs in bfd/gold/lld. Normally the bugs present themselves as relocation errors that appear with only one of the three linkers. Therefore, the users need to be able to swap between the different linkers and any option that is not supported by all linkers requires additional effort -- either educating the user to be aware and change their options, or having scripts that automatically detect the active linker and hide the differences. Either takes time to implement and needs to be implemented before we leave the hard error in place.

For your amusement, I filed a feature request/bug report regarding GNU ld's less rigid behavior about R_PPC_REL16_LO https://sourceware.org/bugzilla/show_bug.cgi?id=25500
There is a possibility that it will be closed as a wontfix. I am glad that lld has been rigid since its early days. If we receive feature requests about special casing R_PPC_REL16_LO and silencing such reasonable diagnostics, honestly I will consider them unreasonable.

It looks like you already modify lld source, so I agree this change is a bit inconvenient to you but it is not insurmountable. Many builds also enable -Wl,--fatal-warnings. Even linker warnings will not make them happy.

I remember that Peter Smith once said we probably need to consider a deprecation mechanism

Yes, that would have been appreciated here.