Add lightweight command line option processor to jerry-main #1809

akosthekiss · 2017-05-09T23:08:09Z

Eases command line option definition, usage summary and detailed
help message printing, and option processing.

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

zherczeg · 2017-05-10T06:43:08Z

We should wait landing #1797 first.

akosthekiss · 2017-05-10T07:01:35Z

On the contrary, I'd like to see this landed first. The CLI of our C tools is less than professional, and the snapshot merging tool just continues that bad tradition. I'd like to see the new tool use proper command line option handling, too.

zherczeg · 2017-05-10T07:16:47Z

The command line parsing in that patch is correct, and it would be better if this patch would contain all changes related to moving to a new method. I would rather avoid a situation when that patch needs to rework command line parsing again.

LaszloLango · 2017-05-10T07:26:10Z

Is this a binary size optimization?

akosthekiss · 2017-05-10T07:46:16Z

The command line parsing in that patch is correct

We disagree in that regard. It arbitrarily differs from the other tools we are using/writing. (git is not a right analogy for that tool, but I'll comment there, too.) I'm happy to show how to adapt to this approach and get a more flexible, easier to use, more intuitive CLI.

Is this a binary size optimization?

No (or, at least, I don't know). As far as I know, we don't care about the size of jerry the tool. This turns this

$ build/bin/jerry --help
Usage: build/bin/jerry [OPTION]... [FILE]...

Options:
  -h, --help
  -v, --version
  --mem-stats
  --mem-stats-separate
  --parse-only
  --show-opcodes
  --show-regexp-opcodes
  --start-debug-server
  --save-snapshot-for-global FILE
  --save-snapshot-for-eval FILE
  --save-literals-list-format FILE
  --save-literals-c-format FILE
  --exec-snapshot FILE
  --log-level [0-3]
  --abort-on-fail
  --no-prompt

into this

$ build/bin/jerry --help
build/bin/jerry [-h] [-v] [--mem-stats] [--mem-stats-separate] [--parse-only]
  [--show-opcodes] [--show-regexp-opcodes] [--start-debug-server]
  [--save-snapshot-for-global FILE] [--save-snapshot-for-eval FILE]
  [--save-literals-list-format FILE] [--save-literals-c-format FILE]
  [--exec-snapshot FILE]... [--log-level NUM] [--abort-on-fail] [--no-prompt]
  [FILE]...

  -h, --help            print this help and exit
  -v, --version         print tool and library version and exit
  --mem-stats           dump memory statistics
  --mem-stats-separate  dump memory statistics and reset peak values after parse
  --parse-only          don't execute JS input
  --show-opcodes        dump parser byte-code
  --show-regexp-opcodes dump regexp byte-code
  --start-debug-server  start debug server and wait for a connecting client
  --save-snapshot-for-global FILE
  --save-snapshot-for-eval FILE
  --save-literals-list-format FILE
  --save-literals-c-format FILE
  --exec-snapshot FILE  execute input snapshot file(s)
  --log-level NUM       set log level (0-3)
  --abort-on-fail       segfault on internal failure (instead of non-zero exit code)
  --no-prompt           don't print prompt in REPL mode
  FILE                  input JS file(s)

LaszloLango · 2017-05-10T07:49:59Z

I see. Good patch. Why don't have any description for --save-* options? Is that intentional?

akosthekiss · 2017-05-10T08:14:22Z

Why don't have any description for --save-* options?

Now, I have to reveal that I'm not sure what they do exactly (or rather, what the exact difference between the variants is). I'd be happy to add the help messages if someone sent them to me (PM is fine).

zherczeg · 2017-05-10T08:25:28Z

--save-snapshot-for-global FILE
Snapshot will be evaluated at global context (even if we run it from a JS callback)
--save-snapshot-for-eval FILE
Snapshot will be evaluated at local context (variables in the scope chain are partly visible to the snapshot)
--save-literals-list-format FILE
Literals encountered during parsing exported in a list format (useful for external magic strings).
--save-literals-c-format FILE
Literals encountered during parsing exported in a C format (useful for external magic strings).

galpeter · 2017-05-10T08:40:05Z

jerry-main/cli.c

+{
+  for (int i = 0; i < cnt; i++)
+  {
+    printf (" ");


Just a nitpick: can't we just do this?: printf("%*s", cnt, " ");

I'm not sure, but I think jerry-libc does not support it.

Unfortunately, no. jerry-libc doesn't support * width specifier

akosthekiss · 2017-05-10T12:22:14Z

Thanks for helping me out with the help strings. PR updated

akosthekiss · 2017-05-11T09:02:22Z

PR is updated (option help message line wrapping added + some minor light touches). current output is:

$ ./build/bin/jerry --help
./build/bin/jerry [-h] [-v] [--mem-stats] [--mem-stats-separate] [--parse-only]
  [--show-opcodes] [--show-regexp-opcodes] [--start-debug-server]
  [--save-snapshot-for-global FILE] [--save-snapshot-for-eval FILE]
  [--save-literals-list-format FILE] [--save-literals-c-format FILE]
  [--exec-snapshot FILE]... [--log-level NUM] [--abort-on-fail] [--no-prompt]
  [FILE]...

  -h, --help            print this help and exit
  -v, --version         print tool and library version and exit
  --mem-stats           dump memory statistics
  --mem-stats-separate  dump memory statistics and reset peak values after
                        parse
  --parse-only          don't execute JS input
  --show-opcodes        dump parser byte-code
  --show-regexp-opcodes dump regexp byte-code
  --start-debug-server  start debug server and wait for a connecting client
  --save-snapshot-for-global FILE
                        save binary snapshot of parsed JS input (for execution
                        in global context)
  --save-snapshot-for-eval FILE
                        save binary snapshot of parsed JS input (for execution
                        in local context by eval)
  --save-literals-list-format FILE
                        export literals found in parsed JS input (in list
                        format)
  --save-literals-c-format FILE
                        export literals found in parsed JS input (in C source
                        format)
  --exec-snapshot FILE  execute input snapshot file(s)
  --log-level NUM       set log level (0-3)
  --abort-on-fail       segfault on internal failure (instead of non-zero exit
                        code)
  --no-prompt           don't print prompt in REPL mode
  FILE                  input JS file(s)

zherczeg · 2017-05-11T10:18:35Z

GCC says

Usage: gcc [options] file...
Options:
  -pass-exit-codes         Exit with highest error code from a phase
  ...

I think a similar approach is enough for us.

akosthekiss · 2017-05-11T10:43:24Z

@zherczeg but compare to our other tools tools/build.py --help, tools/run-tests.py --help, etc.

zherczeg

Unfortunately this is very complex for me. I really have no idea how to add a new argument without your help, which is kind of a maintenance risk. Especially because there is no documentation. But it blocks other work so lets land it.

zherczeg · 2017-05-15T06:43:09Z

jerry-main/cli.c

+  {
+    int length = -1;
+    int i = 0;
+    for (; i < CLI_LINE_LENGTH - CLI_LINE_TAB && help[i] != 0; i++)


for (int i = 0;

cannot do that, i is used after the loop

zherczeg · 2017-05-15T06:43:59Z

jerry-main/cli.c

+              int argc, /**< number of command line arguments */
+              char **argv) /**< array of command line arguments */
+{
+  return (cli_opt_state_t)


Returning with structures is always costly. It is better to pass a pointer.

On many platforms, this is exactly what happens when returning structs. AFAIK both on ARM32 and X86, struct returns are done by passing a hidden pointer argument to the function, where it has to write the results. So, x = f(); and f(&x); are exactly the same. (The former might work better with optimizations, as the ABI-enforced extra pointer argument might be easier to track in pointer aliasing algorithms than completely generic pointer arguments, but that can be use case specific, whether there is a real difference).

(BTW, this is not performance-critical code (even if there would be any performance penalty). This function is typically called only once in an application. And this is for jerry-main only, not for anyone to use.)

zherczeg · 2017-05-15T06:44:39Z

jerry-main/cli.c

+void
+cli_opt_help (const cli_opt_t *opts) /**< array of command line option definitions, terminated by CLI_OPT_END */
+{
+  for (; opts->id != CLI_OPT_END; opts++)


This should be a while loop.

zherczeg · 2017-05-15T06:45:40Z

jerry-main/cli.h

+  CLI_QUANT_Q, /**< ? (Question mark: optional, zero or one) */
+  CLI_QUANT_A, /**< * (Asterisk, star: zero or one or more) */
+  CLI_QUANT_P, /**< + (Plus sign: one or more) */
+  CLI_QUANT_1  /**< 1 (One: one) */


What about two argument.

No need for that, as there is no use case other than these, which could be supported easily. Note that the quantifiers are for display only, not enforced in any other way.

? gets displayed as [-x X] or [X]

* gets displayed as [-x X]... or [X]...

+ gets displayed as -x X... or X...

1 gets displayed as -x X or X

There is no easy or conventional notation for more general quantifiers (like min 3, max infinity) for command line arguments, and I'm not sure we'd need them. (This is not a general purpose CLI library, it's for the needs of jerry-main only.)

(Just don't forget that the snapshot merging patch requires an optional argument with two items, a name and an index)

That's not the same, that's what .argc is for. .quant describes whether and option (together with its argument(s)) is optional or not (or how many times it should appear on the command line). E.g., --exec-snapshot snapfile1 --exec-snapshot snapfile2 --exec-snapshot snapfile3 is a valid use, that's why we have [--exec-snapshot FILE]... in the usage. For the snapshot merging patch, another option is to be added, used as --exec-snapshot-func snapfile1 index1 IIRC, which means that it has two arguments, but the option together with its two arguments may also appear arbitrarily many times on the command line (.argc = 2, .meta = "FILE NUM", .quant = CLI_QUANT_A), displayed in usage as [--exec-snapshot-func FILE NUM]....

akosthekiss · 2017-05-15T10:07:33Z

I've updated the PR and rebased it to latest master.

LaszloLango · 2017-05-16T06:16:12Z

tools/cppcheck/suppressions-list

@@ -2,4 +2,4 @@ wrongmathcall:tests/unit-libm/test-libm.inc.h
 variableScope:jerry-libm/*.c
 invalidPointerCast:jerry-libm/*.c
 arithOperationsOnVoidPointer:jerry-libc/*.c
-commaSeparatedReturn:jerry-ext/include/jerryscript-ext/arg.impl.h
+commaSeparatedReturn:*


Could you explain this change? I'm not sure suppressing a warning in the whole code base is a good idea.

Cppcheck warns on every struct literal in returns, because it misinterprets them as comma-separated expressions. This is quite bad IMO and obviously a shortcoming of cppcheck, because struct literals are completely valid C constructs.

LaszloLango · 2017-05-16T06:27:55Z

jerry-main/main-unix.c

+  OPT_LOG_LEVEL,
+  OPT_ABORT_ON_FAIL,
+  OPT_NO_PROMPT
+} jerry_opt_id_t;


Based on the previous conversation on the naming conventions, I'd rename this to cli_opt_id_t. jerry_ prefix should be reserved for API functions and types.

what about main_opt_id_t ? (I'd keep cli_ for cli.{c,h})

akosthekiss · 2017-05-16T07:08:49Z

I've rebased the PR to latest master, resolved conflicts, and renamed variables and types in main-unix.c as requested

LaszloLango

LGTM

Eases command line option and sub-command definition, usage summary and detailed help message printing, and argv processing. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

REPL could read JS file from standard input when we passed "-" option, but jerryscript-project#1809 missed to add this function to the new option parser. JerryScript-DCO-1.0-Signed-off-by: Csaba Osztrogonác [email protected]

akosthekiss added the tools Related to the tooling scripts label May 9, 2017

galpeter reviewed May 10, 2017

View reviewed changes

akosthekiss force-pushed the main-cli branch from 95cc77b to 210aab2 Compare May 10, 2017 12:16

akosthekiss force-pushed the main-cli branch from 210aab2 to f292ad9 Compare May 10, 2017 15:48

akosthekiss mentioned this pull request May 10, 2017

Support multiple primary functions in a single snapshot. #1797

Merged

akosthekiss force-pushed the main-cli branch 3 times, most recently from e2923b0 to 42474f8 Compare May 11, 2017 07:44

akosthekiss force-pushed the main-cli branch 4 times, most recently from d2d83fa to 59e4ef2 Compare May 12, 2017 15:31

zherczeg approved these changes May 15, 2017

View reviewed changes

akosthekiss force-pushed the main-cli branch 2 times, most recently from c4523f7 to e6a01ec Compare May 15, 2017 09:50

LaszloLango reviewed May 16, 2017

View reviewed changes

LaszloLango requested changes May 16, 2017

View reviewed changes

akosthekiss force-pushed the main-cli branch from e6a01ec to 71ae060 Compare May 16, 2017 07:07

LaszloLango approved these changes May 16, 2017

View reviewed changes

Add lightweight command line processor to jerry-main

62522d7

Eases command line option and sub-command definition, usage summary and detailed help message printing, and argv processing. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

akosthekiss force-pushed the main-cli branch from 71ae060 to 62522d7 Compare May 16, 2017 07:23

akosthekiss merged commit 7837440 into jerryscript-project:master May 16, 2017

akosthekiss deleted the main-cli branch May 16, 2017 08:29

ossy-szeged mentioned this pull request Nov 29, 2019

Make REPL option parser handle "-" option properly #3384

Merged

Add lightweight command line option processor to jerry-main #1809

Add lightweight command line option processor to jerry-main #1809

Uh oh!

Conversation

akosthekiss commented May 9, 2017

Uh oh!

zherczeg commented May 10, 2017

Uh oh!

akosthekiss commented May 10, 2017

Uh oh!

zherczeg commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaszloLango commented May 10, 2017

Uh oh!

akosthekiss commented May 10, 2017

Uh oh!

LaszloLango commented May 10, 2017

Uh oh!

akosthekiss commented May 10, 2017

Uh oh!

zherczeg commented May 10, 2017

Uh oh!

galpeter May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented May 10, 2017

Uh oh!

akosthekiss commented May 11, 2017

Uh oh!

zherczeg commented May 11, 2017

Uh oh!

akosthekiss commented May 11, 2017

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented May 16, 2017

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zherczeg commented May 10, 2017 •

edited

Loading

galpeter May 10, 2017 •

edited

Loading