[llvm] r189495 - PR16995: DebugInfo: Don't overwrite existing member lists when adding template arguments

[David Blaikie]

On Wed, Aug 28, 2013 at 1:06 PM, Eric Christopher <echristo at gmail.com> wrote:
> On Wed, Aug 28, 2013 at 10:28 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> Author: dblaikie
>> Date: Wed Aug 28 12:28:16 2013
>> New Revision: 189495
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=189495&view=rev
>> Log:
>> PR16995: DebugInfo: Don't overwrite existing member lists when adding template arguments
>>
>> With the added debug assertions this fix is covered by existing Clang
>> tests. (& found some other issues, also fixed)
>>
>
> This actually seems to be checking that we don't have a NULL array
> being passed in and the debug checks are that we have a
> super/identical set of elements when we're going to replace?

Yes, the first part provides the "don't overwrite existing member list
when adding template arguments", see the call site in Clang,
CGDebugInfo.cpp:2270:

RealDecl.setTypeArray(llvm::DIArray(),
CollectCXXTemplateParams(TSpecial, DefUnit));

When we CollectCXXTemplateParams we might actually end up discovering
some of the members of 'RealDecl' (such as static members, see the
test case in the referenced PR) that we then overwrite to empty when
setTypeArray is called.

> I do wish we could have a test that fails when the code isn't there.

We do - if you remove the "if (Elements)" a Clang test case will fail
the assertions (& without the assertions, it would fail a later
assertion (see PR16995) in a subset of cases (none covered by test
cases)) - I could construct a test case that would fail even in an
assert-free build, but it's bit more involved. Happy to do so if you
think that's valuable.

>> -  N->replaceOperandWith(10, Elements);
>> +  if (Elements) {
>> +#ifndef NDEBUG
>> +    // Check that we're not dropping any elements on the floor here
>
> Silly nit: Needs a '.'
>
>> +    if (const MDNode *El = cast_or_null<MDNode>(N->getOperand(10))) {
>> +      for (unsigned i = 0; i != El->getNumOperands(); ++i) {
>> +        if (i == 0 && isa<ConstantInt>(El->getOperand(i)))
>> +          continue;
>> +        const MDNode *E = cast<MDNode>(El->getOperand(i));
>> +        bool found = false;
>> +        for (unsigned j = 0; !found && j != Elements.getNumElements(); ++j) {
>> +          found = E == Elements.getElement(j);
>> +        }
>> +        assert(found && "Losing a member during member list replacement");
>> +      }
>> +    }
>> +#endif
>> +    N->replaceOperandWith(10, Elements);
>> +  }
>>    if (TParams)
>>      N->replaceOperandWith(13, TParams);
>
> Any reason we don't want the same check here? I haven't gone through
> the path we're taking to get here from the bug to check.

Template parameters aren't lazily added like members are, so this
isn't a concern - I could build the superset check into a function &
use it in both for symmetry, but I don't see it as a sufficiently
interesting risk to secure against.

> Also, comments :)

There was a comment at the start & a textual description in the assert
- are they insufficient or are there other aspects that you'd like
more clarification on?

(I know I don't comment enough, so I'm not pushing back against the
request, just having trouble understanding the specific nature of the
request)

- David