[clang] Add -fcheck-new support

[heatd]

Summary

Add -fcheck-new and -fno-check-new, from GCC, which make the compiler
not assume pointers returned from operator new are non-null.
Fixes #16931.

[jrtc27]

clang/test/CodeGenCXX/fcheck-new.cpp

3

Do you really want -O2 or do you just want to run mem2reg to eliminate all the alloca noise?

[heatd]

clang/test/CodeGenCXX/fcheck-new.cpp

3

I added -O2 because I was testing with it, since if -fcheck-new doesn't work, it's way more noticeable, as the nullptr check gets optimized out; if it works, the branching is pretty visible and shows exactly what the option does. Also, to eliminate all the noise :) I can definitely remove -O2 though, if you want to.

[jrtc27]

clang/test/CodeGenCXX/fcheck-new.cpp

3

Thinking about this more, I think the branching complicates matters. All that optimisation happens in LLVM IR land, but you're just touching Clang CodeGen. Is int *foo() { return new int; } checking the attributes on the call to _Znwm not enough (and with mem2reg just to clean it up so the function body is trivial)?

Also you should specify a target triple as this will just be LLVM_DEFAULT_TARGET_TRIPLE and change both the type for new (i32 vs i64) and its mangling (j vs m for unsigned int vs unsigned long, and *-windows-msvc is totally different), right?

10

Another advantage of not using -O2 is you lose the TBAA noise

clang/test/Driver/clang_f_opts.c

73

Hmmm

[heatd]

Re-did the test, deleted redundant minus line diff.

[jrtc27]

clang/test/CodeGenCXX/fcheck-new.cpp

1

Not sure why they were checked before, but you'll need this to actually get the attributes checked

[heatd]

clang/test/Driver/clang_f_opts.c

73

Huh, running clang-format on clang_f_opts.c makes it do that. I fixed that up in the new revision but that should possibly be fixed.

[jrtc27]

clang/test/CodeGenCXX/fcheck-new.cpp

2

Not sure how this doesn't need -Xclang -disable-O0-optnone to work?..

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

5984

See addOptInFlag in this file.

clang/test/CodeGenCXX/fcheck-new.cpp

1

We generally only use %clang_cc1 for clang code gen tests. %clang generally is only used for test/Driver tests.

[urnathan]

clang/lib/Sema/SemaDecl.cpp

15440

'invalidates' or 'negates' are better words here.

[heatd]

Adjusted the driver code to use addOptInFlag, adjusted the test, fixed the comment.

[heatd]

In D125272#3504113, @heatd wrote:

Adjusted the driver code to use addOptInFlag, adjusted the test, fixed the comment.

Ping.

[heatd]

In D125272#3515874, @heatd wrote:

In D125272#3504113, @heatd wrote:

Adjusted the driver code to use addOptInFlag, adjusted the test, fixed the comment.

Ping.

Ping.

[heatd]

In D125272#3531836, @heatd wrote:

In D125272#3515874, @heatd wrote:

In D125272#3504113, @heatd wrote:

Adjusted the driver code to use addOptInFlag, adjusted the test, fixed the comment.

Ping.

Ping.

Could someone please review it or tell me what needs to be done before pushing this? Thanks.

[MaskRay]

clang/lib/Frontend/CompilerInvocation.cpp

4117

Use CodeGenOpts<"CheckNew"> and avoid change to this file. CodeGenOpts seems more suitable than LangOpts.

clang/test/CodeGenCXX/fcheck-new.cpp

4

Please remove opt. Optimizations should be tested in the llvm/ layer, not in clang codegen.

If utils/update_cc_test_checks.py does not generate good looking IR, write it by hand.

[jrtc27]

clang/test/CodeGenCXX/fcheck-new.cpp

4

Disagree. This is a pattern that is extremely useful for testing Clang's IR generation whilst cleaning up the alloca so the IR is readable. This is not about the generated checks, this is about alloca/load/store everywhere being a pain to inspect, whereas a simple mem2reg cleans this up.

[heatd]

clang/test/CodeGenCXX/fcheck-new.cpp

4

How should I proceed about this?

Sorry, I haven't thought about this pending change-set for a few months now, oops!

[lebedev.ri]

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.

[heatd]

clang/lib/Frontend/CompilerInvocation.cpp

4117

