This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tapaswenipathak on Oct 3 2020, 3:05 AM.

Details

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.

Diff Detail

Event Timeline

reikdas created this revision.Oct 3 2020, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2020, 3:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
reikdas requested review of this revision.Oct 3 2020, 3:05 AM
lebedev.ri resigned from this revision.Oct 3 2020, 3:16 AM
shafik added a comment.Oct 8 2020, 9:52 AM

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.

@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 ;)

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?

Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2022, 11:41 AM

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 <vvasilev@cern.ch>

reikdas resigned from this revision.May 31 2022, 9:47 AM

This looks reasonable to me...

@shafik, @teemperor what do you think?

v.g.vassilev accepted this revision.Jun 24 2022, 12:36 AM

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

This revision is now accepted and ready to land.Jun 24 2022, 12:36 AM