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

[Justin Bogner]

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?

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?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ub-clang-sema-3.patch
Type: text/x-patch
Size: 7477 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150710/a14ef780/attachment.bin>