Debugger compatibility

[tkf]

It seems we may need to use disable_sigint to support the JuliaInterpreter.jl out-of-the-box https://discourse.julialang.org/t/debugger-fails-on-pycall/23560/3

At the moment, users have to do push!(JuliaInterpreter.compiled_modules, PyCall): https://discourse.julialang.org/t/debugger-fails-on-pycall/23560/6

@stevengj You didn't want to put disable_sigint for all ccall #574 (comment) but maybe replacing only the sigatomic_begin-sigatomic_end pairs is OK?

[yuyichao]

Afaict if the interpretor has different behavior than normal execution it's the interpreters bug.

[tkf]

Thanks. So it looks like push!(compiled_methods, which(disable_sigint, Tuple{Function})) etc. are added in PR JuliaDebug/JuliaInterpreter.jl#108 to workaround JuliaDebug/JuliaInterpreter.jl#106 and just postponed solving the real bug.

But I'm still interested in #574 so moving to disable_sigint is something I want to do anyway. It's just more PyCall code would become debuggable once JuliaInterpreter.jl fix the issue. So I don't think there is any harm in using disable_sigint. It just makes PyCall rely less on non-public functions.

[yuyichao]

And reading through the list of compiled methods from JuliaInterpreter, they all seems like temporary workaround and none of them seems like the correct long term solution. This means that changing other's code is not the correct solution either.

In this and also a few other cases like pointerset, the problem is simply that the underlying function requires special treatment. For this case specifically, without reading the code or trying myself, I assume the interpreter simply blindly executed the ccall, which causes the error. However, the signal handling is something a debugger has to deal with. The correct thing to do here is to intercept the ccall, use it to update a counter maintained by the interpreter for the current task instead of performing the ccall. Depending on the signal handing user interface you want to expose, the interpreter could itself run either with the signal delivery enabled or disabled and it could run the user code with the signal handling disabled if the counter is not-0.

just postponed solving the real bug.

Correct. And that's pretty much all what I see in that list as mentioned above.

But I'm still interested in #574 so moving to disable_sigint is something I want to do anyway

The correct solution there is likely a cfunction flag.

[yuyichao]

Closing since this is clearly a debugger bug and it doesn't make much sense to workaround it here when there's already an easy workaround for the user. whether to use disable_sigint is an independent issue that already has discussion elsewhere so no reason to do it here.

[tkf]

Closing this issue is OK, I guess. I don't think there is an active issue tracking only disable_sigint vs sigatomic_(begin|end). PR #574 is mixing it with different harder issues. But opening a separate issue that does this is a cleaner approach.

The correct solution there is likely a cfunction flag.

What do you mean by this? We have code like

PyCall.jl/src/pyinit.jl

Line 40 in 0b2dc20

pyjlwrap_call_ptr = @cfunction(pyjlwrap_call, PyPtr, (PyPtr,PyPtr,PyPtr))

and you are suggesting to improve it somehow? Something is better than PyPtr?

[yuyichao]

No, I mean it needs to be a Julia change. The only thin you need a guaranteed on is that n signal is delivered before you can catch it. It has men mentioned a few times but I cannot find it.....

[tkf]

It would be nice if there is an issue in Julia repository that I can subscribe to.

By the way, I just tried the repro code I posted in JuliaLang/julia#29498 and now it works with Julia 1.3.0-DEV.121 (dc6c7c7e6f). Maybe JuliaLang/julia#31191 [1] fixed what you had in mind? But it seems the issue fixed by it was more about the world-age.

[1] mentions JuliaLang/julia#29498

[yuyichao]

What did I had in mind? That julia issue doesn't seem to have anything to do with signal handling.

[tkf]

That's why I was puzzled.

[yuyichao]

What are you puzzled by? I'm not sure what you mean by "what I had in mind" but the only thing I've mentioned in this thread that you didn't understand that is related to #574 is adding new function to cfunction. It has nothing to do with anything else and has nothing to do with this issue. I don't believe there's an issue for it since it was previously only mentioned in the context of PyCall signal handling either in one of the issues here or on discourse.

