Page MenuHomePhabricator

CodeGen: Explicitly initialize structure padding in the -ftrivial-auto-var-init mode
ClosedPublic

Authored by glider on Feb 13 2019, 9:02 AM.

Details

Reviewers
jfb
pcc
rjmccall
Summary

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

Diff Detail

Event Timeline

glider created this revision.Feb 13 2019, 9:02 AM
jfb added a comment.Feb 13 2019, 9:04 AM

Oh this is great, looking forward to it!

glider updated this revision to Diff 186868.Feb 14 2019, 10:26 AM

Somewhat ready.
I chose to replace padding bytes in constants instead of creating a new padded type and generating patternFor for it.

jfb added a comment.Feb 14 2019, 3:29 PM

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.

glider marked an inline comment as done.Feb 15 2019, 2:50 AM
glider added inline comments.
tools/clang/test/CodeGenCXX/auto-var-init.cpp
52

Hm, I suspect we're overly eager here.
Is it really correct to initialize the padding bytes of a fully initialized structure with 0xAA?

glider marked 3 inline comments as done.Feb 15 2019, 6:30 AM

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.

glider updated this revision to Diff 187011.Feb 15 2019, 6:38 AM
glider retitled this revision from [RFC] Initialize structure padding to CodeGen: Explicitly initialize structure padding in the -ftrivial-auto-var-init mode.
glider edited the summary of this revision. (Show Details)
glider added reviewers: jfb, pcc, rjmccall.
glider added subscribers: kcc, dvyukov.

Updated the diff, PTAL

glider marked 2 inline comments as done.Feb 15 2019, 6:38 AM
glider added inline comments.
tools/clang/lib/CodeGen/CGDecl.cpp
1266–1267

Removed both, let me know if I should reinstate the second one.

jfb added a subscriber: kees.Feb 15 2019, 10:20 AM

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.

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.

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; }?
Same with struct paddedpackedarray { struct paddedpacked p[2]; }.

glider marked 3 inline comments as done.Feb 18 2019, 2:02 AM

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.
If at some point we find out that this costs us performance, we can always change this behavior.

glider updated this revision to Diff 187196.Feb 18 2019, 2:03 AM

Fixed JF's comments

@pcc Peter, can you please take a look?

pcc added inline comments.Feb 20 2019, 11:10 AM
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.

glider updated this revision to Diff 187766.Feb 21 2019, 4:41 AM
glider marked 7 inline comments as done.

Addressed Peter's comments.

tools/clang/lib/CodeGen/CGDecl.cpp
1108

Nice catch, thank you!

jfb accepted this revision.Feb 25 2019, 3:09 PM

This LGTM.

This revision is now accepted and ready to land.Feb 25 2019, 3:09 PM
glider closed this revision.Feb 26 2019, 2:46 AM

Landed r354861, thank you!