Bug 26004 - [4.1 Regression] gcc errors on valid code [SVO]
Summary: [4.1 Regression] gcc errors on valid code [SVO]
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.2.0
: P2 normal
Target Milestone: 4.1.1
Assignee: Jason Merrill
URL:
Keywords: rejects-valid
: 26591 27932 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-01-28 13:06 UTC by fnf@specifix.com
Modified: 2006-06-07 15:38 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.0.2 4.2.0
Known to fail: 4.1.0
Last reconfirmed: 2006-03-09 17:22:00


Attachments
infrastructure patch - defective (4.80 KB, patch)
2006-02-15 21:41 UTC, Jorn Wolfgang Rennecke
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description fnf@specifix.com 2006-01-28 13:06:25 UTC

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 Andrew Pinski 2006-01-28 15:17:42 UTC
Confirmed, a regression from at least 4.0.2.
Comment 2 Jorn Wolfgang Rennecke 2006-01-30 17:42:15 UTC
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 Andrew Pinski 2006-01-30 18:01:27 UTC
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 Jorn Wolfgang Rennecke 2006-01-30 18:29:55 UTC
(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 Jorn Wolfgang Rennecke 2006-01-30 18:31:57 UTC
> 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 Jorn Wolfgang Rennecke 2006-01-30 18:38:50 UTC
(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 Mark Mitchell 2006-02-01 03:14:43 UTC
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 Jorn Wolfgang Rennecke 2006-02-15 21:41:54 UTC
Created attachment 10857 [details]
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 Andrew Pinski 2006-02-15 21:47:38 UTC
That patch looks wrong.  There has to be a better way, maybe just rejecting return slot optimization instead.
Comment 10 Andrew Pinski 2006-02-15 21:48:21 UTC
I will look into fixing this bug later today when I get home.
Comment 11 Mark Mitchell 2006-02-24 00:26:48 UTC
This issue will not be resolved in GCC 4.1.0; retargeted at GCC 4.1.1.
Comment 12 Andrew Pinski 2006-03-07 12:28:41 UTC
*** Bug 26591 has been marked as a duplicate of this bug. ***
Comment 13 Andrew Pinski 2006-03-07 19:52:21 UTC
This is fixed on the mainline how I don't know.
Comment 14 Andrew Pinski 2006-03-07 19:55:25 UTC
Janis could you figure out which patch fixed this on the mainline if it was ever broken on the mainline.
Comment 15 Andrew Pinski 2006-03-07 21:02:32 UTC
The reduced testcase in this bug is fixed but the full testcase in the gdb testsuite.
Comment 16 Andrew Pinski 2006-03-07 21:06:21 UTC
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 Janis Johnson 2006-03-08 01:34:27 UTC
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 Jason Merrill 2006-03-10 22:47:13 UTC
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 Andrew Pinski 2006-03-10 23:20:41 UTC
Fixed on the mainline.
Comment 20 Jason Merrill 2006-03-10 23:42:51 UTC
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 Andrew Pinski 2006-03-11 00:09:59 UTC
Fixed.
Comment 22 Andrew Pinski 2006-06-07 15:38:49 UTC
*** Bug 27932 has been marked as a duplicate of this bug. ***