Page MenuHomePhabricator

[lld][RISCV] Print error when encountering R_RISCV_ALIGN
ClosedPublic

Authored by jrtc27 on Dec 22 2019, 3:56 PM.

Details

Summary

Unlike R_RISCV_RELAX, which is a linker hint, R_RISCV_ALIGN requires the
support of the linker even when ignoring all R_RISCV_RELAX relocations.
This is because the compiler emits as many NOPs as may be required for
the requested alignment, more than may be required pre-relaxation, to
allow for the target becoming more unaligned after relaxing earlier
sequences. This means that the target is often not initially aligned in
the object files, and so the R_RISCV_ALIGN relocations cannot just be
ignored. Since we do not support linker relaxation, we must turn these
into errors.

Diff Detail

Event Timeline

jrtc27 created this revision.Dec 22 2019, 3:56 PM
MaskRay added inline comments.Dec 22 2019, 5:13 PM
lld/ELF/Arch/RISCV.cpp
240–247

I think R_HINT is not useful, so I created D71822.

245

If you think there are needs (which I think so) to test lld on existing R_RISCV_ALIGN relocations,
use errorOrWarn so that errors can be downgraded to warnings with --noinhibit-exec.

443

Full stop, though the examples above and below (they are inconsistent with other places) do not use it...

lld/test/ELF/riscv-reloc-align.s
4

-unknown-elf is implied, I think.

5

-o /dev/null

jrtc27 marked 4 inline comments as done.Dec 22 2019, 5:22 PM
jrtc27 added inline comments.
lld/ELF/Arch/RISCV.cpp
240–247

It might be if LLD learns RISC-V's linker relaxations, although that could instead be a new R_RELAX (or RISC-V specific type). I can rebase if your patch is going to land first.

245

I guess it does no harm, given --noinhibit-exec is a debugging option rather than intended for users (for whom this should definitely be an error).

443

Even for phrases that aren't complete sentences?

lld/test/ELF/riscv-reloc-align.s
4

Yeah, not that it even matters which is used; the tests are inconsistent and I copied from one that happened to spell it in full. Will make shorter.

@MaskRay Do you think we could get this landed (with or without D71822) before 10 branches tomorrow?

jrtc27 updated this revision to Diff 237914.Jan 14 2020, 3:28 AM

Rebased and addressed review comments.

@MaskRay Do you think we could get this landed (with or without D71822) before 10 branches tomorrow?

I see you've already committed the RISC-V parts of that, so the dependency no longer exists.

MaskRay accepted this revision.Jan 14 2020, 10:59 AM
MaskRay added a subscriber: grimar.

I was waiting on D71822 . @grimar thought it is good, and I guess you would say so. So I went ahead and commit it. LG, since users can use --noinhibit-exec to downgrade the error to a warning.

This revision is now accepted and ready to land.Jan 14 2020, 10:59 AM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Jan 23 2020, 5:52 AM

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

It feels like getting this error reporting in place ASAP would be valuable for end users, and could reduce erroneous bug reports / wasted debugging time.

In D71820#1836028, @asb wrote:

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

It feels like getting this error reporting in place ASAP would be valuable for end users, and could reduce erroneous bug reports / wasted debugging time.

FWIW, I think it is fine to backport this to 10.0

MaskRay added a comment.EditedJan 23 2020, 11:46 PM
In D71820#1836028, @asb wrote:

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

It feels like getting this error reporting in place ASAP would be valuable for end users, and could reduce erroneous bug reports / wasted debugging time.

FWIW, I think it is fine to backport this to 10.0

+1

@hans

MaskRay added a subscriber: hans.Jan 23 2020, 11:47 PM