I never once mentioned either JuliaLang/julia#29498 or JuliaLang/julia#31191 so I never had anything in particular about them in mind, apart from that it doesn't seem to be signal handling related at all. And if you are just puzzled by why it's fixed or why you can hit a non-signal handling related issue, then, well, that's just how things usually works..........................

[tkf]

You said

The correct solution there is likely a cfunction flag.

From your later clarifications, my understanding was that you were saying some Julia internal related to cfunction and signal handler has to be fixed to resolve #574. However, it seems that #574 is fixed by JuliaLang/julia#31191 while it seems, as I said, that

the issue fixed by it was more about the world-age

and that the PR JuliaLang/julia#31191 does not have

anything to do with signal handling

as you said.

So, my understanding is that #574 is fixed by a patch that has nothing to do with the "correct solution" I thought you had in mind. That's why I was puzzled.

I never once mentioned either JuliaLang/julia#29498 or JuliaLang/julia#31191

JuliaLang/julia#29498 is the "Julia mirror" of #574. That's why I'm mentioning it. I'm mentioning JuliaLang/julia#31191 because, as I said, JuliaLang/julia#31191 is linking JuliaLang/julia#29498 and JuliaLang/julia#31191 may have fixed JuliaLang/julia#29498.

And if you are just puzzled by why it's fixed or why you can hit a non-signal handling related issue, then, well, that's just how things usually works

I don't understand this sentence. Are you saying the signal handler bug is still there and JuliaLang/julia#31191 just hid the symptom without fixing the cause?

[yuyichao]

From your later clarifications, my understanding was that you were saying some Julia internal related to cfunction and signal handler has to be fixed to resolve #574

Errr, #574 is a pull request and that's all what I was talking about. There's nothing to "fix" for a PR. In the context of a PR, I think it's pretty clear that what I was saying is the implementation of it, especially since I explicitly quoted your "moving to disable_sigint". I'm mentioning it because I don't think moving to disable_sigint has anything to do with that issue and it is also not the correct solution for the issue that PR is trying to fix.

However, it seems that #574 is fixed by JuliaLang/julia#31191 while it seems, as I said, that

If you are talking about the issue you hit in #574, you should be more clear on that........ It wasn't even mentioned anywhere in this issue before my mentioning of the correct solution.

So, my understanding is that #574 is fixed by a patch that has nothing to do with the "correct solution" I thought you had in mind. That's why I was puzzled.

So, no it's not. The issue you hit in #574 is unrelated to signal handling nor the correct solution. #574 is a PyCall PR and needs changes in julia to correctly fix.

JuliaLang/julia#29498 is the "Julia mirror" of #574

And no, really.... #574 is about signal handling, JuliaLang/julia#29498 is not...... A mirror issue is when closing one closes the other one. You merely have some dependencies in between due to the current implementation in the PR. The two are not equivalent...

I don't understand this sentence

I'm making very wield guesses of what you are puzzled by. Since you were puzzled by me mentioning the issue you hit in #574 being not signal handling related, I was guessing you were puzzled by or didn't realized that JuliaLang/julia#29498 is not really related to #574 or signal handling at all. From

Are you saying the signal handler bug is still there and JuliaLang/julia#31191 just hid the symptom without fixing the cause?

It seems that I wasn't too off. To clarify again, JuliaLang/julia#29498 is not a signal handler bug. JuliaLang/julia#29498 might have been fixed JuliaLang/julia#31191. I didn't check either so I don't know but either way, neither of the two fix or hide any signal handling bug. There are AFAICT no julia signal handler bug mentioned anywhere in any linked issues in this thread so far. The only thing I said that'll help implementing the correct solution in #574 is a small feature addition, not a bug.

[yuyichao]

I don't think there is an active issue tracking only disable_sigint vs sigatomic_(begin|end)

And I don't see a reason there need to be one. Debugger is completely irrelevant since it's a debugger bug that already has a workaround. The two doesn't make any difference for #574 (the PR, not any issue you hit in that PR though those issues have even less to do with either functions). Only minor issue being sigatomic_* being unexported functions. This being the only reason and that's what I believe can just fit in #574, which would be removing all signal handling code from ccall site.