Re: [PATCH]Save/restore crtl->is_thunk for emitting thunk code (PR/debug 39378)

[Jan Hubicka <hubicka at ucw dot cz>]

> I have looked at the code in GCC4.2.1, because GCC4.2.1 does not break
> the test case. In GCC4.2.1, there was not crtl structure. All fields
> in crtl were in the per function structure "function" node. Since some
> time, the "function" structure split into 2 parts, one is still called
> "function", the other is the shared structure "rtl_data". I guess the
> reason of the split is to save space and reduce access time.
> 
> So it should work to move is_thunk flag back from crtl to "function"
> structure. I did not do that, because

When I was breaking up stuff from function to crtl, I was looking for
cases that don't need to survive across function context changes.
is_thunk seems to be overlook here, so I would second for putting it
back to struct function.

In longer run we need to get rid of the thunks anyway since they are
problematic for LTO.  At very least it should be possible to emit
associated thunks after the function body is compiled.  This would
however require more testing on multiple platforms as thunks are not
allowed, so it is not good idea at this stage.

Honza
> 1. Moving a shared structure field to a per function structure makes
> the access time longer.
> 2. There will be more changes. It is easy to introduce other bugs.
> 3. is_thunk is only involved in processing a thunk and a thunk target.
> 
> We can discuss.
> 
> Thanks,
> Jing
> 
> 
> 
> On Thu, Mar 12, 2009 at 7:34 AM, Daniel Berlin <dberlin@dberlin.org> wrote:
> > On Wed, Mar 11, 2009 at 4:09 PM, Jing Yu <jingyuuiuc@gmail.com> wrote:
> >> ?This patch is to fix the bug PR/debug 39378 that crtl is not reset
> >> ?after emitting thunk functions. The thunk is emitted with crtl->is_thunk=1,
> >> and the thunk target function is also emitted
> >> ?with crtl->is_thunk=1. The bug exposes if compiled with -mthumb flag
> >> ?on arm-eabi backend, where the generated binary causes segmentation
> >> ?fault, because the thunk target function is coded in 32 bit mode, but
> >> ?the thunk calls the target via arm-to-thunk approach.
> >>
> >> The fix is that save crtl->is_thunk before the call to lang_hooks.callgraph.emit
> >> _associated_thunks() in cgraph_expand_function(), and restore crtl->is_thunk
> >> after the call.
> >> I also add a test case to make sure the bug will not happen again.
> >
> > Shouldn't the fix be to just move the flag to the per-function
> > structure and use it directly, instead of keeping it in the shared
> > structure?
> >
> >>
> >> DejaGNU test:
> >> I built the toolchain with GNU trunk GCC (gcc version
> >> 4.4.0 20090305) and newlib, and tested it on the arm emulator.
> >> This patch does not trigger more regression errors or warnings.
> >>
> >> Target: arm-eabi
> >> Configured with: /home/jingyu/projects/gcc/gcc_src/configure
> >> --prefix=/home/jingyu/local/gcc_arm_eabi --target=arm-eabi
> >> --build=i686-linux --host=i686-linux --enable-multilib --with-newlib
> >> --with-gnu-as --with-gnu-ld --enable-languages=c,c++
> >> Thread model: single
> >> gcc version 4.4.0 20090305 (experimental) (GCC)
> >>
> >> Thanks,
> >> Jing
> >>
> >>
> >> Index: gcc/cgraphunit.c
> >> ===================================================================
> >> --- gcc/cgraphunit.c??? (revision 144771)
> >> +++ gcc/cgraphunit.c??? (working copy)
> >> @@ -1043,7 +1043,11 @@
> >>
> >> ?? /* Generate RTL for the body of DECL.? */
> >> ?? if (lang_hooks.callgraph.emit_associated_thunks)
> >> -??? lang_hooks.callgraph.emit_associated_thunks (decl);
> >> +??? {
> >> +????? bool is_thunk = crtl->is_thunk;
> >> +????? lang_hooks.callgraph.emit_associated_thunks (decl);
> >> +????? crtl->is_thunk = is_thunk;
> >> +??? }
> >> ?? tree_rest_of_compilation (decl);
> >>
> >> ?? /* Make sure that BE didn't give up on compiling.? */
> >>
> >> Index: gcc/testsuite/g++.dg/inherit/thunk10.C
> >> ===================================================================
> >> --- gcc/testsuite/g++.dg/inherit/thunk10.C????? (revision 0)
> >> +++ gcc/testsuite/g++.dg/inherit/thunk10.C????? (revision 0)
> >> @@ -0,0 +1,61 @@
> >> +/* { dg-options "-mthumb" { target arm*-*-* } } */
> >> +/* { dg-do run } */
> >> +/* { dg-timeout 10 } */
> >> +
> >> +/* PR target/39378 */
> >> +/* Check if the thunk target function is emitted correctly. */
> >> +class B1
> >> +{
> >> +public:
> >> +? virtual int foo1(void);
> >> +? int b1;
> >> +};
> >> +
> >> +
> >> +class B2
> >> +{
> >> +public:
> >> +? virtual int foo2 (void);
> >> +? int b2;
> >> +};
> >> +
> >> +class D : public B1, public B2
> >> +{
> >> +? int foo1(void);
> >> +? int foo2(void);
> >> +? int d;
> >> +};
> >> +
> >> +int B1::foo1(void)
> >> +{
> >> +? return 3;
> >> +}
> >> +
> >> +int B2::foo2(void)
> >> +{
> >> +? return 4;
> >> +}
> >> +
> >> +int D::foo1(void)
> >> +{
> >> +? return 1;
> >> +}
> >> +
> >> +int D::foo2(void)
> >> +{
> >> +? return 2;
> >> +}
> >> +
> >> +__attribute__((noinline)) int test(B2* bp)
> >> +{
> >> +? return bp->foo2();
> >> +}
> >> +
> >> +int main()
> >> +{
> >> +? B2 *bp = new D();
> >> +? if (test(bp) == 2)
> >> +??? return 0;
> >> +? else
> >> +??? return 1;
> >> +}
> >>
> >