* [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
@ 2020-12-03 21:21 Jeremy Drake
2020-12-04 12:10 ` Corinna Vinschen
0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Drake @ 2020-12-03 21:21 UTC (permalink / raw)
To: cygwin-patches
if a new CYGWIN/MSYS environment option `winjitdebug` is true, allowing
native subprocesses to get Windows-default error handling behavior (such
as invoking the registered JIT debugger). Cygwin processes will quickly
set their error mode on start, so getting JIT debugging for them will
still require setting `error_start`.
This patch was previously submitted to MSYS2
(https://github.com/msys2/msys2-runtime/pull/18) but it was suggested I
should try sending it upstream.
---
winsup/cygwin/environ.cc | 1 +
winsup/cygwin/globals.cc | 1 +
winsup/cygwin/spawn.cc | 2 ++
winsup/doc/cygwinenv.xml | 9 +++++++++
4 files changed, 13 insertions(+)
diff --git a/winsup/cygwin/environ.cc b/winsup/cygwin/environ.cc
index 3a03657db..fa47f4b31 100644
--- a/winsup/cygwin/environ.cc
+++ b/winsup/cygwin/environ.cc
@@ -120,6 +120,7 @@ static struct parse_thing
{"wincmdln", {&wincmdln}, setbool, NULL, {{false}, {true}}},
{"winsymlinks", {func: set_winsymlinks}, isfunc, NULL, {{0}, {0}}},
{"disable_pcon", {&disable_pcon}, setbool, NULL, {{false}, {true}}},
+ {"winjitdebug", {&spawn_default_errmode}, setbool, NULL, {{false}, {true}}},
{NULL, {0}, setdword, 0, {{0}, {0}}}
};
diff --git a/winsup/cygwin/globals.cc b/winsup/cygwin/globals.cc
index 942bd1c83..2d2ac0949 100644
--- a/winsup/cygwin/globals.cc
+++ b/winsup/cygwin/globals.cc
@@ -71,6 +71,7 @@ bool reset_com;
bool wincmdln;
winsym_t allow_winsymlinks = WSYM_sysfile;
bool disable_pcon;
+bool spawn_default_errmode;
bool NO_COPY in_forkee;
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 92d190d1a..6239e3539 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -430,6 +430,8 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
sigproc_printf ("priority class %d", c_flags);
c_flags |= CREATE_SEPARATE_WOW_VDM | CREATE_UNICODE_ENVIRONMENT;
+ if (spawn_default_errmode)
+ c_flags |= CREATE_DEFAULT_ERROR_MODE;
/* We're adding the CREATE_BREAKAWAY_FROM_JOB flag here to workaround
issues with the "Program Compatibility Assistant (PCA) Service".
diff --git a/winsup/doc/cygwinenv.xml b/winsup/doc/cygwinenv.xml
index a52b6ac19..7137edcb9 100644
--- a/winsup/doc/cygwinenv.xml
+++ b/winsup/doc/cygwinenv.xml
@@ -103,6 +103,15 @@ pty will be disabled. This is for programs which do not work properly
under pty with pseudo console enabled. Defaults to not set.</para>
</listitem>
+<listitem>
+<para><envar>winjitdebug</envar> - if set, the
+<literal>CREATE_DEFAULT_ERROR_MODE</literal> flag is passed to
+<literal>CreateProcess</literal> in <literal>spawn</literal>. This prevents
+cygwin-set error mode flags from being inherited by the new process, allowing
+native processes to invoke any system-registered JIT debugger, and/or invoke
+Windows Error Reporting. Defaults to not set.</para>
+</listitem>
+
</itemizedlist>
</sect2>
--
2.29.2.windows.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
2020-12-03 21:21 [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn Jeremy Drake
@ 2020-12-04 12:10 ` Corinna Vinschen
2020-12-04 18:35 ` Jeremy Drake
0 siblings, 1 reply; 7+ messages in thread
From: Corinna Vinschen @ 2020-12-04 12:10 UTC (permalink / raw)
To: cygwin-patches
Hi Jeremy,
On Dec 3 13:21, Jeremy Drake via Cygwin-patches wrote:
> if a new CYGWIN/MSYS environment option `winjitdebug` is true, allowing
> native subprocesses to get Windows-default error handling behavior (such
> as invoking the registered JIT debugger). Cygwin processes will quickly
> set their error mode on start, so getting JIT debugging for them will
> still require setting `error_start`.
I'm not happy about a new CYGWIN option.
Wouldn't it make sense, perhaps, to switch to CREATE_DEFAULT_ERROR_MODE
for all non-Cygwin processes by default instead?
Corinna
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
2020-12-04 12:10 ` Corinna Vinschen
@ 2020-12-04 18:35 ` Jeremy Drake
2020-12-07 9:43 ` Corinna Vinschen
0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Drake @ 2020-12-04 18:35 UTC (permalink / raw)
To: Corinna Vinschen via Cygwin-patches
On Fri, 4 Dec 2020, Corinna Vinschen via Cygwin-patches wrote:
> I'm not happy about a new CYGWIN option.
>
> Wouldn't it make sense, perhaps, to switch to CREATE_DEFAULT_ERROR_MODE
> for all non-Cygwin processes by default instead?
In fact, my first iteration was to set that flag unconditionally (relying
on the fact that SetErrorMode is called extremely early in Cygwin process
startup rather than only setting it for non-Cygwin processes), but I
received feedback that it would be better to put it behind an option:
https://github.com/msys2/msys2-runtime/pull/18#issuecomment-723683606
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
2020-12-04 18:35 ` Jeremy Drake
@ 2020-12-07 9:43 ` Corinna Vinschen
2020-12-08 14:43 ` Jon Turney
0 siblings, 1 reply; 7+ messages in thread
From: Corinna Vinschen @ 2020-12-07 9:43 UTC (permalink / raw)
To: cygwin-patches; +Cc: Jon TURNEY
On Dec 4 10:35, Jeremy Drake via Cygwin-patches wrote:
> On Fri, 4 Dec 2020, Corinna Vinschen via Cygwin-patches wrote:
>
> > I'm not happy about a new CYGWIN option.
> >
> > Wouldn't it make sense, perhaps, to switch to CREATE_DEFAULT_ERROR_MODE
> > for all non-Cygwin processes by default instead?
>
> In fact, my first iteration was to set that flag unconditionally (relying
> on the fact that SetErrorMode is called extremely early in Cygwin process
> startup rather than only setting it for non-Cygwin processes), but I
> received feedback that it would be better to put it behind an option:
>
> https://github.com/msys2/msys2-runtime/pull/18#issuecomment-723683606
Jon, thoughts on that as GDB maintainer?
Thanks,
Corinna
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
2020-12-07 9:43 ` Corinna Vinschen
@ 2020-12-08 14:43 ` Jon Turney
2020-12-08 19:58 ` Jeremy Drake
0 siblings, 1 reply; 7+ messages in thread
From: Jon Turney @ 2020-12-08 14:43 UTC (permalink / raw)
To: cygwin-patches
On 07/12/2020 09:43, Corinna Vinschen wrote:
> On Dec 4 10:35, Jeremy Drake via Cygwin-patches wrote:
>> On Fri, 4 Dec 2020, Corinna Vinschen via Cygwin-patches wrote:
>>
>>> I'm not happy about a new CYGWIN option.
>>>
>>> Wouldn't it make sense, perhaps, to switch to CREATE_DEFAULT_ERROR_MODE
>>> for all non-Cygwin processes by default instead?
I agree.
Cygwin calls SetErrorMode(), so we need to use this flag to prevent that
non-default behaviour being inherited by processes started with
CreateProcess().
>> In fact, my first iteration was to set that flag unconditionally (relying
>> on the fact that SetErrorMode is called extremely early in Cygwin process
>> startup rather than only setting it for non-Cygwin processes), but I
>> received feedback that it would be better to put it behind an option:
>>
>> https://github.com/msys2/msys2-runtime/pull/18#issuecomment-723683606
I don't really understand what that comment is saying. If Git for
Windows doesn't want the default SetErrorMode(), it should change it itself.
(The same executable not started by Cygwin will get the default error
mode in any case).
> Jon, thoughts on that as GDB maintainer?
I don't think this actually interacts with gdb.
gdb either CreateProcess() with DEBUG_PROCESS (which presumably
overrides that flag) or attaches to an existing process (possibly due to
being configured as a JIT debugger).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
2020-12-08 14:43 ` Jon Turney
@ 2020-12-08 19:58 ` Jeremy Drake
2020-12-09 9:25 ` Corinna Vinschen
0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Drake @ 2020-12-08 19:58 UTC (permalink / raw)
To: cygwin-patches
On Tue, 8 Dec 2020, Jon Turney wrote:
> On 07/12/2020 09:43, Corinna Vinschen wrote:
> > On Dec 4 10:35, Jeremy Drake via Cygwin-patches wrote:
> > > On Fri, 4 Dec 2020, Corinna Vinschen via Cygwin-patches wrote:
> > >
> > > > I'm not happy about a new CYGWIN option.
> > > >
> > > > Wouldn't it make sense, perhaps, to switch to CREATE_DEFAULT_ERROR_MODE
> > > > for all non-Cygwin processes by default instead?
>
> I agree.
>
> Cygwin calls SetErrorMode(), so we need to use this flag to prevent that
> non-default behaviour being inherited by processes started with
> CreateProcess().
>
In that case, here's my initial, much simpler patch
-- >8 --
Subject: [PATCH] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
This allows native processes to get Windows-default error handling
behavior (such as invoking the registered JIT debugger), while cygwin
processes would quickly set their error mode back to what they expect.
---
winsup/cygwin/spawn.cc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index c77d62984..f5952d53b 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -429,7 +429,8 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
c_flags = GetPriorityClass (GetCurrentProcess ());
sigproc_printf ("priority class %d", c_flags);
- c_flags |= CREATE_SEPARATE_WOW_VDM | CREATE_UNICODE_ENVIRONMENT;
+ c_flags |= CREATE_SEPARATE_WOW_VDM | CREATE_UNICODE_ENVIRONMENT
+ | CREATE_DEFAULT_ERROR_MODE;
/* We're adding the CREATE_BREAKAWAY_FROM_JOB flag here to workaround
issues with the "Program Compatibility Assistant (PCA) Service".
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn
2020-12-08 19:58 ` Jeremy Drake
@ 2020-12-09 9:25 ` Corinna Vinschen
0 siblings, 0 replies; 7+ messages in thread
From: Corinna Vinschen @ 2020-12-09 9:25 UTC (permalink / raw)
To: cygwin-patches
On Dec 8 11:58, Jeremy Drake via Cygwin-patches wrote:
> On Tue, 8 Dec 2020, Jon Turney wrote:
> > On 07/12/2020 09:43, Corinna Vinschen wrote:
> > > On Dec 4 10:35, Jeremy Drake via Cygwin-patches wrote:
> > > > On Fri, 4 Dec 2020, Corinna Vinschen via Cygwin-patches wrote:
> > > >
> > > > > I'm not happy about a new CYGWIN option.
> > > > >
> > > > > Wouldn't it make sense, perhaps, to switch to CREATE_DEFAULT_ERROR_MODE
> > > > > for all non-Cygwin processes by default instead?
> >
> > I agree.
> >
> > Cygwin calls SetErrorMode(), so we need to use this flag to prevent that
> > non-default behaviour being inherited by processes started with
> > CreateProcess().
> >
>
> In that case, here's my initial, much simpler patch
> [...]
> - c_flags |= CREATE_SEPARATE_WOW_VDM | CREATE_UNICODE_ENVIRONMENT;
> + c_flags |= CREATE_SEPARATE_WOW_VDM | CREATE_UNICODE_ENVIRONMENT
> + | CREATE_DEFAULT_ERROR_MODE;
> [...]
Thanks, but the flag should only be added when starting a non-cygwin
process. Checking against real_path.iscygexec() will do the job.
Can you please resend this as a standard git format-patch'ed mail?
Thanks,
Corinna
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-09 9:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 21:21 [PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn Jeremy Drake
2020-12-04 12:10 ` Corinna Vinschen
2020-12-04 18:35 ` Jeremy Drake
2020-12-07 9:43 ` Corinna Vinschen
2020-12-08 14:43 ` Jon Turney
2020-12-08 19:58 ` Jeremy Drake
2020-12-09 9:25 ` Corinna Vinschen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).