[Modules] Handle sanitizer feature mismatches when importing modules

[vsk]

Summary

This patch makes it an error to have a mismatch between the enabled
sanitizers in a CU, and in any module being imported into the CU. Only
mismatches between non-modular sanitizers are treated as errors.

This patch also includes non-modular sanitizers in module hashes, in
order to ensure module rebuilds occur when -fsanitize=X is toggled on
and off for non-modular sanitizers, and to cut down on module rebuilds
when the option is toggled for modular sanitizers.

This fixes a longstanding issue with implicit modules and sanitizers,
which Duncan originally diagnosed.

When building with implicit modules it's possible to hit a scenario
where modules are built without -fsanitize=address, and are subsequently
imported into CUs with -fsanitize=address enabled. This causes strange
failures at runtime. The case Duncan found affects libcxx, since its
vector implementation behaves differently when ASan is enabled.

Implicit module builds should "just work" when -fsanitize=X is toggled
on and off across multiple compiler invocations, which is what this
patch does.

[vsk]

• Exclude sanitizers which cannot affect AST generation from the module hash.
• Improve the test to check modules are actually rebuilt when we expect them to be rebuilt, and not rebuilt otherwise.

[spyffe]

A few minor nits, but the operation of ignoring certain sanitizer flags seems to be happening in the wrong place.

lib/Basic/LangOptions.cpp

32

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...

lib/Frontend/CompilerInvocation.cpp

2699

If clearNonModularOptions() does the right thing, you may not need to clear these flags here

test/Modules/check-for-sanitizer-feature.cpp

25

Typo: build

42

This error isn't very illuminating: I'd say

Module doesn't have the address_sanitizer feature, but main program does.

Same below.

[aprantl]

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

[vsk]

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).

lib/Basic/LangOptions.cpp

32

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!");

test/Modules/check-for-sanitizer-feature.cpp

25

Will fix, pending resolution of the issue you raised above. Same for your next comment about the #error messages.

[spyffe]

lib/Basic/LangOptions.cpp

32

Okay, I probably just didn't understand the role of resetNonModularOptions().

[dexonsmith]

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in this case changing the hash SGTM. Otherwise, users toggling back and forth between build configurations would have to rebuild the modules each time.

[vsk]

In D32724#749868, @dexonsmith wrote:

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in this case changing the hash SGTM. Otherwise, users toggling back and forth between build configurations would have to rebuild the modules each time.

Great, it looks like changing the module hash is acceptable. However, I'm not sure whether it's OK to make sanitizer feature mismatches errors for explicit modules. @aprantl suggests checking for language option mismatches early on instead of just relying on the module hash, but @rsmith mentioned:

I would expect this [sanitizer features] to be permitted to differ between an explicit module build and its use. (Ideally we would apply the sanitization settings from the module build to the code generated for its inline functions in that case, but that can wait.)

Should we diagnose sanitizer feature mismatches in ASTReader (checkLanguageOptions) as warnings, errors, or not at all?

[dexonsmith]

In D32724#750074, @vsk wrote:

In D32724#749868, @dexonsmith wrote:

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in this case changing the hash SGTM. Otherwise, users toggling back and forth between build configurations would have to rebuild the modules each time.

Great, it looks like changing the module hash is acceptable. However, I'm not sure whether it's OK to make sanitizer feature mismatches errors for explicit modules. @aprantl suggests checking for language option mismatches early on instead of just relying on the module hash, but @rsmith mentioned:

I would expect this [sanitizer features] to be permitted to differ between an explicit module build and its use. (Ideally we would apply the sanitization settings from the module build to the code generated for its inline functions in that case, but that can wait.)

Ah, interesting. That does seem ideal.

Should we diagnose sanitizer feature mismatches in ASTReader (checkLanguageOptions) as warnings, errors, or not at all?

Either warning or error seems fine with me; only compiler engineers are going to see this anyway (because implicit users are protected by the hash). As long as it's overridable... there's a -cc1 flag to allow configuration mismatches, right?

