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

[Ben Langmuir]

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.

Ben

> 
> 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/ba8740bd/attachment.html>