This is an archive of the discontinued LLVM Phabricator instance.

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
rsmith added a subscriber: rsmith.Jun 11 2020, 5:00 PM

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?)

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 added inline comments.Jun 12 2020, 10:30 AM
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
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.

nikic added a subscriber: nikic.Jun 12 2020, 2:01 PM

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.

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 )

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.

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.

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.

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.

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.

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.

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).

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 updated this revision to Diff 272213.EditedJun 19 2020, 4:40 PM
guiand retitled this revision from Introduce partialinit attribute at call sites for stricter poison analysis to Introduce frozen attribute at call sites for stricter poison analysis.
guiand edited the summary of this revision. (Show Details)

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 accepted this revision.Jun 19 2020, 7:21 PM

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?

This revision is now accepted and ready to land.Jun 19 2020, 7:21 PM

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 requested changes to this revision.Jun 20 2020, 12:47 PM

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.

This revision now requires changes to proceed.Jun 20 2020, 12:47 PM

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).

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.

lenary removed a subscriber: lenary.Jun 22 2020, 3:52 AM

@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 updated this revision to Diff 272489.Jun 22 2020, 10:19 AM

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

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

jdoerfert added inline comments.Jun 22 2020, 3:06 PM
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 marked 2 inline comments as done.Jun 22 2020, 3:46 PM
guiand added inline comments.
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 added inline comments.Jun 22 2020, 4:28 PM
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.

bbn added a subscriber: bbn.Jun 22 2020, 8:05 PM

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.

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 added inline comments.Jun 23 2020, 1:32 PM
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.

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.

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.

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 added inline comments.Jun 24 2020, 5:59 PM
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 marked an inline comment as done.Jun 25 2020, 9:47 AM
guiand added inline comments.
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 updated this revision to Diff 273456.Jun 25 2020, 10:57 AM
guiand retitled this revision from Introduce frozen attribute at call sites for stricter poison analysis to Introduce noundef attribute at call sites for stricter poison analysis.
guiand edited the summary of this revision. (Show Details)

Renamed to noundef, added additional checks before determining attribute

guiand marked 3 inline comments as done.Jun 25 2020, 10:58 AM
guiand marked an inline comment as done.Jun 25 2020, 11:13 AM
guiand added inline comments.
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 marked 2 inline comments as not done.Jun 25 2020, 11:13 AM
guiand marked an inline comment as done.Jun 25 2020, 2:49 PM
guiand added inline comments.
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 updated this revision to Diff 273558.Jun 25 2020, 5:03 PM

Made DetermineNoUndef more robust

nikic added inline comments.Jun 27 2020, 1:42 AM
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 added inline comments.Jun 29 2020, 8:27 AM
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 added inline comments.Jun 29 2020, 9:15 AM
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.

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?

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

LibCallSimplifier::optimizePow

guiand updated this revision to Diff 274895.Jul 1 2020, 12:51 PM

Addressed comments, added test for indirect calls

guiand updated this revision to Diff 276487.Jul 8 2020, 10:32 AM

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

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

jdoerfert resigned from this revision.Jul 17 2020, 7:23 PM

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!

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