This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [WebAssembly] Skeleton support


Hi Pip,

  Thanks for the patch.  Here are some comments:

> +++ b/bfd/ChangeLog
> @@ -1,3 +1,18 @@
> +2017-03-20  Pip Cet  <pipcet@gmail.com>
> +
> +    * Makefile.am: Add WebAssembly architecture.
> +    * configure.ac: Likewise.
> +    * targets.c: Likewise.
> +    * config.bfd (targ_cpu): Likewise.
> +    * archures.c: Likewise.
> +    * cpu-wasm32.c: New file to support WebAssembly architecture.
> +    * elf32-wasm32.c: Likewise.
> +    * wasm-module.c: Initial backend for WebAssembly modules.
> +    * doc/webassembly.texi: Start documentation for wasm-module.c.
> +    * bfd-in2.h: Regenerate.
> +    * Makefile.in: Regenerate.
> +    * configure: Regenerate.

It is often easier (for us) if changelog entries are not included in
the patch, but instead are just part of the accompanying email.  This 
is because they almost never apply when patching the sources, and so 
they always have to be added by hand.

> diff --git a/bfd/Makefile.am b/bfd/Makefile.am

You missed out some source files.  You need to add elf32-wasm and wasm-module to
the BFD32_BACKENDS and BFD32_BACKENDS_CFILES definitions.


> diff --git a/bfd/Makefile.in b/bfd/Makefile.in

You can save space in a patch by leaving out generated files, such as
Makefile.in.  We always regenerate them ourselves (just to be sure that
the regeneration works properly).

> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 59403af698..432caafd32 100644

Likewise for this auto-generated header file...


> diff --git a/bfd/cpu-wasm32.c b/bfd/cpu-wasm32.c
> new file mode 100644
> index 0000000000..929778d531
> --- /dev/null
> +++ b/bfd/cpu-wasm32.c
> @@ -0,0 +1,36 @@
> +/* BFD support for the WebAssembly target
> +   Copyright (C) 1994-2017 Free Software Foundation, Inc.

Given that this file is brand new, it is hard to see how the FSF can claim
copyright from 1994...


> +static const bfd_arch_info_type arch_info_struct[] =
> +{
> +  N (bfd_mach_wasm32,      "wasm32",   TRUE, NULL)
> +};

Not serious - but there is some extraneous whitespace here.

> +#define ELF_TARGET_ID        0x4157 /* 'WA' */
> +#define ELF_MACHINE_CODE    0x4157 /* 'WA' */

You could use the value EM_WEBASSEMBLY here.  It is defined in include/elf/common.h.

> +++ b/bfd/targets.c
> @@ -893,6 +893,8 @@ extern const bfd_target vax_aout_nbsd_vec;
>  extern const bfd_target vax_elf32_vec;
>  extern const bfd_target visium_elf32_vec;
>  extern const bfd_target w65_coff_vec;
> +extern const bfd_target wasm_vec;
> +extern const bfd_target wasm32_elf32_vec;
>  extern const bfd_target we32k_coff_vec;
>  extern const bfd_target x86_64_coff_vec;
>  extern const bfd_target x86_64_elf32_vec;

You missed out a required patch to this file.  The _bfd_target_vector
array (starting at line 940) needs to have the new wasm vectors added to it.
To see why, try building a set of the binutils sources configured as:

  --enable-64-bit-bfd --enable-targets=all



> diff --git a/bfd/wasm-module.c b/bfd/wasm-module.c

> +/* From elf-eh-frame.c: */
> +/* If *ITER hasn't reached END yet, read the next byte into *RESULT and
> +   move onto the next byte.  Return true on success.  */
> +
> +static inline bfd_boolean
> +read_byte (bfd_byte **iter, bfd_byte *end, unsigned char *result)

Given that you are duplicating code that is already in another source file,
it might be better to just change those functions to non-static and use them
directly.  This avoids unnecessary code duplication...

> +static bfd_vma
> +wasm_get_uleb128 (bfd* abfd, bfd_boolean* error)

It is *very* helpful to have a comment before the start of a function,
explaining what it does.  (Well if it is a non-trivial function).  In
particular I wondered why you need a webasm specific ULEB128 reading
function.  Does webasm use non-standard ULEB128 values, or is there
something else going on ?

