Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opcode for catch_all #147

Closed
dschuff opened this issue Feb 8, 2021 · 9 comments
Closed

Opcode for catch_all #147

dschuff opened this issue Feb 8, 2021 · 9 comments

Comments

@dschuff
Copy link
Member

dschuff commented Feb 8, 2021

The opcode for catch_all is currently specified to be 0x05. This is the same as else and it's technically unambiguous because it appears in the context of a try. But it's also the first reuse that breaks the current 1:1 correspondence between opcodes and operators; that correspondence complicates some tools and implementations, so it might not be worthwhile just to save the extra number. Should we choose an unused opcode instead? See also the discussion here

@RossTate
Copy link
Contributor

RossTate commented Feb 8, 2021

Nuanced thoughts:

For the binary format, we have a common end bytecode that is shared across all block-based operators. That's useful given the desire for small binaries. It seems that else might similarly be generalized to a mid marker to be shared across all multi-block-based operators for the same reasons.

That said, although try is multi-block, different kinds of secondary blocks have different behavior. catch_all is only one such kind of secondary block.

So I can see the justification for catch_all having the same encoding as else, but I feel like it's better suited to having its own.

@rossberg
Copy link
Member

rossberg commented Feb 9, 2021

Is this merely a problem of pretty-printing? The opcode 0x0b (end) is also generic for various constructs, which potentially have to perform different operations at this point. You could argue that 0x03 is in a similar class as a generic separator (which may or may fit here).

@wingo
Copy link
Contributor

wingo commented Feb 9, 2021

It's an inconvenience for error reporting in tooling; if you see a 0x03 you don't know whether to report a misplaced else or a misplaced catch_all. It's an inconvenience in one-pass compilers to have two possible success continuations after seeing 0x03 (one inside if, one inside try). Nothing insurmountable but if there are opcodes to spare, makes sense to me to allocate one.

@aheejin
Copy link
Member

aheejin commented Feb 9, 2021

I'd support splitting them if that makes tool implementation and error reporting easier, unless there is any reason not to. Are we running out of opcode space for future instructions or something? It seems we have 0x19 available (see #145).

Also, not sure if this is relevant, but that end ends all block-like structure does not hold anymore; try-delegate does not have an end at the end. (It could have an end, but given that delegate does not have a body it is not necessary.)

Also cc'ing other people working on implementations: @takikawa @thibaudmichaud

@takikawa
Copy link
Collaborator

takikawa commented Feb 9, 2021

I would prefer having it split as well unless there are good reasons against. Both in Spidermonkey and wabt, I've noticed some cases where error messages or printing can be confusing or more difficult to make clear with the opcode sharing. It would make sense to me to keep the 1:1 correspondence as much as possible.

@eqrion
Copy link
Contributor

eqrion commented Feb 10, 2021

The difference for me between end and catch_all is that the end opcode is always 'end' in the text format, no matter where it is used. Whereas the else/catch_all opcode would have a different text name depending on the context.

This leads me to think that it could be okay to share an opcode if we could agree on a common text name for the uses of the opcode. For backwards compatibility, this would likely be else.

e.g.

try
  ...
catch
  ...
else (* catch_all *)
  ...
end

I personally think re-using else for catch_all, as above, is unclear with regards to control flow. So I would agree that we should look at other opcodes.

@aheejin
Copy link
Member

aheejin commented Feb 11, 2021

@rossberg Are we running out of opcode spaces? If not, it seems the sentiments are it'd be less confusing to split the opcodes.

@rossberg
Copy link
Member

I'm fine with it. We won't run out because of one individual opcode (we currently have 40-50 opcodes left -- though if this sets the precedence for future constructs, then we will obviously use them up faster).

@aheejin
Copy link
Member

aheejin commented Feb 11, 2021

Then would it be OK with everyone if we make catch_all 0x19?

aheejin added a commit to llvm/llvm-project that referenced this issue Feb 17, 2021
We decided to change `catch_all`'s opcode from 0x05, which is the same
as `else`, to 0x19, to avoid some complicated handling in the tools.

See: WebAssembly/exception-handling#147

Reviewed By: sbc100

Differential Revision: https://reviews.llvm.org/D96863
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 17, 2021
We decided to change `catch_all`'s opcode from 0x05, which is the same
as `else`, to 0x19, to avoid some complicated handling in the tools.

See: WebAssembly/exception-handling#147

---

