Re: [RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

Re: [RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   The patch below excludes this code if __MINGW32__ is defined,
Pierre> but maybe it should be excluded if O_CLOEXEC is zero?

That would be fine by me.

Tom

Re: [RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

[Eli Zaretskii]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Date: Thu, 8 Aug 2013 10:51:37 +0200
> 
> Lately I got several warnings with mingw built 
> GDB's:
> 
> warning: Invalid parameter passed to C runtime function.
> 
> Use the new "set stop-on-debug-string-event on"
> command submitted  previously in
> http://sourceware.org/ml/gdb-patches/2013-08/msg00236.html
> 
> I was able to trace this down to 
> the fopen call with mode that to "re".
> As stated in the source, the "e" mode is a glibc extension
> about close on exec which generates this warning.
> 
>   The patch below excludes this code if __MINGW32__ is defined,
> but maybe it should be excluded if O_CLOEXEC is zero?

Actually, if we want this to be portable, we should have a function to
make a file handle be non-inheritable, instead of relying on glibc
extensions.  There _is_ a way to make a file handle non-inheritable on
Windows.

> diff -u -p -r1.7 filestuff.c
> --- src/gdb/common/filestuff.c	26 Jun 2013 08:01:55 -0000	1.7
> +++ src/gdb/common/filestuff.c	7 Aug 2013 12:35:55 -0000
> @@ -311,6 +311,7 @@ FILE *
>  gdb_fopen_cloexec (const char *filename, const char *opentype)
>  {
>    FILE *result = NULL;
> +#ifndef __MINGW32__
>    static int fopen_e_ever_failed;
>  
>    if (!fopen_e_ever_failed)
> @@ -320,17 +321,21 @@ gdb_fopen_cloexec (const char *filename,
>        copy = alloca (strlen (opentype) + 2);
>        strcpy (copy, opentype);
>        /* This is a glibc extension but we try it unconditionally on
> -	 this path.  */
> +	 this path, except when using Windows OS msvcrt dll,
> +	 in order to avoid a output debug string event.  */
>        strcat (copy, "e");
>        result = fopen (filename, copy);
>      }
> +#endif
>  
>    if (result == NULL)
>      {
>        /* Fallback.  */
>        result = fopen (filename, opentype);
> +#ifndef __MINGW32__
>        if (result != NULL)
>  	fopen_e_ever_failed = 1;
> +#endif
>      }
>  
>    if (result != NULL)

Wouldn't it be better to instead initialize fopen_e_ever_failed to 1
on MinGW?  Then the rest of the code will "just work", no?

RE: [RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé : vendredi 9 août 2013 16:10
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFC] Avoid invalid parameter warnings in C runtime function
for
> mingw builtr GDB
> 
> > From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> > Date: Thu, 8 Aug 2013 10:51:37 +0200
> >
> > Lately I got several warnings with mingw built
> > GDB's:
> >
> > warning: Invalid parameter passed to C runtime function.
> >
> > Use the new "set stop-on-debug-string-event on"
> > command submitted  previously in
> > http://sourceware.org/ml/gdb-patches/2013-08/msg00236.html
> >
> > I was able to trace this down to
> > the fopen call with mode that to "re".
> > As stated in the source, the "e" mode is a glibc extension
> > about close on exec which generates this warning.
> >
> >   The patch below excludes this code if __MINGW32__ is defined,
> > but maybe it should be excluded if O_CLOEXEC is zero?
> 
> Actually, if we want this to be portable, we should have a function to
> make a file handle be non-inheritable, instead of relying on glibc
> extensions.  There _is_ a way to make a file handle non-inheritable on
> Windows.
> 
> > diff -u -p -r1.7 filestuff.c
> > --- src/gdb/common/filestuff.c	26 Jun 2013 08:01:55 -0000	1.7
> > +++ src/gdb/common/filestuff.c	7 Aug 2013 12:35:55 -0000
> > @@ -311,6 +311,7 @@ FILE *
> >  gdb_fopen_cloexec (const char *filename, const char *opentype)
> >  {
> >    FILE *result = NULL;
> > +#ifndef __MINGW32__
> >    static int fopen_e_ever_failed;
> >
> >    if (!fopen_e_ever_failed)
> > @@ -320,17 +321,21 @@ gdb_fopen_cloexec (const char *filename,
> >        copy = alloca (strlen (opentype) + 2);
> >        strcpy (copy, opentype);
> >        /* This is a glibc extension but we try it unconditionally on
> > -	 this path.  */
> > +	 this path, except when using Windows OS msvcrt dll,
> > +	 in order to avoid a output debug string event.  */
> >        strcat (copy, "e");
> >        result = fopen (filename, copy);
> >      }
> > +#endif
> >
> >    if (result == NULL)
> >      {
> >        /* Fallback.  */
> >        result = fopen (filename, opentype);
> > +#ifndef __MINGW32__
> >        if (result != NULL)
> >  	fopen_e_ever_failed = 1;
> > +#endif
> >      }
> >
> >    if (result != NULL)
> 
> Wouldn't it be better to instead initialize fopen_e_ever_failed to 1
> on MinGW?  Then the rest of the code will "just work", no?

  Once again, my insufficient C knowledge is to blame...
Would that be correct?

#if O_CLOEXEC
  static int fopen_e_ever_failed = 0;
#else
  static int fopen_e_ever_failed = 1;
#endif

  But then the variable name is not appropriate anymore...
In fact, when I tried to read the code around that function, I
discovered that there is already another variable called
trust_o_cloexec

  It seems to me that both are testing the same glibc extension,
trust_o_cloexec as it numeric form O_CLOEXEC
and fopen_e_ever_failed as its string mode equivalent "e".
  Is it really possible that trust_o_cloexec and fopen_e_ever_failed
do not match?

  Should a unique variable be enough?

 Concerning my original RFC, should I resubmit it with the above suggestion?

Pierre Muller
  

 

Re: [RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   Once again, my insufficient C knowledge is to blame...
Pierre> Would that be correct?

Pierre> #if O_CLOEXEC
Pierre>   static int fopen_e_ever_failed = 0;
Pierre> #else
Pierre>   static int fopen_e_ever_failed = 1;
Pierre> #endif

O_CLOEXEC is unconditionally defined earlier.
But you can just write:

  /* If we provide a fake O_CLOEXEC, then don't ever try "e".  */
  static int fopen_e_ever_failed = O_CLOEXEC == 0;

Pierre>   But then the variable name is not appropriate anymore...

You can rename it if you want, but I don't really care either way.

Pierre> In fact, when I tried to read the code around that function, I
Pierre> discovered that there is already another variable called
Pierre> trust_o_cloexec

Pierre>   It seems to me that both are testing the same glibc extension,
Pierre> trust_o_cloexec as it numeric form O_CLOEXEC
Pierre> and fopen_e_ever_failed as its string mode equivalent "e".
Pierre>   Is it really possible that trust_o_cloexec and fopen_e_ever_failed
Pierre> do not match?

I forget exactly why I did this.
Maybe I was just being overly cautious.

Pierre>   Should a unique variable be enough?

Maybe.

Tom

Re: [RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

[Eli Zaretskii]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Cc: <gdb-patches@sourceware.org>
> Date: Tue, 13 Aug 2013 11:13:49 +0200
> 
> Would that be correct?
> 
> #if O_CLOEXEC
>   static int fopen_e_ever_failed = 0;
> #else
>   static int fopen_e_ever_failed = 1;
> #endif

Yes, but Tom's suggestion is more elegant.

[RFC] Avoid invalid parameter warnings in C runtime function for mingw builtr GDB

[Pierre Muller]

Lately I got several warnings with mingw built 
GDB's:

warning: Invalid parameter passed to C runtime function.

Use the new "set stop-on-debug-string-event on"
command submitted  previously in
http://sourceware.org/ml/gdb-patches/2013-08/msg00236.html

I was able to trace this down to 
the fopen call with mode that to "re".
As stated in the source, the "e" mode is a glibc extension
about close on exec which generates this warning.

  The patch below excludes this code if __MINGW32__ is defined,
but maybe it should be excluded if O_CLOEXEC is zero?

Comments welcome.


Pierre Muller
GDB pascal language maintainer

2013-08-07  Pierre Muller  <muller@sourceware.org>

	* src/gdb/common/filestuff.c (gdb_fopen_cloexec): Do not
	try to use "e" mode if linking to Windows OS msvcrt DLL,
	to avoid getting a output debug string event.


Index: src/gdb/common/filestuff.c
===================================================================
RCS file: /cvs/src/src/gdb/common/filestuff.c,v
retrieving revision 1.7
diff -u -p -r1.7 filestuff.c
--- src/gdb/common/filestuff.c	26 Jun 2013 08:01:55 -0000	1.7
+++ src/gdb/common/filestuff.c	7 Aug 2013 12:35:55 -0000
@@ -311,6 +311,7 @@ FILE *
 gdb_fopen_cloexec (const char *filename, const char *opentype)
 {
   FILE *result = NULL;
+#ifndef __MINGW32__
   static int fopen_e_ever_failed;
 
   if (!fopen_e_ever_failed)
@@ -320,17 +321,21 @@ gdb_fopen_cloexec (const char *filename,
       copy = alloca (strlen (opentype) + 2);
       strcpy (copy, opentype);
       /* This is a glibc extension but we try it unconditionally on
-	 this path.  */
+	 this path, except when using Windows OS msvcrt dll,
+	 in order to avoid a output debug string event.  */
       strcat (copy, "e");
       result = fopen (filename, copy);
     }
+#endif
 
   if (result == NULL)
     {
       /* Fallback.  */
       result = fopen (filename, opentype);
+#ifndef __MINGW32__
       if (result != NULL)
 	fopen_e_ever_failed = 1;
+#endif
     }
 
   if (result != NULL)