When using clangd, it's possible to trigger assertions in NumericLiteralParser and CharLiteralParser when switching git branches. This commit removes the initial asserts on invalid input and replaces those asserts with the error handling mechanism from those respective classes instead. This allows clangd to gracefully recover without crashing. See https://github.com/clangd/clangd/issues/888 for more information on the clangd crashes.
|380 ms||x64 debian > LLVM.CodeGen/PowerPC::mi-peepholes-trap-opt.mir|
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple powerpc64le-unknown-linux-gnu -mcpu=pwr8 -x mir < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/mi-peepholes-trap-opt.mir -verify-machineinstrs -start-before=ppc-mi-peepholes | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/mi-peepholes-trap-opt.mir
Added some people that were recent reviewers of changes to this file and some clangd folks.
I don't have the time to properly test this unfortunately (aside from verifying that it fixes all the clangd crashes I'm having), but putting the patch up anyway in case anyone's interested.
err_lexing_string’s message is “failure when lexing a string”, which isn’t accurate here since you’re lexing a character literal or numeric literal instead. Could you emit a more appropriate message for this? That might mean adding additional diagnostics or modifying the existing one so you can insert information about the kind of literal.
(err_lexing_string is only used for “can’t happen” errors, so maybe you could change the message to something like failure when lexing a %0 literal; a file may have been modified during compilation.)
I’m otherwise pretty happy with this change—we’ve seen similar un-reproducible crashes in string literal parsing, and this kind of solution has worked well there.
Added a bit of analysis on https://github.com/clangd/clangd/issues/888.
The short version is we built a PCH. When consuming it, the parser decides to re-lex and this means sources must be loaded from disk.
These sources are inconsistent with the PCH, so the precondition that the input range describes a reasonable literal doesn't hold.
This patch fixes it by removing the precondition and defending against it instead.
This seems like an OK fallback unless there's hundreds of other places where we might re-lex and make similar assumptions.
I'm going to dig and try to find out but if anyone knows about this it'd be nice to chime in :-)