Support multiple primary functions in a single snapshot.

[zherczeg]

This patch adds an extension to snapshots which allows storing multiple position independent primary functions in a single snapshot data. A new application called jerry-snapshot is added to the project to manage snapshots. Currently the only option is merging snapshots.

[martijnthe]

It would be good to start a new doc page about these cmd line tools.
In my mind they are very useful and not really talked about in the docs.

[zherczeg]

Yes. However the tools provide help themselves, and their arguments are simple, so this documentation would probably be rarely used. However it wouldn't hurt, so at some point we could work on it.

[martijnthe]

I'd rather add a new API to avoid breaking existing projects.
jerry_exec_snapshot_at_index?

[zherczeg]

I have been thinking about it myself. This is a rarely used function, and only an extra 0 needs to be passed, so introducing another function provides little gain. Furthermore if we want to make this function obsolete, and remove it eventually, we have the same problem again, moving from a longer function name to the shorter function name.

However if people insist on it I can add another function.

[martijnthe]

FWIW, I would be interested in using it as a "library function", not as part of this CLI tool.

[martijnthe]

Whoops, wrong comment box...

[martijnthe]

Do you think having multiple snapshots in one will be common?
If not, I think there's value in having a simpler API w/o the index.

On top of that, I think a stable API is generally valued by users... It seems like a very low effort to accommodate that in this case.

[zherczeg]

Ok, you have a good point. I will add another function.

[martijnthe]

Why not split the logic to merge snapshots together into jerry-snapshot.c?

[martijnthe]

FWIW, I would be interested in using it as a "library function", not as part of this CLI tool.

[zherczeg]

Currently I have no plans to expose this to user level. It would be very complex, since you need multiple buffers, and you need to expose too much internals (which might change in the future). However it would be good to further improve this tool with other functions, e.g. with a native->other endian conversion, which also needs internal snapshot knowledge. If debugging info will be available for snapshots, we could also think about merging that as well (although the functions itself do not change, so this might be unnecessary).

Anyway this tool might not be in the best place at the moment, we could open a new directory for it (@akosthekiss ?)

[zherczeg]

Well, you need to suggest a good API header for it. And if we go this way, a single header for everything seems less appealing for me. #1793

[zherczeg]

We are not too lucky with APIs these days, so waht about creating this API when it is actually needed?

[martijnthe]

Yeah, that's fine too of course. Thanks.

[akosthekiss]

I've taken a closer look at the code and at the possibility of separating reusable logic from driver code (i.e., from code in a command line tool). I think it is not that hard to split them, actually, and it would be possible to come up with a single function call on library side that fits quite well into the existing public jerry-core snapshot API (no matter whether considered or marked as experimental or not).

@zherczeg I'd be glad if you'd consider the idea manifested in the following snippet. (There may be some loose ends but hopefully not too many. Actually, this is very much the code you wrote, just reorganized. Variable names, control structures, etc. kept as much as possible. The main change is that there is no need for snapshot contexts and linked lists, because VLAs help on the tool side -- and the library can work then with traditional arrays.)

/* Copyright JS Foundation and other contributors, http://js.foundation
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#include <string.h>

#include "byte-code.h"
#include "ecma-literal-storage.h"
#include "jerry-snapshot.h"
#include "jerryscript.h"
#include "jerryscript-port.h"
#include "jmem.h"

//// CORE

/**
 * Collect all literals from a snapshot file.
 */
static void
scan_snapshot_functions (const uint8_t *buffer_p, /**< snapshot buffer start */
                         const uint8_t *buffer_end_p, /**< snapshot buffer end */
                         const uint8_t *literal_base_p, /**< start of literal data */
                         const uint8_t *number_base_p) /**< start of number data */
{
  JERRY_ASSERT (buffer_end_p > buffer_p);

  do
  {
    ecma_compiled_code_t *bytecode_p = (ecma_compiled_code_t *) buffer_p;
    uint32_t code_size = ((uint32_t) bytecode_p->size) << JMEM_ALIGNMENT_LOG;

    if (bytecode_p->status_flags & CBC_CODE_FLAGS_FUNCTION)
    {
      jmem_cpointer_t *literal_start_p;
      uint32_t const_literal_end;

      if (bytecode_p->status_flags & CBC_CODE_FLAGS_UINT16_ARGUMENTS)
      {
        literal_start_p = (jmem_cpointer_t *) (buffer_p + sizeof (cbc_uint16_arguments_t));

        cbc_uint16_arguments_t *args_p = (cbc_uint16_arguments_t *) buffer_p;
        const_literal_end = args_p->const_literal_end;
      }
      else
      {
        literal_start_p = (jmem_cpointer_t *) (buffer_p + sizeof (cbc_uint8_arguments_t));

        cbc_uint8_arguments_t *args_p = (cbc_uint8_arguments_t *) buffer_p;
        const_literal_end = args_p->const_literal_end;
      }

      for (uint32_t i = 0; i < const_literal_end; i++)
      {
        if (literal_start_p[i] != JMEM_CP_NULL)
        {
          literal_start_p[i] = ecma_snapshot_get_literal (literal_base_p, number_base_p, literal_start_p[i]);
        }
      }
    }

    buffer_p += code_size;
  }
  while (buffer_p < buffer_end_p);
} /* scan_snapshot_functions */

