Page MenuHomePhabricator

[ELF] Support for setting the base address
ClosedPublic

Authored by phosek on Jul 7 2016, 3:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek updated this revision to Diff 63149.Jul 7 2016, 3:29 PM
phosek retitled this revision from to [ELF] Support for setting the base address.
phosek updated this object.
phosek added a project: lld.
phosek added a subscriber: llvm-commits.
ruiu added a subscriber: ruiu.Jul 7 2016, 3:44 PM

This patch sets the process start address, but is this what -Ttext-segment should do? GNU ld's manual page says that the option sets the start address of the text segment. So it should set the start address of .text segment, no?

phosek added a comment.Jul 7 2016, 7:30 PM

Yes it is, because text segment is not the same thing as .text section, text segment really means the base address, at least that's the semantics as implemented by both ld and gold.

grimar added a subscriber: grimar.Jul 8 2016, 12:00 AM
ruiu added a comment.Jul 8 2016, 10:46 AM

I think doing it is valid only for ld.gold because the linker layouts a executable segment at beginning of a file image. Our segment layout is not the same as GNU gold -- we actually layout a read-only data segment at beginning of a file. So I think just setting the base pointer is not enough for us to handle -Ttext-segment.

phosek added a comment.EditedJul 8 2016, 11:33 AM

There are two issues here. The first is correctness, and you're right that for lld -Trodata-segment as an option for setting the start address would be better since lld puts rodata segment at the beginning. The other is compatibility, as large number of existing build scripts use -Ttext-segment as an option for setting the start address, irrespective of which linker is being used.

In our use case, we would like to have an option to set the start address in lld. Would it be fine with you if I rename the option to -image-base? This option is already supported by BFD ld, even though it's only used for PE targets, and OS X ld also has a similar option -image_base. Alternatively we could use a completely different name.

In the future, if it's needed by other users for backwards compatibility, we can decide whether this option should be an alias for -Trodata-segment or -Ttext-segment, or other option.

Ed, do you know if the uses of -Ttext is FreeBSD are trying to set the
address of the text segment or just control the the lowest address in
the binary (i.e., set the address of the first segment)?

Cheers,
Rafael

rafael added inline comments.Jul 8 2016, 12:06 PM
ELF/Config.h
104 ↗(On Diff #63149)

Don't give it an initial value in here.

ELF/Driver.cpp
534 ↗(On Diff #63149)

Always set VAStart in here, since this is already looking at Target-> it may as well have an else to set it to the target default.

ELF/Writer.cpp
1129 ↗(On Diff #63149)

And with Config->VAStart always having the correct value you don't need this function.

phosek updated this revision to Diff 63352.Jul 8 2016, 4:15 PM
phosek updated this object.
phosek marked 3 inline comments as done.

I've renamed the option to -image-base.

ruiu added a comment.Jul 8 2016, 4:24 PM

Once we add a new option, we have to support it basically forever, so I'd like to be conservative. First of all, why do you want this option? What program are you trying to link with LLD, and why the program needs this option?

ELF/Config.h
104 ↗(On Diff #63352)

Move this after EntryAddr.

phosek updated this revision to Diff 63366.Jul 8 2016, 6:11 PM
phosek marked an inline comment as done.
phosek added a comment.Jul 8 2016, 6:21 PM

In general, this is an option supported by all existing linkers and it's used by the existing software; having to use a custom linker script just to change the start address is a bit of an overkill.

In our case, we would like to use lld as our system linker, but in our system the lowest available start address is different from the default start address used by lld. Having an option to set the start address allows us to pass this option to the linker from our Clang toolchain driver and avoid the use of linker script.

rafael accepted this revision.Jul 11 2016, 12:33 PM
rafael added a reviewer: rafael.
This revision is now accepted and ready to land.Jul 11 2016, 12:33 PM
ruiu added a comment.Jul 11 2016, 1:09 PM

LGTM. I think this option makes sense. It seems to me that --image-base feels a bit too Windows-ish name, but I can't think of a better name, so I'm okay with it.

Please make the following changes before commiting.

ELF/Config.h
104 ↗(On Diff #63366)

Did you do this?

ELF/Driver.cpp
534 ↗(On Diff #63366)

Please move this code to readConfigs.

ruiu accepted this revision.Jul 11 2016, 1:09 PM
ruiu added a reviewer: ruiu.
phosek updated this revision to Diff 63613.Jul 11 2016, 5:12 PM
phosek edited edge metadata.
phosek marked an inline comment as done.
phosek added inline comments.
ELF/Driver.cpp
534 ↗(On Diff #63366)

That's a problem; I need Target to exists to read PageSize and VAStart, but Target is only created in link which is executed after readConfigs. Shall I move Target initialization out from link before readConfigs?

ruiu added inline comments.Jul 11 2016, 5:13 PM
ELF/Driver.cpp
534 ↗(On Diff #63613)

Ah, okay, then keeping this code here makes sense. Sorry about that!

Petr, do you have commit access?

Cheers,
Rafael

I do, I can land the change if you don't have any other comments.

Fine with me.

Cheers ,
Rafael

ruiu added a comment.Jul 12 2016, 12:31 PM

I think I have already LGTM'ed.

This revision was automatically updated to reflect the committed changes.