Page MenuHomePhabricator

[LLD][ELF] Full support for -n (--nmagic) and -N (--omagic)
AbandonedPublic

Authored by peter.smith on Apr 26 2019, 10:25 AM.

Details

Summary

The -n (--nmagic) and -N (--omagic) options are used in non-paged environments such as embedded systems. They have the following documented
behavior:
--nmagic disables page alignment of OutputSections and disables linking against dynamic libraries.
--omagic is the same as --nmagic but additionally makes the text and data sections to be readable and writeable.
In addition to the documented behavior:

  • We don't generate a PT_RELO as there are no pages.
  • Unless explicitly instructed to by PHDRS we do not allocate the headers in the first program segment.
  • The linkerscript expressions that refer to the max and common page size are unaffected.

fixes pr40542 https://bugs.llvm.org/show_bug.cgi?id=40542

Our previous implementation of -N in D26888 just made the text and data sections to be readable and writeable as this was sufficient for the use-case needed at the time. This change builds upon that to add the remaining behavior from GNU ld.bfd.

Implementation Notes:
I originally thought that this would be as simple as -n = --max-page-size=1, unfortunately we have both MaxPageSize which is configurable and Target->PageSize (often known as common-page-size) that isn't. It could be possible to make Target->PageSize configurable via implementing -z common-page-size, however while looking into GNU ld, it seems like the linkerscript expressions like COMMONPAGESIZE and MAXPAGESIZE are not affected by --nmagic or --omagic.

It is worth mentioning that it is not clear that LLD -n will give sensible results with the builtin layout which assumes paging. In practice using -n will require a linker script. I've tested against the aarch64 linux vdso which uses -n with its own linker script and it does give the required result.

Considered alternatives:

  • Implement -z common-page-size (handle Target->PageSize in the way that we handle Config->MaxPageSize) and set both common and max page sizes to 1. This leaves the page calculations untouched but would also affect COMMONPAGESIZE and MAXPAGESIZE and other linker script expressions. This could break some scripts.
  • Make specific versions of alignTo and alignDown for page alignment rather than calling them directly and then test for Config->Paged. I rejected this as we would need at least 4 functions to handle the combination of (To, Down) and (PageSize, MaxPageSize).

Diff Detail

Event Timeline

peter.smith created this revision.Apr 26 2019, 10:25 AM

I had just a few small suggestions to make things more consistent. Feel free to ignore my fallthrough comment, but I feel like -Wimplicit-fallthrough has been quite valuable in Android.

ELF/Driver.cpp
1119

This should probably make use of LLVM_FALLTHROUGH.

ELF/Options.td
255

Instead of "and link" this should be just ", link" based on the existing usage.

srhines marked an inline comment as done.Apr 26 2019, 10:50 AM
srhines added inline comments.
ELF/Driver.cpp
1119

Actually ignore me. I for some reason thought that there was something else happening in the first case. -Wimplicit-fallthrough doesn't warn for this case, since it is obvious that you are handling both cases the same way.

MaskRay added inline comments.Apr 26 2019, 7:04 PM
ELF/Driver.cpp
826

Since --omagic implies --nmagic, we can probably use:

Config->Nmagic = Config->OMagic || Args.hasFlag(OPT_nmagic, OPT_no_nmagic, false);

and delete Config::Paged.

or

Config->Paged = !(Config->OMagic || Args.hasFlag(OPT_nmagic, OPT_no_nmagic, false));

958–961

Then, this will become if (Config->Nmagic) or if (!Config->Paged)

ELF/Writer.cpp
2082–2083

Do other occurrences of Config->MaxPageSize need similar changes?

i.e, what will happen if we do if (Config->Nmagic) Config->MaxPageSize = 1;

