[patch] Minor O_CLOEXEC optimization, "regression" fix

[patch] Minor O_CLOEXEC optimization, "regression" fix

[Jan Kratochvil]

Hi Tom,

I just noticed GDB does many needless double-opens with ENOENT like:

open("/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY) = -1 ENOENT (No such file or directory)

The fix is simple, it saved 30 syscalls on a small example (3076->3046) like:

 open("/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
-open("/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY) = -1 ENOENT (No such file or directory)
 open("/usr/lib/debug/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
-open("/usr/lib/debug/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY) = -1 ENOENT (No such file or directory)
 open("/usr/share/gdb/auto-load/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
-open("/usr/share/gdb/auto-load/usr/lib64/libc-2.18.90.so-gdb.gdb", O_RDONLY) = -1 ENOENT (No such file or directory)

No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu.


Jan


gdb/
2013-10-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Minor performance optimization of opening non-existing files.
	* common/filestuff.c (gdb_fopen_cloexec): New variable
	fopen_e_ever_succeeded, use it.

diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index d3b13e8..e10a013 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -319,6 +319,10 @@ gdb_fopen_cloexec (const char *filename, const char *opentype)
      supported.  */
   static int fopen_e_ever_failed = O_CLOEXEC == 0;
 
+  /* If we know the operating system supports "e" do not retry the open
+     without "e".  The file for example just does not exist.  */
+  static int fopen_e_ever_succeeded;
+
   if (!fopen_e_ever_failed)
     {
       char *copy;
@@ -331,7 +335,9 @@ gdb_fopen_cloexec (const char *filename, const char *opentype)
       result = fopen (filename, copy);
     }
 
-  if (result == NULL)
+  if (result != NULL)
+    fopen_e_ever_succeeded = 1;
+  else if (!fopen_e_ever_succeeded)
     {
       /* Fallback.  */
       result = fopen (filename, opentype);

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I just noticed GDB does many needless double-opens with ENOENT like:

Whoops.

Jan> -  if (result == NULL)
Jan> +  if (result != NULL)
Jan> +    fopen_e_ever_succeeded = 1;
Jan> +  else if (!fopen_e_ever_succeeded)

What if we have it check for EINVAL instead?

Tom

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Jan Kratochvil]

On Tue, 08 Oct 2013 21:44:33 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> -  if (result == NULL)
> Jan> +  if (result != NULL)
> Jan> +    fopen_e_ever_succeeded = 1;
> Jan> +  else if (!fopen_e_ever_succeeded)
> 
> What if we have it check for EINVAL instead?

May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.

original post:
	[patch] Minor O_CLOEXEC optimization, "regression" fix
	https://sourceware.org/ml/gdb-patches/2013-10/msg00233.html


Thanks,
Jan

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Kai Tietz]

----- Original Message -----
> On Tue, 08 Oct 2013 21:44:33 +0200, Tom Tromey wrote:
> > >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > Jan> -  if (result == NULL)
> > Jan> +  if (result != NULL)
> > Jan> +    fopen_e_ever_succeeded = 1;
> > Jan> +  else if (!fopen_e_ever_succeeded)
> > 
> > What if we have it check for EINVAL instead?
> 
> May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
> because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.

Yes, it fails due the 'e' command in mode.  Obviously this option isn't supported on Windows due there is no fork.  The allowed options for mode-flag is dependent to runtime-version.  The function (it is under the hood the _openfile routine, which handles this) returns EINVAL on any not supported mode-options.  So yes, probing for EINVAL seems to me like a valid way to probe for valid arguments here.
 
> original post:
> 	[patch] Minor O_CLOEXEC optimization, "regression" fix
> 	https://sourceware.org/ml/gdb-patches/2013-10/msg00233.html
> 
> 
> Thanks,
> Jan
> 

Regards,
Kai

[patchv2] Minor O_CLOEXEC optimization, "regression" fix

[Jan Kratochvil]

On Wed, 09 Oct 2013 15:34:17 +0200, Kai Tietz wrote:
> So yes, probing for EINVAL seems to me like a valid way to probe for valid
> arguments here.

OK, therefore updated the patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu.
It still reduces the number of syscalls.


Thanks,
Jan


gdb/
2013-10-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* common/filestuff.c (gdb_fopen_cloexec): Remove initialization of
	result variable.  Rename variable fopen_e_ever_failed to
	fopen_e_ever_failed_einval.  Retry fopen only for errno EINVAL.

diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index d3b13e8..4032f36 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -310,16 +310,16 @@ gdb_open_cloexec (const char *filename, int flags, unsigned long mode)
 FILE *
 gdb_fopen_cloexec (const char *filename, const char *opentype)
 {
-  FILE *result = NULL;
+  FILE *result;
   /* Probe for "e" support once.  But, if we can tell the operating
      system doesn't know about close on exec mode "e" without probing,
      skip it.  E.g., the Windows runtime issues an "Invalid parameter
      passed to C runtime function" OutputDebugString warning for
      unknown modes.  Assume that if O_CLOEXEC is zero, then "e" isn't
      supported.  */
-  static int fopen_e_ever_failed = O_CLOEXEC == 0;
+  static int fopen_e_ever_failed_einval = O_CLOEXEC == 0;
 
-  if (!fopen_e_ever_failed)
+  if (!fopen_e_ever_failed_einval)
     {
       char *copy;
 
@@ -329,15 +329,16 @@ gdb_fopen_cloexec (const char *filename, const char *opentype)
 	 this path.  */
       strcat (copy, "e");
       result = fopen (filename, copy);
-    }
 
-  if (result == NULL)
-    {
-      /* Fallback.  */
-      result = fopen (filename, opentype);
-      if (result != NULL)
-	fopen_e_ever_failed = 1;
+      if (result == NULL && errno == EINVAL)
+	{
+	  result = fopen (filename, opentype);
+	  if (result != NULL)
+	    fopen_e_ever_failed_einval = 1;
+	}
     }
+  else
+    result = fopen (filename, opentype);
 
   if (result != NULL)
     maybe_mark_cloexec (fileno (result));

Re: [patchv2] Minor O_CLOEXEC optimization, "regression" fix

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> On Wed, 09 Oct 2013 15:34:17 +0200, Kai Tietz wrote:
>> So yes, probing for EINVAL seems to me like a valid way to probe for valid
>> arguments here.

Jan> OK, therefore updated the patch.

Jan> No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu.
Jan> It still reduces the number of syscalls.

Looks good to me.

Tom

[commit] [patchv2] Minor O_CLOEXEC optimization, "regression" fix

[Jan Kratochvil]

On Wed, 09 Oct 2013 17:48:18 +0200, Tom Tromey wrote:
> Looks good to me.

Checked in:
	https://sourceware.org/ml/gdb-cvs/2013-10/msg00058.html


Thanks,
Jan

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Eli Zaretskii]

> Date: Wed, 9 Oct 2013 15:10:16 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org, Tom Tromey <tromey@redhat.com>
> 
> On Tue, 08 Oct 2013 21:44:33 +0200, Tom Tromey wrote:
> > >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > Jan> -  if (result == NULL)
> > Jan> +  if (result != NULL)
> > Jan> +    fopen_e_ever_succeeded = 1;
> > Jan> +  else if (!fopen_e_ever_succeeded)
> > 
> > What if we have it check for EINVAL instead?
> 
> May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
> because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.

Please don't do that.  On latest versions of Windows, the runtime
library functions tend to invoke the "invalid parameter handler" in
these cases, which more often than not will crash the program.

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Eli Zaretskii]

> Date: Wed, 9 Oct 2013 09:34:17 -0400 (EDT)
> From: Kai Tietz <ktietz@redhat.com>
> Cc: gdb-patches@sourceware.org, Tom Tromey <tromey@redhat.com>
> 
> > May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
> > because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.
> 
> Yes, it fails due the 'e' command in mode.  Obviously this option isn't supported on Windows due there is no fork.

What does this have to do with fork?  Windows does know how to start
child processes, so I think making the handle not inherited is the
proper equivalent on Windows.

Latest runtime libraries have the non-standard 'N' flag.
Alternatively, one can make the handle non-inheritable after opening
the file, by using a Win32 API.

> So yes, probing for EINVAL seems to me like a valid way to probe for valid arguments here.

As I wrote earlier, this is not a good idea, as GDB will likely crash
on latest Windows versions due to invalid parameter handling in the
library.

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Please don't do that.  On latest versions of Windows, the runtime
Eli> library functions tend to invoke the "invalid parameter handler" in
Eli> these cases, which more often than not will crash the program.

The status quo ante was to do the call but not check for EINVAL.
So the bug, if there is one, is already there.
Can you check?

Tom

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, ktietz@redhat.com,
>         gdb-patches@sourceware.org
> Date: Wed, 09 Oct 2013 11:08:47 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> Please don't do that.  On latest versions of Windows, the runtime
> Eli> library functions tend to invoke the "invalid parameter handler" in
> Eli> these cases, which more often than not will crash the program.
> 
> The status quo ante was to do the call but not check for EINVAL.
> So the bug, if there is one, is already there.
> Can you check?

I could check, if I know what to check.

This patch confused me: it tests whether O_CLOEXEC is zero, which it
is on Windows, so there should be no need to try "e" at all.
Therefore, I don't understand why this is still an issue.  What am I
missing?

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Jan Kratochvil]

On Wed, 09 Oct 2013 19:28:54 +0200, Eli Zaretskii wrote:
> This patch confused me: it tests whether O_CLOEXEC is zero, which it
> is on Windows, so there should be no need to try "e" at all.

If O_CLOEXEC is 0 then the "e" flag is not tried at all, therefore it could
not crash on MS-Windows.


> Therefore, I don't understand why this is still an issue.  What am I
> missing?

On platforms where O_CLOEXEC is 0 this new patch has no effect.


Jan

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Eli Zaretskii]

> Date: Wed, 9 Oct 2013 19:41:24 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Tom Tromey <tromey@redhat.com>, ktietz@redhat.com,
>         gdb-patches@sourceware.org
> 
> On Wed, 09 Oct 2013 19:28:54 +0200, Eli Zaretskii wrote:
> > This patch confused me: it tests whether O_CLOEXEC is zero, which it
> > is on Windows, so there should be no need to try "e" at all.
> 
> If O_CLOEXEC is 0 then the "e" flag is not tried at all, therefore it could
> not crash on MS-Windows.

That's what I thought.

> > Therefore, I don't understand why this is still an issue.  What am I
> > missing?
> 
> On platforms where O_CLOEXEC is 0 this new patch has no effect.

Then why did you ask this upthread:

> May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
> because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.

?  That's what confused me.  Sorry if I missed something.

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Jan Kratochvil]

On Wed, 09 Oct 2013 19:58:12 +0200, Eli Zaretskii wrote:
> Then why did you ask this upthread:
> 
> > May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
> > because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.
> 
> ?  That's what confused me.  Sorry if I missed something.

I did not realize / expect O_CLOEXEC is 0 on MS-Windows.  Also MS-Windows has
many build platforms like MinGW, Cygwin, ActiveState(?), 32 vs. 64 bit, native
vs. cross, there exist various sets of include files either original or stolen
by dirty room / clean room approaches, so when we talk about "MS-Windows" here
do not believe we cover all the possible ways how to build MS-Windows hosted
GDB.

Also I do not believe that every platform not supporting "e" has to return
EINVAL for it, some platform could IMO return a different error code, relying
on EINVAL for "e" is IMO not cross-platform compatible enough.

But whenever I try to build GDB on a non-Linux platform it fails for me anyway
so I do not much believe in the claimed and attempted wide platform
compatibility of GDB.  Therefore I found it fine to make the code simpler and
more optimal on GNU/Linux when there is an agreement from other maintainer(s).


Regards,
Jan

Re: [patch] Minor O_CLOEXEC optimization, "regression" fix

[Eli Zaretskii]

> Date: Wed, 9 Oct 2013 20:37:28 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: tromey@redhat.com, ktietz@redhat.com, gdb-patches@sourceware.org
> 
> On Wed, 09 Oct 2013 19:58:12 +0200, Eli Zaretskii wrote:
> > Then why did you ask this upthread:
> > 
> > > May one rely on MS-Windows fopen("","re") will fail with EINVAL if it fails
> > > because of the "e" flag, Kai?  It is in GDB function gdb_fopen_cloexec.
> > 
> > ?  That's what confused me.  Sorry if I missed something.
> 
> I did not realize / expect O_CLOEXEC is 0 on MS-Windows.

Ah, okay.  Then I guess the problem doesn't exist, as "e" will never
be tried in the MinGW build.

> Also MS-Windows has
> many build platforms like MinGW, Cygwin, ActiveState(?), 32 vs. 64 bit, native
> vs. cross, there exist various sets of include files either original or stolen
> by dirty room / clean room approaches, so when we talk about "MS-Windows" here
> do not believe we cover all the possible ways how to build MS-Windows hosted
> GDB.

I was talking about MinGW only.

> But whenever I try to build GDB on a non-Linux platform it fails for me anyway

Well, whenever I build with MinGW on Windows, it always succeeds
(unless there are real bugs in GDB).

Anyway, thanks for clarifying, and sorry for making noise.