Re: [PATCH v2 3/3] [PowerPC] Fix debug register issues in ppc-linux-nat

[Pedro Franco de Carvalho <pedromfc@linux.ibm.com>]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:

> The one thing I'm wondering about here is that in order to detect
> the interface, the code will rely on inferior_ptid.  This introduces
> a new use of the inferior_ptid deeply buried within a function that
> is called from a large number of places -- are we sure all those
> callers actually always have a (correct) inferior_ptid in place?
>
> Now, in practice, inferior_ptid is only needed for the *first*
> call, and that first call will likely happen only from a small
> subset of all the possible callers here.
>
> But it would seem cleaner to make this explicit by having an
> explicit "initialize" or "detect" call, which gets called in
> those places we expect to be "first", and which gets passed
> a ptid_t to use (where the callers will still pass inferior_ptid,
> but then at least the dependency will be explicit.

I'm taking a while to post an updated patch because I've been thinking
about this issue in some detail.

I agree that it would be better to make the uses of inferior_ptid
explicit with a "detect" method that takes the pid.

I don't know if all the target methods guarantee that inferior_ptid will
be properly set.  However, after going through them, I don't believe I
have introduced any additional effective uses of inferior_ptid, compared
to the old have_ptrace_hwdebug_interface function (which is also a
deeply buried use of inferior_ptid), with the exception of
low_stopped_data_address and low_stopped_by_watchpoint, but I've checked
that linux_nat_target explicitly sets inferior_ptid to the stopped lwp
before calling these, so I assume they are allowed to use it.  Another
exception is that the interface class can call ptrace twice when
detecting the interface, since it now also checks for the DEBUGREG
interface, while have_ptrace_hwdebug_interface only checked for the
HWDEBUG interface.

All the new and modified low_ methods only use the interface if it has
already been detected previously, otherwise, they don't have any work to
do.  So in practice they never use inferior_ptid.  But here too it would
be cleaner if the interface class had an explicit "is_detected" method
and these low_ methods don't use "detect".

Another solution I considered at length that seemed more natural is to
detect the interface in post_startup_inferior and post_attach.  ARM does
something similar to figure out their debug register capabilities.  The
post_startup_inferior method seems to clearly work.  I'm not sure about
post_attach.  I saw some weird cases where attaching to a threaded
program causes post_attach to be called with inferior_ptid set to one of
the extra threads, even though post_attach takes only the pid as an
argument and all implementations (e.g. in linux_nat_target) use the this
process pid for their ptrace calls.  This happens because in all-stop
mode all the threads are forced stopped and setup_inferior is called as
a consequence of that.  I suspect that using this pid would still work
since the main thread which is attached to is explicitly waited for in
linux_nat_target::attach.

I'm not sure if other weird effects can happen in non-stop mode that
would cause a watchpoint target method to be called before post_attach
is called, e.g. a thread stopping after an attach command is issued, but
before post_attach is executed, and the user trying to set a watchpoint
in this instant, when the interface won't have been detected yet.  I'm
having some trouble reasoning about all these possibilities in infrun.

Note that linux_nat_target sets ptrace flags in post_attach, but also
sets a bool in every additional lwp object to set these same flags for
every other thread the next time they stop.

Considering these issues, would you advise me to:

1. Just use inferior ptid in the target methods, like it was done
   before, except more explicitly, with a "detect method".

2. Assume that post_startup_inferior and post_startup will always have
   been called before any watchpoint-related target method has a chance
   to be called, and detect the interface only in these two places.  I
   don't know if this assumption holds true.
   
   - If so, should every target method that needs the interface assert
     that it is detected when they are called?  This would probably be
     the easiest thing to do, as I don't know if there are any issues in
     case something else is done (such as returning an error code) and
     the interface is not detected in one call but is in a subsequent
     call.  However, considering the issues above, these asserts could
     be triggered in some weird scenarios.

> [ *Maybe* (and I'm not sure here) it would even make sense to move the
> ppc_linux_dreg_interface into that per-process struct, to clearly
> associate it with the pid that was used to query the kernel? ]

This would naturally solve the problem of getting an ESRCH when querying
the kernel if the process is unlucky enough to get a sigkill right
before the ptrace call.  In this case, only the signalled process would
get no interface and be prevented from using hardware watchpoints.  With
the current version of the patch, if this happens, the whole GDB session
will be prevented from using hardware watchpoints, since the interface
is associated with the native target and there's only one global native
target.

Before the patch, this would only prevent the HWDEBUG interface from
being used, since GDB would still try the DEBUGREG interface in
can_use_hw_breakpoint, but this seems accidental and I think is more
confusing that testing if DEBUGREG is available in the interface class.

However, doing this would introduce additional dependencies on
inferior_ptid in all the target methods, since they would need the pid
to access the interface.  Most of the target methods don't actually use
the inferior_ptid, because by the time they are called, the interface
will already have been detected, but associating the interface object
with the per-process struct would necessarily require using the pid in
inferior_ptid.

Thanks!

--
Pedro Franco de Carvalho