[ELF] Allow --noinhibit-exec to produce corrupted executable with relocation overflow

[MaskRay]

Summary

When --noinhibit-exec is used, ld.bfd/gold emit errors but allow to produce corrupted executable, which is handy for debugging purpose. lld's --noinhibit-exec has a different meaning and changes some errors to warnings. This patch replaces "error" with "errorOrWarn" to exploit that property.

We may revisit this: if we should keep them as errors but allow to produce a corrupted executable.

[grimar]

test/ELF/x86-64-reloc-range.s

4 ↗

(On Diff #174447)

I think you should change the \lld\test\ELF\no-inhibit-exec.s test case instead.

[jhenderson]

Functionality wise, this seems like a reasonable idea to me. Personally, I think I'd prefer --noinhibit-exec to more closely match the BFD approach of still emitting an actual error, but that's orthogonal to the issue this is trying to solve.

[Timmmm]

This seems like a good change to me. Although if I'm understanding this correctly it isn't exactly the same as our --ignore-relocations option because other errors (e.g. undefined symbols) would cause an error in that case and they won't with --noinhibit-exec. Is that correct? I suspect we may be able to make things work even if that is the case though (hopefully without resorting to parsing the linker output).

[grimar]

In D54651#1302611, @Timmmm wrote:

Although if I'm understanding this correctly it isn't exactly the same as our --ignore-relocations option because other errors (e.g. undefined symbols) would cause an error in that case and they won't with --noinhibit-exec. Is that correct?

Yes.

[MaskRay]

test/ELF/x86-64-reloc-range.s

4 ↗

(On Diff #174447)

When more than features interact it may be hazy where we should put the test but I do think the check should be put here as it checks that relocation overflow isn't fatal in the case of --noinhibit-exec. --noinhibit-exec is also used in other tests.

[grimar]

LGTM.

[MaskRay]

Functionality wise, this seems like a reasonable idea to me. Personally, I think I'd prefer --noinhibit-exec to more closely match the BFD approach of still emitting an actual error, but that's orthogonal to the issue this is trying to solve.

@jhenderson I'm also in favor of this: producing an executable but still an error. The current framework cannot express this as I've noted in the description. Making this possible can be a future improvement.

[ruiu]

In order to make lld's --noinhibit-exec behave like GNU linkers', we need to classify errors into "hard errors" (when that happens we need to stop at a nearest checkpoint) or "soft errors" (when that happens we can continue but the output is not likely to make sense.) We cannot simply continue executing the linker after all error()s until the linker creates an output file because some errors are not continuable, and if we force linker to continue, the linker may crash because of an internal inconsistency. I'm fairly satisfied with the current lld's error handling, so I personally don't think categorizing errors into two groups is worth doing though.

lld/trunk/ELF/Target.h

 static inline void reportRangeError(uint8_t *Loc, RelType Type, const Twine &V,
                                     int64_t Min, uint64_t Max) {
   ErrorPlace ErrPlace = getErrorPlace(Loc);
   StringRef Hint;
   if (ErrPlace.IS && ErrPlace.IS->Name.startswith(".debug"))
     Hint = "; consider recompiling with -fdebug-types-section to reduce size "
            "of debug sections";
 
-  error(ErrPlace.Loc + "relocation " + lld::toString(Type) +
-        " out of range: " + V.str() + " is not in [" + Twine(Min).str() + ", " +
-        Twine(Max).str() + "]" + Hint);
+  errorOrWarn(ErrPlace.Loc + "relocation " + lld::toString(Type) +
+              " out of range: " + V.str() + " is not in [" + Twine(Min).str() +
+              ", " + Twine(Max).str() + "]" + Hint);
 }
 
 // Make sure that V can be represented as an N bit signed integer.
 inline void checkInt(uint8_t *Loc, int64_t V, int N, RelType Type) {
   if (V != llvm::SignExtend64(V, N))
     reportRangeError(Loc, Type, Twine(V), llvm::minIntN(N), llvm::maxIntN(N));
 }
 

lld/trunk/test/ELF/x86-64-reloc-range.s

 // REQUIRES: x86
 // RUN: llvm-mc %s -o %t.o -triple x86_64-pc-linux -filetype=obj
 // RUN: not ld.lld %t.o -o /dev/null -shared 2>&1 | FileCheck %s
+// RUN: ld.lld --noinhibit-exec -shared %t.o -o %t 2>&1 | FileCheck %s
+// RUN: ls %t
 
 // CHECK: {{.*}}:(.text+0x3): relocation R_X86_64_PC32 out of range: 2147483648 is not in [-2147483648, 2147483647]
 // CHECK-NOT: relocation
 
         lea     foo(%rip), %rax
         lea     foo(%rip), %rax
 
         .hidden foo
         .bss
         .zero 0x7fffe007
 foo: