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

[manojgupta]

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.

[efriedma]

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

[manojgupta]

In D47894#1125406, @efriedma wrote:

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.

[efriedma]

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.)

[efriedma]

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]

In D47894#1125653, @efriedma wrote:

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.

[xbolva00]

In D47894#1125653, @efriedma wrote:

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?

[efriedma]

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

[srhines]

In D47894#1125728, @jyknight wrote:

In D47894#1125653, @efriedma wrote:

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.

[jyknight]

In D47894#1125961, @srhines wrote:

In D47894#1125728, @jyknight wrote:

In D47894#1125653, @efriedma wrote:

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.

[manojgupta]

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.

[manojgupta]

@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 ↗

(On Diff #154895)

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

[manojgupta]

Ping!

[jyknight]

In D47894#1158811, @manojgupta wrote:

@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

1077

Do we need help text for this one too?

test/CodeGen/nonnull.c

1–2 ↗

(On Diff #154895)

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 ↗

(On Diff #154895)

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

[manojgupta]

Added helper text, updated tests.

[jyknight]

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 ↗

(On Diff #155891)

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

1078

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

1081

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

include/clang/Driver/Options.td

 def frewrite_includes : Flag<["-"], "frewrite-includes">, Group<f_Group>,
   Flags<[CC1Option]>;
 def fno_rewrite_includes : Flag<["-"], "fno-rewrite-includes">, Group<f_Group>;
 
 def frewrite_imports : Flag<["-"], "frewrite-imports">, Group<f_Group>,
   Flags<[CC1Option]>;
 def fno_rewrite_imports : Flag<["-"], "fno-rewrite-imports">, Group<f_Group>;
 
+def fdelete_null_pointer_checks : Flag<["-"],
+  "fdelete-null-pointer-checks">, Group<f_Group>;
+def fno_delete_null_pointer_checks : Flag<["-"],
+  "fno-delete-null-pointer-checks">, Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Assume that null pointer dereferences are not undefined">;
+
 def frewrite_map_file : Separate<["-"], "frewrite-map-file">,
                         Group<f_Group>,
                         Flags<[ DriverOption, CC1Option ]>;
 def frewrite_map_file_EQ : Joined<["-"], "frewrite-map-file=">,
                            Group<f_Group>,
                            Flags<[DriverOption]>;
 
 def fuse_line_directives : Flag<["-"], "fuse-line-directives">, Group<f_Group>,

 def fdiagnostics_show_location_EQ : Joined<["-"], "fdiagnostics-show-location=">, Group<clang_ignored_f_Group>;
 
 defm fcheck_new : BooleanFFlag<"check-new">, Group<clang_ignored_f_Group>;
 defm caller_saves : BooleanFFlag<"caller-saves">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm reorder_blocks : BooleanFFlag<"reorder-blocks">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm eliminate_unused_debug_types : BooleanFFlag<"eliminate-unused-debug-types">, Group<clang_ignored_f_Group>;
 defm branch_count_reg : BooleanFFlag<"branch-count-reg">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm default_inline : BooleanFFlag<"default-inline">, Group<clang_ignored_gcc_optimization_f_Group>;
-defm delete_null_pointer_checks : BooleanFFlag<"delete-null-pointer-checks">,
-    Group<clang_ignored_gcc_optimization_f_Group>;
 defm fat_lto_objects : BooleanFFlag<"fat-lto-objects">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm float_store : BooleanFFlag<"float-store">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm friend_injection : BooleanFFlag<"friend-injection">, Group<clang_ignored_f_Group>;
 defm function_attribute_list : BooleanFFlag<"function-attribute-list">, Group<clang_ignored_f_Group>;
 defm gcse : BooleanFFlag<"gcse">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm gcse_after_reload: BooleanFFlag<"gcse-after-reload">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm gcse_las: BooleanFFlag<"gcse-las">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm gcse_sm: BooleanFFlag<"gcse-sm">, Group<clang_ignored_gcc_optimization_f_Group>;

include/clang/Frontend/CodeGenOptions.def

CODEGENOPT(NoDwarfDirectoryAsm , 1, 0) ///< Set when -fno-dwarf-directory-asm is
                                        ///< enabled.
 CODEGENOPT(NoExecStack       , 1, 0) ///< Set when -Wa,--noexecstack is enabled.
 CODEGENOPT(FatalWarnings     , 1, 0) ///< Set when -Wa,--fatal-warnings is
                                      ///< enabled.
 CODEGENOPT(EnableSegmentedStacks , 1, 0) ///< Set when -fsplit-stack is enabled.
 CODEGENOPT(NoImplicitFloat   , 1, 0) ///< Set when -mno-implicit-float is enabled.
 CODEGENOPT(NoInfsFPMath      , 1, 0) ///< Assume FP arguments, results not +-Inf.
 CODEGENOPT(NoSignedZeros     , 1, 0) ///< Allow ignoring the signedness of FP zero
+CODEGENOPT(NullPointerIsValid , 1, 0) ///< Assume Null pointer deference is defined.
 CODEGENOPT(Reassociate       , 1, 0) ///< Allow reassociation of FP math ops
 CODEGENOPT(ReciprocalMath    , 1, 0) ///< Allow FP divisions to be reassociated.
 CODEGENOPT(NoTrappingMath    , 1, 0) ///< Set when -fno-trapping-math is enabled.
 CODEGENOPT(NoNaNsFPMath      , 1, 0) ///< Assume FP arguments, results not NaN.
 CODEGENOPT(FlushDenorm       , 1, 0) ///< Allow FP denorm numbers to be flushed to zero
 CODEGENOPT(CorrectlyRoundedDivSqrt, 1, 0) ///< -cl-fp32-correctly-rounded-divide-sqrt
 
 /// When false, this attempts to generate code as if the result of an

lib/CodeGen/CGCall.cpp

if (AttrOnCallSite) {
     } else {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     }
 
     FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 
+    if (CodeGenOpts.NullPointerIsValid)
+      FuncAttrs.addAttribute("null-pointer-is-valid", "true");
     if (!CodeGenOpts.FPDenormalMode.empty())
       FuncAttrs.addAttribute("denormal-fp-math", CodeGenOpts.FPDenormalMode);
 
     FuncAttrs.addAttribute("no-trapping-math",
                            llvm::toStringRef(CodeGenOpts.NoTrappingMath));
 
     // Strict (compliant) code is the default, so only add this attribute to
     // indicate that we are trying to workaround a problem case.

lib/Driver/ToolChains/Clang.cpp

else
     CmdArgs.push_back(Args.MakeArgString(getToolChain().getThreadModel()));
 
   Args.AddLastArg(CmdArgs, options::OPT_fveclib);
 
   if (Args.hasFlag(options::OPT_fmerge_all_constants,
                    options::OPT_fno_merge_all_constants, false))
     CmdArgs.push_back("-fmerge-all-constants");
 
+  if (Args.hasFlag(options::OPT_fno_delete_null_pointer_checks,
+                   options::OPT_fdelete_null_pointer_checks, false))
+    CmdArgs.push_back("-fno-delete-null-pointer-checks");
+
   // LLVM Code Generator Options.
 
   if (Args.hasArg(options::OPT_frewrite_map_file) ||
       Args.hasArg(options::OPT_frewrite_map_file_EQ)) {
     for (const Arg *A : Args.filtered(options::OPT_frewrite_map_file,
                                       options::OPT_frewrite_map_file_EQ)) {
       StringRef Map = A->getValue();
       if (!llvm::sys::fs::exists(Map)) {

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.DataSections = Args.hasFlag(OPT_fdata_sections,
                                    OPT_fno_data_sections, false);
   Opts.StackSizeSection =
       Args.hasFlag(OPT_fstack_size_section, OPT_fno_stack_size_section, false);
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
                                          OPT_fno_unique_section_names, true);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
+  Opts.NullPointerIsValid = Args.hasArg(OPT_fno_delete_null_pointer_checks);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
 
   Opts.ProfileSampleAccurate = Args.hasArg(OPT_fprofile_sample_accurate);
 
   Opts.PrepareForLTO = Args.hasArg(OPT_flto, OPT_flto_EQ);
   Opts.EmitSummaryIndex = false;
   if (Arg *A = Args.getLastArg(OPT_flto_EQ)) {

test/CodeGen/delete-null-pointer-checks.c

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck -check-prefix=NULL-POINTER-INVALID  %s
+// RUN: %clang_cc1 -emit-llvm -o - %s -fno-delete-null-pointer-checks | FileCheck -check-prefix=NULL-POINTER-VALID  %s
+
+int test1(int a) {
+  return a;
+}
+
+// NULL-POINTER-INVALID-NOT: attributes #0 = {{.*}} "null-pointer-is-valid"="true"
+// NULL-POINTER-VALID: attributes #0 = {{.*}} "null-pointer-is-valid"="true"

test/Driver/clang_f_opts.c

 // RUN: -funsafe-loop-optimizations                                           \
 // RUN: -fuse-linker-plugin                                                   \
 // RUN: -fvect-cost-model                                                     \
 // RUN: -fvariable-expansion-in-unroller                                      \
 // RUN: -fweb                                                                 \
 // RUN: -fwhole-program                                                       \
 // RUN: -fcaller-saves                                                        \
 // RUN: -freorder-blocks                                                      \
-// RUN: -fdelete-null-pointer-checks                                          \
 // RUN: -ffat-lto-objects                                                     \
 // RUN: -fmerge-constants                                                     \
 // RUN: -finline-small-functions                                              \
 // RUN: -ftree-dce                                                            \
 // RUN: -ftree-ter                                                            \
 // RUN: -ftree-vrp                                                            \
 // RUN: -fno-devirtualize                                                     \
 // RUN: -fno-devirtualize-speculatively                                       \

 // CHECK-WARNING-DAG: optimization flag '-funsafe-loop-optimizations' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fuse-linker-plugin' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fvect-cost-model' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fvariable-expansion-in-unroller' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fweb' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fwhole-program' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fcaller-saves' is not supported
 // CHECK-WARNING-DAG: optimization flag '-freorder-blocks' is not supported
-// CHECK-WARNING-DAG: optimization flag '-fdelete-null-pointer-checks' is not supported
 // CHECK-WARNING-DAG: optimization flag '-ffat-lto-objects' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fmerge-constants' is not supported
 // CHECK-WARNING-DAG: optimization flag '-finline-small-functions' is not supported
 // CHECK-WARNING-DAG: optimization flag '-ftree-dce' is not supported
 // CHECK-WARNING-DAG: optimization flag '-ftree-ter' is not supported
 // CHECK-WARNING-DAG: optimization flag '-ftree-vrp' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fno-devirtualize' is not supported
 // CHECK-WARNING-DAG: optimization flag '-fno-devirtualize-speculatively' is not supported

 // CHECK-NO-DISCARD-NAMES-NOT: "-discard-value-names"
 //
 // RUN: %clang -### -S -fmerge-all-constants %s 2>&1 | FileCheck -check-prefix=CHECK-MERGE-ALL-CONSTANTS %s
 // RUN: %clang -### -S -fno-merge-all-constants %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MERGE-ALL-CONSTANTS %s
 // RUN: %clang -### -S -fmerge-all-constants -fno-merge-all-constants %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MERGE-ALL-CONSTANTS %s
 // RUN: %clang -### -S -fno-merge-all-constants -fmerge-all-constants %s 2>&1 | FileCheck -check-prefix=CHECK-MERGE-ALL-CONSTANTS %s
 // CHECK-NO-MERGE-ALL-CONSTANTS-NOT: "-fmerge-all-constants"
 // CHECK-MERGE-ALL-CONSTANTS: "-fmerge-all-constants"
+
+// RUN: %clang -### -S -fdelete-null-pointer-checks %s 2>&1 | FileCheck -check-prefix=CHECK-NULL-POINTER-CHECKS %s
+// RUN: %clang -### -S -fno-delete-null-pointer-checks %s 2>&1 | FileCheck -check-prefix=CHECK-NO-NULL-POINTER-CHECKS %s
+// RUN: %clang -### -S -fdelete-null-pointer-checks -fno-delete-null-pointer-checks %s 2>&1 | FileCheck -check-prefix=CHECK-NO-NULL-POINTER-CHECKS %s
+// RUN: %clang -### -S -fno-delete-null-pointer-checks -fdelete-null-pointer-checks %s 2>&1 | FileCheck -check-prefix=CHECK-NULL-POINTER-CHECKS %s
+// CHECK-NO-NULL-POINTER-CHECKS: "-fno-delete-null-pointer-checks"
+// CHECK-NULL-POINTER-CHECKS-NOT: "-fno-delete-null-pointer-checks"