On 11/01/2024 09:42, Corinna Vinschen wrote:
On Jan 10 17:38, Jon Turney wrote:
On 10/01/2024 15:30, Corinna Vinschen wrote:
On Jan 10 13:57, Jon Turney wrote:
[...]
Also: Fix the (deprecated) cygwin_dumpstack() function so it will now
write a .stackdump file, even when ulimit -c is zero. (Note that
cygwin_dumpstack() is still idempotent, which is perhaps odd)
Given it's deprecated and not exposed in the headers, and given
we only still need the symbol for backward compat, how about making
this function a no-op instead?
We still need the function internally to write stackdumps.
Oh, right, I missed the usage in api_fatal. I was only talking
about the exported function, or rather, the fact that it's exported.
We could split it in internal and external function and...
I know it's use has long been discouraged, but doing a GitHub code search
does find some uses of it. What is the suggested replacement?
...doing nothing in the exported function was the idea. There appear to
be a handful of projects on github though, which call it. Not sure it's
the right thing to do, though. External code should better raise a
signal in this case.
I was more thinking that anything that's using it for a serious
purposes, should probably be using libunwind or something similar, which
maybe doesn't exist or work on Cygwin currently (or didn't back when it
was written).
In any case, any change to cygwin_stackdump() seems out of scope for
this work, so can we park that discussion for the moment.
However, if we take it as given, and if external code calls
cygwin_stackdump, do we really want it to create a stackdump, or
shouldn't the behaviour be the same as if a core-creating signal has
been raised? See below.
(I'm also wondering if the idempotency is in the wrong place. Is it
possible for signal_exit() get called by multiple threads? In which case it
probably needs to do something sane in that case)
signal_exit is only called from sigpacket::process, and this method
in turn is only called from the wait_sig function, so it's only
called from the signal thread.
Good news.
I just wonder if we really want to create a stackdump unconditionally
at all, after introducing corefile support and handling ulimit the
way you do it.
I.e., we have (basically) three situations:
- A core-creating signal has been raised
- api_fatal calls cygwin_stackdump
- External code calls cygwin_stackdump
Wouldn't it make sense to handle them equally, depending on
the ulimit settings?
I don't think we want to get into second-guessing why an application is
calling cygwin_stackdump(), and changing it when it's deprecated and
going to be removed seems silly.
Looking at the history, the current behaviour (of cygwin_stackdump() not
dumping based on rlim_core) seems like simply an unintended side-effect
of where the check is done.
Anyhow, rather than improving this function, there are, or at least
should be, better user-space alternatives to use.
I hadn't noticed that api_fatal() also calls cygwin_stackdump().
Assuming generating a stackdump at that point has some value, it does
seems to make sense to treat it the same. (But given all the
signal-specific things that signal_exit() does, I'm not sure the code
can be made common).
Can't this be done by adding the max size as parameter to dumper?
Maybe. That would make forward/backwards compatibility problems when mixing
dumper and cygwin versions.
How's that supposed to happen? dumper is part of the Cygwin package,
so, together with using the right absolute path, there's no way to
use the wrong dumper version. If so, it's certainly an SEP.
I don't think we can control the size of the file as we write it, we'd need
to check afterwards if it was too big, and then remove/truncate.
And we need to do the same action for stackdumps, so I think it makes more
sense to do that checking in the DLL.
I see. It's a bit unfortunate though, if dumper tries to create
a 2 Gigs file which is later truncated, if we're low on disk space.
But yeah, disk space isn't much of a problem these days, I guess...
Assuming there isn't a clear specification of which of these is supposed
to happen, I think removing is the better choice, since partial
coredumps are just useless.
(There's still some potential lossage if the coredump is big enough to
fill the disk, but less than the (perhaps badly-chosen) ulimit. But
maybe that could be fixed by having dumper remove the file if it
couldn't be written successfully))