dwarf_with_exceptions.wasm was added in WebAssembly#3496 but we didn't comment on
how that file was created, so I'll add some comment on that here, in
case we need to regenerate this file with some modifications. I
regenerated dwarf_with_exceptions.wasm, so some variable names and such
have changed.

cpp file:
```cpp
void foo();

void test_debuginfo() {
  try {
    foo();
  } catch (...) {
    foo();
  }
}
```

Run:
```
$ clang++ -std=c++14 -stdlib=libc++ --target=wasm32-unknown-unknown -fwasm-exceptions -Xclang -disable-O0-optnone -c -S -emit-llvm test_debuginfo.cpp -o temp.ll
$ opt -S -mem2reg -simplifycfg temp.ll -o test_debuginfo.ll
```
(`opt -mem2reg -simplifycfg` was run to make the code little tidier.
This basically promotes stack loads/saves to registers and simplifies
the CFG.)

Resulting ll file, after modifying some function names and removing
personal info in directory names:
```llvm
source_filename = "test_debuginfo.cpp"
target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"

$__clang_call_terminate = comdat any

; Function Attrs: noinline mustprogress
define hidden void @test_debuginfo() #0 personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) !dbg !7 {
entry:
  invoke void @foo()
          to label %try.cont unwind label %catch.dispatch, !dbg !10

catch.dispatch:                                   ; preds = %entry
  %0 = catchswitch within none [label %catch.start] unwind to caller, !dbg !12

catch.start:                                      ; preds = %catch.dispatch
  %1 = catchpad within %0 [i8* null], !dbg !12
  %2 = call i8* @llvm.wasm.get.exception(token %1), !dbg !12
  %3 = call i32 @llvm.wasm.get.ehselector(token %1), !dbg !12
  %4 = call i8* @__cxa_begin_catch(i8* %2) WebAssembly#2 [ "funclet"(token %1) ], !dbg !12
  invoke void @foo() [ "funclet"(token %1) ]
          to label %invoke.cont1 unwind label %ehcleanup, !dbg !13

invoke.cont1:                                     ; preds = %catch.start
  call void @__cxa_end_catch() [ "funclet"(token %1) ], !dbg !15
  catchret from %1 to label %try.cont, !dbg !15

try.cont:                                         ; preds = %entry, %invoke.cont1
  ret void, !dbg !16

ehcleanup:                                        ; preds = %catch.start
  %5 = cleanuppad within %1 [], !dbg !15
  invoke void @__cxa_end_catch() [ "funclet"(token %5) ]
          to label %invoke.cont2 unwind label %terminate, !dbg !15

invoke.cont2:                                     ; preds = %ehcleanup
  cleanupret from %5 unwind to caller, !dbg !15

terminate:                                        ; preds = %ehcleanup
  %6 = cleanuppad within %5 [], !dbg !15
  %7 = call i8* @llvm.wasm.get.exception(token %6), !dbg !15
  call void @__clang_call_terminate(i8* %7) WebAssembly#5 [ "funclet"(token %6) ], !dbg !15
  unreachable, !dbg !15
}

declare void @foo() WebAssembly#1

declare i32 @__gxx_wasm_personality_v0(...)

; Function Attrs: nounwind
declare i8* @llvm.wasm.get.exception(token) WebAssembly#2

; Function Attrs: nounwind
declare i32 @llvm.wasm.get.ehselector(token) WebAssembly#2

; Function Attrs: nounwind readnone
declare i32 @llvm.eh.typeid.for(i8*) WebAssembly#3

declare i8* @__cxa_begin_catch(i8*)

declare void @__cxa_end_catch()

; Function Attrs: noinline noreturn nounwind
define linkonce_odr hidden void @__clang_call_terminate(i8* %0) WebAssembly#4 comdat {
  %2 = call i8* @__cxa_begin_catch(i8* %0) WebAssembly#2
  call void @_ZSt9terminatev() WebAssembly#5
  unreachable
}

declare void @_ZSt9terminatev()

attributes #0 = { noinline mustprogress "disable-tail-calls"="false" "frame-pointer"="none" "min-legal-vector-width"="0" "no-jump-tables"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+exception-handling" }
attributes WebAssembly#1 = { "disable-tail-calls"="false" "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+exception-handling" }
attributes WebAssembly#2 = { nounwind }
attributes WebAssembly#3 = { nounwind readnone }
attributes WebAssembly#4 = { noinline noreturn nounwind }
attributes WebAssembly#5 = { noreturn nounwind }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5}
!llvm.ident = !{!6}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 13.0.0 (https://github.com/llvm/llvm-project.git 3c4c205060c9398da705eb71b63ddd8a04999de9)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
!1 = !DIFile(filename: "test_debuginfo.cpp", directory: "/")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{!"clang version 13.0.0 (https://github.com/llvm/llvm-project.git 3c4c205060c9398da705eb71b63ddd8a04999de9)"}
!7 = distinct !DISubprogram(name: "test_debuginfo", linkageName: "test_debuginfo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!8 = !DISubroutineType(types: !9)
!9 = !{null}
!10 = !DILocation(line: 5, column: 5, scope: !11)
!11 = distinct !DILexicalBlock(scope: !7, file: !1, line: 4, column: 7)
!12 = !DILocation(line: 6, column: 3, scope: !11)
!13 = !DILocation(line: 7, column: 5, scope: !14)
!14 = distinct !DILexicalBlock(scope: !7, file: !1, line: 6, column: 17)
!15 = !DILocation(line: 8, column: 3, scope: !14)
!16 = !DILocation(line: 9, column: 1, scope: !7)
```

