Page MenuHomePhabricator

[clang] Add -fcheck-new support
Needs RevisionPublic

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
5984

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
15440

'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

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

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

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
3

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
3

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.