[PATCH] Check -Werror options to see if a module should be rebuilt.

[Richard Smith]

On Mon, Apr 28, 2014 at 5:11 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Apr 28, 2014, at 4:33 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> I would marginally prefer using < DiagnosticsEngine::Error rather than <=
> DiagnosticsEngine::Warning, since I think it's clearer about what we're
> testing for.
>
>
> Sure.
>
>
>
> +  ModuleFile *TopImport = *ModuleMgr.rbegin();
>
> Are we guaranteed that the last-imported ModuleFile is the one we're
> validating at this point? (What happens if we read the diagnostic options
> block after we read the imported modules block?)
>
>
> It actually isn’t the currently validating module (unless it is a leaf).
>  But it must be in the transitive closure of its imports, so it ends up
> finding the same top-level import.
>

OK, that makes sense, but is a bit subtle =) Explaining this in the comment
would help.

> Other than that, LGTM, thanks!
>
>
> On Mon, Apr 28, 2014 at 4:22 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>> Updated patch attached.
>>
>> Ben
>>
>>
>>
>> On Apr 28, 2014, at 3:02 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>>
>>
>> On Apr 28, 2014, at 2:13 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> On Sat, Apr 26, 2014 at 8:35 PM, Ben Langmuir <blangmuir at apple.com>
>> wrote:
>>
>>> As discussed here
>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140414/103530.html
>>>
>>> This patch checks whether the diagnostic options that could lead to
>>> errors (principally -Werror) are consistent between when a module was built
>>> and when it is loaded.  If there are new -Werror flags, then the module is
>>> rebuilt.  In order to canonicalize the options we do this check at the
>>> level of the constructed DiagnosticsEngine, which contains the final set of
>>> diag to diagnostic level mappings.  Currently we only rebuild with the new
>>> diagnostic options, but we intend to refine this in the future to include
>>> the union of the new and old flags, since we know the old ones did not
>>> cause errors.  System modules are only rebuilt when -Wsystem-headers is
>>> enabled.
>>>
>>> One oddity is that unlike checking language options, we don’t perform
>>> this diagnostic option checking when loading from a precompiled header.
>>>  The reason for this is that the compiler cannot rebuild the PCH, so
>>> anything that requires it to be rebuilt effectively leaks into the build
>>> system.  And in this case, that would mean the build system understanding
>>> the complex relationship between diagnostic options and the underlying
>>> diagnostic mappings, which is unreasonable.
>>
>>
>> More than that, if the AST file was explicitly built by the user, they
>> may *intend* to apply different warning flags to the two cases, and we
>> shouldn't stop them from doing that. (Likewise, we will eventually want a
>> properly-productized way to explicitly build modules, and this constraint
>> shouldn't apply to such explicitly-built modules either.)
>>
>>
>> Makes sense to me.
>>
>>
>>
>>> Skipping the check is safe, because these options do not affect the
>>> generated AST.  You simply won’t get new build errors due to changed
>>> -Werror options automatically, which is also true for non-module cases.
>>
>>
>> @@ -4481,18 +4579,18 @@ bool ASTReader::ParseTargetOptions(const
>> RecordData &Record,
>>
>>  bool ASTReader::ParseDiagnosticOptions(const RecordData &Record, bool
>> Complain,
>>                                         ASTReaderListener &Listener) {
>> -  DiagnosticOptions DiagOpts;
>> +  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions);
>>    unsigned Idx = 0;
>> -#define DIAGOPT(Name, Bits, Default) DiagOpts.Name = Record[Idx++];
>> +#define DIAGOPT(Name, Bits, Default) DiagOpts->Name = Record[Idx++];
>>  #define ENUM_DIAGOPT(Name, Type, Bits, Default) \
>> -  DiagOpts.set##Name(static_cast<Type>(Record[Idx++]));
>> +  DiagOpts->set##Name(static_cast<Type>(Record[Idx++]));
>>  #include "clang/Basic/DiagnosticOptions.def"
>>
>>    for (unsigned N = Record[Idx++]; N; --N) {
>> -    DiagOpts.Warnings.push_back(ReadString(Record, Idx));
>> +    DiagOpts->Warnings.push_back(ReadString(Record, Idx));
>>    }
>>
>> -  return Listener.ReadDiagnosticOptions(DiagOpts, Complain);
>> +  return Listener.ReadDiagnosticOptions(*DiagOpts, Complain);
>>  }
>>
>>  bool ASTReader::ParseFileSystemOptions(const RecordData &Record, bool
>> Complain,
>>
>>
>> This change suggests that the interface to ReadDiagnosticOptions has
>> become error-prone: if we now need the DiagnosticOptions object to outlive
>> the call to ReadDiagnosticOptions, it should not be passed by const
>> reference. If we need it to be allocated inside an IntrusiveRefCntPtr, we
>> should require that in the interface. What's going on here?
>>
>>
>> The latter. DiagnosticsEngine expects DiagOpts to be an
>> IntrusiveRefCntPtr.  I will change the interfaces to use the correct types.
>>
>>
>>
>> +  if ((*ModuleMgr.begin())->Kind != serialization::MK_Module)
>> +    return false;
>>
>> This doesn't look quite right. We could be loading a new module file
>> that's unrelated to the PCH or preamble, and in that case we should apply
>> these checks.
>>
>>
>> True. I suppose iterating ImportedBy[0] is good enough to tell us whether
>> this came from an MK_Module or not.
>>
>>
>>
>> +  // Check current mappings for new -Werror mappings
>> +  for (auto DiagIDMappingPair : Diags.getDiagnosticMappings()) {
>> +    diag::kind DiagID = DiagIDMappingPair.first;
>> +    Level CurLevel = Diags.getDiagnosticLevel(DiagID, SourceLocation());
>> +    if (CurLevel <= DiagnosticsEngine::Warning)
>> +      continue; // not significant
>> +    Level StoredLevel = StoredDiags.getDiagnosticLevel(DiagID,
>> SourceLocation());
>> +    if (StoredLevel <= DiagnosticsEngine::Warning) {
>> +      if (Complain)
>> +        Diags.Report(diag::err_pch_diagopt_mismatch) << "-Werror=" +
>> +
>>  Diags.getDiagnosticIDs()->getWarningOptionForDiag(DiagID).str();
>> +      return true;
>> +    }
>> +  }
>> +
>> +  // Check the stored mappings for cases that were explicitly mapped to
>> *not* be
>> +  // errors that are now errors because of options like -Werror.
>> +  for (auto DiagIDMappingPair : StoredDiags.getDiagnosticMappings()) {
>> +    diag::kind DiagID = DiagIDMappingPair.first;
>> +    Level StoredLevel = StoredDiags.getDiagnosticLevel(DiagID,
>> SourceLocation());
>> +    if (StoredLevel >= DiagnosticsEngine::Error)
>> +      continue; // already an error
>> +    Level CurLevel = Diags.getDiagnosticLevel(DiagID, SourceLocation());
>> +    if (CurLevel >= DiagnosticsEngine::Error) {
>> +      if (Complain)
>> +        Diags.Report(diag::err_pch_diagopt_mismatch) << "-Werror=" +
>> +
>>  Diags.getDiagnosticIDs()->getWarningOptionForDiag(DiagID).str();
>> +      return true;
>> +    }
>> +  }
>>
>> Your two loop bodies are semantically identical; can you factor out the
>> redundancy here? (I guess you're duplicating this to try to avoid adding
>> extra diagnostic mapping entries?)
>>
>> Sure, I can simplify this.
>>
>>
>> +static bool checkDiagnosticMappings(DiagnosticsEngine &StoredDiags,
>> +                                    DiagnosticsEngine &Diags,
>> +                                    bool IsSystem, bool Complain) {
>>
>> We should also check for differences in ExtBehavior here (in case the
>> user has enabled -fpedantic-errors).
>>
>>
>>  Yes we should!
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140428/24cfe003/attachment.html>