/**
 * Update all literal offsets in a snapshot data.
 */
static void
update_literal_offsets (uint8_t *buffer_p, /**< snapshot buffer start */
                        const uint8_t *buffer_end_p, /**< snapshot buffer start */
                        lit_mem_to_snapshot_id_map_entry_t *lit_map_p) /**< literal map */
{
  JERRY_ASSERT (buffer_end_p > buffer_p);

  do
  {
    ecma_compiled_code_t *bytecode_p = (ecma_compiled_code_t *) buffer_p;
    uint32_t code_size = ((uint32_t) bytecode_p->size) << JMEM_ALIGNMENT_LOG;

    if (bytecode_p->status_flags & CBC_CODE_FLAGS_FUNCTION)
    {
      jmem_cpointer_t *literal_start_p;
      uint32_t const_literal_end;

      if (bytecode_p->status_flags & CBC_CODE_FLAGS_UINT16_ARGUMENTS)
      {
        literal_start_p = (jmem_cpointer_t *) (buffer_p + sizeof (cbc_uint16_arguments_t));

        cbc_uint16_arguments_t *args_p = (cbc_uint16_arguments_t *) buffer_p;
        const_literal_end = args_p->const_literal_end;
      }
      else
      {
        literal_start_p = (jmem_cpointer_t *) (buffer_p + sizeof (cbc_uint8_arguments_t));

        cbc_uint8_arguments_t *args_p = (cbc_uint8_arguments_t *) buffer_p;
        const_literal_end = args_p->const_literal_end;
      }

      for (uint32_t i = 0; i < const_literal_end; i++)
      {
        if (literal_start_p[i] != JMEM_CP_NULL)
        {
          lit_mem_to_snapshot_id_map_entry_t *current_p = lit_map_p;

          while (current_p->literal_id != literal_start_p[i])
          {
            current_p++;
          }

          literal_start_p[i] = current_p->literal_offset;
        }
      }
    }

    buffer_p += code_size;
  }
  while (buffer_p < buffer_end_p);
} /* update_literal_offsets */

/**
 * Merge multiple snapshots into one.
 *
 * @return length of the merged snapshot - if successful,
 *         0 - otherwise.
 */