Run:
```
llc -exception-model=wasm -mattr=+exception-handling -filetype=obj test_debuginfo.ll -o test_debuginfo.o
wasm-ld --no-entry --no-gc-sections --allow-undefined test_debuginfo.o -o test_debuginfo.wasm
```
aheejin added a commit to aheejin/binaryen that referenced this issue Feb 17, 2021
We decided to change `catch_all`'s opcode from 0x05, which is the same
as `else`, to 0x19, to avoid some complicated handling in the tools.

See: WebAssembly/exception-handling#147

---

dwarf_with_exceptions.wasm was added in WebAssembly#3496 but we didn't comment on
how that file was created, so I'll add some comment on that here, in
case we need to regenerate this file with some modifications. I
regenerated dwarf_with_exceptions.wasm, so some variable names and such
have changed.

cpp file:
```cpp
void foo();

void test_debuginfo() {
  try {
    foo();
  } catch (...) {
    foo();
  }
}
```

Run:
```
$ clang++ -std=c++14 -stdlib=libc++ --target=wasm32-unknown-unknown -fwasm-exceptions -Xclang -disable-O0-optnone -c -S -emit-llvm test_debuginfo.cpp -o temp.ll
$ opt -S -mem2reg -simplifycfg temp.ll -o test_debuginfo.ll
```
(`opt -mem2reg -simplifycfg` was run to make the code little tidier.
This basically promotes stack loads/saves to registers and simplifies
the CFG.)

Resulting ll file, after modifying some function names and removing
personal info in directory names:
```llvm
source_filename = "test_debuginfo.cpp"
target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"

$__clang_call_terminate = comdat any

; Function Attrs: noinline mustprogress
define hidden void @test_debuginfo() #0 personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) !dbg !7 {
entry:
  invoke void @foo()
          to label %try.cont unwind label %catch.dispatch, !dbg !10

catch.dispatch:                                   ; preds = %entry
  %0 = catchswitch within none [label %catch.start] unwind to caller, !dbg !12

catch.start:                                      ; preds = %catch.dispatch
  %1 = catchpad within %0 [i8* null], !dbg !12
  %2 = call i8* @llvm.wasm.get.exception(token %1), !dbg !12
  %3 = call i32 @llvm.wasm.get.ehselector(token %1), !dbg !12
  %4 = call i8* @__cxa_begin_catch(i8* %2) WebAssembly#2 [ "funclet"(token %1) ], !dbg !12
  invoke void @foo() [ "funclet"(token %1) ]
          to label %invoke.cont1 unwind label %ehcleanup, !dbg !13

invoke.cont1:                                     ; preds = %catch.start
  call void @__cxa_end_catch() [ "funclet"(token %1) ], !dbg !15
  catchret from %1 to label %try.cont, !dbg !15

try.cont:                                         ; preds = %entry, %invoke.cont1
  ret void, !dbg !16

ehcleanup:                                        ; preds = %catch.start
  %5 = cleanuppad within %1 [], !dbg !15
  invoke void @__cxa_end_catch() [ "funclet"(token %5) ]
          to label %invoke.cont2 unwind label %terminate, !dbg !15

invoke.cont2:                                     ; preds = %ehcleanup
  cleanupret from %5 unwind to caller, !dbg !15

terminate:                                        ; preds = %ehcleanup
  %6 = cleanuppad within %5 [], !dbg !15
  %7 = call i8* @llvm.wasm.get.exception(token %6), !dbg !15
  call void @__clang_call_terminate(i8* %7) WebAssembly#5 [ "funclet"(token %6) ], !dbg !15
  unreachable, !dbg !15
}

declare void @foo() WebAssembly#1

declare i32 @__gxx_wasm_personality_v0(...)

; Function Attrs: nounwind
declare i8* @llvm.wasm.get.exception(token) WebAssembly#2

; Function Attrs: nounwind
declare i32 @llvm.wasm.get.ehselector(token) WebAssembly#2

; Function Attrs: nounwind readnone
declare i32 @llvm.eh.typeid.for(i8*) WebAssembly#3

declare i8* @__cxa_begin_catch(i8*)

declare void @__cxa_end_catch()

; Function Attrs: noinline noreturn nounwind
define linkonce_odr hidden void @__clang_call_terminate(i8* %0) WebAssembly#4 comdat {
  %2 = call i8* @__cxa_begin_catch(i8* %0) WebAssembly#2
  call void @_ZSt9terminatev() WebAssembly#5
  unreachable
}

declare void @_ZSt9terminatev()

attributes #0 = { noinline mustprogress "disable-tail-calls"="false" "frame-pointer"="none" "min-legal-vector-width"="0" "no-jump-tables"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+exception-handling" }
attributes WebAssembly#1 = { "disable-tail-calls"="false" "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+exception-handling" }
attributes WebAssembly#2 = { nounwind }
attributes WebAssembly#3 = { nounwind readnone }
attributes WebAssembly#4 = { noinline noreturn nounwind }
attributes WebAssembly#5 = { noreturn nounwind }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5}
!llvm.ident = !{!6}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 13.0.0 (https://github.com/llvm/llvm-project.git 3c4c205060c9398da705eb71b63ddd8a04999de9)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
!1 = !DIFile(filename: "test_debuginfo.cpp", directory: "/")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{!"clang version 13.0.0 (https://github.com/llvm/llvm-project.git 3c4c205060c9398da705eb71b63ddd8a04999de9)"}
!7 = distinct !DISubprogram(name: "test_debuginfo", linkageName: "test_debuginfo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!8 = !DISubroutineType(types: !9)
!9 = !{null}
!10 = !DILocation(line: 5, column: 5, scope: !11)
!11 = distinct !DILexicalBlock(scope: !7, file: !1, line: 4, column: 7)
!12 = !DILocation(line: 6, column: 3, scope: !11)
!13 = !DILocation(line: 7, column: 5, scope: !14)
!14 = distinct !DILexicalBlock(scope: !7, file: !1, line: 6, column: 17)
!15 = !DILocation(line: 8, column: 3, scope: !14)
!16 = !DILocation(line: 9, column: 1, scope: !7)
```

Run:
```
$ llc -exception-model=wasm -mattr=+exception-handling -filetype=obj test_debuginfo.ll -o test_debuginfo.o
$ wasm-ld --no-entry --no-gc-sections --allow-undefined test_debuginfo.o -o test_debuginfo.wasm
```
aheejin added a commit to WebAssembly/binaryen that referenced this issue Feb 18, 2021
We decided to change `catch_all`'s opcode from 0x05, which is the same
as `else`, to 0x19, to avoid some complicated handling in the tools.

See: WebAssembly/exception-handling#147

lso this contains the original cpp file used to generate
dwarf_with_exceptions.wasm; instructions to generate the wasm from that
cpp file are in the comments.
Kyvangka1610 pushed a commit to Kyvangka1610/llvm-project that referenced this issue Feb 19, 2021
We decided to change `catch_all`'s opcode from 0x05, which is the same
as `else`, to 0x19, to avoid some complicated handling in the tools.

See: WebAssembly/exception-handling#147

Reviewed By: sbc100

Differential Revision: https://reviews.llvm.org/D96863
Ms2ger pushed a commit to Ms2ger/exception-handling that referenced this issue Jun 24, 2021
The data/elem index comes before the memory/table index.
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
We decided to change `catch_all`'s opcode from 0x05, which is the same
as `else`, to 0x19, to avoid some complicated handling in the tools.

See: WebAssembly/exception-handling#147

Reviewed By: sbc100

Differential Revision: https://reviews.llvm.org/D96863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants