Bug 26004 - [4.1 Regression] gcc errors on valid code [SVO]
Bug#: 26004 Product:  gcc Version: 4.2.0
Host:  Target:  Build: 
Status: RESOLVED Severity: normal Priority: P2
Resolution: FIXED Assigned To: jason@gcc.gnu.org Reported By: fnf@specifix.com
Component: middle-end Target Milestone: 4.1.1
Summary: [4.1 Regression] gcc errors on valid code [SVO]
Keywords:  rejects-valid
Opened: 2006-01-28 13:06
Description:   Last confirmed: 2006-03-09 17:22 Opened: 2006-01-28 13:06

Gcc complains about legal code.  Test case stripped down from a gdb
testsuite test case.

Environment:
System: Linux fishfood.ninemoons.com 2.6.14-1.1656_FC4 #1 Thu Jan 5 22:13:22
EST 2006 i686 i686 i386 GNU/Linux
Architecture: i686
host: i686-pc-linux-gnu
build: i686-pc-linux-gnu
target: i686-pc-linux-gnu
configured with: /src/latest/trunk/src/gcc/configure -v
--prefix=/opt/local/latest/trunk --enable-languages=c,c++
--cache-file=.././config.cache --srcdir=/src/latest/trunk/src/gcc

How-To-Repeat:

$ cat /tmp/bug.c
struct s_1 { short s[1]; } z_1;

struct s_1
add_struct_1 (struct s_1 s)
{
  return s;
}

struct s_1
wack_struct_1 (void)
{
  register struct s_1 u = z_1;
  u = add_struct_1 (u);
  return u;
}

$ gcc -c /tmp/bug.c
/tmp/bug.c: In function ‘wack_struct_1’:
/tmp/bug.c:13: error: address of register variable ‘u’ requested

------- Comment #1 From Andrew Pinski 2006-01-28 15:17 -------
Confirmed, a regression from at least 4.0.2.

------- Comment #2 From Jorn Wolfgang Rennecke 2006-01-30 17:42 -------
Strange - both on mainline and the 4.1 branch, I can reproduce this (albeit
with
a more sensible variable name of "u") for i686-pc-linux-gnu native, but not for
a cross to sh-elf.  Yet the failing mark_addressable call is done for i686 from
the gimplifier.

------- Comment #3 From Andrew Pinski 2006-01-30 18:01 -------
This is caused by the following code in gimplifier:
3297                if (use_target)
3298                  {
3299                    CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
3300                    lang_hooks.mark_addressable (*to_p);
3301                  }


Should we really be calling lang_hooks.mark_addressable, here?

