[Ltrace-devel] [PATCH v2] ltrace: Add support for Imagination Technologies Meta
Markos Chandras
markos.chandras at gmail.com
Thu Mar 21 20:32:09 UTC 2013
On 21 March 2013 16:25, Petr Machata <pmachata at redhat.com> wrote:
> Markos Chandras <markos.chandras at gmail.com> writes:
>
>> +void *
>> +get_instruction_pointer(struct process *proc)
>> +void *
>> +get_stack_pointer(struct process *proc)
>> +void *
>> +get_return_addr(struct process *proc, void *stack_pointer)
>
> Please make those return arch_addr_t.
>
>> +void
>> +set_instruction_pointer(struct process *proc, void *addr)
>
> This should take arch_addr_t addr instead of void*.
>
>> +enum metag_unitnum {
>> + METAG_UNIT_CT, /* 0x0 */
>> + METAG_UNIT_D0,
>> + METAG_UNIT_D1,
>> + METAG_UNIT_A0,
>> + METAG_UNIT_A1, /* 0x4 */
>> + METAG_UNIT_PC,
>> + METAG_UNIT_RA,
>> + METAG_UNIT_TR,
>> + METAG_UNIT_TT, /* 0x8 */
>> + METAG_UNIT_FX,
>> + METAG_UNIT_MAX,
>> +};
>
> This has wrong indentation.
>
>> +static int
>> +get_regval_from_unit(enum metag_unitnum unit, unsigned int reg, struct user_gp_regs *regs)
>> +{
>> + /*
>> + * Check if reg has a sane value.
>> + * We do have N_UNITS, each one having X registers
>> + * and each register is REG_SIZE bytes
>> + */
>> + if ((unit == METAG_UNIT_A0) || (unit == METAG_UNIT_A1)) {
>> + if (reg >= ((sizeof(regs->ax)/N_UNITS/REG_SIZE)))
>> + goto bad_reg;
>> + } else if ((unit == METAG_UNIT_D0) || (unit == METAG_UNIT_D1)) {
>> + if (reg >= ((sizeof(regs->dx)/N_UNITS/REG_SIZE)))
>> + goto bad_reg;
>> + }
>> +
>> + switch(unit)
>> + {
>
> This should be "switch (unit) {".
>
>> + case METAG_UNIT_A1:
>> + return regs->ax[reg][1];
>> + case METAG_UNIT_D0:
>> + return regs->dx[reg][0];
>> + case METAG_UNIT_D1:
>> + return regs->dx[reg][1];
>> + case METAG_UNIT_A0:
>> + return regs->ax[reg][0];
>> + /* We really shouldn't be here */
>
> There should be a period and two spaces before the */.
>
>> + default:
>> + fprintf(stderr, "unhandled unit=%d\n", unit);
>
> The idiom commonly used in ltrace for handling this is assert(unit !=
> unit); followed by an outright abort(); (for -DNDEBUG builds). The
> logic behind that is that either it's a hard error, and we should fail
> early and loudly, or it's to be expected, and then instead of emitting
> an error message, we shoud handle the situation gracefully.
>
>> + }
>> + return 0;
>> +
>> +bad_reg:
>> + fprintf(stderr,"Reading from register %d of unit %d is not implemented\n", reg, unit);
>> + return 0;
>
> Line too long, missing space after first comma, missing period at the
> end of the sentence.
>
> Is this a "can't happen" situation? If yes, then see above. Otherwise
> keeping this at "not yet implemented" level is probably fine.
>
>> +static int
>> +metag_next_pcs(struct process *proc, uint32_t pc, uint32_t *newpc)
>> +{
>> + uint32_t inst;
>> + int nr = 0, imm, reg_val;
>> + unsigned int unit = 0, reg;
>> + struct user_gp_regs regs;
>> + struct iovec iov;
>> +
>> + inst = ptrace(PTRACE_PEEKTEXT, proc->pid, pc, 0);
>> +
>> + if (inst == 0xa0fffffe) { /* NOP (Special branch instruction) */
>> + newpc[nr++] = pc + 4;
>> + } else if ((inst & 0xff000000) == 0xa0000000) {
>> + /* Matching 0xA 0x0 for opcode for B #S19 or B<cc> #S19
>> + *
>> + * Potential Targets:
>> + * - pc + #S19 * METAG_INSN_SIZE if R=1 or <CC> true
>> + * - pc + 4 */
>> + imm = ((inst << 8) >> 13) * METAG_INSN_SIZE;
>> + newpc[nr++] = pc + imm;
>> + newpc[nr++] = pc + 4;
>> + } else if ((inst & 0xff000000) == 0xac000000) {
>> + /* Matching 0xA 0xC for opcode
>> + * JUMP BBx.r,#X16 or CALL BBx.r,#X16
>> + *
>> + * pc = reg + #x16 (aligned) */
>> + imm = (inst >> 3) & 0xffff;
>> + reg = (inst >> 19) & 0x1f;
>> + unit = metag_bu_map[inst & 0x3];
>> + iov.iov_base = ®s;
>> + iov.iov_len = sizeof(regs);
>> + if (ptrace(PTRACE_GETREGSET, proc->pid, NT_PRSTATUS, (long)&iov))
>> + goto ptrace_fail;
>
> This line is too long, keep it below 80 characters please. There are
> several such problems.
>
>> + reg = (inst >> 14) & 0x1f;
>> + unit = (inst >> 5) & 0xf;
>> + } else { /* PC is the destination register. Find the source register */
>
> Each English sentence in a string or a comment (unless it's a simple
> explanatory tag, e.g. list of candidate instructions) should end with a
> punctuation mark, and be followed by two spaces or line end. The above
> should be:
>
> + } else { /* PC is the destination register. Find the source
> + * register. */
>
> Or perhaps even:
>
> + } else {
> + /* PC is the destination register. Find the source
> + * register. */
>
> There are more cases like that in your patch.
>
>> + reg = (inst >> 19) & 0x1f;
>> + unit = (inst >> 10) & 0xf;
>> + }
>> +
>> + switch(unit)
>> + {
>
> This should be "switch (unit) {". There are several occurences of this.
>
>> + if (nr <= 0 || nr > 2)
>> + goto fail;
>> + if (nr == 2) {
>> + if (newpc[1] == 0)
>> + goto fail;
>> + }
>
> Maybe fold the latter two conditions into one?
>
>> +enum sw_singlestep_status
>> +arch_sw_singlestep(struct process *proc, struct breakpoint *bp,
>> + int (*add_cb)(arch_addr_t, struct sw_singlestep_data *),
>> + struct sw_singlestep_data *add_cb_data)
>> +{
>> + uint32_t pc = (uint32_t) get_instruction_pointer(proc);
>> + uint32_t newpcs[2];
>> + int nr;
>> +
>> + nr = metag_next_pcs(proc, pc, newpcs);
>> +
>> + while (nr-- > 0) {
>> + arch_addr_t baddr = (arch_addr_t) newpcs[nr];
>> + if (dict_find(proc->leader->breakpoints, &baddr) != NULL) {
>> + fprintf(stderr, "skip %p %p\n", baddr, add_cb_data);
>
> Hmm, we really need to do something with this mixing of artifical and
> user-requested breakpoints. This is bound to bite us eventually.
>
> OK, I don't think I've found anything substantially wrong with your
> patch. I can't of course comment on correctness of actual instruction
> decoding details, but it's all generally readable and seems sane. If
> you correct the above trivial points, then I don't see why this couldn't
> be put in ltrace.
>
> Out of curiosity, did you have a chance to run full test suite, or do
> you only have some sort of cross toolchain set up?
>
> Finally, would you please also update README with triplets of systems
> where you think ltrace should work with this patch?
>
> Thank you,
> PM
Hi,
Thanks. I will fix these issues an send a new patch again.
No I didn't run the testsuite. Just tested a lot of ELF files to make
sure it works as expected.
I will also update the README file as requested.
Thanks for the review.
--
Regards,
Markos Chandras
More information about the Ltrace-devel
mailing list