size_t
jerry_merge_snapshots (uint8_t **in_buffer_pp, /**< array of (pointers to start of) input buffers */
                       size_t *in_buffer_size_p /**< array of input buffer sizes */
                       size_t in_buffer_cnt, /**< number of input buffers */
                       uint8_t *out_buffer_p, /**< (pointer to start of) output buffer */
                       size_t out_buffer_size) /**< size of output buffer */
{
  JERRY_UNUSED (in_buffer_size_p); /* it could be useful for memory safety but is unused right now */

  uint32_t number_of_funcs = 0;
  size_t functions_size = sizeof (jerry_snapshot_header_t);

  for (size_t i = 0; i < in_buffer_cnt; i++)
  {
    jerry_snapshot_header_t *header_p = (jerry_snapshot_header_t *) in_buffer_pp[i];

    uint32_t start_offset = header_p->func_offsets[0] & ~JERRY_SNAPSHOT_EVAL_CONTEXT;
    const uint8_t *data_p = (const uint8_t *) header_p;
    const uint8_t *literal_base_p;
    const uint8_t *number_base_p;

    literal_base_p = ecma_snapshot_get_literals_base ((uint32_t *) (data_p + header_p->lit_table_offset),
                                                      &number_base_p);

    JERRY_ASSERT (header_p->number_of_funcs > 0);

    number_of_funcs += header_p->number_of_funcs;
    functions_size += header_p->lit_table_offset - start_offset;

    scan_snapshot_functions (data_p + start_offset,
                             data_p + header_p->lit_table_offset,
                             literal_base_p,
                             number_base_p);
  }

  JERRY_ASSERT (number_of_funcs > 0);

  functions_size += JERRY_ALIGNUP ((number_of_funcs - 1) * sizeof (uint32_t), JMEM_ALIGNMENT);

  if (functions_size >= out_buffer_size)
  {
    return 0; /* Output buffer is too small */
  }

  jerry_snapshot_header_t *header_p = (jerry_snapshot_header_t *) out_buffer_p;

  header_p->version = JERRY_SNAPSHOT_VERSION;
  header_p->lit_table_offset = (uint32_t) functions_size;
  header_p->number_of_funcs = number_of_funcs;

  lit_mem_to_snapshot_id_map_entry_t *lit_map_p;
  uint32_t literals_num;

  if (!ecma_save_literals_for_snapshot ((uint32_t *) out_buffer_p,
                                        out_buffer_size,
                                        &functions_size,
                                        &lit_map_p,
                                        &literals_num))
  {
    return 0; /* Not enough space for literals */
  }

  uint32_t *func_offset_p = header_p->func_offsets;
  uint8_t *dst_p = out_buffer_p + sizeof (jerry_snapshot_header_t);
  dst_p += JERRY_ALIGNUP ((number_of_funcs - 1) * sizeof (uint32_t), JMEM_ALIGNMENT);

  for (size_t i = 0; i < in_buffer_cnt; i++)
  {
    jerry_snapshot_header_t *current_header_p = (jerry_snapshot_header_t *) in_buffer_pp[i];

    uint32_t start_offset = current_header_p->func_offsets[0] & ~JERRY_SNAPSHOT_EVAL_CONTEXT;

    memcpy (dst_p,
            ((const uint8_t *) current_header_p) + start_offset,
            current_header_p->lit_table_offset - start_offset);

    update_literal_offsets (dst_p,
                            dst_p + current_header_p->lit_table_offset - start_offset,
                            lit_map_p);

    uint32_t current_offset = (uint32_t) (dst_p - out_buffer_p) - start_offset;

    for (uint32_t i = 0; i < current_header_p->number_of_funcs; i++)
    {
      /* Updating offset without changing any flags. */
      *func_offset_p++ = current_header_p->func_offsets[i] + current_offset;
    }

    dst_p += current_header_p->lit_table_offset - start_offset;
  }

  JERRY_ASSERT (dst_p - out_buffer_p == header_p->lit_table_offset);

  jmem_heap_free_block (lit_map_p, literals_num * sizeof (lit_mem_to_snapshot_id_map_entry_t));

  return functions_size;
} /* jerry_merge_snapshots */

//// TOOL

/**
 * Maximum size for loaded snapshots
 */
#define JERRY_BUFFER_SIZE (1048576)

/**
 * Standalone Jerry exit codes
 */
#define JERRY_STANDALONE_EXIT_CODE_OK   (0)
#define JERRY_STANDALONE_EXIT_CODE_FAIL (1)

static uint8_t input_buffer[ JERRY_BUFFER_SIZE ];
static uint8_t output_buffer[ JERRY_BUFFER_SIZE ];

/**
 * Loading a single file into the memory.
 *
 * @return length of the read file - loading is successful
 *         0 - otherwise
 */
static size_t
read_file (uint8_t *input_pos_p, /**< position to read file into */
           const char *file_name) /**< file name */
{
  FILE *file = fopen (file_name, "r");
  if (file == NULL)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error: failed to open file: %s\n", file_name);
    return 0;
  }

  size_t max_size = (size_t) (input_buffer + JERRY_BUFFER_SIZE - input_pos_p);

  size_t bytes_read = fread (input_pos_p, 1u, max_size, file);
  fclose (file);

  if (!bytes_read)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error: failed to read file: %s\n", file_name);
    return 0;
  }

  if (bytes_read >= max_size)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error: file too large: %s\n", file_name);
    return 0;
  }

  if (bytes_read < sizeof (jerry_snapshot_header_t))
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error: invalid snapshot file: %s\n", file_name);
    return 0;
  }

  jerry_snapshot_header_t *header_p = (jerry_snapshot_header_t *) input_pos_p;

  if (header_p->version != JERRY_SNAPSHOT_VERSION)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Error: invalid snapshot version: %s\n", file_name);
    return 0;
  }

  printf ("Input file '%s' loaded. Contains %d function(s).\n", file_name, header_p->number_of_funcs);

  return JERRY_ALIGNUP (bytes_read, JMEM_ALIGNMENT);
} /* read_file */

/**
 * Print help.
 */
static void
print_help (char *name) /**< program name */
{
  printf ("Usage: %s [COMMAND] [ARGUMENTS]\n"
          "\n"
          "Command list:\n"
          "  merge [OUTPUT_FILE] [INPUT_FILEs]...\n"
          "      Merge input files into output file.\n"
          "  help\n"
          "      Prints this help.\n"
          "\n",
          name);
} /* print_help */

/**
 * Main function.
 *
 * @return error code (0 - no error)
 */
