[llvm-dev] UBSan, StringRef and Allocator.h

Philip Reames via llvm-dev llvm-dev at lists.llvm.org
Wed Mar 30 13:03:12 PDT 2016



On 03/30/2016 10:42 AM, Pete Cooper wrote:
>
>> On Mar 29, 2016, at 4:45 PM, Philip Reames <listmail at philipreames.com 
>> <mailto:listmail at philipreames.com>> wrote:
>>
>>
>>
>> On 03/29/2016 01:59 PM, Pete Cooper via llvm-dev wrote:
>>>
>>>> On Mar 28, 2016, at 6:02 PM, Pete Cooper via llvm-dev 
>>>> <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>>>
>>>>>
>>>>> On Mar 28, 2016, at 3:12 PM, Chandler Carruth 
>>>>> <chandlerc at google.com> wrote:
>>>>>
>>>>> FWIW, I agree with Mehdi that we should just assert that our types 
>>>>> don't get called with size zero.
>>>> Yeah, I agree.  I’ve tried running the tests with the assert in 
>>>> place and there’s about 1000 failures across llvm/clang.  I’ll see 
>>>> what I can fix as I would like to get these to behave.  There may 
>>>> be actual logic errors in some of these cases.
>>> Well this is fun.  There is a long standing bug in the SelectionDAG 
>>> tablegen matcher this has uncovered.  Seems that ComplexPattern’s 
>>> don’t track MemRefs.  It probably (hopefully!) just causes 
>>> downstream code to be conservative but its possible there’s real 
>>> issues being hidden by this.
>>>
>>> The problematic code is something like ‘xop(load) -> X86XOPrm’ where 
>>> the resulting X86XOP MachineInstr wouldn’t have an 
>>> MachineMemOperand’s, despite being marked as loading from memory.
>>>
>>> Will file a PR so that we can track some of these issues.
>> Coming late to this discussion since I just noticed this was talking 
>> about MemRefs...
>>
>> We have deep and horrible problems around our handling of 
>> MemoryOperands.  As it stands, there does not appear to be a single 
>> invariant which is documented and accepted throughout the various 
>> backends.
> Yeah, i’ve been noticing the same thing :(
>> There are conflicting assumptions in various bits of code.  I tried 
>> to clean up some of this a while back and made some forward progress, 
>> but there's a lot of work left to do here.
> Yeah, the piece I was seeing seemed local to the DAG matcher and 
> complex patterns, until I realized that fixing it probably requires 
> changing all of the complex pattern matchers which need to handle 
> MemRefs to actually return the list of MemRefs they are interested in. 
>  This would impact pretty much every backend so isn’t something I want 
> to suggest without thinking of less invasive alternatives.
If you want to chat about this, I'm happy to take some time on IRC, 
phone, or in person.  I don't have the time to devote to fixing this, 
but I'm happy to help strategize.
>
> Cheers,
> Pete
>>>
>>> Pete
>>>>>
>>>>> That said, I don't think we can be terribly cavalier with what we 
>>>>> expect from standard allocator types, operator new, or malloc. And 
>>>>> so I would expect LLVM_ATTRIBUTE_RETURNS_NOALIAS to not imply 
>>>>> NONNULL, and while it seems reasonable to put NONNULL on *our* 
>>>>> allocate function because of the assert and the fact that we 
>>>>> assume the underlying allocation routine never produces a null, it 
>>>>> doesn't seem reasonable for any old function with NOALIAS to have 
>>>>> NONNULL inferred.
>>>> I agree.  I don’t actually know if NOALIAS and NONNULL were put on 
>>>> this function independently, or if the committer assumed one 
>>>> implied the other.  But I do agree that they should be independent 
>>>> in general, even if they happen to both apply here.
>>>>
>>>> Cheers,
>>>> Pete
>>>>>
>>>>> -Chandler
>>>>>
>>>>> On Tue, Mar 22, 2016 at 9:18 PM Mehdi Amini via llvm-dev 
>>>>> <llvm-dev at lists.llvm.org> wrote:
>>>>>
>>>>>>     On Mar 22, 2016, at 5:39 PM, Pete Cooper via llvm-dev
>>>>>>     <llvm-dev at lists.llvm.org> wrote:
>>>>>>
>>>>>>>
>>>>>>>     On Mar 22, 2016, at 5:35 PM, David Blaikie
>>>>>>>     <dblaikie at gmail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>     On Tue, Mar 22, 2016 at 5:30 PM, Pete
>>>>>>>     Cooper<peter_cooper at apple.com>wrote:
>>>>>>>
>>>>>>>         Hi all
>>>>>>>
>>>>>>>         (No idea if I have the correct audience. Please CC more
>>>>>>>         people as needed).
>>>>>>>
>>>>>>>         I have an UBSan failure in BumpPtrAllocatorImpl.Allocate.
>>>>>>>
>>>>>>>         The problem is that lld requests that we StringRef::copy
>>>>>>>         an empty string. This passes a length of 0 to a
>>>>>>>         BumpPtrAllocator. The BumpPtrAllocator happened to not
>>>>>>>         have anything allocated yet so the CurPtr is nullptr,
>>>>>>>         but given that we need 0 space we think we have enough
>>>>>>>         space and return an allocation of size 0 at address
>>>>>>>         nullptr.  This therefore returns nullptr from Allocate,
>>>>>>>         but that method is marked
>>>>>>>         with LLVM_ATTRIBUTE_RETURNS_NONNULL
>>>>>>>         and LLVM_ATTRIBUTE_RETURNS_NOALIAS, both of which aren’t
>>>>>>>         true in this case.
>>>>>>>
>>>>>>>         To put this in code, if I have
>>>>>>>
>>>>>>>             BumpPtrAllocator allocator;
>>>>>>>             StringRef s;
>>>>>>>             s.copy(allocator);
>>>>>>>
>>>>>>>
>>>>>>>         then i’m going to allocate 0 bytes in the allocator and
>>>>>>>         get a StringRef(nullptr, 0).  Its a valid StringRef, but
>>>>>>>         an UBSan failures in the allocator.
>>>>>>>
>>>>>>>         Lang and I looked up malloc behaviour online as this is
>>>>>>>         fairly analogous. The answer there is that you are
>>>>>>>         allowed to return nullptr, or not, its implementation
>>>>>>>         defined.  So no help there.
>>>>>>>
>>>>>>>         So the question is, how do we want this to behave in our
>>>>>>>         code?
>>>>>>>
>>>>>>>         Some options:
>>>>>>>         - Assert that Allocate never gets a size 0 allocation. 
>>>>>>>         So fix StringRef::copy to see this case
>>>>>>>         - Remove the attributes from Allocate but continue to
>>>>>>>         return nullptr (or base of the allocator?) in this case
>>>>>>>         - Keep the attributes on Allocate and treat size 0
>>>>>>>         allocations as size 1
>>>>>>>
>>>>>>>
>>>>>>>     I believe the last is closer to 'new's behavior - which I
>>>>>>>     think returns a unique non-null address (well, unique
>>>>>>>     amongst current allocations - can be recycled once deleted)
>>>>>>>     if I recall correctly.
>>>>>>     That’s what I would have expected too.  Its like
>>>>>>     sizeof(struct {}) which can be a 1 depending on factors we
>>>>>>     don’t need to get in to here.
>>>>>
>>>>>     Well except that if sizeof(struct{}) is 1, the allocator is
>>>>>     never called with a 0.
>>>>>
>>>>>     I would consider forbidding zero sized allocation in the
>>>>>     allocator (assert()) by design (hey we're controlling every
>>>>>     possible uses!), unless there is a real use-case for that.
>>>>>
>>>>>     This would also be in line with the C++ standard requirement
>>>>>     for allocator which specifies that the result of
>>>>>     "a.allocate(0)" is unspecified (ref: C++14 Table 28 —
>>>>>     Allocator requirements).
>>>>>
>>>>>     -- 
>>>>>     Mehdi
>>>>>
>>>>>
>>>>>>>     Can check for wording if that's helpful/desired.
>>>>>>     No need for my benefit :)  I’m in agreement that this is a
>>>>>>     good behavior to go for, but will leave it to others to say
>>>>>>     if they’d like the extra detail.
>>>>>>
>>>>>>     One thing I did forget to say is that I’d like to fix
>>>>>>     StringRef::copy in all of the above cases.  I think that this
>>>>>>     method should always avoid the allocator and return
>>>>>>     StringRef(nullptr, 0) when length is 0.  I’ll get a patch up
>>>>>>     on llvm-commits if there’s no objections there.
>>>>>>
>>>>>>     Thanks,
>>>>>>     Pete
>>>>>>>
>>>>>>>
>>>>>>>         Thanks,
>>>>>>>         Pete
>>>>>>>
>>>>>>
>>>>>>     _______________________________________________
>>>>>>     LLVM Developers mailing list
>>>>>>     llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>>>>>>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>     _______________________________________________
>>>>>     LLVM Developers mailing list
>>>>>     llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>>>>>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160330/8d5441fc/attachment.html>


More information about the llvm-dev mailing list