Page MenuHomePhabricator

LiteralSupport: Don't assert() on invalid input
ClosedPublic

Authored by DaanDeMeyer on Nov 16 2021, 8:02 AM.

Details

Summary
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.

Diff Detail

Unit TestsFailed

TimeTest
380 msx64 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

Event Timeline

DaanDeMeyer created this revision.Nov 16 2021, 8:02 AM
DaanDeMeyer requested review of this revision.Nov 16 2021, 8:02 AM

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.

clang-format

beccadax requested changes to this revision.EditedNov 16 2021, 9:49 AM

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.

This revision now requires changes to proceed.Nov 16 2021, 9:49 AM

Addressed comments by adding two new errors, one for character literals and one for numeric literals.

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 :-)

beccadax accepted this revision.Nov 17 2021, 7:23 AM

The diagnostics are now much better—thanks!

This revision is now accepted and ready to land.Nov 17 2021, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2022, 1:53 PM