<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 1 May 2017 at 16:43, Vedant Kumar via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">vsk created this revision.<br>
<br>
When building with implicit modules it's possible to hit a scenario<br>
where modules are built without -fsanitize=address, and are subsequently<br>
imported into CUs with -fsanitize=address enabled. This can cause<br>
strange failures at runtime. One case I've seen affects libcxx, since<br>
its vector implementation behaves differently when ASan is enabled.<br>
<br>
Implicit module builds should work even when -fsanitize=X is toggled on<br>
and off across multiple compiler invocations. This patch implements a<br>
fix by including the set of enabled sanitizers in the module hash.<br>
<br>
I am not sure if the module hash should change when building with<br>
explicit modules, and would appreciate any input on this.<br></blockquote><div><br></div><div>I would expect this 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.)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="https://reviews.llvm.org/D32724" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D32724</a><br>
<br>
Files:<br>
lib/Basic/LangOptions.cpp<br>
lib/Frontend/<wbr>CompilerInvocation.cpp<br>
test/Modules/Inputs/check-for-<wbr>sanitizer-feature/check.h<br>
test/Modules/Inputs/check-for-<wbr>sanitizer-feature/map<br>
test/Modules/check-for-<wbr>sanitizer-feature.cpp<br>
<br>
<br>
Index: test/Modules/check-for-<wbr>sanitizer-feature.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- /dev/null<br>
+++ test/Modules/check-for-<wbr>sanitizer-feature.cpp<br>
@@ -0,0 +1,32 @@<br>
+// RUN: rm -rf %t<br>
+// RUN: mkdir %t<br>
+<br>
+// Build and use an ASan-enabled module.<br>
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \<br>
+// RUN: -fmodule-map-file=%S/Inputs/<wbr>check-for-sanitizer-feature/<wbr>map \<br>
+// RUN: -I %S/Inputs/check-for-sanitizer-<wbr>feature -verify %s<br>
+<br>
+// Force a module rebuild by disabling ASan.<br>
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t \<br>
+// RUN: -fmodule-map-file=%S/Inputs/<wbr>check-for-sanitizer-feature/<wbr>map \<br>
+// RUN: -I %S/Inputs/check-for-sanitizer-<wbr>feature -verify %s<br>
+<br>
+// Force another module rebuild by enabling ASan again.<br>
+// RUN: %clang_cc1 -fsanitize=address -fmodules -fmodules-cache-path=%t \<br>
+// RUN: -fmodule-map-file=%S/Inputs/<wbr>check-for-sanitizer-feature/<wbr>map \<br>
+// RUN: -I %S/Inputs/check-for-sanitizer-<wbr>feature -verify %s<br>
+<br>
+#include "check.h"<br>
+<br>
+#if __has_feature(address_<wbr>sanitizer)<br>
+#if HAS_ASAN != 1<br>
+#error Does not have the address_sanitizer feature, but should.<br>
+#endif<br>
+#else<br>
+#if HAS_ASAN != 0<br>
+#error Has the address_sanitizer feature, but shouldn't.<br>
+#endif<br>
+<br>
+#endif<br>
+<br>
+// expected-no-diagnostics<br>
Index: test/Modules/Inputs/check-for-<wbr>sanitizer-feature/map<br>
==============================<wbr>==============================<wbr>=======<br>
--- /dev/null<br>
+++ test/Modules/Inputs/check-for-<wbr>sanitizer-feature/map<br>
@@ -0,0 +1,3 @@<br>
+module check_feature {<br>
+ header "check.h"<br>
+}<br>
Index: test/Modules/Inputs/check-for-<wbr>sanitizer-feature/check.h<br>
==============================<wbr>==============================<wbr>=======<br>
--- /dev/null<br>
+++ test/Modules/Inputs/check-for-<wbr>sanitizer-feature/check.h<br>
@@ -0,0 +1,5 @@<br>
+#if __has_feature(address_<wbr>sanitizer)<br>
+#define HAS_ASAN 1<br>
+#else<br>
+#define HAS_ASAN 0<br>
+#endif<br>
Index: lib/Frontend/<wbr>CompilerInvocation.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- lib/Frontend/<wbr>CompilerInvocation.cpp<br>
+++ lib/Frontend/<wbr>CompilerInvocation.cpp<br>
@@ -2693,6 +2693,11 @@<br>
code = ext->hashExtension(code);<br>
}<br>
<br>
+ // Extend the signature with the enabled sanitizers, if at least one is<br>
+ // enabled.<br>
+ if (!LangOpts->Sanitize.empty())<br>
+ code = hash_combine(code, LangOpts->Sanitize.Mask);<br>
+<br>
return llvm::APInt(64, code).toString(36, /*Signed=*/false);<br>
}<br>
<br>
Index: lib/Basic/LangOptions.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- lib/Basic/LangOptions.cpp<br>
+++ lib/Basic/LangOptions.cpp<br>
@@ -29,9 +29,11 @@<br>
Name = Default;<br>
#include "clang/Basic/LangOptions.def"<br>
<br>
- // FIXME: This should not be reset; modules can be different with different<br>
- // sanitizer options (this affects __has_feature(address_<wbr>sanitizer) etc).<br>
- Sanitize.clear();<br>
+ // FIXME: Some sanitizers do not expose any information via the preprocessor<br>
+ // (e.g UBSan). We may be able to avoid some module rebuilds by disabling<br>
+ // those sanitizers.<br>
+<br>
+ // These options do not affect AST generation.<br>
SanitizerBlacklistFiles.clear(<wbr>);<br>
XRayAlwaysInstrumentFiles.<wbr>clear();<br>
XRayNeverInstrumentFiles.<wbr>clear();<br>
<br>
<br>
</blockquote></div><br></div></div>