Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The patch claims to support i386 PLT unwinding but fundamentally breaks it by hardcoding x86-64 DWARF register macros. --
--- Patch [20]: [RFC PATCH v2 20/24] unwind_user/eh_frame/x86: Handle PLT expressions --- commit 75d2af7b6df62f61cc44905f96f37d91b80e466a Author: Jens Remus <[email protected]> unwind_user/eh_frame/x86: Handle PLT expressions This commit adds x86-specific support for handling the CFA expression found in GNU linker generated .eh_frame for PLT. It intends to enable unwinding through GNU linker generated PLT entries on both i386 and x86-64. > diff --git a/arch/x86/include/asm/unwind_user_eh_frame.h > b/arch/x86/include/asm/unwind_user_eh_frame.h > index fdccbda2fe4b1..8268eb1a0ff5a 100644 > --- a/arch/x86/include/asm/unwind_user_eh_frame.h > +++ b/arch/x86/include/asm/unwind_user_eh_frame.h [ ... ] > @@ -15,7 +17,71 @@ > #define EH_FRAME_MIN_DATA_ALIGN -8 > #define EH_FRAME_MAX_DATA_ALIGN -1 > > -#endif > +#endif /* CONFIG_X86_64 */ [ ... ] > +static inline int eh_frame_do_def_cfa_expression(const char *expr, > + int size, > + unsigned long ip, > + struct eh_frame_reg_state > *reg_state) > +{ [ ... ] > + if (!DW_OP_is_breg(sp_op) || !DW_OP_is_breg(ip_op)) > + return -EOPNOTSUPP; > + > + sp_reg = DW_OP_breg_register(sp_op); > + ip_reg = DW_OP_breg_register(ip_op); > + if (sp_reg != EH_FRAME_REG_SP || ip_reg != EH_FRAME_REG_RA) > + return -EOPNOTSUPP; [Severity: Medium] The commit message mentions enabling unwinding for both i386 and x86-64, but does this check inadvertently break i386 support? EH_FRAME_REG_SP and EH_FRAME_REG_RA are defined under CONFIG_X86_64 and correspond to x86-64 DWARF register numbers (7 and 16). For i386, the DWARF register numbers are different (ESP is 4, EIP is 8). When unwinding an i386 PLT expression, such as for a 32-bit compat process running under a 64-bit kernel, will this check unconditionally reject the expression and return -EOPNOTSUPP because 4 does not equal 7? Also, if this code is compiled natively for 32-bit, would this cause a build failure because the EH_FRAME_REG_SP and EH_FRAME_REG_RA macros are not defined outside of the CONFIG_X86_64 block? > + > + /* CFA = (SP + SP_offset) + (((IP & 0xf) >= N) << shift) */ > + reg_state->cfa_rule = CFA_REG_OFFSET; > + reg_state->cfa_regnum = EH_FRAME_REG_SP; > + reg_state->cfa_offset = sp_offset + (((ip & 15) >= n) << shift); > + return 0; > + } > + > + return -EOPNOTSUPP; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=20
