<div dir="ltr">> Continuing from above: in other words, since your use case is always<br>> setting the `null-is-unknown` bit, can you re-define @llvm.objectsize<br>> to have that semantic without adding a new parameter?<br>> <br>> That's not backward compatible, but there's a simple but conservative<br>> update strategy.<br><div class="gmail_extra"><br></div><div class="gmail_extra">That would almost definitely be the simplest way forward, but I'm unsure how happy this would make other users of objectsize. In particular, our objectsize sanitizer will no longer catch any null-related issues like:</div><div class="gmail_extra"><br></div><div class="gmail_extra">struct Foo { int a; };</div><div class="gmail_extra">int getA(struct Foo *f) { return f->a; }</div><div class="gmail_extra">int getA2() { struct Foo *f = 0; return f->a; }</div><div class="gmail_extra"><br></div><div class="gmail_extra">// getA2 now traps rather than calling a special ubsan-check-failed function</div><div class="gmail_extra"><br></div><div class="gmail_extra">Grepping through, that's the only other direct user of objectsize that I can find. If we're okay with that loss in accuracy (and don't believe that we have other objectsize users that care), I'm happy to just alter objectsize's default behavior and see how it does.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">> However, none of this matters if we're not in a position to change the<br>> specification.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Sadly, we're not. :/</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 2, 2017 at 11:45 AM, Sanjoy Das <span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.co<wbr>m</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi George,<br>
<span><br>
On Mon, Jan 2, 2017 at 10:41 AM, George Burgess IV<br>
<<a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a>> wrote:<br>
> Thanks for the comments!<br>
><br>
>> Have you considered changing our existing behavior to match GCC's<br>
>> builtin_object_size instead of adding a new parameter<br>
><br>
> Yup! My issue with turning `i1 %min` into `i8 %flags` is that<br>
> __builtin_object_size would get lowered like:<br>
><br>
> __builtin_object_size(p, 0) -> @llvm.objectsize(i8* %p, i8 2)<br>
> __builtin_object_size(p, 1) -> @llvm.objectsize(i8* %p, i8 2)<br>
> __builtin_object_size(p, 2) -> @llvm.objectsize(i8* %p, i8 3)<br>
> (__builtin_object_size(p, 3) doesn't actually get lowered)<br>
><br>
> ...Which might be a bit surprising to some people. If we think that's a<br>
> non-issue, I'm happy to take the simpler approach and use an i8.<br>
<br>
</span>What I had in mind was:<br>
<br>
 __builtin_object_size(p, 0) -> @llvm.objectsize(i8* %p, i1 0)<br>
 __builtin_object_size(p, 1) -> @llvm.objectsize(i8* %p, i1 0)<br>
 __builtin_object_size(p, 2) -> @llvm.objectsize(i8* %p, i1 1)<br>
 __builtin_object_size(p, 3) -> @llvm.objectsize(i8* %p, i1 1)<br>
<br>
and changing the specification of @llvm.objectsize to match.<br>
<span><br>
>> We'll have to have some <min>-awareness either in clang (to decide if<br>
>> the <null-is-unknown> bit needs to be set) or in the middle end.  What<br>
>> is your plan here?<br>
><br>
> My plan was just to always set the `null-is-unknown` bit when lowering a<br>
> call to __builtin_object_size in clang. If `min` is true, we treat unknown<br>
> and null values identically in @llvm.objectsize, so whether<br>
> `null-is-unknown` is set in that case shouldn't matter.<br>
<br>
</span>Continuing from above: in other words, since your use case is always<br>
setting the `null-is-unknown` bit, can you re-define @llvm.objectsize<br>
to have that semantic without adding a new parameter?<br>
<br>
That's not backward compatible, but there's a simple but conservative<br>
update strategy.<br>
<span><br>
>> However, since Malloc can return null:<br>
><br>
> I think I was unclear: this behavior would only exist if @llvm.objectsize<br>
> was actually handed null. I wasn't planning on changing how we'd handle<br>
> memory allocation functions that may return null (GCC gives back 2 for<br>
> __builtin_object_size(malloc(2<wbr>), 0)). In other words, this `null-is-unknown`<br>
> bit only makes the objectsize evaluator see `T* null` as though it was `call<br>
> T* @opaque_user_defined_function(<wbr>)`, nothing else.<br>
<br>
</span>I meant to say that it looks like the semantics of ObjectSizeMin(X) is<br>
that "return the conservative minimum object size for all values that<br>
X may take at runtime" (resp. ObjectSizeMax(X)).<br>
<br>
For instance this:<br>
<br>
void i(int c, volatile int* sink) {<br>
  void* mem1 = malloc(20);<br>
  void* mem2 = malloc(40);<br>
<br>
  *sink = __builtin_object_size(c ? mem1 : mem2, 0);<br>
  *sink = __builtin_object_size(c ? mem1 : mem2, 2);<br>
}<br>
<br>
is lowered to<br>
<br>
        movl    $40, (%rsi)<br>
        movl    $20, (%rsi)<br>
        ret<br>