int
main (int argc, /**< number of arguments */
      char **argv) /**< argument list */
{
  if (argc <= 1 || (argc == 2 && (!strcmp ("help", argv[1]))))
  {
    print_help (argv[0]);
    return JERRY_STANDALONE_EXIT_CODE_OK;
  }

  if (strcmp ("merge", argv[1]))
  {
    printf ("Unknown command: %s\n", argv[1]);
    print_help (argv[0]);
    return JERRY_STANDALONE_EXIT_CODE_OK;
  }

  if (argc < 5)
  {
    printf ("At least an output and two input files must be passed\n");
    return JERRY_STANDALONE_EXIT_CODE_FAIL;
  }

  jerry_init (JERRY_INIT_EMPTY);

  size_t input_count = argc - 3;
  uint8_t *input_buffers[input_count];
  size_t input_sizes[input_count];
  uint8_t *input_pos_p = input_buffer;

  for (int i = 0; i < input_count; i++)
  {
    size_t input_size = read_file (input_pos_p, argv[3 + i]);
    if (input_size == 0)
    {
      jerry_cleanup ();
      return JERRY_STANDALONE_EXIT_CODE_FAIL;
    }

    input_buffers[i] = input_pos_p;
    input_sizes[i] = input_size;
    input_pos_p += input_size;
  }

  size_t output_size = jerry_merge_snapshots (input_buffers, input_sizes, input_count,
                                              output_buffer, JERRY_BUFFER_SIZE);
  if (output_size == 0)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Cannot merge snapshots\n");
    jerry_cleanup ();
    return JERRY_STANDALONE_EXIT_CODE_FAIL;
  }

  FILE *file = fopen (argv[2], "w");
  if (file == NULL)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "Cannot open file: '%s'\n", argv[2]);
    jerry_cleanup ();
    return JERRY_STANDALONE_EXIT_CODE_FAIL;
  }

  fwrite (output_buffer, 1u, output_size, file);
  fclose (file);

  jerry_cleanup ();
  return JERRY_STANDALONE_EXIT_CODE_OK;
} /* main */

[zherczeg]

Patch updated according to the comments. Shall we wait until we have conclusion #1798 and add an experimental API or we can land this and add the API later?

[LaszloLango]

LGTM

[LaszloLango]

IMHO we can land this and add the API later if needed.

[zherczeg]

Any more comments?

[akosthekiss]

First comments on the command line tools, mostly on usability.

A general note though: I'm worried about the snapshot merging tool reaching so deep into jerry-core. It needs knowledge on byte code types, on snapshot serialisation internals, on literals. I'm not sure that this is something that anyone should know about. It would be worth investigating whether these routines could be made part of jerry-core. (I've seen @martijnthe having similar comments, perhaps for slightly different reasons. Anyway, I'd +1 that.)

[akosthekiss]

There is already a str_to_uint in this file. I see no need for duplicating that logic. The error handling can be done at the single call site of to_index

[akosthekiss]

this option is not documented in the usage help (around line 220)

[akosthekiss]

This is an unconventional command line syntax. Users would be more familiar with a "progname [-h] [-o outfilename] infilename infilename [...]" command line interface. It does need a bit more coding (a for loop like in main-unix.c).

("-o outfilename" may be mandatory, of course. And perhaps "-o" is not aligned with the option names of main-unix.c)

[zherczeg]

Not unconventional. See git diff.

[zherczeg]

I don't think mandatory options needs any kind of dash-something. And this is a special application.

[zherczeg]

This is the reason why #1798 was created. After we have a decision we can create an experimental API for this.

[zherczeg]

Patch updated.

[akosthekiss]

(minor) base is always starts -> base always starts

[akosthekiss]

would be better (smaller) to have error messages the jerry.c way:

#ifdef JERRY_ENABLE_ERROR_MESSAGES
static const char * const some_msg_p = "some error message";
#endif /* JERRY_ENABLE_ERROR_MESSAGES */

jerry_value_t
some_function ()
{
  return ecma_raise_type_error (ECMA_ERR_MSG (some_msg_p));
}

Then, if error messages are disabled, the string literal does not get built in the binary.

(Unfortunately, this is only used in jerry.c consistently.)

[akosthekiss]

BTW, don't we have range error for this kind of error?

[akosthekiss]

I've promised a proper CLI based on #1809

/**
 * Command line option IDs
 */
typedef enum
{
  OPT_HELP,
  OPT_OUT
} snap_opt_id_t;

/**
 * Command line options
 */
static cli_opt_t snap_opts[] =
{
  CLI_OPT_DEF (.id = OPT_HELP, .opt = "-h", .longopt = "--help",
               .help = "print this help and exit"),
  CLI_OPT_DEF (.id = OPT_OUT, .opt = '-o', .argc = 1, .meta = "FILE", .quant = CLI_QUANT_1,
               .help = "output merged snapshot file (mandatory)"),
  CLI_OPT_DEF (.id = CLI_OPT_POSITIONAL, .meta = "FILE", .quant = CLI_QUANT_P,
               .help = "input snapshot files"),
  CLI_OPT_DEF (.id = CLI_OPT_END)
};

