Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix issue with @nogc breaking the signal API #879

Closed

Conversation

Hackerpilot
Copy link
Member

This is a regression from 2.065, and needs to make it into the next beta.

Context: http://forum.dlang.org/thread/myqafgvflwmttmqhhtti@forum.dlang.org

@quickfur
Copy link
Member

I thought the purpose of the original PR that introduced nothrow for signal handlers is to enforce nothrow in signal handlers? It seems to be a bad idea for a signal handler to be throwing, since the stack unwinder will probably interact very badly with signal handler context.

@Safety0ff
Copy link
Contributor

GC calls aren't async signal safe.

@Hackerpilot
Copy link
Member Author

Well, if we decide to not pull this, we'll have to add something to the release notes saying that this API has changed. (I'll also have to add '@nogc' to several functions in std.socket, as right now this change to the signals API is breaking https://github.com/CyberShadow/ae)

@complexmath
Copy link
Member

Is this change for nothrow or nogc? I think they should be considered separately. While a signal handler can never safely allocate because mutexes are not signal-safe, throwing from a signal handler does work on some platforms even though this absolutely isn't supported. In fact, I think there is some code in Druntime to do this for... segfaults? Or perhaps that pull request was never approved. Either way, I'm unsure whether throwing from signal handlers should be disallowed from a language perspective, as it can be pretty handy when you've verified that it actually works on your target platform.

@Safety0ff
Copy link
Contributor

Or perhaps that pull request was never approved.

It was merged: #187

@MartinNowak
Copy link
Member

It was merged: #187

It's in etc.c.memoryerror and still is a broken hack by design.

Either way, I'm unsure whether throwing from signal handlers should be disallowed from a language perspective, as it can be pretty handy when you've verified that it actually works on your target platform.

No, it never works because the compiler doesn't know about async exceptions, so it cannot correctly output handlers and cleanup code.
If you allow to throw exceptions from signals, then any a + b might throw, e.g. because of floating point signals. There is no way that we can emit EH code for any possible async exception.
Synchronous exceptions are fine, because they are rare and only emitted in well defined places.

@dnadlinger
Copy link
Member

GC calls certainly aren't signal safe, so the signal handler should be made @nogc eventually. Whether we should wait until the @nogc-related dust has settled down a bit is a different question.

@quickfur
Copy link
Member

ping
Are we going ahead with this, or should we close it?

@quickfur
Copy link
Member

ping

@Hackerpilot
Copy link
Member Author

I assume that's not a ping for me, right?

@quickfur
Copy link
Member

It's a general ping for everybody, reviewers, submitters alike, to move forward with this PR, either reach consensus to merge it, or close it. Anything other than leaving it to rot here. :-)

@MartinNowak
Copy link
Member

I think the best idea here is to add an overload and deprecate the old function.

private alias sigfn_t = void function(int);
private alias sigfn_nothrow_nogc_t = void function(int) @nogc nothrow;

deprecated("signal handlers should be @nogc nothrow")
sigfn_t signal(int sig, sigfn_t func);
sigfn_nothrow_nogc_t signal(int sig, sigfn_nothrow_nogc_t func);

@quickfur
Copy link
Member

I didn't know we can overload on function attributes??

P.S. Just tested, wow, apparently we can overload on function attributes!

@MartinNowak
Copy link
Member

I didn't know we can overload on function attributes??

You can't overload on attributes (I have an ER (Issue 9511) for that). Here you're overloading on the argument type, that's why you need sigfn_nothrow_nogc_t.

@quickfur
Copy link
Member

Sorry, I was imprecise in what I said. I meant overloading between two functions whose parameters are identical except for attributes attached to the parameter. I didn't know this was actually accepted by the compiler.

@MartinNowak
Copy link
Member

@Hackerpilot can you update the pull and add the deprecated overload?

@Hackerpilot
Copy link
Member Author

What about the enums such as enum SIG_ERR = cast(sigfn_t) -1;?

@MartinNowak
Copy link
Member

Those should also be sigfn_nothrow_nogc_t so that they can be used in @nogc nothrow code.

@Hackerpilot
Copy link
Member Author

Won't that disable their use in the deprecated function?

@MartinNowak
Copy link
Member

Won't that disable their use in the deprecated function?

Yes, but a nothrow @nogc function is covariant, so you can call wherever you could call the deprecated overload.


private alias void function(int) sigfn_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be a private alias to begin with. This type is visible via the public API.

@Orvid
Copy link
Contributor

Orvid commented Feb 27, 2015

Any updates on this?

@MartinNowak
Copy link
Member

Someone should still fix it, fairly easy.

@MartinNowak
Copy link
Member

Since it took so long. we can probably just leave things as they are.

@MartinNowak MartinNowak closed this Mar 7, 2015
@Orvid
Copy link
Contributor

Orvid commented Mar 7, 2015

I think this is still needed. My comment about sigfn_t being private is still a problem.

@MartinNowak
Copy link
Member

How about you make a pull then, should be trivial and I'm extremely short on time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants