[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 
> > > 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?
> >
> >
> > I'm not sure if there is (or should be) a hard-and-fast rule, but certainly 
> > in this case changing the hash SGTM.  Otherwise, users toggling back and 
> > forth between build configurations would have to rebuild the modules each 
> > time.
>
>
> Great, it looks like changing the module hash is acceptable. However, I'm not 
> sure whether it's OK to make sanitizer feature mismatches errors for explicit 
> modules. @aprantl suggests checking for language option mismatches early on 
> instead of just relying on the module hash, but @rsmith mentioned:
>
> > I would expect this [sanitizer features] to be permitted to differ between 
> > an explicit module build and its use. (Ideally we would apply the 
> > sanitization settings from the module build to the code generated for its 
> > inline functions in that case, but that can wait.)


Ah, interesting.  That does seem ideal.

> Should we diagnose sanitizer feature mismatches in ASTReader 
> (checkLanguageOptions) as warnings, errors, or not at all?

Either warning or error seems fine with me; only compiler engineers are going 
to see this anyway (because implicit users are protected by the hash).  As long 
as it's overridable... there's a -cc1 flag to allow configuration mismatches, 
right?

Once the flag can be changed after the fact (i.e., in a post-Richard's-plan 
world), we should remove the error/warning, and possibly do something like: 
always build the module without sanitizers.


https://reviews.llvm.org/D32724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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 and the 
> > module hash is only there to tune the performance/disk size tradeoff?
>
>
> I'm not sure if there is (or should be) a hard-and-fast rule, but certainly 
> in this case changing the hash SGTM.  Otherwise, users toggling back and 
> forth between build configurations would have to rebuild the modules each 
> time.


Great, it looks like changing the module hash is acceptable. However, I'm not 
sure whether it's OK to make sanitizer feature mismatches errors for explicit 
modules. @aprantl suggests checking for language option mismatches early on 
instead of just relying on the module hash, but @rsmith mentioned:

> I would expect this [sanitizer features] to be permitted to differ between an 
> explicit module build and its use. (Ideally we would apply the sanitization 
> settings from the module build to the code generated for its inline functions 
> in that case, but that can wait.)

Should we diagnose sanitizer feature mismatches in ASTReader 
(checkLanguageOptions) as warnings, errors, or not at all?


https://reviews.llvm.org/D32724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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 size tradeoff?


I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in 
this case changing the hash SGTM.  Otherwise, users toggling back and forth 
between build configurations would have to rebuild the modules each time.


https://reviews.llvm.org/D32724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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 affect AST generation.
   SanitizerBlacklistFiles.clear();

vsk wrote:
> spyffe wrote:
> > I'd replace this with
> > ```
> > Sanitize.clear(SanitizerKind::CFI | SanitizerKind::Integer | 
> > SanitizerKind::Nullability | SanitizerKind::Undefined);
> > ```
> > We know those options don't affect modules, as demonstrated by you clearing 
> > them anyway in CompilerInvocation...
> I don't think this would work, and don't quite see a way to make it work. The 
> problem is that when importing a module into a CU, the CU's hash is required 
> to match the to-be-imported module's hash [1]. If we clear some sanitizer 
> options in resetNonModularOptions(), then the "matching hashes" check would 
> break, because you can't reset the non-modular options in a CU that you're 
> importing a module into. You'd end up disabling the sanitizers for the CU 
> you're building.
> 
> [1] CompilerInstance.cpp
> ```
> 1095   assert(ImportingInstance.getInvocation().getModuleHash() ==
>   
>   
>   
>   
> 1096  Invocation->getModuleHash() && "Module hash mismatch!"); 
> ```
Okay, I probably just didn't understand the role of `resetNonModularOptions()`.


https://reviews.llvm.org/D32724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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 -fsanitize=address enabled. This can cause
> strange failures at runtime. One case I've seen affects libcxx, since
> its vector implementation behaves differently when ASan is enabled.
>
> Implicit module builds should work even when -fsanitize=X is toggled on
> and off across multiple compiler invocations. This patch implements a
> fix by including the set of enabled sanitizers in the module hash.
>
> I am not sure if the module hash should change when building with
> explicit modules, and would appreciate any input on this.
>

I would expect this to be permitted to differ between an explicit module
build and its use. (Ideally we would apply the sanitization settings from
the module build to the code generated for its inline functions in that
case, but that can wait.)


> https://reviews.llvm.org/D32724
>
> Files:
>   lib/Basic/LangOptions.cpp
>   lib/Frontend/CompilerInvocation.cpp
>   test/Modules/Inputs/check-for-sanitizer-feature/check.h
>   test/Modules/Inputs/check-for-sanitizer-feature/map
>   test/Modules/check-for-sanitizer-feature.cpp
>
>
> Index: test/Modules/check-for-sanitizer-feature.cpp
> ===
> --- /dev/null
> +++ test/Modules/check-for-sanitizer-feature.cpp
> @@ -0,0 +1,32 @@
> +// RUN: rm -rf %t
> +// RUN: mkdir %t
> +
> +// Build and use an ASan-enabled module.
> +// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \
> +// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
> +// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
> +
> +// Force a module rebuild by disabling ASan.
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t \
> +// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
> +// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
> +
> +// Force another module rebuild by enabling ASan again.
> +// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \
> +// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
> +// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
> +
> +#include "check.h"
> +
> +#if __has_feature(address_sanitizer)
> +#if HAS_ASAN != 1
> +#error Does not have the address_sanitizer feature, but should.
> +#endif
> +#else
> +#if HAS_ASAN != 0
> +#error Has the address_sanitizer feature, but shouldn't.
> +#endif
> +
> +#endif
> +
> +// expected-no-diagnostics
> Index: test/Modules/Inputs/check-for-sanitizer-feature/map
> ===
> --- /dev/null
> +++ test/Modules/Inputs/check-for-sanitizer-feature/map
> @@ -0,0 +1,3 @@
> +module check_feature {
> +  header "check.h"
> +}
> Index: test/Modules/Inputs/check-for-sanitizer-feature/check.h
> ===
> --- /dev/null
> +++ test/Modules/Inputs/check-for-sanitizer-feature/check.h
> @@ -0,0 +1,5 @@
> +#if __has_feature(address_sanitizer)
> +#define HAS_ASAN 1
> +#else
> +#define HAS_ASAN 0
> +#endif
> Index: lib/Frontend/CompilerInvocation.cpp
> ===
> --- lib/Frontend/CompilerInvocation.cpp
> +++ lib/Frontend/CompilerInvocation.cpp
> @@ -2693,6 +2693,11 @@
>  code = ext->hashExtension(code);
>}
>
> +  // Extend the signature with the enabled sanitizers, if at least one is
> +  // enabled.
> +  if (!LangOpts->Sanitize.empty())
> +code = hash_combine(code, LangOpts->Sanitize.Mask);
> +
>return llvm::APInt(64, code).toString(36, /*Signed=*/false);
>  }
>
> Index: lib/Basic/LangOptions.cpp
> ===
> --- lib/Basic/LangOptions.cpp
> +++ lib/Basic/LangOptions.cpp
> @@ -29,9 +29,11 @@
>Name = Default;
>  #include "clang/Basic/LangOptions.def"
>
> -  // FIXME: This should not be reset; modules can be different with
> different
> -  // sanitizer options (this affects __has_feature(address_sanitizer)
> etc).
> -  Sanitize.clear();
> +  // FIXME: Some sanitizers do not expose any information via the
> preprocessor
> +  // (e.g UBSan). We may be able to avoid some module rebuilds by
> disabling
> +  // those sanitizers.
> +
> +  // These options do not affect AST generation.
>SanitizerBlacklistFiles.clear();
>XRayAlwaysInstrumentFiles.clear();
>XRayNeverInstrumentFiles.clear();
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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 '-DFOO'-style preprocessor defs).




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 affect AST generation.
   SanitizerBlacklistFiles.clear();