/**
 * Check whether a usage-related condition holds. If not, print an error
 * message, print the usage, and terminate the application.
 */
static void
check_usage (bool condition, /**< the condition that must hold */
             const char *name, /**< name of the application (argv[0]) */
             const char *msg, /**< error message to print if condition does not hold */
             const char *opt) /**< optional part of the error message */
{
  if (!condition)
  {
    jerry_port_log (JERRY_LOG_LEVEL_ERROR, "%s%s\n", msg, opt != NULL ? opt : "");
    cli_usage (name, snap_opts);
    exit (JERRY_STANDALONE_EXIT_CODE_FAIL);
  }
} /* check_usage */

int
main (int argc,
      char **argv)
{
  const char *in_files[argc];
  int in_file_counter = 0;
  const char *out_file = NULL;

  cli_state_t cli_state = cli_init (snap_opts, argc - 1, argv + 1);
  for (bool process = true; process;)
  {
    switch (cli_process (&cli_state))
    {
      case OPT_HELP:
      {
        cli_help (argv[0], snap_opts);
        return JERRY_STANDALONE_EXIT_CODE_OK;
      }
      case OPT_OUT:
      {
        check_usage (out_file == NULL, argv[0], "Error: output file name already specified", NULL);
        out_file = cli_state.arg[1];
        break;
      }
      case CLI_OPT_POSITIONAL:
      {
        in_files[in_file_counter++] = cli_state.arg[0];
        break;
      }
      case CLI_OPT_END:
      {
        process = false;
        break;
      }
      case CLI_OPT_INCOMPLETE:
      {
        check_usage (false, argv[0], "Error: incomplete option: ", cli_state.arg[0]);
        break;
      }
      case CLI_OPT_UNKNOWN:
      default:
      {
        check_usage (false, argv[0], "Error: unrecognized option: ", cli_state.arg[0]);
        break;
      }
    }
  }

  check_usage (in_file_counter > 1, argv[0], "Error: at least two input files must be specified", NULL);
  check_usage (out_file != NULL, argv[0], "Error: output file name must be specified", NULL);

  // below this point, you have in_files[], in_file_counter, out_file set up
  // and you can write the file names in any order, you wont have to keep it in mind
  //    jerry-snap -o outf inf1 inf2
  //    jerry-snap inf1 inf2 -o outf
  // (and not overwrite something by mistake, -o will always mark the output explicitly)
  // and if you do, you still have a nice CLI with usual -h, --help
}

[zherczeg]

This is clearly wrong. The format of the parameters:

First, the command part:

Command: must be merge or help (no -h and such)

Second the arguments of the command.

help can have a single argument, the type of a command, e.g. merge. Currently it does not process the arguments

merge has an output file, and then input file arguments

The output is not an option, so it has no -o flag. At least two input files must be specified.

Fortunately I realized we don't need to use argument parsing in this tool, so we can just keep the current code. The only important thing is that we need add an option with two arguments to main-unix.c, how can I do that?

[akosthekiss]

We still disagree. Sub-commands in programs are intuitive only if they combine various really different functionalities in a single tool. Like git: git branch has semantics and CLI syntax very different from git add or git clone. (git pushes that to the extremes, though, having sub-sub-commands at some places, like git submodule update.) But then, even in git there is the option syntax after the sub-(sub-)commands.

And in this tool, there is only one functionality: merging snapshots. Printing help does not count as another, IMO.

(Even if there would be more sub-commands in this tool, it would probably be highly beneficial to have proper CLI for each, which is possible with cli.c by starting argv-processing from the 2nd index, roughly by cli_init (snap_opts, argc - 2, argv + 2);)

As for the merging functionality, a linker tool is a much better analogy. There, just like here, you have (potentially multiple) inputs and a single output. And the usual syntax is tool -o outfile infile1 infile2... or tool infile1 infile2... -o outfile (or, it is possible to put the -o outfile part anywhere, actually). I'd argue that for those tools -o is mandatory (omitting it and producing a.out should not really be a valid option). But having it as an option allows better flexibility and user-error-proofness on the command line.

Finally, two args with an option go with:

  CLI_OPT_DEF (.id = OPT_XXX, .longopt = '--xxx', .argc = 2, .meta = "META1 META2",
               .help = "xxx"),

and they will be available as cli_state.arg[1] and cli_state.arg[2]

[zherczeg]

I mentioned in the beginning that eventually we should add all snapshot management to this program, e.g. other endian conversion was requested before. So this is just a start.

I am unhappy with this argument parsing direction (putting the output in the middle of the argument list is not a clever idea for me) but I decided to give up. This work is very important for binary size reduction and delaying it further is not worth it.

[akosthekiss]

All my reviews and comments are made on professional basis for the sake of quality. If you write a management tool for users (even if they are developers), make it familiar, user friendly, and user proof.

[akosthekiss]

If this tool is indeed going to be a multi-purpose tool then sub-commands are justified. I've added a commit to #1809 that adds sub-command handling support to the CLI code.

Here is a completely working code that does all what seems to be needed by the current variant of this tool (should work almost by copy-pasting):

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include "cli.h"

/* Exit code */
#define JERRY_STANDALONE_EXIT_CODE_OK   (0)
#define JERRY_STANDALONE_EXIT_CODE_FAIL (1)

/** General options */
typedef enum
{
  OPT_HELP,
  OPT__FIRST_NON_GENERAL /**< sub-command-specific options should start from here */
} snap_opt_id_t;

static cli_opt_t snap_opts[] =
{
  CLI_OPT_DEF (.id = OPT_HELP, .opt = "-h", .longopt = "--help", .quant = CLI_QUANT_1,
               .help = "print this help and exit"),
  CLI_OPT_DEF (.id = CLI_OPT_END)
};

/** Options specific to the merge command */
typedef enum
{
  OPT_MERGE_OUT = OPT__FIRST_NON_GENERAL
} merge_opt_id_t;

static cli_opt_t merge_opts[] =
{
  CLI_OPT_DEF (.id = OPT_HELP, .opt = "-h", .longopt = "--help",
               .help = "print this help and exit"),
  CLI_OPT_DEF (.id = OPT_MERGE_OUT, .opt = "-o", .argc = 1, .meta = "FILE", .quant = CLI_QUANT_1,
               .help = "output merged snapshot file (mandatory)"),
  CLI_OPT_DEF (.id = CLI_OPT_POSITIONAL, .meta = "FILE", .quant = CLI_QUANT_P,
               .help = "input snapshot files"),
  CLI_OPT_DEF (.id = CLI_OPT_END)
};

/** Tool sub-commands */
typedef enum
{
  CMD_MERGE
} snap_cmd_id_t;

static cli_cmd_t snap_cmds[] =
{
  CLI_CMD_DEF (.id = CMD_MERGE, .cmd = "merge", .opts = merge_opts,
               .help = "merge snapshot file"),
  CLI_CMD_DEF (.id = CLI_CMD_END)
};

/** argv[0] */
static char *progname;

/**
 * Check whether a usage-related condition holds. If not, print an error
 * message, print the usage, and terminate the application.
 */
static void
check_usage (bool condition, /**< the condition that must hold */
             const char *name, /**< name of the application/command */
             cli_opt_t *cli_opts, /**< command line options to print usage for */
             const char *msg, /**< error message to print if condition does not hold */
             const char *opt) /**< optional part of the error message */
{
  if (!condition)
  {
    printf /*jerry_port_log*/ (/*JERRY_LOG_LEVEL_ERROR, */"%s%s\n", msg, opt != NULL ? opt : "");
    cli_opt_usage (name, cli_opts);
    exit (JERRY_STANDALONE_EXIT_CODE_FAIL);
  }
} /* check_usage */

/** The function that deals with the merge sub-command */
int
do_merge (int argc, /**< number of command line arguments (beyond "argv[0] merge") */
          char **argv) /**< array of command line arguments (beyond "argv[0] merge") */
{
  CLI_CMD_NAME (cmdname, progname, "merge");

  const char *in_files[argc];
  int in_file_counter = 0;
  const char *out_file = NULL;

  cli_opt_state_t state = cli_opt_init (merge_opts, argc, argv);
  for (bool process = true; process;)
  {
    switch (cli_opt_process (&state))
    {
      case OPT_HELP:
      {
        cli_opt_usage (cmdname, merge_opts);
        printf ("\n");
        cli_opt_help (merge_opts);
        return JERRY_STANDALONE_EXIT_CODE_OK;
      }
      case OPT_MERGE_OUT:
      {
        check_usage (out_file == NULL, cmdname, merge_opts, "Error: output file name already specified", NULL);
        out_file = state.arg[1];
        break;
      }
      case CLI_OPT_POSITIONAL:
      {
        in_files[in_file_counter++] = state.arg[0];
        break;
      }
      case CLI_OPT_END:
      {
        process = false;
        break;
      }
      case CLI_OPT_INCOMPLETE:
      {
        check_usage (false, cmdname, merge_opts, "Error: incomplete option: ", state.arg[0]);
        break;
      }
      case CLI_OPT_UNKNOWN:
      default:
      {
        check_usage (false, cmdname, merge_opts, "Error: unrecognized option: ", state.arg[0]);
        break;
      }
    }
  }

  check_usage (in_file_counter > 1, cmdname, merge_opts, "Error: at least two input files must be specified", NULL);
  check_usage (out_file != NULL, cmdname, merge_opts, "Error: output file name must be specified", NULL);

  // below this point, you have in_files[], in_file_counter, out_file set up
  // and you can write the file names in any order, you wont have to keep it in mind
  //    jerry-snap merge -o outf inf1 inf2
  //    jerry-snap merge inf1 inf2 -o outf
  // (and not overwrite something by mistake, -o will always mark the output explicitly)
  // and if you do, you still have a nice CLI with usual -h, --help

  /* THIS IS DUMMY CODE */
  printf ("in files:");
  for (int i = 0; i < in_file_counter; i++)
  {
    printf (" %s", in_files[i]);
  }
  printf ("\n");
  printf ("out file: %s\n", out_file);

  return JERRY_STANDALONE_EXIT_CODE_OK;
} /* do_merge */

