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

[Alexandre Chartre]

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 <alexandre.chartre@xxxxxxxxxx>


Thanks,

alex.