From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 91417384A050 for ; Thu, 11 Jul 2024 13:15:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 91417384A050 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 91417384A050 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1720703731; cv=none; b=lq3ayTz2QRC1/GzdqhU56eopXsA1QzcOHj/hKrGtclDBjKmvxwbAtJI1sGPGWD0vVvk3CLye7JPnfwdL4an3CPKRfyKTQtsrf0smay4fTxWc+13Uto6YgH4tZaWl7xxNygFVkKX62XYp7hoE6UfGZOIwEI7cCEb1JS7yNb3gO4c= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1720703731; c=relaxed/simple; bh=jIUirKG0c+4PD4SL2EcV5RSfU2NAEhBSYlZWTFyeFHU=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=b+v/Wtieo+w5U3jowG25z317SbP+zeHe8m2Gi1qb/4qPaW9oTghyn7n5esu8dcJ/BSz0czrjiRefXZol4szA0zm6Pj6InbfmWX6sxmt0Q5iKTesekSGgd79xJmpbUU7hvQwAfZ7SE6mAmxuzYGBh7+5jo0YFr6VyyvQIXjkurPU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 940741007; Thu, 11 Jul 2024 06:15:53 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D037D3F766; Thu, 11 Jul 2024 06:15:27 -0700 (PDT) From: Richard Sandiford To: Indu Bhagat Mail-Followup-To: Indu Bhagat ,binutils@sourceware.org, Richard.Earnshaw@arm.com, richard.sandiford@arm.com Cc: binutils@sourceware.org, Richard.Earnshaw@arm.com Subject: Re: [PATCH,V4 7/8] gas: aarch64: add experimental support for SCFI In-Reply-To: (Indu Bhagat's message of "Wed, 10 Jul 2024 23:30:45 -0700") References: <20240701025404.3361349-1-indu.bhagat@oracle.com> <20240701025404.3361349-8-indu.bhagat@oracle.com> Date: Thu, 11 Jul 2024 14:15:26 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Indu Bhagat writes: > On 7/1/24 12:49, Richard Sandiford wrote: >> Indu Bhagat writes: >>> +{ >>> + enum aarch64_operand_class opnd_class; >>> + unsigned int dw2reg_num = 0; >>> + >>> + opnd_class = aarch64_get_operand_class (opnd->type); >>> + >>> + switch (opnd_class) >>> + { >>> + case AARCH64_OPND_CLASS_FP_REG: >>> + dw2reg_num = opnd->reg.regno + 64; >>> + break; >>> + case AARCH64_OPND_CLASS_SVE_REGLIST: >>> + dw2reg_num = opnd->reglist.first_regno + 64; >>> + break; >>> + case AARCH64_OPND_CLASS_MODIFIED_REG: >>> + case AARCH64_OPND_CLASS_INT_REG: >>> + case AARCH64_OPND_CLASS_ADDRESS: >>> + /* Use a dummy register value in case of WZR, else this will be an >>> + incorrect dependency on REG_SP. */ >>> + if (!sp_allowed_p && opnd->reg.regno == REG_SP) >>> + dw2reg_num = GINSN_DW2_REGNUM_R1_DUMMY; >>> + else >>> + /* For GPRs of our interest (callee-saved regs, SP, FP, LR), >>> + DWARF register number is the same as AArch64 register number. */ >>> + dw2reg_num = opnd->reg.regno; >>> + break; >> >> I think the AARCH64_OPND_CLASS_ADDRESS case should look at opnd->addr >> instead. AARCH64_OPND_CLASS_MODIFIED_REG should look at opnd->shifter. >> > > Made the correction for AARCH64_OPND_CLASS_ADDRESS. > > But for AARCH64_OPND_CLASS_MODIFIED_REG, the register information is > still correct in opnd->reg.regno, IIUC. It seems to me that the only > information available in the opnd->shifter is how the register is > modified by additional work (shift, multiply etc.); This information is > not used by SCFI: > - an add/sub with two source register and destination REG_SP/ REG_FP > makes REG_SP/ REG_FP untraceable. So ignoring the shift amount etc does > not hurt SCFI correctness. > - Cant think of other operations where the shift amount will affect > SCFI correctness.. > > So I am not sure of the "AARCH64_OPND_CLASS_MODIFIED_REG should look at > opnd->shifter." of the review comment. It's more about type correctness. "shifter" is the data associated with AARCH64_OPND_CLASS_MODIFIED_REG and "reg" is the data associated with AARCH64_OPND_CLASS_INT_REG etc. I think it's mostly a coincidence that the "reg" and "shifter" alternatives of the union put the register at the same byte offset from the start of the structure. >>> + default: >>> + as_bad ("Unexpected value in ginsn_dw2_regnum"); >>> + break; >>> + } >>> + >>> + return dw2reg_num; >>> +} >>> + >>> +/* Generate ginsn for addsub instructions with immediate opnd. */ >>> + >>> +static ginsnS * >>> +aarch64_ginsn_addsub_imm (const symbolS *insn_end_sym) >>> +{ >>> + ginsnS *ginsn = NULL; >>> + bool add_p, sub_p; >>> + offsetT src_imm = 0; >>> + unsigned int dst_reg, opnd_reg; >>> + aarch64_opnd_info *dst, *opnd; >>> + ginsnS *(*ginsn_func) (const symbolS *, bool, >>> + enum ginsn_src_type, unsigned int, offsetT, >>> + enum ginsn_src_type, unsigned int, offsetT, >>> + enum ginsn_dst_type, unsigned int, offsetT); >>> + >>> + aarch64_inst *base = &inst.base; >>> + const aarch64_opcode *opcode = base->opcode; >>> + >>> + add_p = aarch64_opcode_subclass_p (opcode, F_ARITH_ADD); >>> + sub_p = aarch64_opcode_subclass_p (opcode, F_ARITH_SUB); >>> + gas_assert (add_p || sub_p); >>> + ginsn_func = add_p ? ginsn_new_add : ginsn_new_sub; >>> + >>> + gas_assert (aarch64_num_of_operands (opcode) == 3); >>> + dst = &base->operands[0]; >>> + opnd = &base->operands[1]; >>> + >>> + dst_reg = ginsn_dw2_regnum (dst, true); >>> + >>> + if (aarch64_gas_internal_fixup_p () && inst.reloc.exp.X_op == O_constant) >>> + src_imm = inst.reloc.exp.X_add_number; >>> + /* For any other relocation type, e.g., in add reg, reg, symbol, skip now >>> + and handle via aarch64_ginsn_unhandled () code path. */ >>> + else if (inst.reloc.type != BFD_RELOC_UNUSED) >>> + return ginsn; >>> + /* FIXME - verify the understanding and remove assert. */ >>> + else >>> + gas_assert (0); >>> + >>> + opnd_reg = ginsn_dw2_regnum (opnd, true); >>> + >>> + ginsn = ginsn_func (insn_end_sym, true, >>> + GINSN_SRC_REG, opnd_reg, 0, >>> + GINSN_SRC_IMM, 0, src_imm, >>> + GINSN_DST_REG, dst_reg, 0); >>> + ginsn_set_where (ginsn); >>> + >>> + return ginsn; >>> +} >>> + >>> +/* Generate ginsn for addsub instructions with reg opnd. */ >>> + >>> +static ginsnS * >>> +aarch64_ginsn_addsub_reg (const symbolS *insn_end_sym) >>> +{ >>> + ginsnS *ginsn = NULL; >>> + bool add_p, sub_p; >>> + unsigned int dst_reg, src1_reg, src2_reg; >>> + aarch64_opnd_info *dst, *src1, *src2; >>> + ginsnS *(*ginsn_func) (const symbolS *, bool, >>> + enum ginsn_src_type, unsigned int, offsetT, >>> + enum ginsn_src_type, unsigned int, offsetT, >>> + enum ginsn_dst_type, unsigned int, offsetT); >>> + >>> + aarch64_inst *base = &inst.base; >>> + const aarch64_opcode *opcode = base->opcode; >>> + >>> + add_p = aarch64_opcode_subclass_p (opcode, F_ARITH_ADD); >>> + sub_p = aarch64_opcode_subclass_p (opcode, F_ARITH_SUB); >>> + gas_assert (add_p || sub_p); >>> + ginsn_func = add_p ? ginsn_new_add : ginsn_new_sub; >>> + >>> + gas_assert (aarch64_num_of_operands (opcode) == 3); >>> + dst = &base->operands[0]; >>> + src1 = &base->operands[1]; >>> + src2 = &base->operands[2]; >>> + >>> + dst_reg = ginsn_dw2_regnum (dst, true); >>> + src1_reg = ginsn_dw2_regnum (src1, true); >>> + src2_reg = ginsn_dw2_regnum (src2, false); >>> + >>> + ginsn = ginsn_func (insn_end_sym, true, >>> + GINSN_SRC_REG, src1_reg, 0, >>> + GINSN_SRC_REG, src2_reg, 0, >>> + GINSN_DST_REG, dst_reg, 0); >>> + ginsn_set_where (ginsn); >> >> It looks like this ignores any shift that is applied to src2_reg. >> > > Yes. An add/sub operation with two source registers makes REG_SP/REG_FP > untraceable (if dest is REG_SP/REG_FP), so any further information > carried forward in the ginsn is of little use for SCFI. > > For a usecase apart from SCFI, yes, this may amount to loss of vital > information. I will add a TBD_GINSN_INFO_LOSS comment. If SCFI doesn't handle add/sub of two registers, then do we still need this code for correctness? I would have expected SCFI to handle unrecognised writes to SP in a conservative way, even if there is no ginsn type defined for the instruction that sets SP. >>> + >>> + ginsn = ginsn_new_mov (insn_end_sym, false, >>> + src_type, src_reg, src_imm, >>> + GINSN_DST_REG, dst_reg, 0); >>> + ginsn_set_where (ginsn); >>> + >>> + return ginsn; >>> +} >>> + >>> +/* Check if an instruction is whitelisted. >>> + >>> + An instruction is a candidate for whitelisting if not generating ginsn for >>> + it, does not affect SCFI correctness. >>> + >>> + TBD_GINSN_GEN_NOT_SCFI. This function assumes GINSN_GEN_SCFI is in effect. >>> + When other ginsn_gen_mode are added, this will need fixing. */ >>> + >>> +static bool >>> +aarch64_ginsn_safe_to_skip_p (void) >>> +{ >>> + bool skip_p = false; >>> + aarch64_opnd_info *opnd = NULL; >>> + unsigned int dw2_regnum; >>> + unsigned int opnd_reg; >>> + int num_opnds = 0; >>> + >>> + aarch64_inst *base = &inst.base; >>> + const aarch64_opcode *opcode = base->opcode; >>> + >>> + /* ATM, whitelisting operations with no operands does not seem to be >>> + necessary. */ >>> + num_opnds = aarch64_num_of_operands (opcode); >>> + if (!num_opnds) >>> + return skip_p; >> >> Things like ERET show that this would be dangerous, so it might be better >> to make the false return explicit (and adjust the comment). >> > > I am not sure I understand the "make false return explicit". I have > adjusted the code and comment: > > /* ATM, whitelisting operations with no operands does not seem to be > necessary. In fact, whitelisting insns like ERET will be > dangerous for > SCFI. So, return false now and bar any such insns from being > whitelisted > altogether. */ > num_opnds = aarch64_num_of_operands (opcode); > if (!num_opnds) > return false; > > Is this what you meant ? Yes, thanks. >>> + if (opnd->type == AARCH64_OPND_Rd_SP) >>> + { >>> + dw2_regnum = ginsn_dw2_regnum (opnd, true); >>> + if (dw2_regnum == REG_SP) >>> + skip_p = true; >>> + } >>> + break; >>> + >>> + default: >>> + break; >>> + } >>> + >>> + return skip_p; >>> +} >>> + >>> +#define AARCH64_GINSN_UNHANDLED_NONE 0 >>> +#define AARCH64_GINSN_UNHANDLED_DEST_REG 1 >>> +#define AARCH64_GINSN_UNHANDLED_CFG 2 >>> +#define AARCH64_GINSN_UNHANDLED_STACKOP 3 >>> +#define AARCH64_GINSN_UNHANDLED_UNEXPECTED 4 >>> + >>> +/* Check the input insn for its impact on the correctness of the synthesized >>> + CFI. Returns an error code to the caller. */ >>> + >>> +static int >>> +aarch64_ginsn_unhandled (void) >>> +{ >>> + int err = AARCH64_GINSN_UNHANDLED_NONE; >>> + aarch64_inst *base = &inst.base; >>> + const aarch64_opcode *opcode = base->opcode; >>> + aarch64_opnd_info *dest = &base->operands[0]; >>> + int num_opnds = aarch64_num_of_operands (opcode); >>> + aarch64_opnd_info *addr; >>> + unsigned int dw2_regnum; >>> + unsigned int addr_reg; >>> + aarch64_opnd_info *opnd = NULL; >>> + unsigned int opnd_reg; >>> + bool sp_allowed_p = false; >>> + >>> + /* All change of flow instructions are important for SCFI. */ >>> + if (opcode->iclass == condbranch >>> + || opcode->iclass == compbranch >>> + || opcode->iclass == testbranch >>> + || opcode->iclass == branch_imm >>> + || opcode->iclass == branch_reg) >>> + err = AARCH64_GINSN_UNHANDLED_CFG; >>> + /* Also, any memory instructions that may involve an update to the stack >>> + pointer or save/restore of callee-saved registers must not be skipped. >>> + Note that, some iclasses cannot be used to push or pop stack because of >>> + disallowed writeback: ldst_unscaled, ldst_regoff, ldst_unpriv, ldstexcl, >>> + loadlit, ldstnapair_offs. FIXME double-check. >>> + Also, these iclasses do not seem to be amenable to being used for >>> + save/restore ops either. FIXME double-check. */ >>> + else if (opcode->iclass == ldstpair_off >>> + || opcode->iclass == ldstpair_indexed >>> + || opcode->iclass == ldst_imm9 >>> + || opcode->iclass == ldst_imm10 >>> + || opcode->iclass == ldst_pos >>> + /* STR Zn are especially complicated as they do not store in the same byte >>> + order for big-endian: STR Qn stores as a 128-bit integer (MSB first), >>> + whereas STR Zn stores as a stream of bytes (LSB first). FIXME Simply punt >>> + on the big-endian and little-endian SVE PCS case for now. */ >>> + || opcode->iclass == sve_misc) >>> + { >>> + opnd = &base->operands[0]; >>> + addr = &base->operands[num_opnds - 1]; >>> + addr_reg = ginsn_dw2_regnum (addr, true); >>> + opnd_reg = ginsn_dw2_regnum (opnd, false); >>> + /* For all skipped memory operations, check if an update to REG_SP or >>> + REG_FP is involved. */ >>> + if ((addr_reg == REG_SP || addr_reg == REG_FP) >>> + && (((addr->addr.postind || addr->addr.preind) && addr->addr.writeback) >>> + || aarch64_scfi_callee_saved_p (opnd_reg))) >>> + >>> + err = AARCH64_GINSN_UNHANDLED_STACKOP; >> >> The way I'd imagined this working is that we'd use two-directional data >> flow to record which callee-saved registers are "protected" at which >> instructions. That is, for saves we'd walk the cfg forward from the >> entry point, recording which registers have been saved earlier in the walk >> (and being conservatively correct for cycles). And for restores we'd >> walk the cfg backwards from the exit points, recording which registers >> are restored later in the walk. If a register is both "saved ealier" and >> "restored later" for a given instruction, it wouldn't matter what the >> instruction does with that register. >> >> (For infinite loops we would pretend that all registers are restored later.) >> >> It seems like instead, we'd generate a warning for: >> >> ... >> str d8, [sp, #...] >> ... >> ld1d z8.b, p0/z, [sp, #...] >> ... >> ldr d8, [sp, #...] >> ... >> ret >> >> even though that should be ok (even without interpreting SVE instructions). >> >> On the other hand, any read or write of a callee-saved register is suspicious >> if it isn't saved earlier or isn't restored later. >> >> In other words, it seems like this information should be context-dependent, >> or used in a context-dependent way. Whether a particular register matters >> depends on whether the register might still contain the caller's data. >> > > We do something similar for non-memory ops: > - In the aarch64_ginsn_unhandled () codepath, if the destination is of > interest (REG_SP, REG_FP), synthesize a GINSN_TYPE_OTHER with the > appropriate destination register if no ginsn has been generated. > - The SCFI machinery checks if this instruction affects SCFI correctness. > - If it does, only then the insn is issued a complaint for and brought > to attention. > > What you suggest is take a similar approach for memory operations too. > I think it makes sense. It needs some work now to accommodate this, but > I think we can re-evaluate after some basic handling for Z registers is > in place ? I plan to work on adding handling for Z registers after this > series is merged. (I have renamed an old testcase involving Z registers > to "scfi-unsupported-2.s" to keep track of the currently unsupported > cases to be worked on in near future, so I dont lose track of this..). Yeah, leaving it till later sounds ok. But I don't think this should depend on recognising Z loads and stores. It should just depend on processing Z registers in the operand lists, and recognising that the low 64 bits of Z8-Z15 are the same as D8-D15. In other words, I imagined the set-up would be: (1) the operand list should tell us whether a register is referenced (2) recognising 8-byte+ loads and stores, plus support arithmetic, lets us recognise instructions that would function as a save or restore (and at what offset) (3) dataflow analysis based on (2) tells us, for a given instruction, which registers might have caller data (4) A reference to possible caller data triggers a warning (references detected by (1), caller data by (3)) Is that roughly how it works? Thanks, Richard