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

[rsmith]

It's not entirely clear to me what partialinit means. Does it mean that some bytes of the parameter might be undef / poison? Or does it mean that some bytes of the parameter that (for a struct parameter or array thereof) correspond to a struct member might be undef / poison? (Eg, if I pass a { i8, i32 } to a function, do we need to explicitly say that the three padding bytes are not initialized?)

I agree with @efriedma that a positive property, something like "these byte / bit ranges within the parameter are not undef", would probably fit better, and are likely usable by optimization passes as well as by msan. (Can we put metadata on parameters yet? This would seem well-suited to being modeled as metadata.)

clang/include/clang/AST/Type.h

2139–2141 ↗

(On Diff #270192)

This seems like a CodeGen-specific concern; I'm not sure this makes sense as a query on the Type.

clang/lib/AST/Type.cpp

2752–2753 ↗

(On Diff #270192)

Under -fstrict-enums in C++, enum E { a = 0, b = 1 }; has only two distinct valid values. Should we consider that case?

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

679 ↗

(On Diff #270192)

We already have support for C++'s __has_unique_object_representations (ASTContext::hasUniqueObjectRepresentations), which does something very similar to this. Do we need both? (Are there cases where this is intentionally diverging from __has_unique_object_representations?)

[aqjune]

Hi, this is really interesting. I was interested in statically analyzing whether a value is undef/poison, so I also thought about the existence of this attribute, but I never imagined that MSan would benefit from this attribute as well.

The partialinit attribute is, in some sense, backwards: the definition is essentially "an argument *not* marked partialinit must not contain any poison values". We usually try to avoid negative reasoning like this; I'm afraid it'll make transforms harder to reason about.

An alternative is to invert the meaning of the attribute and put it on all arguments that must be not poison. Those are a lot more common though.

I agree with these two opinions. IIUC, in LLVM IR, attaching attribute to an argument imposes more restriction to the value.
Changing the semantics of IR to raise UB when poison or undef is passed to a function call will affect soundness of optimizations as well: for example, function outlining or dead arg elimination can introduce such function calls.
If the change is big, maybe we can split the patch to (1) implement the attribute (2) enable adding the attribute, so (2) contains all the big & mechanical diffs.

@efriedma
The way that call argument coercion works is unsound in the presence of poison. An integer can't be partially poisoned: it's either poison, or not poison. We probably need to come up with some safer way to pass structs/unions.

This is true, clang frontend may lower an argument with aggregate type into one with large int type (such as i64).
However, can poison value be safely generated in C? Paddings or union with different size may contain undef bits, but not poison. Signed overflow is UB.
Undef value can exist bitwisely, so I think this is an orthogonal issue.

@rsmith
It's not entirely clear to me what partialinit means. Does it mean that some bytes of the parameter might be undef / poison? Or does it mean that some bytes of the parameter that (for a struct parameter or array thereof) correspond to a struct member might be undef / poison? (Eg, if I pass a { i8, i32 } to a function, do we need to explicitly say that the three padding bytes are not initialized?)

For poison, I believe it is now used as a valuewise concept in LLVM, so if the argument is a non-aggregate type such as i32/float/i8* it should be just binary (whether it is poison or not).

If it is agreed that we should have inverted version of partialinit, I'd like to suggest calling it frozen instead... :)
We have the notion of frozen value in LangRef already: it is used as e.g. the branch condition should be frozen, otherwise it is undefined behavior.
If an argument is frozen, it does not have any undef bits or poison value.

[guiand]

clang/include/clang/AST/Type.h

2139–2141 ↗

(On Diff #270192)

Makes sense, I can move it.

clang/lib/AST/Type.cpp

2752–2753 ↗

(On Diff #270192)

I factored this code out of CGExpr.cpp, as it looked like this function governed whether i1 values were lifted to i8 (to meet the requirements of bool). I wanted to avoid struct bool members being marked partialinit. Do you think that would be a worthwhile separate change?

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

679 ↗

(On Diff #270192)

For the purposes of this commit, I think I can change the decisions made in CGRecordLayoutBuilder to defer to ASTContext::hasUniqueObjectRepresentations. In an upcoming change though, I'd like to determine exactly what *kind* of padding is present in the field, most importantly between basic field padding and bitfield/union padding. The reasoning is as follows:

• Union padding (e.g. union { i32 a; i8 b; }) is completely erased from the outgoing LLVM type, as it's lowered to a { i32 }; same thing goes for bitfield tail padding.
• On the other hand, field padding (e.g. in struct { i8 a; i16 b; }) is preserved in the lowered LLVM type { i8, i16 }.

This means that under certain CGCall conventions, if we only observe field padding, we can get away with not using partialinit.

• If we're flattening a struct (without coercion) that contains no "lost" padding, then each function argument will be a fully initialized field of the struct.
• Same thing goes if we're returning an uncoerced LLVM type such as { i8, i16 }: each individual field is still present and fully initialized.

[efriedma]

In D81678#2089041, @aqjune wrote:

@efriedma
The way that call argument coercion works is unsound in the presence of poison. An integer can't be partially poisoned: it's either poison, or not poison. We probably need to come up with some safer way to pass structs/unions.

This is true, clang frontend may lower an argument with aggregate type into one with large int type (such as i64).
However, can poison value be safely generated in C? Paddings or union with different size may contain undef bits, but not poison. Signed overflow is UB.
Undef value can exist bitwisely, so I think this is an orthogonal issue.

In C semantics, an expression can't produce a poison value. As long as variables and allocations are initialized to undef, not poison, there isn't any way to sneak poison into the padding of a variable, so argument passing is sound. So I guess it's not an issue unless we start poisoning uninitialized variables.

[eugenis]

Positive attribute sounds good to me (frozen is not a bad name), but the tests update change is going to be huge. Any concerns about IR size bloat? The attribute will apply to the majority of function arguments, 8 bytes per instance as far as I can see.

Good point about uninitialized variables being undef, not poison. The meaning of the attribute should be "poison or undef (even partial) in this argument is UB". An extension that specifies which bytes, or even bits, of the argument are required to be undef-free would be nice, but seems too much at this point.

Another way to design this thing that would work for MSan, but not so much for general poison/undef analysis, is to emit an intrinsic call on the pre-coerced type to force an MSan check, and then an attribute ("partialinit") on the argument to skip checking the coerced value. This way the frontend has greater control over which parts of the arguments correspond to C++-things, and what does it mean for them to be initialized. I'm not sure I like this design though.

[aqjune]

In D81678#2091089, @eugenis wrote:

Positive attribute sounds good to me (frozen is not a bad name), but the tests update change is going to be huge. Any concerns about IR size bloat? The attribute will apply to the majority of function arguments, 8 bytes per instance as far as I can see.

Hi,

The issue stems from the difference between whether passing poison/undef to fn arg is allowed in C and IR, so (to add a positive attribute) increase in diff and # of attributes will happen, the question is how we can make the increase as small as possible.
To minimize diff, what about additionally introducing a function-level attribute such as args_frozen stating that all arguments are frozen. (e.g define void @f(i32 x, i32 y) args_frozen)?
The attribute will appear at the end of the line, so many CHECK of function signatures will still pass. (e.g. clang/test/CodeGen/mips64-padding-arg.c )

[efriedma]

In terms of the C++ API, we definitely want to provide an API phrased positively in terms of individual arguments, so transforms don't have to deal with inverted logic.

In terms of the actual internal memory representation, or textual IR, maybe we can be a bit more flexible. args_frozen might be a reasonable compromise; the memory overhead should be pretty minimal even if we're attaching it to a bunch of functions, and it should be mostly readable. But maybe a little awkward to explain the textual IR if it has both arg_frozen and args_frozen.

[guiand]

In D81678#2097081, @aqjune wrote:

To minimize diff, what about additionally introducing a function-level attribute such as args_frozen stating that all arguments are frozen. (e.g define void @f(i32 x, i32 y) args_frozen)?

I like this idea, and I think it will vastly reduce the tests diff. I'll try to update this patch with a positive frozen arg attribute and args_frozen function attribute ASAP.

[guiand]

Adding the function attribute turns out to have other challenges. Specifically, we don't want to have transforms remove a frozen from a parameter and then have to go through and update all the other parameters. This might happen if the function is marked with frozen_args; something like ArgumentPromotion, which usually removes attributes, would have to be aware of it.

I spoke with Evgenii about having a "magic" function attribute that's only really exposed at the bitcode/text level. It would get converted to/from the individual parameter frozens so that transforms don't need to know or care that it exists. frozen_args would only serve as a sort of compression keyword in the bitcode/text IR. He wasn't thrilled with the idea but suggested I get peoples' thoughts on it.

[efriedma]

So I guess we've discussed the following alternatives so far:

1. Attach the "frozen" attribute everywhere; this makes the textual IR generated by clang messy, and likely bloats memory usage (not sure by how much).
2. Invert the meaning of the attribute; this makes reasoning about it messy.
3. Have a "frozen" attribute, but have a function attribute "frozen_args" to freeze all arguments. This is slightly messy to access from C++, and messy to modify from C++.
4. Choose one of the previous three for memory, and a different one for textual IR, and do some magic to translate. This makes it harder to understand the in-memory representation from reading textual IR.

I'm not particularly happy with any of these...

Maybe (1) is the least-bad; all the others compromise by making LLVM harder to understand. We can make porting the clang tests easier by adding a cc1 flag to turn off emitting frozen attributes, I guess (so instead of updating the CHECK lines, you could just mechanically update the RUN line).

Maybe worth sending an email to llvm-dev.

[eugenis]

In D81678#2099444, @efriedma wrote:

So I guess we've discussed the following alternatives so far:

1. Attach the "frozen" attribute everywhere; this makes the textual IR generated by clang messy, and likely bloats memory usage (not sure by how much).
2. Invert the meaning of the attribute; this makes reasoning about it messy.
3. Have a "frozen" attribute, but have a function attribute "frozen_args" to freeze all arguments. This is slightly messy to access from C++, and messy to modify from C++.
4. Choose one of the previous three for memory, and a different one for textual IR, and do some magic to translate. This makes it harder to understand the in-memory representation from reading textual IR.

I'm not particularly happy with any of these...

Maybe (1) is the least-bad; all the others compromise by making LLVM harder to understand. We can make porting the clang tests easier by adding a cc1 flag to turn off emitting frozen attributes, I guess (so instead of updating the CHECK lines, you could just mechanically update the RUN line).

I agree that (1) is the easiest to work with and the least error-prone, and that's what we must shoot for in the design. We could do (4) later as an optimization.

Not sure about the cc1 flag - it's an option, but it would mean we are not testing the same thing that is shipped to the users. It will make things a lot easier, on the other hand.

[guiand]

In an email conversation with @rsmith and @eugenis, they raised the issue that it's not necessarily wrong to pass aggregate types by value, even when some fields are uninit.

A relevant excerpt from Richard:

In addition to the union case, there's another strange case for passing class types: in C++17 onwards, this example:

void f(S s);
void g() { f(S()); }

... results in the parameter to f being constructed in place, rather than by calling the copy constructor. (The same happens for a call to "f({})" in C++11 onwards; it's just a lot more common in C++17.) So it is not the case in general that f cannot be called with a partially-uninitialized S object. :-(

From this, it's probably best for now not to mark aggregate types frozen. There's potentially some means to have the compiler prove aggregates must be frozen, but it's best to land frozen scalars first. And for the purposes of msan at least, scalars can account for the majority of the optimizations coming out of frozen.

[aqjune]

In D81678#2099444, @efriedma wrote:

So I guess we've discussed the following alternatives so far:

(snip)

Maybe (1) is the least-bad; all the others compromise by making LLVM harder to understand. We can make porting the clang tests easier by adding a cc1 flag to turn off emitting frozen attributes, I guess (so instead of updating the CHECK lines, you could just mechanically update the RUN line).

In D81678#2099591, @eugenis wrote:

I agree that (1) is the easiest to work with and the least error-prone, and that's what we must shoot for in the design. We could do (4) later as an optimization.

I agree. After going to (1) we can bring optimizations as a follow-up if it needed. I vote for the -cc1 option as a workaround.
BTW, was there a case in the past that needed a massive updates in tests as well? I wonder how it was addressed at that time.

[guiand]

Reversing the meaning of the attribute to frozen and having the code not apply it to records removed pretty much all the logical changes in the code.

I took Juneyoung's suggestion and added a new cc1 flag, -disable-frozen-args (doesn't apply to return position, only argument position frozen attributes), and applied that to some 150 particularly problematic tests. Thousands of other tests were programatically and manually changed to include the new attribute.

[aqjune]

For the LangRef part, LGTM modulo the suggested change. Could anyone review clang and LLVM's updated part?

llvm/docs/LangRef.rst

1644 ↗

(On Diff #272213)

Could you explicitly state that if it is aggregate or vector type all elements and paddings should be frozen too?

[eugenis]

Could you explicitly state that if it is aggregate or vector type all elements and paddings should be frozen too?

If an aggregate is passed as an aggregate at IR level, we should not care about the padding.
Unless it's coerced to an integer.

[jdoerfert]

First, apologies for being late, I didn't properly monitor the list recently.

---

This diff is impossible to review and later to understand.

I would ask you to split it, at least for review purposes, such that (1) lang ref changes, (2) code changes, (3) test changes are separate.
Please also upload it with a full diff (at least for (1) and (2)).

The revision title needs to be updated (maybe add [LangRef] or [Attr]), and the commit message needs more explanation of the reasons and semantics.

---

Lang Ref (https://reviews.llvm.org/differential/changeset/?ref=2023449 <- so people can find it)

I think the spelling is not in line with other attributes and needs to be changed.

I'm unsure if we want the name frozen as it is less helpful to anyone now familiar with the frozen instruction.
In a different thread we concluded that we need some sort of nopoison as an attribute to convey the behavior is UB if the value would be poison.
I would very much prefer a self explanatory spelling here, especially since nopoison will be derived from sources other than the frozen instruction.

In the lang ref this is listed with and shown as string attribute. It should be with the regular ones (as it is implemented as such). It is also not target specific.

---

All in all we need to open this up to more people and go over the lang ref changes again.

[aqjune]

In D81678#2105077, @eugenis wrote:

Could you explicitly state that if it is aggregate or vector type all elements and paddings should be frozen too?

If an aggregate is passed as an aggregate at IR level, we should not care about the padding.
Unless it's coerced to an integer.

Oh sorry, I missed the comment of @guiand above.
I wanted to suggest making things simpler by having its memory representation always have well-defined bits if stored into memory.
But, if it is helpful for MSan's performance because it is well-defined in C, I think it is fine to not care about the paddings.
I suggest explicitly mentioning that we're not considering paddings of aggregates in the LangRef (at the definition of frozen value too if you want; actually I thought it was already described in the definition, but it didn't).

In D81678#2105429, @jdoerfert wrote:

I'm unsure if we want the name frozen as it is less helpful to anyone now familiar with the frozen instruction.
In a different thread we concluded that we need some sort of nopoison as an attribute to convey the behavior is UB if the value would be poison.
I would very much prefer a self explanatory spelling here, especially since nopoison will be derived from sources other than the frozen instruction.

In the nonnull thread I was in favor of nopoison as well, but became to think that nopoison is a bit misleading in this patch because it doesn't talk about undef bit.
Personally I preferred frozen because whenever there is an ambiguity in its semantics, the precise definition can be made by simply updating the notion of a frozen value, but if a more intuitive spelling is consideration I'm open to any suggestions.

[guiand]

@jdoerfert I've separated out changes to the language reference to https://reviews.llvm.org/D82316 as you suggested. I kept the name frozen for now while we reach a consensus regarding its final name.

[guiand]

I've updated this patch to only include the actual implementation of frozen, for easier review.

[guiand]

I've added the test change to yet another diff, which you can find here: https://reviews.llvm.org/D82317

[jdoerfert]

clang/include/clang/Driver/CC1Options.td

507 ↗

(On Diff #272489)

Do we want a second flag for return values or one to disable everything? Having the ability to disable it partially seems odd.

clang/lib/CodeGen/CGCall.cpp

4372

Why would we do this? Function attributes are valid at the call site, no need to copy them.

[guiand]

clang/include/clang/Driver/CC1Options.td

507 ↗

(On Diff #272489)

The attribute is supposed to be a stopgap while not all the tests are up to date. It turns out that the return values are much less complicated to update the call operands, so a flag for disabling return position attributes wasn't necessary.

clang/lib/CodeGen/CGCall.cpp

4372

Do you mean that for some definition: define @foo(i32 frozen %a, i32 frozen %b), it's valid to issue a call instruction like call @foo(i32 %a, i32 %b) and its operands will be correctly identified as frozen? That's the kind of behavior I was seeing and I wasn't sure if it was an error.

[eugenis]

clang/lib/CodeGen/CGCall.cpp

4372

If you see CallBase::paramHasAttr, function definition attributes will be taken into account when available.

I'm not sure what happens for indirect calls that do not have a Callee - we need to make sure that the frontend emits the callsite attributes that match the signature of the call.

[nlopes]

I'm a bit concerned with this patch as it increases the amount of UB that LLVM exploits without any study of the impact.
For example, right now it's ok do this with clang (not with constants; make it less trivial so clang doesn't fold it right away):

int f() { return INT_MAX + 1; }

While technically this is UB in C, when lowered to LLVM IR, this function returns poison.
When the frozen attribute is attached, the function will now trigger UB in LLVM IR as well.
Is this what we want? It would be worthwhile to at least compile a few programs to check if they break. Also, what's the plan to detect these cases in ubsan? We shouldn't be exploiting new UB without having good warnings/checks in place IMO.

Note that pure function calls with 'frozen' arguments become harder to hoist from loops, for example. Since now calling this pure function can trigger UB if one of the arguments is poison. You would need to introduce frozen beforehand. I don't see this issue addressed in this patch.

For msan & friends, this patch makes sense, as they operate in a rely-guarantee way. Initialization of memory is checked, so it can be relied upon by the compiler later on.

[jdoerfert]

In D81678#2109059, @nlopes wrote:

I'm a bit concerned with this patch as it increases the amount of UB that LLVM exploits without any study of the impact.
For example, right now it's ok do this with clang (not with constants; make it less trivial so clang doesn't fold it right away):

int f() { return INT_MAX + 1; }

While technically this is UB in C, when lowered to LLVM IR, this function returns poison.
When the frozen attribute is attached, the function will now trigger UB in LLVM IR as well.
Is this what we want?

I think this is at least a mode we want to support, yes. I would try to make this default if we have proper tooling in place.

It would be worthwhile to at least compile a few programs to check if they break. Also, what's the plan to detect these cases in ubsan? We shouldn't be exploiting new UB without having good warnings/checks in place IMO.

+1, good points, especially the ubsan one.

Note that pure function calls with 'frozen' arguments become harder to hoist from loops, for example. Since now calling this pure function can trigger UB if one of the arguments is poison. You would need to introduce frozen beforehand. I don't see this issue addressed in this patch.

TBH, I don't think this is the case. Pure is not sufficient to hoist if it was not executed at all, only speculatable is. For a speculatable call you should be able to remove frozen from the attributes if you can't prove it. Actually, it might not be a good idea to have speculatable and frozen on the same call, given that they kinda contradict each other. Back to pure, aka readnone (I would assume is what you meant): If it is pure you cannot hoist it as it could cause UB (return 1/arg; or return 1 / 0; for all we know.). Generally, I guess the return value attribute frozen can be dropped though.

For msan & friends, this patch makes sense, as they operate in a rely-guarantee way. Initialization of memory is checked, so it can be relied upon by the compiler later on.

At least the nopoison part is something we need in the IR so we can make violated value attributes cause poison and not UB, e.g., if you pass NULL to a nonnull argument.

[jdoerfert]

clang/lib/CodeGen/CGCall.cpp

4372

If you see CallBase::paramHasAttr, function definition attributes will be taken into account when available.

Yes :)

I'm not sure what happens for indirect calls that do not have a Callee - we need to make sure that the frontend emits the callsite attributes that match the signature of the call.

This would not trigger for indirect calls, f would be nullptr.

[efriedma]

In D81678#2109059, @nlopes wrote:

I'm a bit concerned with this patch as it increases the amount of UB that LLVM exploits without any study of the impact.
For example, right now it's ok do this with clang (not with constants; make it less trivial so clang doesn't fold it right away):

int f() { return INT_MAX + 1; }

While technically this is UB in C, when lowered to LLVM IR, this function returns poison.
When the frozen attribute is attached, the function will now trigger UB in LLVM IR as well.
Is this what we want? It would be worthwhile to at least compile a few programs to check if they break.

Even if we say that it's undefined behavior, we don't have to start converting "ret undef" to "unreachable". I mean, it's something we could consider doing, but we don't have to immediately start doing it just because the attribute exists. I expect that just hooking up the attribute to isGuaranteedNotToBeUndefOrPoison() will have almost no immediate effect.

Also, what's the plan to detect these cases in ubsan?

I don't think this has any practical impact on our goals with sanitizers. We should detect undefined behavior before it gets to the point of actually passing or returning an undef or poison value.

Note that pure function calls with 'frozen' arguments become harder to hoist from loops, for example. Since now calling this pure function can trigger UB if one of the arguments is poison. You would need to introduce frozen beforehand. I don't see this issue addressed in this patch.

A pure function can have undefined behavior, in general.

I guess the interaction between "speculatable" and "frozen" is a little weird. We don't have any optimizations that infer either "speculatable" or "frozen", though, so I'm not sure there's any practical impact here.

[eugenis]

Also, what's the plan to detect these cases in ubsan?

I don't think this has any practical impact on our goals with sanitizers. We should detect undefined behavior before it gets to the point of actually passing or returning an undef or poison value.

MSan will take advantage of this by validating no-undef at runtime before the function call. This will detect more bugs, as well as detect existing bugs earlier, making them easier to reason about.

I'm not sure how we could use this information in ubsan.

[jdoerfert]

We don't have any optimizations that infer either "speculatable" or "frozen", though, so I'm not sure there's any practical impact here.

speculatable is on the shortlist for the Attributor. Will happen eventually. I am sure frozen will follow once it is in. We already came across the "need" in the nonnull conversation for example.

[rsmith]

clang/lib/CodeGen/CGCall.cpp

2196

Other types that we should think about from a padding perspective:

• nullptr_t (which has the same size and alignment as void* but is all padding)
• 80-bit long double and _Complex long double (I don't know whether we guarantee to initialize the 6 padding bytes)
• member pointers might contain padding under some ABI rules -- under the MS ABI, you get a struct containing N pointers followed by M ints, which could have 4 bytes of tail padding on 64-bit targets
• vector types with tail padding (eg, a vector of 3 chars is sometimes passed as an i32 with one byte undef)
• matrix types (presumably the same concerns as for vector types apply here)
• maybe Obj-C object types?
• _ExtInt types (eg, returning an _ExtInt(65) initialized to 0 produces an {i64, i64} containing 7 bytes of undef)

It would be safer to list exactly those types for which we know this assumption is correct rather than assuming that it's correct by default -- I think more than half of the Type subclasses for concrete canonical types can have undef bits in their IR representation.

[guiand]

clang/lib/CodeGen/CGCall.cpp

2196

This is a good point, and I think there was some nuance that I had previously included in determining this for records (since removed) that I didn't think to include in CGCall.cpp.

I think one particular check, llvm::DataLayout::typeSizeEqualsStoreSize, handles a lot of these cases:

• long double
• oddly-sized vectors
• _ExtInt types

I don't know if the matrix type has internal padding (like structs) but if not it would cover that as well.
I believe that the struct with padding wouldn't be an issue as we're excluding record types here.
And I'd have to take a closer look at nullptr_t to see how it behaves here.

~~~

But if I'd like to build an exhaustive list of types I think will work correctly with a check like this:

• scalars
• floating point numbers
• pointers

I think I'd need also need an inner typeSizeEqualsStoreSize check on the base type of vectors/complex numbers.

[guiand]

Renamed to noundef, added additional checks before determining attribute

[guiand]

clang/lib/CodeGen/CGCall.cpp

2196

nullptr_t (which has the same size and alignment as void* but is all padding)

From what I can gather looking at the IR of test/CodeGenCXX/nullptr.cpp, it looks like nullptr_t arguments and return values are represented as normal i8* types but given the special value null. From this it seems like we can mark them noundef without worry?

[guiand]

clang/lib/CodeGen/CGCall.cpp

2196

Actually I've been thinking more about these cases, and they should only really be a problem if clang coerces them away from their native types to something larger.

For example, if clang emits i33 that's not a problem, as potential padding belonging to i33 can be always be determined by looking at the data layout.
But if it emits { i32, i32 } or i64 or something similar, it's introducing new hidden/erased padding and so we can't mark it noundef.
Same thing goes for long double: if it's emitted as x86_fp80 that's no problem.

There's been some confusion wrt this in the LangRef patch as well. I think what we really want is an attribute that indicates the presence of undef bits *not inherent to the IR type*. So (for the sake of argument, we're not handling structs right now) struct { char; short } emitted as { i8, i16 } noundef is fine, but emitting it as i64 is a problem.

[guiand]

Made DetermineNoUndef more robust

[nikic]

llvm/lib/Transforms/Utils/CodeExtractor.cpp

932 ↗

(On Diff #273558)

Not familiar with this code, but based on the placement of other similar attributes like nonnull, this should probably be in the first list.

[jdoerfert]

llvm/lib/Transforms/Utils/CodeExtractor.cpp

932 ↗

(On Diff #273558)

Technically, it should not matter because this is not a function attribute. That said, the first list makes logically more sense. I would even prefer three lists, OK to propagate, not OK, and assertion.

[jdoerfert]

clang/lib/CodeGen/CGCall.cpp

2196

struct { char; short } emitted as { i8, i16 } noundef is fine, but emitting it as i64 is a problem.

FWIW, I agree with that.

[guiand]

I've run across an issue when bootstrapping clang, that manifests itself in the following code when compiled with optimizations:

#include <math.h>

float my_exp2(float a) {
    return pow(2.0, a);
}

With this code, the call to pow is lifted to a exp2, and then the resulting call causes llvm::verifyFunction to fail. The error is that attributes are present past the end of the argument list.
I'm thinking that whatever code does the lifting here fails to remove the attribute list corresponding to the 2.0 parameter here.

Could anyone point me to where I might need to make a change for that?

[efriedma]

In D81678#2120709, @guiand wrote:

Could anyone point me to where I might need to make a change for that?

LibCallSimplifier::optimizePow

[guiand]

Addressed comments, added test for indirect calls

[guiand]

Per @nikic's suggestion, I isolated the LLVM side of the changes to a separate revision D83412, which should be good to go.

[guiand]

Is anything still pending here (besides the tests, of course)?

[jdoerfert]

My concerns have been addressed. I am not the right one to look at the clang changes but what we merged into LLVM was really useful, thanks again for working on this!

[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

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(DiscardValueNames , 1, 0) ///< Discard Value Names from the IR (LLVMContext flag)
 CODEGENOPT(DisableLLVMPasses , 1, 0) ///< Don't run any LLVM IR passes to get
                                      ///< the pristine IR generated by the
                                      ///< frontend.
 CODEGENOPT(DisableLifetimeMarkers, 1, 0) ///< Don't emit any lifetime markers
 CODEGENOPT(DisableO0ImplyOptNone , 1, 0) ///< Don't annonate function with optnone at O0
 CODEGENOPT(ExperimentalStrictFloatingPoint, 1, 0) ///< Enables the new, experimental
                                                   ///< strict floating point.
+CODEGENOPT(EnableNoundefAttrs, 1, 0) ///< Enable emitting `noundef` attributes on IR call arguments and return values
 CODEGENOPT(LegacyPassManager, 1, 0) ///< Use the legacy pass manager.
 CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the new
                                    ///< pass manager.
 CODEGENOPT(DisableRedZone    , 1, 0) ///< Set when -mno-red-zone is enabled.
 CODEGENOPT(EmitCallSiteInfo, 1, 0) ///< Emit call site info only in the case of
                                    ///< '-g' + 'O>0' level.
 CODEGENOPT(IndirectTlsSegRefs, 1, 0) ///< Set when -mno-tls-direct-seg-refs
                                      ///< is specified.

clang/include/clang/Driver/Options.td

def code_completion_brief_comments : Flag<["-"], "code-completion-brief-comments">,
   HelpText<"Include brief documentation comments in code-completion results.">,
   MarshallingInfoFlag<FrontendOpts<"CodeCompleteOpts.IncludeBriefComments">>;
 def code_completion_with_fixits : Flag<["-"], "code-completion-with-fixits">,
   HelpText<"Include code completion results which require small fix-its.">,
   MarshallingInfoFlag<FrontendOpts<"CodeCompleteOpts.IncludeFixIts">>;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">,
   MarshallingInfoFlag<FrontendOpts<"DisableFree">>;
+def enable_noundef_analysis : Flag<["-"], "enable-noundef-analysis">, Group<f_Group>,
+  HelpText<"Enable analyzing function argument and return types for mandatory definedness">,
+  MarshallingInfoFlag<CodeGenOpts<"EnableNoundefAttrs">>;
 def discard_value_names : Flag<["-"], "discard-value-names">,
   HelpText<"Discard value names in LLVM IR">,
   MarshallingInfoFlag<CodeGenOpts<"DiscardValueNames">>;
 def load : Separate<["-"], "load">, MetaVarName<"<dsopath>">,
   HelpText<"Load the named plugin (dynamic shared object)">;
 def plugin : Separate<["-"], "plugin">, MetaVarName<"<name>">,
   HelpText<"Use the named plugin action instead of the default action (use \"help\" to list available options)">;
 def plugin_arg : JoinedAndSeparate<["-"], "plugin-arg-">,
     MetaVarName<"<name> <arg>">,
     HelpText<"Pass <arg> to plugin <name>">;
 def add_plugin : Separate<["-"], "add-plugin">, MetaVarName<"<name>">,
   HelpText<"Use the named plugin action in addition to the default action">,

clang/lib/CodeGen/CGCall.cpp

static void AddAttributesFromFunctionProtoType(ASTContext &Ctx,
   if (!FPT)
     return;
 
   if (!isUnresolvedExceptionSpec(FPT->getExceptionSpecType()) &&
       FPT->isNothrow())
     FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
 }
 
+bool CodeGenModule::MayDropFunctionReturn(const ASTContext &Context,
+                                          QualType ReturnType) {
+  // We can't just discard the return value for a record type with a
+  // complex destructor or a non-trivially copyable type.
+  if (const RecordType *RT =
+          ReturnType.getCanonicalType()->getAs<RecordType>()) {
+    if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl()))
+      return ClassDecl->hasTrivialDestructor();
+  }
+  return ReturnType.isTriviallyCopyableType(Context);
+}
+
 void CodeGenModule::getDefaultFunctionAttributes(StringRef Name,
                                                  bool HasOptnone,
                                                  bool AttrOnCallSite,
                                                llvm::AttrBuilder &FuncAttrs) {
   // OptimizeNoneAttr takes precedence over -Os or -Oz. No warning needed.
   if (!HasOptnone) {
     if (CodeGenOpts.OptimizeSize)
       FuncAttrs.addAttribute(llvm::Attribute::OptimizeForSize);
if (llvm::is_contained(NBA->builtinNames(), "*")) {
     FuncAttrs.addAttribute("no-builtins");
     return;
   }
 
   // And last, add the rest of the builtin names.
   llvm::for_each(NBA->builtinNames(), AddNoBuiltinAttr);
 }
 
+static bool DetermineNoUndef(QualType QTy, CodeGenTypes &Types,
+                             const llvm::DataLayout &DL, const ABIArgInfo &AI,
+                             bool CheckCoerce = true) {
+  llvm::Type *Ty = Types.ConvertTypeForMem(QTy);
+  if (AI.getKind() == ABIArgInfo::Indirect)
+    return true;
+  if (AI.getKind() == ABIArgInfo::Extend)
+    return true;
+  if (!DL.typeSizeEqualsStoreSize(Ty))
+    // TODO: This will result in a modest amount of values not marked noundef
+    // when they could be. We care about values that *invisibly* contain undef
+    // bits from the perspective of LLVM IR.
+    return false;
+  if (CheckCoerce && AI.canHaveCoerceToType()) {
+    llvm::Type *CoerceTy = AI.getCoerceToType();
+    if (llvm::TypeSize::isKnownGT(DL.getTypeSizeInBits(CoerceTy),
+                                  DL.getTypeSizeInBits(Ty)))
+      // If we're coercing to a type with a greater size than the canonical one,
+      // we're introducing new undef bits.
+      // Coercing to a type of smaller or equal size is ok, as we know that
+      // there's no internal padding (typeSizeEqualsStoreSize).
+      return false;
+  }
+  if (QTy->isExtIntType())
+    return true;
+  if (QTy->isReferenceType())
+    return true;
+  if (QTy->isNullPtrType())
+    return false;
+  if (QTy->isMemberPointerType())
+    // TODO: Some member pointers are `noundef`, but it depends on the ABI. For
+    // now, never mark them.
+    return false;
+  if (QTy->isScalarType()) {
+    if (const ComplexType *Complex = dyn_cast<ComplexType>(QTy))
+      return DetermineNoUndef(Complex->getElementType(), Types, DL, AI, false);
+    return true;
+  }
+  if (const VectorType *Vector = dyn_cast<VectorType>(QTy))
+    return DetermineNoUndef(Vector->getElementType(), Types, DL, AI, false);
+  if (const MatrixType *Matrix = dyn_cast<MatrixType>(QTy))
+    return DetermineNoUndef(Matrix->getElementType(), Types, DL, AI, false);
+  if (const ArrayType *Array = dyn_cast<ArrayType>(QTy))
+    return DetermineNoUndef(Array->getElementType(), Types, DL, AI, false);
+
+  // TODO: Some structs may be `noundef`, in specific situations.
+  return false;
+}
+
 /// Construct the IR attribute list of a function or call.
 ///
 /// When adding an attribute, please consider where it should be handled:
 ///
 ///   - getDefaultFunctionAttributes is for attributes that are essentially
 ///     part of the global target configuration (but perhaps can be
 ///     overridden on a per-function basis).  Adding attributes there
 ///     will cause them to also be set in frontends that build on Clang's
if (!AttrOnCallSite) {
     GetCPUAndFeaturesAttributes(CalleeInfo.getCalleeDecl(), FuncAttrs);
   }
 
   // Collect attributes from arguments and return values.
   ClangToLLVMArgMapping IRFunctionArgs(getContext(), FI);
 
   QualType RetTy = FI.getReturnType();
   const ABIArgInfo &RetAI = FI.getReturnInfo();
+  const llvm::DataLayout &DL = getDataLayout();
+
+  // C++ explicitly makes returning undefined values UB. C's rule only applies
+  // to used values, so we never mark them noundef for now.
+  bool HasStrictReturn = getLangOpts().CPlusPlus;
+  if (TargetDecl) {
+    if (const FunctionDecl *FDecl = dyn_cast<FunctionDecl>(TargetDecl))
+      HasStrictReturn &= !FDecl->isExternC();
+    else if (const VarDecl *VDecl = dyn_cast<VarDecl>(TargetDecl))
+      // Function pointer
+      HasStrictReturn &= !VDecl->isExternC();
+  }
+
+  // We don't want to be too aggressive with the return checking, unless
+  // it's explicit in the code opts or we're using an appropriate sanitizer.
+  // Try to respect what the programmer intended.
+  HasStrictReturn &= getCodeGenOpts().StrictReturn ||
+                     !MayDropFunctionReturn(getContext(), RetTy) ||
+                     getLangOpts().Sanitize.has(SanitizerKind::Memory) ||
+                     getLangOpts().Sanitize.has(SanitizerKind::Return);
+
+  // Determine if the return type could be partially undef
+  if (CodeGenOpts.EnableNoundefAttrs && HasStrictReturn) {
+    if (!RetTy->isVoidType() && RetAI.getKind() != ABIArgInfo::Indirect &&
+        DetermineNoUndef(RetTy, getTypes(), DL, RetAI))
+      RetAttrs.addAttribute(llvm::Attribute::NoUndef);
+  }
+
   switch (RetAI.getKind()) {
   case ABIArgInfo::Extend:
     if (RetAI.isSignExt())
       RetAttrs.addAttribute(llvm::Attribute::SExt);
     else
       RetAttrs.addAttribute(llvm::Attribute::ZExt);
     LLVM_FALLTHROUGH;
   case ABIArgInfo::Direct:
if (IRFunctionArgs.hasPaddingArg(ArgNo)) {
       if (AI.getPaddingInReg()) {
         ArgAttrs[IRFunctionArgs.getPaddingArgNo(ArgNo)] =
             llvm::AttributeSet::get(
                 getLLVMContext(),
                 llvm::AttrBuilder().addAttribute(llvm::Attribute::InReg));
       }
     }
 
+    // Decide whether the argument we're handling could be partially undef
+    bool ArgNoUndef = DetermineNoUndef(ParamType, getTypes(), DL, AI);
+    if (CodeGenOpts.EnableNoundefAttrs && ArgNoUndef)
+      Attrs.addAttribute(llvm::Attribute::NoUndef);
+
     // 'restrict' -> 'noalias' is done in EmitFunctionProlog when we
     // have the corresponding parameter variable.  It doesn't make
     // sense to do it here because parameters are so messed up.
     switch (AI.getKind()) {
     case ABIArgInfo::Extend:
       if (AI.isSignExt())
         Attrs.addAttribute(llvm::Attribute::SExt);
       else
llvm::CallInst *CodeGenFunction::EmitRuntimeCall(llvm::FunctionCallee callee,
   llvm::CallInst *call = Builder.CreateCall(
       callee, args, getBundlesForFunclet(callee.getCallee()), name);
   call->setCallingConv(getRuntimeCC());
   return call;
 }
 
 /// Emits a call or invoke to the given noreturn runtime function.
 void CodeGenFunction::EmitNoreturnRuntimeCallOrInvoke(
     llvm::FunctionCallee callee, ArrayRef<llvm::Value *> args) {
   SmallVector<llvm::OperandBundleDef, 1> BundleList =
       getBundlesForFunclet(callee.getCallee());
 
   if (getInvokeDest()) {
     llvm::InvokeInst *invoke =
       Builder.CreateInvoke(callee,
                            getUnreachableBlock(),
                            getInvokeDest(),

clang/lib/CodeGen/CodeGenFunction.cpp

QualType CodeGenFunction::BuildFunctionArgList(GlobalDecl GD,
   }
 
   if (MD && (isa<CXXConstructorDecl>(MD) || isa<CXXDestructorDecl>(MD)))
     CGM.getCXXABI().addImplicitStructorParams(*this, ResTy, Args);
 
   return ResTy;
 }
 
-static bool
-shouldUseUndefinedBehaviorReturnOptimization(const FunctionDecl *FD,
-                                             const ASTContext &Context) {
-  QualType T = FD->getReturnType();
-  // Avoid the optimization for functions that return a record type with a
-  // trivial destructor or another trivially copyable type.
-  if (const RecordType *RT = T.getCanonicalType()->getAs<RecordType>()) {
-    if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl()))
-      return !ClassDecl->hasTrivialDestructor();
-  }
-  return !T.isTriviallyCopyableType(Context);
-}
-
 void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
                                    const CGFunctionInfo &FnInfo) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
   CurGD = GD;
 
   FunctionArgList Args;
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
   //   a value-returning function.
   // C11 6.9.1p12:
   //   If the '}' that terminates a function is reached, and the value of the
   //   function call is used by the caller, the behavior is undefined.
   if (getLangOpts().CPlusPlus && !FD->hasImplicitReturnZero() && !SawAsmBlock &&
       !FD->getReturnType()->isVoidType() && Builder.GetInsertBlock()) {
     bool ShouldEmitUnreachable =
         CGM.getCodeGenOpts().StrictReturn ||
-        shouldUseUndefinedBehaviorReturnOptimization(FD, getContext());
+        !CGM.MayDropFunctionReturn(FD->getASTContext(), FD->getReturnType());
     if (SanOpts.has(SanitizerKind::Return)) {
       SanitizerScope SanScope(this);
       llvm::Value *IsFalse = Builder.getFalse();
       EmitCheck(std::make_pair(IsFalse, SanitizerKind::Return),
                 SanitizerHandler::MissingReturn,
                 EmitCheckSourceLocation(FD->getLocation()), None);
     } else if (ShouldEmitUnreachable) {
       if (CGM.getCodeGenOpts().OptimizationLevel == 0)

clang/lib/CodeGen/CodeGenModule.h

public:
   /// This may either be an MDString (for external identifiers) or a distinct
   /// unnamed MDNode (for internal identifiers).
   llvm::Metadata *CreateMetadataIdentifierGeneralized(QualType T);
 
   /// Create and attach type metadata to the given function.
   void CreateFunctionTypeMetadataForIcall(const FunctionDecl *FD,
                                           llvm::Function *F);
 
+  /// Whether this function's return type has no side effects, and thus may
+  /// be trivially discarded if it is unused.
+  bool MayDropFunctionReturn(const ASTContext &Context, QualType ReturnType);
+
   /// Returns whether this module needs the "all-vtables" type identifier.
   bool NeedAllVtablesTypeId() const;
 
   /// Create and attach type metadata for the given vtable.
   void AddVTableTypeMetadata(llvm::GlobalVariable *VTable, CharUnits Offset,
                              const CXXRecordDecl *RD);
 
   /// Return a vector of most-base classes for RD. This is used to implement

clang/test/CodeGen/attr-noundef.cpp

+// RUN: %clang -cc1 -triple x86_64-gnu-linux -x c++ -S -emit-llvm -enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-INTEL
+// RUN: %clang -cc1 -triple aarch64-gnu-linux -x c++ -S -emit-llvm -enable-noundef-analysis %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-AARCH
+
+//************ Passing structs by value
+// TODO: No structs may currently be marked noundef
+
+namespace check_structs {
+struct Trivial {
+  int a;
+};
+Trivial ret_trivial() { return {}; }
+void pass_trivial(Trivial e) {}
+// CHECK-INTEL: [[DEFINE:define( dso_local)?]] i32 @{{.*}}ret_trivial
+// CHECK-AARCH: [[DEFINE:define( dso_local)?]] i64 @{{.*}}ret_trivial
+// CHECK-INTEL: [[DEFINE]] void @{{.*}}pass_trivial{{.*}}(i32 %
+// CHECK-AARCH: [[DEFINE]] void @{{.*}}pass_trivial{{.*}}(i64 %
+
+struct NoCopy {
+  int a;
+  NoCopy(NoCopy &) = delete;
+};
+NoCopy ret_nocopy() { return {}; }
+void pass_nocopy(NoCopy e) {}
+// CHECK: [[DEFINE]] void @{{.*}}ret_nocopy{{.*}}(%"struct.check_structs::NoCopy"* noalias sret({{[^)]+}}) align 4 %
+// CHECK: [[DEFINE]] void @{{.*}}pass_nocopy{{.*}}(%"struct.check_structs::NoCopy"* noundef %
+
+struct Huge {
+  int a[1024];
+};
+Huge ret_huge() { return {}; }
+void pass_huge(Huge h) {}
+// CHECK: [[DEFINE]] void @{{.*}}ret_huge{{.*}}(%"struct.check_structs::Huge"* noalias sret({{[^)]+}}) align 4 %
+// CHECK: [[DEFINE]] void @{{.*}}pass_huge{{.*}}(%"struct.check_structs::Huge"* noundef
+} // namespace check_structs
+
+//************ Passing unions by value
+// No unions may be marked noundef
+
+namespace check_unions {
+union Trivial {
+  int a;
+};
+Trivial ret_trivial() { return {}; }
+void pass_trivial(Trivial e) {}
+// CHECK-INTEL: [[DEFINE]] i32 @{{.*}}ret_trivial
+// CHECK-AARCH: [[DEFINE]] i64 @{{.*}}ret_trivial
+// CHECK-INTEL: [[DEFINE]] void @{{.*}}pass_trivial{{.*}}(i32 %
+// CHECK-AARCH: [[DEFINE]] void @{{.*}}pass_trivial{{.*}}(i64 %
+
+union NoCopy {
+  int a;
+  NoCopy(NoCopy &) = delete;
+};
+NoCopy ret_nocopy() { return {}; }
+void pass_nocopy(NoCopy e) {}
+// CHECK: [[DEFINE]] void @{{.*}}ret_nocopy{{.*}}(%"union.check_unions::NoCopy"* noalias sret({{[^)]+}}) align 4 %
+// CHECK: [[DEFINE]] void @{{.*}}pass_nocopy{{.*}}(%"union.check_unions::NoCopy"* noundef %
+} // namespace check_unions
+
+//************ Passing `this` pointers
+// `this` pointer must always be defined
+
+namespace check_this {
+struct Object {
+  int data[];
+
+  Object() {
+    this->data[0] = 0;
+  }
+  int getData() {
+    return this->data[0];
+  }
+  Object *getThis() {
+    return this;
+  }
+};
+
+void use_object() {
+  Object obj;
+  obj.getData();
+  obj.getThis();
+}
+// CHECK: define linkonce_odr void @{{.*}}Object{{.*}}(%"struct.check_this::Object"* noundef %
+// CHECK: define linkonce_odr noundef i32 @{{.*}}Object{{.*}}getData{{.*}}(%"struct.check_this::Object"* noundef %
+// CHECK: define linkonce_odr noundef %"struct.check_this::Object"* @{{.*}}Object{{.*}}getThis{{.*}}(%"struct.check_this::Object"* noundef %
+} // namespace check_this
+
+//************ Passing vector types
+
+namespace check_vecs {
+typedef int __attribute__((vector_size(12))) i32x3;
+i32x3 ret_vec() {
+  return {};
+}
+void pass_vec(i32x3 v) {
+}
+
+// CHECK: [[DEFINE]] noundef <3 x i32> @{{.*}}ret_vec{{.*}}()
+// CHECK-INTEL: [[DEFINE]] void @{{.*}}pass_vec{{.*}}(<3 x i32> noundef %
+// CHECK-AARCH: [[DEFINE]] void @{{.*}}pass_vec{{.*}}(<4 x i32> %
+} // namespace check_vecs
+
+//************ Passing exotic types
+// Function/Array pointers, Function member / Data member pointers, nullptr_t, ExtInt types
+
+namespace check_exotic {
+struct Object {
+  int mfunc();
+  int mdata;
+};
+typedef int Object::*mdptr;
+typedef int (Object::*mfptr)();
+typedef decltype(nullptr) nullptr_t;
+typedef int (*arrptr)[32];
+typedef int (*fnptr)(int);
+
+arrptr ret_arrptr() {
+  return nullptr;
+}
+fnptr ret_fnptr() {
+  return nullptr;
+}
+mdptr ret_mdptr() {
+  return nullptr;
+}
+mfptr ret_mfptr() {
+  return nullptr;
+}
+nullptr_t ret_npt() {
+  return nullptr;
+}
+void pass_npt(nullptr_t t) {
+}
+_ExtInt(3) ret_extint() {
+  return 0;
+}
+void pass_extint(_ExtInt(3) e) {
+}
+void pass_large_extint(_ExtInt(127) e) {
+}
+
+// Pointers to arrays/functions are always noundef
+// CHECK: [[DEFINE]] noundef [32 x i32]* @{{.*}}ret_arrptr{{.*}}()
+// CHECK: [[DEFINE]] noundef i32 (i32)* @{{.*}}ret_fnptr{{.*}}()
+
+// Pointers to members are never noundef
+// CHECK: [[DEFINE]] i64 @{{.*}}ret_mdptr{{.*}}()
+// CHECK-INTEL: [[DEFINE]] { i64, i64 } @{{.*}}ret_mfptr{{.*}}()
+// CHECK-AARCH: [[DEFINE]] [2 x i64] @{{.*}}ret_mfptr{{.*}}()
+
+// nullptr_t is never noundef
+// CHECK: [[DEFINE]] i8* @{{.*}}ret_npt{{.*}}()
+// CHECK: [[DEFINE]] void @{{.*}}pass_npt{{.*}}(i8* %
+
+// TODO: for now, ExtInt is only noundef if it is sign/zero-extended
+// CHECK-INTEL: [[DEFINE]] noundef signext i3 @{{.*}}ret_extint{{.*}}()
+// CHECK-AARCH: [[DEFINE]] i3 @{{.*}}ret_extint{{.*}}()
+// CHECK-INTEL: [[DEFINE]] void @{{.*}}pass_extint{{.*}}(i3 noundef signext %
+// CHECK-AARCH: [[DEFINE]] void @{{.*}}pass_extint{{.*}}(i3 %
+// CHECK-INTEL: [[DEFINE]] void @{{.*}}pass_large_extint{{.*}}(i64 %{{.*}}, i64 %
+// CHECK-AARCH: [[DEFINE]] void @{{.*}}pass_large_extint{{.*}}(i127 %
+} // namespace check_exotic

clang/test/CodeGen/indirect-noundef.cpp

+// RUN: %clang -cc1 -x c++ -triple x86_64-unknown-unknown -O0 -emit-llvm -enable-noundef-analysis -o - %s | FileCheck %s
+
+union u1 {
+  int val;
+};
+
+// CHECK: @indirect_callee_int_ptr = [[GLOBAL:(dso_local )?global]] i32 (i32)*
+int (*indirect_callee_int_ptr)(int);
+// CHECK: @indirect_callee_union_ptr = [[GLOBAL]] i32 (i32)*
+union u1 (*indirect_callee_union_ptr)(union u1);
+
+// CHECK: [[DEFINE:define( dso_local)?]] noundef i32 @{{.*}}indirect_callee_int{{.*}}(i32 noundef %
+int indirect_callee_int(int a) { return a; }
+// CHECK: [[DEFINE]] i32 @{{.*}}indirect_callee_union{{.*}}(i32 %
+union u1 indirect_callee_union(union u1 a) {
+  return a;
+}
+
+int main() {
+  // CHECK: call noundef i32 @{{.*}}indirect_callee_int{{.*}}(i32 noundef 0)
+  indirect_callee_int(0);
+  // CHECK: call i32 @{{.*}}indirect_callee_union{{.*}}(i32 %
+  indirect_callee_union((union u1){0});
+
+  indirect_callee_int_ptr = indirect_callee_int;
+  indirect_callee_union_ptr = indirect_callee_union;
+
+  // CHECK: call noundef i32 %{{.*}}(i32 noundef 0)
+  indirect_callee_int_ptr(0);
+  // CHECK: call i32 %{{.*}}(i32 %
+  indirect_callee_union_ptr((union u1){});
+
+  return 0;
+}