This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Set Dot initially to --image-base value when using linker scripts
ClosedPublic

Authored by jhenderson on Sep 28 2017, 7:42 AM.

Details

Summary

This is an alternative to D38238. When parsing linker scripts, LLD starts with a '.' value of 0, regardless of the internal default image base for the target, and regardless of switches such as --image-base. It seems reasonable to use a different image base value when using linker scripts and --image-base is specified, since otherwise the switch has no effect. However, the default image base should not be used when processing linker scripts, because this will change the behaviour for existing linker script users, and potentially result in invalid output being produced, as a subsequent assignment to dot could move the location counter backwards (see the comments in D38238 for examples).

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Sep 28 2017, 7:42 AM
grimar added inline comments.Sep 28 2017, 8:02 AM
ELF/Config.h
177 ↗(On Diff #116989)

I think proper place for this would be ScriptConfiguration,
though please wait for Rui's opinion.

ruiu added inline comments.Oct 2 2017, 9:51 AM
ELF/Driver.cpp
911–912 ↗(On Diff #116989)

This and other functions in this file are intended to be "pure" functions, that don't have any side-effects. They are "getters" and are not expected to do anything other than returning values. So please do not set a value to a Config member from a "get" function.

jhenderson updated this revision to Diff 117682.Oct 4 2017, 9:06 AM

Address review comment.

@grimar - I don't think the new variable should be in ScriptConfiguration. From what I can tell, ScriptConfiguration is intended for settings controlled by the linker script, which this is not. This is controlled by the command-line.

jhenderson marked an inline comment as done.Oct 4 2017, 9:06 AM
ruiu added inline comments.Oct 4 2017, 9:50 AM
ELF/Config.h
176–177 ↗(On Diff #117682)

So, now you have two variables for the image base. Their valid combinations are:

  • ImageBase has value N, and InitialDot also has N. This happens only when you explicitly passed an -image-base.
  • ImageBase has value N, and InitialDot has 0. This happens only when you didn't pass an -image-base.

Other combinations are all invalid. That means you are effectively using InitiaDot as a boolean variable. It doesn't have to be an integer, and to convey the intention how it is used, it should be a boolean.

I'd make these changes:

  • Change the type of ImageBase from uint64_t to Optional<uint64_t>, and set a value only when an -image-base is given.
  • Define a function, getImageBase, which returns either Config->ImageBase or Target->DefaultImageBase.
jhenderson updated this revision to Diff 117810.Oct 5 2017, 6:36 AM

Addressed review comments.

I'm not sure where to put the new getImageBase function, because it is needed in SyntheticSections.cpp as well as LinkerScript.cpp. To me it feels like it really belongs as a member of the Configuration class, but that is currently made up entirely of variables, and no methods, so I've put it in the LinkerScript class. I'm open to other suggestions.

grimar added inline comments.Oct 5 2017, 6:51 AM
ELF/Driver.cpp
924 ↗(On Diff #117810)

It is common to

return None;
jhenderson updated this revision to Diff 117814.Oct 5 2017, 6:55 AM

Thanks @grimar. I've changed to use None.

jhenderson marked 2 inline comments as done.Oct 5 2017, 6:55 AM
ruiu added inline comments.Oct 5 2017, 8:09 PM
ELF/LinkerScript.cpp
349 ↗(On Diff #117814)

I wonder if you really have to call this function twice. Can you initialize Dot only once in LinkerScript's ctor?

355 ↗(On Diff #117814)

This function does not use any LinkerScript's private/protected members. It is a good practice to not make such function a member function of some class. They can just be a non-member function. By doing that, it is obvious that the function doesn't access any private members.

That said, you can move this function to Target, and by doing that, you can make Target::DefaultImageBase a protected member. Can you do that?

jhenderson updated this revision to Diff 118193.Oct 9 2017, 3:43 AM
jhenderson marked an inline comment as done.

Address review comments.

jhenderson added inline comments.Oct 9 2017, 3:44 AM
ELF/LinkerScript.cpp
349 ↗(On Diff #117814)

No, because the value is not final until after command-line processing, whereas the LinkerScript instance is constructed during program initialisation. We also need to reset Dot every time we call assignAddresses, because it might be called multiple times. However, having looked closer it does not need setting in processCommands (it's never updated, and should never be referenced, although it currently could be in SUBALIGN expressions - this is a bug, these expressions should only accept constants, and the current behaviour results in an assertion failure if it is used). This allows me to move the function inline into assignAddresses.

ruiu accepted this revision.Oct 9 2017, 5:08 PM

LGTM

ELF/LinkerScript.cpp
349 ↗(On Diff #117814)

Thank you for removing the assignment to Dot from this function!

ELF/Target.cpp
173 ↗(On Diff #118193)

this is a TargetInfo, so remove Target->.

This revision is now accepted and ready to land.Oct 9 2017, 5:08 PM
This revision was automatically updated to reflect the committed changes.