[llvm-dev] UBSan, StringRef and Allocator.h
Philip Reames via llvm-dev
llvm-dev at lists.llvm.org
Tue Mar 29 16:45:42 PDT 2016
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
>>> <mailto: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. 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.
>
> 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 <mailto: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 <mailto:llvm-dev at lists.llvm.org>> wrote:
>>>>
>>>>>
>>>>> On Mar 22, 2016, at 5:35 PM, David Blaikie <dblaikie at gmail.com
>>>>> <mailto:dblaikie at gmail.com>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Mar 22, 2016 at 5:30 PM, Pete
>>>>> Cooper<peter_cooper at apple.com
>>>>> <mailto: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/20160329/f981fbfe/attachment.html>
More information about the llvm-dev
mailing list