[ELF] - Do not call fatal() in Target.cpp, call error() instead.

[grimar]

Summary

That is useful from many sides I think.
We probably would want to avoid fatal() if we can in context of librarification,
but for me reason of that patch is to help D27900 go.

D27900 changes errors reporting to something like
error: text1
note: text2
note: text3

where hint used to provide additional information about location. In that case
I can't just call fatal() because user will not see notes after that what adds additional complication to handle.
What is good that Target.cpp looks the only such place and this patch changes it.

Also it adds testcase with broken relocation. Previously we did not have any,
It checks that error() instead of fatal() works fine.

[davide]

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

[grimar]

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

[grimar]

I mean I tried to create broken relocation, I do not know if just numeric alias works, but huge value like I use in this patch - 0x99 - definetely does not.

[davide]

In D27973#627791, @grimar wrote:

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

Can you please take a look at how hard is to fix yaml2obj for this testcase? I think we should avoid checking in a binary unless really necessary, as it makes reconstructing the testcase harder in the future.

[grimar]

In D27973#627793, @davide wrote:

In D27973#627791, @grimar wrote:

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

Can you please take a look at how hard is to fix yaml2obj for this testcase? I think we should avoid checking in a binary unless really necessary, as it makes reconstructing the testcase harder in the future.

Sure. I'll take a look on that tomorrow than. I hope that should not affect on that patch though. But that would be a good change I think anyways. I assume yaml2obj is best using when need to create something unusual, so I would expect to have as less limitations as possible here.

[davide]

In D27973#627791, @grimar wrote:

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

Can you please take a look at how hard is to fix yaml2obj for this testcase? I think we should avoid checking in a binary unless really necessary, as it makes reconstructing the testcase harder in the future.

In D27973#627794, @grimar wrote:

In D27973#627793, @davide wrote:

In D27973#627791, @grimar wrote:

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

Can you please take a look at how hard is to fix yaml2obj for this testcase? I think we should avoid checking in a binary unless really necessary, as it makes reconstructing the testcase harder in the future.

Sure. I'll take a look on that tomorrow than. I hope that should not affect on that patch though. But that would be a good change I think anyways. I assume yaml2obj is best using when need to create something unusual, so I would expect to have as less limitations as possible here.

*sigh* It sounds fixing this might be non-trivial as the YAML parser expects an enumerated scalar (which probably will get from ELF.h or something). So, I'd leave this as is but please add specific instructions on how to reproduce (source test file, commands, and tools version, so that it's not a pain to recreate this in the future).

[grimar]

And also we faced with some limitations when did a fuzzing of lld. Probably it is reasonable to tweak yaml2obj and try to get rid of pre-compiled objects if possible. That looks to be usefull.

[grimar]

In D27973#627795, @davide wrote:

In D27973#627791, @grimar wrote:

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

Can you please take a look at how hard is to fix yaml2obj for this testcase? I think we should avoid checking in a binary unless really necessary, as it makes reconstructing the testcase harder in the future.

In D27973#627794, @grimar wrote:

In D27973#627793, @davide wrote:

In D27973#627791, @grimar wrote:

In D27973#627788, @davide wrote:

Instead of checking in a binary, I'll see if you can craft a broken object using yaml2obj.
I'm not a huge fan of the tool itself, but I find this a very legitimate usecase for it (creating broken binaries).

No it looks not possible. It showed me a error when I tried to create a relocation using numeric value and not a pre-defined enum. So I believe it is a unreasonable limitaion of yaml2obj now.

Can you please take a look at how hard is to fix yaml2obj for this testcase? I think we should avoid checking in a binary unless really necessary, as it makes reconstructing the testcase harder in the future.

Sure. I'll take a look on that tomorrow than. I hope that should not affect on that patch though. But that would be a good change I think anyways. I assume yaml2obj is best using when need to create something unusual, so I would expect to have as less limitations as possible here.

*sigh* It sounds fixing this might be non-trivial as the YAML parser expects an enumerated scalar (which probably will get from ELF.h or something). So, I'd leave this as is but please add specific instructions on how to reproduce (source test file, commands, and tools version, so that it's not a pain to recreate this in the future).

Hmm, ok, I'll do.

[grimar]

• Addressed review comments.

ELF/Target.cpp

