Page MenuHomePhabricator

[Modules] Handle sanitizer feature mismatches when importing modules
ClosedPublic

Authored by vsk on May 1 2017, 4:43 PM.

Details

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.

Diff Detail

Event Timeline

vsk created this revision.May 1 2017, 4:43 PM
vsk updated this revision to Diff 98030.May 5 2017, 3:26 PM
vsk edited the summary of this revision. (Show Details)
  • 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.
vsk added a reviewer: spyffe.May 5 2017, 3:28 PM
spyffe requested changes to this revision.May 5 2017, 3:44 PM

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
26

Typo: build

43

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

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

Same below.

This revision now requires changes to proceed.May 5 2017, 3:44 PM
aprantl added a subscriber: aprantl.May 5 2017, 3:45 PM

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 added a comment.May 5 2017, 4:27 PM

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
26

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

spyffe added inline comments.May 8 2017, 10:47 AM
lib/Basic/LangOptions.cpp
32

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

dexonsmith edited edge metadata.May 9 2017, 10:17 AM

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 added a comment.May 9 2017, 12:07 PM

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?

In D32724#750074, @vsk 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 updated this revision to Diff 98343.May 9 2017, 1:00 PM
vsk edited edge metadata.
vsk retitled this revision from [Modules] Include the enabled sanitizers in the module hash to [Modules] Handle sanitizer feature mismatches when importing modules.
vsk edited the summary of this revision. (Show Details)

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 updated this revision to Diff 98349.May 9 2017, 1:46 PM
vsk marked 7 inline comments as done.

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

vsk updated this revision to Diff 98363.May 9 2017, 4:01 PM

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

aprantl added inline comments.May 9 2017, 4:51 PM
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 updated this revision to Diff 98371.May 9 2017, 5:01 PM
vsk marked 2 inline comments as done.

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

aprantl accepted this revision.May 23 2017, 2:56 PM

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

This revision was automatically updated to reflect the committed changes.