Closed Bug 1571230 Opened 1 year ago Closed 1 year ago

WasmTextToBinary: does not accept a infinity value

Categories

(Core :: Javascript: WebAssembly, defect, P5, minor)

defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: volker.berlin, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36

Steps to reproduce:

I want to use the follow text syntax with the function WasmTextToBinary to push an infinity value on the stack:

f64.const inf

This syntax is working with the Wabt tool.

Actual results:

I receive the error message:
SyntaxError: wasm text error: parsing wasm text at 693:17

Thanks for filing a bug: the correct token is "infinity", not "inf". It's trivial to accept it too, so I'll make a patch.

Assignee: nobody → bbouvier
Blocks: 1527871
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Severity: normal → minor
Priority: -- → P5

WABT does not accept "infinity". This is the value that I had expect first.

Ah, that's right. Let's all keep "inf" then, and close this bug; unless WABT does otherwise any time soon.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX
Attachment #9082989 - Attachment is obsolete: true

The fix does not work for me. I have test https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/jsshell-win64.zip from 08-Aug-2019 12:08.

I have also try the suggested "infinity" which is incompatible with WABT and it work. Look like your fix has no effect.

Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Oops, I think there was some miscommunication here: I've just not landed the fix, since you said that WABT didn't support it, and we try to be aligned with wabt in general.

So, just to be clear, are you saying that:

  • using inf doesn't work for you? (Can you use it, or is there a particular reason why you can't?)
  • or you tried to use infinity, but since we never landed the patch (for the reason that WABT doesn't implement it), it had not effect and didn't work?

Answering this will help us figure out if this is needed.

Flags: needinfo?(volker.berlin)

Sorry for my bad communication. What I have means:

  • First I have try infinity with WABT but it does not work. This seems the natural syntax for me. That I can understand that SpiderMonkey support it.
  • After asking the community I have try inf and this work with WABT without any problems. But it does not work with SpiderMonkey.

If both (WABT and SM) should be compatible then inf is the correct syntax. You can try it self in the online demo at https://webassembly.github.io/wabt/demo/wat2wasm/

Flags: needinfo?(volker.berlin)

Duh, right, I was thinking backwards, sorry (I thought inf was implemented but not infinity, while the reverse is true in Spidermonkey). Well, we can just reuse my previous patch, at least :)

Attachment #9082989 - Attachment is obsolete: false
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a2d23bd5015
Accept "inf" for infinity in wasmTextToBinary; r=jseward
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

It look like that you have only fix the syntax "+inf" and "-inf" (line 909) but not the simple syntax "inf" without sign in (line 2092).

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2841c9e7397
Allow inf in wasmTextToBinary without a sign prefix; r=jseward
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED

Works for me now. Thanks.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.