nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 99 forks source link

src: fix getopt format string #347

Closed bnoordhuis closed 4 years ago

bnoordhuis commented 4 years ago

v8 inspect -l <n> takes an argument.

Bug introduced in commit 2fc1571 ("src: option to limit output of findjsinstances") from September 2018.

Fixes: https://github.com/nodejs/llnode/issues/346

mmarchini commented 4 years ago

We should add some testing here. Since we don't seem to have specific tests for command arguments, we can change one of the inspect tests:

diff --git test/plugin/inspect-test.js test/plugin/inspect-test.js
index 0a59c9b..f664237 100644
--- test/plugin/inspect-test.js
+++ test/plugin/inspect-test.js
@@ -244,7 +244,7 @@ const hashMapTests = {
       });
     }, (t, sess, addresses, name, cb) => {
       const address = addresses[name];
-      sess.send(`v8 inspect --array-length 1 ${address}`);
+      sess.send(`v8 inspect -l 1 ${address}`);

       sess.linesUntil(/\]>/, (err, lines) => {
         if (err) return cb(err);

We use --array-length on other places in this test, so it should be fine.

Also our documentation is incomplete, --array-length and --string-length are not documented, but this can be handled in a separate PR.

bnoordhuis commented 4 years ago

I updated one of the tests. I should perhaps mention that I don't plan to spend much more time on this patch, I just sent it because I'm such a nice guy (and also because obvious bug is obvious.)

mmarchini commented 4 years ago

Landed in 83c810faa06d