[Ltrace-devel] [PATCH] Tracing PLT-less MIPS binaries

[Petr Machata]

Thanks for the patch.  Most of it looks good, but I have some points
about the software singlestepping.

Faraz Shahbazker <faraz.shahbazker at imgtec.com> writes:

> diff --git a/sysdeps/linux-gnu/mips/trace.c b/sysdeps/linux-gnu/mips/trace.c
> index 88e13ac..1e462d0 100644
> --- a/sysdeps/linux-gnu/mips/trace.c
> +++ b/sysdeps/linux-gnu/mips/trace.c
> @@ -277,6 +277,39 @@ arch_sw_singlestep(struct process *proc, struct breakpoint *bp,
>  
>  	while (nr-- > 0) {
>  		arch_addr_t baddr = (arch_addr_t) newpcs[nr];
> +		arch_addr_t lladdr = baddr;
> +		uint32_t inst;
> +
> +		/* An atomic Read-Modify-Write, starting with LL and ending with
> +		 * SC needs to be treated as a single instruction and stepped
> +		 * over, otherwise ERET issued within the SYSCALL will cause the
> +		 * write to fail, even for a single thread of execution. LL and
> +		 * SC must exist within a 2048-byte contiguous region.
> +		 *
> +		 * Note: this check is sloppy, in that in only scans forward
> +		 * linearly, instead of exploring all possible paths using
> +		 * mips_next_pcs; this is deemed sufficient for now, as it
> +		 * captures how atomic RMWs are typically coded. */
> +		proc_read_32(proc, lladdr, &inst);
> +		/* Found LL(linked-load), scan ahead for SC(store-conditional) */
> +		if (itype_op(inst) == 0x30) {
> +			while (lladdr+=4)  {

This is actually

			for (; laddr - baddr <= 2048; lladdr += 4) {
				...

And you can drop the final "if" down...

> +				proc_read_32(proc, lladdr, &inst);
> +				/* Found SC, now stepover trailing branch */
> +				if (itype_op(inst) == 0x38) {
> +					uint32_t postrmw[2];
> +					if (mips_next_pcs(proc, (uint32_t)lladdr+4,
> +							  postrmw) > 0)
> +						baddr = (arch_addr_t)postrmw[0];
> +					break;
> +				}
> +
> +				/* No SC within 2048b, assume LL is standalone */
> +				if (lladdr - baddr > 2048)
> +					break;

... here.

You also have a couple overlong lines there, please keep the line length
at 80 characters at most.

And one more nit: 2048b reads 2048 bits.  The symbol of a byte is B.

> +			}

Correct me if I'm wrong, but this code si there so that you avoid
setting software-singlestepping breakpoints on a LL.  Instead, you go
ahead and put it after the whole LL-SC block.  I don't think that's
necessary.  Only when stepping over LL do you need to do this, so that
you avoid interrupts between LL and SC.

So it seems to me this logic should be folded into mips_next_pcs, and
only activated when the stepped-over instruction is an LL.

You should also check for branches along the way.  If you don't put
breakpoints to branches that escape the LL-SC block, ltrace will lose
control of the process, and may consider the next legitimate breakpoint
a singlestep end, or do something silly like that.  I don't even know
what would happen, but it's likely it would desync the whole tracing
process.

I think ltrace currently support two breakpoints for software
singlestep.  So you put one just after the SC, and one extra is
available for when there's a jump between LL and SC.  If there are more
jumps, we just don't have a good answer for that.  PowerPC just gives up
and returns SWS_FAIL, which should at least clean up things and get
ltrace into the right states.  (But we don't have tests for this, so
it's likely broken one way or another.)

So, I would like your code to handle at least jumps as well.  Hm, which
makes me think, what if I have a code like this (pardon pseudo-assembly,
I don't actually speak MIPS ;) )

	JMP xyz
	LL
	... etc ...
xyz:
	SC

Would the delay slot kick in and it would all work as expected?  If yes,
then the jump that you are looking for may be just before the
instruction that you are supposed to single-step over.

Thanks,
Petr