When generating initializers for local structures in the -ftrivial-auto-var-init mode, explicitly wipe the padding bytes with either 0x00 or 0xAA.
This will allow us to automatically handle the padding when splitting the initialization stores (see https://reviews.llvm.org/D57898).
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Somewhat ready.
I chose to replace padding bytes in constants instead of creating a new padded type and generating patternFor for it.
Comment Actions
This is a good start!
I think eventually we want to put this behind a flag, off by default, just like auto-init. Auto-init should imply padding init for stack values. In a way you're doing the later part, but I think you want to initialize (and test) not just stack values. You want every padding bit to be blown away, whether it's on the heap or on the stack. Of course for heap values, when uninitialized, the padding is also uninitialized (unless you're doing auto-init).
Maybe this narrow patch is a good start, and the rest can be handled later? For example, this seems to handle union *initialization* correctly (says your @__const.test_unmatchedreverse_custom.custom update), but it won't handle union *assignments* correctly when we want to avoid padding leaks. Say I have this:
union U { int i; char c; } u;
void foo() {
u.i = 0xdeadbeef;
// ...
u.c = 0x42; // Here I've left some dead beef behind.
// ...
leak(u.i); // It's UB, but it's leaking the beef!
}We want to handle padding comprehensively, not just initialization. Here you want to blow away whatever was in u.i when assigning to u.c, that way reads of u.i as well as any memcpy or just random reads don't leak what was there before.
You'll want to add tests which use __attribute__((aligned(X)) in weird places.
| tools/clang/lib/CodeGen/CGDecl.cpp | ||
|---|---|---|
| 1053 | Use enum class isPattern { No, Yes }; instead of bool isPattern. | |
| 1056 | You should use dyn_cast here and hoist STy in the if. | |
| 1068 | You should add a test for __attribute__((packed)). | |
| 1109 | It seems like some of this can be factored out to its own function. I'm a bit worried that real-world code will end up spending some amount of time in here, once the patch is further along it might be useful to compile LLVM with itself, and see if there's a compile-time hit. If so, it might be worth memoizing some stuff. | |
| 1266–1267 | I think you can remove this FIXME. | |
| tools/clang/test/CodeGenCXX/auto-var-init.cpp | ||
|---|---|---|
| 52 | Hm, I suspect we're overly eager here. | |
Comment Actions
We want to handle padding comprehensively, not just initialization. Here you want to blow away whatever was in u.i when assigning to u.c, that way reads of u.i as well as any memcpy or just random reads don't leak what was there before.
Wonder if the kernel doesn't contain code that relies on both union fields being available at the same time.
| tools/clang/lib/CodeGen/CGDecl.cpp | ||
|---|---|---|
| 1266–1267 | Removed both, let me know if I should reinstate the second one. | |
Comment Actions
Overall this LGTM, but it would be great to get @pcc to double-check. He's got way more attention to details than I do :)
Did you get a chance to compare compile-time numbers? I want to make sure this doesn't regress compile time in any measurable way.
I sure hope not! This seems like something @kees might know? IMO they'd definitely want to get it fixed because initializing padding removes infoleaks.
| tools/clang/test/CodeGenCXX/auto-var-init.cpp | ||
|---|---|---|
| 52 | Yes, because it wasn't being always initialized before. It was up to the backend to decide whether to synthesize a value into padding or not. Now we're always putting a value in there. That being said, we could put something else that 0xAA if we know adjacent things are being initialized. Sometimes that'll codegen better. That's what I did in patternFor when there's undef: I pick an adjacent byte value, so we might be lucky and get something that's easier to synthesize as an immediate. | |
| 62 | Can you also add a test where you have struct paddedpackednested { struct paddedpacked p1, p2; }? | |
Comment Actions
Did you get a chance to compare compile-time numbers? I want to make sure this doesn't regress compile time in any measurable way.
I've compared compile times for the Linux kernel with -j64 (total user time about 20 minutes). The difference is well beyond the noise.
| tools/clang/test/CodeGenCXX/auto-var-init.cpp | ||
|---|---|---|
| 52 | Ok, since the standard actually doesn't mandate the padding byte values, we can make them pattern bytes for now just for the sake of consistency. | |
| tools/clang/lib/CodeGen/CGDecl.cpp | ||
|---|---|---|
| 1085 | Can this be CurOp = constant->getAggregateElement(i);? | |
| 1096 | I think this needs to be getTypeAllocSize to match the struct layout algorithm: http://llvm-cs.pcc.me.uk/lib/IR/DataLayout.cpp#67 | |
| 1108 | I think that if you use ConstantStruct::getAnon here you won't need to track the types. | |
| 1118 | This could be if (auto *STy = dyn_cast<SequentialType>(OrigTy)). | |
| 1120 | Now this can be Size = STy->getNumElements() (and remove the two lines below). | |
| 1125 | ElemTy = STy->getElementType(). | |
| 1135 | I don't think the cast is necessary. | |
Comment Actions
Addressed Peter's comments.
| tools/clang/lib/CodeGen/CGDecl.cpp | ||
|---|---|---|
| 1108 | Nice catch, thank you! | |
Use enum class isPattern { No, Yes }; instead of bool isPattern.