This is an archive of the discontinued LLVM Phabricator instance.

Handle CET for -exception-model sjlj
ClosedPublic

Authored by xiangzhangllvm on Mar 31 2020, 2:15 AM.

Details

Summary

In SjLj exception mode, the old landingpad BB will create a new landingpad BB and use indirect branch jump to the old landingpad BB in lowering.
So we should add 2 endbr for this exception model.

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Mar 31 2020, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 2:15 AM
LuoYuanke added inline comments.Apr 8 2020, 10:55 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
153

Can you comments with an example to elaborate which pointer endbr is needed? Do we need to insert endbr on each EHPad? Two EHPad must be in different MBB, right?

LuoYuanke added inline comments.Apr 8 2020, 11:26 PM
llvm/test/CodeGen/X86/indirect-branch-tracking-eh.ll
6

The test just show an endbr is inserted, but it doesn't demo where the endbr is inserted.

LuoYuanke added inline comments.Apr 8 2020, 11:33 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
157

After build a local test case, I can see in line 39 it has indirect jump to lpad. Can you explain why checking "MF.hasCallSiteLandingPad(Sym)" is needed? Is there any test case for it?

37 .LBB0_7:                                #   in Loop: Header=BB0_6 Depth=1
38         leaq    .LJTI0_0(%rip), %rcx
39         jmpq    *(%rcx,%rax,8)
40 .LBB0_1:                                # %lpad
41                                         #   in Loop: Header=BB0_6 Depth=1
42 .Ltmp2:
43         movl    -132(%rbp), %edi
44         movl    -128(%rbp), %eax
xiangzhangllvm marked 2 inline comments as done.Apr 9 2020, 12:08 AM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
153

Do we need to insert endbr on each EHPad?

Yes
Two EHPad must be in different MBB, right?
Right

I'll update the test case, let it easy to see. Thank you!

157

Because before converting to SjLj exception mode, this information "MF.hasCallSiteLandingPad(Sym) is true" has been established. And it do not changed after converting to SjLj exception mode.

in fact, line 39 jump to line 42(not line 40), .Ltmp2 is a hasCallSiteLandingPad symbol.

xiangzhangllvm marked an inline comment as done.
xiangzhangllvm edited the summary of this revision. (Show Details)
hjl.tools added inline comments.Apr 9 2020, 4:51 AM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
142

Why do you have to iterate the same loop twice?

xiangzhangllvm marked an inline comment as done.Apr 9 2020, 6:05 PM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
142

I move out the condition out of the loop, because in most time the "if" and "else if" is false.
It will rarely come in to the loops (line 149-173).
I think it is more performance than putting these conditions into loop (line 141).
And the code is more readable than before.

llvm/test/CodeGen/X86/indirect-branch-tracking-eh.ll
6

Yes, here just shows there one more endbr in sjlj model than other model. I'll refine it.

xiangzhangllvm marked 3 inline comments as done and an inline comment as not done.Apr 9 2020, 6:30 PM
xiangzhangllvm marked 2 inline comments as done.
LuoYuanke added inline comments.Apr 13 2020, 10:47 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
143

The isEHPad() include CatchSwitch, CatchPad, CleanupPad and LandingPad. Do we want to insert endbr to all of them or just for LandingPad?

xiangzhangllvm marked an inline comment as done.Apr 14 2020, 11:17 PM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
143

The LandingPad is the exception where will be indirectly jump when the exception happen. The others uses direct jump, them belong to the "internal exception handle block" following the LandingPad.

LuoYuanke added inline comments.Apr 14 2020, 11:26 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
143

Yes. Could you extend your test case to cover CatchPad or some other Pad to make sure we don't insert endbr on these blocks.

xiangzhangllvm marked an inline comment as done.Apr 15 2020, 12:01 AM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
143

OK, I'll use CHECK-COUNT =n to limit the count of endbr in the tests.

Update test about endbr checking for sjlj model.
And from the test you can see the endbr is not add before catchpad, total 3 endbr, one at the entry of function, 2nd one before "old landpad" (line62) and last one at the begin of BB (new landpad without name) generated before "old landpad"(line 51)

LuoYuanke added inline comments.Apr 15 2020, 1:07 AM
llvm/test/CodeGen/X86/indirect-branch-tracking-eh.ll
27

Also add a cache pad. Some code as this.

catch:                                            ; preds = %lpad
  %5 = tail call i8* @__cxa_begin_catch(i8* %2) #6
  %6 = bitcast i8* %5 to i32*
xiangzhangllvm marked an inline comment as done.

Add a test to enhance the patch.

llvm/test/CodeGen/X86/indirect-branch-tracking-eh.ll
27

OK, but this will make test a little complex, in fact the patch is focus on dealing with the change of landpad (old +new). So current test is written very simple to show this.
Catch branch is handled in catch.dispatch which usually follow the landpad, they are direct branch.

Add a test to enhance the patch.

This revision is now accepted and ready to land.Apr 19 2020, 6:57 PM
This revision was automatically updated to reflect the committed changes.