This is an archive of the discontinued LLVM Phabricator instance.

[Parser] Don't crash on MS assembly if target desc/asm parser isn't linked in.
ClosedPublic

Authored by sammccall on Dec 9 2019, 2:57 AM.

Details

Summary

Instead, emit a diagnostic and return an empty ASM node, as we do if the target
is missing.

Filter this diagnostic out in clangd, where it's not meaningful.

Fixes https://github.com/clangd/clangd/issues/222

Diff Detail

Event Timeline

sammccall created this revision.Dec 9 2019, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 2:57 AM
sammccall updated this revision to Diff 232791.Dec 9 2019, 2:59 AM

revert random meaningless changes

kadircet added inline comments.Dec 9 2019, 3:03 AM
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
1017

nit: empty line

clang/lib/Parse/ParseStmtAsm.cpp
591

is it ok for MRI to be dereferenced here?

599

the original bail out(no target or no tokens) doesn't seem to be emitting diags, are we sure we want to emit diags here? (same for the one below)

Build result: pass - 60607 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

Build result: pass - 60607 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

sammccall marked 4 inline comments as done.Dec 9 2019, 3:27 AM
sammccall added inline comments.
clang/lib/Parse/ParseStmtAsm.cpp
591

No :-( Fixed with yet more code
(In practice it didn't cause a problem because all these are null or not-null, and the function doesn't do anything if they're null)

599

For no tokens there's no diagnostic required, the empty statement is correct.

Missing target does have a diagnostic: the if statement on line 555 will either set Target to non-null or emit a diagnostic.

sammccall updated this revision to Diff 232798.Dec 9 2019, 3:28 AM
sammccall marked an inline comment as done.

address comments

Build result: pass - 60607 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

kadircet accepted this revision.Dec 9 2019, 4:39 AM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 9 2019, 4:39 AM
This revision was automatically updated to reflect the committed changes.