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

[David Blaikie]

On Wed, Aug 28, 2013 at 1:26 PM, Eric Christopher <echristo at gmail.com> wrote:
>> 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.
>
> Fun. Seems error prone in some way, but...
>
>>
>>> 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.
>
> Perhaps take the code from the PR and verifying all of the elements
> are there at least. It won't reproduce the bug necessarily but could
> be a good general test.

Maybe - it's a bit scattershot. It requires particularly circuitous
references to create these cases where the expansion of context,
template arguments, etc, causes the type itself to be expanded. The
asserts pull these failure modes into less esoteric test cases.

>
>>> 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.
>
> Reasonable.
>
>>
>>> 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?
>>
>
> Just a block comment above the verification code on what you're
> verifying. I.e. something along the lines of "Iterate through the two
> lists of elements and ensure that we've got an identical or superset
> of elements." Comments are easier to read than the code.

Pulled this out into a separate function to hopefully make the code
more self-documenting without comments (& should make the main path
through the code easier as you don't have to visually skip all the
loop details, etc) in r189512.