This is an archive of the discontinued LLVM Phabricator instance.

[lld][RISCV] Use an e_flags of 0 if there are only binary input files.
ClosedPublic

Authored by bsdjhb on Dec 5 2019, 4:59 PM.

Details

Summary

If none of the input files are ELF object files (for example, when
generating an object file from a single binary input file via
"-b binary"), use a fallback value for the ELF header flags instead
of crashing with an assertion failure.

Diff Detail

Event Timeline

bsdjhb created this revision.Dec 5 2019, 4:59 PM
bsdjhb marked an inline comment as done.Dec 5 2019, 5:01 PM

This is needed to link kernel modules for firmware blobs used in a FreeBSD/riscv64 kernel.

lld/ELF/Arch/RISCV.cpp
123

The BFD linker is a bit more forgiving in these flag checks to seemingly cater to mixing objects linked with '-b binary' with other objects. Specifically, it only checks for flag mismatches when an object file contains text sections (since the float ABI and RVE only matter for instructions). In FreeBSD's case the kernel is soft-float and doesn't use RVE, so a flags value of 0 for the '-b binary' case is already compatible.

jrtc27 added inline comments.Dec 5 2019, 5:05 PM
lld/ELF/Arch/RISCV.cpp
123

It's worth noting that, just because an object file only has data, doesn't mean it's ABI-independent. For example, a jmp_buf will change size based on the floating-point ABI. Having said that, that is a rare case, and the flags are never intended to stop every way of screwing things up, but as a safety net to spot obviously-wrong things, so I think it's ok to have that accepted.

MaskRay added a subscriber: grimar.Dec 5 2019, 6:10 PM
MaskRay added inline comments.
lld/test/ELF/riscv-binary.s
1 ↗(On Diff #232476)

The test should probably be added into format-binary.test.

Also change riscv to x86. I think @grimar has concerns about common tests with an uncommon target requirement.

MaskRay added inline comments.Dec 5 2019, 6:15 PM
lld/ELF/Arch/RISCV.cpp
111–114

For reasonable change that applies to other targets as well, we should try making them generic.

jrtc27 added inline comments.Dec 5 2019, 6:27 PM
lld/ELF/Arch/RISCV.cpp
111–114

There are only 6 architectures that override calcEFlags to be non-zero:

AMDGPU: Also has the same assert
ARM: Conditioned solely on config
Hexagon: Also has the same assert
Mips: Defaults to 0 if the list is empty
PPC64: Hard-coded to 2 (for the ABI version), but checks any input if present (allowed to be empty)
RISC-V: This

calcEFlags is already very target-specific and inconsistent, so I don't know how you intend to make this generic?

ruiu added inline comments.Dec 11 2019, 12:21 AM
lld/ELF/Arch/RISCV.cpp
111–114

I think defaulting to 0 is fine, as that is the best that we could do if no object files are given. A short comment explaining that objectFiles is empty if for example the only file given to the linker is -b <somefile> and we don't want to crash but instead create an output would help readers of the code, though.

grimar added inline comments.Dec 11 2019, 1:21 AM
lld/test/ELF/riscv-binary.s
1 ↗(On Diff #232476)

This test touches logic in RISCV::calcEFlags(), i.e. it is RISC-V specific, so I am fine with # REQUIRES: riscv

But I wonder if we want to have something more generic as a rest name rather than riscv-binary.s?
For example, I have not found tests for "cannot link object files with different floating-point ABI" or
"cannot link object files with different EF_RISCV_RVE" error messages. I think we might want to add a test
named "riscv-eflags" or alike and test this case there (assuming at one day somebody will
add missing tests for errors there).

asb added inline comments.Dec 12 2019, 5:29 AM
lld/ELF/Arch/RISCV.cpp
111–114

I agree with others on this review that defaulting to 0 seems safe here, but I feel the rationale and reasoning needs to be captured:

  • In the commit message if/when this lands
  • At least in brief as a comment here in the code
kevans added a subscriber: kevans.Dec 16 2019, 8:51 PM
kevans added inline comments.
lld/ELF/Arch/RISCV.cpp
111–114

Minor correction: mips new-ish behavior is to derive eflags from emulation string if the input list is empty, which was introduced initially to fix FreeBSD kmod builds on mips n32, since ABI is described here and lld refuses to link later due to ABI mismatch otherwise.

bsdjhb updated this revision to Diff 234421.Dec 17 2019, 5:17 PM
  • Add a comment and expand the description in the commit log.
  • Rename test from riscv-binary.s to riscv-elf-flags.s
MaskRay accepted this revision.Dec 17 2019, 5:55 PM
This revision is now accepted and ready to land.Dec 17 2019, 5:55 PM
ruiu accepted this revision.Dec 17 2019, 6:28 PM
jrtc27 edited the summary of this revision. (Show Details)Dec 21 2019, 9:58 AM
This revision was automatically updated to reflect the committed changes.