[PATCH] D36615: [XRay][CodeGen] Use PIC-friendly code in XRay sleds; remove synthetic references in .text

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2807
 
   // Before we switch over, we force a reference to a label inside the
   // xray_fn_idx sections. This makes sure that the xray_fn_idx section is kept
----------------
Remove this comment.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2835
+  // pointers. This should work for both 32-bit and 64-bit platforms. The index
+  // label/symbol we define we mark as "hidden" so they get removed by the
+  // linker when linking.
----------------
Remove this part of the comment.


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1093
+            MCInstBuilder(X86::PUSH64r).addReg(UsedRegs[I]));
         EmitAndCountInstruction(MCInstBuilder(X86::MOV64ri)
                                     .addReg(UsedRegs[I])
----------------
(This is somewhat orthogonal to this patch, but...)

Is this code reachable? The reg-imm form of the mov instruction uses a variable number of bytes depending on the width of the immediate (I think MOV64ri will always select the 64-bit variant which uses 10 bytes), so if the reg-imm form is selected, the jmp instruction above will jump to the wrong instruction. Since this can't possibly work (nor could it have worked before), maybe assert that the operand is a register?


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1100
+          EmitAndCountInstruction(
+              MCInstBuilder(X86::PUSH64r).addReg(UsedRegs[I]));
           EmitAndCountInstruction(MCInstBuilder(X86::MOV64rr)
----------------
Here you are pushing rdi and rsi onto the stack whereas you weren't doing that before.
1) I don't understand why the pushes are necessary now when they weren't before.
2) Since you are pushing one more value than before, is the stack still aligned correctly?


================
Comment at: test/CodeGen/Mips/xray-section-group.ll:17
   ret i32 0
-; CHECK: .section xray_instr_map,"a", at progbits
+; CHECK: .section xray_instr_map,"awo", at progbits
 }
----------------
This test should make sure that the `.section` directive specifies a unique ID and associated section.
https://llvm.org/docs/Extensions.html#id1


================
Comment at: test/CodeGen/Mips/xray-section-group.ll:27
   ret i32 1
-; CHECK: .section xray_instr_map,"aG", at progbits,bar,comdat
+; CHECK: .section xray_instr_map,"aGwo", at progbits,bar,comdat
 }
----------------
Same here for the unique ID.


https://reviews.llvm.org/D36615