Re: [PATCH v6 3/4] Share fork_inferior et al with gdbserver

[Pedro Alves <palves@redhat.com>]

On 05/31/2017 04:43 AM, Sergio Durigan Junior wrote:
> 
>>> >> index 4ea7913..8aa85db 100644
>>> >> --- a/gdb/gdbserver/configure.ac
>>> >> +++ b/gdb/gdbserver/configure.ac
>>> >> @@ -462,7 +462,9 @@ esac],
>>> >>  
>>> >>  if $want_ipa ; then
>>> >>     if $have_ipa ; then
>>> >> -     IPA_DEPFILES="$ipa_obj"
>>> >> +     # Needed because safe_strerror's definition is host-dependent
>> >
>> > Why do we end up needing safe_strerror in the IPA in the first place?
> This is needed because I moved the definition of
> trace_start_error_with_name from the old gdb/fork-child.c to
> common/common-utils.c.  This function which uses safe_strerror, and
> common/common-utils.c is compiled by IPA.
> 
> An option would be to keep these trace_start_error.* functions in
> nat/fork-inferior.c, but I think it is more logical to keep them on
> common-utils.c.

I'd rather not add them to the IPA.  The least unnecessary code
is included in that library the better, because it is injected
into the target process.  So keeping them in fork-inferior.c
sounds better.

> 
> Now, I can obviously expand the comment if that's what you meant.
> 
>>> >> +     strerror_obj="`echo $srv_host_obs | sed 's/\(.*-strerror\)\.o/\1-ipa.o/'`"
>>> >> +     IPA_DEPFILES="$ipa_obj $strerror_obj"
>>> >>       extra_libraries="$extra_libraries libinproctrace.so"
>>> >>     else
>>> >>       AC_MSG_ERROR([inprocess agent not supported for this target])
>> >
>> >
>> >
>>> >> -#if defined(__UCLIBC__) && defined(HAS_NOMMU)
>>> >> -  pid = vfork ();
>> >
>> > Does fork_inferior end up always using vfork on no-MMU
>> > ports somehow?
> Sorry, I am not sure.  How would I go about finding that?

I'm not sure either.  configure.ac for anything fork related?
Look at git blame history around those lines, see what other
bits were touched at the same time?  gdbserver clearly cares about
being built for no-MMU ports, so the new code must too.  We can't
just delete that support without an alternative.

>>> >> +
>>> >> +/* This function is NOT reentrant.  Some of the variables have been
>>> >> +   made static to ensure that they survive the vfork call.  */
>>> >> +extern pid_t fork_inferior (const char *exec_file_arg,
>>> >> +			    const std::string &allargs,
>> >
>> > Should include <string>.
> Should it?  It's compiling fine without it.  I included just for the
> sake of clarity anyway.

Yes, headers should include what they depend on directly.

> Now, something that came up while I was testing things with mingw here.
> gdb/gdbserver/server.c now calls startup_inferior (define in
> nat/fork-inferior.c) directly, instead of doing things on its own.  This
> is one of the goals of this series, but for targets that don't compile
> fork-inferior.c (like Windows) this is an obvious problem.  So here's
> what I'm doing for the new version of the series: I'll move
> startup_inferior to common/ (probably common/common-fork-inferior.c or
> some such), so that all targets can have access to it (it's
> target-agnostic anyway).  If you have any comments about this, please
> let me know.

This sounds quite contorted, but I'll take a better look at it in v7.

Thanks,
Pedro Alves