Fwd: [llvm] r221709 - Totally forget deallocated SDNodes in SDDbgInfo.
David Blaikie
dblaikie at gmail.com
Wed Nov 12 12:50:43 PST 2014
(oops, forgot to reply-all)
---------- Forwarded message ----------
From: David Blaikie <dblaikie at gmail.com>
Date: Wed, Nov 12, 2014 at 12:50 PM
Subject: Re: [llvm] r221709 - Totally forget deallocated SDNodes in
SDDbgInfo.
To: Frédéric Riss <friss at apple.com>
On Wed, Nov 12, 2014 at 11:16 AM, Frédéric Riss <friss at apple.com> wrote:
>
> On Nov 12, 2014, at 11:02 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Nov 11, 2014 at 8:57 PM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> On Nov 11, 2014, at 2:26 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Tue, Nov 11, 2014 at 2:16 PM, Frédéric Riss <friss at apple.com> wrote:
>>
>>>
>>> On Nov 11, 2014, at 1:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Tue, Nov 11, 2014 at 1:21 PM, Frederic Riss <friss at apple.com> wrote:
>>>
>>>> Author: friss
>>>> Date: Tue Nov 11 15:21:08 2014
>>>> New Revision: 221709
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=221709&view=rev
>>>> Log:
>>>> Totally forget deallocated SDNodes in SDDbgInfo.
>>>>
>>>> What would happen before that commit is that the SDDbgValues associated
>>>> with
>>>> a deallocated SDNode would be marked Invalidated, but SDDbgInfo would
>>>> keep
>>>> a map entry keyed by the SDNode pointer pointing to this list of
>>>> invalidated
>>>> SDDbgNodes. As the memory gets reused, the list might get wrongly
>>>> associated
>>>> with another new SDNode. As the SDDbgValues are cloned when they are
>>>> transfered,
>>>> this can lead to an exponential number of SDDbgValues being produced
>>>> during
>>>> DAGCombine like in http://llvm.org/bugs/show_bug.cgi?id=20893
>>>>
>>>> Note that the previous behavior wasn't really buggy as the invalidation
>>>> made
>>>> sure that the SDDbgValues won't be used. This commit can be considered a
>>>> memory optimization and as such is really hard to validate in a
>>>> unit-test.
>>>>
>>>
>>> This was sort-of undefined behavior previously (comparing a previously
>>> free'd pointer to a new allocation) but is still sort-of detectable. If we
>>> added an assertion into some point of initial construction or SDNode usage,
>>> that there were zero valuse in its map - then this assertion would fire on
>>> the buggy code? (but only when the allocation got lucky and put it in the
>>> same place) But would at least fail rather than behave strangely (leakily,
>>> mind you, but strangely)
>>>
>>> I imagine at the call to setHasDebugValue, if it previously had
>>> !getHasDebugValue, then the number of entries in the map should be 1 (this
>>> should be the first thing we've added to the map), something like changing
>>> this:
>>>
>>> DbgInfo->add(DB, SD, isParameter); if (SD) SD->setHasDebugValue(true);
>>>
>>>
>>> to this:
>>>
>>>
>>> if (SD) { assert(DbgInfo->getSDDbgValues(SD).empty() || !SD->getHasDebugValue());
>>> SD->setHasDebugValue(true);
>>> }
>>> DbgInfo->add(DB, SD, isParameter);
>>> I agree that adding a test case may still be questionable, given the need for this to end up getting a matching allocation, but maybe there are some simple cases that pretty obviously get the same memory address because they're back-to-back release-then-reallocate? (I think I added a test case like this when I fixed some similar bugs in ArgPromo or DAE, maybe... one of them has a two-pass algorithm that had some invalidity going on - http://llvm.org/viewvc/llvm-project?rev=219210&view=rev )
>>>
>>>
>>> Sorry, I missed that part of the mail. With the assert in place, it
>>> might be possible to come up with an IR that reproduces this behavior, but
>>> as it relies on allocation order it would be quite fragile I suppose (Note
>>> that I do know absolutely nothing about SDAG allocation strategy). I’ll try
>>> to toy with it when I find some time, but no promises…
>>>
>>
>> Yep - see my example from ArgPromo - in that case an easy example existed
>> due to the deallocation and immediate reallocation (of course this test
>> case won't always exercise the problematic case - different memory
>> allocators could easily defeat it - but it seems at least simple/plausible
>> enough to include (adding appropriate comments & caveats to the test case
>> in case it entirely rots, so someone knows they can remove it in that case))
>>
>> I'd expect the case where the allocation is reused is a fairly simple one
>> like the ArgPromo issue I hit, but if it doesn't fall out so easily,
>> certainly don't spend a lot of time on it. Though I wouldn't mind if you
>> looked at it sooner rather than later - at least enough to decide one way
>> or the other. Unspecified times in the future are easily forgotten (but you
>> know, doesn't have to be today - but within a week or so seems reasonable,
>> beyond that and I imagine it'll get lost).
>>
>>
>> So I came up with a small example that triggers the assert on the bus
>> home. I’m still wondering if it’s worth committing. I think the test is
>> fragile not because of the allocation strategy (SelectionDAG uses a
>> Recycling allocator which is fairly predictable), but because it depends
>> really a lot on the ISel/Combiner behavior in itself, and I suppose this is
>> much more prone to change. Moreover it would be a test with just a RUN
>> line, testing that it doesn’t crash which are usually frowned upon.
>>
>
> It's usually frowned upon (usually by me - I don't see anyone else much
> caring) because it represents a lack of test coverage (program was crashing
> on some codepath, now it's not crashing - but presumably there was/is
> specific behavior other than "anything other than crash" that we were
> expecting) - as you say, other codepaths ensured that this bad behavior
> didn't leak out anywhere, but we could still test the output on this
> codepath to make sure it's the obviously right thing.
>
>
>> That said I’m happy to commit it along with the assert if you think it’s
>> worth it.
>>
>
> Either way, the assertion's probably worth adding I imagine.
>
>
> No questions, the assertion will go in anyway.
>
> I wouldn't mind at least seeing the test case - either in code review/just
> an attached patch, or committed (though do include your comment about what
> makes this test brittle/likely to become invalid later - and if you do
> commit it, the commit message should reference the revision with the fix so
> that should someone wonder if the test is still meaningful, they can
> locally revert the fix and see if the test still fails)
>
>
> I haven’t turned it into a FileCheck test, but this is the base code:
>
> #include <stdint.h>
>
> int foo(int a) {
> int b = (int16_t)a + 8;
> int c = (int16_t)b + 8;
> int d = (int16_t)c + 8;
> int e = (int16_t)d + 8;
> int f = (int16_t)e + 8;
> return f;
> }
>
> I’d then do:
> $ ~/llvm/build_ninja/bin/opt -mem2reg repro.ll -S -o repro.opt.ll
>
> to get the real test file, which crashes the compiler (with the assert but
> without the fix obviously) when doing:
>
> $ ~/llvm/build_ninja/bin/llc repro.opt.ll -O2
>
> (not sure if the -O2 is necessary)
> The thing that makes this very prone to get to a
> not-testing-anything-at-all state is that the combiner is required not only
> to combine nodes (this gives you node deletion), but also to create nodes
> (this is the purpose of the narrowing casts, they’ll enable code paths
> where the combiner tries various widening options). If you remove one of
> the additions in the chain, then the assert doesn’t trigger because the
> memory doesn’t get reused by the “right” nodes.
>
> WDYT?
>
I'm glad we've got the test recorded here/demonstrating the failure -
you're right that it's a bit brittle (though I suspect not /too/ bad).
If I were in your shoes I'd probably commit the test (I'd check if the -O2
is necessary and/or figure out what specific flags llc needs to reproduce
the issue (& apply any other transformations ahead of time to the committed
.ll file)) but it's marginal - up to you whether you want to commit it or
not. It'll be here in the email history otherwise.
Thanks for your patience,
- David
>
> Fred
>
>
> - David
>
>
>>
>> Fred
>>
>> - David
>>
>>
>>>
>>> Fred
>>>
>>>
>>>> Modified:
>>>> llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
>>>> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>>>
>>>> Modified: llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SelectionDAG.h?rev=221709&r1=221708&r2=221709&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/CodeGen/SelectionDAG.h (original)
>>>> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Tue Nov 11 15:21:08
>>>> 2014
>>>> @@ -128,6 +128,10 @@ public:
>>>> DbgValMap[Node].push_back(V);
>>>> }
>>>>
>>>> + /// \brief Invalidate all DbgValues attached to the node and remove
>>>> + /// it from the Node-to-DbgValues map.
>>>> + void erase(const SDNode *Node);
>>>> +
>>>> void clear() {
>>>> DbgValMap.clear();
>>>> DbgValues.clear();
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp?rev=221709&r1=221708&r2=221709&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Tue Nov 11
>>>> 15:21:08 2014
>>>> @@ -687,6 +687,15 @@ void SelectionDAG::DeleteNodeNotInCSEMap
>>>> DeallocateNode(N);
>>>> }
>>>>
>>>> +void SDDbgInfo::erase(const SDNode *Node) {
>>>> + DbgValMapType::iterator I = DbgValMap.find(Node);
>>>> + if (I == DbgValMap.end())
>>>> + return;
>>>> + for (auto &Val: I->second)
>>>> + Val->setIsInvalidated();
>>>> + DbgValMap.erase(I);
>>>> +}
>>>> +
>>>> void SelectionDAG::DeallocateNode(SDNode *N) {
>>>> if (N->OperandsNeedDelete)
>>>> delete[] N->OperandList;
>>>> @@ -697,10 +706,9 @@ void SelectionDAG::DeallocateNode(SDNode
>>>>
>>>> NodeAllocator.Deallocate(AllNodes.remove(N));
>>>>
>>>> - // If any of the SDDbgValue nodes refer to this SDNode, invalidate
>>>> them.
>>>> - ArrayRef<SDDbgValue*> DbgVals = DbgInfo->getSDDbgValues(N);
>>>> - for (unsigned i = 0, e = DbgVals.size(); i != e; ++i)
>>>> - DbgVals[i]->setIsInvalidated();
>>>> + // If any of the SDDbgValue nodes refer to this SDNode, invalidate
>>>> + // them and forget about that node.
>>>> + DbgInfo->erase(N);
>>>> }
>>>>
>>>> #ifndef NDEBUG
>>>>
>>>>
>>>> _______________________________________________
>>>> 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/20141112/6576a326/attachment.html>
More information about the llvm-commits
mailing list