[Ltrace-devel] [PATCH v2] ltrace: Add support for Imagination Technologies Meta

Petr Machata pmachata at redhat.com
Thu Mar 21 16:25:09 UTC 2013


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



More information about the Ltrace-devel mailing list