[v2,1/5] Import "glob" and "getcwd" modules from gnulib

  On 09/19/2017 05:37 AM, Sergio Durigan Junior wrote:
> [ Resending as the original e-mail got bounced because it is too long to
>   be posted here. ]
> 
> The full patch can be seen here:
> 
>   https://git.sergiodj.net/binutils-gdb.git/commit/?h=sergiodj/set-cwd-command&id=acf35a31bac3951f81d0446564b7f910a0fee21c
> 
> These two modules are necessary because of the rework that will be
> done in the "change directory" logic on GDB/gdbserver in the next
> commits.
> 
> First, we will get rid of the "gdb_dirbuf" global variable and instead
> rely on the fact that "getcwd (NULL, 0)" returns a heap-allocated
> string with the necessary bytes to hold the full path.  

Should mention that that's a GNU extension here.

> As a side note, we no longer need to define "close" on gdb/ser-tcp.c,
> so the patch removes that.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gnulib/aclocal.m4: Regenerate.
> 	* gnulib/config.in: Likewise.
...

> 	* gnulib/import/unistd-safer.h: Likewise.

Please say "New file." for new files above, not Likewise->Regenerate.

> 	* ser-tcp.c: Do not (re)define "close".
> 

This is:

is defined, but it wasn't obvious to me whether it'll end
up defined with the current set of modules.

Boy, these modules sure bring in a lot of stuff.
I noticed that this is bringing in the strerror module, which was
problematic in the past on mingw [1].  Could you please try
cross building gdb for mingw (should be easy since Fedora has
cross compilers handy), to confirm that it's alright now?
It it works, we could get rid of safe_strerror in a follow up
patch.

[1] https://sourceware.org/ml/gdb-patches/2013-11/msg00597.html
I don't recall whether that's been fixed meanwhile.

Thanks,
Pedro Alves
  

On Wednesday, September 20 2017, Pedro Alves wrote:

> On 09/19/2017 05:37 AM, Sergio Durigan Junior wrote:
>> [ Resending as the original e-mail got bounced because it is too long to
>>   be posted here. ]
>> 
>> The full patch can be seen here:
>> 
>>   https://git.sergiodj.net/binutils-gdb.git/commit/?h=sergiodj/set-cwd-command&id=acf35a31bac3951f81d0446564b7f910a0fee21c
>> 
>> These two modules are necessary because of the rework that will be
>> done in the "change directory" logic on GDB/gdbserver in the next
>> commits.
>> 
>> First, we will get rid of the "gdb_dirbuf" global variable and instead
>> rely on the fact that "getcwd (NULL, 0)" returns a heap-allocated
>> string with the necessary bytes to hold the full path.  
>
> Should mention that that's a GNU extension here.

Done.

>> As a side note, we no longer need to define "close" on gdb/ser-tcp.c,
>> so the patch removes that.
>> 
>> gdb/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* gnulib/aclocal.m4: Regenerate.
>> 	* gnulib/config.in: Likewise.
> ...
>
>> 	* gnulib/import/unistd-safer.h: Likewise.
>
> Please say "New file." for new files above, not Likewise->Regenerate.

There's actually a "New file." in the middle, and there's also another
"Regenerate." (and yet another "New file.") also.

But I agree that specifying what happened explicitly is better for such
long ChangeLogs.  Done.

>> 	* ser-tcp.c: Do not (re)define "close".
>> 
>
> This is:
>
> --- a/gdb/ser-tcp.c
> +++ b/gdb/ser-tcp.c
> @@ -42,7 +42,6 @@
>  #ifndef ETIMEDOUT
>  #define ETIMEDOUT WSAETIMEDOUT
>  #endif
> -#define close(fd) closesocket (fd)
>
> Are you sure that the gnulib code that makes close work
> for sockets is enabled?  I guess it will if WINDOWS_SOCKETS
> is defined, but it wasn't obvious to me whether it'll end
> up defined with the current set of modules.

One of the dependencies of "getcwd" is the "close" module, and whenever
I tried to compile this patch with mingw it would fail because of this
re-definition of close made by ser-tcp.c.  My first approach was to
#undef close before ser-tcp.c redefined it, but then I decided to just
use "close" from gnulib.  I didn't know about this WINDOWS_SOCKETS
requirement; maybe we can define it before the inclusion of <stdlib.h>?
Or maybe choose the safe side and let ser-tcp.c redefine close as
needed.

> Boy, these modules sure bring in a lot of stuff.

Yeah, "getcwd" did.

> I noticed that this is bringing in the strerror module, which was
> problematic in the past on mingw [1].  Could you please try
> cross building gdb for mingw (should be easy since Fedora has
> cross compilers handy), to confirm that it's alright now?
> It it works, we could get rid of safe_strerror in a follow up
> patch.
>
> [1] https://sourceware.org/ml/gdb-patches/2013-11/msg00597.html
> I don't recall whether that's been fixed meanwhile.

