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