Sorry, I haven't looked at this rev in a good while due to $REASONS. Could you elaborate on this? I'm not opposed to moving it to CodeGenOpts per se, but I would like to understand your reasoning. It seems to me that fcheck-new is more of a language change (since it removes the implicit [[nonnull]] on the result of operator new) than a codegen change, although it does have its effects on codegen, naturally. A quick look at CodeGenOpts in Options.td would hint to me that these mostly/only affect codegen itself. What do you think?

[MaskRay]

clang/lib/Frontend/CompilerInvocation.cpp

4117

OK. LangOpts looks good since it affects Sema and in GCC this option is a C++ dialect option (https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html).

Can you use MarshallingInfoFlag<LangOpts and remove the change to CompilerInvocation.cpp? You can find other MarshallingInfoFlag<LangOpts examples in Options.td

[heatd]

Address MaskRay's comments on Options.td, adjust the help message, rebase to current tip

[heatd]

Update the test to work with tip (completely forgot, apologies)

[MaskRay]

clang/test/CodeGenCXX/fcheck-new.cpp

3

In this test, if I remove -fcheck-new , nothing will change. So this isn't a good test.

I think we need something like:

struct A { A(); };
void *test0() { return A; }

4

[heatd]

clang/test/CodeGenCXX/fcheck-new.cpp

3

It will change (the attributes of _Znwm).

From the test (with -fcheck-new):

[[CALL:%.*]] = call noalias noundef ptr @_Znwm(i64 noundef 4) #[[ATTR2:[0-9]+]]

The same test (but without -fcheck-new, tested locally):

%1 = tail call noalias noundef nonnull dereferenceable(4) ptr @_Znwm(i64 noundef 4) #2

As you'll see, the non-fcheck-new has nonnull attached to _Znwm, while -fcheck-new doesn't. This is exactly the behavior we want to test for.

[MaskRay]

clang/test/CodeGenCXX/fcheck-new.cpp

3

OK, you are right about nonull. I think my point still holds as with a constructor the effect will be more visible with a new.notnull: basic block.
You may want to add another function to do A *test1() { return new A; }

Also, with the simple IR, opt -S -mem2reg is a no-op. You should remove it.

8

#[[ATTR2:[0-9]+]] If ATTR2 is not checked, remove #[[ATTR2:[0-9]+]]

[heatd]

Addressed MaskRay's feedback:

Added a testcase that showcases the conditional calling of a constructor (due to new not being tagged nonnull)
Kept the testcase with the simple _Znwm call
Deleted the ATTR tags at the end
Removed opt mem2reg

[MaskRay]

clang/test/CodeGenCXX/fcheck-new.cpp

4

remove -S since -emit-llvm wins .This is different from clang driver -S -emit-llvm

[heatd]

Remove -S from the fcheck-new test RUN:, as requested by MaskRay

clang/include/clang/Basic/LangOptions.h

public:
   bool IsHeaderFile = false;
 
   /// The default stream kind used for HIP kernel launching.
   GPUDefaultStreamKind GPUDefaultStream;
 
   /// The seed used by the randomize structure layout feature.
   std::string RandstructSeed;
 
+  // Indicates whether we should keep all nullptr checks for pointers
+  // received as a result of a standard operator new (-fcheck-new)
+  bool CheckNew = false;
+
   LangOptions();
 
   /// Set language defaults for the given input language and
   /// language standard in the given LangOptions object.
   ///
   /// \param Opts - The LangOptions object to set up.
   /// \param Lang - The input language.
   /// \param T - The target triple.

clang/include/clang/Basic/LangOptions.def

 LANGOPT(VScaleMax, 32, 0, "Maximum vscale value")
 
 ENUM_LANGOPT(ExtendIntArgs, ExtendArgsKind, 1, ExtendArgsKind::ExtendTo32,
              "Controls how scalar integer arguments are extended in calls "
              "to unprototyped and varargs functions")
 
 VALUE_LANGOPT(FuchsiaAPILevel, 32, 0, "Fuchsia API level")
 
+BENIGN_LANGOPT(CheckNew, 1, 0, "Do not eliminate null pointer checks for storage allocated by new")
+
 #undef LANGOPT
 #undef COMPATIBLE_LANGOPT
 #undef BENIGN_LANGOPT
 #undef ENUM_LANGOPT
 #undef COMPATIBLE_ENUM_LANGOPT
 #undef BENIGN_ENUM_LANGOPT
 #undef VALUE_LANGOPT
 #undef COMPATIBLE_VALUE_LANGOPT
 #undef BENIGN_VALUE_LANGOPT

