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

[Petr Machata]

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

> To capture the last one, I've added a check for whether proc->pid occurs 
> in options.h:opt_p - not sure if there is a simpler way of performing
> this check.

Both a -p and a command can be given.  Probably not something that would
typically be used, but nonetheless this is not a correct way to check
for this.

arch_dylink_done is called either for a process whose execution ltrace
has under control (i.e. initial process or forks of already traced
processes), or always for a process that we attach to.  In theory it can
be called even though arch_dylink_ was not, in fact, _done, there's no
deep logic in ltrace to figure out whether the dynamic linker actually
did finish doing its thing.

Which is a shame, but still you could use the callback to make a mark
somewhere and later check it.  Or create breakpoins as delayed and only
activate them in the callback--that's what PPC does.

> 2. mips_unresolved_data is shallow copied for each cloned symbol. If a
> child detaches whilst a symbol remains in NEED_UNRESOLVE state, then
> we shouldn't free its unresolve_data because the parent/siblings may 
> still need it. I've added a ref_count field to the struct to fix this.
> I am not sure how(if at all) PPC tackles this problem.

Looking in, I don't think it does.  That's probably a bug, I'll need to
address that.  Good catch!

The refcounting is fine by me, but for ppc I'll just do deep copy.
Easier to get right, I feel.

> +	/* Scan LL<->SC range for branches going outside that range */
> +	uint32_t spc;
> +	for (spc = lladdr + 4; spc < scaddr; spc += 4) {
> +		uint32_t scanpcs[2];
> +		int snr = mips_next_pcs(proc, spc, scanpcs);
> +
> +		if (!inrange(scanpcs[0], lladdr, scaddr)) {
> +			if ((newpcs = realloc(newpcs,
> +					      (nr + 1) * sizeof(spc))) != NULL)
> +				newpcs[nr++] = scanpcs[0];
> +		}
> +
> +		if ((snr == 2) && !inrange(scanpcs[1], lladdr, scaddr)) {
> +			if ((newpcs = realloc(newpcs,
> +					      (nr + 1) * sizeof(spc))) != NULL)
> +				newpcs[nr++] = scanpcs[1];
> +		}
> +	}

Sorry for picking yet more nits, but how about replacing the repetition
in the loop with this?

		int i;	// actually, ideally unsigned, but you keep it in int
		for (i = 0; i < snr; ++i)
			if (!inrange(scanpcs[i], lladdr, scaddr))
				if ((newpcs = realloc(...)) != NULL)
					newpcs[nr++] = scanpcs[i];

Also note that the realloc check is wrong.  If the reallocation fails,
you leak, and newpcs becomes NULL.  In the next branch, the NULL gets
passed to realloc, which I believe is valid and behaves as if you called
malloc.  Thus an error gets muffled and you lose breakpoints.  So really
this should be rather something like:

		for (i = 0; i < snr; ++i)
			if (!inrange(scanpcs[i], lladdr, scaddr)) {
				uint32_t *tmp = realloc(newpcs,
							(nr + 1) * sizeof(spc));
				if (tmp == NULL)
					return -1;
					/* And have the caller handle -1.  */

				newpcs = tmp;
				newpcs[nr++] = scanpcs[i];
			}

And looking into it yet more, that sizeof(spc) seems wrong as well.  It
should be sizeof(*newpcs).  The fact that they are both uint32_t is of
course no coincidence, but the sizeof should have a clear indication of
the buffer under allocation.

Thanks,
Petr