[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-09 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits
dexonsmith added a comment. In https://reviews.llvm.org/D32724#750074, @vsk wrote: > In https://reviews.llvm.org/D32724#749868, @dexonsmith wrote: > > > In https://reviews.llvm.org/D32724#747728, @aprantl wrote: > > > > > Is it the right solution to use the module hash for correctness, or > > >

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-09 Thread Vedant Kumar via Phabricator via cfe-commits
vsk added a comment. In https://reviews.llvm.org/D32724#749868, @dexonsmith wrote: > In https://reviews.llvm.org/D32724#747728, @aprantl wrote: > > > Is it the right solution to use the module hash for correctness, or should > > the mismatch of the serialized langopts trigger a module rebuild an

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-09 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits
dexonsmith added a comment. In https://reviews.llvm.org/D32724#747728, @aprantl wrote: > Is it the right solution to use the module hash for correctness, or should > the mismatch of the serialized langopts trigger a module rebuild and the > module hash is only there to tune the performance/disk

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-08 Thread Sean Callanan via Phabricator via cfe-commits
spyffe added inline comments. Comment at: lib/Basic/LangOptions.cpp:32 - // FIXME: This should not be reset; modules can be different with different - // sanitizer options (this affects __has_feature(address_sanitizer) etc). - Sanitize.clear(); + // These options do not aff

Re: [PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-05 Thread Richard Smith via cfe-commits
On 1 May 2017 at 16:43, Vedant Kumar via Phabricator < revi...@reviews.llvm.org> wrote: > vsk created this revision. > > When building with implicit modules it's possible to hit a scenario > where modules are built without -fsanitize=address, and are subsequently > imported into CUs with -fsanitiz

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-05 Thread Vedant Kumar via Phabricator via cfe-commits
vsk added a comment. Offline, @aprantl mentioned a concern about module hashes being required for correctness. I'm not sure whether this is OK or not (@bruno, any thoughts?). AFAICT there are items included in the module hash that, were they removed, would break implicit module builds (e.g '-DF

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-05 Thread Adrian Prantl via Phabricator via cfe-commits
aprantl added a comment. Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff? https://reviews.llvm.org/D32724

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-05 Thread Sean Callanan via Phabricator via cfe-commits
spyffe requested changes to this revision. spyffe added a comment. This revision now requires changes to proceed. A few minor nits, but the operation of ignoring certain sanitizer flags seems to be happening in the wrong place. Comment at: lib/Basic/LangOptions.cpp:32 - //

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-05 Thread Vedant Kumar via Phabricator via cfe-commits
vsk updated this revision to Diff 98030. vsk edited the summary of this revision. vsk added a comment. - Exclude sanitizers which cannot affect AST generation from the module hash. - Improve the test to check modules are actually rebuilt when we expect them to be rebuilt, and not rebuilt otherwis

[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

2017-05-01 Thread Vedant Kumar via Phabricator via cfe-commits
vsk created this revision. When building with implicit modules it's possible to hit a scenario where modules are built without -fsanitize=address, and are subsequently imported into CUs with -fsanitize=address enabled. This can cause strange failures at runtime. One case I've seen affects libcxx,