Before submitting the patch I had compiled it for mingw, and everything
worked OK.  I did it again just in case, and it's still working.  So I
believe the issue is fixed.
  

On 09/20/2017 06:17 PM, Sergio Durigan Junior wrote:
> On Wednesday, September 20 2017, Pedro Alves wrote:

>> --- a/gdb/ser-tcp.c
>> +++ b/gdb/ser-tcp.c
>> @@ -42,7 +42,6 @@
>>  #ifndef ETIMEDOUT
>>  #define ETIMEDOUT WSAETIMEDOUT
>>  #endif
>> -#define close(fd) closesocket (fd)
>>
>> Are you sure that the gnulib code that makes close work
>> for sockets is enabled?  I guess it will if WINDOWS_SOCKETS
>> is defined, but it wasn't obvious to me whether it'll end
>> up defined with the current set of modules.
> 
> One of the dependencies of "getcwd" is the "close" module, and whenever
> I tried to compile this patch with mingw it would fail because of this
> re-definition of close made by ser-tcp.c.  My first approach was to
> #undef close before ser-tcp.c redefined it, but then I decided to just
> use "close" from gnulib.  I didn't know about this WINDOWS_SOCKETS
> requirement; maybe we can define it before the inclusion of <stdlib.h>?
> Or maybe choose the safe side and let ser-tcp.c redefine close as
> needed.

I don't know much about WINDOWS_SOCKETS either.  I just looked
at gnulib/lib/close.c, and found:

~~~
/* Override close() to call into other gnulib modules.  */

int
rpl_close (int fd)
{
#if WINDOWS_SOCKETS
  int retval = execute_all_close_hooks (close_nothrow, fd);
#else
  int retval = close_nothrow (fd);
#endif

#if REPLACE_FCHDIR
  if (retval >= 0)
    _gl_unregister_fd (fd);
#endif

  return retval;
}
~~~


and then figured out that there's a close_fd_maybe_socket
close hook implemented in lib/sockets.c.