grimar added inline comments.Apr 29 2019, 2:23 AM
test/ELF/aarch64-script-nmagic.s
1 ↗(On Diff #196876)

Can it be not AArch64 specific test, but generic, i.e. x86?

21 ↗(On Diff #196876)

This like does not have any FileCheck.

21 ↗(On Diff #196876)

like->line

31 ↗(On Diff #196876)

Should this test live in the linkerscript folder?

Looks like this place could shorter, like:

.text
  ret

If then perhaps this test should be .test, not .s,
like we often do for linker script tests.

peter.smith marked 9 inline comments as done.Apr 29 2019, 3:51 AM

Thanks all for the comments. I'll upload a new diff with the changes shortly.

ELF/Driver.cpp
826

I've gone for the second option (Config->Paged) as I think that disabling Page Alignment when Config->Paged == false makes much more sense than referring directly to Omagic and Nmagic as these option names aren't self describing.

ELF/Writer.cpp
2082–2083

Do other occurrences of Config->MaxPageSize need similar changes?

I don't think any other occurrences need changing in the current implementation, there are some raw uses of Config->MaxPageSize and Target->PageSize but these are not called unless Config->Paged. For example writeTrapInstr() will early exit unless Config->Paged. I thought of wrapping all calls to Config->MaxPageSize and Target->PageSize with a function that substituted 1 if !Config->Paged, I thought that might hide the intention behind the code too much. I don't have a strong opinion so I'd be happy to do this another way.

i.e, what will happen if we do if (Config->Nmagic) Config->MaxPageSize = 1;

I'm not sure I fully understand here, I think what you mean is that if we make Config->NMagic set Config->MaxPageSize = 1 then will everything work? My first implementation did that but I ran across a couple of problems:

  • The Target->PageSize is unaffected which causes problems as there seems to be a working assumption that this is smaller than MaxPageSize
  • Use of -n and -N in ld.bfd does not affect the linkerscript commands that refer to Config->MaxPageSize and Target->PageSize such as COMMONPAGESIZE, MAXPAGESIZE, DATA_SEGMENT_RELRO_END. I have my suspicion that these were not supposed to be used in a linker script that uses -N and -n but I prefer consistency where possible.

I think there are some alternative implementations that could work:

  • Handle Target->PageSize like Config->MaxPageSize and set them both to 1 (would affect linker scripts as above, but most likely minor).
  • Have Config->Paged skip any use of Target->PageSize

Hope I've interpreted that question the right way?

test/ELF/aarch64-script-nmagic.s
1 ↗(On Diff #196876)

Sure, I've changed it to x86 (just need to change the alignment to 4096).

21 ↗(On Diff #196876)

Thanks for the spot, now fixed.

peter.smith marked 2 inline comments as done.

Updated diff to address review comments. Main changes:

  • Removed Config->Nmagic in favour of just using Config->Paged
  • Moved test to linkerscript, converted to .test and switched to x86

Generally looks good I think. Few more comments/suggestions from me.

ELF/Config.h
167

Should it be here? I think it is the place for the flags that
are named similar to a command line option. Since there is no --paged option,
should it be below, under the following comment?

(line 232)
 // The following config options do not directly correspond to any
 // particualr command line options.
ELF/Driver.cpp
907

Not sure why Page Alignment of Sections are in upper case here..

1122

Should it be combined with case OPT_Bstatic?
That would be easier to find all the options that affect a particular flag, what is better I think.

ELF/Options.td
222

You should update lld\docs\ld.lld.1 I think after adding an option.
The same for below ones.

grimar added inline comments.Apr 30 2019, 3:32 AM
test/ELF/linkerscript/nmagic-alignment.test
10

Ah, and here instead of adding an input, I think you can just do:

# RUN: echo ".text; .globl foo; foo: nop" > %t.s
# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t.s -o %t.o

(We often use echo in *.test for minor asm pieces)

peter.smith marked 4 inline comments as done.Apr 30 2019, 8:21 AM

Thanks for the suggestions I'll apply and upload a new diff.

Updated diff for review comments. Main changes:

  • Move location of Config::Paged
  • Update captitalisation in comments
  • Add documentation
  • Use echo in linkerscript test
grimar accepted this revision.May 1 2019, 2:35 AM

LGTM with a nit.

docs/ld.lld.1
258

You also need to document at least --no-nmagic, since it is added by this patch.

(ideally we want to document --no-omagic too)

This revision is now accepted and ready to land.May 1 2019, 2:35 AM
MaskRay added inline comments.May 1 2019, 3:10 AM
ELF/Writer.cpp
2082–2083

I think what you mean is that if we make Config->NMagic set Config->MaxPageSize = 1 then will everything work?

That was my question. Thank you for the explanation.

I also asked this in the binutils-gdb bugzilla https://sourceware.org/bugzilla/show_bug.cgi?id=24505
I will forward their responses.

Thanks for the review, I'll update the docs for --no-omagic and --no-nmagic and will wait for the response to MaskRay's question before committing just in case it changes anything significant.

https://sourceware.org/bugzilla/show_bug.cgi?id=24505 doesn't get a response, but I researched the users of CONSTANT(...):

CONSTANT(COMMONPAGESIZE): 2 users: edk2, u-boot
CONSTANT(MAXPAGESIZE): 1 user: fuchsia zircon, but they don't use -n or -N

u-boot and fuchsia zircon don't seem to use -n or -N. edk2 is probably the only project that uses both -n/-N and CONSTANT(...). It has DEFINE GCC44_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20

We probably shouldn't be constrained by ld.bfd's behavior for this single user,
if making MaxPageSize and PageSize to 1 makes our implementation simpler.

The potential users can make straightforward changes to adapt to lld's behavior: change CONSTANT(COMMONPAGESIZE) to a literal constant.

https://sourceware.org/bugzilla/show_bug.cgi?id=24505 doesn't get a response, but I researched the users of CONSTANT(...):

CONSTANT(COMMONPAGESIZE): 2 users: edk2, u-boot
CONSTANT(MAXPAGESIZE): 1 user: fuchsia zircon, but they don't use -n or -N

u-boot and fuchsia zircon don't seem to use -n or -N. edk2 is probably the only project that uses both -n/-N and CONSTANT(...). It has DEFINE GCC44_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20

We probably shouldn't be constrained by ld.bfd's behavior for this single user,
if making MaxPageSize and PageSize to 1 makes our implementation simpler.

The potential users can make straightforward changes to adapt to lld's behavior: change CONSTANT(COMMONPAGESIZE) to a literal constant.

I'll make an alternative implementation that in effect implements -z common-page, and have -n/-N set it to 1 and will post as a separate review. It will probably be of similar complexity to this. In the example above it does look to be using -z common-page-size and CONSTANT(COMMONPAGESIZE) as a way of passing a parameter to the linker script.

https://sourceware.org/bugzilla/show_bug.cgi?id=24505 doesn't get a response, but I researched the users of CONSTANT(...):

CONSTANT(COMMONPAGESIZE): 2 users: edk2, u-boot
CONSTANT(MAXPAGESIZE): 1 user: fuchsia zircon, but they don't use -n or -N

u-boot and fuchsia zircon don't seem to use -n or -N. edk2 is probably the only project that uses both -n/-N and CONSTANT(...). It has DEFINE GCC44_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20

We probably shouldn't be constrained by ld.bfd's behavior for this single user,
if making MaxPageSize and PageSize to 1 makes our implementation simpler.

The potential users can make straightforward changes to adapt to lld's behavior: change CONSTANT(COMMONPAGESIZE) to a literal constant.

I'll make an alternative implementation that in effect implements -z common-page, and have -n/-N set it to 1 and will post as a separate review. It will probably be of similar complexity to this. In the example above it does look to be using -z common-page-size and CONSTANT(COMMONPAGESIZE) as a way of passing a parameter to the linker script.

I've implemented the alternative with the max and common page size set to 1 in D61688 . I'll leave this open as I think the complexity of the implementation is similar.

peter.smith abandoned this revision.May 9 2019, 3:54 AM

Abandoned in favour of a different approach D61688