<br>
by GCC.<br>
<br>
Applying the same logic to malloc(N), since it returns a location that<br>
has N dereferenceable bytes or NULL, it follows that<br>
ObjectSizeMin(malloc(N)) should return the smaller of<br>
ObjectSizeMin(NULL) and ObjectSizeMin(MemoryLocOfNByte<wbr>s) == the<br>
smaller of ObjectSizeMin(NULL) and N.  Given that we want the result<br>
of ObjectSizeMin(malloc(N)) == UMIN(ObjectSizeMin(NULL), N) to be N,<br>
we'd want ObjectSizeMin(NULL) to be (unsigned)-1 for consistency.<br>
<br>
However, none of this matters if we're not in a position to change the<br>
specification.<br>
<span class="m_3661850945630791323m_-3772949128988401680m_6892711880939968507m_498093261494322810HOEnZb"><font color="#888888"><br>
-- Sanjoy<br>
</font></span><div class="m_3661850945630791323m_-3772949128988401680m_6892711880939968507m_498093261494322810HOEnZb"><div class="m_3661850945630791323m_-3772949128988401680m_6892711880939968507m_498093261494322810h5"><br>
<br>
> This is also consistent with how @llvm.objectsize already acts: if the<br>
> pointer it's given is the result of a call to `malloc(N)`, it'll return N<br>
> regardless of the value of `min`.<br>
><br>
> But yeah, I agree that some of the features of __builtin_object_size aren't<br>
> entirely intuitive. :)<br>
><br>
> On Mon, Jan 2, 2017 at 12:01 AM, Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.co<wbr>m</a>><br>
> wrote:<br>
>><br>
>> Hi George,<br>
>><br>
>> Have you considered changing our existing behavior to match GCC's<br>
>> builtin_object_size instead of adding a new parameter?  That may be<br>
>> simpler overall.  There's also a clear upgrade strategy -- fold every<br>
>> old style call to "<min> ? 0 : 1".<br>
>><br>
>> You probably already know this, but GCC folds<br>
>> builtin_object_size(0, 0) to -1 and builtin_object_size(0, 2) to 0.<br>
>> We'll have to have some <min>-awareness either in clang (to decide if<br>
>> the <null-is-unknown> bit needs to be set) or in the middle end.  What<br>
>> is your plan here?<br>
>><br>
>> I also found gcc's choice of folding builtin_object_size(0, 2) to 0 and<br>
>> builtin_object_size(0, 0) to -1 somewhat odd; I'd have expected the<br>
>> inverse.  This follows from the following "intuitive" rules<br>
>><br>
>> ObjSizeMax(X) = UMAX(ObjSizeMax(A), ObjSizeMax(B))<br>
>> ObjSizeMin(X) = UMIN(ObjSizeMin(A), ObjSizeMin(B))<br>
>><br>
>> (where X is a value that can either be A or B at runtime)<br>
>><br>
>> and that we want to fold<br>
>><br>
>> ObjSizeMax(Malloc(N)) = ObjSizeMin(Malloc(N)) = N<br>
>><br>
>> However, since Malloc can return null:<br>
>><br>
>> ObjSizeMax(Malloc(N)) = UMAX(N, ObjSizeMax(NULL)) = N<br>
>> ObjSizeMin(Malloc(N)) = UMIN(N, ObjSizeMin(NULL)) = N<br>
>><br>
>> and for that to be true ObjSizeMax(NULL) =<br>
>> builtin_object_size(NULL, 0) needs to be 0 and ObjSizeMin(NULL) =<br>
>> builtin_object_size(NULL, 2) needs to be (unsigned)-1.<br>
>><br>
>> However, I'm not sure if it is up to us to change that; given the very<br>
>> motivation of this thread is GCC compatibility.<br>
>><br>
>> -- Sanjoy<br>
>><br>
>> On Sun, Jan 1, 2017 at 10:03 PM, George Burgess IV via llvm-dev<br>
>> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> > Happy New Year ping. :)<br>
>> ><br>
>> > Will ping again on Wednesday. If I don't get comments by EOD Thursday,<br>
>> > I'll<br>
>> > assume everyone's OK with this and put together a patch.<br>
>> ><br>
>> > On Wed, Dec 21, 2016 at 11:44 AM, George Burgess IV<br>
>> > <<a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a>> wrote:<br>
>> >><br>
>> >> tl;dr: We'd like to add a bit to @llvm.objectsize to make it optionally<br>
>> >> be<br>
>> >> conservative when it's handed a null pointer.<br>
>> >><br>
>> >> Happy Wednesday!<br>
>> >><br>
>> >> We're trying to fix PR23277, which is a bug about how clang+LLVM treat<br>
>> >> __builtin_object_size when it's given a null pointer. For compatibility<br>
>> >> with<br>
>> >> GCC, clang would like to be able to hand back a conservative result<br>
>> >> (e.g.<br>
>> >> (size_t)-1), but the LLVM intrinsic that clang lowers<br>
>> >> __builtin_object_size(p, N) to, @llvm.objectsize, only hands back 0<br>
>> >> when<br>
>> >> given a null pointer. Because it's often assumed that<br>
>> >> __builtin_object_size<br>
>> >> always folds to a constant, handling this only in clang may be tricky:<br>
>> >> if we<br>
>> >> emit the IR equivalent to ((p) ? __builtin_object_size(p, N) : -1ULL)<br>
>> >> and<br>
>> >> LLVM can't fold away the null check, we've failed to emit a constant.<br>
>> >><br>
>> >> So, the best path forward that I can see is to add a "null is unknown<br>
>> >> size" bit to @llvm.objectsize, much like the "min" bit it already has.<br>
>> >> If<br>
>> >> this bit is true, null would be treated like an opaque pointer.<br>
>> >> Otherwise,<br>
>> >> @llvm.objectsize would act no differently than it does today.<br>
>> >><br>
>> >> If we like this approach, I'm assuming it would be best to have this<br>
>> >> bit<br>
>> >> as a third argument to @llvm.objectsize, rather than making the second<br>
>> >> argument an i8 and using it as a bag of bits.<br>
>> >><br>
>> >> All thoughts/comments/alternative approaches/etc. highly appreciated.<br>
>> >> :)<br>
>> >><br>
>> >> Thanks (and Happy Holidays)!<br>
>> >> George<br>
>> ><br>
>> ><br>
>> ><br>
>> > ______________________________<wbr>_________________<br>
>> > LLVM Developers mailing list<br>
>> > <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
>> ><br>
>><br>
>><br>
>><br>
>> --<br>
>> Sanjoy Das<br>
>> <a href="http://playingwithpointers.com" rel="noreferrer" target="_blank">http://playingwithpointers.com</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>