618852 - Add tools support for the "float" type

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Tools, enhancement)

Description

[Adam Welc]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
Build Identifier: 

Modifications to the existing tools (abcasm, abcdump) are required to support the "float" type.

Reproducible: Always

Comment 1

[Virgil Palanciuc]

Attachment: update abcasm + abcdump to support float

This is mostly Adam's implementation, that was slightly changed/updated by me. Needs a review to make sure it is in sync with the latest spec.

Comment 2

[Virgil Palanciuc]

Attachment: implementation for float&float4 in abcdump, abcasm

I think this could even land... if it weren't for two issues :)
1. opcodes-table.as is autogenerated from opcodes.tbl, so landing it would make it out-of-sync with current (mainline) opcodes.tbl
2. I think that the "magic number" (ABC version) that enables float support might change from 47|15 to 47|16, right?

Comment 3

[Virgil Palanciuc]

Attachment: implementation for float&float4 in abcdump, abcasm

Oops, forgot to add a file to the patch (Float4.java).

Comment 4

[Lars T Hansen]

Comment on attachment 560874 [details] [diff] [review]
implementation for float&float4 in abcdump, abcasm

The patch is mostly fine.

Major objection: This can't land until we've signed off on the ABC extensions spec (it's a little unclear who "we" are but you could assume myself and Edwin).  That spec was last updated on 7 July and looks anything but complete, in particular there's no mention of a float4 pool, no unminus instruction, etc.  Cleaning it up presumably won't be a major effort but is a prerequisite.

We cannot merge the test cases into arithmetic.abs like you have here, since the new test cases are only for new ABC versions and we may have to version the behavior generally speaking as we change ABC versions.  I suggest you move the new test cases into a file called eg arithmetic_47_16.abs to indicate tests that become introduced in that version (and a comment to that effect in the file will be helpful).

The test cases offered here are certainly insufficient, so we need to open a separate bug to flesh out the test cases for the new VM behavior.  This bug can block that one; that bug blocks the float work.

As you say, the version number needs to be 47.16 not 47.15 everywhere.

Style: Prevailing style for Java and AS3 code is indenting four spaces (Float4.java, abcdump.as).

Style: This condition nest in abcdump.as is a mess and has to be rewritten:

  if (magic != (46<<16|14) && magic != (46<<16|15) && magic != (46<<16|16) && magic != (47<<16|12)
              && magic != (47<<16|13)  && magic != (47<<16|14)  && magic != (47<<16|15))

Nit: it's not desirable to put jar files in the patch for review, at most it should be attached as separate non-reviewed patch.

Comment 5

[Lars T Hansen]

(In reply to Virgil Palanciuc from comment #2)

> 1. opcodes-table.as is autogenerated from opcodes.tbl, so landing it would
> make it out-of-sync with current (mainline) opcodes.tbl

Yes.  But once the ABC extensions spec is done, can we not land opcodes.tbl before we land everything else, so that we can land the abcasm/abcdump changes too?  These are all new opcodes so no content uses them, the verifier rejects them, etc - will we go wrong anywhere?

In opcodes.tbl we would want to have comments on all of the new instructions about the version in which they were introduced.

Comment 6

[Lars T Hansen]

Comment on attachment 560874 [details] [diff] [review]
implementation for float&float4 in abcdump, abcasm

R- for the test cases, by and large, and for the incorrect version number.

Also missing prerequisites: ABC extensions spec; opcodes.tbl update.

Comment 7

[Lars T Hansen]

These comments are relative to the patch in the patch queue as of 28 October.

- abcasm needs to accept the 'F' suffix as well as the 'f' suffix

- It would be good to have one test case for the 'F' suffix

- It should be possible to force abcasm to emit 47.16 bytecode even if the
  program does not use float, this will be handy for testing.  In fact it
  should be possible to ask for /any/ version number, in order to do testing;
  all abcasm needs to do is emit the right sections depending on the version
  number.  A command line switch and a little logic will do.  It may be
  adequate to fix that as a follow-on patch to the one we already have, but
  it does need to be fixed eventually.

(Consider this an R+ for the patch; the above are niggly details.)

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.