http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60406
Bug ID: 60406
Summary: reflect.go:test13reflect2 test failure
Product: gcc
Version: 4.9.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: go
Assignee: ian at airs dot com
Reporter: vogt at linux dot vnet.ibm.com
>From a email discussion between me (DV) and Ian Lance Taylor (IT):
DV: The bug shows up when running recover.go from the go testsuite
DV: in the function test13reflect2 (and test14reflect2).
DV:
DV: When panic is called, the deferred reflection function is invoked
DV: by calling the thunk function. (See below for the assembly code
DV: on s390). The call to __go_set_defer_retaddr is passed the address
DV: of a label that was generated originally right behind the call to
DV: makeFuncStub, and the fake jump is there to make sure the
DV: automatically generated label does not get deleted during
DV: compilation. However, in the assembler code a different address
DV: is passed to __go_set_defer_retaddr that the target address of the
DV: fake jump:
DV:
DV: -- snip assembler code --
DV: 0000000080002490 <main.$thunk0>:
DV: ...
DV: 8000249e: larl %r2,800024f4 <main.$thunk0+0x64> -------
DV: 800024a4: brasl %r14,80002c48 <__go_set_defer_retaddr> |
DV: 800024aa: tmll %r2,255 |
DV: # fake jump |
DV: 800024ae: jne 80002500 <main.$thunk0+0x70> -------- |
DV: 800024b2: ltgr %r13,%r13 | |
DV: ... | |
DV: # code that copies T5 | |
DV: 800024de: lghi %r3,31 | |
DV: 800024e2: mvc 0(256,%r2),0(%r1) | |
DV: 800024e8: la %r2,256(%r2) | |
DV: 800024ec: la %r1,256(%r1) | |
DV: 800024f0: brctg %r3,800024e2 <main.$thunk0+0x52> | |
DV: # wrong position of split label | |
DV: 800024f4: mvc 0(256,%r2),0(%r1) <-------|--
DV: 800024fa: la %r2,160(%r15) |
DV: # call makeFuncStub |
DV: 800024fe: basr %r14,%r13 <--- makeFuncStub |
DV: # original position of the label "retaddr" |
DV: 80002500: lghi %r2,0 <-------
DV: ...
DV: -- snip --
DV:
DV: The code in libgo/runtime/go-recover.c:__go_can_recover tests
DV: whether the address passed to __go_set_defer_retaddr is in a
DV: certain range around the the real return address of makeFuncStub:
DV:
DV: if (ret <= dret && ret + 16 >= dret)
DV: return 1;
DV:
DV: In the above assembly language code the assumption that the defer
DV: retaddr will always end up in that range is violated. Thus,
DV: __go_can_recover returns 0, the panic is not recovered and the test
DV: case fails.
DV:
DV: --
DV:
DV: There are two problems in the current code. First, the assumption
DV: in __go_can_recover is too strict. I think the "correct" test
DV: would be to check whether dret is inside the thunk function, i.e.
DV:
DV: if (dret >= thunk_start && dret < thunk_end)
DV: return 1;
IT: Yes.
DV: Except that thunk_start and thunk_end are not easily available at
DV: that place.
IT: Well, we could get thunk_start fairly easily: we could pass it to
IT: __go_set_defer_retaddr, just as we currently pass the return label. I
IT: can't think of a reliable way for us to get thunk_end, though.
DV: Second, the code ("hack") in gcc/go/gofrontend/statementis.cc:build_thunk()
DV: simply does not work:
DV:
DV: -- snip --
DV: // For a defer statement, start with a call to
DV: // __go_set_defer_retaddr. */
DV: Label* retaddr_label = NULL;
DV: if (may_call_recover)
DV: {
DV: retaddr_label = gogo->add_label_reference("retaddr", location,
false);
DV: Expression* arg = Expression::make_label_addr(retaddr_label,
location);
DV: Expression* call = Runtime::make_call(Runtime::SET_DEFER_RETADDR,
DV: location, 1, arg);
DV:
DV: // This is a hack to prevent the middle-end from deleting the
DV: // label.
DV: gogo->start_block(location);
DV: gogo->add_statement(Statement::make_goto_statement(retaddr_label,
DV: location));
DV: ...
DV: }
DV:
DV: ...
DV:
DV: // If this is a defer statement, the label comes immediately after
DV: // the call.
DV: if (may_call_recover)
DV: {
DV: gogo->add_label_definition("retaddr", location);
DV: ...
DV: }
DV: -- snip --
DV:
DV: The address of the label "retaddr" is passed to
DV: __go_set_defer_retaddr and then a fake jump to that label is
DV: inserted after that (fake because __go_set_defer_retaddr always
DV: returns 0). Although the fake jump does prevent the label of the
DV: jump target from being deleted, it does not help keeping the label
DV: at the passed address alive. What actually happens is this:
DV:
DV: * At first, the call and the jump both use the retaddr label.
DV: * After the cprop1 pass, the cfgcleanup pass calls
DV: try_forward_edges().
DV: * try_forward_edges() decides that the edge described by the jump
DV: can be forwarded to some other place. So, it creates a new
DV: label at the new jump target and redirects the jump there.
DV: Then it tries to delete the label (cfgrtl.c:delete_insn()) and
DV: notices it's a user generated label that cannot be deleted.
DV: Instead, the label is replaced by a NOTE_DELETED_LABEL_NAME to
DV: keep the address reference alive. We now have _two_ labels,
DV: the new, live one and the old deleted one.
DV: * The following passes move the deleted label around the function
DV: and it ends up in a more or less random location inside the
DV: thunk function. (Actually, where it ends up depends on the
DV: amount of code that is needed to copy T5. If T5 is small
DV: enough that it can be copied without a loop, the test succeeds.)
DV:
DV: In other words, when the edge is forwarded, only jumps to the
DV: target label are forwarded but not simple references to the
DV: address of the label. I'm not sure whether this is a Gcc bug or
DV: not.
IT: The design assumes that the thunk is very simple, which it normally
IT: is. A simple thunk won't have any basic blocks so the problematic
IT: optimization won't kick in. I think that assumption is failing in
IT: this case because the function argument is so large. The large
IT: argument is introducing a loop and thus permitting basic block
IT: optimizations to occur.
IT:
IT: I suppose we could introduce yet another thunk, and pass along the
IT: pointer to the parameters, rather than expanding them. That is
IT: getting pretty horrible, though. It would be better to use
IT: thunk_start and thunk_end as you suggest above, if we can figure out
IT: how to do it.
--
DV: To get the thunk address and length, the correct approach is
DV: probably to utilize the DWARF info, ...
IT: To get the thunk address I would suggest just changing the Go
IT: frontend to pass it to __go_defer_set_retaddr.
IT:
IT: I have no particular suggestions for the thunk length. There may
IT: be some relatively simple way to pick that up as well.
