[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

aqjune added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

guiand added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

eugenis added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

efriedma added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

guiand added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

efriedma added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

guiand added a comment. 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)

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

aqjune added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

eugenis added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

efriedma added a comment. 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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

guiand added inline comments. Comment at: clang/include/clang/AST/Type.h:2139-2141 + /// Check if this type has only two possible values, and so may be lowered to + /// a bool. + bool hasBooleanRepresentation() const; rsmith wrote: > This seems like a

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

aqjune added a comment. 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,

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

guiand added a comment. As it stands, this attribute is applied whether or not msan is enabled, specifically because we think it can be useful in other contexts. As for the negativity of this attribute, it's true that it would be more intuitive to have it be something like `fullinit` instead.

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

rsmith added a comment. 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 /

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

eugenis added a comment. In D81678#2088284 , @efriedma wrote: > I usually like to start reading this sort of patch with the proposed LangRef > change, but I'm not seeing one. > > There are a couple of related issues here in the existing representation of

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

efriedma added a reviewer: aqjune. efriedma added a comment. Herald added a subscriber: wuzish. I usually like to start reading this sort of patch with the proposed LangRef change, but I'm not seeing one. There are a couple of related issues here in the existing representation of IR: 1. The