This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Mar 10 2016, 6:21 AM.

Details

Reviewers
ruiu
rafael
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.

Diff Detail

Event Timeline

grimar updated this revision to Diff 50270.Mar 10 2016, 6:21 AM
grimar retitled this revision from to [ELF] use fatal() instead of llvm_unreachable when performing relaxations..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
rafael edited edge metadata.Mar 10 2016, 8:14 AM
rafael added a subscriber: 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

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 ?

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.

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 added a subscriber: silvas.Mar 10 2016, 2:27 PM

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 abandoned this revision.Mar 11 2016, 12:31 AM

Per discussion + llvm_unreachable was correct here.