[PATCH] D32724: [Modules] Include the enabled sanitizers in the module hash

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D32724#750074, @vsk wrote:

> In https://reviews.llvm.org/D32724#749868, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/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.


https://reviews.llvm.org/D32724