Page MenuHomePhabricator

[PCH] Serialize skipped preprocessor ranges
ClosedPublic

Authored by cameron314 on May 10 2016, 12:32 PM.

Details

Summary

This fixes, for example, libclang's clang_getSkippedRanges returning zero ranges after reparsing a translation unit.

Fixes PR34971.

Diff Detail

Repository
rC Clang

Event Timeline

cameron314 retitled this revision from to [PCH] Serialize skipped preprocessor ranges.
cameron314 updated this object.
cameron314 added a reviewer: rsmith.
cameron314 set the repository for this revision to rL LLVM.
cameron314 added a subscriber: cfe-commits.
cameron314 updated this object.May 10 2016, 12:39 PM
cameron314 updated this object.May 10 2016, 1:48 PM
erikjv edited edge metadata.Nov 3 2017, 2:10 AM

The patch has aged a bit since upload, so does it still apply? And, can you add a testcase please?

I'll rebase the patch and add a test. Thanks for looking at this!

nik added a subscriber: nik.Nov 15 2017, 2:10 AM

Fully rebased, with a test.

nik added a comment.Nov 16 2017, 2:39 AM

Here are my observations:

  • Your test case works fine for me even without having this change applied/build. Looks like this is already fixed in current trunk. Please confirm/test.
  • Can you come up with another test case that fixes something that is not yet addressed in trunk?
  • It does not fix https://bugs.llvm.org/show_bug.cgi?id=34971

Apart from that the patch applies cleanly and all tests pass (check-clang). However, some new warnings are emitted during building:

/home/nik/dev/llvm/trunk/source/tools/clang/include/clang/Lex/PreprocessingRecord.h: In constructor ‘clang::PreprocessingRecord::PreprocessingRecord(clang::SourceManager&)’:
/home/nik/dev/llvm/trunk/source/tools/clang/include/clang/Lex/PreprocessingRecord.h:339:40: warning: ‘clang::PreprocessingRecord::ExternalSource’ will be initialized after [-Wreorder]
	 ExternalPreprocessingRecordSource *ExternalSource;
										^~~~~~~~~~~~~~
/home/nik/dev/llvm/trunk/source/tools/clang/include/clang/Lex/PreprocessingRecord.h:312:10: warning:   ‘bool clang::PreprocessingRecord::SkippedRangesAllLoaded’ [-Wreorder]
	 bool SkippedRangesAllLoaded;
		  ^~~~~~~~~~~~~~~~~~~~~~
/home/nik/dev/llvm/trunk/source/tools/clang/lib/Lex/PreprocessingRecord.cpp:36:1: warning:   when initialized here [-Wreorder]
lib/Lex/PreprocessingRecord.cpp
349

Remove excess new line.

cameron314 marked an inline comment as done.Nov 16 2017, 12:14 PM
  • Well that's odd, because the test definitely fails for me without the patch. I'm only a few days behind the trunk.
  • I'm looking at your test case now. I can reproduce it even with the patch; I'm investigating what's happening.
  • I've fixed the warning, thanks! Using MSVC on my end which doesn't emit that particular warning.

Alright, with my patch the c-index-test *does* correctly serialize and restore the skipped ranges; the problem is that it searches for only ranges matching the target file. When there's a preamble, it's seen as a different file than the main file, even though they're really one and the same in this case.

So my test, which uses clang_getAllSkippedRanges, passes, but c-index-test, which uses clang_getSkippedRanges, fails. I'll update my test to use both.

I'm trying to figure out how to fix this.

Well, it seems like preamble PCH source location translation is fundamentally broken. The entry file has a single, positive file ID. The preamble PCH is treated as an imported module, so it has a negative file ID for the part that overlaps the preamble of the entry file. That means locations in the preamble part of the entry file can have two different file IDs depending on how they are arrived at.

I really don't know how to fix this. Any ideas?

ilya-biryukov edited edge metadata.Nov 17 2017, 7:11 AM

