Re: [PATCH 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

[Julien Thierry]

On 9/18/20 10:43 PM, Josh Poimboeuf wrote:
> On Tue, Sep 15, 2020 at 09:12:04AM +0100, Julien Thierry wrote:
> > Architectures without PUSH/POP instructions will always access the stack
> > though memory operations (SRC/DEST_INDIRECT). Make those operations have
> > the same effect on the CFA as PUSH/POP, with no stack pointer
> > modification.
> >
> > Signed-off-by: Julien Thierry <jthierry@xxxxxxxxxx>
> > ---
> >   tools/objtool/check.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > index f45991c2db41..7ff87fa3caec 100644
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -2005,6 +2005,13 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> >   			break;
> >   
> >   		case OP_SRC_REG_INDIRECT:
> > +			if (!cfi->drap && op->dest.reg == cfa->base) {
>
> && op->dest.reg == CFI_BP ?

Does it matter? My unstandig was that the register used to point to the 
CFA is getting overwritten, so we need to fallback to something known 
which is the offset from the stack pointer.

Was that not the case?

> > +
> > +				/* mov disp(%rsp), %rbp */
> > +				cfa->base = CFI_SP;
> > +				cfa->offset = cfi->stack_size;
> > +			}
> > +
> >   			if (cfi->drap && op->src.reg == CFI_BP &&
> >   			    op->src.offset == cfi->drap_offset) {
> >   
> > @@ -2026,6 +2033,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> >   				/* mov disp(%rbp), %reg */
> >   				/* mov disp(%rsp), %reg */
> >   				restore_reg(cfi, op->dest.reg);
> > +			} else if (op->src.reg == CFI_SP &&
>
> An empty line above the else would help readability.
>
> > +				   op->src.offset == regs[op->dest.reg].offset + cfi->stack_size) {
> > +
> > +				/* mov disp(%rsp), %reg */
> > +				restore_reg(cfi, op->dest.reg);
>
> >   			}
> >   
> >   			break;
> > @@ -2103,6 +2115,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> >   			/* mov reg, disp(%rsp) */
> >   			save_reg(cfi, op->src.reg, CFI_CFA,
> >   				 op->dest.offset - cfi->cfa.offset);
> > +		} else if (op->dest.reg == CFI_SP) {
>
> Same here.

I'll add those.

> > +
> > +			/* mov reg, disp(%rsp) */
> > +			save_reg(cfi, op->src.reg, CFI_CFA,
> > +				 op->dest.offset - cfi->stack_size);
> >   		}
> >   
> >   		break;
> > --
> > 2.21.3

--
Julien Thierry