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