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

[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