Faraz Shahbazker <[email protected]> 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
_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
