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

[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