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

[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