* [patch] Minor O_CLOEXEC optimization, "regression" fix
@ 2013-10-08 18:32 Jan Kratochvil
2013-10-08 19:44 ` Tom Tromey
0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2013-10-08 18:32 UTC (permalink / raw)
To: gdb-patches
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);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-08 18:32 [patch] Minor O_CLOEXEC optimization, "regression" fix Jan Kratochvil
@ 2013-10-08 19:44 ` Tom Tromey
2013-10-09 13:10 ` Jan Kratochvil
0 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2013-10-08 19:44 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-08 19:44 ` Tom Tromey
@ 2013-10-09 13:10 ` Jan Kratochvil
2013-10-09 13:34 ` Kai Tietz
2013-10-09 17:02 ` Eli Zaretskii
0 siblings, 2 replies; 15+ messages in thread
From: Jan Kratochvil @ 2013-10-09 13:10 UTC (permalink / raw)
To: Kai Tietz; +Cc: gdb-patches, Tom Tromey
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 13:10 ` Jan Kratochvil
@ 2013-10-09 13:34 ` Kai Tietz
2013-10-09 14:41 ` [patchv2] " Jan Kratochvil
2013-10-09 17:07 ` [patch] " Eli Zaretskii
2013-10-09 17:02 ` Eli Zaretskii
1 sibling, 2 replies; 15+ messages in thread
From: Kai Tietz @ 2013-10-09 13:34 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: gdb-patches, Tom Tromey
----- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* [patchv2] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 13:34 ` Kai Tietz
@ 2013-10-09 14:41 ` Jan Kratochvil
2013-10-09 15:48 ` Tom Tromey
2013-10-09 17:07 ` [patch] " Eli Zaretskii
1 sibling, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2013-10-09 14:41 UTC (permalink / raw)
To: Kai Tietz; +Cc: gdb-patches, Tom Tromey
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));
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patchv2] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 14:41 ` [patchv2] " Jan Kratochvil
@ 2013-10-09 15:48 ` Tom Tromey
2013-10-09 16:01 ` [commit] " Jan Kratochvil
0 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2013-10-09 15:48 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Kai Tietz, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 13:34 ` Kai Tietz
2013-10-09 14:41 ` [patchv2] " Jan Kratochvil
@ 2013-10-09 17:07 ` Eli Zaretskii
1 sibling, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2013-10-09 17:07 UTC (permalink / raw)
To: Kai Tietz; +Cc: jan.kratochvil, gdb-patches, tromey
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 13:10 ` Jan Kratochvil
2013-10-09 13:34 ` Kai Tietz
@ 2013-10-09 17:02 ` Eli Zaretskii
2013-10-09 17:08 ` Tom Tromey
1 sibling, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2013-10-09 17:02 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: ktietz, gdb-patches, tromey
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 17:02 ` Eli Zaretskii
@ 2013-10-09 17:08 ` Tom Tromey
2013-10-09 17:29 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2013-10-09 17:08 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Jan Kratochvil, ktietz, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 17:08 ` Tom Tromey
@ 2013-10-09 17:29 ` Eli Zaretskii
2013-10-09 17:41 ` Jan Kratochvil
0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2013-10-09 17:29 UTC (permalink / raw)
To: Tom Tromey; +Cc: jan.kratochvil, ktietz, gdb-patches
> 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?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 17:29 ` Eli Zaretskii
@ 2013-10-09 17:41 ` Jan Kratochvil
2013-10-09 17:58 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2013-10-09 17:41 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Tom Tromey, ktietz, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 17:41 ` Jan Kratochvil
@ 2013-10-09 17:58 ` Eli Zaretskii
2013-10-09 18:37 ` Jan Kratochvil
0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2013-10-09 17:58 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: tromey, ktietz, gdb-patches
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 17:58 ` Eli Zaretskii
@ 2013-10-09 18:37 ` Jan Kratochvil
2013-10-09 19:08 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2013-10-09 18:37 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: tromey, ktietz, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patch] Minor O_CLOEXEC optimization, "regression" fix
2013-10-09 18:37 ` Jan Kratochvil
@ 2013-10-09 19:08 ` Eli Zaretskii
0 siblings, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2013-10-09 19:08 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: tromey, ktietz, gdb-patches
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-10-09 19:08 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-08 18:32 [patch] Minor O_CLOEXEC optimization, "regression" fix Jan Kratochvil
2013-10-08 19:44 ` Tom Tromey
2013-10-09 13:10 ` Jan Kratochvil
2013-10-09 13:34 ` Kai Tietz
2013-10-09 14:41 ` [patchv2] " Jan Kratochvil
2013-10-09 15:48 ` Tom Tromey
2013-10-09 16:01 ` [commit] " Jan Kratochvil
2013-10-09 17:07 ` [patch] " Eli Zaretskii
2013-10-09 17:02 ` Eli Zaretskii
2013-10-09 17:08 ` Tom Tromey
2013-10-09 17:29 ` Eli Zaretskii
2013-10-09 17:41 ` Jan Kratochvil
2013-10-09 17:58 ` Eli Zaretskii
2013-10-09 18:37 ` Jan Kratochvil
2013-10-09 19:08 ` Eli Zaretskii
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox