diff mbox

Limit alignment on error_mark_node variable

Message ID 20150709095228.GA3414@gmail.com
State New
Headers show

Commit Message

H.J. Lu July 9, 2015, 9:52 a.m. UTC 
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.lu@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?

H.J.
---
There is no need to analyze error_mark_node type decls.

gcc/

	PR target/66810
	* varpool.cvarpool.c (varpool_node::analyze): Skip
	error_mark_node type decls.

gcc/testsuite/

	PR target/66810
	* gcc.target/i386/pr66810.c: New test.
---
 gcc/testsuite/gcc.target/i386/pr66810.c | 10 ++++++++++
 gcc/varpool.c                           | 29 +++++++++++++++++------------
 2 files changed, 27 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr66810.c

Comments

Richard Biener July 9, 2015, 9:54 a.m. UTC | #1 
On Thu, Jul 9, 2015 at 11:52 AM, H.J. Lu <hjl.tools@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.lu@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 ;)

Richard.

> H.J.
> ---
> There is no need to analyze error_mark_node type decls.
>
> gcc/
>
>         PR target/66810
>         * varpool.cvarpool.c (varpool_node::analyze): Skip
>         error_mark_node type decls.
>
> gcc/testsuite/
>
>         PR target/66810
>         * gcc.target/i386/pr66810.c: New test.
> ---
>  gcc/testsuite/gcc.target/i386/pr66810.c | 10 ++++++++++
>  gcc/varpool.c                           | 29 +++++++++++++++++------------
>  2 files changed, 27 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr66810.c
>
> 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" } */
> +}
> diff --git a/gcc/varpool.c b/gcc/varpool.c
> index 10fa93c..f7c4d46 100644
> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -514,20 +514,25 @@ varpool_node::get_availability (void)
>  void
>  varpool_node::analyze (void)
>  {
> -  /* When reading back varpool at LTO time, we re-construct the queue in order
> -     to have "needed" list right by inserting all needed nodes into varpool.
> -     We however don't want to re-analyze already analyzed nodes.  */
> -  if (!analyzed)
> +  /* Skip error_mark_node type decls.  */
> +  if (TREE_TYPE (decl) != error_mark_node)
>      {
> -      gcc_assert (!in_lto_p || symtab->function_flags_ready);
> -      /* Compute the alignment early so function body expanders are
> -        already informed about increased alignment.  */
> -      align_variable (decl, 0);
> +      /* When reading back varpool at LTO time, we re-construct the
> +        queue in order to have "needed" list right by inserting all
> +        needed nodes into varpool.  We however don't want to re-analyze
> +        already analyzed nodes.  */
> +      if (!analyzed)
> +       {
> +         gcc_assert (!in_lto_p || symtab->function_flags_ready);
> +         /* Compute the alignment early so function body expanders are
> +            already informed about increased alignment.  */
> +         align_variable (decl, 0);
> +       }
> +      if (alias)
> +       resolve_alias (varpool_node::get (alias_target));
> +      else if (DECL_INITIAL (decl))
> +       record_references_in_initializer (decl, analyzed);
>      }
> -  if (alias)
> -    resolve_alias (varpool_node::get (alias_target));
> -  else if (DECL_INITIAL (decl))
> -    record_references_in_initializer (decl, analyzed);
>    analyzed = true;
>  }
>
> --
> 2.4.3
>
H.J. Lu July 9, 2015, 11:08 a.m. UTC | #2 
On Thu, Jul 9, 2015 at 2:54 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 11:52 AM, H.J. Lu <hjl.tools@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.lu@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?
Richard Biener July 9, 2015, 1:57 p.m. UTC | #3 
On Thu, Jul 9, 2015 at 1:08 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 2:54 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jul 9, 2015 at 11:52 AM, H.J. Lu <hjl.tools@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.lu@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?

I don't see why the C FE would need to invoke finish_decl twice here.
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.

Richard.

> --
> H.J.
H.J. Lu July 9, 2015, 2:06 p.m. UTC | #4 
On Thu, Jul 9, 2015 at 6:57 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 1:08 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Jul 9, 2015 at 2:54 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Jul 9, 2015 at 11:52 AM, H.J. Lu <hjl.tools@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.lu@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?
>
> I don't see why the C FE would need to invoke finish_decl twice here.
> 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.
>

I changed bug report to C and leave it to C FE people.
Richard Biener July 9, 2015, 2:10 p.m. UTC | #5 
On Thu, Jul 9, 2015 at 4:06 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 6:57 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jul 9, 2015 at 1:08 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Jul 9, 2015 at 2:54 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Jul 9, 2015 at 11:52 AM, H.J. Lu <hjl.tools@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.lu@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?
>>
>> I don't see why the C FE would need to invoke finish_decl twice here.
>> 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.
>>
>
> I changed bug report to C and leave it to C FE people.

Thanks.

Richard.

>
> --
> H.J.
diff mbox

Patch

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" } */
+}
diff --git a/gcc/varpool.c b/gcc/varpool.c
index 10fa93c..f7c4d46 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -514,20 +514,25 @@  varpool_node::get_availability (void)
 void
 varpool_node::analyze (void)
 {
-  /* When reading back varpool at LTO time, we re-construct the queue in order
-     to have "needed" list right by inserting all needed nodes into varpool.
-     We however don't want to re-analyze already analyzed nodes.  */
-  if (!analyzed)
+  /* Skip error_mark_node type decls.  */
+  if (TREE_TYPE (decl) != error_mark_node)
     {
-      gcc_assert (!in_lto_p || symtab->function_flags_ready);
-      /* Compute the alignment early so function body expanders are
-	 already informed about increased alignment.  */
-      align_variable (decl, 0);
+      /* When reading back varpool at LTO time, we re-construct the
+	 queue in order to have "needed" list right by inserting all
+	 needed nodes into varpool.  We however don't want to re-analyze
+	 already analyzed nodes.  */
+      if (!analyzed)
+	{
+	  gcc_assert (!in_lto_p || symtab->function_flags_ready);
+	  /* Compute the alignment early so function body expanders are
+	     already informed about increased alignment.  */
+	  align_variable (decl, 0);
+	}
+      if (alias)
+	resolve_alias (varpool_node::get (alias_target));
+      else if (DECL_INITIAL (decl))
+	record_references_in_initializer (decl, analyzed);
     }
-  if (alias)
-    resolve_alias (varpool_node::get (alias_target));
-  else if (DECL_INITIAL (decl))
-    record_references_in_initializer (decl, analyzed);
   analyzed = true;
 }