On 21 March 2013 16:25, Petr Machata <[email protected]> wrote: > Markos Chandras <[email protected]> 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 _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
