Allow interfaces to operate on in-memory buffers with no source location info.

[tapaswenipathak]

Summary

This is a part of the RFC mentioned here - https://lists.llvm.org/pipermail/cfe-dev/2020-July/066203.html where we plan to move parts of Cling upstream.

Cling has the ability to spawn child interpreters (mainly for auto completions). We needed
to apply this patch on top of our fork of Clang, because otherwise when we try to
import a Decl into the Cling child interpreter using
Clang::ASTImporter, the assertion here - https://github.com/llvm/llvm-project/blob/65eb74e94b414fcde6bfa810d1c30c7fcb136b77/clang/include/clang/Basic/SourceLocation.h#L322 fails.

[shafik]

This looks reasonable, I am guessing we can't test this b/c it would only come up in a cling use case?

Was there ever a design document that Hal and JF were asking for? I was reading through the cfe-dev thread and I don't see it.

[v.g.vassilev]

@shafik, I suppose that with a good amount of effort we may be able to test it in a unittest setup.

In my point of view, this patch is rather fixing an interface inconsistency which was discovered in the context of cling.

The ASTImporter already makes a promise to handle invalid source locations here: https://github.com/llvm-project/clang/blob/master/lib/AST/ASTImporter.cpp#L8388-L8392

However, if the presumed location is invalid we call an isWrittenInBuiltinFile and fail. I think isWrittenInBuiltinFile should not assert if called with an invalid source location or presumed source location -- the answer should be false. Alternatively, we can extend the ASTImporter to support also invalid presumed locations.

The design document will likely not contain such details ;)

[teemperor]

I think we could at least have a unittest that just calls these functions with an invalid SourceLocation. unittests/Basic/SourceManagerTest.cpp already takes care of creating a valid SourceManager object, so the unit test would just be a single call to each method. Of course it wouldn't be the exact same setup as whatever is Cling is doing, but it's better than nothing.

Also, I'm still trying to puzzle together what exactly the situation is that triggers this bug in Cling. From what I understand:

1. We call the ASTImporter::Import(Decl) with a Decl.
2. That triggers the importing of some SourceLocation. Given the ASTImporter is early-exiting on an invalid SourceLocation, this means you were importing a valid SourceLocation when hitting this crash.
3. We enter with a valid SourceLocation isWrittenInBuiltinFile from ASTImporter::Import(SourceLocation). Now the function is computing the PresumedLoc via getPresumedLoc and that returns an invalid PresumedLoc.
4. We hit the assert due to the invalid PresumedLoc.

Do you know where exactly is getPresumedLoc returning the invalid error value? The review title just mentions a "in-memory buffer with no source location info", but it doesn't really explain what's going on. Are there SourceLocations pointing to that memory buffer but it's not registered with the SourceManager?

[tapaswenipathak]

Ref: https://reviews.llvm.org/D126600.

[tapaswenipathak]

Allow interfaces to operate on in-memory buffers with no source location info.

This patch avoids an assert PresumedLoc::getFilename if it is invalid.

Add unit tests for allowing the interface to operate on in-memory buffers with no
source location info.

It addresses all review comments of https://reviews.llvm.org/D88780.

Ref: https://reviews.llvm.org/D126271.

Co-authored-by: Vassil Vassilev <[e-mail]>

[v.g.vassilev]

This looks reasonable to me...

@shafik, @teemperor what do you think?

[v.g.vassilev]

I'd propose to move forward here and rely on eventual post-commit review.

clang/include/clang/Basic/SourceManager.h

public:
   ///
   /// This check ignores line marker directives.
   bool isWrittenInMainFile(SourceLocation Loc) const {
     return getFileID(Loc) == getMainFileID();
   }
 
   /// Returns whether \p Loc is located in a <built-in> file.
   bool isWrittenInBuiltinFile(SourceLocation Loc) const {
-    StringRef Filename(getPresumedLoc(Loc).getFilename());
+    PresumedLoc Presumed = getPresumedLoc(Loc);
+    if (Presumed.isInvalid())
+      return false;
+    StringRef Filename(Presumed.getFilename());
     return Filename.equals("<built-in>");
   }
 
   /// Returns whether \p Loc is located in a <command line> file.
   bool isWrittenInCommandLineFile(SourceLocation Loc) const {
-    StringRef Filename(getPresumedLoc(Loc).getFilename());
+    PresumedLoc Presumed = getPresumedLoc(Loc);
+    if (Presumed.isInvalid())
+      return false;
+    StringRef Filename(Presumed.getFilename());
     return Filename.equals("<command line>");
   }
 
   /// Returns whether \p Loc is located in a <scratch space> file.
   bool isWrittenInScratchSpace(SourceLocation Loc) const {
-    StringRef Filename(getPresumedLoc(Loc).getFilename());
+    PresumedLoc Presumed = getPresumedLoc(Loc);
+    if (Presumed.isInvalid())
+      return false;
+    StringRef Filename(Presumed.getFilename());
     return Filename.equals("<scratch space>");
   }
 
   /// Returns if a SourceLocation is in a system header.
   bool isInSystemHeader(SourceLocation Loc) const {
+    if (Loc.isInvalid())
+      return false;
     return isSystem(getFileCharacteristic(Loc));
   }
 
   /// Returns if a SourceLocation is in an "extern C" system header.
   bool isInExternCSystemHeader(SourceLocation Loc) const {
     return getFileCharacteristic(Loc) == SrcMgr::C_ExternCSystem;
   }
 

