[PATCH 1/1] cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn

[cygwin-patches mailing list]

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

[cygwin-patches mailing list]

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

[cygwin-patches mailing list]

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

[cygwin-patches mailing list]

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

[Jon TURNEY]

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).

[cygwin-patches mailing list]

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".

[cygwin-patches mailing list]

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