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

[Dario Domizioli]

Thanks!

I have committed r231100.




On 3 March 2015 at 18:35, David Blaikie <dblaikie at gmail.com> wrote:

> 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/6f2f609d/attachment.html>