On Wed, Jul 09, 2025 at 03:13:06PM -0800, Phillip Wood wrote: > On 26/06/2025 21:09, Carlo Marcelo Arenas Belón wrote: > > On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@xxxxxxxxx wrote: > > > On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote: > > > > On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote: > > > > > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote: > > > > > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote: > > > > > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote: > > > > > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@xxxxxxxxx> > > > > > > > > > > > > > > > > A future change will start using sigaction to setup a SIGCHLD signal > > > > > > > > handler. > > > > > > > > > > > > > > > > The current code uses signal() which returns SIG_ERR (but doesn't > > > > > > > > seem to set errno) so instruct sigaction() to do the same. > > > > > > > > > > > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to > > > > > > > match? > > > > > > > > > > > > By "match", I mean that in both cases we will get an error return value > > > > > > and errno won't be set to EINVAL (which is what POSIX requires) > > > > > > > > > > > > In our codebase since we ignore the return code anyway, it wouldn't make > > > > > > a difference, either way. > > > > > > > > > > > > signal() returns a pointer, and sigaction() returns and int, > > > > > > > > > > Oh right, I'd forgotten they have different return types. I think we should > > > > > probably be setting errno = EINVAL before returning -1 to match what this > > > > > function does with other signals it does not support - just because our > > > > > current callers ignore the return value doesn't mean that future callers > > > > > will and they might want check errno if they see the function fail. > > > > > > > > I agree, and indeed had to triple check and change my implementation after I > > > > confirmed that signal(SIGCHLD) does not change errno on Windows (not our > > > > version, neither of the windows libc or mingw, even if it is documented[1] to > > > > do so. > > > > > > > > It might be because the signal number itself is bogus (there is none for > > > > SIGCHLD in their headers, and git uses their own numbers in compat), but > > > > either way, I would rather be consistent with signal() at least originally. > > > > > > I'm not sure I understand - don't we want the sigaction() wrapper to behave > > > like sigaction() would? > > > > for at least the first iteration, I would rather have sigaction() behave > > like signal(), so that the change doesn't introduce any regressions. > > What regressions are you worried about? Any code that might be surprised by a non 0 errno, even if I agree it unlikely to be an issue. FWIW, the Windows compat layer is not very strict on setting errno, and since a call to the CRT (at least with the current codepaths) doesn't either > We're talking about changing a > single call from signal() to sigaction(). I'd have thought we're far more > likely to introduce regressions if we change the behavior of the windows > implementation of sigaction() to behave like signal() as that introduces > more variation between different platforms. Don't get me wrong, I also want sigaction() to behave the same regardless of platform, but I would rather do that in an independent change. Indeed that is why I was asking for the possibility to change the SIGCHLD definition, so that signal() starts also behaving as documented and we can fix sigaction(SIGCHLD) to match. > > eventually, sigaction() should behave like any other sigaction(), but to > > do so, I suspect the windows emulation might need to change their SIGCHLD > > to match. > > > > just confirmed with MSVC that if I use 20 instead of 17, errno gets updated > > just like the documentation says it should. > > Oh not so setting errno as you want to do would not actually match signal() > on Windows in that case? I explained my thinking before already. I am not a Windows expert and indeed I am surprised that signal() in the CRT behaves this way, but I suspect it might be because of the same reasons why raise() triggers debugger breaks that are explicitally called for and avoided in the mingw compat code. Eitherway my preference is for this to be handled indpendently and preferably not as a prerequisite of this change. > > Carlo > > > > PS. Maybe we should get dscho involved? > > > > > > > > [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal
