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

[Jing Yu]

Ok, I will move is_thunk back to function structure and re-send the patch.

Thanks,
Jing

2009/3/12 Jan Hubicka <hubicka@ucw.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;
>> >> +}
>> >>
>> >
>