clang/include/clang/Driver/Options.td

 defm align_loops : BooleanFFlag<"align-loops">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm align_jumps : BooleanFFlag<"align-jumps">, Group<clang_ignored_gcc_optimization_f_Group>;
 def falign_jumps_EQ : Joined<["-"], "falign-jumps=">, Group<clang_ignored_gcc_optimization_f_Group>;
 
 // FIXME: This option should be supported and wired up to our diognostics, but
 // ignore it for now to avoid breaking builds that use it.
 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 check_new : BoolFOption<"check-new",
+  LangOpts<"CheckNew">, DefaultFalse,
+  PosFlag<SetTrue, [], "Check for a null pointer before using storage allocated by new">,
+  NegFlag<SetFalse>, BothFlags<[CC1Option]>>;
+
 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 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 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>;

clang/lib/AST/ExprCXX.cpp

void *Mem =
       Ctx.Allocate(totalSizeToAlloc<Stmt *, SourceRange>(
                        IsArray + HasInit + NumPlacementArgs, IsParenTypeId),
                    alignof(CXXNewExpr));
   return new (Mem)
       CXXNewExpr(EmptyShell(), IsArray, NumPlacementArgs, IsParenTypeId);
 }
 
 bool CXXNewExpr::shouldNullCheckAllocation() const {
+  if (getOperatorNew()->getLangOpts().CheckNew)
+    return true;
   return !getOperatorNew()->hasAttr<ReturnsNonNullAttr>() &&
          getOperatorNew()
              ->getType()
              ->castAs<FunctionProtoType>()
              ->isNothrow() &&
          !getOperatorNew()->isReservedGlobalPlacementOperator();
 }
 

clang/lib/Driver/ToolChains/Clang.cpp

   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
   Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
                   options::OPT_fno_emulated_tls);
   Args.AddLastArg(CmdArgs, options::OPT_fzero_call_used_regs_EQ);
 
+  if (Args.hasFlag(options::OPT_fcheck_new, options::OPT_fno_check_new, false))
+    CmdArgs.push_back("-fcheck-new");
+
   if (Arg *A = Args.getLastArg(options::OPT_fzero_call_used_regs_EQ)) {
     // FIXME: There's no reason for this to be restricted to X86. The backend
     // code needs to be changed to include the appropriate function calls
     // automatically.
     if (!Triple.isX86())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
   }

clang/lib/Frontend/CompilerInvocation.cpp

if (!SeedFile.is_open())
           << A->getValue(0);
 
     std::getline(SeedFile, Opts.RandstructSeed);
   }
 
   if (const Arg *A = Args.getLastArg(OPT_frandomize_layout_seed_EQ))
     Opts.RandstructSeed = A->getValue(0);
 
