Merged master:1e87261ba17 into amd-gfx:b313f433300

Local branch amd-gfx b313f43 Merged master:3dcfd482cb1 into amd-gfx:b927e58a6c3
Remote branch master 1e87261 [clangd] Turn on RecoveryAST for clangd by default.

.gitignore

@@ -34,6 +34,7 @@
 /GRTAGS
 /GSYMS
 /GTAGS
+/ID
 .gitusers
 autom4te.cache
 cscope.files

clang-tools-extra/clangd/ClangdServer.h

@@ -121,7 +121,7 @@ class ClangdServer {
     ClangTidyOptionsBuilder GetClangTidyOptions;
 
     /// If true, turn on the `-frecovery-ast` clang flag.
-    bool BuildRecoveryAST = false;
+    bool BuildRecoveryAST = true;
 
     /// If true, turn on the `-frecovery-ast-type` clang flag.
     bool PreserveRecoveryASTType = false;
@@ -354,7 +354,7 @@ class ClangdServer {
   bool SuggestMissingIncludes = false;
 
   // If true, preserve expressions in AST for broken code.
-  bool BuildRecoveryAST = false;
+  bool BuildRecoveryAST = true;
   // If true, preserve the type for recovery AST.
   bool PreserveRecoveryASTType = false;
 

clang-tools-extra/clangd/Diagnostics.cpp

@@ -24,6 +24,7 @@
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -120,49 +121,96 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   return halfOpenToRange(M, R);
 }
 
-// Returns whether the \p D is modified.
-bool adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
-                          const LangOptions &LangOpts) {
-  // We only report diagnostics with at least error severity from headers.
-  // Use default severity to avoid noise with -Werror.
-  if (!Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
-          Info.getID()))
-    return false;
-
-  const SourceManager &SM = Info.getSourceManager();
-  const SourceLocation &DiagLoc = SM.getExpansionLoc(Info.getLocation());
-  SourceLocation IncludeInMainFile;
+// Try to find a location in the main-file to report the diagnostic D.
+// Returns a description like "in included file", or nullptr on failure.
+const char *getMainFileRange(const Diag &D, const SourceManager &SM,
+                             SourceLocation DiagLoc, Range &R) {
+  // Look for a note in the main file indicating template instantiation.
+  for (const auto &N : D.Notes) {
+    if (N.InsideMainFile) {
+      switch (N.ID) {
+      case diag::note_template_class_instantiation_was_here:
+      case diag::note_template_class_explicit_specialization_was_here:
+      case diag::note_template_class_instantiation_here:
+      case diag::note_template_member_class_here:
+      case diag::note_template_member_function_here:
+      case diag::note_function_template_spec_here:
+      case diag::note_template_static_data_member_def_here:
+      case diag::note_template_variable_def_here:
+      case diag::note_template_enum_def_here:
+      case diag::note_template_nsdmi_here:
+      case diag::note_template_type_alias_instantiation_here:
+      case diag::note_template_exception_spec_instantiation_here:
+      case diag::note_template_requirement_instantiation_here:
+      case diag::note_evaluating_exception_spec_here:
+      case diag::note_default_arg_instantiation_here:
+      case diag::note_default_function_arg_instantiation_here:
+      case diag::note_explicit_template_arg_substitution_here:
+      case diag::note_function_template_deduction_instantiation_here:
+      case diag::note_deduced_template_arg_substitution_here:
+      case diag::note_prior_template_arg_substitution:
+      case diag::note_template_default_arg_checking:
+      case diag::note_concept_specialization_here:
+      case diag::note_nested_requirement_here:
+      case diag::note_checking_constraints_for_template_id_here:
+      case diag::note_checking_constraints_for_var_spec_id_here:
+      case diag::note_checking_constraints_for_class_spec_id_here:
+      case diag::note_checking_constraints_for_function_here:
+      case diag::note_constraint_substitution_here:
+      case diag::note_constraint_normalization_here:
+      case diag::note_parameter_mapping_substitution_here:
+        R = N.Range;
+        return "in template";
+      default:
+        break;
+      }
+    }
+  }
+  // Look for where the file with the error was #included.
   auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
     return SM.getIncludeLoc(SM.getFileID(SLoc));
   };
-  for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
+  for (auto IncludeLocation = GetIncludeLoc(SM.getExpansionLoc(DiagLoc));
+       IncludeLocation.isValid();
        IncludeLocation = GetIncludeLoc(IncludeLocation)) {
     if (clangd::isInsideMainFile(IncludeLocation, SM)) {
-      IncludeInMainFile = IncludeLocation;
-      break;
+      R.start = sourceLocToPosition(SM, IncludeLocation);
+      R.end = sourceLocToPosition(
+          SM,
+          Lexer::getLocForEndOfToken(IncludeLocation, 0, SM, LangOptions()));
+      return "in included file";
     }
   }
-  if (IncludeInMainFile.isInvalid())
-    return false;
+  return nullptr;
+}
 
-  // Update diag to point at include inside main file.
-  D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
-  D.Range.start = sourceLocToPosition(SM, IncludeInMainFile);
-  D.Range.end = sourceLocToPosition(
-      SM, Lexer::getLocForEndOfToken(IncludeInMainFile, 0, SM, LangOpts));
-  D.InsideMainFile = true;
+// Place the diagnostic the main file, rather than the header, if possible:
+//   - for errors in included files, use the #include location
+//   - for errors in template instantiation, use the instantation location
+// In both cases, add the original header location as a note.
+bool tryMoveToMainFile(Diag &D, FullSourceLoc DiagLoc) {
+  const SourceManager &SM = DiagLoc.getManager();
+  DiagLoc = DiagLoc.getExpansionLoc();
+  Range R;
+  const char *Prefix = getMainFileRange(D, SM, DiagLoc, R);
+  if (!Prefix)
+    return false;
 
   // Add a note that will point to real diagnostic.
   const auto *FE = SM.getFileEntryForID(SM.getFileID(DiagLoc));
-  D.Notes.emplace_back();
-  Note &N = D.Notes.back();
+  D.Notes.emplace(D.Notes.begin());
+  Note &N = D.Notes.front();
   N.AbsFile = std::string(FE->tryGetRealPathName());
   N.File = std::string(FE->getName());
   N.Message = "error occurred here";
-  N.Range = diagnosticRange(Info, LangOpts);
+  N.Range = D.Range;
 
+  // Update diag to point at include inside main file.
+  D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
+  D.Range = std::move(R);
+  D.InsideMainFile = true;
   // Update message to mention original file.
-  D.Message = llvm::Twine("in included file: ", D.Message).str();
+  D.Message = llvm::formatv("{0}: {1}", Prefix, D.Message);
   return true;
 }
 
@@ -475,6 +523,7 @@ void StoreDiags::BeginSourceFile(const LangOptions &Opts,
 }
 
 void StoreDiags::EndSourceFile() {
+  flushLastDiag();
   LangOpts = None;
 }
 
@@ -512,19 +561,23 @@ static void fillNonLocationData(DiagnosticsEngine::Level DiagLevel,
 void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+  bool OriginallyError =
+      Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
+          Info.getID());
 
   if (Info.getLocation().isInvalid()) {
     // Handle diagnostics coming from command-line arguments. The source manager
     // is *not* available at this point, so we cannot use it.
-    if (!Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
-            Info.getID())) {
+    if (!OriginallyError) {
       IgnoreDiagnostics::log(DiagLevel, Info);
       return; // non-errors add too much noise, do not show them.
     }
 
     flushLastDiag();
 
     LastDiag = Diag();
+    LastDiagLoc.reset();
+    LastDiagOriginallyError = OriginallyError;
     LastDiag->ID = Info.getID();
     fillNonLocationData(DiagLevel, Info, *LastDiag);
     LastDiag->InsideMainFile = true;
@@ -550,6 +603,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     D.File = std::string(SM.getFilename(Info.getLocation()));
     D.AbsFile = getCanonicalPath(
         SM.getFileEntryForID(SM.getFileID(Info.getLocation())), SM);
+    D.ID = Info.getID();
     return D;
   };
 
@@ -634,10 +688,9 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     LastPrimaryDiagnosticWasSuppressed = false;
 
     LastDiag = Diag();