static int
close_fd_maybe_socket (const struct fd_hook *remaining_list,
                       gl_close_fn primary,
                       int fd)
{

This is all wrapped in #ifdef WINDOWS_SOCKETS, hence the question.

It should be easy for you to determine whether WINDOWS_SOCKETS
is defined in your mingw build, and thus whether all this code
is part of the build or not.

> 
>> Boy, these modules sure bring in a lot of stuff.
> 
> Yeah, "getcwd" did.
> 
>> I noticed that this is bringing in the strerror module, which was
>> problematic in the past on mingw [1].  Could you please try
>> cross building gdb for mingw (should be easy since Fedora has
>> cross compilers handy), to confirm that it's alright now?
>> It it works, we could get rid of safe_strerror in a follow up
>> patch.
>>
>> [1] https://sourceware.org/ml/gdb-patches/2013-11/msg00597.html
>> I don't recall whether that's been fixed meanwhile.
> 
> Before submitting the patch I had compiled it for mingw, and everything
> worked OK.  I did it again just in case, and it's still working.  So I
> believe the issue is fixed.

Alright, that's great.

Thanks,
Pedro Alves
  

On Wednesday, September 20 2017, Pedro Alves wrote:

> On 09/20/2017 06:17 PM, Sergio Durigan Junior wrote:
>> On Wednesday, September 20 2017, Pedro Alves wrote:
>
>>> --- a/gdb/ser-tcp.c
>>> +++ b/gdb/ser-tcp.c
>>> @@ -42,7 +42,6 @@
>>>  #ifndef ETIMEDOUT
>>>  #define ETIMEDOUT WSAETIMEDOUT
>>>  #endif
>>> -#define close(fd) closesocket (fd)
>>>
>>> Are you sure that the gnulib code that makes close work
>>> for sockets is enabled?  I guess it will if WINDOWS_SOCKETS
>>> is defined, but it wasn't obvious to me whether it'll end
>>> up defined with the current set of modules.
>> 
>> One of the dependencies of "getcwd" is the "close" module, and whenever
>> I tried to compile this patch with mingw it would fail because of this
>> re-definition of close made by ser-tcp.c.  My first approach was to
>> #undef close before ser-tcp.c redefined it, but then I decided to just
>> use "close" from gnulib.  I didn't know about this WINDOWS_SOCKETS
>> requirement; maybe we can define it before the inclusion of <stdlib.h>?
>> Or maybe choose the safe side and let ser-tcp.c redefine close as
>> needed.
>
> I don't know much about WINDOWS_SOCKETS either.  I just looked
> at gnulib/lib/close.c, and found:
>
> ~~~
> /* Override close() to call into other gnulib modules.  */
>
> int
> rpl_close (int fd)
> {
> #if WINDOWS_SOCKETS
>   int retval = execute_all_close_hooks (close_nothrow, fd);
> #else
>   int retval = close_nothrow (fd);
> #endif
>
> #if REPLACE_FCHDIR
>   if (retval >= 0)
>     _gl_unregister_fd (fd);
> #endif
>
>   return retval;
> }
> ~~~
>
>
> and then figured out that there's a close_fd_maybe_socket
> close hook implemented in lib/sockets.c.
>
> static int
> close_fd_maybe_socket (const struct fd_hook *remaining_list,
>                        gl_close_fn primary,
>                        int fd)
> {
>
> This is all wrapped in #ifdef WINDOWS_SOCKETS, hence the question.
>
> It should be easy for you to determine whether WINDOWS_SOCKETS
> is defined in your mingw build, and thus whether all this code
> is part of the build or not.

I will do that and report back.  Thanks,
  

On Wednesday, September 20 2017, I wrote:

> On Wednesday, September 20 2017, Pedro Alves wrote:
>
>> On 09/20/2017 06:17 PM, Sergio Durigan Junior wrote:
>>> On Wednesday, September 20 2017, Pedro Alves wrote:
>>
>>>> --- a/gdb/ser-tcp.c
>>>> +++ b/gdb/ser-tcp.c
>>>> @@ -42,7 +42,6 @@
>>>>  #ifndef ETIMEDOUT
>>>>  #define ETIMEDOUT WSAETIMEDOUT
>>>>  #endif
>>>> -#define close(fd) closesocket (fd)
>>>>
>>>> Are you sure that the gnulib code that makes close work
>>>> for sockets is enabled?  I guess it will if WINDOWS_SOCKETS
>>>> is defined, but it wasn't obvious to me whether it'll end
>>>> up defined with the current set of modules.
>>> 
>>> One of the dependencies of "getcwd" is the "close" module, and whenever
>>> I tried to compile this patch with mingw it would fail because of this
>>> re-definition of close made by ser-tcp.c.  My first approach was to
>>> #undef close before ser-tcp.c redefined it, but then I decided to just
>>> use "close" from gnulib.  I didn't know about this WINDOWS_SOCKETS
>>> requirement; maybe we can define it before the inclusion of <stdlib.h>?
>>> Or maybe choose the safe side and let ser-tcp.c redefine close as
>>> needed.
>>
>> I don't know much about WINDOWS_SOCKETS either.  I just looked
>> at gnulib/lib/close.c, and found:
>>
>> ~~~
>> /* Override close() to call into other gnulib modules.  */
>>
>> int
>> rpl_close (int fd)
>> {
>> #if WINDOWS_SOCKETS
>>   int retval = execute_all_close_hooks (close_nothrow, fd);
>> #else
>>   int retval = close_nothrow (fd);
>> #endif
>>
>> #if REPLACE_FCHDIR
>>   if (retval >= 0)
>>     _gl_unregister_fd (fd);
>> #endif
>>
>>   return retval;
>> }
>> ~~~
>>
>>
>> and then figured out that there's a close_fd_maybe_socket
>> close hook implemented in lib/sockets.c.
>>
>> static int
>> close_fd_maybe_socket (const struct fd_hook *remaining_list,
>>                        gl_close_fn primary,
>>                        int fd)
>> {
>>
>> This is all wrapped in #ifdef WINDOWS_SOCKETS, hence the question.
>>
>> It should be easy for you to determine whether WINDOWS_SOCKETS
>> is defined in your mingw build, and thus whether all this code
>> is part of the build or not.
>
> I will do that and report back.  Thanks,

WINDOWS_SOCKETS is not defined when building with the mingw compiler
from Fedora.  This means that removing that "#define" was actually not
correct, because "close" will not work as expected even with gnulib.

My proposal is to define "close" as it was being defined before, but
actually "#undef" it if it's already defined by other headers, like:

  #ifdef close
  #undef close
  #endif
  #define close(fd) closesocket (fd)

Does that work for you?
  

On 09/20/2017 09:30 PM, Sergio Durigan Junior wrote:
> On Wednesday, September 20 2017, I wrote:
> 
>> On Wednesday, September 20 2017, Pedro Alves wrote:
>>
>>> This is all wrapped in #ifdef WINDOWS_SOCKETS, hence the question.
>>>
>>> It should be easy for you to determine whether WINDOWS_SOCKETS
>>> is defined in your mingw build, and thus whether all this code
>>> is part of the build or not.
>>
>> I will do that and report back.  Thanks,
> 
> WINDOWS_SOCKETS is not defined when building with the mingw compiler
> from Fedora.  This means that removing that "#define" was actually not
> correct, because "close" will not work as expected even with gnulib.

Looks like it's defined by gnulib/m4/socketlib.m4, and seemingly
we're not pulling in that module.

> 
> My proposal is to define "close" as it was being defined before, but
> actually "#undef" it if it's already defined by other headers, like:
> 
>   #ifdef close
>   #undef close
>   #endif
>   #define close(fd) closesocket (fd)
> 
> Does that work for you?
> 

(There's no need of wrap #undef with #ifdef/#endif.  That's redundant.)

That'd #undef 'close' on all hosts, even if gnulib decides to
replace it for some reason.  E.g., REPLACE_FCHDIR
check in rpl_close (see my previous email).

How about we switch close/closesocket around:

#ifndef USE_WIN32API
# define closesocket close
#endif

And then use closesocket instead of close?

Thanks,
Pedro Alves
  

On Wednesday, September 20 2017, Pedro Alves wrote:

> On 09/20/2017 09:30 PM, Sergio Durigan Junior wrote:
>> On Wednesday, September 20 2017, I wrote:
>> 
>>> On Wednesday, September 20 2017, Pedro Alves wrote:
>>>
>>>> This is all wrapped in #ifdef WINDOWS_SOCKETS, hence the question.
>>>>
>>>> It should be easy for you to determine whether WINDOWS_SOCKETS
>>>> is defined in your mingw build, and thus whether all this code
>>>> is part of the build or not.
>>>
>>> I will do that and report back.  Thanks,
>> 
>> WINDOWS_SOCKETS is not defined when building with the mingw compiler
>> from Fedora.  This means that removing that "#define" was actually not
>> correct, because "close" will not work as expected even with gnulib.
>
> Looks like it's defined by gnulib/m4/socketlib.m4, and seemingly
> we're not pulling in that module.
>
>> 
>> My proposal is to define "close" as it was being defined before, but
>> actually "#undef" it if it's already defined by other headers, like:
>> 
>>   #ifdef close
>>   #undef close
>>   #endif
>>   #define close(fd) closesocket (fd)
>> 
>> Does that work for you?
>> 
>
> (There's no need of wrap #undef with #ifdef/#endif.  That's redundant.)
>
> That'd #undef 'close' on all hosts, even if gnulib decides to
> replace it for some reason.  E.g., REPLACE_FCHDIR
> check in rpl_close (see my previous email).

Not all hosts; only on hosts that define USE_WIN32API.  This would
basically make sure we stick to the current behaviour, which is to
always define "close" as "closesocket" on win32.

> How about we switch close/closesocket around:
>
> #ifndef USE_WIN32API
> # define closesocket close
> #endif
>
> And then use closesocket instead of close?

That'd work, but my preference is to use "close" everywhere because
that's the de facto way of dealing with sockets.  Only win32 hosts need
this "closesocket" thing, and I don't think it makes sense to base use
this name in our sources.

But that's my opinion; if you want, I can reverse the logic on the
define's.

Thanks,
  

On 09/21/2017 12:12 AM, Sergio Durigan Junior wrote:
> On Wednesday, September 20 2017, Pedro Alves wrote:
> 
>> (There's no need of wrap #undef with #ifdef/#endif.  That's redundant.)
>>
>> That'd #undef 'close' on all hosts, even if gnulib decides to
>> replace it for some reason.  E.g., REPLACE_FCHDIR
>> check in rpl_close (see my previous email).
> 
> Not all hosts; only on hosts that define USE_WIN32API.  This would
> basically make sure we stick to the current behaviour, which is to
> always define "close" as "closesocket" on win32.

Ah, this is all wrapped in #ifdef USE_WIN32API.  Missed that,
somehow.

> 
>> How about we switch close/closesocket around:
>>
>> #ifndef USE_WIN32API
>> # define closesocket close
>> #endif
>>
>> And then use closesocket instead of close?
> 
> That'd work, but my preference is to use "close" everywhere because
> that's the de facto way of dealing with sockets.  Only win32 hosts need
> this "closesocket" thing, and I don't think it makes sense to base use
> this name in our sources.
> 
> But that's my opinion; if you want, I can reverse the logic on the
> define's.

I don't see the problem.  Imagine it as if struct serial had
been C++-ified already, making struct ser_tcp a class, and we
added a private method like this:

void
ser_tcp::closesocket ()
{
#ifdef USE_WIN32API
   ::closesocket (fd);
#else
   close (fd);
#endif
}

Then we'd be calling closesocket instead of close.  :-)

Actually, that's pretty much the existing net_close, and
we call it in several places instead of close directly
in very nearby code:

...
	    close (scb->fd);
	    goto retry;
	  }
	if (err)
	  errno = err;
	net_close (scb);
...

Eh.

But I digress...

The #undef close approach is fine with me too
since it turns out that's wrapped in #if USE_WIN32API,
and I missed it the first time.

Thanks,
Pedro Alves