Re: [PATCH] [WebAssembly] Skeleton support

[Pip Cet <pipcet at gmail dot com>]

Okay, here's the first part of the changes, adding a new architecture,
wasm32, and minimal ELF support. I've left the date range in the
copyright notice for elf32-wasm32.c because future patches will add
substantial parts from other architectures' ELF support files; if that
needs changing, please let me know.

Any comments would be appreciated.
Thanks,
Pip

Suggested change log entries:
include/:
2017-03-22  Pip Cet  <pipcet@gmail.com>

    * elf/wasm32.h: New file to support wasm32 architecture.

bfd/:
2017-03-22  Pip Cet  <pipcet@gmail.com>
    * cpu-wasm32.c: New file to support wasm32 architecture.
    * elf32-wasm32.c: New file to support wasm32 architecture.
    * Makefile.am: Add wasm32 architecture.
    * archures.c: Likewise.
    * config.bfd: Likewise.
    * configure.ac: Likewise.
    * targets.c: Likewise.
-----

diff --git a/bfd/Makefile.am b/bfd/Makefile.am
index 0b0226306f..6fa8302020 100644
--- a/bfd/Makefile.am
+++ b/bfd/Makefile.am
@@ -169,6 +169,7 @@ ALL_MACHINES = \
     cpu-vax.lo \
     cpu-visium.lo \
     cpu-w65.lo \
+    cpu-wasm32.lo \
     cpu-we32k.lo \
     cpu-xc16x.lo \
     cpu-xgate.lo \
@@ -257,6 +258,7 @@ ALL_MACHINES_CFILES = \
     cpu-v850_rh850.c \
     cpu-vax.c \
     cpu-visium.c \
+    cpu-wasm32.c \
     cpu-w65.c \
     cpu-we32k.c \
     cpu-xc16x.c \
@@ -383,6 +385,7 @@ BFD32_BACKENDS = \
     elf32-v850.lo \
     elf32-vax.lo \
     elf32-visium.lo \
+    elf32-wasm32.lo \
     elf32-xc16x.lo \
     elf32-xgate.lo \
     elf32-xstormy16.lo \
@@ -576,6 +579,7 @@ BFD32_BACKENDS_CFILES = \
     elf32-v850.c \
     elf32-vax.c \
     elf32-visium.c \
+    elf32-wasm32.c \
     elf32-xc16x.c \
     elf32-xgate.c \
     elf32-xstormy16.c \
diff --git a/bfd/archures.c b/bfd/archures.c
index c909db012d..c6e7152057 100644
--- a/bfd/archures.c
+++ b/bfd/archures.c
@@ -528,6 +528,8 @@ DESCRIPTION
 .#define bfd_mach_nios2r2    2
 .  bfd_arch_visium,    {* Visium *}
 .#define bfd_mach_visium    1
+.  bfd_arch_wasm32,     {* WebAssembly *}
+.#define bfd_mach_wasm32        1
 .  bfd_arch_pru,    {* PRU *}
 .#define bfd_mach_pru    0
 .  bfd_arch_last
@@ -654,6 +656,7 @@ extern const bfd_arch_info_type bfd_v850_arch;
 extern const bfd_arch_info_type bfd_v850_rh850_arch;
 extern const bfd_arch_info_type bfd_vax_arch;
 extern const bfd_arch_info_type bfd_visium_arch;
+extern const bfd_arch_info_type bfd_wasm32_arch;
 extern const bfd_arch_info_type bfd_w65_arch;
 extern const bfd_arch_info_type bfd_we32k_arch;
 extern const bfd_arch_info_type bfd_xstormy16_arch;
@@ -746,6 +749,7 @@ static const bfd_arch_info_type * const
bfd_archures_list[] =
     &bfd_vax_arch,
     &bfd_visium_arch,
     &bfd_w65_arch,
+    &bfd_wasm32_arch,
     &bfd_we32k_arch,
     &bfd_xstormy16_arch,
     &bfd_xtensa_arch,
diff --git a/bfd/config.bfd b/bfd/config.bfd
index 52db9a47f1..abcb7ae210 100644
--- a/bfd/config.bfd
+++ b/bfd/config.bfd
@@ -197,6 +197,7 @@ tilegx*)     targ_archs=bfd_tilegx_arch ;;
 tilepro*)     targ_archs=bfd_tilepro_arch ;;
 v850*)         targ_archs="bfd_v850_arch bfd_v850_rh850_arch" ;;
 visium*)     targ_archs=bfd_visium_arch ;;