------- Comment #4 From Jorn Wolfgang Rennecke 2006-01-30 18:29 -------
(In reply to comment #3)
> This is caused by the following code in gimplifier:
> 3297                if (use_target)
> 3298                  {
> 3299                    CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
> 3300                    lang_hooks.mark_addressable (*to_p);
> 3301                  }
> 
> 
> Should we really be calling lang_hooks.mark_addressable, here?
> 

I think the problem is actually not testing properly if the value is a
register variable first.

A few lines before in gimplify.c:gimplify_modify_expr_rhs, we have:

            else if (is_gimple_reg_type (TREE_TYPE (*to_p)))
              /* Also don't force regs into memory.  */
              use_target = false;

However, is_gimple_reg_type merely looks if the type is an aggregate.
The test that c-typeck.c:c_mark_addressable uses tests C_DECL_REGISTER .

It appears we actually don't have any way to query from the
frontend-indenpendent code if we can mark something as
safely.

------- Comment #5 From Jorn Wolfgang Rennecke 2006-01-30 18:31 -------
> It appears we actually don't have any way to query from the
> frontend-indenpendent code if we can mark something as
> safely.
 ^ addressable

------- Comment #6 From Jorn Wolfgang Rennecke 2006-01-30 18:38 -------
(In reply to comment #2)
> Strange - both on mainline and the 4.1 branch, I can reproduce this (albeit
> with
> a more sensible variable name of "u") for i686-pc-linux-gnu native, but not for
> a cross to sh-elf.  Yet the failing mark_addressable call is done for i686 from
> the gimplifier.

FWIW, the optimization that malfunctions applies to calls of functions that
return their value in memory.  Because the s_1 type is not returned in memory
on sh-elf, the testcase doesn't trigger there.  For the test to trigger, s_1
has to be an aggregate (to satisfy the gimple test that this is not a register)
which is returned in memory (so that the optimization is relevant), and the
return value must be assigne to a variable that is declared as register. 

------- Comment #7 From Mark Mitchell 2006-02-01 03:14 -------
This is a design bug.  The middle-end should not be calling mark_addressable
hooks in the front ends.  The fix may be to factor the front end
mark_addressable functions into a part that just does the appropriate
marking/recursion for language-dependent tree codes, and a separate part that
enforces semantic constraints.   The former function would be the one available
to the middle end; the latter (which would presumably call the former) would be
used by the front end itself.

This should be a relatively straightforward fix.

------- Comment #8 From Jorn Wolfgang Rennecke 2006-02-15 21:41 -------
Created an attachment (id=10857) [edit]
infrastructure patch - defective

(In reply to comment #7)
> This should be a relatively straightforward fix.

I had a go at this, but ran into a problem.  When an optimizer requests
a variable to be made addressable, I think we need to keep track of this, so
that we don't complain henceforth when the address is taken again, where
addressing the variable once causes further instances of addressing it.

I thought I could reset DECL_REGISTER for this purpose, and accept addressing
if DECL_REGISTER is cleared on entry in c_mark_addressable.

(The C front end uses C_DECL_REGISTER to keep track of a variable being
declared
 register, and DECL_REGISTER more or less means that we intent do use this as a
 register.)

However, this causes a regression on gcc.dg/reg-vol-struct-1.c line 16 - the
DECL_REGISTER flag has already been reset for 'a' on account of it being
volatile.

So it appears that we need another flag to keep track of when a register has
been made addressable on behalf of the optimizers.

------- Comment #9 From Andrew Pinski 2006-02-15 21:47 -------
That patch looks wrong.  There has to be a better way, maybe just rejecting
return slot optimization instead.

------- Comment #10 From Andrew Pinski 2006-02-15 21:48 -------
I will look into fixing this bug later today when I get home.

------- Comment #11 From Mark Mitchell 2006-02-24 00:26 -------
This issue will not be resolved in GCC 4.1.0; retargeted at GCC 4.1.1.

------- Comment #12 From Andrew Pinski 2006-03-07 12:28 -------
*** Bug 26591 has been marked as a duplicate of this bug. ***

------- Comment #13 From Andrew Pinski 2006-03-07 19:52 -------
This is fixed on the mainline how I don't know.

------- Comment #14 From Andrew Pinski 2006-03-07 19:55 -------
Janis could you figure out which patch fixed this on the mainline if it was
ever broken on the mainline.

------- Comment #15 From Andrew Pinski 2006-03-07 21:02 -------
The reduced testcase in this bug is fixed but the full testcase in the gdb
testsuite.

------- Comment #16 From Andrew Pinski 2006-03-07 21:06 -------
New reduced testcase:
struct s_3 { short s[3]; } z_3, s_3;
struct s_3 add_struct_3 (struct s_3 s){}
wack_struct_3 (void)
{
  int i; register struct s_3 u = z_3;
  u = add_struct_3 (u);
}

------- Comment #17 From Janis Johnson 2006-03-08 01:34 -------
A regression hunt on powerpc-linux using the testcase from comment #16
identified this patch:

http://gcc.gnu.org/viewcvs?view=rev&rev=101269

r101269 | jason | 2005-06-23 14:44:21 +0000 (Thu, 23 Jun 2005)

------- Comment #18 From Jason Merrill 2006-03-10 22:47 -------
Subject: Bug 26004

Author: jason
Date: Fri Mar 10 22:47:08 2006
New Revision: 111947

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=111947
Log:
        PR c/26004
        * gimplify.c (gimplify_modify_expr_rhs): Don't do return slot opt if
        the target was declared 'register'.

Added:
    trunk/gcc/testsuite/gcc.dg/pr26004.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimplify.c

------- Comment #19 From Andrew Pinski 2006-03-10 23:20 -------
Fixed on the mainline.

------- Comment #20 From Jason Merrill 2006-03-10 23:42 -------
Subject: Bug 26004

Author: jason
Date: Fri Mar 10 23:42:48 2006
New Revision: 111952

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=111952
Log:
        PR c/26004
        * gimplify.c (gimplify_modify_expr_rhs): Don't do return slot opt if
        the target was declared 'register'.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/gcc.dg/pr26004.c
      - copied unchanged from r111947, trunk/gcc/testsuite/gcc.dg/pr26004.c
Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/gimplify.c

------- Comment #21 From Andrew Pinski 2006-03-11 00:09 -------
Fixed.

------- Comment #22 From Andrew Pinski 2006-06-07 15:38 -------
*** Bug 27932 has been marked as a duplicate of this bug. ***