Page MenuHomePhabricator

[clang]: Add support for "-fno-delete-null-pointer-checks"
ClosedPublic

Authored by manojgupta on Jun 7 2018, 11:16 AM.

Details

Summary

Support for this option is needed for building Linux kernel.
This is a very frequently requested feature by kernel developers.

More details : https://lkml.org/lkml/2018/4/4/601

GCC option description for -fdelete-null-pointer-checks:
This Assume that programs cannot safely dereference null pointers,
and that no code or data element resides at address zero.

-fno-delete-null-pointer-checks is the inverse of this implying that
null pointer dereferencing is not undefined.

This feature is implemented in as the function attribute
"null-pointer-is-valid"="true".
This CL only adds the attribute on the function.
It also strips "nonnull" attributes from function arguments but
keeps the related warnings unchanged.

Corresponding LLVM change rL336613 already updated the
optimizations to not treat null pointer dereferencing
as undefined if the attribute is present.

Diff Detail

Event Timeline

manojgupta created this revision.Jun 7 2018, 11:16 AM

Does IR generation need any special handling for this? We add nonnull attributes in various places.

Does IR generation need any special handling for this? We add nonnull attributes in various places.

My interpretation is adding nonnull attributes is fine as long is it is not derived from a pointer dereference.
E.g. addresses from alloca can be treated non-null.

The problem would come from propagating nonnull-ness from something which isn't inherently nonnull. For example, strlen has a nonnull argument, so strlen(NULL) is UB, therefore given int z = strlen(x); if (x) {...}, we can remove the null check. (Not sure we actually do this transform at the moment, but it's something we could do in the future.)

For the kernel, specifically, strlen() isn't an issue because it builds with -fno-builtin, but the kernel uses explicit nonnull attributes in a few places. But I guess we can assume they know what they're doing if nonnull is explicitly specified.
We probably want to call this out in the users manual, though.

We add nonnull attributes to the LLVM IR in a few other places which don't involve the C nonnull attribute: we assume C++ references are always non-null, and we use the "static" array modifier to assume arrays are non-null. We probably shouldn't add those attributes if fno-delete-null-pointer-checks is specified.

jyknight added a comment.EditedJun 7 2018, 2:55 PM

The problem would come from propagating nonnull-ness from something which isn't inherently nonnull. For example, strlen has a nonnull argument, so strlen(NULL) is UB, therefore given int z = strlen(x); if (x) {...}, we can remove the null check. (Not sure we actually do this transform at the moment, but it's something we could do in the future.)

I think the question there is actually whether we need to, in addition to supporting null pointer dereference, also cause all __attribute__((nonnull)) annotations at the C/C++ level to be ignored.

And, yes, I believe we should.

The problem would come from propagating nonnull-ness from something which isn't inherently nonnull. For example, strlen has a nonnull argument, so strlen(NULL) is UB, therefore given int z = strlen(x); if (x) {...}, we can remove the null check. (Not sure we actually do this transform at the moment, but it's something we could do in the future.)

This is interesting. Extra pass for such transformation? or any better place for it?

For potential transforms based on nonnull, see https://reviews.llvm.org/D36656, I think?

The problem would come from propagating nonnull-ness from something which isn't inherently nonnull. For example, strlen has a nonnull argument, so strlen(NULL) is UB, therefore given int z = strlen(x); if (x) {...}, we can remove the null check. (Not sure we actually do this transform at the moment, but it's something we could do in the future.)

I think the question there is actually whether we need to, in addition to supporting null pointer dereference, also cause all __attribute__((nonnull)) annotations at the C/C++ level to be ignored.

And, yes, I believe we should.

Is that what the kernel community actually wants though? There are certainly others who want a way to strip __attribute__((nonnull)) completely, which seems a bit orthogonal to the removal of null checks in general. I think it would be good to support such a flag, but the presence of nonnull inside kernel sources leads me to believe they want those cases to be treated specially.

The problem would come from propagating nonnull-ness from something which isn't inherently nonnull. For example, strlen has a nonnull argument, so strlen(NULL) is UB, therefore given int z = strlen(x); if (x) {...}, we can remove the null check. (Not sure we actually do this transform at the moment, but it's something we could do in the future.)

I think the question there is actually whether we need to, in addition to supporting null pointer dereference, also cause all __attribute__((nonnull)) annotations at the C/C++ level to be ignored.

And, yes, I believe we should.

Is that what the kernel community actually wants though? There are certainly others who want a way to strip __attribute__((nonnull)) completely, which seems a bit orthogonal to the removal of null checks in general. I think it would be good to support such a flag, but the presence of nonnull inside kernel sources leads me to believe they want those cases to be treated specially.

In GCC, the nonnull attribute still has an effect on warnings but not on codegen if you enable -fno-delete-null-pointer-checks: https://godbolt.org/g/7SSi2V

That seems a pretty sensible behavior, IMO.

Do not generate calls with "nonnull" attribute with "-fno-delete-null-pointer-checks".
The warnings are still generated when nullptr is passed to a function with nonnull
attribute.

@efriedma @jyknight Does the change match your expectations where warnings are still generated but codeGen does not emit nonnull attribute?

test/Sema/nonnull.c
2

All warnings are still issued if nullptr is passed to function nonnull attribute.

@efriedma @jyknight Does the change match your expectations where warnings are still generated but codeGen does not emit nonnull attribute?

Yes, this seems sensible IMO.

include/clang/Driver/Options.td
1080

Do we need help text for this one too?

test/CodeGen/nonnull.c
1–2

These prefixes are very confusingly named. NONNULL and NO-NULL sound like the same thing.
I'd suggest using, across all the files here,
CHECK:
NULL-INVALID:
NULL-VALID:

That'd also avoid the need to rename all the "CHECK" lines to something else, which is most of the diff in these files.

test/Sema/nonnull.c
2

SGTM, but this should be a comment in the file, not the code review.

Added helper text, updated tests.

jyknight accepted this revision.Jul 17 2018, 8:50 AM

Please refer to the commit for the LLVM half of the change in the commit message for this.

LGTM, other than some minor suggestions for the help texts.

docs/ClangCommandLineReference.rst
1548

Suggested text:
When enabled, treat null pointer dereference, creation of a reference to null, or passing a null pointer to a function parameter annotated with the "nonnull" attribute as undefined behavior. (And, thus the optimizer may assume that any pointer used in such a way must not have been null and optimize away branches accordingly.) On by default.

include/clang/Driver/Options.td
1081

Suggested text:
Treat usage of null pointers as undefined behavior.

1084

Suggested text:
Do not treat usage of null pointers as undefined behavior.

This revision is now accepted and ready to land.Jul 17 2018, 8:50 AM
manojgupta edited the summary of this revision. (Show Details)Jul 18 2018, 1:38 PM
This revision was automatically updated to reflect the committed changes.