Page MenuHomePhabricator

Annotate return values of allocation functions with dereferenceable_or_null
ClosedPublic

Authored by xbolva00 on Aug 23 2019, 6:50 AM.

Details

Summary

Example
define dso_local noalias i8* @_Z6maixxnv() local_unnamed_addr #0 {
entry:

%call = tail call noalias dereferenceable_or_null(64) i8* @malloc(i64 64) #6
ret i8* %call

}

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Aug 23 2019, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2019, 6:50 AM
xbolva00 marked an inline comment as done.Aug 23 2019, 6:52 AM

I will update/add tests after some initial code review.

lib/Transforms/InstCombine/InstCombineCalls.cpp
4189 ↗(On Diff #216830)

Not a typo. OpNewLike never returns null, it just throws exception.

It seems inconsistent to declare Op0C initially and Op1C late given that both are need 2 times in almost the same fashion.

We also need tests.

I think this is the right thing to do though, dereferenceability will be used for LICM and AA (D66157), and nonnull can be eliminated with any access or null check (D65402 and following).

xbolva00 updated this revision to Diff 216876.Aug 23 2019, 9:12 AM

Improved code.
Added/updated tests.

xbolva00 updated this revision to Diff 217371.Aug 27 2019, 4:54 AM

Precommited tests - Rebased.

I'm not an LLVM person, but I just wanted to double-check: is this correct even in the face of situations where malloc() and friends return a non-null pointer that is *not* dereferenceable? e.g., when someone calls malloc(0) with certain libc implementations?

xbolva00 added a comment.EditedAug 27 2019, 7:05 AM

We ignore malloc(0), realloc(p, 0), etc cases (no annotation, deref_or_null(0) is useless). All edge cases should be coveree in deref-alloc-fns.ll.

We annotate if size is known constant and > 0.

xbolva00 updated this revision to Diff 217393.Aug 27 2019, 7:20 AM

Fixed function names in new tests. (op_new_constant_size and op_new_constant_zero_size was swapped).

Two last comments from my side.

lib/Transforms/InstCombine/InstCombineCalls.cpp
4183 ↗(On Diff #217393)

What is wrong with invoke?

4187 ↗(On Diff #217393)

You can "speed things up" if you check Op0C and Op1C for null first (early exit). At least one has to be nonnull and its value as well. Also below, the isXXXX check is more expensive so we should first check the operands.

xbolva00 updated this revision to Diff 217427.Aug 27 2019, 10:11 AM

Fixed review comments

xbolva00 marked 2 inline comments as done.Aug 27 2019, 10:11 AM
jdoerfert accepted this revision.Aug 27 2019, 12:10 PM

Last problem mentioned below. Otherwise, LGTM.

lib/Transforms/InstCombine/InstCombineCalls.cpp
4187 ↗(On Diff #217427)

You cannot remove the isNullValue() check here and below, Op1C could be the reason we skipped the return above!

This revision is now accepted and ready to land.Aug 27 2019, 12:10 PM
xbolva00 marked an inline comment as done.Aug 27 2019, 12:29 PM
xbolva00 added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
4187 ↗(On Diff #217427)

Sorry, I dont know what is wrong here - newly added tests pass.

We dont need OP1C for malloc.

This revision was automatically updated to reflect the committed changes.
jdoerfert added inline comments.Aug 28 2019, 7:38 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

What if one is NULL but the other not? Will getWithDereferenceableOrNullBytes(0) do the right thing?

xbolva00 marked an inline comment as done.Aug 28 2019, 7:55 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

Maybe isNullValue confused you. isNullValue means just CstInt->getValue() == 0.

if Op0C is NULL -> no calloc annotation
if Op1C is NULL -> no calloc annotation

if Op0C is '0' -> no annotation; see
if ((Op0C && Op0C->isNullValue()) || (Op1C && Op1C->isNullValue()))

return;

Same for Op1C.

xbolva00 marked an inline comment as done.Aug 28 2019, 7:56 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

I will add a comment like

// Bail out if the allocation size is zero.
if ((Op0C && Op0C->isNullValue()) || (Op1C && Op1C->isNullValue()))
  return;
xbolva00 marked an inline comment as done.Aug 28 2019, 7:59 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

Aha, I know what do you mean.

Hmm, it could work for calloc case - I will look on it.

xbolva00 marked an inline comment as done.Aug 28 2019, 8:01 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

eh, no :D

calloc(1, unknown_var) - We can do nothing in this case ..

jdoerfert added inline comments.Aug 28 2019, 8:04 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

calloc(1, 0) or calloc(0, 1) is the question

xbolva00 marked an inline comment as done.Aug 28 2019, 8:50 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

No annotation for this cases - why should we? Calloc internally multiplies it

So calloc(0,1) is logically same as

p = malloc(0*1);
memset(p, 0, 0*1)

xbolva00 marked an inline comment as done.Aug 28 2019, 8:56 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

man calloc
"If nmemb or size is 0, then calloc() returns either NULL, or a unique pointer value that can later be successfully passed to free()."

(so same as malloc(0))

We could fold these cases to NULL, but not worth since this rather programmer's mistake than optimization opportunity.

jdoerfert added inline comments.Aug 28 2019, 9:35 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

I am not asking for an annotation but if we don't accidentally put one as we should not.

What if one is NULL but the other not? Will getWithDereferenceableOrNullBytes(0) do the right thing?

xbolva00 marked an inline comment as done.Aug 28 2019, 9:51 AM
xbolva00 added inline comments.
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

getWithDereferenceableOrNullBytes(Call.getContext(), 0) never happens here.

If Op1 is zero int or Op0 is zero int (or both are zeros), we bail out soon.

We multiplies non zero numbers here.

jdoerfert added inline comments.Aug 28 2019, 10:39 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
4200

My bad. sry.