Given that there are already LEB128 reading functions in libbfd.c, maybe
they could be used instead ?


> +static bfd_boolean
> +wasm_get_magic (bfd *abfd, bfd_boolean *errorptr)
> +{
> +  bfd_byte magic[4];
> +  if (bfd_bread (magic, (bfd_size_type) 4, abfd) != 4)

It is nice to have a blank line between the declaration of variables and
the start of the code.


> +    {
> +      if (bfd_get_error () != bfd_error_file_truncated)
> +        *errorptr = TRUE;
> +      return FALSE;
> +    }

Why is it not an error if the read failed for some reason other than file
truncation ?


> +
> +  if (magic[0] != 0 ||
> +      magic[1] != 'a' ||
> +      magic[2] != 's' ||
> +      magic[3] != 'm')

Formatting: Please split lines before operators, not after them.  See:

  https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting

> +static int
> +wasm_get_byte (bfd *abfd, bfd_boolean *errorptr)
> +{
> +  bfd_byte byte;
> +  if (bfd_bread (&byte, (bfd_size_type) 1, abfd) != 1)
> +    {
> +      if (bfd_get_error () != bfd_error_file_truncated)
> +        *errorptr = TRUE;
> +      return EOF;
> +    }

This particular piece of code is repeated several times in the wasm-module.c
source file.  Perhaps this is a suitable case for a macro or inline function ?


> +#define WASM_NUMBERED_SECTIONS 11

This definition and the strings used in the next few functions would be better
off being replaced by a static array, and using the ARRAY_SIZE macro defined in
libiberty.h header.  A simple for() loop can then be used to iterate over the
table and if/when new sections are added it will only require changing one line
in the sources.

> +static bfd_boolean
> +bfd_wasm_read_header (bfd *abfd, bfd_boolean *error)
> +{
> +  if (!wasm_get_magic (abfd, error))
> +    goto error_return;
> +
> +  if (!wasm_get_version (abfd, error))
> +    goto error_return;
> +
> +  return TRUE;
> +
> + error_return:
> +  return FALSE;
> +}

The goto's could just be replaced with "return FALSE"...

> +static bfd_boolean
> +wasm_scan_name_function_section (bfd *abfd, sec_ptr asect,
> +                                 void *data ATTRIBUTE_UNUSED)
> +{
> +  if (!asect)
> +    return FALSE;
> +
> +  if (strcmp (asect->name, ".wasm.name") != 0)
> +    return FALSE;
> +
> +  bfd_byte *p = asect->contents;
> +  bfd_byte *end = asect->contents + asect->size;

Please move variable declarations to the start of the block.  (So
that the sources can be built with any ISO-C compliant C compiler,
not just gcc).

> +  while (p && p < end)
> +    {
> +      if (*p++ == 1)
> +        break;
> +      bfd_vma payload_size;
> +      if (!read_uleb128 (&p, end, &payload_size))
> +        return FALSE;
> +
> +      p += payload_size;
> +    }

A very large value for payload_size could result in p wrapping round
to before asect->contents.  Is there a maximum known payload size ?
If so then you ought to check for it here.  Otherwise check that:
p < p + payload_size < end


> +  bfd_vma payload_size;

You are declaring payload_size twice in this function!

> +  sec_ptr space_function_index = bfd_make_section_with_flags (abfd,
> ".space.function_index", SEC_READONLY | SEC_CODE);
> +  if (!space_function_index)
> +    space_function_index = bfd_get_section_by_name (abfd,
> ".space.function_index");

Constant strings like this are best #define'd in a header file (if they
are going to be used in more than one source file), or at the start of
the file (if they are local).

> +      name = bfd_alloc (abfd, len + 1);
> +
> +      name[len] = 0;

Paranoia: bfd_alloc() can return NULL...

> +static bfd_boolean
> +wasm_compute_section_file_positions (bfd *abfd)
> +{
> +  bfd_byte magic[] = { 0x00, 'a', 's', 'm' };

Magic byte values should definitely be defined in a header.

> +  bfd_byte vers[] = { 0x01, 0x00, 0x00, 0x00 };

Magic version numbers likewise...



Phew - well that is it for a first pass.  I hope that these comments were helpful.

Cheers
  Nick



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]