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 signed to unsigned conversion in QuickJS stack overflow check #19

Merged
merged 3 commits into from Aug 12, 2019
Merged

Fix signed to unsigned conversion in QuickJS stack overflow check #19

merged 3 commits into from Aug 12, 2019

Conversation

ghost
Copy link

@ghost ghost commented Aug 9, 2019

Hi!

Sometimes QuickJS gives stack overflow exception when it shouldn't because of signed to unsigned conversion in the code that checks stack usage. I've sent the patch to QuickJS mailing list, but it would be nice to have this merged (and maybe even published) before the next QuickJS release.

@theduke
Copy link
Owner

theduke commented Aug 11, 2019

Hey. I also wanted to apply some patches, eg for GetOwnPropertyNames support, but in the end I decided against it due to

  • the extra maintenance burden of keeping them up to date
  • bug and behaviour drift between the embedded version and the system version...

I guess I would be ok with it with a different approach:

  • add embed/patches directory that contains patch files to be applied
  • add something like a patched feature that applies the patches before build

@ghost
Copy link
Author

ghost commented Aug 11, 2019

@theduke I've added "patched" feature as you suggested.

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

Looks awesome!
One idea: we could use a crate like patch-rs as a build dependency and use it to apply the patches instead of depending on the patch executable.

But I'm not sure if that's worth it - you need make and a C compiler to build, which means you will in all likelihood also have patch available.
Not sure what the situation on Windows is though.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

I've checked that possibility before using patch executable, but it looks like neither patch-rs nor patch crates support patching given directory using a patch file.

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

Looking at the docs, I think patch-rs support applying a patch to a String.
The API is just very messy.

But yeah I think we can just use the patch executable for now and reconsider if someone complains.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

But we have a directory with multiple files. So in order to use it the patches should be per-file, if I understand it correctly.

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

Good to merge after a rebase.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

I want to also add feature matrix to CircleCI build, as I've missed it at first.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

I've added it, but not sure that it would work, as CircleCI builds seem to work only on master.

a-rodin and others added 3 commits August 13, 2019 00:06
This commit adds a new "patched" feature to both the
bindings and the main crate.

This feature applies patches to the embedded version that fix up/extend quickjs.

All patches are in libquickjs-sys/embed/patches.
@theduke theduke merged commit e4643a6 into theduke:master Aug 12, 2019
@theduke
Copy link
Owner

theduke commented Aug 12, 2019

I fixed up the circleci config and squashed your commits down to 2.

big thanks!

@theduke
Copy link
Owner

theduke commented Aug 13, 2019

@a-rodin the patching fails on windows due to argument escaping: https://dev.azure.com/the-duke/quickjs-rs/_build/results?buildId=211&view=logs

Related: rust-lang/rust#29494

@ghost
Copy link
Author

ghost commented Aug 13, 2019

@theduke I'm thinking about the following workaround: copy the patches to the build directory and then use relative paths instead of the absolute paths.

@ghost ghost mentioned this pull request Aug 13, 2019
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.

2 participants