Page MenuHomePhabricator

[ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file
ClosedPublic

Authored by akyrtzi on Jan 21 2021, 11:46 AM.

Details

Summary

This addresses an issue with how the PCH preable works, specifically:

  1. When using a PCH/preamble the module hash changes and a different cache directory is used
  2. When the preamble is used, PCH & PCM validation is disabled.

Due to combination of #1 and #2, reparsing with preamble enabled can end up loading a stale module file before a header change and using it without updating it because validation is disabled and it doesn’t check that the header has changed and the module file is out-of-date.

rdar://72611253

Diff Detail

Event Timeline

akyrtzi created this revision.Jan 21 2021, 11:46 AM
akyrtzi requested review of this revision.Jan 21 2021, 11:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 11:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
akyrtzi updated this revision to Diff 318314.Jan 21 2021, 2:06 PM

clang-format changes

bnbarham accepted this revision.Jan 21 2021, 2:08 PM

LGTM, just the one minor comment

clang/lib/Serialization/ASTReader.cpp
2221–2222

There's a getValueOr method that you could use instead (ie. CurrentDeserializingModuleKind.getValueOr(M.Kind)).

clang/test/Index/preamble-reparse-changed-module.m
9

For some reason I was expecting the trial's to be 1 based, not 0. I'm not sure why though.

This revision is now accepted and ready to land.Jan 21 2021, 2:08 PM
akyrtzi updated this revision to Diff 318333.Jan 21 2021, 3:36 PM

Use getValueOr

akyrtzi updated this revision to Diff 318335.Jan 21 2021, 3:37 PM
akyrtzi edited the summary of this revision. (Show Details)

Fix typo in commit message, 'state' -> 'stale'

The newly added test case clang/test/Index/preamble-reparse-changed-module.m is failing on the patch that I'm trying to commit, https://reviews.llvm.org/D93769

Actually the result of the test is a bit flaky, when I build the Debug version on Linux, running ninja check-clang does show the test fail, but when I do an identical build and in addition enabling llvm assertions, llvm-lit does not show this lit test failing.

I think the problem has something to do with the macro FLT_EVAL_METHOD. The patch now treats this macro specially, more like LINE a macro that can vary depending on where it is seen in the source code.

In my patch, FLT_EVAL_MACRO is not expanded in -E mode, it just appears as its name in the preprocessed output. The reason is, the actual value of the macro depends on the floating point settings at a particular source code location (depends on #pragma float_control settings). The pragma are determined in Sema, and -E runs before Sema so the actual value isn't available.

This macro is the only reason i can think why this test is failing.

Can you help me understand what to do about this test case/about my patch/ so that i can get my patch committed? I don't understand what this test is doing. In the debugger it hits a failed assertion, looks like index out-of-range. here's a little massaged traceback
__assert_fail
llvm::SmallVectorTemplateCommon unsigned long, void ::indexing operator
clang::ASTReader::ReadString
clang::ASTReader::ParseLanguageOptions
clang::ASTReader::ReadOptionsBlock
clang::ASTReader::ReadControlBlock
clang::ASTReader::ReadASTCore
clang::ASTReader::ReadAST
clang::CompilerInstance::findOrCompileModuleAndReadAST
clang::CompilerInstance::loadModule
clang::Preprocessor::LexAfterModuleImport

Thanks a lot

@aaron.ballman suggested I may have made a mistake adding the new option, I'll look there.

More info about the test failure I'm seeing for clang/test/Index/preamble-reparse-changed-module.m

It appears it might be a non-deterministic failure, perhaps some test dependencies are missing?

Yesterday before I pushed my clang patch, I saw it fail when I used "ninja check-clang". I quickly executed each step used by this test (llvm-lit -v ; then save all the RUN commands to a file ; then "source" the file -- nothing was printed to stdout and also the return code was 0). I thought the fail report wasn't real at this point and I pushed the patch. This was a Debug build.

Then I saw a bot failure, @MaskRay pinged my clang patch and said my patch had caused this test to fail. He told me there was an assertion fail.

Then I created a new build area, configured with Debug and "enable llvm assertions", and "ninja check-clang". This time 0 tests reported as fail.

Then I went back into the build-debug build where "ninja check-clang" had reported the fail and used gdb to debug. In the course of debugging gdb told me that the build was out-of-date w.r.t. the source file, so I exitted gdb, "ninja clean" "ninja check-clang". But now no tests are failing. I don't know why gdb told me the build was out of date, I believe I did not change any of the files but I'm not 100% certain.

I removed the patch and created another build area, and rebuilt with Debug and asserts, everything is clean there too.

I captured the stdout from executing c-index-test, there is a difference between patched and unpatched. The FLT_EVAL_METHOD line is not present in the patched, this is expected.

Can you suggest how to find the problem? Thanks a lot.

Can you suggest how to find the problem? Thanks a lot.

@mibintc the test is reusing the module cache directory and that's causing some issues between runs, sorry about that. You could add a rm -rf %t or rm -rf %t/mcp to this test to fix that. Looks like the same issue was run into in https://reviews.llvm.org/D96816#2572589. I'd prefer clang/test/Index/preamble-reparse-changed-module.m add the rm and another test added for the LLVM_APPEND_VC_REV=NO if we want that.

@bnbarham
I think you mean a patch like this, is it right? I'd like to fix the test in a pre-commit and then re-push my patch

 git diff
diff --git a/clang/test/Index/preamble-reparse-changed-module.m b/clang/test/Index/preamble-reparse-changed-module.m
index 1c63e802ce0c..fc336c6e0670 100644
--- a/clang/test/Index/preamble-reparse-changed-module.m
+++ b/clang/test/Index/preamble-reparse-changed-module.m
@@ -1,6 +1,7 @@
 // REQUIRES: shell

-// RUN: mkdir -p %t/mod
+// RUN: rm -rf %t
+// RUN: mkdir %t/mod
 // RUN: touch %t/empty.h
 // RUN: cp %S/Inputs/preamble-reparse-changed-module/module.modulemap %t/mod
 // RUN: cp %S/Inputs/preamble-reparse-changed-module/head.h %t/mod

Whoops, shouldn't mess with the mkdir line

diff --git a/clang/test/Index/preamble-reparse-changed-module.m b/clang/test/Index/preamble-reparse-changed-module.m
index 1c63e802ce0c..349ed0db27d0 100644

  • a/clang/test/Index/preamble-reparse-changed-module.m

+++ b/clang/test/Index/preamble-reparse-changed-module.m
@@ -1,5 +1,6 @@
// REQUIRES: shell

+ RUN: rm -rf %t
RUN: mkdir -p %t/mod
RUN: touch %t/empty.h
RUN: cp %S/Inputs/preamble-reparse-changed-module/module.modulemap %t/mod

Yep, that would be perfect :). Thanks @mibintc