Richard Smith <rich...@metafoo.co.uk> writes: > On Fri, Jul 10, 2015 at 3:15 PM, Justin Bogner <m...@justinbogner.com> wrote: > > Justin Bogner <m...@justinbogner.com> writes: > > Justin Bogner <m...@justinbogner.com> writes: > >> Richard Smith <rich...@metafoo.co.uk> writes: > >>> On Tue, Jun 30, 2015 at 5:32 PM, Justin Bogner <m...@justinbogner.com> > wrote: > >>>> Richard Smith <rich...@metafoo.co.uk> writes: > >>>>> On Tue, Jun 30, 2015 at 4:57 PM, Justin Bogner > >>>>> <m...@justinbogner.com> wrote: > >>>>>> + if (Name && !isAcceptableTagRedeclaration(PrevTagDecl, > Kind, > >>>>>> + TUK == TUK_Definition, KWLoc, > >>>>>> + *Name)) { > >>>>> > >>>>> So, this is tricky: if SafeToContinue is false, we don't want to > >>>>> carry on because we'd make an EnumDecl a redeclaration of a > >>>>> RecordDecl or vicer versa, which would violate the AST > >>>>> invariants. I'm not sure how we get here from error recovery: > >>>>> both places that set Name to nullptr also jump past this code. > >>>>> I think the only way to reach this is if SkipBody->Previous is > >>>>> non-null, in which case this is not error recovery and we should > >>>>> perform this check. > >>>>> > >>>>> Can you instead change isAcceptableTagRedeclaration to take a > >>>>> const IdentifierInfo*? > >>>> > >>>> The IdentifierInfo is only used in a diagnostic - I think it makes > sense > >>>> to pass `OrigName` here if we want the check. This is what we do a > >>>> little below for diagnoseQualifiedDeclaration(). > >>> > >>> But OrigName is null too, isn't it? The only places that make Name and > >>> OrigName differ also jump past this code. > >> > >> You're right - I misread the assert where OrigName is assigned and was > >> confused. > >> > >> I don't think passing const IdentifierInfo* is correct - > >> isAcceptableTagRedeclaration implements [dcl.type.elab]p3, which is > >> about elaborated-type-specifiers with the same name agreeing on kind. > >> It doesn't make much sense to do this check if there isn't a name at > >> all, and it seems suspicious to me that we're getting here at all. > >> > >> What does !Previous.empty() mean if we don't have a Name? > >> > >> This behaviour only triggers on one test in the test suite, > >> test/Modules/submodules-merge-defs.cpp. You can reproduce with > >> assert(Name) before the call to isAcceptableTagRedeclaration() if > you're > >> curious. > > > > FWIW, we're no longer hitting the UB here as of r241763 - I believe > > r241743 is the relevant commit. It's not totally clear to me whether > > that fixed or hid the bug though. > > Sorry, I was wrong about this, but now I understand how we get here. > This fires when we're merging anonymous enum definitions (as per > r236690). We're merging an enum like the following with a definition > we've already seen: > > enum { f, g, h }; > > In this case, it's perfectly reasonable to not have a name and we do > want to check that the redecl is compatible, so I've gone with the > "const IdentifierInfo *" solution, attached. Okay to commit? > > LGTM
r241963 > > On a side note, passing null pointers to `operator<<(const > DiagnosticBuilder, const IdentifierInfo)` eventually leads to this code: > > case DiagnosticsEngine::ak_identifierinfo: { > const IdentifierInfo *II = getArgIdentifier(ArgNo); > assert(ModifierLen == 0 && "No modifiers for strings yet"); > > // Don't crash if get passed a null pointer by accident. > if (!II) { > const char *S = "(null)"; > OutStr.append(S, S + strlen(S)); > continue; > } > > llvm::raw_svector_ostream(OutStr) << '\'' << II->getName() << '\''; > break; > } > > Here, the comment claims that passing a null pointer is an accident, but > we seem to do it intentionally (at least, a few tests fail if I change > it to an assert). Is the comment wrong, or should we look into fixing > that? > > Just looking through ActOnTag it seems there are a couple of different > diagnostics which can already produce such errors for anonymous declarations. > Ideally we should diagnose this better, though that won't be completely > trivial because it's not always reasonable to assume that a null II means " > <anonymous declaration>". So yes, we should fix this, but it's unlikely we > have good test coverage for these cases today so it may be a little tricky to > track them all down. _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits