[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe881d85371bf: Allow interfaces to operate on in-memory
buffers with no source location info. (authored by tapaswenipathak, committed
by v.g.vassilev).
Changed prior to commit:
https://reviews.llvm.org/D88780?vs=432746=440061#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88780/new/
https://reviews.llvm.org/D88780
Files:
clang/include/clang/Basic/SourceManager.h
clang/unittests/Basic/SourceManagerTest.cpp
Index: clang/unittests/Basic/SourceManagerTest.cpp
===
--- clang/unittests/Basic/SourceManagerTest.cpp
+++ clang/unittests/Basic/SourceManagerTest.cpp
@@ -51,6 +51,73 @@
IntrusiveRefCntPtr 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 BuiltInBuf =
+ llvm::MemoryBuffer::getMemBuffer(Source);
+ const FileEntry *BuiltInFile =
+ FileMgr.getVirtualFile("", 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 CommandLineBuf =
+ llvm::MemoryBuffer::getMemBuffer(Source);
+ const FileEntry *CommandLineFile = FileMgr.getVirtualFile(
+ "", 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 ScratchSpaceBuf =
+ llvm::MemoryBuffer::getMemBuffer(Source);
+ const FileEntry *ScratchSpaceFile = FileMgr.getVirtualFile(
+ "", 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"
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1473,24 +1473,35 @@
/// Returns whether \p Loc is located in a 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("");
}
/// Returns whether \p Loc is located in a 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("");
}
/// Returns whether \p Loc is located in a file.
bool isWrittenInScratchSpace(SourceLocation Loc) const {
-StringRef Filename(getPresumedLoc(Loc).getFilename());
+PresumedLoc Presumed = getPresumedLoc(Loc);
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
v.g.vassilev accepted this revision. v.g.vassilev added a comment. This revision is now accepted and ready to land. I'd propose to move forward here and rely on eventual post-commit review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88780/new/ https://reviews.llvm.org/D88780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
v.g.vassilev added a comment. This looks reasonable to me... @shafik, @teemperor what do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88780/new/ https://reviews.llvm.org/D88780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
tapaswenipathak updated this revision to Diff 432746.
tapaswenipathak added a comment.
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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88780/new/
https://reviews.llvm.org/D88780
Files:
clang/include/clang/Basic/SourceManager.h
clang/unittests/Basic/SourceManagerTest.cpp
Index: clang/unittests/Basic/SourceManagerTest.cpp
===
--- clang/unittests/Basic/SourceManagerTest.cpp
+++ clang/unittests/Basic/SourceManagerTest.cpp
@@ -51,6 +51,73 @@
IntrusiveRefCntPtr 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 BuiltInBuf =
+ llvm::MemoryBuffer::getMemBuffer(Source);
+ const FileEntry *BuiltInFile =
+ FileMgr.getVirtualFile("", 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 CommandLineBuf =
+ llvm::MemoryBuffer::getMemBuffer(Source);
+ const FileEntry *CommandLineFile = FileMgr.getVirtualFile(
+ "", 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 ScratchSpaceBuf =
+ llvm::MemoryBuffer::getMemBuffer(Source);
+ const FileEntry *ScratchSpaceFile = FileMgr.getVirtualFile(
+ "", 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"
@@ -84,11 +151,11 @@
ASSERT_EQ(tok::l_square, toks[0].getKind());
ASSERT_EQ(tok::identifier, toks[1].getKind());
ASSERT_EQ(tok::r_square, toks[2].getKind());
-
+
SourceLocation lsqrLoc = toks[0].getLocation();
SourceLocation idLoc = toks[1].getLocation();
SourceLocation rsqrLoc = toks[2].getLocation();
-
+
SourceLocation macroExpStartLoc = SourceMgr.translateLineCol(mainFileID, 2, 1);
SourceLocation macroExpEndLoc = SourceMgr.translateLineCol(mainFileID, 2, 6);
ASSERT_TRUE(macroExpStartLoc.isFileID());
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1470,24 +1470,35 @@
/// Returns whether \p Loc is located in a file.
bool isWrittenInBuiltinFile(SourceLocation Loc) const {
-StringRef Filename(getPresumedLoc(Loc).getFilename());
+PresumedLoc Presumed = getPresumedLoc(Loc);
+
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
tapaswenipathak commandeered this revision. tapaswenipathak added a reviewer: reikdas. tapaswenipathak added a comment. Herald added a project: All. Ref: https://reviews.llvm.org/D126600. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88780/new/ https://reviews.llvm.org/D88780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
teemperor added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88780/new/ https://reviews.llvm.org/D88780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
v.g.vassilev added a comment. @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 ;) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88780/new/ https://reviews.llvm.org/D88780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
shafik added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88780/new/ https://reviews.llvm.org/D88780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.
reikdas created this revision.
reikdas added reviewers: rsmith, lebedev.ri, shafik, v.g.vassilev.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
reikdas requested review of this revision.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D88780
Files:
clang/include/clang/Basic/SourceManager.h
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1440,19 +1440,28 @@
/// Returns whether \p Loc is located in a 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("");
}
/// Returns whether \p Loc is located in a 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("");
}
/// Returns whether \p Loc is located in a 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("");
}
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1440,19 +1440,28 @@
/// Returns whether \p Loc is located in a 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("");
}
/// Returns whether \p Loc is located in a 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("");
}
/// Returns whether \p Loc is located in a 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("");
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