+wasm32)         targ_archs=bfd_wasm32_arch ;;
 x86_64*)     targ_archs=bfd_i386_arch ;;
 xtensa*)     targ_archs=bfd_xtensa_arch ;;
 xgate)         targ_archs=bfd_xgate_arch ;;
@@ -1793,6 +1794,10 @@ case "${targ}" in
     targ_defvec=visium_elf32_vec
     ;;

+  wasm32-*-*)
+    targ_defvec=wasm32_elf32_vec
+    ;;
+
   we32k-*-*)
     targ_defvec=we32k_coff_vec
     ;;
diff --git a/bfd/configure.ac b/bfd/configure.ac
index ee0c537ea2..77961945d1 100644
--- a/bfd/configure.ac
+++ b/bfd/configure.ac
@@ -700,6 +700,7 @@ do
     ft32_elf32_vec)         tb="$tb elf32-ft32.lo elf32.lo $elf" ;;
     visium_elf32_vec)         tb="$tb elf32-visium.lo elf32.lo $elf" ;;
     w65_coff_vec)         tb="$tb coff-w65.lo reloc16.lo $coffgen" ;;
+    wasm32_elf32_vec)            tb="$tb elf32-wasm32.lo elf32.lo $elf" ;;
     we32k_coff_vec)         tb="$tb coff-we32k.lo $coffgen" ;;
     x86_64_coff_vec)         tb="$tb coff-x86_64.lo $coff"; target_size=64 ;;
     x86_64_elf32_vec)         tb="$tb elf64-x86-64.lo elf-ifunc.lo
elf-nacl.lo elf64.lo elf32.lo $elf"; target_size=64 ;;
diff --git a/bfd/cpu-wasm32.c b/bfd/cpu-wasm32.c
new file mode 100644
index 0000000000..19d4cb9270
--- /dev/null
+++ b/bfd/cpu-wasm32.c
@@ -0,0 +1,36 @@
+/* BFD support for the WebAssembly target
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of BFD, the Binary File Descriptor library.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+   MA 02110-1301, USA.  */
+
+#include "sysdep.h"
+#include "bfd.h"
+#include "libbfd.h"
+#include "libiberty.h"
+
+#define N(number, print, default, next)  \
+{  32, 32, 8, bfd_arch_wasm32, number, "wasm32", "wasm32", 4,
default, bfd_default_compatible, \
+   bfd_default_scan, bfd_arch_default_fill, next }
+
+static const bfd_arch_info_type arch_info_struct[] =
+{
+  N (bfd_mach_wasm32, "wasm32", TRUE, NULL)
+};
+
+const bfd_arch_info_type bfd_wasm32_arch =
+  N (bfd_mach_wasm32, "wasm32", TRUE, & arch_info_struct[0]);
diff --git a/bfd/elf32-wasm32.c b/bfd/elf32-wasm32.c
new file mode 100644
index 0000000000..2088146cc4
--- /dev/null
+++ b/bfd/elf32-wasm32.c
@@ -0,0 +1,52 @@
+/* 32-bit ELF for the WebAssembly target
+   Copyright (C) 1999-2017 Free Software Foundation, Inc.
+
+   This file is part of BFD, the Binary File Descriptor library.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin Street - Fifth Floor,
+   Boston, MA 02110-1301, USA.  */
+
+#include "sysdep.h"
+#include "bfd.h"
+#include "libbfd.h"
+#include "elf-bfd.h"
+#include "bfd_stdint.h"
+#include "elf/wasm32.h"
+
+#define ELF_ARCH        bfd_arch_wasm32
+#define ELF_TARGET_ID        EM_WEBASSEMBLY
+#define ELF_MACHINE_CODE    EM_WEBASSEMBLY
+/* FIXME we don't have paged executables, see
+ * https://github.com/pipcet/binutils-gdb/issues/4 */
+#define ELF_MAXPAGESIZE        4096
+
+#define TARGET_LITTLE_SYM       wasm32_elf32_vec
+#define TARGET_LITTLE_NAME    "elf32-wasm32"
+
+#define elf_backend_can_gc_sections          1
+#define elf_backend_rela_normal              1
+/* For testing. */
+#define elf_backend_want_dynrelro            1
+
+#define bfd_elf32_bfd_reloc_type_lookup _bfd_norelocs_bfd_reloc_type_lookup
+#define bfd_elf32_bfd_reloc_name_lookup _bfd_norelocs_bfd_reloc_name_lookup
+
+#define ELF_DYNAMIC_INTERPRETER  "/sbin/elf-dynamic-interpreter.so"
+
+#define elf_backend_want_got_plt 1
+#define elf_backend_plt_readonly 1
+#define elf_backend_got_header_size 0
+
+#include "elf32-target.h"
diff --git a/bfd/targets.c b/bfd/targets.c
index 1a7c6b87d6..74559ac4c6 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -893,6 +893,7 @@ 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 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;
@@ -1421,6 +1422,8 @@ static const bfd_target * const _bfd_target_vector[] =

     &w65_coff_vec,

