-
Notifications
You must be signed in to change notification settings - Fork 12.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SelectionDAGBuilder.cpp's llvm_unreachable
is quite reachable, actually
#104718
Comments
Mind, it is probably the case that rustc should not emit the IR in question, but it seems incorrect to call something unreachable if it isn't. Especially if this causes errors to flow downstream to places in the machine optimizer and emitter which probably shouldn't be asked to handle such questionably-formed IR, and it should instead be eagerly rejected, perhaps with an inquiry as to why the author ever thought that LLVMIR would pass muster. |
Reduced test case: define void @test(ptr %ptr) naked {
getelementptr i8, ptr %ptr, i64 1
call void @llvm.trap()
unreachable
} Probably the IR verifier should check that the arguments of a naked function are not used? We don't lower arguments for naked functions, so everything in the backend will blow up if you try to use them. |
Should we change SelectionDAGBuilder to have a report_fatal_error instead of llvm_unreachable too? That way compiler optimizations can't send the code down some random path the next time something like this happens? |
This IR:
Hits this
llvm_unreachable
:llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Line 1957 in bf5cd42
If you don't enable trap on
llvm_unreachable
in the CMake, we get this instead:It seems to me, given the frequency with which code hits this, that LLVM prefers to put this macro in reachable branches of code. May I suggest renaming it, or perhaps removing the feature that it causes undefined behavior if reached at runtime?
The text was updated successfully, but these errors were encountered: