Page MenuHomePhabricator
Differential D95159

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

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

Details

Reviewers
jansvoboda11
bnbarham
Commits
rGb0e89906f5b7: [ASTReader] Allow controlling separately whether validation should be disabled…
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

Unit TestsFailed
View All

TimeTest
1,780 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c

Event Timeline

akyrtzi created this revision.Jan 21 2021, 11:46 AM
Herald added a reviewer: jansvoboda11. · View Herald TranscriptJan 21 2021, 11:46 AM
Herald added subscribers: dang, arphaman. · View Herald Transcript
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 added a reviewer: bnbarham.Jan 21 2021, 11:50 AM
Harbormaster completed remote builds in B86138: Diff 318274.Jan 21 2021, 1:22 PM
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'

Harbormaster completed remote builds in B86164: Diff 318314.Jan 21 2021, 5:09 PM
Harbormaster completed remote builds in B86179: Diff 318333.Jan 21 2021, 6:19 PM
Harbormaster completed remote builds in B86181: Diff 318335.Jan 21 2021, 6:44 PM
Closed by commit rGb0e89906f5b7: [ASTReader] Allow controlling separately whether validation should be disabled… (authored by akyrtzi). · Explain WhyJan 21 2021, 8:46 PM
This revision was automatically updated to reflect the committed changes.
akyrtzi added a commit: rGb0e89906f5b7: [ASTReader] Allow controlling separately whether validation should be disabled….

Revision Contents
Changeset List

PathSize
clang/
include/
clang/
Driver/
3 lines
Frontend/
12 lines
Lex/
26 lines
Serialization/
18 lines
lib/
Frontend/
5 lines
11 lines
15 lines
6 lines
2 lines
3 lines
Serialization/
47 lines
test/
Index/
Inputs/
preamble-reparse-changed-module/
3 lines
3 lines
3 lines
18 lines
tools/
c-index-test/
17 lines
25 lines

Diff 318333

View Options

clang/include/clang/Driver/Options.td

Loading...
View Options

clang/include/clang/Frontend/CompilerInstance.h

Loading...
View Options

clang/include/clang/Lex/PreprocessorOptions.h

Loading...
View Options

clang/include/clang/Serialization/ASTReader.h

Loading...
View Options

clang/lib/Frontend/ASTUnit.cpp

Loading...
View Options

clang/lib/Frontend/ChainedIncludesSource.cpp

Loading...
View Options

clang/lib/Frontend/CompilerInstance.cpp

Loading...
View Options

clang/lib/Frontend/FrontendAction.cpp

Loading...
View Options

clang/lib/Frontend/FrontendActions.cpp

Loading...
View Options

clang/lib/Frontend/PrecompiledPreamble.cpp

Loading...
View Options

clang/lib/Serialization/ASTReader.cpp

Loading...
View Options

clang/test/Index/Inputs/preamble-reparse-changed-module/head.h

Loading...
View Options

clang/test/Index/Inputs/preamble-reparse-changed-module/module.modulemap

Loading...
View Options

clang/test/Index/Inputs/preamble-reparse-changed-module/new-head.h

Loading...
View Options

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

Loading...
View Options

clang/tools/c-index-test/c-index-test.c

Loading...
View Options

clang/tools/c-index-test/core_main.cpp

Loading...