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

Fix CrashHandler & make async-signal-safe #144

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nickdowell
Copy link

This pull request restores to working order the CrashHandler that was added in 4084b3b but later broken by a9ba442 which removed the call to initialiseCrashHandler().

With this fixed pluginval once more shows a backtrace when a plugin crashes, addressing one of the issues raised in #136

This PR also makes handleCrash() more async-signal-safe compliant by avoiding functions like malloc() which use locks and could deadlock if already held by the crashed thread.

I have manually tested these changes on macOS, Windows & Linux.

@sudara
Copy link
Collaborator

sudara commented Dec 7, 2024

NIce! RIP kill9WithSomeMercy

drowaudio added a commit that referenced this pull request Dec 9, 2024
@drowaudio
Copy link
Contributor

Thanks a lot for this. It looks good but I've refactored it a bit to make the style more consistent with the existing code base and factored out the stack trace writing functions from the crash handler.

Can you take a look here and let me know if that still looks ok?
https://github.com/Tracktion/pluginval/tree/nickdowel/develop

If so, I'll get it merged.

@nickdowell
Copy link
Author

Factoring out the stack trace writing functions from the crash handler has broken the numLinesToSkip functionality. This required backtrace() to be called directly from the signal handler so we knew how many stack entries to skip - inlined function calls are not reported by backtrace() and we cannot know whether or not the compiler will inline writeStackTrace(). Therefore it's probably better to simply remove numLinesToSkip entirely as it was only a nicety.

Aside from that, I have not spotted any technical issues but also have not tried to build or test your branch. I did notice a number of unrelated changes on that branch that may have been accidentally pushed there instead of develop?

On on personal level, it's rewarding when PRs get that lovely "Merged" status and to see ones name appear on git blame, so I wish you had requested changes to this PR rather than refactoring on your end. May be something to keep in mind if you want to encourage external contributions.

@drowaudio
Copy link
Contributor

I needed to add those other changes in order to get it to pass CI.

Ok, I'll remove the numLinesToSkip as it might be a bit error prone if the structure ever changes again or things get inlined etc.

Regarding the PR and merging, I just needed a place to do the refactoring and be able to test it, ensure it passed CI and show it to you. The fastest way to do that seemed to just copy your changes on to a local branch. My intention was to merge all the non-stacktrace related changes back to develop and then you could copy the changes on to your fork and update the PR so I can merge it. Would that be ok?

Thanks again,
Dave

@nickdowell
Copy link
Author

I needed to add those other changes in order to get it to pass CI.

Ok, I'll remove the numLinesToSkip as it might be a bit error prone if the structure ever changes again or things get inlined etc.

Regarding the PR and merging, I just needed a place to do the refactoring and be able to test it, ensure it passed CI and show it to you. The fastest way to do that seemed to just copy your changes on to a local branch. My intention was to merge all the non-stacktrace related changes back to develop and then you could copy the changes on to your fork and update the PR so I can merge it. Would that be ok?

Thanks again, Dave

Ah that would be cool, sorry for misunderstanding!

@drowaudio
Copy link
Contributor

Yes, sorry, I'm on holiday at the moment so trying to fit bits in when I get a spare half hour etc. which means my communication isn't what it normally is. I'll factor what I've done in to comments on this PR when I get a chance.

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

Successfully merging this pull request may close these issues.

3 participants