search

fungos / cr

cr.h: A Simple C Hot Reload Header-only Library
https://fungos.github.io/cr-simple-c-hot-reload/
MIT License
1.54k stars 101 forks source link

Assert protection? #66

Closed clibequilibrium closed 2 years ago

clibequilibrium commented 2 years ago

Hey, I hope this message finds you well.

Is there a possibility to add assert protection? I was looking at cr_signal_to_failureand sigsetjmp

For instance when imgui throws something like Assertion failed: (g.WithinEndChild) && "Must call EndChild() and not End()!", file imgui.cpp, line 7162 currently my .dll just crashes and isn't rolling back as it's not considering it as a seg fault or anything like that .

I am on windows, gcc + mingw32

fungos commented 2 years ago

Hello! In linux an assert would emit an abort which would translate to a sigabrt which should be handled by cr. I'm not sure what mingw32 does here, but I would expect it to be compatible with linux. In resume, assert should be handled, if it is not working then it is something to fix. I'll need time to investigate all situations, but I don't have an easy mingw32 setup ready. If you want to contribute a fix, it would be awesome otherwise I can't promise any timeline on this.

clibequilibrium commented 2 years ago

Hello! In linux an assert would emit an abort which would translate to a sigabrt which should be handled by cr. I'm not sure what mingw32 does here, but I would expect it to be compatible with linux. In resume, assert should be handled, if it is not working then it is something to fix. I'll need time to investigate all situations, but I don't have an easy mingw32 setup ready. If you want to contribute a fix, it would be awesome otherwise I can't promise any timeline on this.

Thank you for your reply. I will try on Ubuntu to see if that's the case and see what I can do for mingw32. Once I find anything I will try to make a solution and have a PR ready. Thank you 🙌

fungos commented 2 years ago

See: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=msvc-170 You can try putting an abort() somewhere and debugging what happens on signal handler side, but following this doc, it should be the same expected behavior.

clibequilibrium commented 2 years ago

Okay I think because the assert is thrown from a 3rd party library which is statically linked to my core library hence why the assert protection is not working. Could that be the reason?

I have core library (which is statically linked to imgui) and is not hot reloaded. And I have an application that dynamically links to the core library. When I change something on application side related to imgui that will purposely trigger an assert (i.e imgui layer poping of ids) this causes assert and host halts.

EDIT: I confirm that crash protection does not work for me at all on windows + mingw32 with basic samples , I will investigate I think it's related to try catch and __except

image

clibequilibrium commented 2 years ago

Could you please take a look at this PR when you have time? @fungos I also have question is that okay to return -1 in the try catch block ?

Please note while this PR adds crash protection it doesn't add assertation protection yet. Thank you

https://github.com/fungos/cr/pull/67

clibequilibrium commented 2 years ago

I also think I will explore your sigsetjmpand longjmpunder MingW32 to see if I can properly support everything.

EDIT: the PR has been updated to properly support assert and crash protection under MingW32, please let me know if I missed anything. Please note I noticed that signal handlers would reset hence I call cr_plat_initagain after rollback.

Everything appears to be working as wanted. I am now protected from both asserts and segmentation faults 😄

clibequilibrium commented 2 years ago

I see longjmp and setjmp fails from time to time. I am not sure if my PR is stable enough to be merged. https://web.archive.org/web/20210625044548/http://www.agardner.me/golang/windows/cgo/64-bit/setjmp/longjmp/2016/02/29/go-windows-setjmp-x86.html

EDIT: I found some clues as well and updated the PR. Should be good to go now !

fungos commented 2 years ago

Hi @clibequilibrium , sorry about the delay. Great to know that you found the issue and fixed it yourself, this is awesome! I put some comments on the PR, just details. Overall it looks good to me.

Can you also take the time and add your name to the contributors list in the top comment/documentation in the file before merging the PR?

Thanks again for contributing!

clibequilibrium commented 2 years ago

Will do @fungos ! Thank you for your feedback

fungos commented 2 years ago

Fixed by #67