Once the flag can be changed after the fact (i.e., in a post-Richard's-plan world), we should remove the error/warning, and possibly do something like: always build the module without sanitizers.

[vsk]

If compatible differences between a CU's lang opts and an imported module's lang opts are not allowed, issue an error when there is a mismatch in the set of enabled, non-modular sanitizers.

[vsk]

Add a comment explaining how and why ASTReader checks for sanitizer feature mismatches.

[vsk]

I've uploaded the correct diff this time, sorry for the confusion.

[aprantl]

include/clang/Basic/Sanitizers.h

64–66

Might as well delete the \brief while you are modifying the comment.

85

getModularSanitizers is a bit misleading. How about getASTTransparentSanitizers or something along those lines?

[vsk]

Address comments from Adrian. (I settled on renaming 'getModularSanitizers' to 'getPPTransparentSanitizers'.)

[vsk]

Ping.

[aprantl]

This is good from my point of view now. You might want to wait for an ok from @spyffe.

include/clang/Basic/Sanitizers.h

struct SanitizerSet {
   bool hasOneOf(SanitizerMask K) const { return Mask & K; }
 
   /// \brief Enable or disable a certain (single) sanitizer.
   void set(SanitizerMask K, bool Value) {
     assert(llvm::isPowerOf2_64(K));
     Mask = Value ? (Mask | K) : (Mask & ~K);
   }
 
-  /// \brief Disable all sanitizers.
-  void clear() { Mask = 0; }
+  /// \brief Disable the sanitizers specified in \p K (by default, disable
+  /// all sanitizers).
+  void clear(SanitizerMask K = SanitizerKind::All) { Mask &= ~K; }
 
   /// \brief Returns true if at least one sanitizer is enabled.
   bool empty() const { return Mask == 0; }
 
   /// \brief Bitmask of enabled sanitizers.
   SanitizerMask Mask = 0;
 };
 
 /// Parse a single value from a -fsanitize= or -fno-sanitize= value list.
 /// Returns a non-zero SanitizerMask, or \c 0 if \p Value is not known.
 SanitizerMask parseSanitizerValue(StringRef Value, bool AllowGroups);
 
 /// For each sanitizer group bit set in \p Kinds, set the bits for sanitizers
 /// this group enables.
 SanitizerMask expandSanitizerGroups(SanitizerMask Kinds);
 
 }  // end namespace clang
 
 #endif

lib/Basic/LangOptions.cpp

 
 void LangOptions::resetNonModularOptions() {
 #define LANGOPT(Name, Bits, Default, Description)
 #define BENIGN_LANGOPT(Name, Bits, Default, Description) Name = Default;
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
   Name = Default;
 #include "clang/Basic/LangOptions.def"
 
-  // 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();
   XRayAlwaysInstrumentFiles.clear();
   XRayNeverInstrumentFiles.clear();
 
   CurrentModule.clear();
   IsHeaderFile = false;
 }
 
 bool LangOptions::isNoBuiltinFunc(StringRef FuncName) const {
   for (unsigned i = 0, e = NoBuiltinFuncs.size(); i != e; ++i)
     if (FuncName.equals(NoBuiltinFuncs[i]))
       return true;
   return false;
 }

lib/Frontend/CompilerInvocation.cpp

#include "clang/Basic/LangOptions.def"
   code = hash_combine(code, hsOpts.ModuleUserBuildPath);
 
   // Extend the signature with the module file extensions.
   const FrontendOptions &frontendOpts = getFrontendOpts();
   for (const auto &ext : frontendOpts.ModuleFileExtensions) {
     code = ext->hashExtension(code);
   }
 
+  // Extend the signature with the enabled sanitizers, if at least one is
+  // enabled. Sanitizers which cannot affect AST generation aren't hashed.
+  SanitizerSet SanHash = LangOpts->Sanitize;
+  SanHash.clear(SanitizerKind::CFI | SanitizerKind::Integer |
+                SanitizerKind::Nullability | SanitizerKind::Undefined);
+  if (!SanHash.empty())
+    code = hash_combine(code, SanHash.Mask);
+
   return llvm::APInt(64, code).toString(36, /*Signed=*/false);
 }
 
 namespace clang {
 
 template<typename IntTy>
 static IntTy getLastArgIntValueImpl(const ArgList &Args, OptSpecifier Id,
                                     IntTy Default,

test/Modules/Inputs/check-for-sanitizer-feature/check.h

+#if __has_feature(address_sanitizer)
+#define HAS_ASAN 1
+#else
+#define HAS_ASAN 0
+#endif

test/Modules/Inputs/check-for-sanitizer-feature/map

+module check_feature {
+  header "check.h"
+}

test/Modules/check-for-sanitizer-feature.cpp

+// RUN: rm -rf %t.1 %t.2
+// RUN: mkdir %t.1 %t.2
+
+// Build and use an ASan-enabled module.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t.1 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.1 | count 2
+
+// Force a module rebuild by disabling ASan.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.1 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.1 | count 3
+
+// Enable ASan again: check that there is no import failure, and no rebuild.
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t.1 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.1 | count 3
+
+// Some sanitizers can not affect AST generation when enabled. Check that
+// module rebuilds don't occur when these sanitizers are enabled.
+//
+// First, built without any sanitization.
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.2 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.2 | count 2
+//
+// Next, build with sanitization, and check that a new module isn't built.
+// RUN: %clang_cc1 -fsanitize=cfi-vcall,unsigned-integer-overflow,nullability-arg,null -fmodules \
+// RUN:   -fmodules-cache-path=%t.2 \
+// RUN:   -fmodule-map-file=%S/Inputs/check-for-sanitizer-feature/map \
+// RUN:   -I %S/Inputs/check-for-sanitizer-feature -verify %s
+// RUN: ls %t.2 | count 2
+
+#include "check.h"
+
+#if __has_feature(address_sanitizer)
+#if HAS_ASAN != 1
+#error Does not have the address_sanitizer feature, but should.
+#endif
+#else
+#if HAS_ASAN != 0
+#error Has the address_sanitizer feature, but shouldn't.
+#endif
+#endif
+
+// expected-no-diagnostics