samitolvanen accepted this revision. samitolvanen added a comment. This revision is now accepted and ready to land.
Looks good to me, but maybe worth waiting for someone more familiar with compiler-rt to take a look as well. ================ Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92 ; CHECK-NEXT: .Ltmp{{.*}}: ; CHECK-NEXT: nop ; CHECK-NEXT: .word 3238382334 // 0xc105cafe ---------------- peter.smith wrote: > Assuming the test is the equivalent of `-fpatchable-function-entry=1,1` I > think this is the wrong place for the nop, I think it needs to be after the > signature and the loads adjusted. For example with -fsanitize=kcfi > -fpatchable-function-entries=1,1 > ``` > typedef int Fptr(void); > > int function(void) { > return 0; > } > > int call(Fptr* fp) { > return fp(); > } > ``` > Results in code like: > ``` > .word 1670944012 // @call > // 0x6398950c > .Ltmp1: > nop > call: > .Lfunc_begin1: > .cfi_startproc > // %bb.0: // %entry > ldur w16, [x0, #-8] > movk w17, #50598 > movk w17, #14001, lsl #16 > cmp w16, w17 > b.eq .Ltmp2 > brk #0x8220 > .Ltmp2: > br x0 > .Lfunc_end1: > ``` > Note the NOP is after the signature, with the `ldur` having an offset of -8 > and not the usual -4. I think you would need to make sure the signature is a > branch instruction for each target for this scheme to work. No, this looks correct to me. Note that in AsmPrinter the type hash is emitted after the patchable-function-prefix nops, while the KCFI type hash is emitted before them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148785/new/ https://reviews.llvm.org/D148785 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits