[PATCH] D24085: arm: Fix ttype encoding assertion failure.
logan closed this revision. logan added a comment. Thanks for reviewing. Committed as https://reviews.llvm.org/rL286760. https://reviews.llvm.org/D24085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
mclow.lists accepted this revision. mclow.lists added a comment. This revision is now accepted and ready to land. Thanks. https://reviews.llvm.org/D24085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
logan updated this revision to Diff 76394.
logan added a comment.
Refine assertions to address the comments from mclow.lists.
https://reviews.llvm.org/D24085
Files:
src/cxa_personality.cpp
test/lit.cfg
test/lit.site.cfg.in
test/native/arm-linux-eabi/lit.local.cfg
test/native/arm-linux-eabi/ttype-encoding-00.pass.sh.s
test/native/arm-linux-eabi/ttype-encoding-90.pass.sh.s
Index: test/native/arm-linux-eabi/ttype-encoding-90.pass.sh.s
===
--- /dev/null
+++ test/native/arm-linux-eabi/ttype-encoding-90.pass.sh.s
@@ -0,0 +1,96 @@
+@ RUN: %cxx %flags %link_flags %s -o %t.exe
+@ RUN: %exec %t.exe
+
+@ PURPOSE: Check that 0x90 is a valid value for ttype encoding.
+
+@ NOTE:
+@
+@ This file is generated from the following C++ source code and then change the
+@ `TType Encoding` to 0x90.
+@
+@ ```
+@ int main() {
+@ try {
+@ throw 5;
+@ } catch (int i) {
+@ if (i != 5)
+@ abort();
+@ return 0;
+@ }
+@ }
+@ ```
+
+ .syntax unified
+
+ .text
+ .globl main
+ .p2align 2
+ .type main,%function
+main: @ @main
+.Lfunc_begin0:
+ .fnstart
+@ BB#0: @ %entry
+ .save {r11, lr}
+ push {r11, lr}
+ .setfp r11, sp
+ mov r11, sp
+ mov r0, #4
+ bl __cxa_allocate_exception
+ mov r1, #5
+ str r1, [r0]
+.Ltmp0:
+ ldr r1, .LCPI0_0
+ mov r2, #0
+ bl __cxa_throw
+.Ltmp1:
+
+@ BB#2: @ %lpad
+.Ltmp2:
+ bl __cxa_begin_catch
+ ldr r0, [r0]
+ cmp r0, #5
+ bne .LBB0_4
+@ BB#3: @ %if.end
+ bl __cxa_end_catch
+ mov r0, #0
+ pop {r11, lr}
+ bx lr
+.LBB0_4:@ %if.then
+ bl abort
+ .p2align 2
+@ BB#5:
+.LCPI0_0:
+ .long _ZTIi
+.Lfunc_end0:
+
+ .size main, .Lfunc_end0-main
+ .globl __gxx_personality_v0
+ .personality __gxx_personality_v0
+ .handlerdata
+ .p2align 2
+GCC_except_table0:
+.Lexception0:
+ .byte 255 @ @LPStart Encoding = omit
+ .byte 0x90@ @TType Encoding = indirect | pcrel
+ .asciz "\257\200" @ @TType base offset
+ .byte 3 @ Call site Encoding = udata4
+ .byte 39 @ Call site table length
+ .long .Lfunc_begin0-.Lfunc_begin0 @ >> Call Site 1 <<
+ .long .Ltmp0-.Lfunc_begin0@ Call between .Lfunc_begin0 and .Ltmp0
+ .long 0 @ has no landing pad
+ .byte 0 @ On action: cleanup
+ .long .Ltmp0-.Lfunc_begin0@ >> Call Site 2 <<
+ .long .Ltmp1-.Ltmp0 @ Call between .Ltmp0 and .Ltmp1
+ .long .Ltmp2-.Lfunc_begin0@ jumps to .Ltmp2
+ .byte 1 @ On action: 1
+ .long .Ltmp1-.Lfunc_begin0@ >> Call Site 3 <<
+ .long .Lfunc_end0-.Ltmp1 @ Call between .Ltmp1 and .Lfunc_end0
+ .long 0 @ has no landing pad
+ .byte 0 @ On action: cleanup
+ .byte 1 @ >> Action Record 1 <<
+@ Catch TypeInfo 1
+ .byte 0 @ No further actions
+@ >> Catch TypeInfos <<
+ .long _ZTIi(target2) @ TypeInfo 1
+ .p2align 2
+ .fnend
Index: test/native/arm-linux-eabi/ttype-encoding-00.pass.sh.s
===
--- /dev/null
+++ test/native/arm-linux-eabi/ttype-encoding-00.pass.sh.s
@@ -0,0 +1,97 @@
+@ RUN: %cxx %flags %link_flags %s -o %t.exe
+@ RUN: %exec %t.exe
+
+@ PURPOSE: Check that 0x00 is a valid value for ttype encoding. LLVM and
+@ GCC 4.6 are generating 0x00 as ttype encoding. libc++abi should provide
+@ legacy support.
+
+@ NOTE:
+@
+@ This file is generated from the following C++ source code:
+@
+@ ```
+@ int main() {
+@ try {
+@ throw 5;
+@ } catch (int i) {
+@ if (i != 5)
+@ abort();
+@ return 0;
+@ }
+@ }
+@ ```
+
+ .syntax unified
+
+ .text
+ .globl main
+ .p2align 2
+ .type main,%function
+main: @ @main
+.Lfunc_begin0:
+ .fnstart
+@ BB#0: @ %entry
+ .save {r11, lr}
+ push {r11, lr}
+ .setfp r11, sp
+ mov r11, sp
+ mov r0, #4
+ bl __cxa_allocate_exception
+ mov r1, #5
+ str r1, [r0]
+.Ltmp0:
+ ldr r1, .LCPI0_0
+ mov r2, #0
+ bl __cxa_throw
+.Ltmp1:
+
+@ BB#2: @ %lpad
+.Ltmp2:
+ bl __cxa_begin_catch
+ ldr r0, [r0]
+ cmp r0, #5
+ bne .LBB0_4
+@ BB#3: @ %if.end
+ bl __cxa_end_catch
+ mov r0, #0
+ pop {r11, lr}
+ bx lr
+.LBB0_4:@ %if.then
+ bl abort
+ .p2align 2
+@ BB#5:
+.LCPI0_0:
+ .long _ZTIi
+.Lfunc_end0:
+
+ .size main, .Lfunc_end0-main
+ .globl __gxx_personality_v0
+ .personality __gxx_personality_v0
+ .handlerdata
+ .p2align 2
+GCC_except_table0:
+.Lexception0:
+ .byte 255 @ @LPStart Encoding = omit
+ .byte 0 @ @TType Encoding = absptr
+ .asciz
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
logan added inline comments. Comment at: src/cxa_personality.cpp:363 + "Unexpected TTypeEncoding"); (void)ttypeEncoding; mclow.lists wrote: > logan wrote: > > mclow.lists wrote: > > > It's not clear to me how this accomplishes what you want. > > > You're looking for `00/10/90`, right? Why not just check for that? > > > > > > Why are you anding with 0x0f ? > > > Before, this would pass only a single value - `DW_EH_PE_absptr` (aka 0) > > > With this change, it passes 32 values: 00, 03, 10, 13, 20, 23, and so on. > > > > > > Was that your intent? > > > > > `ttypeEncoding` is encoded with following rules: > > > > 1. Lower 4 bits stand for the representation of the data, such as `absptr`, > > `uleb128`, `udata1`, `udata2`, `udata4`, `udata8`, etc. These bits control > > the way we extract the bytes from the exception table. > > > > 2. Upper 4 bits stand for the post processing action, such as `pcrel`, > > `indirect`, etc. For example, if `pcrel` is specified, then we should add > > the value, which was read in step 1, with the address of the value. > > > > My intention is to weaken the assertion (only assert the essential > > assumption) so that we don't have to alter the assertion if there are new > > configurations that I am not aware of or new compiler which is generating > > different ttypeEncoding. > > > > Since the upcoming lines (L365) only uses `sizeof(uintptr_t)` to decode the > > TType pointer, it is not necessary to impose restriction on the upper 4 > > bits. That's the reason why I wrote `ttypeEncoding & 0xf`. For the same > > reason, both `absptr` and `udata` have the same meaning (4 bytes in the > > exception table) in this context, thus I am adding extra `(ttypeEncoding & > > 0x0f) == DW_EH_PE_udata4`. > Ok; thanks for the explanation. However, I'm still concerned. > The assert is there to catch bad assumptions. (i.e, this can never happen). > > the old code would assert on 255 of 256 possible values. > It turns out that this is too strict - there are at least three values that > we need to pass. > > But your assertion passes 32 possible values (out of 256). So if something > goes wrong, and a random value for `ttypeEncoding` gets passed in here, > there's a 1 in 8 chance that the assertion will not fire. *Thats* my > concern. It should never be an issue, because we don't have bugs, and never > pass random values around, right? ;-) > > As for "dealing with new configurations" or "new compilers", I would say > those are very infrequent events; and I wouldn't worry about them until they > happen. (Demonstrations that I am misinformed here are welcome) Thanks. I'll update the patch soon. https://reviews.llvm.org/D24085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
mclow.lists added inline comments. Comment at: src/cxa_personality.cpp:363 + "Unexpected TTypeEncoding"); (void)ttypeEncoding; logan wrote: > mclow.lists wrote: > > It's not clear to me how this accomplishes what you want. > > You're looking for `00/10/90`, right? Why not just check for that? > > > > Why are you anding with 0x0f ? > > Before, this would pass only a single value - `DW_EH_PE_absptr` (aka 0) > > With this change, it passes 32 values: 00, 03, 10, 13, 20, 23, and so on. > > > > Was that your intent? > > > `ttypeEncoding` is encoded with following rules: > > 1. Lower 4 bits stand for the representation of the data, such as `absptr`, > `uleb128`, `udata1`, `udata2`, `udata4`, `udata8`, etc. These bits control > the way we extract the bytes from the exception table. > > 2. Upper 4 bits stand for the post processing action, such as `pcrel`, > `indirect`, etc. For example, if `pcrel` is specified, then we should add > the value, which was read in step 1, with the address of the value. > > My intention is to weaken the assertion (only assert the essential > assumption) so that we don't have to alter the assertion if there are new > configurations that I am not aware of or new compiler which is generating > different ttypeEncoding. > > Since the upcoming lines (L365) only uses `sizeof(uintptr_t)` to decode the > TType pointer, it is not necessary to impose restriction on the upper 4 bits. > That's the reason why I wrote `ttypeEncoding & 0xf`. For the same reason, > both `absptr` and `udata` have the same meaning (4 bytes in the exception > table) in this context, thus I am adding extra `(ttypeEncoding & 0x0f) == > DW_EH_PE_udata4`. Ok; thanks for the explanation. However, I'm still concerned. The assert is there to catch bad assumptions. (i.e, this can never happen). the old code would assert on 255 of 256 possible values. It turns out that this is too strict - there are at least three values that we need to pass. But your assertion passes 32 possible values (out of 256). So if something goes wrong, and a random value for `ttypeEncoding` gets passed in here, there's a 1 in 8 chance that the assertion will not fire. *Thats* my concern. It should never be an issue, because we don't have bugs, and never pass random values around, right? ;-) As for "dealing with new configurations" or "new compilers", I would say those are very infrequent events; and I wouldn't worry about them until they happen. (Demonstrations that I am misinformed here are welcome) https://reviews.llvm.org/D24085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
logan added inline comments. > mclow.lists wrote in cxa_personality.cpp:363 > It's not clear to me how this accomplishes what you want. > You're looking for `00/10/90`, right? Why not just check for that? > > Why are you anding with 0x0f ? > Before, this would pass only a single value - `DW_EH_PE_absptr` (aka 0) > With this change, it passes 32 values: 00, 03, 10, 13, 20, 23, and so on. > > Was that your intent? `ttypeEncoding` is encoded with following rules: 1. Lower 4 bits stand for the representation of the data, such as `absptr`, `uleb128`, `udata1`, `udata2`, `udata4`, `udata8`, etc. These bits control the way we extract the bytes from the exception table. 2. Upper 4 bits stand for the post processing action, such as `pcrel`, `indirect`, etc. For example, if `pcrel` is specified, then we should add the value, which was read in step 1, with the address of the value. My intention is to weaken the assertion (only assert the essential assumption) so that we don't have to alter the assertion if there are new configurations that I am not aware of or new compiler which is generating different ttypeEncoding. Since the upcoming lines (L365) only uses `sizeof(uintptr_t)` to decode the TType pointer, it is not necessary to impose restriction on the upper 4 bits. That's the reason why I wrote `ttypeEncoding & 0xf`. For the same reason, both `absptr` and `udata` have the same meaning (4 bytes in the exception table) in this context, thus I am adding extra `(ttypeEncoding & 0x0f) == DW_EH_PE_udata4`. https://reviews.llvm.org/D24085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
mclow.lists added inline comments. > cxa_personality.cpp:363 > + "Unexpected TTypeEncoding"); > (void)ttypeEncoding; > It's not clear to me how this accomplishes what you want. You're looking for `00/10/90`, right? Why not just check for that? Why are you anding with 0x0f ? Before, this would pass only a single value - `DW_EH_PE_absptr` (aka 0) With this change, it passes 32 values: 00, 03, 10, 13, 20, 23, and so on. Was that your intent? https://reviews.llvm.org/D24085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24085: arm: Fix ttype encoding assertion failure.
logan created this revision.
logan added reviewers: mclow.lists, rengolin.
logan added a subscriber: cfe-commits.
Herald added subscribers: samparker, rengolin, aemerson.
GCC 4.7 or newer emits 0x90 (indirect | pcrel) as the ttype encoding.
This would hit an assertion in cxa_personality.cpp. This commit fixes
the problem by relaxing the assertion.
http://llvm.org/PR28884
https://reviews.llvm.org/D24085
Files:
src/cxa_personality.cpp
test/lit.cfg
test/lit.site.cfg.in
test/native/arm-linux-eabi/lit.local.cfg
test/native/arm-linux-eabi/ttype-encoding-00.pass.sh.s
test/native/arm-linux-eabi/ttype-encoding-90.pass.sh.s
Index: test/native/arm-linux-eabi/ttype-encoding-90.pass.sh.s
===
--- /dev/null
+++ test/native/arm-linux-eabi/ttype-encoding-90.pass.sh.s
@@ -0,0 +1,96 @@
+@ RUN: %cxx %flags %link_flags %s -o %t.exe
+@ RUN: %exec %t.exe
+
+@ PURPOSE: Check that 0x90 is a valid value for ttype encoding.
+
+@ NOTE:
+@
+@ This file is generated from the following C++ source code and then change the
+@ `TType Encoding` to 0x90.
+@
+@ ```
+@ int main() {
+@ try {
+@ throw 5;
+@ } catch (int i) {
+@ if (i != 5)
+@ abort();
+@ return 0;
+@ }
+@ }
+@ ```
+
+ .syntax unified
+
+ .text
+ .globl main
+ .p2align 2
+ .type main,%function
+main: @ @main
+.Lfunc_begin0:
+ .fnstart
+@ BB#0: @ %entry
+ .save {r11, lr}
+ push {r11, lr}
+ .setfp r11, sp
+ mov r11, sp
+ mov r0, #4
+ bl __cxa_allocate_exception
+ mov r1, #5
+ str r1, [r0]
+.Ltmp0:
+ ldr r1, .LCPI0_0
+ mov r2, #0
+ bl __cxa_throw
+.Ltmp1:
+
+@ BB#2: @ %lpad
+.Ltmp2:
+ bl __cxa_begin_catch
+ ldr r0, [r0]
+ cmp r0, #5
+ bne .LBB0_4
+@ BB#3: @ %if.end
+ bl __cxa_end_catch
+ mov r0, #0
+ pop {r11, lr}
+ bx lr
+.LBB0_4:@ %if.then
+ bl abort
+ .p2align 2
+@ BB#5:
+.LCPI0_0:
+ .long _ZTIi
+.Lfunc_end0:
+
+ .size main, .Lfunc_end0-main
+ .globl __gxx_personality_v0
+ .personality __gxx_personality_v0
+ .handlerdata
+ .p2align 2
+GCC_except_table0:
+.Lexception0:
+ .byte 255 @ @LPStart Encoding = omit
+ .byte 0x90@ @TType Encoding = indirect | pcrel
+ .asciz "\257\200" @ @TType base offset
+ .byte 3 @ Call site Encoding = udata4
+ .byte 39 @ Call site table length
+ .long .Lfunc_begin0-.Lfunc_begin0 @ >> Call Site 1 <<
+ .long .Ltmp0-.Lfunc_begin0@ Call between .Lfunc_begin0 and .Ltmp0
+ .long 0 @ has no landing pad
+ .byte 0 @ On action: cleanup
+ .long .Ltmp0-.Lfunc_begin0@ >> Call Site 2 <<
+ .long .Ltmp1-.Ltmp0 @ Call between .Ltmp0 and .Ltmp1
+ .long .Ltmp2-.Lfunc_begin0@ jumps to .Ltmp2
+ .byte 1 @ On action: 1
+ .long .Ltmp1-.Lfunc_begin0@ >> Call Site 3 <<
+ .long .Lfunc_end0-.Ltmp1 @ Call between .Ltmp1 and .Lfunc_end0
+ .long 0 @ has no landing pad
+ .byte 0 @ On action: cleanup
+ .byte 1 @ >> Action Record 1 <<
+@ Catch TypeInfo 1
+ .byte 0 @ No further actions
+@ >> Catch TypeInfos <<
+ .long _ZTIi(target2) @ TypeInfo 1
+ .p2align 2
+ .fnend
Index: test/native/arm-linux-eabi/ttype-encoding-00.pass.sh.s
===
--- /dev/null
+++ test/native/arm-linux-eabi/ttype-encoding-00.pass.sh.s
@@ -0,0 +1,97 @@
+@ RUN: %cxx %flags %link_flags %s -o %t.exe
+@ RUN: %exec %t.exe
+
+@ PURPOSE: Check that 0x00 is a valid value for ttype encoding. LLVM and
+@ GCC 4.6 are generating 0x00 as ttype encoding. libc++abi should provide
+@ legacy support.
+
+@ NOTE:
+@
+@ This file is generated from the following C++ source code:
+@
+@ ```
+@ int main() {
+@ try {
+@ throw 5;
+@ } catch (int i) {
+@ if (i != 5)
+@ abort();
+@ return 0;
+@ }
+@ }
+@ ```
+
+ .syntax unified
+
+ .text
+ .globl main
+ .p2align 2
+ .type main,%function
+main: @ @main
+.Lfunc_begin0:
+ .fnstart
+@ BB#0: @ %entry
+ .save {r11, lr}
+ push {r11, lr}
+ .setfp r11, sp
+ mov r11, sp
+ mov r0, #4
+ bl __cxa_allocate_exception
+ mov r1, #5
+ str r1, [r0]
+.Ltmp0:
+ ldr r1, .LCPI0_0
+ mov r2, #0
+ bl __cxa_throw
+.Ltmp1:
+
+@ BB#2: @ %lpad
+.Ltmp2:
+ bl __cxa_begin_catch
+ ldr r0, [r0]
+ cmp r0, #5
+ bne .LBB0_4
+@ BB#3: @ %if.end
+ bl __cxa_end_catch
+ mov r0, #0
+ pop {r11, lr}
+ bx lr
+.LBB0_4:@ %if.then
+ bl abort
+ .p2align 2
+@ BB#5:
+.LCPI0_0:
+ .long _ZTIi
+.Lfunc_end0:
+
+ .size main, .Lfunc_end0-main
+ .globl
