Introduce noundef attribute at call sites for stricter poison analysis

[guiand]

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

[guiand]

This comment was removed by guiand.

[guiand]

Sorry, closed this mistakenly!

[rsmith]

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

[rsmith]

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]

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]

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]

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]

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

[guiand]

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

[guiand]

Fix typo in MayDropFunctionReturn

[rsmith]

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.

[guiand]

Addressed comments

[guiand]

Updated comment on disable-noundef-args option

[guiand]

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]

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]

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]

Made the compiler flag non-public

[aqjune]

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]

fix type size check for vscale types

[aqjune]

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

[eugenis]

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

[aqjune]

In D81678#2438264, @eugenis wrote:

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]

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.

[eugenis]

In D81678#2503931, @nikic wrote:

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]

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

[guiand]

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]

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

[guiand]

Updated to use -enable-noundef-analysis

[eugenis]

LGTM

[eugenis]

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

[guiand]

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.

[nlopes]

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

Reupload patch to trigger build