[llvm] r210221 - DebugInfo: Reapply r209984 (reverted in r210143), asserting that abstract DbgVariables have DIEs.

[David Blaikie]

On Mon, Jun 16, 2014 at 1:11 PM, Kuba Břečka <kuba.brecka at gmail.com> wrote:
> I'm experiencing a heap use-after-free memory issue after this patch. Is
> this change (more context provided) intented?
>
> // Construct abstract scopes.
> for (LexicalScope *AScope : LScopes.getAbstractScopesList()) {
>   DISubprogram SP(AScope->getScopeNode());
>   if (!SP.isSubprogram())
>     continue;
>   // Collect info for variables that were optimized out.
>   DIArray Variables = SP.getVariables();
>   for (unsigned i = 0, e = Variables.getNumElements(); i != e; ++i) {
>     DIVariable DV(Variables.getElement(i));
>     assert(DV && DV.isVariable());
>     if (!ProcessedVars.insert(DV))
>       continue;
> -   findAbstractVariable(DV, DV.getContext());
> +   getOrCreateAbstractVariable(DV, DV.getContext());
>   }
>   constructAbstractSubprogramScopeDIE(TheCU, AScope);
> }
>
> It seems like getOrCreateAbstractVariable calls
> LScopes.getOrCreateAbstractScope which can cause the AbstractScopesList
> member vector to be resized (via push_back), introducing a
> modify-container-while-iterating-it bug.
>
> Unfortunately, I don't have a test case for this, any ideas how could I
> create one?

If you put an assert after the getOrCreateAbstractVariable that the
size of the abstract scopes list hasn't changed (eg:

auto numAbstractScopes = LScopes.getAbstractScopesList();
getOrCreateAbstractVariable...
assert(numAbstractScopes == LScopes.getAbstractScopesList());

Then you should be able to reliably reduce a test case either manually
or using something like creduce, etc.

If it's failing for the reason you've explained, that should fail with
the assertion in the provided test case
(test/DebugInfo/missing-abstract-variable.ll). Ideally we should just
be iterating over the subprogram abstract scopes (& that code should
never cause one of /those/ to be created at this late stage)... but
I'm not sure if it'll be worth creating a separate list for them or
just to change this loop to handle insertions (by using indexes
instead of a range-for).

- David

>
> Kuba
>
>
>>
>> Author: dblaikie
>> Date: Wed Jun  4 18:50:52 2014
>> New Revision: 210221
>> URL: http://llvm.org/viewvc/llvm-project?rev=210221&view=rev
>> Log:
>> DebugInfo: Reapply r209984 (reverted in r210143), asserting that abstract
>> DbgVariables have DIEs.
>> Abstract variables within abstract scopes that are entirely optimized
>> away in their first inlining are omitted because their scope is not
>> present so the variable is never created. Instead, we should ensure the
>> scope is created so the variable can be added, even if it's been
>> optimized away in its first inlining.
>> This fixes the incorrect debug info in missing-abstract-variable.ll
>> (added in r210143) and passes an asserts self-hosting build, so
>> hopefully there's not more of these issues left behind... *fingers
>> crossed*.
>> Modified:
>>     llvm/trunk/include/llvm/CodeGen/LexicalScopes.h
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>     llvm/trunk/test/DebugInfo/missing-abstract-variable.ll