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

[Gui Andrade via Phabricator via cfe-commits]

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 CodeGen-specific concern; I'm not sure this makes sense as a query on the Type.
Makes sense, I can move it.


================
Comment at: clang/lib/AST/Type.cpp:2752-2753
+
+  if (const EnumType *ET = getAs<EnumType>())
+    return ET->getDecl()->getIntegerType()->isBooleanType();
+
----------------
rsmith wrote:
> Under `-fstrict-enums` in C++, `enum E { a = 0, b = 1 };` has only two distinct valid values. Should we consider that case?
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?


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:679
 
+void CGRecordLowering::determineMemberPartialInit() {
+  auto hasTailPadding = [&](QualType FieldTypePtr) {
----------------
rsmith wrote:
> 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`?)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678