RustWrapper: work around unification of diagnostic handlers #83425
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
r? @nikic |
|
From what I can tell this will lose information about location of the error – the |
|
I just spent a few minutes trying to trace how the inline_asm_handler works, but I'm honestly entirely lost at this point. The old (asm) handler gets a cookie argument that the new one doesn't, so it's not even obvious to me that things can be ported over without loss? I'm also not sure what that cookie argument does: I got as far as seeing that it goes in InlineAsmError, and then gets used as a position in a span, so I strongly suspect that we're just SOL on showing spans on the newer LLVM. Happy to be proven wrong, naturally, but I don't see the span information unless it's hiding in the CodegenContext or DiagnosticInfo types (but that's somewhat beyond my skill - I can try and figure it out, I guess, but that'll have to wait a while). I suppose I can drop a TODO that the diagnostic_handler should try and grovel around for some span information? |
|
It would be fairly unfortunate if the span situation ends up not being looked at before the bump to LLVM 13, which I feel will almost definitely happen if this PR lands in its current form. FWIW this area of code in clang seems relevant: https://github.com/llvm/llvm-project/blob/3cb2346982399892eae36e660ffa7e77a27ee067/clang/lib/CodeGen/CodeGenAction.cpp#L508-L529 – it also has something to do with a cookie and whatnot. But implementing something like this might involve adding more bindings to RustWrapper or similar. |
THis came up in the review of rust-lang#83425: it's hard to imagine a use of LLVM_VERSION_LE() or LLVM_VERSION_EQ() that's not asking for trouble when a point release gets created, so let's just discard them to prevent the issue.
… r=cuviper LLVMWrapper: attractive nuisance macros This came up in the review of rust-lang#83425: it's hard to imagine a use of LLVM_VERSION_LE() or LLVM_VERSION_EQ() that's not asking for trouble when a point release gets created, so let's just discard them to prevent the issue.
|
Note that a lot of the asm tests aren't being run in CI because they require LLVM 10. These tests will fail when run by bors if source location support is missing. |
JohnCSimon
added
S-waiting-on-review
|
@nikic any updates on this? thanks |
|
@Dylan-DPC I believe right now this is blocked on someone (probably me, but I was on vacation for over a week and just got back) fixing up some of the diagnostic handler stuff to be correctly implemented on LLVM HEAD. I hope to have time for that this week. |
Dylan-DPC-zz
added
S-waiting-on-author
|
Alright, I just spent some time with this, and I think it's already good: the only two callsites of At this point I'm back to thinking this change is ready: the fixes I was expecting to need to make seem to have been mostly done in 5541f68, or other revisions that touch |
|
Can you squash the two commits? Given the Amanieu's explanation I think I'm comfortable adding whatever support necessary for >= 13.0 as tests failing will definitely make us take another look if anything is wrong there. |
This lets me build against llvm/main as of March 23rd, 2021. I'm not entirely sure this is _correct_, but it appears to be functionally identical to what was done in LLVM: existing callsites of setInlineAsmDiagnosticHandler were moved to SetDiagnosticHandler() on the context object, which we already set up in both places that we called setInlineAsmDiagnosticHandler().
|
Squashed! (In my local test that I did just now, I believe this patch is the only thing required to get rustc to build against LLVM HEAD as of today!) |
|
@bors r+ |
|
📌 Commit fc2a74c has been approved by |
bors
removed
the
S-waiting-on-author
bors
added
the
S-waiting-on-bors
RustWrapper: work around unification of diagnostic handlers This lets me build against llvm/main as of March 23rd, 2021. I'm not entirely sure this is _correct_, but it appears to be functionally identical to what was done in LLVM: existing callsites of setInlineAsmDiagnosticHandler were moved to SetDiagnosticHandler() on the context object, which we already set up in both places that we called setInlineAsmDiagnosticHandler().
|
☀️ Test successful - checks-actions |
This lets me build against llvm/main as of March 23rd, 2021. I'm not
entirely sure this is correct, but it appears to be functionally
identical to what was done in LLVM: existing callsites of
setInlineAsmDiagnosticHandler were moved to SetDiagnosticHandler() on
the context object, which we already set up in both places that we
called setInlineAsmDiagnosticHandler().