Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Introduce noundef attribute at call sites for stricter poison analysis
ClosedPublic

Authored by guiand on Jun 11 2020, 11:43 AM.

Details

Summary

This change adds a new IR noundef attribute, which denotes when a function call argument or return val may never contain uninitialized bits.

In MemorySanitizer, this attribute enables optimizations which decrease instrumented code size by up to 17% (measured with an instrumented build of clang) . I'll introduce the change allowing msan to take advantage of this information in a separate patch.

Tests as a separate patch:
https://reviews.llvm.org/D82317

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision is now accepted and ready to land.Jul 17 2020, 7:23 PM
guiand closed this revision.Jul 17 2020, 8:13 PM
This comment was removed by guiand.
guiand reopened this revision.Jul 17 2020, 8:14 PM

Sorry, closed this mistakenly!

This revision is now accepted and ready to land.Jul 17 2020, 8:14 PM
rsmith requested changes to this revision.Jul 21 2020, 2:26 PM

I'd like to see some testcases for the C++ side of this. The following things all seem like they might be interesting: passing and returning classes and unions, especially ones that aren't trivially-copyable, nullptr_t, pointers to members, this parameters, VTTs, the "complete object" flag for MS ABI constructors, the this return from constructors in the ARM ABI, parameters and return values of virtual adjustment thunks.

clang/lib/CodeGen/CGCall.cpp
1933

This includes nullptr_t (which is never noundef) and member pointers (which are sometimes noundef, and you'll need to ask the CXXABI implementation if you don't want to conservatively return false).

2198

This only applies in C++. In C, it's valid for a function to omit its return statement if the result value is unused, and in that case the returned value will legitimately be undef.

Even in C++, we're conservative about enforcing the constraint that functions return a proper value and control flow doesn't just fall off the bottom of the function. See the full set of checks here: https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CodeGenFunction.cpp#L1322

This revision now requires changes to proceed.Jul 21 2020, 2:26 PM
rsmith added inline comments.Jul 21 2020, 2:27 PM
clang/lib/CodeGen/CGCall.cpp
2198

It would seem sensible to suppress this for extern "C" functions too, on the basis that the definition of such functions is probably in C code, and thus they might return undef.

guiand updated this revision to Diff 279949.Jul 22 2020, 3:00 PM

Adds additional constraints on noundef: Not a nullptr_t, not a member pointer, and not coerced to a type of larger size. Disabled emitting in return position for non-C++ languages (or inside extern "C").

@rsmith, I just didn't address your comment about being conservative with black-holeing C++ functions returning undef. At least for msan purposes this shouldn't be a *too* much of a problem, but of course this attribute is for general use by LLVM passes. I'm just wondering what kind of tradeoff we want to make here, I suppose.

guiand edited the summary of this revision. (Show Details)Jul 22 2020, 3:08 PM

Just saw your comment about tests as well. The idea was to have all tests ported over as part of a separate commit (I linked it in the main patch description) and then only to push either commit once both are ready to land.

To make it easier to be sure this specific feature is working correctly, I can port over some of those tests to this patch for review purposes.

guiand updated this revision to Diff 280622.Jul 24 2020, 5:03 PM

Added an across-the-board test with several different interesting cases. @rsmith, I'm not sure how to test things like VTables/VTTs, since afaik the way they would be exposed to function attributes would be if a struct is being passed by value. But in that case, we currently never emit noundef.

guiand updated this revision to Diff 281311.Jul 28 2020, 11:46 AM

Incorporate C++'s more conservative checks for omitted return values

guiand updated this revision to Diff 281323.Jul 28 2020, 12:08 PM
guiand edited the summary of this revision. (Show Details)

Fixes regression; allows emitting noundef for non-FunctionDecls as well.

guiand updated this revision to Diff 281345.Jul 28 2020, 1:23 PM

Fix typo in MayDropFunctionReturn

rsmith accepted this revision.Jul 28 2020, 1:45 PM

Looks good with suggestions applied, and with the portability problems in the test fixed. (Maybe just add a -triple? Though it would be good to also test inalloca and ARM constructor this returns and so on.)

clang/include/clang/Driver/Options.td
5003

