Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New dsp perform routine segfaults if there is no signal inlet #44

Closed
agraef opened this issue Mar 4, 2024 · 5 comments
Closed

New dsp perform routine segfaults if there is no signal inlet #44

agraef opened this issue Mar 4, 2024 · 5 comments

Comments

@agraef
Copy link
Owner

agraef commented Mar 4, 2024

I noticed this while playing around with the lua_sig example. The attached variation of that example simply segfaults if you turn on dsp in Pd, probably because of the missing in1 table argument? Here's the output from the terminal when running pd lua_sig-test.pd and turning on dsp:

PANIC: unprotected error in call to Lua API (attempt to call a nil value)
Pd: signal 6

Looking at the C code for pdlua_perform(), it seems that only the sample blocks from the signal inlets are passed to the perform method on the Lua side, not the outputs. So if there is no signal inlet, the Lua perform routine has no way of knowing how many samples to produce, and how many blocks of samples.

I think that this needs to be reworked, so that the Lua perform routine gets both the input and output data (as two tables of tables maybe). At the very least the routine needs to know exactly how many outputs there are and how large the block size is. And it should never segfault, no matter how badly garbled the Lua code is. 😜

@timothyschoen Can you please have a look?

lua_sig.zip

@agraef
Copy link
Owner Author

agraef commented Mar 4, 2024

Thinking about it some more, of course the Lua object knows about the numbers of inputs and outputs, since that information is in the self.inlets and self.outlet members, silly me.

So it should probably be enough to pass in tables for both the input and the output buffers. That should be easy enough to fix, working on it.

@agraef
Copy link
Owner Author

agraef commented Mar 4, 2024

Thinking about it some more, of course the Lua object knows about the numbers of inputs and outputs, since that information is in the self.inlets and self.outlet members, silly me.

Well, I also missed that the block size is being passed to the dsp method on the Lua side.

So it should probably be enough to pass in tables for both the input and the output buffers.

That seems to do the trick. Not sure whether it's the right thing to do, though. A Lua signal object without a signal inlet can also just create a table of the right size and then return that.

Also, I'm still worried about that PANIC message with the subsequent segfault (or rather signal 6 = abort). That seems to happen inside the Lua interpreter whenever the Lua perform method tries to access a non-existing argument. This will obviously trigger an exception in the Lua interpreter, but I thought that pcall is supposed to catch and handle this?

@timothyschoen
Copy link
Contributor

timothyschoen commented Mar 4, 2024

Also, I'm still worried about that PANIC message with the subsequent segfault (or rather signal 6 = abort). That seems to happen inside the Lua interpreter whenever the Lua perform method tries to access a non-existing argument. This will obviously trigger an exception in the Lua interpreter, but I thought that pcall is supposed to catch and handle this?

Figured out the problem: when the call to perform fails, we catch that error, but we don't return early. So it will still try to unpack the output of the function, even though the function call failed. Should be fixed with 46f9eae. Now it will just tell you without crashing:

pdlua: error in perform:
[string "lua_sig"]:34: attempt to get length of a nil value (local 'in1')

We can also considering filling the output buffers with 0 when the function call fails, just to be sure.

@agraef
Copy link
Owner Author

agraef commented Mar 4, 2024

Yeah, just figured that out as well. ;-) 46f9eae should be fine as is, no need to fill the buffers.

@agraef
Copy link
Owner Author

agraef commented Mar 4, 2024

Fixed in #43.

@agraef agraef closed this as completed Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants