[Bug c++/39179] New: Wrong code in c++ for const members initialized in external file

[Bug c++/39179] New: Wrong code in c++ for const members initialized in external file

[ktietz at gcc dot gnu dot org]

It seems so that for optimization levels one or higher, gcc produces wrong code
for the two test files below.
If I compile the same test with -O or -Os everything works fine.
On taking a look into produced assembly, it seems that it wrongly assumes the
assert alway have to raise.

File 1: t1.cpp
      //class.h
      struct K {
       static const unsigned k;
      };

      #include <assert.h>

      int main() {
       assert ( K::k == 1 );
       return 1;
      }

File 2: t2.cpp
      struct K {
       static const unsigned k;
      };

      //class.cpp
      const unsigned K::k = 1;

Then compile both file g++ -o tst t1.cpp t2.cpp -O2


-- 
           Summary: Wrong code in c++ for const members initialized in
                    external file
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: ktietz at gcc dot gnu dot org
GCC target triplet: x86_64-pc-mingw32


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #1 from rguenth at gcc dot gnu dot org  2009-02-14 16:55 -------
Single-file testcase, reproduces with a x86_64-pc-mingw32 cross at any
optimization level.  get_symbol_constant_value returns zero for K::k.

struct K {
    static const unsigned k;
};
extern "C" void abort (void);
int main() {
    if ( K::k != 1 )
      abort ();
    return 1;
}


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-02-14 16:55:18
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #2 from rguenth at gcc dot gnu dot org  2009-02-14 17:03 -------
The problem is that targetm.binds_local_p returns true for

 <var_decl 0xb7866000 k
    type <integer_type 0xb785edd0 unsigned int readonly unsigned SI
        size <integer_cst 0xb778b4a4 constant 32>
        unit size <integer_cst 0xb778b1f8 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0xb785edd0 precision 32
min <integer_cst 0xb778b4c8 0> max <integer_cst 0xb778b480 4294967295>>
    readonly used public static unsigned external nonlocal decl_3 decl_5 decl_6
SI file t.ii line 2 col 27 size <integer_cst 0xb778b4a4 32> unit size
<integer_cst 0xb778b1f8 4>
    align 32 context <record_type 0xb785ec30 K>
    chain <type_decl 0xb785ed00 K>>

though probably nobody thought of handling TREE_STATIC && DECL_EXTERNAL being
true at the same time.  Thus, this looks like a possible C++ FE problem to me?

A simple fix would be for i386_pe_binds_local_p to return false if
DECL_EXTERNAL is set, as this is what default_binds_local_p_1 does.
Or better, it should dispatch to default_binds_local_p_1 and only
adjust the shlib flag according to DECL_DLLIMPORT_P.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[ebotcazou at gcc dot gnu dot org]

------- Comment #3 from ebotcazou at gcc dot gnu dot org  2009-02-14 17:16 -------
> The problem is that targetm.binds_local_p returns true for
> 
>  <var_decl 0xb7866000 k
>     type <integer_type 0xb785edd0 unsigned int readonly unsigned SI
>         size <integer_cst 0xb778b4a4 constant 32>
>         unit size <integer_cst 0xb778b1f8 constant 4>
>         align 32 symtab 0 alias set -1 canonical type 0xb785edd0 precision 32
> min <integer_cst 0xb778b4c8 0> max <integer_cst 0xb778b480 4294967295>>
>     readonly used public static unsigned external nonlocal decl_3 decl_5 decl_6
> SI file t.ii line 2 col 27 size <integer_cst 0xb778b4a4 32> unit size
> <integer_cst 0xb778b1f8 4>
>     align 32 context <record_type 0xb785ec30 K>
>     chain <type_decl 0xb785ed00 K>>
> 
> though probably nobody thought of handling TREE_STATIC && DECL_EXTERNAL being
> true at the same time.  Thus, this looks like a possible C++ FE problem to me?

FWIW we had the same problem in Ada on this platform and we fixed Gigi.


-- 

ebotcazou at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #4 from rguenth at gcc dot gnu dot org  2009-02-14 18:48 -------
Btw, this looks like similar to PR36207 which was fixed in the Ada frontend.
Still I think that i386_pe_binds_local_p is completely bogus.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #5 from rguenth at gcc dot gnu dot org  2009-02-14 18:49 -------
Jason, can we fix this in the C++ frontend?  Having both TREE_STATIC and
DECL_EXTERNAL set seems bogus.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at redhat dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[ktietz at gcc dot gnu dot org]

