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;
Subject: Re: [PATCH 3/8] objtool: Rework allocating stack_ops on decode
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     jpoimboe@redhat.com, linux-kernel@vger.kernel.org,
        jthierry@redhat.com, tglx@linutronix.de, x86@kernel.org,
        mbenes@suse.cz
References: <20200423125013.452964352@infradead.org>
 <20200423125042.713052240@infradead.org>
 <7df9ec97-dc14-c4b6-fb26-f163e9afb1cd@oracle.com>
 <20200423155425.GW20730@hirez.programming.kicks-ass.net>
From:   Alexandre Chartre <alexandre.chartre@oracle.com>
Message-ID: <cc303a83-58eb-2b61-ddf1-672f2c8644e1@oracle.com>
Date:   Fri, 24 Apr 2020 09:06:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200423155425.GW20730@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@oracle.com>


Thanks,

alex.