2020-04-23 12:54:52

by Peter Zijlstra

Subject: [PATCH 3/8] objtool: Rework allocating stack_ops on decode

Wrap each stack_op in a macro that allocates and adds it to the list.
This simplifies trying to figure out what to do with the pre-allocated
stack_op at the end.

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
tools/objtool/arch/x86/decode.c | 257 +++++++++++++++++++++++-----------------
1 file changed, 153 insertions(+), 104 deletions(-)

--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
return insn->offset + insn->len + insn->immediate;
}

+#define PUSH_OP(op) \
+({ \
+ list_add_tail(&op->list, ops_list); \
+ NULL; \
+})
+
+#define ADD_OP(op) \
+ if (!(op = calloc(1, sizeof(*op)))) \
+ return -1; \
+ else for (; op; op = PUSH_OP(op))
+
int arch_decode_instruction(struct elf *elf, struct section *sec,
unsigned long offset, unsigned int maxlen,
unsigned int *len, enum insn_type *type,
@@ -88,7 +99,7 @@ int arch_decode_instruction(struct elf *
unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
modrm_reg = 0, sib = 0;
- struct stack_op *op;
+ struct stack_op *op = NULL;

x86_64 = is_x86_64(elf);
if (x86_64 == -1)
@@ -129,10 +140,6 @@ int arch_decode_instruction(struct elf *
if (insn.sib.nbytes)
sib = insn.sib.bytes[0];

- op = calloc(1, sizeof(*op));
- if (!op)
- return -1;
-
switch (op1) {

case 0x1:
@@ -141,10 +148,12 @@ int arch_decode_instruction(struct elf *

/* add/sub reg, %rsp */
*type = INSN_STACK;
- op->src.type = OP_SRC_ADD;
- op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_ADD;
+ op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }
}
break;

@@ -152,9 +161,11 @@ int arch_decode_instruction(struct elf *

/* push reg */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG;
- op->src.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
- op->dest.type = OP_DEST_PUSH;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG;
+ op->src.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
+ op->dest.type = OP_DEST_PUSH;
+ }

break;

@@ -162,9 +173,11 @@ int arch_decode_instruction(struct elf *

/* pop reg */
*type = INSN_STACK;
- op->src.type = OP_SRC_POP;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
+ ADD_OP(op) {
+ op->src.type = OP_SRC_POP;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
+ }

break;

@@ -172,8 +185,10 @@ int arch_decode_instruction(struct elf *
case 0x6a:
/* push immediate */
*type = INSN_STACK;
- op->src.type = OP_SRC_CONST;
- op->dest.type = OP_DEST_PUSH;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_CONST;
+ op->dest.type = OP_DEST_PUSH;
+ }
break;

case 0x70 ... 0x7f:
@@ -188,11 +203,13 @@ int arch_decode_instruction(struct elf *
if (modrm == 0xe4) {
/* and imm, %rsp */
*type = INSN_STACK;
- op->src.type = OP_SRC_AND;
- op->src.reg = CFI_SP;
- op->src.offset = insn.immediate.value;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_AND;
+ op->src.reg = CFI_SP;
+ op->src.offset = insn.immediate.value;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }
break;
}

@@ -205,11 +222,13 @@ int arch_decode_instruction(struct elf *

/* add/sub imm, %rsp */
*type = INSN_STACK;
- op->src.type = OP_SRC_ADD;
- op->src.reg = CFI_SP;
- op->src.offset = insn.immediate.value * sign;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_ADD;
+ op->src.reg = CFI_SP;
+ op->src.offset = insn.immediate.value * sign;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }
break;

case 0x89:
@@ -217,10 +236,12 @@ int arch_decode_instruction(struct elf *

/* mov %rsp, reg */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG;
- op->src.reg = CFI_SP;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG;
+ op->src.reg = CFI_SP;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+ }
break;
}

@@ -228,10 +249,12 @@ int arch_decode_instruction(struct elf *

/* mov reg, %rsp */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG;
- op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG;
+ op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }
break;
}

@@ -242,21 +265,25 @@ int arch_decode_instruction(struct elf *

/* mov reg, disp(%rbp) */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG;
- op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
- op->dest.type = OP_DEST_REG_INDIRECT;
- op->dest.reg = CFI_BP;
- op->dest.offset = insn.displacement.value;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG;
+ op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ op->dest.type = OP_DEST_REG_INDIRECT;
+ op->dest.reg = CFI_BP;
+ op->dest.offset = insn.displacement.value;
+ }

} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {

/* mov reg, disp(%rsp) */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG;
- op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
- op->dest.type = OP_DEST_REG_INDIRECT;
- op->dest.reg = CFI_SP;
- op->dest.offset = insn.displacement.value;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG;
+ op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ op->dest.type = OP_DEST_REG_INDIRECT;
+ op->dest.reg = CFI_SP;
+ op->dest.offset = insn.displacement.value;
+ }
}

