RFC: Allowing @llvm.objectsize to be more conservative with null.

tl;dr: We’d like to add a bit to @llvm.objectsize to make it optionally be conservative when it’s handed a null pointer.

Happy Wednesday!

We’re trying to fix PR23277, which is a bug about how clang+LLVM treat __builtin_object_size when it’s given a null pointer. For compatibility with GCC, clang would like to be able to hand back a conservative result (e.g. (size_t)-1), but the LLVM intrinsic that clang lowers __builtin_object_size(p, N) to, @llvm.objectsize, only hands back 0 when given a null pointer. Because it’s often assumed that __builtin_object_size always folds to a constant, handling this only in clang may be tricky: if we emit the IR equivalent to ((p) ? __builtin_object_size(p, N) : -1ULL) and LLVM can’t fold away the null check, we’ve failed to emit a constant.

So, the best path forward that I can see is to add a “null is unknown size” bit to @llvm.objectsize, much like the “min” bit it already has. If this bit is true, null would be treated like an opaque pointer. Otherwise, @llvm.objectsize would act no differently than it does today.

If we like this approach, I’m assuming it would be best to have this bit as a third argument to @llvm.objectsize, rather than making the second argument an i8 and using it as a bag of bits.

All thoughts/comments/alternative approaches/etc. highly appreciated. :slight_smile:

Thanks (and Happy Holidays)!
George

Happy New Year ping. :slight_smile:

Will ping again on Wednesday. If I don’t get comments by EOD Thursday, I’ll assume everyone’s OK with this and put together a patch.

Hi George,

Have you considered changing our existing behavior to match GCC's
builtin_object_size instead of adding a new parameter? That may be
simpler overall. There's also a clear upgrade strategy -- fold every
old style call to "<min> ? 0 : 1".

You probably already know this, but GCC folds
builtin_object_size(0, 0) to -1 and builtin_object_size(0, 2) to 0.
We'll have to have some <min>-awareness either in clang (to decide if
the <null-is-unknown> bit needs to be set) or in the middle end. What
is your plan here?

I also found gcc's choice of folding builtin_object_size(0, 2) to 0 and
builtin_object_size(0, 0) to -1 somewhat odd; I'd have expected the
inverse. This follows from the following "intuitive" rules

ObjSizeMax(X) = UMAX(ObjSizeMax(A), ObjSizeMax(B))
ObjSizeMin(X) = UMIN(ObjSizeMin(A), ObjSizeMin(B))

(where X is a value that can either be A or B at runtime)

and that we want to fold

ObjSizeMax(Malloc(N)) = ObjSizeMin(Malloc(N)) = N

However, since Malloc can return null:

ObjSizeMax(Malloc(N)) = UMAX(N, ObjSizeMax(NULL)) = N
ObjSizeMin(Malloc(N)) = UMIN(N, ObjSizeMin(NULL)) = N

and for that to be true ObjSizeMax(NULL) =
builtin_object_size(NULL, 0) needs to be 0 and ObjSizeMin(NULL) =
builtin_object_size(NULL, 2) needs to be (unsigned)-1.

However, I'm not sure if it is up to us to change that; given the very
motivation of this thread is GCC compatibility.

-- Sanjoy

Thanks for the comments!> builtin_object_size instead of adding a new parameter

Yup! My issue with turning i1 %min into i8 %flags is that __builtin_object_size would get lowered like:

__builtin_object_size(p, 0) → @llvm.objectsize(i8* %p, i8 2)
__builtin_object_size(p, 1) → @llvm.objectsize(i8* %p, i8 2)
__builtin_object_size(p, 2) → @llvm.objectsize(i8* %p, i8 3)
(__builtin_object_size(p, 3) doesn’t actually get lowered)

…Which might be a bit surprising to some people. If we think that’s a non-issue, I’m happy to take the simpler approach and use an i8.

Hi George,

Thanks for the comments!

Have you considered changing our existing behavior to match GCC's
builtin_object_size instead of adding a new parameter

Yup! My issue with turning `i1 %min` into `i8 %flags` is that
__builtin_object_size would get lowered like:

__builtin_object_size(p, 0) -> @llvm.objectsize(i8* %p, i8 2)
__builtin_object_size(p, 1) -> @llvm.objectsize(i8* %p, i8 2)
__builtin_object_size(p, 2) -> @llvm.objectsize(i8* %p, i8 3)
(__builtin_object_size(p, 3) doesn't actually get lowered)

...Which might be a bit surprising to some people. If we think that's a
non-issue, I'm happy to take the simpler approach and use an i8.

What I had in mind was:

__builtin_object_size(p, 0) -> @llvm.objectsize(i8* %p, i1 0)
__builtin_object_size(p, 1) -> @llvm.objectsize(i8* %p, i1 0)
__builtin_object_size(p, 2) -> @llvm.objectsize(i8* %p, i1 1)
__builtin_object_size(p, 3) -> @llvm.objectsize(i8* %p, i1 1)

and changing the specification of @llvm.objectsize to match.

We'll have to have some <min>-awareness either in clang (to decide if
the <null-is-unknown> bit needs to be set) or in the middle end. What
is your plan here?

My plan was just to always set the `null-is-unknown` bit when lowering a
call to __builtin_object_size in clang. If `min` is true, we treat unknown
and null values identically in @llvm.objectsize, so whether
`null-is-unknown` is set in that case shouldn't matter.

Continuing from above: in other words, since your use case is always
setting the `null-is-unknown` bit, can you re-define @llvm.objectsize
to have that semantic without adding a new parameter?

That's not backward compatible, but there's a simple but conservative
update strategy.

However, since Malloc can return null:

I think I was unclear: this behavior would only exist if @llvm.objectsize
was actually handed null. I wasn't planning on changing how we'd handle
memory allocation functions that may return null (GCC gives back 2 for
__builtin_object_size(malloc(2), 0)). In other words, this `null-is-unknown`
bit only makes the objectsize evaluator see `T* null` as though it was `call
T* @opaque_user_defined_function()`, nothing else.

I meant to say that it looks like the semantics of ObjectSizeMin(X) is
that "return the conservative minimum object size for all values that
X may take at runtime" (resp. ObjectSizeMax(X)).

For instance this:

void i(int c, volatile int* sink) {
  void* mem1 = malloc(20);
  void* mem2 = malloc(40);

  *sink = __builtin_object_size(c ? mem1 : mem2, 0);
  *sink = __builtin_object_size(c ? mem1 : mem2, 2);
}

is lowered to

        movl $40, (%rsi)
        movl $20, (%rsi)
        ret

by GCC.

Applying the same logic to malloc(N), since it returns a location that
has N dereferenceable bytes or NULL, it follows that
ObjectSizeMin(malloc(N)) should return the smaller of
ObjectSizeMin(NULL) and ObjectSizeMin(MemoryLocOfNBytes) == the
smaller of ObjectSizeMin(NULL) and N. Given that we want the result
of ObjectSizeMin(malloc(N)) == UMIN(ObjectSizeMin(NULL), N) to be N,
we'd want ObjectSizeMin(NULL) to be (unsigned)-1 for consistency.

However, none of this matters if we're not in a position to change the
specification.

-- Sanjoy

Continuing from above: in other words, since your use case is always

Hi George,

Continuing from above: in other words, since your use case is always
setting the `null-is-unknown` bit, can you re-define @llvm.objectsize
to have that semantic without adding a new parameter?

That's not backward compatible, but there's a simple but conservative
update strategy.

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:

struct Foo { int a; };
int getA(struct Foo *f) { return f->a; }
int getA2() { struct Foo *f = 0; return f->a; }

I guess that's another sign that the spec is wrong. :slight_smile:

However, maybe we can use a trick here -- instead of lowering the
check to assert(ObjectSize(f) UGE 4) how about assert((1 +
ObjectSize(f)) UGT 4)? The addition should always constant fold, and
if `f` is null then we'll assert((1 + (-1)) UGT 4) ==> assert(false).

-- Sanjoy

Hi George,

Continuing from above: in other words, since your use case is always
setting the `null-is-unknown` bit, can you re-define @llvm.objectsize
to have that semantic without adding a new parameter?

That's not backward compatible, but there's a simple but conservative
update strategy.

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:

struct Foo { int a; };
int getA(struct Foo *f) { return f->a; }
int getA2() { struct Foo *f = 0; return f->a; }

I guess that's another sign that the spec is wrong. :slight_smile:

However, maybe we can use a trick here -- instead of lowering the
check to assert(ObjectSize(f) UGE 4) how about assert((1 +
ObjectSize(f)) UGT 4)? The addition should always constant fold, and
if `f` is null then we'll assert((1 + (-1)) UGT 4) ==> assert(false).

Never mind; that won't work. We'll also fail the assert for
unanalyzable values.