spyffe wrote:
> I'd replace this with
> ```
> Sanitize.clear(SanitizerKind::CFI | SanitizerKind::Integer | 
> SanitizerKind::Nullability | SanitizerKind::Undefined);
> ```
> We know those options don't affect modules, as demonstrated by you clearing 
> them anyway in CompilerInvocation...
I don't think this would work, and don't quite see a way to make it work. The 
problem is that when importing a module into a CU, the CU's hash is required to 
match the to-be-imported module's hash [1]. If we clear some sanitizer options 
in resetNonModularOptions(), then the "matching hashes" check would break, 
because you can't reset the non-modular options in a CU that you're importing a 
module into. You'd end up disabling the sanitizers for the CU you're building.

[1] CompilerInstance.cpp
```
1095   assert(ImportingInstance.getInvocation().getModuleHash() ==  


  
1096  Invocation->getModuleHash() && "Module hash mismatch!"); 
```



Comment at: test/Modules/check-for-sanitizer-feature.cpp:25
+//
+// First, built without any sanitization.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.2 \

spyffe wrote:
> Typo: //build//
Will fix, pending resolution of the issue you raised above. Same for your next 
comment about the #error messages.


https://reviews.llvm.org/D32724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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
 
-  // 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 affect AST generation.
   SanitizerBlacklistFiles.clear();

