[PATCH] D61881: Deal with return-twice function such as vfork, setjmp when CET-IBT enabled

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm marked 4 inline comments as done.
xiangzhangllvm added inline comments.


================
Comment at: lib/Target/X86/X86IndirectBranchTracking.cpp:76
          "Unexpected Endbr opcode");
-
+  if (I != nullptr) {
+    if (EndbrOpcode != I->getOpcode()) {
----------------
craig.topper wrote:
> 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.
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!


================
Comment at: lib/Target/X86/X86IndirectBranchTracking.cpp:141
+         ++I) {
+      bool notcall = !I->isCall();
+      if (notcall)
----------------
craig.topper wrote:
> 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.
Yes! make sense!


================
Comment at: test/CodeGen/X86/indirect-branch-tracking-r2.ll:9
+;; setjmp, sigsetjmp, savectx, vfork, getcontext
+;; setzx is not return twice funtion, should not followed by endbr.
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
----------------
craig.topper wrote:
> funtion->function
Yes!


================
Comment at: test/CodeGen/X86/indirect-branch-tracking-r2.ll:69
+
+attributes #0 = { noinline nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-le    af" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { returns_twice "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="    false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
craig.topper wrote:
> Please reduce this to only the attributes necessary. target-cpu, no-trapping-math, etc. are all unnecessary.
OK! I'll simply them. Thank you!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61881/new/

https://reviews.llvm.org/D61881