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

[David Blaikie]

Great, please commit!

On Tue, Mar 3, 2015 at 10:29 AM, Dario Domizioli <dario.domizioli at gmail.com>
wrote:

> 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/309bee47/attachment.html>