[PATCH] Fix non-determinism causing assertion failure (PR22750)

[Dario Domizioli]

Rebased patch with added comment and without test.


Cheers,
    Dario Domizioli
    SN Systems - Sony Computer Entertainment Group





On 3 March 2015 at 18:22, Dario Domizioli <dario.domizioli at gmail.com> wrote:

> Thanks David!
>
> The test case is actually pretty reliable (I'm surprised myself). Five
> scopes and twenty-five declarations seem to be enough to trigger the
> unstable sort behaviour, at least with the std::sort in glibc.
> Still, it is a maintenance burden if the metadata format changes, so I
> defer to your judgement.
>
> Would a comment like this be OK?
>
>     // Stable sort to preserve the order of appearance of imported
> entities.
>     // This is to avoid out-of-order processing of interdependent
> declarations
>     // within the same scope, e.g. { namespace A = base; namespace B = A; }
>
> (I'm rebasing the patch and rebuilding at the moment)
>
> Cheers,
>     Dario Domizioli
>     SN Systems - Sony Computer Entertainment Group
>
>
>
>
> On 3 March 2015 at 17:33, David Blaikie <dblaikie at gmail.com> wrote:
>
>> I think this sounds reasonable - though, yeah, it is perhaps a bit subtle
>> that the order of entries in the imported entities list must be such that
>> there are no forward references. I'm kind of OK with that.
>>
>> A comment on the sort explaining why it needs to be stable wouldn't go
>> astray.
>>
>> & I'd probably skip the test case - testing for non-determinism is a bit
>> of a lost cause. We regularly fix non-determinism bugs without
>> corresponding test cases.
>>
>> On Tue, Mar 3, 2015 at 8:13 AM, Dario Domizioli <
>> dario.domizioli at gmail.com> wrote:
>>
>>> Hello LLVM,
>>>
>>> I have found and filed PR22750, and I've looked into fixing it.
>>>
>>> Basically, it's an assertion failure during debug info generation, and
>>> its cause is the interaction of two factors:
>>> 1) When generating a DW_TAG_imported_declaration DIE which imports
>>> another imported declaration, the code in
>>> lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp asserts that the second
>>> imported declaration must already have a DIE (line 661).
>>> 2) There is a non-determinism in the order in which imported
>>> declarations within the same scope are processed.
>>>
>>> Because of the non-determinism (2), it is possible that an imported
>>> declaration is processed before another one it depends on, breaking the
>>> assumption in (1).
>>>
>>> I have tracked down the source of the non-determinism.
>>> The imported declaration DIDescriptors are sorted by scope in
>>> DwarfDebug::beginModule(), in lib/CodeGen/AsmPrinter/DwarfDebug.cpp:480;
>>> however that sort is not a stable_sort, therefore the order of the
>>> declarations within the same scope is not preserved.
>>>
>>> The attached patch changes the std::sort to a std::stable_sort and it
>>> fixes the problem.
>>>
>>> I'm not 100% sure this is the right solution, even if it works. My
>>> question is: would it be better to relax the assumption in (1), e.g. by
>>> making the code create the dependency DIE on demand?
>>>
>>> Cheers,
>>>     Dario Domizioli
>>>     SN Systems - Sony Computer Entertainment Group
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150303/ef0a43b3/attachment.html>
-------------- next part --------------
Index: lib/CodeGen/AsmPrinter/DwarfDebug.cpp
===================================================================
--- lib/CodeGen/AsmPrinter/DwarfDebug.cpp	(revision 231088)
+++ lib/CodeGen/AsmPrinter/DwarfDebug.cpp	(working copy)
@@ -477,8 +477,11 @@
       ScopesWithImportedEntities.push_back(std::make_pair(
           DIImportedEntity(ImportedEntities.getElement(i)).getContext(),
           ImportedEntities.getElement(i)));
-    std::sort(ScopesWithImportedEntities.begin(),
-              ScopesWithImportedEntities.end(), less_first());
+    // Stable sort to preserve the order of appearance of imported entities.
+    // This is to avoid out-of-order processing of interdependent declarations
+    // within the same scope, e.g. { namespace A = base; namespace B = A; }
+    std::stable_sort(ScopesWithImportedEntities.begin(),
+                     ScopesWithImportedEntities.end(), less_first());
     DIArray GVs = CUNode.getGlobalVariables();
     for (unsigned i = 0, e = GVs.getNumElements(); i != e; ++i)
       CU.getOrCreateGlobalVariableDIE(DIGlobalVariable(GVs.getElement(i)));