Postpone expanding va_arg until pass_stdarg
[ was: Re: pass_stdarg problem when run after pass_lim ]
On 03-02-15 14:36, Michael Matz wrote:
Hi,
On Tue, 3 Feb 2015, Tom de Vries wrote:
Ironically, that fix breaks the va_list_gpr/fpr_size optimization, so
I've disabled that by default for now.
I've done a non-bootstrap and bootstrap build using all languages.
The non-bootstrap test shows (at least) two classes of real failures:
- gcc.c-torture/execute/20020412-1.c, gcc.target/i386/memcpy-strategy-4.c and
gcc.dg/lto/20090706-1_0.c.
These are test-cases with vla as va_arg argument. It ICEs in
force_constant_size with call stack
gimplify_va_arg_expr -> create_tmp_var -> gimple_add_tmp_var ->
force_constant_size
Hah, yeah, that's the issue I remembered with create_tmp_var. This needs
a change in how to represent the va_arg "call", because the LHS can't be a
temporary that's copied to the real LHS afterwards.
- most/all va_arg tests with flto, f.i. gcc.c-torture/execute/stdarg-1.c.
It segfaults in lto1 during pass_stdarg, in gimplify_va_arg_internal when
accessing have_va_type which is NULL_TREE after
'have_va_type = targetm.canonical_va_list_type (have_va_type)'.
I don't think the flto issue is difficult to fix. But the vla issue
probably needs more time than I have available right now.
I'll think about this.
A status update. I've worked a bit more on this patch series, latest version
available at vries/expand-va-arg-at-pass-stdarg (and last patch in series attached).
I've done a non-bootstrap x86_64 build for all languages and ran the regression
testsuite for unix/ unix/-m32. I'm left with one failing test-case.
[ Of course there are a bunch of scan-dump-tree failures because the
va_list_gpr/fpr_size optimization is switched off. ]
The patch series handles things now as follows. At gimplify_va_arg_expr, the
VA_ARG expr is not gimplified, but replaced by the internal function call.
That is passed upwards to gimplify_modify_expr, which does the actual
gimplification. In this function we have sufficient scope of the problem to deal
with it.
I've added two modifications to gimplify_modify_expr:
- the WITH_SIZE_EXPR in which the CALL_TREE is wrapped, is dropped after
gimplification, but we need the size expression at expansion in pass_stdarg.
So I added the size expression as argument to the internal function.
[ And at pass_stdarg::execute, we wrap the result of gimplify_va_arg_internal
in a WITH_SIZE_EXPR before generating the assign to the lhs ]
- we detect after gimplify_arg (&ap) whether it created a copy ap.1 of ap,
rather than use ap itself, and if so, we copy the value back from ap.1 to ap
after va_arg.
I've worked around the issue of targetm.canonical_va_list_type (have_va_type)
returning NULL_TREE in gimplify_va_arg_internal during lto1, by simply working
with the original type in that case:
...
tree have_va_type = TREE_TYPE (valist);
tree cano_type = targetm.canonical_va_list_type (have_va_type);
if (cano_type != NULL_TREE)
have_va_type = cano_type;
...
I'm not really sure yet why std_gimplify_va_arg_expr has a part commented out.
Michael, can you comment?
The single failing testcase (both with and without -m32) is
g++.dg/torture/pr45843.C:
...
./gcc/testsuite/g++/g++.sum:FAIL: g++.dg/torture/pr45843.C -O2 -flto
-fno-use-linker-plugin -flto-partition=none (internal compiler error)
...
The failure looks like this (it happens during the gimplify_assign after calling
gimplify_va_arg_internal):
...
src/gcc/testsuite/g++.dg/torture/pr45843.C: In function ‘foo(int, ...)’:
src/gcc/testsuite/g++.dg/torture/pr45843.C:11:1: internal compiler error:
Segmentation fault
0x10a5b04 crash_signal
src/gcc/toplev.c:383
0x6a8985 tree_check(tree_node*, char const*, int, char const*, tree_code)
src/gcc/tree.h:2845
0x7c2f6a is_really_empty_class(tree_node*)
src/gcc/cp/class.c:7923
0x923855 cp_gimplify_expr(tree_node**, gimple_statement_base**,
gimple_statement_base**)
src/gcc/cp/cp-gimplify.c:625
0xd34641 gimplify_expr(tree_node**, gimple_statement_base**,
gimple_statement_base**, bool (*)(tree_node*), int)
src/gcc/gimplify.c:7843
0xd2a04d gimplify_stmt(tree_node**, gimple_statement_base**)
src/gcc/gimplify.c:5551
0xd173e3 gimplify_and_add(tree_node*, gimple_statement_base**)
src/gcc/gimplify.c:419
0xd39c94 gimplify_assign(tree_node*, tree_node*, gimple_statement_base**)
src/gcc/gimplify.c:9452
0x130ad18 execute
src/gcc/tree-stdarg.c:779
...
The testcase contains this struct:
...
struct S { struct T { } a[14]; char b; };
...
and uses that struct S as type in va_arg:
...
arg = va_arg (ap, struct S);
...
The segfault happens because we're calling is_really_empty_class for struct S,
and TYPE_BINFO is NULL_TREE, which causes BINFO_BASE_ITERATE to segfault. I'm
not sure yet what this issue is or how this is supposed to be fixed.
Thanks,
- Tom
Postpone expanding va_arg until pass_stdarg
---
Re: Postpone expanding va_arg until pass_stdarg
On Tue, Feb 10, 2015 at 10:22 AM, Tom de Vries wrote:
> [ was: Re: pass_stdarg problem when run after pass_lim ]
>
> On 03-02-15 14:36, Michael Matz wrote:
>>
>> Hi,
>>
>> On Tue, 3 Feb 2015, Tom de Vries wrote:
>>
>>> Ironically, that fix breaks the va_list_gpr/fpr_size optimization, so
>>> I've disabled that by default for now.
>>>
>>> I've done a non-bootstrap and bootstrap build using all languages.
>>>
>>> The non-bootstrap test shows (at least) two classes of real failures:
>>> - gcc.c-torture/execute/20020412-1.c, gcc.target/i386/memcpy-strategy-4.c
>>> and
>>>gcc.dg/lto/20090706-1_0.c.
>>>These are test-cases with vla as va_arg argument. It ICEs in
>>>force_constant_size with call stack
>>>gimplify_va_arg_expr -> create_tmp_var -> gimple_add_tmp_var ->
>>>force_constant_size
>>
>>
>> Hah, yeah, that's the issue I remembered with create_tmp_var. This needs
>> a change in how to represent the va_arg "call", because the LHS can't be a
>> temporary that's copied to the real LHS afterwards.
>>
>>> - most/all va_arg tests with flto, f.i. gcc.c-torture/execute/stdarg-1.c.
>>>It segfaults in lto1 during pass_stdarg, in gimplify_va_arg_internal
>>> when
>>>accessing have_va_type which is NULL_TREE after
>>>'have_va_type = targetm.canonical_va_list_type (have_va_type)'.
>>>
>>> I don't think the flto issue is difficult to fix. But the vla issue
>>> probably needs more time than I have available right now.
>>
>>
>> I'll think about this.
>>
>
> A status update. I've worked a bit more on this patch series, latest version
> available at vries/expand-va-arg-at-pass-stdarg (and last patch in series
> attached).
>
> I've done a non-bootstrap x86_64 build for all languages and ran the
> regression testsuite for unix/ unix/-m32. I'm left with one failing
> test-case.
> [ Of course there are a bunch of scan-dump-tree failures because the
> va_list_gpr/fpr_size optimization is switched off. ]
>
> The patch series handles things now as follows. At gimplify_va_arg_expr, the
> VA_ARG expr is not gimplified, but replaced by the internal function call.
>
> That is passed upwards to gimplify_modify_expr, which does the actual
> gimplification. In this function we have sufficient scope of the problem to
> deal with it.
>
> I've added two modifications to gimplify_modify_expr:
> - the WITH_SIZE_EXPR in which the CALL_TREE is wrapped, is dropped after
> gimplification, but we need the size expression at expansion in
> pass_stdarg.
> So I added the size expression as argument to the internal function.
> [ And at pass_stdarg::execute, we wrap the result of
> gimplify_va_arg_internal
> in a WITH_SIZE_EXPR before generating the assign to the lhs ]
> - we detect after gimplify_arg (&ap) whether it created a copy ap.1 of ap,
> rather than use ap itself, and if so, we copy the value back from ap.1 to
> ap
> after va_arg.
>
> I've worked around the issue of targetm.canonical_va_list_type
> (have_va_type) returning NULL_TREE in gimplify_va_arg_internal during lto1,
> by simply working with the original type in that case:
> ...
> tree have_va_type = TREE_TYPE (valist);
> tree cano_type = targetm.canonical_va_list_type (have_va_type);
>
> if (cano_type != NULL_TREE)
> have_va_type = cano_type;
> ...
>
> I'm not really sure yet why std_gimplify_va_arg_expr has a part commented
> out. Michael, can you comment?
>
>
> The single failing testcase (both with and without -m32) is
> g++.dg/torture/pr45843.C:
> ...
> ./gcc/testsuite/g++/g++.sum:FAIL: g++.dg/torture/pr45843.C -O2 -flto
> -fno-use-linker-plugin -flto-partition=none (internal compiler error)
> ...
>
> The failure looks like this (it happens during the gimplify_assign after
> calling gimplify_va_arg_internal):
> ...
> src/gcc/testsuite/g++.dg/torture/pr45843.C: In function ‘foo(int, ...)’:
> src/gcc/testsuite/g++.dg/torture/pr45843.C:11:1: internal compiler error:
> Segmentation fault
> 0x10a5b04 crash_signal
> src/gcc/toplev.c:383
> 0x6a8985 tree_check(tree_node*, char const*, int, char const*, tree_code)
> src/gcc/tree.h:2845
> 0x7c2f6a is_really_empty_class(tree_node*)
> src/gcc/cp/class.c:7923
> 0x923855 cp_gimplify_expr(tree_node**, gimple_statement_base**,
> gimple_statement_base**)
> src/gcc/cp/cp-gimplify.c:625
> 0xd34641 gimplify_expr(tree_node**, gimple_statement_base**,
> gimple_statement_base**, bool (*)(tree_node*), int)
> src/gcc/gimplify.c:7843
> 0xd2a04d gimplify_stmt(tree_node**, gimple_statement_base**)
> src/gcc/gimplify.c:5551
> 0xd173e3 gimplify_and_add(tree_node*, gimple_statement_base**)
> src/gcc/gimplify.c:419
> 0xd39c94 gimplify_assign(tree_node*, tree_node*, gimple_statement_base**)
> src/gcc/gimplify.c:9452
> 0x130ad18 execute
> src/gcc/tree-stdarg.c:779
> ...
>
> The testcase contains this struct:
> ...
> struct S { struct T { } a[14]; char b; };
> ...
>
> and uses that struct S as type in va_arg:
> ...
> arg = va_arg (ap,
Re: Postpone expanding va_arg until pass_stdarg
On 10-02-15 11:10, Richard Biener wrote:
The single failing testcase (both with and without -m32) is
>g++.dg/torture/pr45843.C:
>...
>./gcc/testsuite/g++/g++.sum:FAIL: g++.dg/torture/pr45843.C -O2 -flto
>-fno-use-linker-plugin -flto-partition=none (internal compiler error)
>...
>
>The failure looks like this (it happens during the gimplify_assign after
>calling gimplify_va_arg_internal):
>...
>src/gcc/testsuite/g++.dg/torture/pr45843.C: In function ‘foo(int, ...)’:
>src/gcc/testsuite/g++.dg/torture/pr45843.C:11:1: internal compiler error:
>Segmentation fault
>0x10a5b04 crash_signal
> src/gcc/toplev.c:383
>0x6a8985 tree_check(tree_node*, char const*, int, char const*, tree_code)
> src/gcc/tree.h:2845
>0x7c2f6a is_really_empty_class(tree_node*)
> src/gcc/cp/class.c:7923
>0x923855 cp_gimplify_expr(tree_node**, gimple_statement_base**,
>gimple_statement_base**)
> src/gcc/cp/cp-gimplify.c:625
>0xd34641 gimplify_expr(tree_node**, gimple_statement_base**,
>gimple_statement_base**, bool (*)(tree_node*), int)
> src/gcc/gimplify.c:7843
>0xd2a04d gimplify_stmt(tree_node**, gimple_statement_base**)
> src/gcc/gimplify.c:5551
>0xd173e3 gimplify_and_add(tree_node*, gimple_statement_base**)
> src/gcc/gimplify.c:419
>0xd39c94 gimplify_assign(tree_node*, tree_node*, gimple_statement_base**)
> src/gcc/gimplify.c:9452
>0x130ad18 execute
> src/gcc/tree-stdarg.c:779
>...
>
>The testcase contains this struct:
>...
>struct S { struct T { } a[14]; char b; };
>...
>
>and uses that struct S as type in va_arg:
>...
> arg = va_arg (ap, struct S);
>...
>
>The segfault happens because we're calling is_really_empty_class for struct
>S, and TYPE_BINFO is NULL_TREE, which causes BINFO_BASE_ITERATE to segfault.
>I'm not sure yet what this issue is or how this is supposed to be fixed.
That's probably free_lang_data being more aggressive after Honza
fiddled with BINFOs? That is - the gimplifications called from tree-stdarg.c
(and others from the middle-end) should never call back into the frontend
via langhooks...
Hmm, that 'should never' sounds like a missing gcc_assert.
This patch is a way to achieve that gimplification doesn't call the actual
gimplify_expr langhook, and it fixes the failure. But I'm guessing that's not
the proper way to fix this.
Thanks,
- Tom
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 443f6d3..d6cd52d 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3. If not see
#include "input.h"
#include "function.h"
#include "langhooks.h"
+#include "langhooks-def.h"
#include "gimple-pretty-print.h"
#include "target.h"
#include "bitmap.h"
@@ -734,6 +735,11 @@ pass_stdarg::execute (function *fun)
const char *funcname = NULL;
tree cfun_va_list;
unsigned int retflags = 0;
+ int (*save_gimplify_expr) (tree *, gimple_seq *, gimple_seq *);
+
+ /* Ensure we don't call language hooks from gimplification. */
+ save_gimplify_expr = lang_hooks.gimplify_expr;
+ lang_hooks.gimplify_expr = lhd_gimplify_expr;
/* Expand va_arg. */
/* TODO: teach pass_stdarg how process the va_arg builtin, and reverse the
@@ -796,6 +802,9 @@ pass_stdarg::execute (function *fun)
}
}
+ /* Restore language hook. */
+ lang_hooks.gimplify_expr = save_gimplify_expr;
+
if (retflags)
{
free_dominance_info (CDI_DOMINATORS);
Re: Postpone expanding va_arg until pass_stdarg
On Tue, Feb 10, 2015 at 2:20 PM, Tom de Vries wrote:
> On 10-02-15 11:10, Richard Biener wrote:
>>>
>>> The single failing testcase (both with and without -m32) is
>>> >g++.dg/torture/pr45843.C:
>>> >...
>>> >./gcc/testsuite/g++/g++.sum:FAIL: g++.dg/torture/pr45843.C -O2 -flto
>>> >-fno-use-linker-plugin -flto-partition=none (internal compiler error)
>>> >...
>>> >
>>> >The failure looks like this (it happens during the gimplify_assign after
>>> >calling gimplify_va_arg_internal):
>>> >...
>>> >src/gcc/testsuite/g++.dg/torture/pr45843.C: In function ‘foo(int, ...)’:
>>> >src/gcc/testsuite/g++.dg/torture/pr45843.C:11:1: internal compiler
>>> > error:
>>> >Segmentation fault
>>> >0x10a5b04 crash_signal
>>> > src/gcc/toplev.c:383
>>> >0x6a8985 tree_check(tree_node*, char const*, int, char const*,
>>> > tree_code)
>>> > src/gcc/tree.h:2845
>>> >0x7c2f6a is_really_empty_class(tree_node*)
>>> > src/gcc/cp/class.c:7923
>>> >0x923855 cp_gimplify_expr(tree_node**, gimple_statement_base**,
>>> >gimple_statement_base**)
>>> > src/gcc/cp/cp-gimplify.c:625
>>> >0xd34641 gimplify_expr(tree_node**, gimple_statement_base**,
>>> >gimple_statement_base**, bool (*)(tree_node*), int)
>>> > src/gcc/gimplify.c:7843
>>> >0xd2a04d gimplify_stmt(tree_node**, gimple_statement_base**)
>>> > src/gcc/gimplify.c:5551
>>> >0xd173e3 gimplify_and_add(tree_node*, gimple_statement_base**)
>>> > src/gcc/gimplify.c:419
>>> >0xd39c94 gimplify_assign(tree_node*, tree_node*,
>>> > gimple_statement_base**)
>>> > src/gcc/gimplify.c:9452
>>> >0x130ad18 execute
>>> > src/gcc/tree-stdarg.c:779
>>> >...
>>> >
>>> >The testcase contains this struct:
>>> >...
>>> >struct S { struct T { } a[14]; char b; };
>>> >...
>>> >
>>> >and uses that struct S as type in va_arg:
>>> >...
>>> > arg = va_arg (ap, struct S);
>>> >...
>>> >
>>> >The segfault happens because we're calling is_really_empty_class for
>>> > struct
>>> >S, and TYPE_BINFO is NULL_TREE, which causes BINFO_BASE_ITERATE to
>>> > segfault.
>>> >I'm not sure yet what this issue is or how this is supposed to be fixed.
>>
>> That's probably free_lang_data being more aggressive after Honza
>> fiddled with BINFOs? That is - the gimplifications called from
>> tree-stdarg.c
>> (and others from the middle-end) should never call back into the frontend
>> via langhooks...
>
>
> Hmm, that 'should never' sounds like a missing gcc_assert.
>
> This patch is a way to achieve that gimplification doesn't call the actual
> gimplify_expr langhook, and it fixes the failure. But I'm guessing that's
> not the proper way to fix this.
More like
Index: gcc/tree.c
===
--- gcc/tree.c (revision 220578)
+++ gcc/tree.c (working copy)
@@ -5815,6 +5815,7 @@ free_lang_data (void)
still be used indirectly via the get_alias_set langhook. */
lang_hooks.dwarf_name = lhd_dwarf_name;
lang_hooks.decl_printable_name = gimple_decl_printable_name;
+ lang_hooks.gimplify_expr = lhd_gimplify_expr;
/* We do not want the default decl_assembler_name implementation,
rather if we have fixed everything we want a wrapper around it
asserting that all non-local symbols already got their assembler
> Thanks,
> - Tom
>
> diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
> index 443f6d3..d6cd52d 100644
> --- a/gcc/tree-stdarg.c
> +++ b/gcc/tree-stdarg.c
> @@ -37,6 +37,7 @@ along with GCC; see the file COPYING3. If not see
> #include "input.h"
> #include "function.h"
> #include "langhooks.h"
> +#include "langhooks-def.h"
> #include "gimple-pretty-print.h"
> #include "target.h"
> #include "bitmap.h"
> @@ -734,6 +735,11 @@ pass_stdarg::execute (function *fun)
>const char *funcname = NULL;
>tree cfun_va_list;
>unsigned int retflags = 0;
> + int (*save_gimplify_expr) (tree *, gimple_seq *, gimple_seq *);
> +
> + /* Ensure we don't call language hooks from gimplification. */
> + save_gimplify_expr = lang_hooks.gimplify_expr;
> + lang_hooks.gimplify_expr = lhd_gimplify_expr;
>
>/* Expand va_arg. */
>/* TODO: teach pass_stdarg how process the va_arg builtin, and reverse
> the
> @@ -796,6 +802,9 @@ pass_stdarg::execute (function *fun)
> }
> }
>
> + /* Restore language hook. */
> + lang_hooks.gimplify_expr = save_gimplify_expr;
> +
>if (retflags)
> {
>free_dominance_info (CDI_DOMINATORS);
>
Re: Postpone expanding va_arg until pass_stdarg
Hi, On Tue, 10 Feb 2015, Tom de Vries wrote: > I've added two modifications to gimplify_modify_expr: > - the WITH_SIZE_EXPR in which the CALL_TREE is wrapped, is dropped after > gimplification, but we need the size expression at expansion in pass_stdarg. > So I added the size expression as argument to the internal function. > [ And at pass_stdarg::execute, we wrap the result of > gimplify_va_arg_internal > in a WITH_SIZE_EXPR before generating the assign to the lhs ] Hmm, why do you need the WITH_SIZE_EXPR actually? For variable-sized types returned by va_arg? > - we detect after gimplify_arg (&ap) whether it created a copy ap.1 of ap, > rather than use ap itself, and if so, we copy the value back from ap.1 to ap > after va_arg. My idea was to not generate temporaries and hence copies for non-scalar types, but rather construct the "result" of va_arg directly into the original LHS (that would then also trivially solve the problem of nno-copyable types). > I'm not really sure yet why std_gimplify_va_arg_expr has a part > commented out. Michael, can you comment? I think I did that because of SSA form. The old sequence calculated vatmp = valist; vatmp = vatmp + boundary-1 vatmp = vatmp & -boundary (where the local variable in that function 'valist_tmp' is the tree VAR_DECL 'vatmp') and then continue to use valist_tmp. When in SSA form the gimplifier will rewrite this into: vatmp_1 = valist; vatmp_2 = vatmp_1 + boundary-1 vatmp_3 = vatmp_2 & -boundary but the local valist_tmp variable will continue to be the VAR_DECL, not the vatmp_3 ssa name. Basically whenever one gimplifies a MODIFY_EXPR while in SSA form it's suspicious. So the new code simply build the expression: ((valist + bound-1) & -bound) gimplifies that into an rvalue (most probably an SSA name) and uses that to go on generating code by making valist_tmp be that returned rvalue. I think you'll find that removing that code will make the SSA verifier scream or generate invalid code with -m32 when that hook is used. Ciao, Michael.
Re: Android native build of GCC
/ > >>> So, anyone probing for dlopen() finds it in libfakechroot. >>> However, when that dlopen() is called you get a (very confusing) >>> link error. This is a bug because if the underlying C library does >>> not have dlopen() then libfakechroot should either not export it or >>> should forward calls to the right library (which was libdl.so, I >>> think.) >> >> Out of curiosity (and future libfakechroot porting purposes) how >would >> this look? I know that this and the previous question are off -topic >> to the original email so feel free to leave the list out of your >> reply. > >I'd rather leave it on-list for future reference. The best thing >would be for libfakechroot to be linked against libdl: that way, when >dlopen() was called the link would be correctly satisfied. If that >isn't possible (if dlopen() doesn't work or is incompatible) then >libfakechroot shouldn't export the symbol for dlopen(). > >Andrew. After experimenting with several builds of the fakechroot library I can't see how this would be possible. Even when libdl is linked in, hiding dlopen guarantees that the resulting library doesn't intercept dlopen calls, which breaks the fakechroot environment and removing the fakechroot dlopen code also ensures that dlopen calls aren't intercepted. Cyd -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
