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 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


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