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

Unit TestsFailed

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
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'