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

[Petr Machata]

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

> +#define inrange(x,lo,hi) ((x)<=(hi) && (x)>=(lo))
> +static int
> +mips_atomic_next_pcs(struct process *proc, uint32_t pc, uint32_t *newpcs)
> +{
> +	uint32_t spc, lladdr = pc, scaddr = pc + 4;
> +	int nr = 0;
> +
> +	for (lladdr = pc; scaddr - lladdr <= 2048; scaddr += 4) {

The lladdr = pc is not necessary.  Come to think of it, you don't
actually need all those variables.  How about this?

static int
mips_atomic_next_pcs(struct process *proc, uint32_t const lladdr,
		     uint32_t *newpcs)
{
	int nr = 0;

	uint32_t scaddr;
	for (scaddr = pc + 4; scaddr - lladdr <= 2048; scaddr += 4) {
		/* ... */
	}

	/* ... */

	uint32_t spc;
	for (spc = lladdr + 4; spc < scaddr; spc += 4) {
		/* ... */
		/* s/sizeof(pc)/sizeof(lladdr)/g */
	}

This way you clearly declare the intent that the variables are loop
iterators.  Formally this is a C99 feature, but GCC has allowed that as
an extension since forever.  Unlike placing the variable declaration in
the loop itself, which would of course be the best way to structure
this:

	for (uint32_t scaddr = pc + 4; scaddr - lladdr <= 2048; scaddr += 4) {

Alternatively, since none of the iterators is shared, you could have a
single uint32_t addr on top, and reuse that in each of the blocks.

In any case, that's two fewer variables, which is two fewer potentially
moving parts to keep an eye on.

> +	newpcs = malloc(2 * sizeof(uint32_t));

In mips_atomic_next_pcs, you use sizeof(pc), which I strongly prefer.
This should be:

	newpcs = malloc(2 * sizeof *newpcs);

Looks good otherwise.

Thanks,
Petr