I'd replace this with
```
Sanitize.clear(SanitizerKind::CFI | SanitizerKind::Integer | 
SanitizerKind::Nullability | SanitizerKind::Undefined);
```
We know those options don't affect modules, as demonstrated by you clearing 
them anyway in CompilerInvocation...



Comment at: lib/Frontend/CompilerInvocation.cpp:2699
+  SanitizerSet SanHash = LangOpts->Sanitize;
+  SanHash.clear(SanitizerKind::CFI | SanitizerKind::Integer |
+SanitizerKind::Nullability | SanitizerKind::Undefined);

If `clearNonModularOptions()` does the right thing, you may not need to clear 
these flags here



Comment at: test/Modules/check-for-sanitizer-feature.cpp:25
+//
+// First, built without any sanitization.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.2 \

Typo: //build//



Comment at: test/Modules/check-for-sanitizer-feature.cpp:42
+#if HAS_ASAN != 1
+#error Does not have the address_sanitizer feature, but should.
+#endif

This error isn't very illuminating: I'd say
> Module doesn't have the address_sanitizer feature, but main program does.
Same below.




https://reviews.llvm.org/D32724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[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 otherwise.


https://reviews.llvm.org/D32724

Files:
  include/clang/Basic/Sanitizers.h
  lib/Basic/LangOptions.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Modules/Inputs/check-for-sanitizer-feature/check.h
  test/Modules/Inputs/check-for-sanitizer-feature/map
  test/Modules/check-for-sanitizer-feature.cpp

Index: test/Modules/check-for-sanitizer-feature.cpp
===
--- /dev/null
+++ test/Modules/check-for-sanitizer-feature.cpp
@@ -0,0 +1,50 @@
+// RUN: rm -rf %t.1 %t.2
+// RUN: mkdir %t.1 %t.2
+
+// Build and use an ASan-enabled module.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t.1 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.1 | count 2
+
+// Force a module rebuild by disabling ASan.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.1 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.1 | count 3
+
+// Enable ASan again: check that there is no import failure, and no rebuild.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t.1 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.1 | count 3
+
+// Some sanitizers can not affect AST generation when enabled. Check that
+// module rebuilds don't occur when these sanitizers are enabled.
+//
+// First, built without any sanitization.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.2 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.2 | count 2
+//
+// Next, build with sanitization, and check that a new module isn't built.
+// RUN: %clang_cc1 -fsanitize=cfi-vcall,unsigned-integer-overflow,nullability-arg,null -fmodules \
+// RUN:   -fmodules-cache-path=%t.2 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.2 | count 2
+
+#include "check.h"
+
+#if __has_feature(address_sanitizer)
+#if HAS_ASAN != 1
+#error Does not have the address_sanitizer feature, but should.
+#endif
+#else
+#if HAS_ASAN != 0
+#error Has the address_sanitizer feature, but shouldn't.
+#endif
+#endif
+
+// expected-no-diagnostics
Index: test/Modules/Inputs/check-for-sanitizer-feature/map
===
--- /dev/null
+++ test/Modules/Inputs/check-for-sanitizer-feature/map
@@ -0,0 +1,3 @@
+module check_feature {
+  header "check.h"
+}
Index: test/Modules/Inputs/check-for-sanitizer-feature/check.h
===
--- /dev/null
+++ test/Modules/Inputs/check-for-sanitizer-feature/check.h
@@ -0,0 +1,5 @@
+#if __has_feature(address_sanitizer)
+#define HAS_ASAN 1
+#else
+#define HAS_ASAN 0
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2693,6 +2693,14 @@
 code = ext->hashExtension(code);
   }
 
+  // Extend the signature with the enabled sanitizers, if at least one is
+  // enabled. Sanitizers which cannot affect AST generation aren't hashed.
+  SanitizerSet SanHash = LangOpts->Sanitize;
+  SanHash.clear(SanitizerKind::CFI | SanitizerKind::Integer |
+SanitizerKind::Nullability | SanitizerKind::Undefined);
+  if (!SanHash.empty())
+code = hash_combine(code, SanHash.Mask);
+
   return llvm::APInt(64, code).toString(36, /*Signed=*/false);
 }
 
