[PProcesssor] Filename should fall back to the written name when typo correction fails.

[hokein]

Summary

The test is added in Testcase is at https://reviews.llvm.org/D52775. I tried to add the test to clang's code
completion test, it doesn't reproduce the crash.

[hokein]

For reference, the crash stack is like:

ClangdTests: ../include/llvm/ADT/StringSet.h:39: std::pair<typename base::iterator, bool> llvm::StringSet<llvm::MallocAllocator>::insert(llvm::StringRef) [AllocatorTy = llvm::MallocAllocator]: Assertion `!Key.empty()' failed.
#0 0x000000000061e5ff llvm::sys::PrintStackTrace(llvm::raw_ostream&) /llvm-upstream/build/../lib/Support/Unix/Signals.inc:490:13
#1 0x000000000061c942 llvm::sys::RunSignalHandlers() llvm-upstream/build/../lib/Support/Signals.cpp:68:18
#2 0x000000000061e942 SignalHandler(int) llvm-upstream/build/../lib/Support/Unix/Signals.inc:353:1
#3 0x00007f1c964fb0c0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x110c0)
#4 0x00007f1c9508cfcf gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x32fcf)
#5 0x00007f1c9508e3fa abort (/lib/x86_64-linux-gnu/libc.so.6+0x343fa)
#6 0x00007f1c95085e37 (/lib/x86_64-linux-gnu/libc.so.6+0x2be37)
#7 0x00007f1c95085ee2 (/lib/x86_64-linux-gnu/libc.so.6+0x2bee2)
#8 0x0000000000a19f75 (llvm-upstream/build/tools/clang/tools/extra/unittests/clangd/./ClangdTests+0xa19f75)
#9 0x0000000000a1b72d (anonymous namespace)::DepCollectorPPCallbacks::InclusionDirective(clang::SourceLocation, clang::Token const&, llvm::StringRef, bool, clang::CharSourceRange, clang::FileEntry const*, llvm::StringRef, llvm::StringRef, clang::Module const*, clang::SrcMgr::CharacteristicKind) llvm-upstream/build/../tools/clang/lib/Frontend/DependencyFile.cpp:0:20
#10 0x0000000000ab958c clang::PPChainedCallbacks::InclusionDirective(clang::SourceLocation, clang::Token const&, llvm::StringRef, bool, clang::CharSourceRange, clang::FileEntry const*, llvm::StringRef, llvm::StringRef, clang::Module const*, clang::SrcMgr::CharacteristicKind) llvm-upstream/build/../tools/clang/include/clang/Lex/PPCallbacks.h:389:3
#11 0x0000000000ab958c clang::PPChainedCallbacks::InclusionDirective(clang::SourceLocation, clang::Token const&, llvm::StringRef, bool, clang::CharSourceRange, clang::FileEntry const*, llvm::StringRef, llvm::StringRef, clang::Module const*, clang::SrcMgr::CharacteristicKind) llvm-upstream/build/../tools/clang/include/clang/Lex/PPCallbacks.h:389:3
#12 0x0000000000abf696 clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, clang::Token&, clang::DirectoryLookup const*, clang::FileEntry const*, bool) llvm-upstream/build/../tools/clang/lib/Lex/PPDirectives.cpp:2043:16
#13 0x0000000000ac2589 clang::Preprocessor::HandleDirective(clang::Token&) llvm-upstream/build/../tools/clang/lib/Lex/PPDirectives.cpp:1021:14
#14 0x0000000000a97ac9 clang::Lexer::LexTokenInternal(clang::Token&, bool) llvm-upstream/build/../tools/clang/lib/Lex/Lexer.cpp:3934:7
#15 0x0000000000a957ad clang::Lexer::Lex(clang::Token&) llvm-upstream/build/../tools/clang/lib/Lex/Lexer.cpp:3155:3
#16 0x0000000000af4954 clang::Preprocessor::Lex(clang::Token&) llvm-upstream/build/../tools/clang/lib/Lex/Preprocessor.cpp:895:12
#17 0x00000000012da277 clang::Parser::ConsumeToken() llvm-upstream/build/../tools/clang/include/clang/Parse/Parser.h:416:12
#18 0x00000000012dc8d9 clang::Parser::Initialize() llvm-upstream/build/../tools/clang/lib/Parse/Parser.cpp:520:1

[kristina]

Could you add a regression test please? Also could you run this through a debugger and get a backtrace from a debugger instead of the crash report, it seems to be missing a few things likely from optimizations which a debugger will usually pick up. This seems to be asserting due to a duplicate element in a set.

Thank you.

[hokein]

Rescope the patch.

[hokein]

In D52774#1252294, @kristina wrote:

Could you add a regression test please? Also could you run this through a debugger and get a backtrace from a debugger instead of the crash report, it seems to be missing a few things likely from optimizations which a debugger will usually pick up. This seems to be asserting due to a duplicate element in a set.

Thank you.

Thanks for the comment. The assertion is caused by inserting an empty Filename to StringSet. The typo correction code change the Filename (which will be used afterwards) even the typo correction heuristic fails (e.g. for #include "./^", Filename is changed to empty, which is unexpected). The testcase is added at https://reviews.llvm.org/D52775. Unfortunately, I couldn't reproduce it with a simple clang CodeCompletion test.

As discussed offline with @sammccall, it would be better to let the author of typo correction to fix its issues (as the typo correction code is submitted recently, and maybe buggy). The original patch fixes two issues, I rescoped the patch to make it clearer for review.

[hokein]

Rebase.

lib/Lex/PPDirectives.cpp

void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc,
   bool IsMapped = false;
   const DirectoryLookup *CurDir;
   SmallString<1024> SearchPath;
   SmallString<1024> RelativePath;
   // We get the raw path only if we have 'Callbacks' to which we later pass
   // the path.
   ModuleMap::KnownHeader SuggestedModule;
   SourceLocation FilenameLoc = FilenameTok.getLocation();
+  auto NormalizePathForMSVCCompat = [this](llvm::StringRef FilePath) {
     SmallString<128> NormalizedPath;
-  if (LangOpts.MSVCCompat) {
-    NormalizedPath = Filename.str();
+    if (!LangOpts.MSVCCompat)
+      return NormalizedPath;
+    NormalizedPath = FilePath;
 #ifndef _WIN32
     llvm::sys::path::native(NormalizedPath);
 #endif
-  }
+    return NormalizedPath;
+  };
+  SmallString<128> NormalizedPath = NormalizePathForMSVCCompat(Filename);
+
   const FileEntry *File = LookupFile(
       FilenameLoc, LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename,
       isAngled, LookupFrom, LookupFromFile, CurDir,
       Callbacks ? &SearchPath : nullptr, Callbacks ? &RelativePath : nullptr,
       &SuggestedModule, &IsMapped);
 
   if (!File) {
     if (Callbacks) {
if (!SuppressIncludeNotFoundError) {
           Diag(FilenameTok, diag::err_pp_file_not_found_angled_include_not_fatal) <<
             Filename <<
             FixItHint::CreateReplacement(Range, "\"" + Filename.str() + "\"");
         }
       }
 
       // Check for likely typos due to leading or trailing non-isAlphanumeric
       // characters
-      StringRef OriginalFilename = Filename;
       if (!File) {
         // A heuristic to correct a typo file name by removing leading and
         // trailing non-isAlphanumeric characters.
         auto CorrectTypoFilename = [](llvm::StringRef Filename) {
           Filename = Filename.drop_until(isAlphanumeric);
           while (!Filename.empty() && !isAlphanumeric(Filename.back())) {
             Filename = Filename.drop_back();
           }
           return Filename;
         };
-        Filename = CorrectTypoFilename(Filename);
+        StringRef TypoCorrectionName = CorrectTypoFilename(Filename);
+        auto NormalizedTypoCorrectionName =
+            NormalizePathForMSVCCompat(TypoCorrectionName);
         File = LookupFile(
             FilenameLoc,
-            LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, isAngled,
-            LookupFrom, LookupFromFile, CurDir,
+            LangOpts.MSVCCompat ? NormalizedTypoCorrectionName.c_str()
+                                : TypoCorrectionName,
+            isAngled, LookupFrom, LookupFromFile, CurDir,
             Callbacks ? &SearchPath : nullptr,
             Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped);
         if (File) {
           SourceRange Range(FilenameTok.getLocation(), CharEnd);
-          auto Hint = isAngled ? FixItHint::CreateReplacement(
-                                     Range, "<" + Filename.str() + ">")
-                               : FixItHint::CreateReplacement(
-                                     Range, "\"" + Filename.str() + "\"");
+          auto Hint = isAngled
+                          ? FixItHint::CreateReplacement(
+                                Range, "<" + TypoCorrectionName.str() + ">")
+                                Range, "\"" + TypoCorrectionName.str() + "\"");
           Diag(FilenameTok, diag::err_pp_file_not_found_typo_not_fatal)
-              << OriginalFilename << Filename << Hint;
+              << Filename << TypoCorrectionName << Hint;
+          // We found the file, so set the Filename and NormalizedPath to the
+          // name after typo correction.
+          Filename = TypoCorrectionName;
+          NormalizedPath = NormalizedTypoCorrectionName;
         }
       }
 
       // If the file is still not found, just go with the vanilla diagnostic
       if (!File)
-        Diag(FilenameTok, diag::err_pp_file_not_found) << OriginalFilename
+        Diag(FilenameTok, diag::err_pp_file_not_found) << Filename
                                                        << FilenameRange;
     }
   }
 
   if (usingPCHWithThroughHeader() && SkippingUntilPCHThroughHeader) {
     if (isPCHThroughHeader(File))
       SkippingUntilPCHThroughHeader = false;
     return;