Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction

Julien Thierry
2020-04-03T08:01:54Z

On 4/2/20 6:54 PM, Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 03:28:47PM +0000, Julien Thierry wrote:
>> @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>> if (insn.sib.nbytes)
>> sib = insn.sib.bytes[0];
>>
>> + op = calloc(1, sizeof(*op));
>> + if (!op)
>> + return -1;
>> +
>
> Why not malloc()?
>

It's just that previsously, stack_op was part of the instruction
structure and was initialized to all 0 in decode_instructions(). Now
that it's created here, I assumed it would be better to have the same
thing here and initialized the new stack_op to all 0.

Do you prefer to have an explicit malloc() + memset()?

>> +static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
>> +{
>> + struct stack_op *op;
>> +
>> + list_for_each_entry(op, &insn->stack_ops, list) {
>> + int res;
>> +
>> + res = update_insn_state(insn, state, op);
>> + if (res)
>> + return res;
>
> This should probably be like:
>
> if (update_insn_state(insn, state, op))
> return 1;
>
> That way the error codes are converted to non-fatal warnings like before
> (which I admit is confusing...)
>

Right, I'll change this.

>> @@ -2205,29 +2244,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>> return 0;
>>
>> case INSN_STACK:
>> - if (update_insn_state(insn, &state))
>> + if (handle_insn_ops(insn, &state))
>> return 1;
>
> How about "handle_stack_ops"?
>

Works for me!

Thanks,

--
Julien Thierry