[LLD][RISCV] Report error for unsatisfiable RISCV_ALIGN

[preames]

If we have a RISCV_ALIGN relocation which can't be satisfied with the available space provided, report an error rather than silently continuing with a corrupt state.

For context, #73977 fixes an LLD bug which can cause this effect, but that's not the only source of such cases.

Another is our hard-to-fix set of LTO problems. We can have a single function which was compiled without C in an otherwise entirely C module. Until we have all of the mapping symbols and related mechanisms implemented, this case can continue to arise.

I think it's very important from a user interface perspective to have non-assertion builds report an error in this case. If we don't report an error here, we can crash the linker (due to the fatal error at the bottom of the function), or if we're less lucky silently produce a malformed binary.

There's a couple of known defects with this patch.

First, there's no test case. I don't know how to write a stable test case for this that doesn't involve hex editing an object file, or abusing the LTO bug that we hope to fix.

Second, this will report an error on each relax iteration. I explored trying to report an error only once after relaxation, but ended up deciding I didn't have the context to implement it safely.

I would be thrilled if someone more knowledgeable of this code wants to write a better version of this patch, but in the meantime, I believe we should land this to address the user experience problem described above.

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Philip Reames (preames)

Changes

If we have a RISCV_ALIGN relocation which can't be satisfied with the available space provided, report an error rather than silently continuing with a corrupt state.

For context, #73977 fixes an LLD bug which can cause this effect, but that's not the only source of such cases.

Another is our hard-to-fix set of LTO problems. We can have a single function which was compiled without C in an otherwise entirely C module. Until we have all of the mapping symbols and related mechanisms implemented, this case can continue to arise.

I think it's very important from a user interface perspective to have non-assertion builds report an error in this case. If we don't report an error here, we can crash the linker (due to the fatal error at the bottom of the function), or if we're less lucky silently produce a malformed binary.

There's a couple of known defects with this patch.

First, there's no test case. I don't know how to write a stable test case for this that doesn't involve hex editing an object file, or abusing the LTO bug that we hope to fix.

Second, this will report an error on each relax iteration. I explored trying to report an error only once after relaxation, but ended up deciding I didn't have the context to implement it safely.

I would be thrilled if someone more knowledgeable of this code wants to write a better version of this patch, but in the meantime, I believe we should land this to address the user experience problem described above.

---

Full diff: https://github.com/llvm/llvm-project/pull/74121.diff

1 Files Affected:

• (modified) lld/ELF/Arch/RISCV.cpp (+8-2)

diff --git a/lld/ELF/Arch/RISCV.cpp b/lld/ELF/Arch/RISCV.cpp
index a556d89c36400d3..435600186143098 100644
--- a/lld/ELF/Arch/RISCV.cpp
+++ b/lld/ELF/Arch/RISCV.cpp
@@ -686,8 +686,14 @@ static bool relax(InputSection &sec) {
       const uint64_t align = PowerOf2Ceil(r.addend + 2);
       // All bytes beyond the alignment boundary should be removed.
       remove = nextLoc - ((loc + align - 1) & -align);
-      assert(static_cast<int32_t>(remove) >= 0 &&
-             "R_RISCV_ALIGN needs expanding the content");
+      // If we can't satisfy this alignment, we've found a bad input.
+      if (LLVM_UNLIKELY(static_cast<int32_t>(remove) < 0)) {
+        error(getErrorLocation((const uint8_t*)loc) +
+              "insufficient padding bytes for " + lld::toString(r.type) +
+              ": " + Twine(r.addend) + " bytes available "
+              + "for requested alignment of " + Twine(align) + " bytes");
+        remove = 0;
+      }
       break;
     }
     case R_RISCV_CALL:

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 9584f5834499e6093797d4a28fde209f927ea556 60dc91ace9ca4a867f44762defbfb2f538f24865 -- lld/ELF/Arch/RISCV.cpp

View the diff from clang-format here.

diff --git a/lld/ELF/Arch/RISCV.cpp b/lld/ELF/Arch/RISCV.cpp
index 485dd39a52..7c903f41f6 100644
--- a/lld/ELF/Arch/RISCV.cpp
+++ b/lld/ELF/Arch/RISCV.cpp
@@ -688,10 +688,12 @@ static bool relax(InputSection &sec) {
       remove = nextLoc - ((loc + align - 1) & -align);
       // If we can't satisfy this alignment, we've found a bad input.
       if (LLVM_UNLIKELY(static_cast<int32_t>(remove) < 0)) {
-        errorOrWarn(getErrorLocation((const uint8_t*)loc) +
+        errorOrWarn(getErrorLocation((const uint8_t *)loc) +
                     "insufficient padding bytes for " + lld::toString(r.type) +
-                    ": " + Twine(r.addend) + " bytes available "
-                    "for requested alignment of " + Twine(align) + " bytes");
+                    ": " + Twine(r.addend) +
+                    " bytes available "
+                    "for requested alignment of " +
+                    Twine(align) + " bytes");
         remove = 0;
       }
       break;

[MaskRay]

Combine consecutive literals: " bytes available for requested alignment of ".

Even if the code is broken ugly by clang-format, we can utilize "a"\n"b"

[MaskRay]

I agree that an errorOrWarn is useful. errorOrWarn is generally better than error since it is recoverable with --noinhibit-exec.

Second, this will report an error on each relax iteration. I explored trying to report an error only once after relaxation, but ended up deciding I didn't have the context to implement it safely.

Yes. An alternative is to store the message in a vector than report it in riscvFinalizeRelax, which may add quite some complexity. This error is for something that is badly broken, repeating it is probably fine.

[preames]

@MaskRay I pushed changes to address your comments, am I clear to land this?

[preames]

ping, @MaskRay