break;
@@ -266,22 +293,26 @@ int arch_decode_instruction(struct elf *

/* mov disp(%rbp), reg */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG_INDIRECT;
- op->src.reg = CFI_BP;
- op->src.offset = insn.displacement.value;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG_INDIRECT;
+ op->src.reg = CFI_BP;
+ op->src.offset = insn.displacement.value;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ }

} else if (rex_w && !rex_b && sib == 0x24 &&
modrm_mod != 3 && modrm_rm == 4) {

/* mov disp(%rsp), reg */
*type = INSN_STACK;
- op->src.type = OP_SRC_REG_INDIRECT;
- op->src.reg = CFI_SP;
- op->src.offset = insn.displacement.value;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ ADD_OP(op) {
+ op->src.type = OP_SRC_REG_INDIRECT;
+ op->src.reg = CFI_SP;
+ op->src.offset = insn.displacement.value;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
+ }
}

break;
@@ -290,27 +321,31 @@ int arch_decode_instruction(struct elf *
if (sib == 0x24 && rex_w && !rex_b && !rex_x) {

*type = INSN_STACK;
- if (!insn.displacement.value) {
- /* lea (%rsp), reg */
- op->src.type = OP_SRC_REG;
- } else {
- /* lea disp(%rsp), reg */
- op->src.type = OP_SRC_ADD;
- op->src.offset = insn.displacement.value;
+ ADD_OP(op) {
+ if (!insn.displacement.value) {
+ /* lea (%rsp), reg */
+ op->src.type = OP_SRC_REG;
+ } else {
+ /* lea disp(%rsp), reg */
+ op->src.type = OP_SRC_ADD;
+ op->src.offset = insn.displacement.value;
+ }
+ op->src.reg = CFI_SP;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
}
- op->src.reg = CFI_SP;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];

} else if (rex == 0x48 && modrm == 0x65) {

/* lea disp(%rbp), %rsp */
*type = INSN_STACK;
- op->src.type = OP_SRC_ADD;
- op->src.reg = CFI_BP;
- op->src.offset = insn.displacement.value;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_ADD;
+ op->src.reg = CFI_BP;
+ op->src.offset = insn.displacement.value;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }

} else if (rex == 0x49 && modrm == 0x62 &&
insn.displacement.value == -8) {
@@ -322,11 +357,13 @@ int arch_decode_instruction(struct elf *
* stack realignment.
*/
*type = INSN_STACK;
- op->src.type = OP_SRC_ADD;
- op->src.reg = CFI_R10;
- op->src.offset = -8;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_ADD;
+ op->src.reg = CFI_R10;
+ op->src.offset = -8;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }

} else if (rex == 0x49 && modrm == 0x65 &&
insn.displacement.value == -16) {
@@ -338,11 +375,13 @@ int arch_decode_instruction(struct elf *
* stack realignment.
*/
*type = INSN_STACK;
- op->src.type = OP_SRC_ADD;
- op->src.reg = CFI_R13;
- op->src.offset = -16;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_ADD;
+ op->src.reg = CFI_R13;
+ op->src.offset = -16;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }
}

break;
@@ -350,8 +389,10 @@ int arch_decode_instruction(struct elf *
case 0x8f:
/* pop to mem */
*type = INSN_STACK;
- op->src.type = OP_SRC_POP;
- op->dest.type = OP_DEST_MEM;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_POP;
+ op->dest.type = OP_DEST_MEM;
+ }
break;

case 0x90:
@@ -361,15 +402,19 @@ int arch_decode_instruction(struct elf *
case 0x9c:
/* pushf */
*type = INSN_STACK;
- op->src.type = OP_SRC_CONST;
- op->dest.type = OP_DEST_PUSHF;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_CONST;
+ op->dest.type = OP_DEST_PUSHF;
+ }
break;

