[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