-    LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
-    if (!InsideMainFile)
-      LastDiagWasAdjusted = adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
+    LastDiagLoc.emplace(Info.getLocation(), Info.getSourceManager());
+    LastDiagOriginallyError = OriginallyError;
 
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
@@ -679,16 +732,28 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
 void StoreDiags::flushLastDiag() {
   if (!LastDiag)
     return;
-  if (!isBlacklisted(*LastDiag) && mentionsMainFile(*LastDiag) &&
-      (!LastDiagWasAdjusted ||
-       // Only report the first diagnostic coming from each particular header.
-       IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) {
-    Output.push_back(std::move(*LastDiag));
-  } else {
-    vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
+  auto Finish = llvm::make_scope_exit([&, NDiags(Output.size())] {
+    if (Output.size() == NDiags) // No new diag emitted.
+      vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
+    LastDiag.reset();
+  });
+
+  if (isBlacklisted(*LastDiag))
+    return;
+  // Move errors that occur from headers into main file.
+  if (!LastDiag->InsideMainFile && LastDiagLoc && LastDiagOriginallyError) {
+    if (tryMoveToMainFile(*LastDiag, *LastDiagLoc)) {
+      // Suppress multiple errors from the same inclusion.
+      if (!IncludedErrorLocations
+               .insert({LastDiag->Range.start.line,
+                        LastDiag->Range.start.character})
+               .second)
+        return;
+    }
   }
-  LastDiag.reset();
-  LastDiagWasAdjusted = false;
+  if (!mentionsMainFile(*LastDiag))
+    return;
+  Output.push_back(std::move(*LastDiag));
 }
 
 } // namespace clangd

clang-tools-extra/clangd/Diagnostics.h

@@ -13,6 +13,7 @@
 #include "support/Path.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/None.h"
@@ -64,6 +65,7 @@ struct DiagBase {
   // Since File is only descriptive, we store a separate flag to distinguish
   // diags from the main file.
   bool InsideMainFile = false;
+  unsigned ID; // e.g. member of clang::diag, or clang-tidy assigned ID.
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D);
 
@@ -82,7 +84,6 @@ struct Note : DiagBase {};
 
 /// A top-level diagnostic that may have Notes and Fixes.
 struct Diag : DiagBase {
-  unsigned ID;      // e.g. member of clang::diag, or clang-tidy assigned ID.
   std::string Name; // if ID was recognized.
   // The source of this diagnostic.
   enum {
@@ -145,9 +146,10 @@ class StoreDiags : public DiagnosticConsumer {
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
-  /// Set iff adjustDiagFromHeader resulted in changes to LastDiag.
-  bool LastDiagWasAdjusted = false;
-  llvm::DenseSet<int> IncludeLinesWithErrors;
+  llvm::Optional<FullSourceLoc> LastDiagLoc; // Valid only when LastDiag is set.
+  bool LastDiagOriginallyError = false;      // Valid only when LastDiag is set.
+
+  llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
   bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 

clang-tools-extra/clangd/Preamble.cpp

@@ -13,6 +13,7 @@
 #include "support/FSProvider.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
@@ -98,6 +99,19 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
     return IWYUHandler.get();
   }
 
+  bool shouldSkipFunctionBody(Decl *D) override {
+    // Generally we skip function bodies in preambles for speed.
+    // We can make exceptions for functions that are cheap to parse and
+    // instantiate, widely used, and valuable (e.g. commonly produce errors).
+    if (const auto *FT = llvm::dyn_cast<clang::FunctionTemplateDecl>(D)) {
+      if (const auto *II = FT->getDeclName().getAsIdentifierInfo())
+        // std::make_unique is trivial, and we diagnose bad constructor calls.
+        if (II->isStr("make_unique") && FT->isInStdNamespace())
+          return false;
+    }
+    return true;
+  }
+
 private:
   PathRef File;
   PreambleParsedCallback ParsedCallback;

clang-tools-extra/clangd/tool/ClangdMain.cpp

@@ -281,11 +281,10 @@ opt<bool> CrossFileRename{
 opt<bool> RecoveryAST{
     "recovery-ast",
     cat(Features),
-    desc("Preserve expressions in AST for broken code (C++ only). Note that "
-         "this feature is experimental and may lead to crashes"),
-    init(false),
-    Hidden,
+    desc("Preserve expressions in AST for broken code (C++ only)."),
+    init(ClangdServer::Options().BuildRecoveryAST),
 };
+
 opt<bool> RecoveryASTType{
     "recovery-ast-type",
     cat(Features),

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

@@ -1197,7 +1197,9 @@ TEST(SignatureHelpTest, OpeningParen) {
     int foo(int a, int b, int c);
     int main() {
     #define ID(X) X
-      ID(foo $p^( foo(10), ^ ))
+      // FIXME: figure out why ID(foo (foo(10), )) doesn't work when preserving
+      // the recovery expression.
+      ID(foo $p^( 10, ^ ))
     })cpp"};
 
   for (auto Test : Tests) {