+        &wasm32_elf32_vec,
+
     &we32k_coff_vec,

 #ifdef BFD64
diff --git a/include/elf/wasm32.h b/include/elf/wasm32.h
new file mode 100644
index 0000000000..38e6c2e950
--- /dev/null
+++ b/include/elf/wasm32.h
@@ -0,0 +1,28 @@
+/* ELF support for BFD for the WebAssembly target
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#ifndef _ELF_WASM32_H
+#define _ELF_WASM32_H
+
+#include "elf/reloc-macros.h"
+
+/* Relocation types.  */
+
+START_RELOC_NUMBERS (elf_wasm32_reloc_type)
+END_RELOC_NUMBERS (R_WASM32_max = 1)
+
+#endif /* _ELF_WASM32_H */

On Wed, Mar 22, 2017 at 4:11 PM, Pip Cet <pipcet@gmail.com> wrote:
> Hi Nick,
> thank you very much for your comments. If it's okay, I shall split the
> patch into two parts (one for the skeleton code, one for wasm-module.c
> plus the new header file), address your comments, and try again.
>
> On Wed, Mar 22, 2017 at 12:55 PM, Nick Clifton <nickc@redhat.com> wrote:
>> 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.
>
> Thanks, noted.
>
>>> 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...
>
> You're right; it is based on an older file, but has been replaced entirely.
>
>>> +#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
>
> Thanks!
>
>>> 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...
>
> Yeah, the situation for the LEB128 code in particular is a bit diffuse
> at the moment: by my count, there are eleven files (including gdb and
> gold in the count) defining at least one LEB128 function, with
> different calling conventions, return types, and error behaviour. I'd
> like to remedy that in a future patch, but for now the risk of
> namespace collisions convinced me not to export the ELF functions.
>
>>> +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).
>
> Thanks, I shall add those.
>
>>  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 ?
>
> The problem is that WebAssembly doesn't consider it necessary to tell
> you in advance how much data there is to be read, so EOF needs to be
> handled carefully. (The other deviation is that WebAssembly allows
> overlong LEB128 representations, but only if the number of bytes
> doesn't exceed the maximum for the integer size (5 bytes for 32 bits,
> usually)). Now that I write it down this should definitely go into
> separate functions...
>
>> Given that there are already LEB128 reading functions in libbfd.c, maybe
>> they could be used instead ?
>
> I'll give that another try.
>
>>> +    {
>>> +      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 ?
>
> I may be misreading the code; the intention is that *errorptr is set
> to TRUE, indicating an error, for all reasons except for file
> truncation, which indicates EOF, which is not an error if we're done
> with the last section.
>
>>> +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 ?
>
> Yes, I'll make it one.
>
>>> +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).
>
> I'll add -Wdeclaration-after-statement to my C flags. Perhaps this
> should be added to warning.m4?
>
> (I tried that, and there are a number of places that trigger the
> warning; I fixed them, mostly to check that there were no actual bugs,
> and things build cleanly with the warning now. Should I submit a patch
> for that?)
>
>>> +  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 ?
>
> Thanks for spotting that!
>
>> If so then you ought to check for it here.  Otherwise check that:
>> p < p + payload_size < end
>
> Will do.
>
>>> +  bfd_vma payload_size;
>>
>> You are declaring payload_size twice in this function!
>
> Oops, sorry about that.
>
>> Phew - well that is it for a first pass.  I hope that these comments were helpful.
>
> They are, absolutely. Thanks again, and sorry for causing you so much work,
> Pip

Attachment: binutils-wasm-003.diff
Description: Text document