Index: lib/Basic/LangOptions.cpp
===
--- lib/Basic/LangOptions.cpp
+++ lib/Basic/LangOptions.cpp
@@ -29,9 +29,7 @@
   Name = Default;
 #include "clang/Basic/LangOptions.def"
 
-  // 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 affect AST generation.
   SanitizerBlacklistFiles.clear();
   XRayAlwaysInstrumentFiles.clear();
   XRayNeverInstrumentFiles.clear();
Index: include/clang/Basic/Sanitizers.h
===
--- include/clang/Basic/Sanitizers.h
+++ include/clang/Basic/Sanitizers.h
@@ -61,8 +61,9 @@
 Mask = Value ? (Mask | K) : (Mask & ~K);
   }
 
-  /// \brief Disable all sanitizers.
-  void 

[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, since
its vector implementation behaves differently when ASan is enabled.

Implicit module builds should work even when -fsanitize=X is toggled on
and off across multiple compiler invocations. This patch implements a
fix by including the set of enabled sanitizers in the module hash.

I am not sure if the module hash should change when building with
explicit modules, and would appreciate any input on this.


https://reviews.llvm.org/D32724

Files:
  lib/Basic/LangOptions.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Modules/Inputs/check-for-sanitizer-feature/check.h
  test/Modules/Inputs/check-for-sanitizer-feature/map
  test/Modules/check-for-sanitizer-feature.cpp


Index: test/Modules/check-for-sanitizer-feature.cpp
===
--- /dev/null
+++ test/Modules/check-for-sanitizer-feature.cpp
@@ -0,0 +1,32 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// Build and use an ASan-enabled module.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+
+// Force a module rebuild by disabling ASan.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+
+// Force another module rebuild by enabling ASan again.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+
+#include "check.h"
+
+#if __has_feature(address_sanitizer)
+#if HAS_ASAN != 1
+#error Does not have the address_sanitizer feature, but should.
+#endif
+#else
+#if HAS_ASAN != 0
+#error Has the address_sanitizer feature, but shouldn't.
+#endif
+
+#endif
+
+// expected-no-diagnostics
Index: test/Modules/Inputs/check-for-sanitizer-feature/map
===
--- /dev/null
+++ test/Modules/Inputs/check-for-sanitizer-feature/map
@@ -0,0 +1,3 @@
+module check_feature {
+  header "check.h"
+}
Index: test/Modules/Inputs/check-for-sanitizer-feature/check.h
===
--- /dev/null
+++ test/Modules/Inputs/check-for-sanitizer-feature/check.h
@@ -0,0 +1,5 @@
+#if __has_feature(address_sanitizer)
+#define HAS_ASAN 1
+#else
+#define HAS_ASAN 0
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2693,6 +2693,11 @@
 code = ext->hashExtension(code);
   }
 
+  // Extend the signature with the enabled sanitizers, if at least one is
+  // enabled.
+  if (!LangOpts->Sanitize.empty())
+code = hash_combine(code, LangOpts->Sanitize.Mask);
+
   return llvm::APInt(64, code).toString(36, /*Signed=*/false);
 }
 
Index: lib/Basic/LangOptions.cpp
===
--- lib/Basic/LangOptions.cpp
+++ lib/Basic/LangOptions.cpp
@@ -29,9 +29,11 @@
   Name = Default;
 #include "clang/Basic/LangOptions.def"
 
-  // FIXME: This should not be reset; modules can be different with different
-  // sanitizer options (this affects __has_feature(address_sanitizer) etc).
-  Sanitize.clear();
+  // FIXME: Some sanitizers do not expose any information via the preprocessor
+  // (e.g UBSan). We may be able to avoid some module rebuilds by disabling
+  // those sanitizers.
+
+  // These options do not affect AST generation.
   SanitizerBlacklistFiles.clear();
   XRayAlwaysInstrumentFiles.clear();
   XRayNeverInstrumentFiles.clear();


Index: test/Modules/check-for-sanitizer-feature.cpp
===
--- /dev/null
+++ test/Modules/check-for-sanitizer-feature.cpp
@@ -0,0 +1,32 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// Build and use an ASan-enabled module.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+
+// Force a module rebuild by disabling ASan.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+
+// Force another module rebuild by enabling ASan again.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \
+// RUN: