This is an archive of the discontinued LLVM Phabricator instance.

Deal with return-twice function such as vfork, setjmp when CET-IBT enabled
ClosedPublic

Authored by xiangzhangllvm on May 13 2019, 10:59 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2019, 10:59 PM
MaskRay added inline comments.
lib/Target/X86/X86IndirectBranchTracking.cpp
72

indent with clang-format.

78

++I;

139

for (const MachineInstr &MI : MBB)

xiangzhangllvm marked 3 inline comments as done.May 14 2019, 6:00 PM
xiangzhangllvm added inline comments.
lib/Target/X86/X86IndirectBranchTracking.cpp
72

Hi! Fang rui, Nice to see you here!
It will more than 80 chars if I aligned the "Machine..", Is it suitable to write like this: ?

bool
X86IndirectBranchTrackingPass::addENDBR(MachineBasicBlock &MBB,
                                        MachineBasicBlock::iterator I) const {
78

It seems same with ++I ? More performance ?

139

I used iterator I, because I want to pass I into BuildMI in addENDBR()

MaskRay added inline comments.May 14 2019, 7:24 PM
lib/Target/X86/X86IndirectBranchTracking.cpp
72

You may use clang-format (git diff -U0 --no-color 'HEAD^' | ~/llvm/tools/clang/tools/clang-format/clang-format-diff.py -i -p1). It reformats this block as:

bool X86IndirectBranchTrackingPass::addENDBR(
    MachineBasicBlock &MBB, MachineBasicBlock::iterator I) const {
78

Thank You very much! Fangrui

craig.topper added inline comments.May 15 2019, 6:01 PM
lib/Target/X86/X86IndirectBranchTracking.cpp
76–84

Is this really "I != MBB.end()"? MachineBasicBlock::iterator is an iterator into a linked list and I'm not sure what it means for that to be nullptr.

141

Variable names should be capitalized and use camel case. But I think this variable is unnecessary and you could just call I->isCall from the if.

test/CodeGen/X86/indirect-branch-tracking-r2.ll
10

funtion->function

70

Please reduce this to only the attributes necessary. target-cpu, no-trapping-math, etc. are all unnecessary.

xiangzhangllvm marked 4 inline comments as done.May 15 2019, 6:42 PM
xiangzhangllvm added inline comments.
lib/Target/X86/X86IndirectBranchTracking.cpp
76–84

Here we just want to use a special "value" to let the addENDBR() ignore the argument "I".
Yes, "I" should "point to" an MachineInstr, it should not be MBB.end() too.
I also feel it a little ugly here, but compile with no error. I'll rewrite to if (I != MBB.end()).
Thank you!

141

Yes! make sense!

test/CodeGen/X86/indirect-branch-tracking-r2.ll
10

Yes!

70

OK! I'll simply them. Thank you!

craig.topper added inline comments.May 15 2019, 9:40 PM
lib/Target/X86/X86IndirectBranchTracking.cpp
144

What if we passed I++ here and passed MBB.begin() on the other callsites. Could we then avoid two different cases in addENDBR?

xiangzhangllvm marked 2 inline comments as done.May 16 2019, 6:20 PM
xiangzhangllvm added inline comments.
lib/Target/X86/X86IndirectBranchTracking.cpp
139

I find here line 139 maybe problem:
I should change to

for (MachineBasicBlock::iterator I = MBB.begin(); I != MBB.end();  //because the MBB may update by insert endbr.
144

Hi Craig, we can not avoid.
because the 2 cases in addENDBR(MBB, I) are different.

one case is insert endbr after I
and another case is insert endbr before I

xiangzhangllvm marked an inline comment as done.May 16 2019, 6:27 PM
xiangzhangllvm added inline comments.
lib/Target/X86/X86IndirectBranchTracking.cpp
144

Sorry, I miss understand before. Your suggestion is right!
Here can pass "I+1" not "I++" to addENDBR()

This revision is now accepted and ready to land.May 20 2019, 6:00 PM
This revision was automatically updated to reflect the committed changes.