On Mon, Apr 28, 2014 at 5:11 PM, Ben Langmuir <blangm...@apple.com> wrote:
> > On Apr 28, 2014, at 4:33 PM, Richard Smith <rich...@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 <blangm...@apple.com> wrote: > >> Updated patch attached. >> >> Ben >> >> >> >> On Apr 28, 2014, at 3:02 PM, Ben Langmuir <blangm...@apple.com> wrote: >> >> >> On Apr 28, 2014, at 2:13 PM, Richard Smith <rich...@metafoo.co.uk> wrote: >> >> On Sat, Apr 26, 2014 at 8:35 PM, Ben Langmuir <blangm...@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! >> >> >> >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
