[llvm] r254794 - CodeGen: Move the SlotIndexes BumpPtrAllocator before the list it allocates

[Justin Bogner via llvm-commits]

Sean Silva <chisophugis at gmail.com> writes:
> Theoretically ASan can catch this, right? Any idea what we weren't tripping
> ASan? It seems like this use-after-free would be consistent.

I guess this is a false-negative / missing feature in ASan. I did play
around to see if I could get ASan to catch it, and even tried Pete's
patch to teach ASan to work with BumpPtrAllocator in llvm.org/pr23516 to
see if that caught it, but I couldn't get anything to notice the bug.

kcc, samsonov: Is this a know limitation of ASan, or possibly a
reasonable feature request?

> -- Sean Silva
>
> On Fri, Dec 4, 2015 at 3:00 PM, Justin Bogner via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: bogner
>> Date: Fri Dec  4 17:00:54 2015
>> New Revision: 254794
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=254794&view=rev
>> Log:
>> CodeGen: Move the SlotIndexes BumpPtrAllocator before the list it allocates
>>
>> When a `SlotIndexes` is destroyed, `ileAllocator` will currently be
>> destructed before `IndexList`, but all of `IndexList`'s storage has
>> been allocated by `ileAllocator`. This means we'll call destructors on
>> garbage data, which is very bad. This can be avoided by putting the
>> BumpPtrAllocator earlier in the class than anything it allocates.
>>
>> Unfortunately, I don't know how to test this. It depends very much on
>> memory layout, and the only evidence I have that this is actually
>> happening in practice are backtraces that might be explained by this.
>> By inspection though, the code is obviously dangerous/wrong, and this
>> is the right thing to do.
>>
>> I'll follow up later with a patch that calls clearAndLeakNodesUnsafely
>> on the list, since there isn't much point in destructing them when
>> they're allocated in a BPA anyway, but I figured it makes sense to
>> commit the correctness fix separately from that optimization.
>>
>> Modified:
>>     llvm/trunk/include/llvm/CodeGen/SlotIndexes.h
>>
>> Modified: llvm/trunk/include/llvm/CodeGen/SlotIndexes.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SlotIndexes.h?rev=254794&r1=254793&r2=254794&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/CodeGen/SlotIndexes.h (original)
>> +++ llvm/trunk/include/llvm/CodeGen/SlotIndexes.h Fri Dec  4 17:00:54 2015
>> @@ -333,6 +333,8 @@ namespace llvm {
>>    /// This pass assigns indexes to each instruction.
>>    class SlotIndexes : public MachineFunctionPass {
>>    private:
>> +    // IndexListEntry allocator.
>> +    BumpPtrAllocator ileAllocator;
>>
>>      typedef ilist<IndexListEntry> IndexList;
>>      IndexList indexList;
>> @@ -353,9 +355,6 @@ namespace llvm {
>>      /// and MBB id.
>>      SmallVector<IdxMBBPair, 8> idx2MBBMap;
>>
>> -    // IndexListEntry allocator.
>> -    BumpPtrAllocator ileAllocator;
>> -
>>      IndexListEntry* createEntry(MachineInstr *mi, unsigned index) {
>>        IndexListEntry *entry =
>>          static_cast<IndexListEntry*>(
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>