/** The function that deals with the case when no sub-command was specified. */
int
do_none (int argc, /**< number of command line arguments (beyond "argv[0]") */
         char **argv) /**< array of command line arguments (beyond "argv[0]") */
{
  cli_opt_state_t state = cli_opt_init (merge_opts, argc, argv);
  for (bool process = true; process;)
  {
    switch (cli_opt_process (&state))
    {
      case OPT_HELP:
      {
        cli_cmd_usage (progname, snap_cmds);
        cli_opt_usage (progname, snap_opts);
        printf ("\n");

        printf ("commands:\n");
        cli_cmd_help (snap_cmds);
        printf ("\n");

        printf ("general options:\n");
        cli_opt_help (snap_opts);
        printf ("\n");

        for (cli_cmd_t *cmds = snap_cmds; cmds->id != CLI_CMD_END; cmds++)
        {
          printf ("%s options:\n", cmds->cmd);
          cli_opt_help (cmds->opts);
        }

        return JERRY_STANDALONE_EXIT_CODE_OK;
      }
      case CLI_OPT_END:
      {
        process = false;
        break;
      }
      case CLI_OPT_POSITIONAL:
      case CLI_OPT_INCOMPLETE:
      case CLI_OPT_UNKNOWN:
      default:
      {
        check_usage (false, progname, snap_opts, "Error: unrecognized option: ", state.arg[0]);
        break;
      }
    }
  }

  check_usage (false, progname, snap_opts, "Error: empty command line", NULL);
  return JERRY_STANDALONE_EXIT_CODE_FAIL;
} /* do_none */

/** Tool entry point: a dispatcher for sub-commands */
int
main (int argc, /**< number of command line arguments, including argv[0] */
      char **argv) /**< array of command line arguments, including argv[0] */
{
  progname = argv[0];
  argc--;
  argv++;

  cli_cmd_state_t state = cli_cmd_init (snap_cmds, argc, argv);
  switch (cli_cmd_process (&state))
  {
    case CMD_MERGE:
    {
      return do_merge (argc - 1, argv + 1);
    }
    case CLI_CMD_NONE:
    {
      return do_none (argc, argv);
    }
    case CLI_CMD_UNKNOWN:
    default:
    {
      check_usage (false, progname, snap_opts, "Error: unrecognized command: ", state.arg[0]);
      break;
    }
  }

  return JERRY_STANDALONE_EXIT_CODE_FAIL;
} /* main */

Some example runs:

$ gcc -o dummy-snap dummy-snap.c cli.c

$ ./dummy-snap
Error: empty command line
./dummy-snap -h

$ ./dummy-snap -h
./dummy-snap merge [-h] -o FILE FILE...
./dummy-snap -h

commands:
  merge                 merge snapshot file

general options:
  -h, --help            print this help and exit

merge options:
  -h, --help            print this help and exit
  -o FILE               output merged snapshot file (mandatory)
  FILE                  input snapshot files

$ ./dummy-snap -x
Error: unrecognized option: -x
./dummy-snap -h

$ ./dummy-snap x
Error: unrecognized command: x
./dummy-snap -h

$ ./dummy-snap merge
Error: at least two input files must be specified
./dummy-snap merge [-h] -o FILE FILE...

$ ./dummy-snap merge -h
./dummy-snap merge [-h] -o FILE FILE...

  -h, --help            print this help and exit
  -o FILE               output merged snapshot file (mandatory)
  FILE                  input snapshot files

$ ./dummy-snap merge -o outfile infile1 infile2
in files: infile1 infile2
out file: outfile

$ ./dummy-snap merge infile1 infile2 -o outfile
in files: infile1 infile2
out file: outfile