clang/unittests/Basic/SourceManagerTest.cpp

protected:
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
   LangOptions LangOpts;
   std::shared_ptr<TargetOptions> TargetOpts;
   IntrusiveRefCntPtr<TargetInfo> Target;
 };
 
+TEST_F(SourceManagerTest, isInMemoryBuffersNoSourceLocationInfo) {
+  // Check for invalid source location for each method
+  SourceLocation LocEmpty;
+  bool isWrittenInBuiltInFileFalse = SourceMgr.isWrittenInBuiltinFile(LocEmpty);
+  bool isWrittenInCommandLineFileFalse =
+      SourceMgr.isWrittenInCommandLineFile(LocEmpty);
+  bool isWrittenInScratchSpaceFalse =
+      SourceMgr.isWrittenInScratchSpace(LocEmpty);
+
+  EXPECT_FALSE(isWrittenInBuiltInFileFalse);
+  EXPECT_FALSE(isWrittenInCommandLineFileFalse);
+  EXPECT_FALSE(isWrittenInScratchSpaceFalse);
+
+  // Check for valid source location per filename for each method
+  const char *Source = "int x";
+
+  std::unique_ptr<llvm::MemoryBuffer> BuiltInBuf =
+      llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *BuiltInFile =
+      FileMgr.getVirtualFile("<built-in>", BuiltInBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(BuiltInFile, std::move(BuiltInBuf));
+  FileID BuiltInFileID =
+      SourceMgr.getOrCreateFileID(BuiltInFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(BuiltInFileID);
+  SourceLocation LocBuiltIn =
+      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
+  bool isWrittenInBuiltInFileTrue =
+      SourceMgr.isWrittenInBuiltinFile(LocBuiltIn);
+
+  std::unique_ptr<llvm::MemoryBuffer> CommandLineBuf =
+      llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *CommandLineFile = FileMgr.getVirtualFile(
+      "<command line>", CommandLineBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(CommandLineFile, std::move(CommandLineBuf));
+  FileID CommandLineFileID =
+      SourceMgr.getOrCreateFileID(CommandLineFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(CommandLineFileID);
+  SourceLocation LocCommandLine =
+      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
+  bool isWrittenInCommandLineFileTrue =
+      SourceMgr.isWrittenInCommandLineFile(LocCommandLine);
+
+  std::unique_ptr<llvm::MemoryBuffer> ScratchSpaceBuf =
+      llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *ScratchSpaceFile = FileMgr.getVirtualFile(
+      "<scratch space>", ScratchSpaceBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(ScratchSpaceFile, std::move(ScratchSpaceBuf));
+  FileID ScratchSpaceFileID =
+      SourceMgr.getOrCreateFileID(ScratchSpaceFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(ScratchSpaceFileID);
+  SourceLocation LocScratchSpace =
+      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
+  bool isWrittenInScratchSpaceTrue =
+      SourceMgr.isWrittenInScratchSpace(LocScratchSpace);
+
+  EXPECT_TRUE(isWrittenInBuiltInFileTrue);
+  EXPECT_TRUE(isWrittenInCommandLineFileTrue);
+  EXPECT_TRUE(isWrittenInScratchSpaceTrue);
+}
+
+TEST_F(SourceManagerTest, isInSystemHeader) {
+  // Check for invalid source location
+  SourceLocation LocEmpty;
+  bool isInSystemHeaderFalse = SourceMgr.isInSystemHeader(LocEmpty);
+  ASSERT_FALSE(isInSystemHeaderFalse);
+}
+
 TEST_F(SourceManagerTest, isBeforeInTranslationUnit) {
   const char *source =
     "#define M(x) [x]\n"
     "M(foo)";
   std::unique_ptr<llvm::MemoryBuffer> Buf =
       llvm::MemoryBuffer::getMemBuffer(source);
   FileID mainFileID = SourceMgr.createFileID(std::move(Buf));
   SourceMgr.setMainFileID(mainFileID);