case 0x9d:
/* popf */
*type = INSN_STACK;
- op->src.type = OP_SRC_POPF;
- op->dest.type = OP_DEST_MEM;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_POPF;
+ op->dest.type = OP_DEST_MEM;
+ }
break;

case 0x0f:
@@ -405,15 +450,19 @@ int arch_decode_instruction(struct elf *

/* push fs/gs */
*type = INSN_STACK;
- op->src.type = OP_SRC_CONST;
- op->dest.type = OP_DEST_PUSH;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_CONST;
+ op->dest.type = OP_DEST_PUSH;
+ }

} else if (op2 == 0xa1 || op2 == 0xa9) {

/* pop fs/gs */
*type = INSN_STACK;
- op->src.type = OP_SRC_POP;
- op->dest.type = OP_DEST_MEM;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_POP;
+ op->dest.type = OP_DEST_MEM;
+ }
}

break;
@@ -427,7 +476,8 @@ int arch_decode_instruction(struct elf *
* pop bp
*/
*type = INSN_STACK;
- op->dest.type = OP_DEST_LEAVE;
+ ADD_OP(op)
+ op->dest.type = OP_DEST_LEAVE;

break;

@@ -449,12 +499,14 @@ int arch_decode_instruction(struct elf *
case 0xcf: /* iret */
*type = INSN_EXCEPTION_RETURN;

- /* add $40, %rsp */
- op->src.type = OP_SRC_ADD;
- op->src.reg = CFI_SP;
- op->src.offset = 5*8;
- op->dest.type = OP_DEST_REG;
- op->dest.reg = CFI_SP;
+ ADD_OP(op) {
+ /* add $40, %rsp */
+ op->src.type = OP_SRC_ADD;
+ op->src.reg = CFI_SP;
+ op->src.offset = 5*8;
+ op->dest.type = OP_DEST_REG;
+ op->dest.reg = CFI_SP;
+ }
break;

case 0xca: /* retf */
@@ -492,8 +544,10 @@ int arch_decode_instruction(struct elf *

/* push from mem */
*type = INSN_STACK;
- op->src.type = OP_SRC_CONST;
- op->dest.type = OP_DEST_PUSH;
+ ADD_OP(op) {
+ op->src.type = OP_SRC_CONST;
+ op->dest.type = OP_DEST_PUSH;
+ }
}

break;
@@ -504,11 +558,6 @@ int arch_decode_instruction(struct elf *

*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;

- if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
- list_add_tail(&op->list, ops_list);
- else
- free(op);
-
return 0;
}

2020-04-23 15:58:46

by Peter Zijlstra

Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode

On Thu, Apr 23, 2020 at 05:40:38PM +0200, Alexandre Chartre wrote:

> > @@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
> > return insn->offset + insn->len + insn->immediate;
> > }
> > +#define PUSH_OP(op) \
> > +({ \
> > + list_add_tail(&op->list, ops_list); \
> > + NULL; \
> > +})
> > +
> > +#define ADD_OP(op) \
> > + if (!(op = calloc(1, sizeof(*op)))) \
> > + return -1; \
> > + else for (; op; op = PUSH_OP(op))
> > +
>
> I would better have a function to alloc+add op instead of weird macros,
> for example:
>
> static struct stack_op *add_op(void)
> {
> struct stack *op;
>
> op = calloc(1, sizeof(*op));
> if (!op)
> return NULL;
> list_add_tail(&op->list, ops_list);
> }
>
> Then it requires two more lines when using it but I think the code is much
> cleaner and clearer, e.g.:
>
> op = add_op();
> if (!op)
> return -1;
> op->src.type = OP_SRC_ADD;
> op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> op->dest.type = OP_DEST_REG;
> op->dest.reg = CFI_SP;

The 'problem' which this is that it doesn't NULL op again, so any later
use will do 'funny' things instead of crashing sensibly. Also, I'm
mightly lazy, I don't like endlessly repeating the same things.

2020-04-23 16:21:48

by Peter Zijlstra

Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode

On Thu, Apr 23, 2020 at 05:40:38PM +0200, Alexandre Chartre wrote:
>
> On 4/23/20 2:47 PM, Peter Zijlstra wrote:
> > Wrap each stack_op in a macro that allocates and adds it to the list.
> > This simplifies trying to figure out what to do with the pre-allocated
> > stack_op at the end.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > tools/objtool/arch/x86/decode.c | 257 +++++++++++++++++++++++-----------------
> > 1 file changed, 153 insertions(+), 104 deletions(-)
> >
> > --- a/tools/objtool/arch/x86/decode.c
> > +++ b/tools/objtool/arch/x86/decode.c
> > @@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
> > return insn->offset + insn->len + insn->immediate;
> > }
> > +#define PUSH_OP(op) \
> > +({ \
> > + list_add_tail(&op->list, ops_list); \
> > + NULL; \
> > +})
> > +
> > +#define ADD_OP(op) \
> > + if (!(op = calloc(1, sizeof(*op)))) \
> > + return -1; \
> > + else for (; op; op = PUSH_OP(op))
> > +
>
> I would better have a function to alloc+add op instead of weird macros,
> for example:

I know it'll not make you feel (much) better, but we can write it
shorter:

+#define ADD_OP(op) \
+ if (!(op = calloc(1, sizeof(*op)))) \
+ return -1; \
+ else for (list_add_tail(&op->list, ops_list); op; op = NULL)

Also, the kernel is full of funny macros like this ;-)

2020-04-23 19:32:48

by Alexandre Chartre

Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode

On 4/23/20 2:47 PM, Peter Zijlstra wrote:
> Wrap each stack_op in a macro that allocates and adds it to the list.
> This simplifies trying to figure out what to do with the pre-allocated
> stack_op at the end.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> tools/objtool/arch/x86/decode.c | 257 +++++++++++++++++++++++-----------------
> 1 file changed, 153 insertions(+), 104 deletions(-)
>
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
> return insn->offset + insn->len + insn->immediate;
> }
>
> +#define PUSH_OP(op) \
> +({ \
> + list_add_tail(&op->list, ops_list); \
> + NULL; \
> +})
> +
> +#define ADD_OP(op) \
> + if (!(op = calloc(1, sizeof(*op)))) \
> + return -1; \
> + else for (; op; op = PUSH_OP(op))
> +

I would better have a function to alloc+add op instead of weird macros,
for example:

static struct stack_op *add_op(void)
{
struct stack *op;

op = calloc(1, sizeof(*op));
if (!op)
return NULL;
list_add_tail(&op->list, ops_list);
}

Then it requires two more lines when using it but I think the code is much
cleaner and clearer, e.g.:

op = add_op();
if (!op)
return -1;
op->src.type = OP_SRC_ADD;
op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
op->dest.type = OP_DEST_REG;
op->dest.reg = CFI_SP;

alex.

> int arch_decode_instruction(struct elf *elf, struct section *sec,
> unsigned long offset, unsigned int maxlen,
> unsigned int *len, enum insn_type *type,
> @@ -88,7 +99,7 @@ int arch_decode_instruction(struct elf *
> unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
> rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
> modrm_reg = 0, sib = 0;
> - struct stack_op *op;
> + struct stack_op *op = NULL;
>
> x86_64 = is_x86_64(elf);
> if (x86_64 == -1)
> @@ -129,10 +140,6 @@ int arch_decode_instruction(struct elf *
> if (insn.sib.nbytes)
> sib = insn.sib.bytes[0];
>
> - op = calloc(1, sizeof(*op));
> - if (!op)
> - return -1;
> -
> switch (op1) {
>
> case 0x1:
> @@ -141,10 +148,12 @@ int arch_decode_instruction(struct elf *
>
> /* add/sub reg, %rsp */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_ADD;
> - op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_ADD;
> + op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
> }
> break;
>
> @@ -152,9 +161,11 @@ int arch_decode_instruction(struct elf *
>
> /* push reg */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG;
> - op->src.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
> - op->dest.type = OP_DEST_PUSH;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG;
> + op->src.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
> + op->dest.type = OP_DEST_PUSH;
> + }
>
> break;
>
> @@ -162,9 +173,11 @@ int arch_decode_instruction(struct elf *
>
> /* pop reg */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_POP;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
> + ADD_OP(op) {
> + op->src.type = OP_SRC_POP;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = op_to_cfi_reg[op1 & 0x7][rex_b];
> + }
>
> break;
>
> @@ -172,8 +185,10 @@ int arch_decode_instruction(struct elf *
> case 0x6a:
> /* push immediate */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_CONST;
> - op->dest.type = OP_DEST_PUSH;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_CONST;
> + op->dest.type = OP_DEST_PUSH;
> + }
> break;
>
> case 0x70 ... 0x7f:
> @@ -188,11 +203,13 @@ int arch_decode_instruction(struct elf *
> if (modrm == 0xe4) {
> /* and imm, %rsp */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_AND;
> - op->src.reg = CFI_SP;
> - op->src.offset = insn.immediate.value;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_AND;
> + op->src.reg = CFI_SP;
> + op->src.offset = insn.immediate.value;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
> break;
> }
>
> @@ -205,11 +222,13 @@ int arch_decode_instruction(struct elf *
>
> /* add/sub imm, %rsp */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_ADD;
> - op->src.reg = CFI_SP;
> - op->src.offset = insn.immediate.value * sign;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_ADD;
> + op->src.reg = CFI_SP;
> + op->src.offset = insn.immediate.value * sign;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
> break;
>
> case 0x89:
> @@ -217,10 +236,12 @@ int arch_decode_instruction(struct elf *
>
> /* mov %rsp, reg */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG;
> - op->src.reg = CFI_SP;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG;
> + op->src.reg = CFI_SP;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> + }
> break;
> }
>
> @@ -228,10 +249,12 @@ int arch_decode_instruction(struct elf *
>
> /* mov reg, %rsp */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG;
> - op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG;
> + op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
> break;
> }
>
> @@ -242,21 +265,25 @@ int arch_decode_instruction(struct elf *
>
> /* mov reg, disp(%rbp) */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG;
> - op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> - op->dest.type = OP_DEST_REG_INDIRECT;
> - op->dest.reg = CFI_BP;
> - op->dest.offset = insn.displacement.value;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG;
> + op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + op->dest.type = OP_DEST_REG_INDIRECT;
> + op->dest.reg = CFI_BP;
> + op->dest.offset = insn.displacement.value;
> + }
>
> } else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
>
> /* mov reg, disp(%rsp) */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG;
> - op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> - op->dest.type = OP_DEST_REG_INDIRECT;
> - op->dest.reg = CFI_SP;
> - op->dest.offset = insn.displacement.value;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG;
> + op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + op->dest.type = OP_DEST_REG_INDIRECT;
> + op->dest.reg = CFI_SP;
> + op->dest.offset = insn.displacement.value;
> + }
> }
>
> break;
> @@ -266,22 +293,26 @@ int arch_decode_instruction(struct elf *
>
> /* mov disp(%rbp), reg */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG_INDIRECT;
> - op->src.reg = CFI_BP;
> - op->src.offset = insn.displacement.value;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG_INDIRECT;
> + op->src.reg = CFI_BP;
> + op->src.offset = insn.displacement.value;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + }
>
> } else if (rex_w && !rex_b && sib == 0x24 &&
> modrm_mod != 3 && modrm_rm == 4) {
>
> /* mov disp(%rsp), reg */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_REG_INDIRECT;
> - op->src.reg = CFI_SP;
> - op->src.offset = insn.displacement.value;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + ADD_OP(op) {
> + op->src.type = OP_SRC_REG_INDIRECT;
> + op->src.reg = CFI_SP;
> + op->src.offset = insn.displacement.value;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
> + }
> }
>
> break;
> @@ -290,27 +321,31 @@ int arch_decode_instruction(struct elf *
> if (sib == 0x24 && rex_w && !rex_b && !rex_x) {
>
> *type = INSN_STACK;
> - if (!insn.displacement.value) {
> - /* lea (%rsp), reg */
> - op->src.type = OP_SRC_REG;
> - } else {
> - /* lea disp(%rsp), reg */
> - op->src.type = OP_SRC_ADD;
> - op->src.offset = insn.displacement.value;
> + ADD_OP(op) {
> + if (!insn.displacement.value) {
> + /* lea (%rsp), reg */
> + op->src.type = OP_SRC_REG;
> + } else {
> + /* lea disp(%rsp), reg */
> + op->src.type = OP_SRC_ADD;
> + op->src.offset = insn.displacement.value;
> + }
> + op->src.reg = CFI_SP;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
> }
> - op->src.reg = CFI_SP;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = op_to_cfi_reg[modrm_reg][rex_r];
>
> } else if (rex == 0x48 && modrm == 0x65) {
>
> /* lea disp(%rbp), %rsp */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_ADD;
> - op->src.reg = CFI_BP;
> - op->src.offset = insn.displacement.value;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_ADD;
> + op->src.reg = CFI_BP;
> + op->src.offset = insn.displacement.value;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
>
> } else if (rex == 0x49 && modrm == 0x62 &&
> insn.displacement.value == -8) {
> @@ -322,11 +357,13 @@ int arch_decode_instruction(struct elf *
> * stack realignment.
> */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_ADD;
> - op->src.reg = CFI_R10;
> - op->src.offset = -8;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_ADD;
> + op->src.reg = CFI_R10;
> + op->src.offset = -8;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
>
> } else if (rex == 0x49 && modrm == 0x65 &&
> insn.displacement.value == -16) {
> @@ -338,11 +375,13 @@ int arch_decode_instruction(struct elf *
> * stack realignment.
> */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_ADD;
> - op->src.reg = CFI_R13;
> - op->src.offset = -16;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_ADD;
> + op->src.reg = CFI_R13;
> + op->src.offset = -16;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
> }
>
> break;
> @@ -350,8 +389,10 @@ int arch_decode_instruction(struct elf *
> case 0x8f:
> /* pop to mem */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_POP;
> - op->dest.type = OP_DEST_MEM;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_POP;
> + op->dest.type = OP_DEST_MEM;
> + }
> break;
>
> case 0x90:
> @@ -361,15 +402,19 @@ int arch_decode_instruction(struct elf *
> case 0x9c:
> /* pushf */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_CONST;
> - op->dest.type = OP_DEST_PUSHF;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_CONST;
> + op->dest.type = OP_DEST_PUSHF;
> + }
> break;
>
> case 0x9d:
> /* popf */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_POPF;
> - op->dest.type = OP_DEST_MEM;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_POPF;
> + op->dest.type = OP_DEST_MEM;
> + }
> break;
>
> case 0x0f:
> @@ -405,15 +450,19 @@ int arch_decode_instruction(struct elf *
>
> /* push fs/gs */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_CONST;
> - op->dest.type = OP_DEST_PUSH;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_CONST;
> + op->dest.type = OP_DEST_PUSH;
> + }
>
> } else if (op2 == 0xa1 || op2 == 0xa9) {
>
> /* pop fs/gs */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_POP;
> - op->dest.type = OP_DEST_MEM;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_POP;
> + op->dest.type = OP_DEST_MEM;
> + }
> }
>
> break;
> @@ -427,7 +476,8 @@ int arch_decode_instruction(struct elf *
> * pop bp
> */
> *type = INSN_STACK;
> - op->dest.type = OP_DEST_LEAVE;
> + ADD_OP(op)
> + op->dest.type = OP_DEST_LEAVE;
>
> break;
>
> @@ -449,12 +499,14 @@ int arch_decode_instruction(struct elf *
> case 0xcf: /* iret */
> *type = INSN_EXCEPTION_RETURN;
>
> - /* add $40, %rsp */
> - op->src.type = OP_SRC_ADD;
> - op->src.reg = CFI_SP;
> - op->src.offset = 5*8;
> - op->dest.type = OP_DEST_REG;
> - op->dest.reg = CFI_SP;
> + ADD_OP(op) {
> + /* add $40, %rsp */
> + op->src.type = OP_SRC_ADD;
> + op->src.reg = CFI_SP;
> + op->src.offset = 5*8;
> + op->dest.type = OP_DEST_REG;
> + op->dest.reg = CFI_SP;
> + }
> break;
>
> case 0xca: /* retf */
> @@ -492,8 +544,10 @@ int arch_decode_instruction(struct elf *
>
> /* push from mem */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_CONST;
> - op->dest.type = OP_DEST_PUSH;
> + ADD_OP(op) {
> + op->src.type = OP_SRC_CONST;
> + op->dest.type = OP_DEST_PUSH;
> + }
> }
>
> break;
> @@ -504,11 +558,6 @@ int arch_decode_instruction(struct elf *
>
> *immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>
> - if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
> - list_add_tail(&op->list, ops_list);
> - else
> - free(op);
> -
> return 0;
> }
>
>
>

2020-04-24 07:03:53

by Alexandre Chartre

Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode

On 4/23/20 5:54 PM, Peter Zijlstra wrote:
> On Thu, Apr 23, 2020 at 05:40:38PM +0200, Alexandre Chartre wrote:
>
>>> @@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
>>> return insn->offset + insn->len + insn->immediate;
>>> }
>>> +#define PUSH_OP(op) \
>>> +({ \
>>> + list_add_tail(&op->list, ops_list); \
>>> + NULL; \
>>> +})
>>> +
>>> +#define ADD_OP(op) \
>>> + if (!(op = calloc(1, sizeof(*op)))) \
>>> + return -1; \
>>> + else for (; op; op = PUSH_OP(op))
>>> +
>>
>> I would better have a function to alloc+add op instead of weird macros,
>> for example:
>>
>> static struct stack_op *add_op(void)
>> {
>> struct stack *op;
>>
>> op = calloc(1, sizeof(*op));
>> if (!op)
>> return NULL;
>> list_add_tail(&op->list, ops_list);
>> }
>>
>> Then it requires two more lines when using it but I think the code is much
>> cleaner and clearer, e.g.:
>>
>> op = add_op();
>> if (!op)
>> return -1;
>> op->src.type = OP_SRC_ADD;
>> op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
>> op->dest.type = OP_DEST_REG;
>> op->dest.reg = CFI_SP;
>
> The 'problem' which this is that it doesn't NULL op again, so any later
> use will do 'funny' things instead of crashing sensibly.

Then you can use a local variable:

{
struct stack_op *op = add_op();
if (!op)
return -1;
op->src.type = OP_SRC_ADD;
op->src.reg = op_to_cfi_reg[modrm_reg][rex_r];
op->dest.type = OP_DEST_REG;
op->dest.reg = CFI_SP;
}

> Also, I'm mightly lazy, I don't like endlessly repeating the same things.

Me too, I often try to use macros to avoid repeating the same thing, and usually
spend a lot of time trying fancy macros and eventually realize that this is
usually not worth it.

So here, we are down to a two line differences:

ADD_OP(op) {
...
}

vs.

{
struct stack *op = add_op();
if (!op)
return -1;
...
}

Anyway, I leave it up to you, that's just coding preferences.

In any case:

Reviewed-by: Alexandre Chartre <[email protected]>


Thanks,

alex.

2020-04-24 09:45:50

by Miroslav Benes

Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode

On Thu, 23 Apr 2020, Peter Zijlstra wrote:

> On Thu, Apr 23, 2020 at 05:40:38PM +0200, Alexandre Chartre wrote:
> >
> > On 4/23/20 2:47 PM, Peter Zijlstra wrote:
> > > Wrap each stack_op in a macro that allocates and adds it to the list.
> > > This simplifies trying to figure out what to do with the pre-allocated
> > > stack_op at the end.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > ---
> > > tools/objtool/arch/x86/decode.c | 257 +++++++++++++++++++++++-----------------
> > > 1 file changed, 153 insertions(+), 104 deletions(-)
> > >
> > > --- a/tools/objtool/arch/x86/decode.c
> > > +++ b/tools/objtool/arch/x86/decode.c
> > > @@ -77,6 +77,17 @@ unsigned long arch_jump_destination(stru
> > > return insn->offset + insn->len + insn->immediate;
> > > }
> > > +#define PUSH_OP(op) \
> > > +({ \
> > > + list_add_tail(&op->list, ops_list); \
> > > + NULL; \
> > > +})
> > > +
> > > +#define ADD_OP(op) \
> > > + if (!(op = calloc(1, sizeof(*op)))) \
> > > + return -1; \
> > > + else for (; op; op = PUSH_OP(op))
> > > +
> >
> > I would better have a function to alloc+add op instead of weird macros,
> > for example:
>
> I know it'll not make you feel (much) better, but we can write it
> shorter:
>
> +#define ADD_OP(op) \
> + if (!(op = calloc(1, sizeof(*op)))) \
> + return -1; \
> + else for (list_add_tail(&op->list, ops_list); op; op = NULL)

FWIW, I like this version the best.

Miroslav