$ ./dummy-snap merge -x
Error: unrecognized option: -x
./dummy-snap merge [-h] -o FILE FILE...

[zherczeg]

@akosthekiss I give this patch to you. I will close this PR after you open a new one. Please update it with the new argument parser and submit the patch.

[akosthekiss]

@zherczeg I'm honoured but I'll not open a new PR based on this one. I didn't write its core and I'm not familiar with either. I expect you to properly componentize it, in which I've already given help and will give in the future, too.

[zherczeg]

Patch updated. It is good for a first version, which we can update later.

[LaszloLango]

still LGTM

[akosthekiss]

main-unix-snapshot.c is still monolithic (code not separated to library and tool parts) and user-unfriendly (no re-use of CLI module for CLI processing)

[akosthekiss]

I'd suggest all metas to be all caps ("FILE IDX") so that they stand out better in the usage string

[zherczeg]

This works partly. The save is enabled (only the save is needed), but the FORCED BY SNAPSHOT TOOL message is not displayed.

[akosthekiss]

Of course. I did not give a complete walkthrough above. You have to amend the status messages

message(STATUS "FEATURE_SNAPSHOT_SAVE     " ${FEATURE_SNAPSHOT_SAVE} ${FEATURE_SNAPSHOT_SAVE_MESSAGE})

[akosthekiss]

The snapshot merge function has a unit test now, which is good, but the tool itself has still no build test.

[akosthekiss]

doc outdated

[akosthekiss]

Is this correct? fread was called with max_size, so bytes_read cannot be larger than max_size. And if the file was exactly max_size large, why would that count as an error? IMO this does not catch the situation when the whole contents of the file did not fit in the buffer. (feof could be used to decide whether we were able to read the whole file, but we don't have that in jerry-libc, unfortunately.)

[zherczeg]

This is what we can do. I don't think we loose too much with not supporting files which size is exactly max_size. We cannot get the file size with jerry libc.

[akosthekiss]

But, as I tried to point out above, the "this is what we can do" is 1) a no-op and 2) incorrect. In the above condition, bytes_read > max_size can never happen, while bytes_read == max_size is simply unnecessary and incorrectly restrictive. Why have this check then if all it does is wrong?

[zherczeg]

I cannot follow. If your file size is 12 bytes long, and you read 10 bytes, you get 10 bytes, but your file is partly loaded. Probably you get a syntax error, but it is not 100%. So there is a chance you compile something wrong, and get a success. Ok, we also loose 10 byte long files, but I can live with that. At least we don't need to process truncated files.

[akosthekiss]

byte_read comes from the statement size_t bytes_read = fread (input_pos_p, 1u, max_size, file);, see above. So, it cannot ever be bigger than max_size. See https://helpmanual.io/man3/fread/ . So, if you have 10 bytes left in the buffer, you'll call fread (input_pos_p, 1u, 10, file). If your file was 12 bytes long, you'll get 10 into bytes_read. If your file was 10 bytes long, you'll also get 10 into bytes_read. You cannot tell.

At the very least, the condition should be changed to bytes_read == max_size.

[akosthekiss]

(For the records: neither main-unix.c nor main-unix-minimal.c try to do anything - e.g., signalling errors - with truncated input files.)

[zherczeg]

(My note: I think truncating source files is less harmful than truncating snapshots)

[akosthekiss]

this is still a printf, while the rest of the diagnostics is via jerry_port_log

[zherczeg]

There is no suitable type for this in port log.

[akosthekiss]

make it a const

[zherczeg]

I don't think it matters since constant propagation eliminates it. But lets do this.

[akosthekiss]

inconsistent error message. elsewhere, it always starts with "Error: "

[akosthekiss]

Inconsistent error message. Elsewhere, it always starts with "Error: "

Additonally, once the error is printed, the function should return with FAIL code, not OK.

[akosthekiss]

This result should most probably be released. Travis logs show that the unit test generated from this example fails, I suspect it happens at cleanup because of unreleased values. Please debug it and make all tests pass.

[akosthekiss]

typo: message instead of nessage (likewise below)

[akosthekiss]

be consistent with existing code, use --jerry-cmdline-snapshot=on. (might even be erroneous to use a space here, as the list should contain command line arguments already separated.)

[zherczeg]

Thank you for the review. Changes done.

[akosthekiss]

Travis CI is still signalling error. Please fix it.

[zherczeg]

Done. Was a style issue.

[akosthekiss]

Did a final sweep through the code. Spotted one minor fault.

[akosthekiss]

typo, just spotted: inidicies -> indices (2 extra 'i's) (likewise throughout the file)

[zherczeg]

Somehow the JRRY didn't get through. Now that is fixed as well.

[akosthekiss]

lo and behold: LGTM

[martijnthe]

🎉