Re: [PATCH v2 1/5] Import "glob" and "getcwd" modules from gnulib

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/