void X86_64TargetInfo<ELFT>::relocateOne(uint8_t *Loc, uint32_t Type,
   case R_X86_64_DTPOFF64:
   case R_X86_64_GLOB_DAT:
   case R_X86_64_PC64:
   case R_X86_64_SIZE64:
   case R_X86_64_GOT64:
     write64le(Loc, Val);
     break;
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 template <class ELFT>
 RelExpr X86_64TargetInfo<ELFT>::adjustRelaxExpr(uint32_t Type,
                                                 const uint8_t *Data,
                                                 RelExpr RelExpr) const {
   if (Type != R_X86_64_GOTPCRELX && Type != R_X86_64_REX_GOTPCRELX)
void PPCTargetInfo::relocateOne(uint8_t *Loc, uint32_t Type,
   case R_PPC_ADDR32:
   case R_PPC_REL32:
     write32be(Loc, Val);
     break;
   case R_PPC_REL24:
     or32be(Loc, Val & 0x3FFFFFC);
     break;
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 RelExpr PPCTargetInfo::getRelExpr(uint32_t Type, const SymbolBody &S) const {
   switch (Type) {
   case R_PPC_REL24:
   case R_PPC_REL32:
     return R_PC;
case R_PPC64_TOC:
     break;
   case R_PPC64_REL24: {
     uint32_t Mask = 0x03FFFFFC;
     checkInt<24>(Loc, Val, Type);
     write32be(Loc, (read32be(Loc) & ~Mask) | (Val & Mask));
     break;
   }
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 AArch64TargetInfo::AArch64TargetInfo() {
   CopyRel = R_AARCH64_COPY;
   RelativeRel = R_AARCH64_RELATIVE;
   IRelativeRel = R_AARCH64_IRELATIVE;
   GotRel = R_AARCH64_GLOB_DAT;
case R_AARCH64_TLSLE_ADD_TPREL_HI12:
     checkInt<24>(Loc, Val, Type);
     or32AArch64Imm(Loc, Val >> 12);
     break;
   case R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
   case R_AARCH64_TLSDESC_ADD_LO12_NC:
     or32AArch64Imm(Loc, Val);
     break;
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 void AArch64TargetInfo::relaxTlsGdToLe(uint8_t *Loc, uint32_t Type,
                                        uint64_t Val) const {
   // TLSDESC Global-Dynamic relocation are in the form:
   //   adrp    x0, :tlsdesc:v             [R_AARCH64_TLSDESC_ADR_PAGE21]
   //   ldr     x1, [x0, #:tlsdesc_lo12:v  [R_AARCH64_TLSDESC_LD64_LO12_NC]
void AMDGPUTargetInfo::relocateOne(uint8_t *Loc, uint32_t Type,
   case R_AMDGPU_ABS64:
     write64le(Loc, Val);
     break;
   case R_AMDGPU_GOTPCREL32_HI:
   case R_AMDGPU_REL32_HI:
     write32le(Loc, Val >> 32);
     break;
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 RelExpr AMDGPUTargetInfo::getRelExpr(uint32_t Type, const SymbolBody &S) const {
   switch (Type) {
   case R_AMDGPU_ABS32:
   case R_AMDGPU_ABS64:
     return R_ABS;
write16le(Loc,
                   ((Val >> 1) & 0x0400) |  // i
                   ((Val >> 12) & 0x000f)); // imm4
     write16le(Loc + 2,
               (read16le(Loc + 2) & 0x8f00) | // opcode
                   ((Val << 4) & 0x7000) |    // imm3
                   (Val & 0x00ff));           // imm8
     break;
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 uint64_t ARMTargetInfo::getImplicitAddend(const uint8_t *Buf,
                                           uint32_t Type) const {
   switch (Type) {
   default:
     return 0;
case R_MIPS_PC21_S2:
     break;
   case R_MIPS_PC26_S2:
     applyMipsPcReloc<E, 26, 2>(Loc, Type, Val);
     break;
   case R_MIPS_PC32:
     applyMipsPcReloc<E, 32, 0>(Loc, Type, Val);
     break;
   default:
-    fatal(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
+    error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 template <class ELFT>
 bool MipsTargetInfo<ELFT>::usesOnlyLowPageBits(uint32_t Type) const {
   return Type == R_MIPS_LO16 || Type == R_MIPS_GOT_OFST;
 }
 }
 }

test/ELF/invalid/invalid-relocation-x64.s

+## invalid-relocation-x64.elf contains relocations with invalid numeric id.
+## Next yaml code was used to create initial binary. After that it
+## was modified with hex-editor to replace known relocations with fake ones,
+## that have 0x98 and 0x98 ids.
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_FREEBSD
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    Info:            .text
+    Relocations:
+      - Offset:          0x0000000000000000
+        Symbol:          ''
+        Type:            R_X86_64_NONE
+      - Offset:          0x0000000000000000
+        Symbol:          ''
+        Type:            R_X86_64_NONE
+
+    
+# RUN: not ld.lld %p/Inputs/invalid-relocation-x64.elf -o %t2 2>&1 | FileCheck %s
+# CHECK: unrecognized reloc 152
+# CHECK: unrecognized reloc 153