[ELF] use fatal() instead of llvm_unreachable when performing relaxations.

[grimar]

Summary

http://reviews.llvm.org/D18039 showed that we need some additional diagnostic.
At least it is good to output relocation type and it seems that fatal() is more appropriate
as we still can meet such critical situations at any time.

[• rafael]

fatal should only be used if this is actually reachable. Can you write
a testcase that shows that you can reach each of these?

Cheers,
Rafael

[grimar]

In D18041#371932, @rafael wrote:

fatal should only be used if this is actually reachable. Can you write
a testcase that shows that you can reach each of these?

Cheers,
Rafael

I think existence of such test would mean a bug in lld.
But we have fatal() in void X86_64TargetInfo::relocateOne() for example, I also don't sure we can write a test for that.
Main point here that we probably want to output the Type of "bad" relocation. It looks in llvm it is not common to use unreachable for that (I did not found any place).
So what about assert here then ?

[grimar]

I think existence of such test would mean a bug in lld.

In which case that is exactly what assert/llvm_unreachable are for.

But we have fatal() in void X86_64TargetInfo::relocateOne() for example, I also don't sure we can write a test for that.

It should be possible with at .o with an invalid reloc number, no?

So the summary is that if you can reach something without a bug, that
should be a fatal/error. If getting there is a a bug, it should be
llvm_unreachable.

Ok, so can I replace with assert() then ? (for http://reviews.llvm.org/D18041).
I want to output the Type of relocation, like I did when used fatal.

Cheers,
Rafael

George.

[grimar]

What is expected behavior if user gets llvm_unreachable in release for lld ?
If compiler will cut off the llvm_unreachable code, user probably will see nothing except crash.
We are using llvm_unreachable everywhere and just assuming it never gonna happen ?

(I am sorry if asking something obvious or already being answered, but that just very wierd for me).
fatal() at least will error out something. Why don't we use it for bugs and not only for errors ?
Doesn't error message is aways more userfriendly than crash ?

[silvas]

In D18041#371932, @rafael wrote:

fatal should only be used if this is actually reachable. Can you write
a testcase that shows that you can reach each of these?

Cheers,
Rafael

I agree. The one in relocateOne is I think actually a proper use of fatal() because it can be reached by an object file with an unknown relocation (say, some weird platform-specific relocation). For the TLS optimization ones, I think it is actually unreachable since it would otherwise indicate that canRelaxTlx and relaxTls are out of sync.

[grimar]

Per discussion + llvm_unreachable was correct here.

ELF/Target.cpp

size_t X86TargetInfo::relaxTls(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
   case R_386_TLS_LDM:
     relocateTlsLdToLe(Loc, BufEnd, P, SA);
     // The next relocation should be against __tls_get_addr, so skip it
     return 1;
   case R_386_TLS_LDO_32:
     relocateOne(Loc, BufEnd, R_386_TLS_LE, P, SA);
     return 0;
   }
-  llvm_unreachable("Unknown TLS optimization");
+  fatal("Unknown TLS optimization for relocation: " + Twine(Type));
 }
 
 // "Ulrich Drepper, ELF Handling For Thread-Local Storage" (5.1
 // IA-32 Linker Optimizations, http://www.akkadia.org/drepper/tls.pdf) shows
 // how GD can be optimized to IE:
 //   leal x@tlsgd(, %ebx, 1),
 //   call __tls_get_addr@plt
 // Is converted to:
case R_X86_64_TLSGD: {
     // The next relocation should be against __tls_get_addr, so skip it
     return 1;
   }
   case R_X86_64_TLSLD:
     relocateTlsLdToLe(Loc, BufEnd, P, SA);
     // The next relocation should be against __tls_get_addr, so skip it
     return 1;
   }
-  llvm_unreachable("Unknown TLS optimization");
+  fatal("Unknown TLS optimization for relocation: " + Twine(Type));
 }
 
 void X86_64TargetInfo::relocateOne(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
                                    uint64_t P, uint64_t SA, uint64_t ZA,
                                    uint8_t *PairedLoc) const {
   switch (Type) {
   case R_X86_64_32:
     checkUInt<32>(SA, Type);
size_t AArch64TargetInfo::relaxTls(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
                                    uint64_t P, uint64_t SA,
                                    const SymbolBody *S) const {
   switch (Type) {
   case R_AARCH64_TLSDESC_ADR_PAGE21:
   case R_AARCH64_TLSDESC_LD64_LO12_NC:
   case R_AARCH64_TLSDESC_ADD_LO12_NC:
   case R_AARCH64_TLSDESC_CALL: {
     if (canBePreempted(S))
-      fatal("Unsupported TLS optimization");
+      fatal("Unsupported TLS optimization for relocation: " + Twine(Type));
     uint64_t X = S ? S->getVA<ELF64LE>() : SA;
     relocateTlsGdToLe(Type, Loc, BufEnd, P, X);
     return 0;
   }
   case R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21:
   case R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
     relocateTlsIeToLe(Type, Loc, BufEnd, P, S->getVA<ELF64LE>());
     return 0;
   }
-  llvm_unreachable("Unknown TLS optimization");
+  fatal("Unknown TLS optimization for relocation: " + Twine(Type));
 }
 
 // Global-Dynamic relocations can be relaxed to Local-Exec if both binary is
 // an executable and target is final (can notbe preempted).
 void AArch64TargetInfo::relocateTlsGdToLe(uint32_t Type, uint8_t *Loc,
                                           uint8_t *BufEnd, uint64_t P,
                                           uint64_t SA) const {
   // TLSDESC Global-Dynamic relocation are in the form: