On Thu, 2014-03-06 at 20:55 +0100, Jan Kratochvil wrote:
> On Tue, 04 Mar 2014 11:40:21 +0100, Mark Wielaard wrote:
> > +/* Structure used for keeping track of ptrace attaching a thread.
> > + Shared by linux-pid-attach and linux-proc-maps. If it has been setup
> > + then get the instance through __libdwfl_get_pid_arg. */
> > +struct pid_arg
>
> IMO the namespace should be 'struct __libdwfl_pid_arg'.
OK, changed everywhere.
> [...]
> > --- a/libdwfl/linux-proc-maps.c
> > +++ b/libdwfl/linux-proc-maps.c
> > @@ -339,34 +339,60 @@ dwfl_linux_proc_find_elf (Dwfl_Module *mod
> > __attribute__ ((unused)),
> [...]
> > + bool detach = false;
> > + bool tid_was_stopped = false;
> > + struct pid_arg *pid_arg = __libdwfl_get_pid_arg (mod->dwfl);
> > + if (pid_arg != NULL && ! pid_arg->assume_ptrace_stopped)
> > + {
> > + pid_t tid = pid_arg->tid_attached;
> > + if (tid != 0)
> > + {
> > + /* If the pid already is attached we are fine, otherwise
> > + just read through the thread that is attached. */
> > + if (tid != pid)
> > + pid = tid;
>
> The 'if' conditional has no meaning but it is probably meant for possible
> better code readability.
No it was just a silly if. I removed it and clarified the comment.
> In general I believe reading /proc/PID/maps is safer than the guessing by
> elf_from_remote_memory(), for example because the image may come from mmap()
> and not from dlopen(). But I guess it may be the common disagreement.
I agree that using all addresses from /proc/PID/maps, and not just the
base address, would be a good idea. But IMHO elf_from_remote_memory
should still be used for sanity checking the segment mapping addresses
and offsets to make sure the ELF file was mapped in as expected.
> I do not mind much using elf_from_remote_memory() instead.
OK, pushed.
Thanks,
Mark
