On Fri, Jul 10, 2015 at 2:19 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Thu, Jul 09, 2015 at 03:57:31PM +0200, Richard Biener wrote:
>> On Thu, Jul 9, 2015 at 1:08 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
>> > On Thu, Jul 9, 2015 at 2:54 AM, Richard Biener
>> > <richard.guent...@gmail.com> wrote:
>> >> On Thu, Jul 9, 2015 at 11:52 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
>> >>> On Thu, Jul 09, 2015 at 10:16:38AM +0200, Richard Biener wrote:
>> >>>> On Wed, Jul 8, 2015 at 5:32 PM, H.J. Lu <hongjiu...@intel.com> wrote:
>> >>>> > There is no need to try different alignment on variable of
>> >>>> > error_mark_node.
>> >>>> >
>> >>>> > OK for trunk if there is no regression?
>> >>>>
>> >>>> Can't we avoid calling align_variable on error_mark_node type decls
>> >>>> completely? That is, punt earlier when we try to emit it.
>> >>>>
>> >>>
>> >>> How about this? OK for trunk?
>> >>
>> >> Heh, you now get the obvious question why we can't simply avoid
>> >> adding the varpool node in the first place ;)
>> >>
>> >
>> > When it was first added to varpool, its type was OK:
>> >
>> > (gdb) bt
>> > #0 varpool_node::get_create (decl=<var_decl 0x7ffff1506900 vv>)
>> > at /export/gnu/import/git/sources/gcc/gcc/varpool.c:150
>> > #1 0x0000000000e1c3e8 in rest_of_decl_compilation (
>> > decl=<var_decl 0x7ffff1506900 vv>, top_level=1, at_end=0)
>> > at /export/gnu/import/git/sources/gcc/gcc/passes.c:271
>> > #2 0x0000000000731d39 in finish_decl (decl=<var_decl 0x7ffff1506900 vv>,
>> > init_loc=0, init=<tree 0x0>, origtype=<tree 0x0>, asmspec_tree=<tree
>> > 0x0>)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-decl.c:4863
>> > #3 0x000000000078d1ed in c_parser_declaration_or_fndef (
>> > parser=0x7ffff15050a8, fndef_ok=false, static_assert_ok=true,
>> > empty_ok=true, nested=false, start_attr_ok=true,
>> > objc_foreach_object_declaration=0x0, omp_declare_simd_clauses=...)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1855
>> > #4 0x000000000078c234 in c_parser_external_declaration
>> > (parser=0x7ffff15050a8)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1435
>> > #5 0x000000000078be45 in c_parser_translation_unit (parser=0x7ffff15050a8)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1322
>> > #6 0x00000000007b3271 in c_parse_file ()
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:15440
>> > #7 0x000000000081cb97 in c_common_parse_file ()
>> > at /export/gnu/import/git/sources/gcc/gcc/c-family/c-opts.c:1059
>> > #8 0x0000000000f27662 in compile_file ()
>> > at /export/gnu/import/git/sources/gcc/gcc/toplev.c:543
>> > ---Type <return> to continue, or q <return> to quit---
>> > #9 0x0000000000f29baa in do_compile ()
>> > at /export/gnu/import/git/sources/gcc/gcc/toplev.c:2041
>> > #10 0x0000000000f29df9 in toplev::main (this=0x7fffffffdc90, argc=17,
>> > argv=0x7fffffffdd98)
>> > at /export/gnu/import/git/sources/gcc/gcc/toplev.c:2142
>> > #11 0x00000000017d8228 in main (argc=17, argv=0x7fffffffdd98)
>> > at /export/gnu/import/git/sources/gcc/gcc/main.c:39
>> >
>> > Later, it was turned into error_mark_node:
>> >
>> > Old value = <array_type 0x7ffff15ee150>
>> > New value = <error_mark 0x7ffff14fac90>
>> > finish_decl (decl=<var_decl 0x7ffff1506990 a>, init_loc=0, init=<tree 0x0>,
>> > origtype=<tree 0x0>, asmspec_tree=<tree 0x0>)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-decl.c:4802
>> > 4802 if (TREE_USED (type))
>> > (gdb) bt
>> > #0 finish_decl (decl=<var_decl 0x7ffff1506990 a>, init_loc=0,
>> > init=<tree 0x0>, origtype=<tree 0x0>, asmspec_tree=<tree 0x0>)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-decl.c:4802
>> > #1 0x000000000078d1ed in c_parser_declaration_or_fndef (
>> > parser=0x7ffff15050a8, fndef_ok=false, static_assert_ok=true,
>> > empty_ok=true, nested=true, start_attr_ok=true,
>> > objc_foreach_object_declaration=0x0, omp_declare_simd_clauses=...)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1855
>> > #2 0x0000000000792a23 in c_parser_compound_statement_nostart (
>> > parser=0x7ffff15050a8)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:4621
>> > #3 0x0000000000792688 in c_parser_compound_statement
>> > (parser=0x7ffff15050a8)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:4532
>> > #4 0x000000000078d5a3 in c_parser_declaration_or_fndef (
>> > parser=0x7ffff15050a8, fndef_ok=true, static_assert_ok=true,
>> > empty_ok=true, nested=false, start_attr_ok=true,
>> > objc_foreach_object_declaration=0x0, omp_declare_simd_clauses=...)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1965
>> > #5 0x000000000078c234 in c_parser_external_declaration
>> > (parser=0x7ffff15050a8)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1435
>> > #6 0x000000000078be45 in c_parser_translation_unit (parser=0x7ffff15050a8)
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:1322
>> > #7 0x00000000007b3271 in c_parse_file ()
>> > ---Type <return> to continue, or q <return> to quit---
>> > at /export/gnu/import/git/sources/gcc/gcc/c/c-parser.c:15440
>> > #8 0x000000000081cb97 in c_common_parse_file ()
>> > at /export/gnu/import/git/sources/gcc/gcc/c-family/c-opts.c:1059
>> > #9 0x0000000000f27662 in compile_file ()
>> > at /export/gnu/import/git/sources/gcc/gcc/toplev.c:543
>> > #10 0x0000000000f29baa in do_compile ()
>> > at /export/gnu/import/git/sources/gcc/gcc/toplev.c:2041
>> > #11 0x0000000000f29df9 in toplev::main (this=0x7fffffffdc90, argc=17,
>> > argv=0x7fffffffdd98)
>> > at /export/gnu/import/git/sources/gcc/gcc/toplev.c:2142
>> > #12 0x00000000017d8228 in main (argc=17, argv=0x7fffffffdd98)
>> > at /export/gnu/import/git/sources/gcc/gcc/main.c:39
>> > (gdb)
>> >
>> > I guess it isn't worth to take it out of varpool.
>> >
>> > Is my patch OK for trunk?
>>
>
> Let me give it another try.
>
>> I don't see why the C FE would need to invoke finish_decl twice here.
>
> finish_decl is called once on
>
> int vv;
>
> and the second time on
>
> static int a[vv];
>
> which leads to error_mark_node decl.
>
>> So I'd rather
>> have the C frontend not invoke rest_of_decl_compilation on this in the
>> first place.
>>
>> Your patch still feels like a hack in the wrong place.
>>
>
> This patch avoids calling varpool_node::finalize_decl on error_mark_node
> type decls. Does it make sense?
I think that makes sense (though I wonder why this loop is here and not, say,
in cgraph_node::finalize_function - but that's for somebody else to cleanup).
Honza?
Richard.
>
> H.J.
> --
> gcc/
>
> PR target/66810
> * cgraphbuild.c (pass_build_cgraph_edges::execute): Skip
> error_mark_node type decls.
>
> gcc/testsuite/
>
> PR target/66810
> * gcc.target/i386/pr66810.c: New test.
> ---
> gcc/cgraphbuild.c | 3 ++-
> gcc/testsuite/gcc.target/i386/pr66810.c | 10 ++++++++++
> 2 files changed, 12 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr66810.c
>
> diff --git a/gcc/cgraphbuild.c b/gcc/cgraphbuild.c
> index 7d2d096..4d679d9 100644
> --- a/gcc/cgraphbuild.c
> +++ b/gcc/cgraphbuild.c
> @@ -381,7 +381,8 @@ pass_build_cgraph_edges::execute (function *fun)
> FOR_EACH_LOCAL_DECL (fun, ix, decl)
> if (TREE_CODE (decl) == VAR_DECL
> && (TREE_STATIC (decl) && !DECL_EXTERNAL (decl))
> - && !DECL_HAS_VALUE_EXPR_P (decl))
> + && !DECL_HAS_VALUE_EXPR_P (decl)
> + && TREE_TYPE (decl) != error_mark_node)
> varpool_node::finalize_decl (decl);
> record_eh_tables (node, fun);
>
> diff --git a/gcc/testsuite/gcc.target/i386/pr66810.c
> b/gcc/testsuite/gcc.target/i386/pr66810.c
> new file mode 100644
> index 0000000..4778b37
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr66810.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-mno-sse -mno-mmx -miamcu" } */
> +
> +int vv;
> +
> +void
> +i (void)
> +{
> + static int a[vv]; /* { dg-error "storage size" } */
> +}
> --
> 2.4.3
>
