Page MenuHomePhabricator

[Clang] Check for returns_nonnull when deciding to add allocation null checks
ClosedPublic

Authored by modimo on May 19 2021, 6:26 PM.

Details

Summary

Non-throwing allocators currently will always get null-check code. However, if the non-throwing allocator is explicitly annotated with returns_nonnull the null check should be elided.

Testing:
ninja check-all
added test case correctly elides

Diff Detail

Event Timeline

modimo created this revision.May 19 2021, 6:26 PM
modimo requested review of this revision.May 19 2021, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 6:26 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
modimo updated this revision to Diff 346610.May 19 2021, 6:32 PM

Go back to single statement return, fix up ATTR_BUILTIN_NOTHROW_NEW test label that pointed to nothing to correct ATTR_NOBUILTIN_NOUNWIND_ALLOCSIZE label.

Discussing with @urnathan this makes more sense for the BE to handle when optimizing by eliding the generated null check. Confirmed this is indeed the case so removing the generation in the FE isn't really needed.

bruno added a comment.May 20 2021, 2:19 PM

Sounds reasonable to me! Can you double check whether this attribute gets correctly serialized/deserialized in face of CXXNewExpr? An example of how to test that would be in clang/test/PCH/cxx-method.cpp.

Sounds reasonable to me! Can you double check whether this attribute gets correctly serialized/deserialized in face of CXXNewExpr? An example of how to test that would be in clang/test/PCH/cxx-method.cpp.

Piggybacking on that test case:

Inputs/cxx-method.h:

typedef __typeof__(sizeof(0)) size_t;

void *operator new(size_t size)
{
  return ::operator new(size);
}

cxx-method.cpp:

int * foo()
{
    return new int;
}

Testing

~/llvm-project/clang/test/PCH# ~/llvm-project/build-rel/bin/clang++ -cc1 -x c++  -emit-pch Inputs/cxx-method.h -o test.pch
~/llvm-project/clang/test/PCH# ~/llvm-project/build-rel/bin/clang++ -cc1 -x c++ cxx-method.cpp -include-pch test.pch -emit-llvm
~/llvm-project/clang/test/PCH# grep _Znwm cxx-method.ll
define dso_local nonnull i8* @_Znwm(i64 %size) #0 {
  %call = call noalias nonnull i8* @_Znwm(i64 %0) #2
  %call = call noalias nonnull i8* @_Znwm(i64 4) #3

Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.

@rsmith @lebedev.ri thoughts on adding this directly to FE generation? As mentioned this isn't strictly needed and the BE can elide the check but we can also not emit it to save on AST/IR processing cost.

bruno accepted this revision.Jun 15 2021, 4:55 PM

Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.

Thanks!

As mentioned this isn't strictly needed and the BE can elide the check but we can also not emit it to save on AST/IR processing cost.

It's a good way for clang based tooling to get the expected result if relying on shouldNullCheckAllocation for an accurate answer.

Maybe wait a few more days to see if anyone else has any comment, otherwise LGTM.

This revision is now accepted and ready to land.Jun 15 2021, 4:55 PM