[PATCH] D84414: [RISCV] Support Shadow Call Stack

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1c466477ad46: [RISCV] Support Shadow Call Stack (authored by zzheng). Changed prior to commit: https://reviews.llvm.org/D84414?vs=292378&id=292651

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 accepted this revision. jrtc27 added a comment. This revision is now accepted and ready to land. Yes I think everything's been addressed now (though if I keep looking over it I might start nit-picking even more :)). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D84414: [RISCV] Support Shadow Call Stack

asb accepted this revision. asb added a comment. I think once @jrtc27 confirms all her issues are addressed this is good to land. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.org/D84414

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 292378. zzheng marked 2 inline comments as done. zzheng added a comment. Fixed comment and lint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.org/D84414 Files: clang/lib/Driver

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:38 + // Do not save RA to the SCS if it's not saved to the regular stack, + // i.e. RA is not at risk of being to overwritten. + std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo(); --

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng marked 2 inline comments as done. zzheng added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:60 + // sw ra, 0(s2) + // addi s2, s2, 4 + BuildMI(MBB, MI, DL, TII->get(IsRV64 ? RISCV::SD : RISCV::SW)) jrtc27 wrote: > Is it in

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 292372. zzheng marked 9 inline comments as done. zzheng added a comment. Addressed comments by @jrtc27 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.org/D84414 Files: clang/lib

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:86 + + // Get shadow call stack pointer register. + Register SCSPReg = RISCVABI::getSCSPReg(); Pointless comment; remove Comment at: llvm/lib/Target/RISCV/

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 requested changes to this revision. jrtc27 added a comment. This revision now requires changes to proceed. This is currently incompatible with the save/restore libcalls, and I don't think there's any way to avoid that (the restore libcall both loads ra and jumps to it). We should ensure c

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 292274. zzheng added a comment. rebase & ping.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.org/D84414 Files: clang/lib/Driver/SanitizerArgs.cpp clang/lib/Driver/ToolChain.

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng added a comment. ping.. @jrt, @lenary, @asb, IMHO, the patch is in good shape now, all concerns raised in comments has been addressed/answered, is there any additional comments before we can land it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng added a comment. In D84414#2234267 , @lenary wrote: > Ok, so any compilation units without `-fsanitize=shadow-call-stack` should be > compiled with `-ffixed-x18` if you want to link those together. That is > reasonable. My question was whether we

[PATCH] D84414: [RISCV] Support Shadow Call Stack

lenary added a comment. In D84414#2234327 , @pcc wrote: > FWIW, on aarch64 we decided to make `-fsanitize=shadow-call-stack` require > the x18 reservation (instead of implying it) to try to avoid ABI mismatch > problems. That is, it should be safe to mix

[PATCH] D84414: [RISCV] Support Shadow Call Stack

pcc added a comment. FWIW, on aarch64 we decided to make `-fsanitize=shadow-call-stack` require the x18 reservation (instead of implying it) to try to avoid ABI mismatch problems. That is, it should be safe to mix and match code compiled with and without `-fsanitize=shadow-call-stack`. If we ma

[PATCH] D84414: [RISCV] Support Shadow Call Stack

lenary added a comment. In D84414#2234255 , @zzheng wrote: > In D84414#2234112 , @lenary wrote: > >> Why do we have to pass `-ffixed-x18` when compiling? Is it enough to just >> reserve `x18` whenever the function

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng added a comment. In D84414#2234112 , @lenary wrote: > Why do we have to pass `-ffixed-x18` when compiling? Is it enough to just > reserve `x18` whenever the function has the shadow call stack attribute? When SCS is on, x18 must be preserved across

[PATCH] D84414: [RISCV] Support Shadow Call Stack

lenary added a comment. Why do we have to pass `-ffixed-x18` when compiling? Is it enough to just reserve `x18` whenever the function has the shadow call stack attribute? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llv

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 287428. zzheng added a comment. Rebased & ping... IMHO, the patch is in good shape. As we discussed in the bi-weekly meetings, RV32E only has 16 registers. Systems based on RV32E may have limited memory as well. Besides, LLVM does not have full support for R

[PATCH] D84414: [RISCV] Support Shadow Call Stack

