Bug 67550 - [5/6 regression] Initialization of local struct array with elements of global array yields zeros instead of initializer values
Summary: [5/6 regression] Initialization of local struct array with elements of global...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 5.2.1
: P1 normal
Target Milestone: 5.4
Assignee: Jason Merrill
URL:
Keywords: wrong-code
: 69901 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-09-11 14:08 UTC by Florian Knauf
Modified: 2016-02-24 07:55 UTC (History)
8 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.9.3
Known to fail: 5.2.1, 6.0
Last reconfirmed: 2015-09-11 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Knauf 2015-09-11 14:08:12 UTC
Initializing a function-local struct array from values in a global struct array yields wrong values with gcc 5.2.1. The problem can be reproduced with the following code:

    #include <iostream>
    #include <limits>

    struct S {
      int x;
      int y;
    };

    S const data[] = {
      { 1, std::numeric_limits<int>::max() }
    };

    int main() {
      S data2[] = {
        data[0]
      };

      std::cout
        << data [0].x << ", " << data [0].y << "\n"
        << data2[0].x << ", " << data2[0].y << "\n"
        ;
    }

The output is:

    $ g++ -Wall -Wextra -pedantic foo.cc
    $ ./a.out 
    1, 2147483647
    1, 0

The expected output was:

    1, 2147483647
    1, 2147483647

I am using gcc 5.2.1 on Debian sid (Linux 4.1.3) for x86-64. The error is also reproducible with gcc 5.2.0 on Arch Linux (also x86-64) and gcc 5.1.1 on Fedora 22 (also x86-64). It is not reproducible with gcc 4.9.3.

A workaround for the error is to remove the "const" keyword from the declaration of "data", i.e.

    S data[] = {
      { 1, std::numeric_limits<int>::max() }
    };

The error also does not appear if std::numeric_limits<int>::max() is replaced with a compile-time constant value (such as 42). Consequently, in C++11 mode  (where std::numeric_limits<int>::max() is constexpr) the above code does not reproduce the error. Instead,

    int foo() { return 23; }

    S const data[] = {
      { 1, foo() }
    };

can be used to produce the wrong result. With

    int constexpr foo() { return 23; }

the correct result is produced.
Comment 1 Markus Trippelsdorf 2015-09-11 15:28:31 UTC
Also happens with a local array:

struct S {
  int x;
  int y;
};
int foo() { return 1; }

int main() {
  S const data[] = {{0, foo()}};

  S data2[] = {data[0]};

  if (!data2[0].y)
    __builtin_abort();
}
Comment 2 Markus Trippelsdorf 2015-09-11 15:58:54 UTC
Started with r217814.
Comment 3 Jason Wyatt 2015-11-20 16:39:44 UTC
Similarly:

#include <iostream>

struct TestStruct
{
    int m1;
    int m2;
};

int main()
{
    int testValue = 1;
    const TestStruct var = { testValue, 2 };
    TestStruct array[1] = {
        var
    };
    
    std::cout << "var: " << var.m1 << "," << var.m2 << std::endl;
    std::cout << "array[0]: " << array[0].m1 << "," << array[0].m2 << std::endl;
    
    return 0;
}

produces:

var: 1,2                                                                   
array[0]: 0,2

(gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC))
Comment 4 Jason Wyatt 2015-11-23 11:27:14 UTC
It appears that while parsing the initialiser for the array, maybe_constant_init switches the var for a constructor. This constructor only sets the m2 member variable. You can see the result in the gimple it produces:

        testValue = 1;
        var = {};
        var.m2 = 2;
        var.m1 = testValue;
        array = {};
        array[0].m2 = 2;
Comment 5 Jason Wyatt 2015-11-23 18:27:09 UTC
When parsing the initialisation of const TestStruct var:
  store_init_value ends up calling split_nonconstant_init, so that only the constant part of the initialisation of var is stored in DECL_INITIAL(t).

