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

Remove duktape and use QuickJS #295

Merged
merged 8 commits into from Nov 23, 2023
Merged

Remove duktape and use QuickJS #295

merged 8 commits into from Nov 23, 2023

Conversation

wargio
Copy link
Owner

@wargio wargio commented Sep 13, 2023

@trufae this adds support to r2dec to use quickjs
unfortunately i couldn't use the code you implemented because i wanted to also support the ci.
i had to remove the Makefile version.

@trufae
Copy link
Contributor

trufae commented Sep 13, 2023

Cool! happy to see the relative paths. But i can't get it to compile:

With make

0$ make -C p
/Applications/Xcode.app/Contents/Developer/usr/bin/make build
make[1]: *** No rule to make target `duktape/duktape.o', needed by `core_pdd.dylib'.  Stop.
make: *** [all] Error 2
2$

and i assume you will deprecate the makefile :? and abandon the p directory where the duktape code was, so this is a leftover i guess.

With meson/ninja

0$ ninja -C b
ninja: Entering directory `b'
[13/18] Generating js/bytecode.h with a custom command
FAILED: js/bytecode.h
/Users/pancake/prg/r2js/b/subprojects/libquickjs/qjsc ../js/r2dec-plugin.js -m -N main_bytecode -c -o js/bytecode.h
Could not load '-m'
ninja: build stopped: subcommand failed.
1$

the fix is this:

0$ git diff
diff --git a/js/meson.build b/js/meson.build
index 364905c..c993154 100644
--- a/js/meson.build
+++ b/js/meson.build
@@ -9,14 +9,14 @@ if get_option('standalone')
       'bytecode.h',
       output : 'bytecode.h',
       input : r2dec_testsuite,
-      command : [qjsc, '@INPUT@', '-m', '-N', 'main_bytecode', '-c', '-o', '@OUTPUT@'],
+      command : [qjsc, '-m', '-N', 'main_bytecode', '-c', '-o', '@OUTPUT@', '@INPUT@'],
   )
 else
   bytecode_h = custom_target(
       'bytecode.h',
       output : 'bytecode.h',
       input : r2dec_plugin,
-      command : [qjsc, '@INPUT@', '-m', '-N', 'main_bytecode', '-c', '-o', '@OUTPUT@'],
+      command : [qjsc, '-m', '-N', 'main_bytecode', '-c', '-o', '@OUTPUT@', '@INPUT@'],
   )
 endif

@@ -25,4 +25,4 @@ bytecode_mod_h = custom_target(
     output : 'bytecode_mod.h',
     input : bytecode_h,
     command : [modjs_gen, '@INPUT@', '@OUTPUT@'],
-)
\ No newline at end of file
+)
0$

after this i managed to get it working, at least with small functions. otherwise i get an error (which maybe affects master, didnt tried)
a.zip <-

another thing i noticed is this CC error:

[0x100003a84]> pddi
ERROR: Missing space after CC
Do you want to print 1 lines? (y/N)
[0x100003a84]>

Do you have some numbers about the performance? hope the whole codebase gets some es6 improvements after merging this

@wargio
Copy link
Owner Author

wargio commented Sep 14, 2023

i haven't tested time differences.

@trufae
Copy link
Contributor

trufae commented Nov 16, 2023

So it is ready for testing then? i can make up some numbers

@wargio
Copy link
Owner Author

wargio commented Nov 16, 2023

looks like there are some issues on windows (tests) but for me can be tested

@wargio
Copy link
Owner Author

wargio commented Nov 16, 2023

i need to still apply your changes

@wargio
Copy link
Owner Author

wargio commented Nov 17, 2023

I have fixed windows build and the issue you had. i cannot tho fix the debian package or the macos one

@trufae
Copy link
Contributor

trufae commented Nov 20, 2023

I did some tests: r2 -q -c af -c '?t pdd' /bin/ls

  • master branch: 0.59s
  • qjs branch: 0.38s

building with meson b --buildtype=release

is there a way to install the plugin in the home? because ninja -C b install is putting things in the system path always

@wargio
Copy link
Owner Author

wargio commented Nov 21, 2023

is there a way to install the plugin in the home? because ninja -C b install is putting things in the system path always

just do --prefix=whatever

@trufae
Copy link
Contributor

trufae commented Nov 23, 2023

yeah thing is that home directory doesnt follow system structure, but thats not really a blocker for me, so im good to get this merged. i have a bunch of other improvements but i would prefer to get this merged before moving forward.

Good work!

@wargio wargio merged commit c882788 into master Nov 23, 2023
5 checks passed
@wargio wargio deleted the qjs branch November 23, 2023 12:01
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.

None yet

2 participants