public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug go/60406] New: reflect.go:test13reflect2 test failure
@ 2014-03-04  7:30 vogt at linux dot vnet.ibm.com
  2014-08-06 17:04 ` [Bug go/60406] recover.go: test13reflect2 " ubizjak at gmail dot com
                   ` (21 more replies)
  0 siblings, 22 replies; 23+ messages in thread
From: vogt at linux dot vnet.ibm.com @ 2014-03-04  7:30 UTC (permalink / raw)
  To: gcc-bugs

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.


^ permalink raw reply	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-10-28  8:50 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-04  7:30 [Bug go/60406] New: reflect.go:test13reflect2 test failure vogt at linux dot vnet.ibm.com
2014-08-06 17:04 ` [Bug go/60406] recover.go: test13reflect2 " ubizjak at gmail dot com
2014-08-07  8:47 ` ubizjak at gmail dot com
2014-08-07 10:34 ` ubizjak at gmail dot com
2014-09-04 15:52 ` ian at airs dot com
2014-09-05  7:15 ` vogt at linux dot vnet.ibm.com
2014-09-16 20:57 ` boger at us dot ibm.com
2014-09-22 20:57 ` boger at us dot ibm.com
2014-09-23 13:34 ` bergner at gcc dot gnu.org
2014-09-29 13:17 ` boger at us dot ibm.com
2014-10-03 22:59 ` ian at airs dot com
2014-10-06 15:43 ` vogt at linux dot vnet.ibm.com
2014-10-06 15:48 ` boger at us dot ibm.com
2014-10-07  9:12 ` vogt at linux dot vnet.ibm.com
2014-10-07 13:56 ` ian at airs dot com
2014-10-07 18:25 ` ian at airs dot com
2014-10-08  5:49 ` ubizjak at gmail dot com
2014-10-08  8:12 ` vogt at linux dot vnet.ibm.com
2014-10-08 10:13 ` vogt at linux dot vnet.ibm.com
2014-10-08 13:28 ` ian at airs dot com
2014-10-08 13:44 ` ian at airs dot com
2014-10-08 14:15 ` ian at airs dot com
2014-10-28  8:55 ` vogt at linux dot vnet.ibm.com

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).