Then when parsing the initialisation of the array, maybe_constant_init eventually calls through to decl_constant_value(t), i.e. constant_value_1(t, false, true). That then uses DECL_INITIAL(t) as if it were the whole initialisation, rather than just the constant part, hence all the non constant parts aren't initialised correctly.

I've got no idea how this is supposed to work though, presumably at some point in the chain of calls it's supposed to realise this is not a constant?
Comment 6 Jason Wyatt 2015-11-24 11:31:00 UTC
Adding a TREE_READONLY (to match TREE_READONLY being unset in split_nonconstant_init) check seems to fix the generated gimple. This is total guesswork though - I have no idea what side effects this change might have.

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index ac11224..ee0405d 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2038,7 +2038,8 @@ constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
         || (strict_p
             ? decl_constant_var_p (decl)
             : (VAR_P (decl)
-               && CP_TYPE_CONST_NON_VOLATILE_P (TREE_TYPE (decl)))))
+               && CP_TYPE_CONST_NON_VOLATILE_P (TREE_TYPE (decl))
+               && TREE_READONLY (decl))))
     {
       tree init;
       /* If DECL is a static data member in a template
Comment 7 Richard Biener 2015-12-04 10:46:43 UTC
GCC 5.3 is being released, adjusting target milestone.
Comment 8 Patrick Palka 2015-12-10 23:52:02 UTC
(In reply to Jason Wyatt from comment #6)
> Adding a TREE_READONLY (to match TREE_READONLY being unset in
> split_nonconstant_init) check seems to fix the generated gimple. This is
> total guesswork though - I have no idea what side effects this change might
> have.
> 
> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> index ac11224..ee0405d 100644
> --- a/gcc/cp/init.c
> +++ b/gcc/cp/init.c
> @@ -2038,7 +2038,8 @@ constant_value_1 (tree decl, bool strict_p, bool
> return_aggregate_cst_ok_p)
>          || (strict_p
>              ? decl_constant_var_p (decl)
>              : (VAR_P (decl)
> -               && CP_TYPE_CONST_NON_VOLATILE_P (TREE_TYPE (decl)))))
> +               && CP_TYPE_CONST_NON_VOLATILE_P (TREE_TYPE (decl))
> +               && TREE_READONLY (decl))))
>      {
>        tree init;
>        /* If DECL is a static data member in a template

Your fix makes sense to me though I am no expert.
Comment 9 Jason Merrill 2015-12-17 14:54:41 UTC
Why was a wrong-code regression only P2?
Comment 10 Jason Merrill 2015-12-17 16:52:29 UTC
Author: jason
Date: Thu Dec 17 16:51:58 2015
New Revision: 231777

URL: https://gcc.gnu.org/viewcvs?rev=231777&root=gcc&view=rev
Log:
	PR c++/67550

	* init.c (constant_value_1): Don't return a CONSTRUCTOR missing
	non-constant elements.

Added:
    trunk/gcc/testsuite/g++.dg/init/aggr13.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/init.c
Comment 11 Jason Merrill 2015-12-17 16:52:42 UTC
Author: jason
Date: Thu Dec 17 16:52:10 2015
New Revision: 231779

URL: https://gcc.gnu.org/viewcvs?rev=231779&root=gcc&view=rev
Log:
	PR c++/67550

	* init.c (constant_value_1): Don't return a CONSTRUCTOR missing
	non-constant elements.

Added:
    branches/gcc-5-branch/gcc/testsuite/g++.dg/init/aggr13.C
Modified:
    branches/gcc-5-branch/gcc/cp/ChangeLog
    branches/gcc-5-branch/gcc/cp/init.c
Comment 12 Jason Merrill 2015-12-17 17:47:52 UTC
Fixed.
Comment 13 Andrew Pinski 2016-02-24 07:55:49 UTC
*** Bug 69901 has been marked as a duplicate of this bug. ***