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

[David Blaikie]

Sure, though you'll need a (void)numAbstractScopes; there somewhere so
that non-asserts builds don't warn about the unused variable (since
it's only used in the assert, in a non-asserts build it appears to be
not used at all and fires the warning).

Did you reduce a good test case once you had this assertion, or did
this fail on existing test cases once added?

On Mon, Jun 23, 2014 at 10:20 AM, Kuba Břečka <kuba.brecka at gmail.com> wrote:
> Hi David,
> thanks for helping me figure this out. What do you think of adding this
> assert permanently? See http://reviews.llvm.org/D4259.
>
> Kuba
>
>
> On Mon, Jun 16, 2014 at 1:32 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> 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
>
>