fix: sourcemap generation for call instruction #5504

JesseCodeBones · 2023-02-20T08:27:31Z

kripken

I took at look at the repo you provided (thanks!), but the testcase is large - looks like it uses all of Binaryen? Instead can you make a small testcase that we can also add to the test suite here?

I made a quick attempt at a lit test now:

;; RUN: wasm-opt %s -o %t.wasm -osm %t.map -q
;; RUN: wasm-opt %t.wasm -ism %t.map -q -o - -S | filecheck %s

(module
  (func $foo (param $x i32) (param $y i32)
    ;;@ src.cpp:10:1
    (if
      ;;@ src.cpp:20:1
      (i32.add
        ;;@ src.cpp:30:1
        (local.get $x)
        ;;@ src.cpp:40:1
        (local.get $y)
      )
      ;;@ src.cpp:50:1
      (return)
    )
    ;;@ src.cpp:60:1
    (call $foo
      ;;@ src.cpp:70:1
      (local.get $x)
      ;;@ src.cpp:80:1
      (local.get $y)
    )
  )
)

;; CHECK: (module
;; CHECK-NEXT:  (func $0 (param $0 i32) (param $1 i32)
;; CHECK-NEXT: 
;; CHECK-NEXT:   ;;@ src.cpp:40:1
;; CHECK-NEXT:   (if
;; CHECK-NEXT:    (i32.add
;; CHECK-NEXT:     ;;@ src.cpp:30:1
;; CHECK-NEXT:     (local.get $0)
;; CHECK-NEXT:     ;;@ src.cpp:40:1
;; CHECK-NEXT:     (local.get $1)
;; CHECK-NEXT:    )
;; CHECK-NEXT:    ;;@ src.cpp:50:1
;; CHECK-NEXT:    (return)
;; CHECK-NEXT:   )
;; CHECK-NEXT:   ;;@ src.cpp:80:1
;; CHECK-NEXT:   (call $0
;; CHECK-NEXT:    ;;@ src.cpp:70:1
;; CHECK-NEXT:    (local.get $0)
;; CHECK-NEXT:    ;;@ src.cpp:80:1
;; CHECK-NEXT:    (local.get $1)
;; CHECK-NEXT:   )

Something does indeed look wrong with the call. But also with the add... so I'm not sure what's going on.

kripken · 2023-02-21T20:24:20Z

src/wasm-stack.h

-  emitDebugLocation(curr);
+  if (!curr->is<Call>()) {
+    emitDebugLocation(curr);
+  }


Please add a comment explaining why Call is special here. That's not obvious to me, sorry if I didn't understand something you already explained in the issue.

In particular, why is Call different from, say, Binary? And what about CallIndirect and CallRef?

JesseCodeBones · 2023-02-22T02:12:26Z

yeah, it seems affact all the misordered instructions, like if call and etc.
the misorder means, it will be first looped by IR tree walker (source map generation), but latest with wasm binary parser (sourcemap reader).
for example:

tree IR:

(call add
  i32.const 1
  i32.const 2
)

binary parser:

i32.const 1
i32.const 2
call add

it is better to combine the debug information generation with binary generation, ie emitDebugLocation and emit,I will have a test, what do you think?

JesseCodeBones · 2023-02-22T06:19:50Z

@kripken
Hi Alon,
for the fixed test, I found the position for the fixed file is also incorrect, I changed it.
here is the detail:

source map decode

moBAEA:
643
0
2
0
...........
4BAKA:
28
0
5
0
...........
QAJA:
8
0
-4
0
...........
OADA:
7
0
-1
0
...........
OAAA:
7
0
0
0
...........
uCAKA:
39
0
5
0
...........

wasm decode

...
 0002d5: 21 01                      |         local.set 1
 0002d7: 0b                         |       end
 0002d8: 0c 01                      |       br 1
 0002da: 0b                         |     end
 0002db: 0b                         |   end
 0002dc: 20 04                      |   local.get 4
 0002de: 0f                         |   return
 0002df: 00                         |   unreachable
 0002e0: 00                         |   unreachable
 0002e1: 0b                         | end
 0002e2: 00                         | unreachable
 0002e3: 0b                         | end

according to source map position, the latest postion of wasm should be:
643+28+8+7+7+39 = 0x2DC
so it should be attached to position:
0002dc: 20 04 | local.get 4

kripken

Nice! This looks much better. I also see the results on the lit testcase from my comment look better with this patch applied (still not perfect, but that can be left for later, after this PR).

I think it would be good to get that lit test into the test suite. Here is an updated version with your patch:

;; RUN: wasm-opt %s -o %t.wasm -osm %t.map -g -q
;; RUN: wasm-opt %t.wasm -ism %t.map -q -o - -S | filecheck %s

(module
  (func $foo (param $x i32) (param $y i32)
    ;;@ src.cpp:10:1
    (if
      ;;@ src.cpp:20:1
      (i32.add
        ;;@ src.cpp:30:1
        (local.get $x)
        ;;@ src.cpp:40:1
        (local.get $y)
      )
      ;;@ src.cpp:50:1
      (return)
    )
    ;;@ src.cpp:60:1
    (call $foo
      ;;@ src.cpp:70:1
      (local.get $x)
      ;;@ src.cpp:80:1
      (local.get $y)
    )
  )
)

;; CHECK:  (func $foo (param $x i32) (param $y i32)
;; CHECK-NEXT:   ;;@ src.cpp:20:1
;; CHECK-NEXT:   (if
;; CHECK-NEXT:    (i32.add
;; CHECK-NEXT:     ;;@ src.cpp:30:1
;; CHECK-NEXT:     (local.get $x)
;; CHECK-NEXT:     ;;@ src.cpp:40:1
;; CHECK-NEXT:     (local.get $y)
;; CHECK-NEXT:    )
;; CHECK-NEXT:    ;;@ src.cpp:50:1
;; CHECK-NEXT:    (return)
;; CHECK-NEXT:   )
;; CHECK-NEXT:   ;;@ src.cpp:60:1
;; CHECK-NEXT:   (call $foo
;; CHECK-NEXT:    ;;@ src.cpp:70:1
;; CHECK-NEXT:    (local.get $x)
;; CHECK-NEXT:    ;;@ src.cpp:80:1
;; CHECK-NEXT:    (local.get $y)
;; CHECK-NEXT:   )

Please add it in tests/lit/source-map.wast.

kripken · 2023-02-22T17:10:17Z

src/wasm-binary.h

@@ -1430,7 +1430,7 @@ class WasmBinaryBuilder {
  MixedArena& allocator;
  const std::vector<char>& input;
  std::istream* sourceMap;
-  std::pair<uint32_t, Function::DebugLocation> nextDebugLocation;
+  std::tuple<uint32_t, uint32_t, Function::DebugLocation> nextDebugLocation; // available pos, previous pos, next debug location


I think it would be good to use a struct here. Something like

struct NextDebugLocation { uint32_t available; uint32_t previous; uint32_t next; };

Hi Alon,
I updated the fix according to your comments, and for the next step (add lit test to main test suite), I donot know how to begin the work, can you give some advice? thank you.

For the test, just create a new file at test/lit/source-map.wast, paste the test contents from here into that file, and add it using git add, then commit it. It should just work. (edit: test not tests)

The lit test just passed as well:

/binaryen$ python build/bin/binaryen-lit -vv test/lit/source-map.wast -- Testing: 1 tests, 1 workers -- PASS: Binaryen lit tests :: source-map.wast (1 of 1) Testing Time: 0.06s Passed: 1

Hi Alon,
the test has been added, please kindly review, thank you.

kripken · 2023-02-22T17:34:18Z

I verified that this patch passes the emscripten source map tests as well.

kripken · 2023-02-23T16:35:45Z

Thanks! I'm still reading the code, but to fix the CI error please apply the style diff from here:

https://github.com/WebAssembly/binaryen/actions/runs/4249525050/jobs/7400946814

kripken

lgtm aside from the code style that needs to be fixed.

JesseCodeBones · 2023-02-24T03:49:37Z

lgtm aside from the code style that needs to be fixed.

fixed it now:)

kripken

Great, thanks!

…5504

With this, the sourcemap testcase outputs the exact same thing as the input. Followup to #5504

The stack logic was incorrect, and led to source locations being emitted on parents instead of children.

…5524) With this, the sourcemap testcase outputs the exact same thing as the input. Followup to WebAssembly#5504

kripken reviewed Feb 21, 2023

View reviewed changes

fix: sourcemap generation for call instruction

8f04946

JesseCodeBones force-pushed the instruction.call.sourcemap branch from 99edfa6 to 8f04946 Compare February 22, 2023 04:10

fix: optimize border check of source map

904694a

kripken reviewed Feb 22, 2023

View reviewed changes

JesseCodeBones force-pushed the instruction.call.sourcemap branch from 9f6268c to 191b42b Compare February 23, 2023 04:48

kripken approved these changes Feb 23, 2023

View reviewed changes

fix: source map test issue and refactor the code

d649bee

JesseCodeBones force-pushed the instruction.call.sourcemap branch from 191b42b to d649bee Compare February 24, 2023 03:42

kripken approved these changes Feb 24, 2023

View reviewed changes

kripken merged commit 5385069 into WebAssembly:main Feb 24, 2023

kripken added a commit that referenced this pull request Feb 24, 2023

Emit source map information for control flow structures. Followup to #…

8e0d92f

…5504

kripken mentioned this pull request Feb 24, 2023

Emit source map information for control flow structures #5524

Merged

kripken added a commit that referenced this pull request Feb 28, 2023

Emit source map information for control flow structures (#5524)

62e0b2f

With this, the sourcemap testcase outputs the exact same thing as the input. Followup to #5504

JesseCodeBones mentioned this pull request Mar 6, 2023

Fix: remove excess optimization for sourcemap #5547

Closed

aheejin mentioned this pull request Aug 31, 2023

[NFC] Source maps: Simplify the code and add comments #5912

Merged

radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024

Fix sourcemap nesting in reading and writing (WebAssembly#5504)

1b74d4c

The stack logic was incorrect, and led to source locations being emitted on parents instead of children.

radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024

Emit source map information for control flow structures (WebAssembly#…

92efc2a

…5524) With this, the sourcemap testcase outputs the exact same thing as the input. Followup to WebAssembly#5504

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sourcemap generation for call instruction #5504

fix: sourcemap generation for call instruction #5504

JesseCodeBones commented Feb 20, 2023 •

edited

Loading

kripken left a comment

kripken Feb 21, 2023

JesseCodeBones commented Feb 22, 2023 •

edited

Loading

JesseCodeBones commented Feb 22, 2023

kripken left a comment •

edited

Loading

kripken Feb 22, 2023

JesseCodeBones Feb 23, 2023

kripken Feb 23, 2023 •

edited

Loading

JesseCodeBones Feb 23, 2023

JesseCodeBones Feb 23, 2023

kripken commented Feb 22, 2023

kripken commented Feb 23, 2023

kripken left a comment

JesseCodeBones commented Feb 24, 2023

kripken left a comment

fix: sourcemap generation for call instruction #5504

fix: sourcemap generation for call instruction #5504

Conversation

JesseCodeBones commented Feb 20, 2023 • edited Loading

kripken left a comment

Choose a reason for hiding this comment

kripken Feb 21, 2023

Choose a reason for hiding this comment

JesseCodeBones commented Feb 22, 2023 • edited Loading

tree IR:

binary parser:

JesseCodeBones commented Feb 22, 2023

source map decode

wasm decode

kripken left a comment • edited Loading

Choose a reason for hiding this comment

kripken Feb 22, 2023

Choose a reason for hiding this comment

JesseCodeBones Feb 23, 2023

Choose a reason for hiding this comment

kripken Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

JesseCodeBones Feb 23, 2023

Choose a reason for hiding this comment

JesseCodeBones Feb 23, 2023

Choose a reason for hiding this comment

kripken commented Feb 22, 2023

kripken commented Feb 23, 2023

kripken left a comment

Choose a reason for hiding this comment

JesseCodeBones commented Feb 24, 2023

kripken left a comment

Choose a reason for hiding this comment

JesseCodeBones commented Feb 20, 2023 •

edited

Loading

JesseCodeBones commented Feb 22, 2023 •

edited

Loading

kripken left a comment •

edited

Loading

kripken Feb 23, 2023 •

edited

Loading