+  if (const Arg *A = Args.getLastArg(OPT_fcheck_new))
+    Opts.CheckNew = true;
+
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
   switch (Action) {
   case frontend::ASTDeclList:
   case frontend::ASTDump:
   case frontend::ASTPrint:

clang/lib/Sema/SemaDecl.cpp

void Sema::AddKnownFunctionAttributesForReplaceableGlobalAllocationFunction(
   if (!FD->isReplaceableGlobalAllocationFunction(&AlignmentParam, &IsNothrow))
     return;
 
   // C++2a [basic.stc.dynamic.allocation]p4:
   //   An allocation function that has a non-throwing exception specification
   //   indicates failure by returning a null pointer value. Any other allocation
   //   function never returns a null pointer value and indicates failure only by
   //   throwing an exception [...]
-  if (!IsNothrow && !FD->hasAttr<ReturnsNonNullAttr>())
+  //
+  // However, -fcheck-new violates this possible assumption, so don't add
+  // NonNull when that is enabled.
+  if (!IsNothrow && !FD->hasAttr<ReturnsNonNullAttr>() &&
+      !getLangOpts().CheckNew)
     FD->addAttr(ReturnsNonNullAttr::CreateImplicit(Context, FD->getLocation()));
 
   // C++2a [basic.stc.dynamic.allocation]p2:
   //   An allocation function attempts to allocate the requested amount of
   //   storage. [...] If the request succeeds, the value returned by a
   //   replaceable allocation function is a [...] pointer value p0 different
   //   from any previously returned value p1 [...]
   //

clang/lib/Sema/SemaExprCXX.cpp

auto CreateAllocationFunctionDecl = [&](Attr *ExtraAttr) {
     FunctionDecl *Alloc = FunctionDecl::Create(
         Context, GlobalCtx, SourceLocation(), SourceLocation(), Name, FnType,
         /*TInfo=*/nullptr, SC_None, getCurFPFeatures().isFPConstrained(), false,
         true);
     Alloc->setImplicit();
     // Global allocation functions should always be visible.
     Alloc->setVisibleDespiteOwningModule();
 
-    if (HasBadAllocExceptionSpec && getLangOpts().NewInfallible)
+    if (HasBadAllocExceptionSpec && getLangOpts().NewInfallible &&
+        !getLangOpts().CheckNew)
       Alloc->addAttr(
           ReturnsNonNullAttr::CreateImplicit(Context, Alloc->getLocation()));
 
     Alloc->addAttr(VisibilityAttr::CreateImplicit(
         Context, LangOpts.GlobalAllocationFunctionVisibilityHidden
                      ? VisibilityAttr::Hidden
                      : VisibilityAttr::Default));
 

clang/test/CodeGenCXX/fcheck-new.cpp

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang -fcheck-new --target=x86_64-linux-gnu -emit-llvm -S %s -o - | opt -S -mem2reg | FileCheck %s
+
+// CHECK-LABEL: @_Z5test0v(
+// CHECK-NEXT:    [[TMP1:%.*]] = call noalias noundef ptr @_Znwm(i64 noundef 4) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:    ret ptr [[TMP1]]
+//
+int *test0() {
+  return new int;
+}

clang/test/Driver/clang_f_opts.c

 // -fprofile-sample-use should not be passed to the GPU compilation
 // CHECK-CUDA-SAMPLE-PROFILE: "-cc1"
 // CHECK-CUDA-SAMPLE-PROFILE-SAME: "-triple" "nvptx
 // CHECK-CUDA-SAMPLE-PROFILE-NOT: "-fprofile-sample-use={{.*}}/file.prof"
 // Host compilation should still have the option.
 // CHECK-CUDA-SAMPLE-PROFILE: "-cc1"
 // CHECK-CUDA-SAMPLE-PROFILE-SAME: "-fprofile-sample-use={{.*}}/file.prof"
 
 
 // RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof %s 2>&1 | FileCheck -check-prefix=CHECK-AUTO-PROFILE %s
 // CHECK-AUTO-PROFILE: "-fprofile-sample-use={{.*}}/file.prof"
 
 // RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof -fno-profile-sample-use %s 2>&1 | FileCheck -check-prefix=CHECK-NO-AUTO-PROFILE %s
 // RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof -fno-auto-profile %s 2>&1 | FileCheck -check-prefix=CHECK-NO-AUTO-PROFILE %s
 // CHECK-NO-AUTO-PROFILE-NOT: "-fprofile-sample-use={{.*}}/file.prof"
 
 // RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof -fno-profile-sample-use -fauto-profile %s 2>&1 | FileCheck -check-prefix=CHECK-AUTO-PROFILE %s

 // RUN:     -fno-inline-small-functions -finline-small-functions              \
 // RUN:     -fno-fat-lto-objects -ffat-lto-objects                            \
 // RUN:     -flto=auto -flto=jobserver                                        \
 // RUN:     -fno-merge-constants -fmerge-constants                            \
 // RUN:     -fno-caller-saves -fcaller-saves                                  \
 // RUN:     -fno-reorder-blocks -freorder-blocks                              \
 // RUN:     -fno-schedule-insns2 -fschedule-insns2                            \
 // RUN:     -fno-stack-check                                                  \
-// RUN:     -fno-check-new -fcheck-new                                        \
 // RUN:     -ffriend-injection                                                \
 // RUN:     -fno-implement-inlines -fimplement-inlines                        \
 // RUN:     -fstack-check                                                     \
 // RUN:     -fforce-addr                                                      \
 // RUN:     -malign-functions=100                                             \
 // RUN:     -malign-loops=100                                                 \
 // RUN:     -malign-jumps=100                                                 \
 // RUN:     %s 2>&1 | FileCheck --check-prefix=IGNORE %s