------- Comment #6 from ktietz at gcc dot gnu dot org  2009-02-14 20:10 -------
(In reply to comment #2)
> The problem is that targetm.binds_local_p returns true for
> 
>  <var_decl 0xb7866000 k
>     type <integer_type 0xb785edd0 unsigned int readonly unsigned SI
>         size <integer_cst 0xb778b4a4 constant 32>
>         unit size <integer_cst 0xb778b1f8 constant 4>
>         align 32 symtab 0 alias set -1 canonical type 0xb785edd0 precision 32
> min <integer_cst 0xb778b4c8 0> max <integer_cst 0xb778b480 4294967295>>
>     readonly used public static unsigned external nonlocal decl_3 decl_5 decl_6
> SI file t.ii line 2 col 27 size <integer_cst 0xb778b4a4 32> unit size
> <integer_cst 0xb778b1f8 4>
>     align 32 context <record_type 0xb785ec30 K>
>     chain <type_decl 0xb785ed00 K>>
> 
> though probably nobody thought of handling TREE_STATIC && DECL_EXTERNAL being
> true at the same time.  Thus, this looks like a possible C++ FE problem to me?
> 
> A simple fix would be for i386_pe_binds_local_p to return false if
> DECL_EXTERNAL is set, as this is what default_binds_local_p_1 does.
> Or better, it should dispatch to default_binds_local_p_1 and only
> adjust the shlib flag according to DECL_DLLIMPORT_P.
> 

Right in winnt.c (i386_pe_bind_local_p) should be something like this patch,
but sadly we get then emitted @GOTPCREL, which aren't handled proper by COFF
targets.

Index: gcc/gcc/config/i386/winnt.c
===================================================================
--- gcc.orig/gcc/config/i386/winnt.c
+++ gcc/gcc/config/i386/winnt.c
@@ -321,13 +321,14 @@ i386_pe_encode_section_info (tree decl,
 bool
 i386_pe_binds_local_p (const_tree exp)
 {
+  bool flag = true;
   /* PE does not do dynamic binding.  Indeed, the only kind of
      non-local reference comes from a dllimport'd symbol.  */
   if ((TREE_CODE (exp) == VAR_DECL || TREE_CODE (exp) == FUNCTION_DECL)
       && DECL_DLLIMPORT_P (exp))
-    return false;
+    flag = false;

-  return true;
+  return flag && default_binds_local_p_1 (exp, flag_shlib);
 }


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #7 from rguenth at gcc dot gnu dot org  2009-02-14 21:12 -------
What happens if you just use

return default_binds_local_p_1 (exp, (TREE_CODE (exp) == VAR_DECL || TREE_CODE
(exp) == FUNCTION_DECL)
       && DECL_DLLIMPORT_P (exp));

?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[ktietz at gcc dot gnu dot org]

------- Comment #8 from ktietz at gcc dot gnu dot org  2009-02-14 21:25 -------
(In reply to comment #7)
> What happens if you just use
> 
> return default_binds_local_p_1 (exp, (TREE_CODE (exp) == VAR_DECL || TREE_CODE
> (exp) == FUNCTION_DECL)
>        && DECL_DLLIMPORT_P (exp));
> 
> ?
> 

Same issue @GOTPCREL code gets emitted.
...
        movq    __ZN1K1kE@GOTPCREL(%rip), %rax
        cmpl    $1, (%rax)
...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[nightstrike at gmail dot com]

------- Comment #9 from nightstrike at gmail dot com  2009-02-14 22:56 -------
Verified to fail on win32 and win64, not just win64.  Can someone with
sufficient privileges adjust "Target"?

Verified to work in 4.3, so this is a regression.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] Wrong code in c++ for const members initialized in external file

[ktietz at gcc dot gnu dot org]

-- 

ktietz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
   Target Milestone|---                         |4.4.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #10 from jason at gcc dot gnu dot org  2009-02-17 18:23 -------
Created an attachment (id=17313)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=17313&action=view)
fix to get_symbol_constant_value (untested)

The bug is not in binds_local_p: binds_local_p returns true if the reference
can bind to a different shared object, which is not the case in this example. 
A reference to another translation unit in the same module is still a local
binding.

The problem is that get_symbol_constant_value assumes that TREE_STATIC means
that the variable is defined in the current translation unit, which is not
currently the case for C++.  Apparently this wasn't the case for Ada either,
but that has been corrected.

It seems to me that the obvious fix for 4.4 is to make
get_symbol_constant_value check !DECL_EXTERNAL.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[hjl dot tools at gmail dot com]

------- Comment #11 from hjl dot tools at gmail dot com  2009-02-17 18:29 -------
Will this also fix PR 35501?


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hjl dot tools at gmail dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #12 from rguenth at gcc dot gnu dot org  2009-02-17 18:48 -------
Well, certainly binds_local_p is used for exactly this semantic.  Maybe
mis-used.  A grep over the tree optimizers is in oder to check that.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[hjl dot tools at gmail dot com]

------- Comment #13 from hjl dot tools at gmail dot com  2009-02-17 18:51 -------
binds_local_p is very weak and fragile. See PR 35513.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[rguenth at gcc dot gnu dot org]

------- Comment #14 from rguenth at gcc dot gnu dot org  2009-02-17 18:54 -------
It won't "fix" PR35501, as if there is a DECL_INITIAL we do not care about
visibility or binding in that case in get_symbol_constant_value.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[ebotcazou at gcc dot gnu dot org]

------- Comment #15 from ebotcazou at gcc dot gnu dot org  2009-02-17 18:56 -------
TREE_STATIC && DECL_EXTERNAL doesn't make sense for a VAR_DECL as per tree.h:

/* In a VAR_DECL, nonzero means allocate static storage.
#define TREE_STATIC(NODE) ((NODE)->base.static_flag)

* In a VAR_DECL or FUNCTION_DECL, nonzero means external reference:
   do not allocate storage, and refer to a definition elsewhere.
#define DECL_EXTERNAL(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.decl_flag_2)

That's why the Ada FE has simply stopped to generate this combination.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #16 from jason at gcc dot gnu dot org  2009-02-17 22:56 -------
(In reply to comment #12)
> Well, certainly binds_local_p is used for exactly this semantic.

Perhaps I'm wrong then, and it does mean "binds to a definition in this
translation unit."

The default definition does check DECL_EXTERNAL, I notice; I guess a quick fix
would be to add that to the PE version of the hook.  I'll look at fixing the
C++ front end.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #17 from jason at gcc dot gnu dot org  2009-02-17 23:01 -------
I will note that the C++ front end has been setting TREE_STATIC and
DECL_EXTERNAL on the same variables for many, many years, so changing that
isn't going to be simple.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jakub at gcc dot gnu dot org]

------- Comment #18 from jakub at gcc dot gnu dot org  2009-02-17 23:40 -------
/* True if EXP names an object for which name resolution must resolve
   to the current module.  */

If something uses it to find if it binds to current TU, then it is using it
incorrectly.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #19 from jason at gcc dot gnu dot org  2009-02-18 02:03 -------
I suppose it's a question of what "module" means.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #20 from jason at gcc dot gnu dot org  2009-02-18 17:47 -------
(In reply to comment #19)
> I suppose it's a question of what "module" means.

"module" is used in a lot of different ways, but this usage definitely refers
to the current translation unit:

/* In a VAR_DECL, FUNCTION_DECL, NAMESPACE_DECL or TYPE_DECL,                   
   nonzero means name is to be accessible from outside this module.             
[...]
#define TREE_PUBLIC(NODE) ((NODE)->base.public_flag)

Looking at more uses of binds_local_p and the RTL SYMBOL_FLAG_LOCAL, it does
seem to be primarily used for references to the current translation unit. 
Which means that the i386 back end is using it incorrectly somewhere; we don't
need a GOT lookup for any reference to a symbol in another translation unit.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #21 from jason at gcc dot gnu dot org  2009-02-18 17:51 -------
OTOH, the use of visibility in default_binds_local_p is also wrong under this
interpretation...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #22 from jason at gcc dot gnu dot org  2009-02-18 18:06 -------
Seems like we already had this discussion last year, starting at

http://gcc.gnu.org/ml/gcc/2007-06/msg00848.html

The conclusion there was that binds_local_p means "binds to this
executable/shared library", and the PE definition is correct under that
meaning.  The conclusion in that discussion was that users need to check
DECL_EXTERNAL as well if they want to check whether something will bind to the
current translation unit.  Which leads me back to my patch attached above.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |jason at gcc dot gnu dot org
                   |dot org                     |
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2009-02-14 16:55:18         |2009-02-18 18:10:48
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #23 from jason at gcc dot gnu dot org  2009-02-18 18:31 -------
...and then of course there's the actual documentation:

TARGET_BINDS_LOCAL_P (tree exp)
Returns true if exp names an object for which name resolution
rules must resolve to the current ``module'' (dynamic shared library
or executable image).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #24 from jason at gcc dot gnu dot org  2009-02-18 21:01 -------
Subject: Bug 39179

Author: jason
Date: Wed Feb 18 21:01:03 2009
New Revision: 144270

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=144270
Log:
        PR target/39179
        * tree-ssa-ccp.c (get_symbol_constant_value): Don't assume zero
        value if DECL_EXTERNAL.
        * tree-sra.c (sra_walk_gimple_assign): Likewise.
        * target.h (gcc_target::binds_local_p): Clarify module.
        * tree.h (TREE_PUBLIC): Clarify module.

Added:
    trunk/gcc/testsuite/g++.dg/opt/const6.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cfns.h
    trunk/gcc/cp/ptree.c
    trunk/gcc/target.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/init/const7.C
    trunk/gcc/tree-pretty-print.c
    trunk/gcc/tree-sra.c
    trunk/gcc/tree-ssa-ccp.c
    trunk/gcc/tree.h
    trunk/gcc/varasm.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #25 from jason at gcc dot gnu dot org  2009-02-18 21:09 -------
Fixed.  The C++ static/extern issue has been added as PR 39236.


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[ebotcazou at gcc dot gnu dot org]

------- Comment #26 from ebotcazou at gcc dot gnu dot org  2009-02-18 22:11 -------
> Fixed.  The C++ static/extern issue has been added as PR 39236.

You have installed a lot more things than what's described in the ChangeLog.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179

[Bug target/39179] [4.4 Regression] Wrong code in c++ for const members initialized in external file

[jason at gcc dot gnu dot org]

------- Comment #27 from jason at gcc dot gnu dot org  2009-02-19 01:12 -------
I reverted the mistaken checkins a few seconds later.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39179