Well, it seems like preamble PCH source location translation is fundamentally broken. The entry file has a single, positive file ID. The preamble PCH is treated as an imported module, so it has a negative file ID for the part that overlaps the preamble of the entry file. That means locations in the preamble part of the entry file can have two different file IDs depending on how they are arrived at.

I really don't know how to fix this. Any ideas?

Why do we store raw source locations in PPSkippedRange? Would storing SourceLocation and using ASTWriter::AddSourceLocation and ASTReader:: ReadSourceLocation do the trick?

Why do we store raw source locations in PPSkippedRange? Would storing SourceLocation and using ASTWriter::AddSourceLocation and ASTReader:: ReadSourceLocation do the trick?

I followed the pattern used to store PPEntityOffsets; it allows the entire array to be written in one chunk. Using AddSourceLocation/ReadSourceLocation boils down to the same thing -- AddSourceLocation simply rotates by one bit, and ReadSourceLocation simply rotates back and calls ASTReader::TranslateSourceLocation (which I call from AST::ReadSkippedRange explicitly instead). So the source location translation path is exactly the same either way.

The thing is, the PCH created for the preamble is imported as a module, meaning the part of the source code that overlaps the preamble of the entry file has a different file ID from the entry file itself. Is there any way to force source locations in the preamble to map to the actual entry file instead of the module's version of the entry file?

Maybe something like this works:

--- a/tools/libclang/CIndex.cpp
+++ b/tools/libclang/CIndex.cpp
@@ -8090,6 +8090,7 @@ CXSourceRangeList *clang_getSkippedRanges(CXTranslationUnit TU, CXFile file) {
   SourceManager &sm = Ctx.getSourceManager();
   FileEntry *fileEntry = static_cast<FileEntry *>(file);
   FileID wantedFileID = sm.translateFile(fileEntry);
+  bool isMainFile = wantedFileID == sm.getMainFileID();
 
   const std::vector<SourceRange> &SkippedRanges = ppRec->getSkippedRanges();
   std::vector<SourceRange> wantedRanges;
@@ -8097,6 +8098,8 @@ CXSourceRangeList *clang_getSkippedRanges(CXTranslationUnit TU, CXFile file) {
        i != ei; ++i) {
     if (sm.getFileID(i->getBegin()) == wantedFileID || sm.getFileID(i->getEnd()) == wantedFileID)
       wantedRanges.push_back(*i);
+    else if (isMainFile && (astUnit->isInPreambleFileID(i->getBegin()) || astUnit->isInPreambleFileID(i->getEnd())))
+      wantedRanges.push_back(*i);
   }
 
   skipped->count = wantedRanges.size();

Brilliant, didn't know isInPreambleFileID existed. All my tests pass for me now with that change, thanks :-)
I'll update the patch on Monday.

cameron314 updated this revision to Diff 125333.Dec 4 2017, 6:54 AM

Here's the final patch that fixes clang_getSkippedRegions with regions in the preamble (as well as serializing the skipped regions in the PCH file).

nik added a comment.Dec 6 2017, 5:14 AM

Here's the final patch that fixes clang_getSkippedRegions with regions in the preamble (as well as serializing the skipped regions in the PCH file).

Works fine for me, thanks! Test from the bug report is fixed by this one.

You should probably add "Fixes PR34971." to the summary.

@erikjv @ilya-biryukov - please review as you know the details here :)

cameron314 edited the summary of this revision. (Show Details)Dec 6 2017, 9:35 AM

Ping?
The patch is ready to go, just needs a final approval...

lgtm. Ilya?

ilya-biryukov accepted this revision.Jan 12 2018, 6:39 AM

LGTM, sorry for the delay.

This revision is now accepted and ready to land.Jan 12 2018, 6:39 AM

Excellent, I'll rebase and commit. Thanks everyone for your patience!

This revision was automatically updated to reflect the committed changes.