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

[Justin Bogner]

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.