(since this doesn't affect return values)

clang/lib/CodeGen/CGCall.cpp
2198

OpenCLCPlusPlus should only ever be true when CPlusPlus is also true, so the second half of this || should be unnecessary.

2202–2204

TargetDecl (the callee of a function call) should never be a variable. You shouldn't need this check.

clang/test/CodeGen/attr-noundef.cpp
23

I think this test will fail on 32-bit Windows because e will be passed inalloca.

79

I believe this test will fail on ARM, due to constructors implicitly returning this. Consider either specifying a target for this test or weakening the void to {{.*}} here.

80

I believe this test will fail on Windows, because getData and Object will appear in the opposite order in the mangling. Consider either specifying a target or matching these names either way around.

This revision is now accepted and ready to land.Jul 28 2020, 1:45 PM
guiand updated this revision to Diff 281676.Jul 29 2020, 11:13 AM

Addressed comments

guiand updated this revision to Diff 281679.Jul 29 2020, 11:16 AM

Updated comment on disable-noundef-args option

guiand marked 5 inline comments as done.Jul 29 2020, 11:18 AM
guiand added inline comments.
clang/lib/CodeGen/CGCall.cpp
2202–2204

I tried replacing the body of this if statement with an assertion to make sure, and the assertion fires.

guiand updated this revision to Diff 284907.Aug 11 2020, 3:06 PM

To try to alleviate the tests issue, @eugenis and I discussed that it might be best to take it slow. So now this patch will mask off emitting the attribute on clang tests by default.

guiand requested review of this revision.Aug 11 2020, 3:06 PM

I think I'd like someone to take a look at the llvm-lit changes to see if this makes sense as an approach

guiand updated this revision to Diff 284931.Aug 11 2020, 4:20 PM

Made the compiler flag non-public

aqjune added a comment.Oct 8 2020, 9:25 AM

I think it makes sense - IIUC, for most of the clang tests, noundef won't be the attribute of interest.
For brevity of tests, I think the change is fine.

eugenis updated this revision to Diff 298692.Oct 16 2020, 11:33 AM

fix type size check for vscale types

Hello all,
What is the status of this patch?
Do we need someone who look into the lit change?

This change looks fine to me, but I'm slightly concerned about https://reviews.llvm.org/D85788 - see my last comment on that revision.

This change looks fine to me, but I'm slightly concerned about https://reviews.llvm.org/D85788 - see my last comment on that revision.

Thank you for the link! I left a comment there.

nikic added a comment.Jan 17 2021, 2:35 PM

As the discussion is spread out across multiple threads, do I understand correctly that the current consensus is to introduce the -disable-noundef-analysis flag, and explicitly add it to all the relevant tests (rather than adding it to the substitutions)?

In any case, I'd recommend changing this patch to default -disable-noundef-analysis to true (so you need to compile with -disable-noundef-analysis=0 to get undef attributes). The flag can then be flipped together with the test changes. That should help get the main technical change landed directly, and avoid the need of landing patches at the same time.

As the discussion is spread out across multiple threads, do I understand correctly that the current consensus is to introduce the -disable-noundef-analysis flag, and explicitly add it to all the relevant tests (rather than adding it to the substitutions)?

Yes, I think so.

In any case, I'd recommend changing this patch to default -disable-noundef-analysis to true (so you need to compile with -disable-noundef-analysis=0 to get undef attributes). The flag can then be flipped together with the test changes. That should help get the main technical change landed directly, and avoid the need of landing patches at the same time.

This is a great idea! Aside from splitting the complexity of landing the large change, it also makes our downstream cleanup easier.

In that case we should probably give the flag a positive name: -enable-noundef-analysis=(0|1).

nikic added a comment.Feb 4 2021, 2:42 PM

@guiand Do you plan to continue working on this patch? If not, I would try to finalize it.

guiand added a comment.Feb 4 2021, 3:10 PM

Changing the mechanism to explicit opt-in seems like a good idea, and bypasses the test issues.

If everyone agrees about proceeding with this, I can fix up this patch within the next couple days and we can hopefully wrap this up!

aqjune added a comment.Feb 8 2021, 8:55 PM

I'm fine with any direction that helps people land test updates/implementations.

guiand updated this revision to Diff 322899.Feb 10 2021, 7:43 PM

Updated to use -enable-noundef-analysis

eugenis accepted this revision.Feb 11 2021, 5:34 PM

LGTM

This revision is now accepted and ready to land.Feb 11 2021, 5:34 PM

Any more comments/opinions?
Gui, would you like to push it to main if no one objects?

For sure. I'll upload a rebased patch shortly and give it another day or so for people to look, and then push if there aren't any issues.

@rsmith already gave his blessing, so please go ahead.

I hope you guys have a plan to enable this by default at some point as features behind a flag get rotten and no one uses them.

guiand updated this revision to Diff 328055.Mar 4 2021, 12:33 AM

Reupload patch to trigger build

guiand updated this revision to Diff 328060.Mar 4 2021, 12:44 AM
ychen added a subscriber: ychen.Oct 7 2021, 4:44 PM