Messages in this thread | | | Subject | Re: [PATCH v2 10/10] objtool: Support multiple stack_op per instruction | From | Julien Thierry <> | Date | Fri, 3 Apr 2020 09:01:46 +0100 |
| |
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
| |