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. OTOH, the following passes ninja check-clang now, which makes sense to me: diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index 9db14bf..6208bae 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -11559,6 +11559,8 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, Redecl = NotForRedeclaration; LookupResult Previous(*this, Name, NameLoc, LookupTagName, Redecl); + assert(Name || Previous.empty()); + if (Name && SS.isNotEmpty()) { // We have a nested-name tag ('struct foo::bar'). _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits