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

[Justin Bogner]

Richard Smith <richard at metafoo.co.uk> writes:
> On Fri, Jul 10, 2015 at 3:15 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
>     Justin Bogner <mail at justinbogner.com> writes:
>     > 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.
>    
>     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.