[PATCH] Sema: Fix some undefined behaviour when acting on redeclarations

[Justin Bogner]

Justin Bogner <mail at justinbogner.com> writes:
> Richard Smith <richard at metafoo.co.uk> writes:
>> On Tue, Jun 30, 2015 at 5:32 PM, Justin Bogner <mail at justinbogner.com> wrote:
>>> Richard Smith <richard at metafoo.co.uk> writes:
>>>> On Tue, Jun 30, 2015 at 4:57 PM, Justin Bogner
>>>> <mail at 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').