[CIR][CodeGen] handle zero init padding case #1257
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| bool shouldZeroInitPadding() const { | ||
| // In C23 (N3096) $6.7.10: | ||
| // """ | ||
| // If any object is initialized with an empty iniitializer, then it is |
There was a problem hiding this comment.
iniitializer -> initializer
| // initialization. | ||
| // """ | ||
| // | ||
| // From my understanding, the standard is ambiguous in the following two |
There was a problem hiding this comment.
From my understanding, the standard is ambiguous -> The standard seems ambiguous
| // 2. It only mentions padding for empty initializer, but doesn't mention | ||
| // padding for a non empty initialization list. And if the aggregation or | ||
| // union contains elements or members that are aggregates or unions, and | ||
| // some are non empty initializers, while others are empty initiailizers, |
There was a problem hiding this comment.
initiailizers -> initializers
| // and s2.s1.b are unclear. | ||
| // struct S2 s2 = { 'c' }; | ||
| // | ||
| // Here we choose to zero initiailize left bytes of a union type. Because |
There was a problem hiding this comment.
union type. Because -> union type because
| // Here we choose to zero initiailize left bytes of a union type. Because | ||
| // projects like the Linux kernel are relying on this behavior. If we don't | ||
| // explicitly zero initialize them, the undef values can be optimized to | ||
| // return gabage data. We also choose to zero initialize paddings for |
| // aggregates and unions, no matter they are initialized by empty | ||
| // initializers or non empty initializers. This can provide a consistent | ||
| // behavior. So projects like the Linux kernel can rely on it. | ||
| return !getLangOpts().CPlusPlus; |
There was a problem hiding this comment.
After this PR, what's the behavior of these examples in face of C++? Do we match what OG LLVM emits?
| @@ -43,6 +43,19 @@ using namespace clang::CIRGen; | |||
| namespace { | |||
| class ConstExprEmitter; | |||
|
|
|||
| mlir::TypedAttr getPadding(CIRGenModule &CGM, CharUnits size) { | |||
There was a problem hiding this comment.
To make this easier to read, can you rename it to computePadding? You should also make it static.
| @@ -508,6 +516,11 @@ class ConstStructBuilder { | |||
| bool Build(InitListExpr *ILE, bool AllowOverwrite); | |||
| bool Build(const APValue &Val, const RecordDecl *RD, bool IsPrimaryBase, | |||
| const CXXRecordDecl *VTableClass, CharUnits BaseOffset); | |||
|
|
|||
| bool DoZeroInitPadding(const ASTRecordLayout &Layout, unsigned FieldNo, | |||
There was a problem hiding this comment.
DoZeroInitPadding -> ApplyZeroInitPadding might be better
I continue to use
csmithand catch run time bags. Now it's time to fix the layout for the const structs.There is a divergence between const structs generated by CIR and the original codegen. And this PR makes one more step to eliminate it. There are cases where the extra padding is required - and here is a fix for some of them. I did not write extra tests, since the fixes in the existing already covers the code I added. The point is that now the layout for all of these structs in the LLVM IR with and without CIR is the same.