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

Use __builtin_setjmp, for MinGW gcc #23

Merged
merged 1 commit into from
Dec 17, 2017
Merged

Use __builtin_setjmp, for MinGW gcc #23

merged 1 commit into from
Dec 17, 2017

Conversation

Corion
Copy link

@Corion Corion commented Mar 10, 2017

As per http://www.agardner.me/golang/windows/cgo/64-bit/setjmp/longjmp/2016/02/29/go-windows-setjmp-x86.html

This makes the module build and test 100% OK with Strawberry Perl on Win64
directly from CPAN.

Ideally, the setjmp change would also be communicated to the upstream
Duktape maintainers, but I don't know their build process and how I can
figure out what genconfig.py does.

@svaarala
Copy link

@Corion I'll try to address this in svaarala/duktape#1443.

Any pointers to what's the "best" and most portable default for Cygwin / MinGW would be appreciated. Current default for Cygwin is _setjmp() and _longjmp() which were preferred over plain setjmp() and longjmp() (in some earlier change).

@Corion
Copy link
Author

Corion commented Apr 12, 2017

Rebased the commit

As per http://www.agardner.me/golang/windows/cgo/64-bit/setjmp/longjmp/2016/02/29/go-windows-setjmp-x86.html

This makes the module build and test 100% OK with Strawberry Perl on Win64
directly from CPAN.

Ideally, the setjmp change would also be communicated to the upstream
Duktape maintainers, but I don't know their build process and how I can
figure out what genconfig.py does.
@Corion
Copy link
Author

Corion commented Dec 16, 2017

Pinging this change

@svaarala , I'm sorry, I don't know anything about Cygwin / MinGW portability etc. - I only applied what I found on the internet to make things compile here...

@mamod , I don't know if you want to incorporate this change into your module or wait for the change upstream. It works for me and passes the tests on Perl 5.26 on Windows , and Travis also seems to have no problems with it.

@mamod
Copy link
Owner

mamod commented Dec 17, 2017

@Corion sorry for not responding to this thread earlier, I wasn't sure if it will be a good idea to add this while there were a chance it would be integrated in duktape, but now I do a small tweak to the duk_config so I think it won't be a bad idea if this change will be useful for you and other users as well

I started the development of this module on windows/ perl strawberry and it was working just fine, I don't use windows at the moment so I just set AppVoyer and to my surprise there are lots of errors and tests do not pass at all, I'll play with this now and make sure things pass on windows too, so no I don't mind integrating this at all :) thank you so much for your contribution.

P.S. I might hit you once or twice maybe for your help with tests, so please bear with me :)

@mamod
Copy link
Owner

mamod commented Dec 17, 2017

just logged out from my old beloved XP, I was mistaken, I was using active perl, strawberry didn't pass, with your change tests on both pass perfectly, so I think it's good to go. Thank you very much and again sorry for not looking into this earlier 👍

@mamod mamod merged commit 5644fca into mamod:master Dec 17, 2017
@Corion
Copy link
Author

Corion commented Dec 17, 2017

Thank you for looking after this and merging the change!

If you need further testing, feel free to ping me!

Thanks for maintaining the module!

@svaarala
Copy link

@Corion @mamod I tried to implement the change in Duktape here: svaarala/duktape#1443. It's a bit unclean to force the setjmp/longjmp provided in the DUK_F_MINGW.h.in file because it's just supposed to be a detection macro helper. As far as I can see, the MinGW specific check should go to config/platforms/platform_windows.h.in for a non-POSIX target build and config/platforms/platform_cygwin.h.in for a POSIX build. I don't actively use Windows/Cygwin/MinGW so if you could confirm this sounds right, that'd be great.

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