Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 23 Apr 2020 17:54:25 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Alexandre Chartre <alexandre.chartre@oracle.com>
Cc:     jpoimboe@redhat.com, linux-kernel@vger.kernel.org,
        jthierry@redhat.com, tglx@linutronix.de, x86@kernel.org,
        mbenes@suse.cz
Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode
Message-ID: <20200423155425.GW20730@hirez.programming.kicks-ass.net>
References: <20200423125013.452964352@infradead.org>
 <20200423125042.713052240@infradead.org>
 <7df9ec97-dc14-c4b6-fb26-f163e9afb1cd@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7df9ec97-dc14-c4b6-fb26-f163e9afb1cd@oracle.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.