Page MenuHomePhabricator

[ELF] Ignore --no-relax for RISC-V
ClosedPublic

Authored by MaskRay on Jun 7 2020, 9:01 PM.

Details

Summary

In GNU ld, --no-relax can disable x86-64 GOTPCRELX relaxation.
It is not useful, so we don't implement it.

For RISC-V, --no-relax disables linker relaxations which have larger
impact.
Linux kernel specifies --no-relax when CONFIG_DYNAMIC_FTRACE is specified
(since http://git.kernel.org/linus/a1d2a6b4cee858a2f27eebce731fbf1dfd72cb4e ).
LLD has not implemented the relaxations, so this option is a no-op.

Diff Detail

Event Timeline

MaskRay created this revision.Jun 7 2020, 9:01 PM
grimar added a comment.Jun 8 2020, 3:25 AM

Can --no-relax just be an ignored option for now? Seems it is only valueable for RISC-V.

Can --no-relax just be an ignored option for now? Seems it is only valueable for RISC-V.

The implementation for x86-64 seems simple enough that we can implement it as well.

ppc64 has a similar --no-toc-optimize and once for a while I used it to bypass side effects caused by instruction rewriting.

grimar added a comment.Jun 8 2020, 8:17 AM

Can --no-relax just be an ignored option for now? Seems it is only valueable for RISC-V.

The implementation for x86-64 seems simple enough that we can implement it as well.

But:

  1. No implementing it, but ignoring is a single line in options.td I think.
  2. --no-relax name is probably itself confusing because of TLS relaxations that are not affected.
  3. It still introduce some code but there is no evidence it is usefull.
MaskRay updated this revision to Diff 269273.Jun 8 2020, 9:51 AM
MaskRay retitled this revision from [ELF] Add --[no-]relax to disable x86-64 GOTPCRELX relaxation to [ELF] Add --[no-]relax for RISC-V.
MaskRay edited the summary of this revision. (Show Details)

Remove x86-64 change

MaskRay updated this revision to Diff 269281.Jun 8 2020, 10:04 AM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed subscribers: kito-cheng, shiva0217, rogfer01 and 3 others.

Add an x86-64 test

Harbormaster completed remote builds in B59507: Diff 269281.
grimar added inline comments.Jun 9 2020, 12:53 AM
lld/ELF/Config.h
188 ↗(On Diff #269281)

You do not need this.

lld/ELF/Driver.cpp
977 ↗(On Diff #269281)

And this.

lld/ELF/Options.td
340

And this too I think.

629

You just need to list the option(s) here I believe.

MaskRay marked an inline comment as done.Jun 9 2020, 8:30 AM
MaskRay added inline comments.
lld/ELF/Config.h
188 ↗(On Diff #269281)

The variable definitions and --help will sooner or later be needed. I don't think it is too troublesome to keep them.

MaskRay marked an inline comment as done.Jun 9 2020, 8:31 AM
MaskRay added inline comments.
lld/ELF/Options.td
629

Adding here will just lead to a future change which deletes the line and adds the help above.

grimar added inline comments.Jun 10 2020, 1:46 AM
lld/ELF/Config.h
188 ↗(On Diff #269281)

In the past we usually never added a code for "sooner or later" when there is a chance that it never be needed. For example, I believe we do not implement all possible relaxations for i386 and nobody seems to care about them. I wouldn't be surprised to see the same situation for RISC-V, though I know almost nothing about it.

Or may be something else might change, as as far I understand this is only used in linux kernel for RISC-V. It sounds strange to me that this is the only target that needs it (it seems). Why do other targets don't need to stop relaxing? Is it a chance this will be solved differently? I.e. a with a compiler flag may be.

In case this will be needed one day (after implementing problematic RISC-V relaxations I guess), the help text might want to explicitly say that only RISC-V is affected.

I'll leave it to others to decide. Perhaps @ruiu might have something to say.

In the past we usually never added a code for "sooner or later" when there is a chance that it never be needed. For example, I believe we do not implement all possible relaxations for i386 and nobody seems to care about them. I wouldn't be surprised to see the same situation for RISC-V, though I know almost nothing about it.

In the first revision I added support for x86 as well. It is just a two line change. config->relax it totally justified. (I still don't think it is so useless that we should not implement it. The implementation is trivial.)

Or may be something else might change, as as far I understand this is only used in linux kernel for RISC-V. It sounds strange to me that this is the only target that needs it (it seems). Why do other targets don't need to stop relaxing? Is it a chance this will be solved differently? I.e. a with a compiler flag may be.

Linker relaxation is more powerful/troublesome on RISC-V: it can rewrite two instructions into one and shrink sections. Many more projects may request --relax or --no-relax if they rely on stable code sequences.

MaskRay added a comment.EditedJun 12 2020, 8:11 AM

I have a use case for x86 R_X86_64_[REX_]GOTPCRELX when debugging a glibc static-pie issue.

GNU ld --no-relax (loads an address from GOT):

// elf_get_dynamic_info called by dl-reloc-static-pie.c
 316│ 176           if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_FILES, 0)
 317│    0x00007fefd511896e <+462>:   mov    0x695b(%rip),%rcx        # 0x7fefd511f2d0
###### %rcx is 0 => null pointer dereference
 318│    0x00007fefd5118975 <+469>:   testb  $0x40,(%rcx)

--relax (PC-relative):

316│ 176           if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_FILES, 0)
317│    0x00007f27dd56cdbe <+462>:   lea    0x6f87b(%rip),%rcx        # 0x7f27dd5dc640 <_dl_debug_mask>
318│    0x00007f27dd56cdc5 <+469>:   testb  $0x40,(%rcx)

In a static pie program, it is a bug accessing GOT entry before the program relocates itself, as the GOT entry is likely 0 initially. lea is safe to use. If LLD has --relax --no-relax, it can be easier to verify that the behavior matches GNU ld. Honestly this is still a small use case and I managed to figure out the bug even if LLD does not support --no-relax.

grimar added a comment.EditedJun 15 2020, 1:22 AM

My main objection against this is that it introduces a config->relax which is not used and hence not needed.
I feel really uncomfortable to accept such way to do things as all last years of develepment we tried to avoid adding an unnecessary code to LLD and
I find it to be a very reasonable approach, I remember how I tried to do this, was stopped on reviews and as a result it actually helped to avoid a dead/unneded code few times.

GNU man says (https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_3.html):
An option with machine dependent effects. This option is only supported on a few targets. See section ld and the H8/300. See section ld and the Intel 960 family.
On some platforms, the `--relax' option performs global optimizations that become possible when the linker resolves addressing in the program, such as relaxing
address modes and synthesizing new instructions in the output object file. On platforms where this is not supported, `--relax' is accepted, but ignored.

I'd be OK just to add it to the "Options listed below are silently ignored for now for compatibility." list to ignore it (+ to docs/ld.lld.1),
as currently no platforms are supporting it in LLD.

grimar added a comment.EditedJun 15 2020, 1:43 AM

I have a use case for x86 R_X86_64_[REX_]GOTPCRELX when debugging a glibc static-pie issue.

GNU ld --no-relax (loads an address from GOT):

// elf_get_dynamic_info called by dl-reloc-static-pie.c
 316│ 176           if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_FILES, 0)
 317│    0x00007fefd511896e <+462>:   mov    0x695b(%rip),%rcx        # 0x7fefd511f2d0
###### %rcx is 0 => null pointer dereference
 318│    0x00007fefd5118975 <+469>:   testb  $0x40,(%rcx)

And this I think shows that supporting "--no-relax" for x86_64 can cause new bugs to appear. It is easy to avoid them by ignoring this option.

My main objection against this is that it introduces a config->relax which is not used and hence not needed.
I feel really uncomfortable to accept such way to do things as all last years of develepment we tried to avoid adding an unnecessary code to LLD and
I find it to be a very reasonable approach, I remember how I tried to do this, was stopped on reviews and as a result it actually helped to avoid a dead/unneded code few times.

GNU man says (https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_3.html):
An option with machine dependent effects. This option is only supported on a few targets. See section ld and the H8/300. See section ld and the Intel 960 family.
On some platforms, the `--relax' option performs global optimizations that become possible when the linker resolves addressing in the program, such as relaxing
address modes and synthesizing new instructions in the output object file. On platforms where this is not supported, `--relax' is accepted, but ignored.

I'd be OK just to add it to the "Options listed below are silently ignored for now for compatibility." list to ignore it (+ to docs/ld.lld.1),
as currently no platforms are supporting it in LLD.

My argument is whether we can go back to my first revision (https://reviews.llvm.org/D81359?id=269101) which did something potentially useful for x86-64. I have provided one use case: it can help compare --no-relax and --relax output. Then, there is no concern that config->relax is added for no use.

MaskRay updated this revision to Diff 275870.Jul 6 2020, 6:05 PM

In the absence of a third opinion, I give up and make --no-relax an ignored option.

grimar accepted this revision.Jul 7 2020, 12:52 AM

LGTM. Thanks..

This revision is now accepted and ready to land.Jul 7 2020, 12:52 AM
MaskRay retitled this revision from [ELF] Add --[no-]relax for RISC-V to [ELF] Ignore --no-relax for RISC-V.Jul 7 2020, 9:47 AM
MaskRay edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.