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. Ben > > 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
