This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Support -z max-page-size option
ClosedPublic

Authored by phosek on Sep 23 2016, 5:49 PM.

Details

Summary

This option is already supported by both BFD ld and gold and allows overriding the max page size whose default value is defined by the target.

Diff Detail

Event Timeline

phosek updated this revision to Diff 72383.Sep 23 2016, 5:49 PM
phosek retitled this revision from to [ELF] Support -z max-page-size option.
phosek updated this object.
phosek added a reviewer: ruiu.
phosek added a project: lld.
phosek added subscribers: llvm-commits, phosek.
davide added a subscriber: davide.Sep 24 2016, 11:03 AM

Out of curiosity, are you using this option anywhere?
I think this is semantically fine if you have an use case, I left some comments around.

ELF/Driver.cpp
663

You might want to elaborate this a bit. Maybe, mentioning that there's a target default MAXPAGESIZE anyway and this option overrides the value.

test/ELF/zmax-page-size.s
1 ↗(On Diff #72383)

Linker script tests go in ELF/linkerscript

We do, see https://fuchsia.googlesource.com/magenta/+/master/kernel/arch/x86/rules.mk#150 for example. This option is supported by both BFD ld and gold and it would be nice if LLD supported the option, since it's used by existing software.

phosek updated this revision to Diff 72404.Sep 24 2016, 5:56 PM
phosek updated this object.
phosek marked 2 inline comments as done.
davide accepted this revision.Sep 24 2016, 7:02 PM
davide added a reviewer: davide.

LGTM

This revision is now accepted and ready to land.Sep 24 2016, 7:02 PM

Thanks, there is also a similar patch for -z common-page-size if you want to take a look: https://reviews.llvm.org/D24891.

Thanks, there is also a similar patch for -z common-page-size if you want to take a look: https://reviews.llvm.org/D24891.

Sure, commented there. In the future, feel free to cc: me or list me as reviewer so that the patch can get my attention more easily.

phosek updated this revision to Diff 72414.Sep 24 2016, 8:55 PM
phosek edited edge metadata.

The updated version combines the logic for handling both -z max-page-size and -z common-page-size which also makes https://reviews.llvm.org/D24898 unnecessary.

phosek updated this revision to Diff 72416.Sep 24 2016, 8:57 PM

What happens if you pass a negative value to -z common-page-size ?

ELF/Driver.cpp
663–670

There are other -z options supported, but not all of them take an integer option, e.g. -z now. Can this function be generalized to cover all them? That seems a little bit too much to ask, so you can probably just rename this one.

665–666

can you add a test for this case?

davide requested changes to this revision.Sep 24 2016, 9:05 PM
davide edited edge metadata.
This revision now requires changes to proceed.Sep 24 2016, 9:05 PM
phosek updated this revision to Diff 72421.Sep 25 2016, 2:03 AM
phosek edited edge metadata.
phosek marked 2 inline comments as done.

You will get the invalid value error (I've added a test for this). This is handled by the Value->getAsInteger method since the value is defined as uint64_t.

grimar added a subscriber: grimar.Sep 26 2016, 1:55 AM
grimar added inline comments.
test/ELF/linkerscript/zcommon-page-size.s
7 ↗(On Diff #72421)

But should this work ?
We had a patch for this option earlier: D19600 and its implementation checked that:

if (Config->ZMaxPageSize < Target->PageSize ||
  (Config->ZMaxPageSize % Target->PageSize != 0))
    error("wrong page size specified: " + Twine(Config->ZMaxPageSize));

This is checks that GNU linkers do (I think both of them, but not really remember now) and this
checks looks reasonable to have for me.

ruiu added inline comments.Sep 26 2016, 10:27 AM
ELF/Driver.cpp
266–268

I think you could change this function to take one more parameter and return an integer.

uint64_t getZOptionValue(opt::InputArgList &Args, StringRef Key, uint64_t Default)
499–500

then you could do

Config->ZStackSize = getZOptionValue(Args, "stack-size", -1);
674–675

and

Config->CommonPageSize = getZOption(Args, "common-page-size", Target->PageSize);
Config->MaxPageSize = getZOption(Args, "max-page-size", Target->PageSize);
phosek updated this revision to Diff 72573.Sep 26 2016, 3:09 PM
phosek edited edge metadata.
phosek marked 3 inline comments as done.

I was checking the behavior of both BFD ld and gold. BFD ld checks whether the argument is power of 2 while gold doesn't, but I don't see any reason why page size wouldn't be a power 2 so I've added that check as well. The handling of common-page-size is strange, there doesn't seem to be a clear consensus on how exactly that value should be interpreted.

ruiu added inline comments.Sep 26 2016, 3:39 PM
ELF/Config.h
140–141

Please remove = -1 because the variable is always assigned now.

ELF/Driver.cpp
276

Can you return Result here and return Default at end?

667

We have isPowerOf2_64 in MathExtras.h.

673

If there's no clear consensus how common-page-size should be interpreted, I'd vote for the simplest rule: just -z common-page-size's value to CommonPageSize.

phosek updated this revision to Diff 72582.Sep 26 2016, 4:13 PM
phosek marked 3 inline comments as done.
phosek marked an inline comment as done.
ruiu accepted this revision.Sep 26 2016, 4:30 PM
ruiu edited edge metadata.

LGTM

davide accepted this revision.Sep 26 2016, 4:32 PM
davide edited edge metadata.

I have one single last comment, then LGTM.
I think you can merge the two *.s files into one as they share the same ASM.

Thanks!

This revision is now accepted and ready to land.Sep 26 2016, 4:32 PM
phosek updated this revision to Diff 72592.Sep 26 2016, 5:55 PM
phosek edited edge metadata.

Done.

This is getting more complicated, I was looking at using -z max-page-size and -z common-page-size values in the Writer and found that lld's use of page size is somewhat inconsistent so I filed a bug 30541 and proposed possible solutions. It'd be great to decide on the solution for the page size handling before landing this patch as it might require further changes (e.g. if we decide to only use the maximum page size, we probably shouldn't accept the -z common-page-size flag).

This is getting more complicated, I was looking at using -z max-page-size and -z common-page-size values in the Writer and found that lld's use of page size is somewhat inconsistent so I filed a bug 30541 and proposed possible solutions. It'd be great to decide on the solution for the page size handling before landing this patch as it might require further changes (e.g. if we decide to only use the maximum page size, we probably shouldn't accept the -z common-page-size flag).

Slightly related. There were problems in the past because lld was aliasing MAXPAGESIZE and PAGE_SIZE on x86-64 and this caused the FreeBSD kernel to crash when linked with lld so I want to be sure that configuration is tested when experimenting with page sizes. I assume that if we mimic what bfd does we should be OK in that case, as the kernel is currently linked with a very old (2.17) version of it.

phosek updated this revision to Diff 72752.Sep 27 2016, 6:30 PM

Now that https://reviews.llvm.org/D24987 has landed, I have revised the patch to make sure that the MaxPageSize value is properly propagated to the writer and also added a test that checks that the wiring is working. I've also removed the support for -z common-page-size, I don't think we should be supporting that option until (if ever) we decide to implement the page saving logic because that's the only place where that option comes into play. Could you please take a look again?

rafael added inline comments.Sep 27 2016, 6:56 PM
ELF/Driver.cpp
671

Why do you need to set bits in Target? It would be more natural to change users to use the Config value and default that to the Target one.

phosek updated this revision to Diff 72754.Sep 27 2016, 7:00 PM
phosek marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.