This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add -fcheck-new support
ClosedPublic

Authored by heatd on May 9 2022, 2:45 PM.

Details

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.

Diff Detail

Event Timeline

heatd created this revision.May 9 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 2:45 PM
heatd requested review of this revision.May 9 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 2:45 PM
jrtc27 added a subscriber: jrtc27.May 9 2022, 3:45 PM
jrtc27 added inline comments.
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 added inline comments.May 9 2022, 5:11 PM
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 added inline comments.May 9 2022, 5:48 PM
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 updated this revision to Diff 428254.May 9 2022, 6:02 PM

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

jrtc27 added inline comments.May 9 2022, 6:05 PM
clang/test/CodeGenCXX/fcheck-new.cpp
2

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

heatd added inline comments.May 9 2022, 6:05 PM
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 added inline comments.May 9 2022, 6:06 PM
clang/test/CodeGenCXX/fcheck-new.cpp
3

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

MaskRay added inline comments.May 9 2022, 11:52 PM
clang/lib/Driver/ToolChains/Clang.cpp
6156

See addOptInFlag in this file.

clang/test/CodeGenCXX/fcheck-new.cpp
2

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

urnathan added inline comments.May 10 2022, 6:51 AM
clang/lib/Sema/SemaDecl.cpp
16150

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

heatd updated this revision to Diff 428433.May 10 2022, 10:38 AM

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

heatd added a comment.May 16 2022, 7:31 AM

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

Ping.

heatd added a comment.May 23 2022, 9:54 AM

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

Ping.

Ping.

heatd added a comment.Jul 23 2022, 5:57 PM

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 requested changes to this revision.Jul 24 2022, 6:08 PM
MaskRay added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
4117 ↗(On Diff #428433)

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.

This revision now requires changes to proceed.Jul 24 2022, 6:08 PM
jrtc27 added inline comments.Jul 24 2022, 7:05 PM
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 added inline comments.Dec 15 2022, 3:00 PM
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 resigned from this revision.Jan 12 2023, 4:59 PM

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

heatd added inline comments.Apr 25 2023, 8:00 AM
clang/lib/Frontend/CompilerInvocation.cpp
4117 ↗(On Diff #428433)

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 added inline comments.Apr 25 2023, 3:24 PM
clang/lib/Frontend/CompilerInvocation.cpp
4117 ↗(On Diff #428433)

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 updated this revision to Diff 517020.Apr 25 2023, 7:05 PM

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

heatd updated this revision to Diff 517029.Apr 25 2023, 7:56 PM

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

MaskRay added inline comments.Apr 26 2023, 6:49 PM
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 added inline comments.Apr 26 2023, 11:14 PM
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 added inline comments.Apr 27 2023, 8:00 PM
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 updated this revision to Diff 530617.Jun 12 2023, 11:36 AM

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 accepted this revision.Jun 12 2023, 6:09 PM
MaskRay added inline comments.
clang/test/CodeGenCXX/fcheck-new.cpp
4

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

This revision is now accepted and ready to land.Jun 12 2023, 6:09 PM
heatd updated this revision to Diff 530755.Jun 12 2023, 8:05 PM

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

This revision was landed with ongoing or failed builds.Jun 23 2023, 10:45 PM
Closed by commit rG52c8f0bb20eb: [clang] Add -fcheck-new support (authored by heatd, committed by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.