From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97965 invoked by alias); 22 Mar 2017 12:55:18 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 97886 invoked by uid 89); 22 Mar 2017 12:55:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=*very*, paranoia X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Mar 2017 12:55:15 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B7B611556C; Wed, 22 Mar 2017 12:55:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B7B611556C Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=nickc@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B7B611556C Received: from [10.36.117.73] (ovpn-117-73.ams2.redhat.com [10.36.117.73]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F3F7A7D85F; Wed, 22 Mar 2017 12:55:14 +0000 (UTC) Subject: Re: [PATCH] [WebAssembly] Skeleton support To: Pip Cet , binutils@sourceware.org References: From: Nick Clifton Message-ID: <6ba7849d-2475-587d-d4a9-6e67c7df550b@redhat.com> Date: Wed, 22 Mar 2017 12:55:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00300.txt.bz2 Hi Pip, Thanks for the patch. Here are some comments: > +++ b/bfd/ChangeLog > @@ -1,3 +1,18 @@ > +2017-03-20 Pip Cet > + > + * 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