lenary added a comment. In D84414#2186257 , @jrtc27 wrote: > There is a possibly-compelling argument against using x18: RV32E only gives > x0-x15, so would not be able to support the current implementation. We discussed this on the RISC-V LLVM Sync-up (b

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 added a comment. There is a possibly-compelling argument against using x18: RV32E only gives x0-x15, so would not be able to support the current implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.o

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 281377. zzheng added a comment. rebased.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.org/D84414 Files: clang/lib/Driver/SanitizerArgs.cpp clang/lib/Driver/ToolChain.cpp

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 281062. zzheng added a comment. clang-formatted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84414/new/ https://reviews.llvm.org/D84414 Files: clang/lib/Driver/SanitizerArgs.cpp clang/lib/Driver/ToolChain

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng marked 2 inline comments as done. zzheng added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:33-37 + std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo(); + if (std::none_of(CSI.begin(), CSI.end(), [](CalleeSavedInfo &CSR) { +retu

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 281038. zzheng marked 7 inline comments as done. zzheng edited the summary of this revision. zzheng added a comment. Addressed styling & code clarity issues. Fixed wrong opcode for RV64. Unlike x18 on AArch64, there's no register that should normally be res

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 added inline comments. Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:2 +; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py +; RUN: llc -mtriple=riscv32-unknown-elf -mattr=+reserve-x18 -verify-machineinstrs < %s \ +; RUN: | FileCheck %s

[PATCH] D84414: [RISCV] Support Shadow Call Stack

pcc added a comment. > Any callee-saved register can be used. In fact, any register may be used, as long as it cannot be used to pass arguments or return values. It may be better to select a temporary register, as `x18` is on aarch64 when not being used as a platform register, so that problems

[PATCH] D84414: [RISCV] Support Shadow Call Stack

pcc added inline comments. Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:12 +; RV32-NEXT:ret +; RV32-NOT: x18 +; Shouldn't it be looking for `s2` since that's how `x18` is spelled in assembly? Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D84414: [RISCV] Support Shadow Call Stack

MaskRay added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:33-37 + std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo(); + if (std::none_of(CSI.begin(), CSI.end(), [](CalleeSavedInfo &CSR) { +return CSR.getReg() == RISCV::X1; + }))

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 requested changes to this revision. jrtc27 added a comment. This revision now requires changes to proceed. 1. Please use local variables with meaningful names for `RISCV::Xn` rather than inlining them everywhere and making it harder at a glance to work out what's going on without knowing

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:95 + .addReg(RISCV::X18) + .addImm(0); +} apazos wrote: > There are thee things to observe here and other reviewers might have some > additional comments: > > - RIS

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng updated this revision to Diff 280210. zzheng marked 3 inline comments as done. zzheng edited the summary of this revision. zzheng added a comment. Using 'BLOCKED' now. clang-formated RISCVFrameLowering.cpp updated style of test/CodeGen/RISCV/shadowcallstack.ll Repository: rG LLVM Gith

[PATCH] D84414: [RISCV] Support Shadow Call Stack

apazos added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:95 + .addReg(RISCV::X18) + .addImm(0); +} There are thee things to observe here and other reviewers might have some additional comments: - RISC-V does not have a re

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 added a comment. Is there a reason for choosing X18? On AArch64 that's either a temporary or saved register depending on ABI, but determined to be the "platform register". Picking X18 on RISC-V because that's the same index as AArch64 seems a little arbitrary, but maybe it happens to mak

[PATCH] D84414: [RISCV] Support Shadow Call Stack

jrtc27 requested changes to this revision. jrtc27 added inline comments. This revision now requires changes to proceed. Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:1-3 +; RUN: llc -verify-machineinstrs -o - %s -mtriple=riscv32-linux-gnu -mattr=+reserve-x18 | FileCheck

[PATCH] D84414: [RISCV] Support Shadow Call Stack

aaron.ballman added inline comments. Comment at: clang/test/CodeGen/shadowcallstack-attr.c:8 +// RUN: %clang_cc1 -triple riscv32-linux-gnu -emit-llvm -o - %s -fsanitize=shadow-call-stack | FileCheck -check-prefix=UNBLACKLISTED %s + Now might be a good opportun

[PATCH] D84414: [RISCV] Support Shadow Call Stack

zzheng created this revision. zzheng added reviewers: apazos, lenary, asb. Herald added subscribers: cfe-commits, aaron.ballman, evandro, luismarques, sameer.abuasal, pzheng, s.egerton, Jim, benna, psnobl, jocewei, PkmX, rkruppe, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, MaskRay,