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

[Jing Yu <jingyuuiuc at gmail dot com>]

I moved is_thunk from the shared rtl_data structure back to function structure.
I grep "is_thunk" on all files under gcc/ directory, and change all
crtl->is_thunk to cfun->is_thunk.
I does the dejaGNU test for arm target again. This patch does not
produce any difference.

Thanks,
Jing

On Thu, Mar 12, 2009 at 10:45 AM, Jing Yu <jingyuuiuc@gmail.com> wrote:
> 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;
>>> >> +}
>>> >>
>>> >
>>
>

Attachment: patch_39378
Description: Binary data