rustc: Fail if LLVM is passed an invalid triple #18411
Conversation
|
|
richo
changed the title
|
This actually wasn't nearly as invasive as I first thought. This should be safe to merge. |
| return Err(error_str); | ||
| } else { | ||
| return Ok(tm); | ||
| }; |
There was a problem hiding this comment.
This may want to instead leverage the llvm_err method at the top of this file just to ensure we're not duplicating logic with LLVMRustGetLastError
There was a problem hiding this comment.
This also seems easy enough to turn the ICE into a normal compiler error via the call to fatal in llvm_err, so could a test be added for this as well?
There was a problem hiding this comment.
I've gone up a bunch of frames, and I can't find a Handler in scope. Is there somewhere specific I should be looking?
(I also have a vague concern that this is limiting the reusability of a lot of the target selection code. I'm not sure that's actually a problem though, because I don't have a good sense of how reusable librustc is meant to be)
There was a problem hiding this comment.
I think you can get one out of a Session via sess.diagnostic().handler() (or something attached to a Session like that).
Also yeah it's fine to not worry about the reusability librustc, it's not planned to :)
|
I've updated the branch. Writing a test for this is hard because I think this condition was only possible due to quirks in the build infrastructure. I can push the power port I've been working on which currently exhibits the failure if that would help, but unless you can see something I can't I think this is ready to go out |
This changes create_target_machine to correctly return a Result (Since the underlying LLVM function can fail and return NULL)
|
Ah I see, I do seem to remember that we try to do validation ahead of time wherever possible anyway. Thanks for looking into it! (it's ok to not have a test) |
This changes create_target_machine to correctly return a Result (Since
the underlying LLVM function can fail and return NULL)
There's an underlying issue that our build infra doesn't stop you from
getting to this point, but this at least turns a segfault into an ICE.