From 94603ec11b55ca22b5dbebcfca5e83f313b632e3 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Mon, 9 Dec 2019 11:57:23 +0100 Subject: [PATCH] [Parser] Don't crash on MS assembly if target desc/asm parser isn't linked in. 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 Reviewers: kadircet Subscribers: mgorny, ilya-biryukov, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D71189 --- clang-tools-extra/clangd/Diagnostics.cpp | 9 ++++++- clang-tools-extra/clangd/unittests/CMakeLists.txt | 1 + .../clangd/unittests/DiagnosticsTests.cpp | 10 ++++++++ clang/lib/Parse/ParseStmtAsm.cpp | 28 ++++++++++++++++++---- 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index cd95807..f971911 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -73,6 +73,13 @@ bool mentionsMainFile(const Diag &D) { return false; } +bool isBlacklisted(const Diag &D) { + // clang will always fail to MS ASM as we don't link in desc + asm parser. + if (D.ID == clang::diag::err_msasm_unable_to_create_target) + return true; + return false; +} + // Checks whether a location is within a half-open range. // Note that clang also uses closed source ranges, which this can't handle! bool locationInRange(SourceLocation L, CharSourceRange R, @@ -642,7 +649,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, void StoreDiags::flushLastDiag() { if (!LastDiag) return; - if (mentionsMainFile(*LastDiag) && + if (!isBlacklisted(*LastDiag) && mentionsMainFile(*LastDiag) && (!LastDiagWasAdjusted || // Only report the first diagnostic coming from each particular header. IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) { diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt index d2a6890..f8b24c6 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -1,5 +1,6 @@ set(LLVM_LINK_COMPONENTS support + AllTargetsInfos ) get_filename_component(CLANGD_SOURCE_DIR diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 3c02578..0941af2 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -19,6 +19,7 @@ #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticSema.h" #include "llvm/Support/ScopedPrinter.h" +#include "llvm/Support/TargetSelect.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include @@ -405,6 +406,15 @@ TEST(DiagnosticsTest, NoFixItInMacro) { Not(WithFix(_))))); } +TEST(ClangdTest, MSAsm) { + // Parsing MS assembly tries to use the target MCAsmInfo, which we don't link. + // We used to crash here. Now clang emits a diagnostic, which we filter out. + llvm::InitializeAllTargetInfos(); // As in ClangdMain + auto TU = TestTU::withCode("void fn() { __asm { cmp cl,64 } }"); + TU.ExtraArgs = {"-fms-extensions"}; + EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty()); +} + TEST(DiagnosticsTest, ToLSP) { URIForFile MainFile = URIForFile::canonicalize(testPath("foo/bar/main.cpp"), ""); diff --git a/clang/lib/Parse/ParseStmtAsm.cpp b/clang/lib/Parse/ParseStmtAsm.cpp index 6b30166..98133aa 100644 --- a/clang/lib/Parse/ParseStmtAsm.cpp +++ b/clang/lib/Parse/ParseStmtAsm.cpp @@ -563,16 +563,19 @@ StmtResult Parser::ParseMicrosoftAsmStatement(SourceLocation AsmLoc) { assert(!LBraceLocs.empty() && "Should have at least one location here"); + SmallString<512> AsmString; + auto EmptyStmt = [&] { + return Actions.ActOnMSAsmStmt(AsmLoc, LBraceLocs[0], AsmToks, AsmString, + /*NumOutputs*/ 0, /*NumInputs*/ 0, + ConstraintRefs, ClobberRefs, Exprs, EndLoc); + }; // If we don't support assembly, or the assembly is empty, we don't // need to instantiate the AsmParser, etc. if (!TheTarget || AsmToks.empty()) { - return Actions.ActOnMSAsmStmt(AsmLoc, LBraceLocs[0], AsmToks, StringRef(), - /*NumOutputs*/ 0, /*NumInputs*/ 0, - ConstraintRefs, ClobberRefs, Exprs, EndLoc); + return EmptyStmt(); } // Expand the tokens into a string buffer. - SmallString<512> AsmString; SmallVector TokOffsets; if (buildMSAsmString(PP, AsmLoc, AsmToks, TokOffsets, AsmString)) return StmtError(); @@ -582,6 +585,11 @@ StmtResult Parser::ParseMicrosoftAsmStatement(SourceLocation AsmLoc) { llvm::join(TO.Features.begin(), TO.Features.end(), ","); std::unique_ptr MRI(TheTarget->createMCRegInfo(TT)); + if (!MRI) { + Diag(AsmLoc, diag::err_msasm_unable_to_create_target) + << "target MC unavailable"; + return EmptyStmt(); + } // FIXME: init MCOptions from sanitizer flags here. llvm::MCTargetOptions MCOptions; std::unique_ptr MAI( @@ -591,6 +599,12 @@ StmtResult Parser::ParseMicrosoftAsmStatement(SourceLocation AsmLoc) { std::unique_ptr MOFI(new llvm::MCObjectFileInfo()); std::unique_ptr STI( TheTarget->createMCSubtargetInfo(TT, TO.CPU, FeaturesStr)); + // Target MCTargetDesc may not be linked in clang-based tools. + if (!MAI || !MII | !MOFI || !STI) { + Diag(AsmLoc, diag::err_msasm_unable_to_create_target) + << "target MC unavailable"; + return EmptyStmt(); + } llvm::SourceMgr TempSrcMgr; llvm::MCContext Ctx(MAI.get(), MRI.get(), MOFI.get(), &TempSrcMgr); @@ -607,6 +621,12 @@ StmtResult Parser::ParseMicrosoftAsmStatement(SourceLocation AsmLoc) { std::unique_ptr TargetParser( TheTarget->createMCAsmParser(*STI, *Parser, *MII, MCOptions)); + // Target AsmParser may not be linked in clang-based tools. + if (!TargetParser) { + Diag(AsmLoc, diag::err_msasm_unable_to_create_target) + << "target ASM parser unavailable"; + return EmptyStmt(); + } std::unique_ptr IP( TheTarget->createMCInstPrinter(llvm::Triple(TT), 1, *MAI, *MII, *MRI)); -- 2.7.4