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

Tue Mar 29 13:59:52 PDT 2016

> On Mar 28, 2016, at 6:02 PM, Pete Cooper via llvm-dev <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.

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 <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 <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 <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/4906b792/attachment.html>