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

Reply via email to