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

[bsdjhb]

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.

[bsdjhb]

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

lld/ELF/Arch/RISCV.cpp

121

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]

lld/ELF/Arch/RISCV.cpp

121

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]

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]

lld/ELF/Arch/RISCV.cpp

109–112

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

[jrtc27]

lld/ELF/Arch/RISCV.cpp

109–112

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]

lld/ELF/Arch/RISCV.cpp

109–112

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]

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]

lld/ELF/Arch/RISCV.cpp

109–112

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]

lld/ELF/Arch/RISCV.cpp

109–112

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]

• Add a comment and expand the description in the commit log.
• Rename test from riscv-binary.s to riscv-elf-flags.s

lld/ELF/Arch/RISCV.cpp

 
 static uint32_t getEFlags(InputFile *f) {
   if (config->is64)
     return cast<ObjFile<ELF64LE>>(f)->getObj().getHeader()->e_flags;
   return cast<ObjFile<ELF32LE>>(f)->getObj().getHeader()->e_flags;
 }
 
 uint32_t RISCV::calcEFlags() const {
-  assert(!objectFiles.empty());
+  // If there are only binary input files (from -b binary), use a
+  // value of 0 for the ELF header flags.
+  if (objectFiles.empty())
+    return 0;
 
   uint32_t target = getEFlags(objectFiles.front());
 
   for (InputFile *f : objectFiles) {
     uint32_t eflags = getEFlags(f);
     if (eflags & EF_RISCV_RVC)
       target |= EF_RISCV_RVC;
 
     if ((eflags & EF_RISCV_FLOAT_ABI) != (target & EF_RISCV_FLOAT_ABI))
       error(toString(f) +
             ": cannot link object files with different floating-point ABI");
 
     if ((eflags & EF_RISCV_RVE) != (target & EF_RISCV_RVE))
       error(toString(f) +
             ": cannot link object files with different EF_RISCV_RVE");
   }
 

lld/test/ELF/riscv-elf-flags.s

+# REQUIRES: riscv
+
+# RUN: echo -n "BLOB" > %t.binary
+# RUN: ld.lld -m elf64lriscv -b binary %t.binary -o %t.out
+# RUN: llvm-readobj -h %t.out | FileCheck %s
+
+# CHECK:      Flags [
+# CHECK-NEXT: ]