<div dir="ltr">Unfortunately, I don't have a test case and the existing ones don't trigger the buggy behavior. I'll see what I can do.<div><br></div><div>Kuba<div class="gmail_extra"><br><div class="gmail_quote">
On Mon, Jun 23, 2014 at 11:02 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Sure, though you'll need a (void)numAbstractScopes; there somewhere so<br>
that non-asserts builds don't warn about the unused variable (since<br>
it's only used in the assert, in a non-asserts build it appears to be<br>
not used at all and fires the warning).<br>
<br>
Did you reduce a good test case once you had this assertion, or did<br>
this fail on existing test cases once added?<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Jun 23, 2014 at 10:20 AM, Kuba Břečka <<a href="mailto:kuba.brecka@gmail.com">kuba.brecka@gmail.com</a>> wrote:<br>
> Hi David,<br>
> thanks for helping me figure this out. What do you think of adding this<br>
> assert permanently? See <a href="http://reviews.llvm.org/D4259" target="_blank">http://reviews.llvm.org/D4259</a>.<br>
><br>
> Kuba<br>
><br>
><br>
> On Mon, Jun 16, 2014 at 1:32 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Jun 16, 2014 at 1:11 PM, Kuba Břečka <<a href="mailto:kuba.brecka@gmail.com">kuba.brecka@gmail.com</a>><br>
>> wrote:<br>
>> > I'm experiencing a heap use-after-free memory issue after this patch. Is<br>
>> > this change (more context provided) intented?<br>
>> ><br>
>> > // Construct abstract scopes.<br>
>> > for (LexicalScope *AScope : LScopes.getAbstractScopesList()) {<br>
>> > DISubprogram SP(AScope->getScopeNode());<br>
>> > if (!SP.isSubprogram())<br>
>> > continue;<br>
>> > // Collect info for variables that were optimized out.<br>
>> > DIArray Variables = SP.getVariables();<br>
>> > for (unsigned i = 0, e = Variables.getNumElements(); i != e; ++i) {<br>
>> > DIVariable DV(Variables.getElement(i));<br>
>> > assert(DV && DV.isVariable());<br>
>> > if (!ProcessedVars.insert(DV))<br>
>> > continue;<br>
>> > - findAbstractVariable(DV, DV.getContext());<br>
>> > + getOrCreateAbstractVariable(DV, DV.getContext());<br>
>> > }<br>
>> > constructAbstractSubprogramScopeDIE(TheCU, AScope);<br>
>> > }<br>
>> ><br>
>> > It seems like getOrCreateAbstractVariable calls<br>
>> > LScopes.getOrCreateAbstractScope which can cause the AbstractScopesList<br>
>> > member vector to be resized (via push_back), introducing a<br>
>> > modify-container-while-iterating-it bug.<br>
>> ><br>
>> > Unfortunately, I don't have a test case for this, any ideas how could I<br>
>> > create one?<br>
>><br>
>> If you put an assert after the getOrCreateAbstractVariable that the<br>
>> size of the abstract scopes list hasn't changed (eg:<br>
>><br>
>> auto numAbstractScopes = LScopes.getAbstractScopesList();<br>
>> getOrCreateAbstractVariable...<br>
>> assert(numAbstractScopes == LScopes.getAbstractScopesList());<br>
>><br>
>> Then you should be able to reliably reduce a test case either manually<br>
>> or using something like creduce, etc.<br>
>><br>
>> If it's failing for the reason you've explained, that should fail with<br>
>> the assertion in the provided test case<br>
>> (test/DebugInfo/missing-abstract-variable.ll). Ideally we should just<br>
>> be iterating over the subprogram abstract scopes (& that code should<br>
>> never cause one of /those/ to be created at this late stage)... but<br>
>> I'm not sure if it'll be worth creating a separate list for them or<br>
>> just to change this loop to handle insertions (by using indexes<br>
>> instead of a range-for).<br>
>><br>
>> - David<br>
>><br>
>> ><br>
>> > Kuba<br>
>> ><br>
>> ><br>
>> >><br>
>> >> Author: dblaikie<br>
>> >> Date: Wed Jun 4 18:50:52 2014<br>
>> >> New Revision: 210221<br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=210221&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=210221&view=rev</a><br>
>> >> Log:<br>
>> >> DebugInfo: Reapply r209984 (reverted in r210143), asserting that<br>
>> >> abstract<br>
>> >> DbgVariables have DIEs.<br>
>> >> Abstract variables within abstract scopes that are entirely optimized<br>
>> >> away in their first inlining are omitted because their scope is not<br>
>> >> present so the variable is never created. Instead, we should ensure the<br>
>> >> scope is created so the variable can be added, even if it's been<br>
>> >> optimized away in its first inlining.<br>
>> >> This fixes the incorrect debug info in missing-abstract-variable.ll<br>
>> >> (added in r210143) and passes an asserts self-hosting build, so<br>
>> >> hopefully there's not more of these issues left behind... *fingers<br>
>> >> crossed*.<br>
>> >> Modified:<br>
>> >> llvm/trunk/include/llvm/CodeGen/LexicalScopes.h<br>
>> >> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
>> >> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h<br>
>> >> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp<br>
>> >> llvm/trunk/test/DebugInfo/missing-abstract-variable.ll<br>
><br>
><br>
</div></div></blockquote></div><br></div></div></div>