Bug 17855 - [4.0/4.1 Regression] modification of function struct return not diagnosed
Summary: [4.0/4.1 Regression] modification of function struct return not diagnosed
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.0.0
: P2 normal
Target Milestone: 4.0.0
Assignee: Joseph S. Myers
URL:
Keywords: ice-checking, ice-on-valid-code, patch, wrong-code
Depends on:
Blocks: 16989
  Show dependency treegraph
 
Reported: 2004-10-05 23:43 UTC by Richard Henderson
Modified: 2005-03-31 23:42 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-03-30 18:05:58


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Henderson 2004-10-05 23:43:26 UTC
struct foo {char x, y, z[2];};
struct foo f();
void bar(int baz)
{
        f().z[baz] = 1;
}

Generates

  z.c:5: internal compiler error: gimplification failed

But I don't see that this is legal according to [6.5.2.2]/5

  # If an attempt is made to modify the result of a function
  # call ... behaviour is undefined.

If we must accept this construct because it might not be
executed, a possible solution is to notice fallback==fb_lvalue
for a CALL_EXPR in c_gimplify_expr and create a temporary for
the call result, and which will also satisfy the lvalue.

This test has crashed in all versions since at least 2.96.
Comment 1 Joseph S. Myers 2004-10-06 08:40:32 UTC
This does need to be accepted (where warning and compiling into an abort is
one valid form of accepting) in case it is not executed, cf. DR#109:

   A conforming implementation must not fail to translate a strictly
   conforming program simply because *some* possible execution of that
   program would result in undefined behavior.
Comment 2 Andrew Pinski 2004-10-06 13:31:00 UTC
Confirmed, the ICE is a regression though.
: Search converges between 2003-06-04-ssa (#3) and 2003-06-05-ssa (#4).

So it is an old regression from the tree-ssa branch.
Comment 3 Joseph S. Myers 2004-10-28 16:35:31 UTC
RTH suggested:

> If we must accept this construct because it might not be
> executed, a possible solution is to notice fallback==fb_lvalue
> for a CALL_EXPR in c_gimplify_expr and create a temporary for
> the call result, and which will also satisfy the lvalue.

c_gimplify_expr doesn't have the fallback information available.
gimplify_expr does (and there's already code specific to this
sort of thing in gimplify.c), but the obvious fix

+         if (fallback == fb_lvalue)
+           *expr_p = get_initialized_tmp_var (*expr_p, pre_p, post_p);

just moves the ICE to expand_assignment (while changing the testcase
to insert an explicit temporary yields almost identical tree dumps
but without the ICE).

A related example

struct foo {char x, y, z[2];};
struct foo p, q; int r;
void bar(int baz)
{
  (r ? p : q).z[baz] = 1;
}

ICEs in 3.4 as well as on mainline (in expand_assignment in both cases);
a temporary is already created in gimplifying the COND_EXPR.

While if the non-lvalue structure is the result of an assignment, 3.4 ICEs
and mainline doesn't.

struct foo {char x, y, z[2];};
struct foo p, q;
void bar(int baz)
{
  (p = q).z[baz] = 1;
}
Comment 4 Joseph S. Myers 2005-03-30 18:05:58 UTC
Testing a fix.
Comment 5 Joseph S. Myers 2005-03-30 23:07:07 UTC
Patch posted <http://gcc.gnu.org/ml/gcc-patches/2005-03/msg02808.html>, awaiting
review.
Comment 6 Joseph S. Myers 2005-03-31 01:55:43 UTC
The reason I don't consider compiling into an abort to be suitable here is that
the problem could in principle arise with more complicated code where the
pointer in the array reference isn't immediately syntactically from a non-lvalue
array but has been through other assignments, casts, etc., the optimizers then
removing these so that at some point an assignment to such an array needs to be
expanded.  Ultimately I think these cases may most reliably be handled by having
the front end create explicit addressable temporaries for every non-lvalue
structure containing an array (mapping non-lvalue-struct to *(tmp =
non-lvalue-struct, &tmp)), but I think putting the special cases in the
gimplifier makes more sense in the context of fixing now a regression we have now.
Comment 7 Joseph S. Myers 2005-03-31 10:09:18 UTC
[resend of comment#6 which did not get emailed out]

The reason I don't consider compiling into an abort to be suitable here is that
the problem could in principle arise with more complicated code where the
pointer in the array reference isn't immediately syntactically from a non-lvalue
array but has been through other assignments, casts, etc., the optimizers then
removing these so that at some point an assignment to such an array needs to be
expanded.  Ultimately I think these cases may most reliably be handled by having
the front end create explicit addressable temporaries for every non-lvalue
structure containing an array (mapping non-lvalue-struct to *(tmp =
non-lvalue-struct, &tmp)), but I think putting the special cases in the
gimplifier makes more sense in the context of fixing now a regression we have now.
Comment 8 Richard Henderson 2005-03-31 22:16:10 UTC
Patch is ok.  I would prefer that this (eventually) be handled by the front end,
but certainly this approach is appropriate for fixing the regression in 4.0.
Comment 9 GCC Commits 2005-03-31 23:34:49 UTC
Subject: Bug 17855

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	jsm28@gcc.gnu.org	2005-03-31 23:34:45

Modified files:
	gcc            : ChangeLog gimplify.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.c-torture/compile: struct-non-lval-1.c 
	                                     struct-non-lval-2.c 
	                                     struct-non-lval-3.c 

Log message:
	PR c/17855
	* gimplify.c (gimplify_expr): Create a temporary for lvalue
	COND_EXPR and CALL_EXPR.
	
	testsuite:
	* gcc.c-torture/compile/struct-non-lval-1.c,
	gcc.c-torture/compile/struct-non-lval-2.c,
	gcc.c-torture/compile/struct-non-lval-3.c: New tests.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.8061&r2=2.8062
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/gimplify.c.diff?cvsroot=gcc&r1=2.120&r2=2.121
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5254&r2=1.5255
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/compile/struct-non-lval-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/compile/struct-non-lval-2.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/compile/struct-non-lval-3.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 10 GCC Commits 2005-03-31 23:37:22 UTC
Subject: Bug 17855

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	jsm28@gcc.gnu.org	2005-03-31 23:37:11

Modified files:
	gcc            : ChangeLog gimplify.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.c-torture/compile: struct-non-lval-1.c 
	                                     struct-non-lval-2.c 
	                                     struct-non-lval-3.c 

Log message:
	PR c/17855
	* gimplify.c (gimplify_expr): Create a temporary for lvalue
	COND_EXPR and CALL_EXPR.
	
	testsuite:
	* gcc.c-torture/compile/struct-non-lval-1.c,
	gcc.c-torture/compile/struct-non-lval-2.c,
	gcc.c-torture/compile/struct-non-lval-3.c: New tests.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.104&r2=2.7592.2.105
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/gimplify.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.113.2.2&r2=2.113.2.3
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.83&r2=1.5084.2.84
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/compile/struct-non-lval-1.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/compile/struct-non-lval-2.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/compile/struct-non-lval-3.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1

Comment 11 Joseph S. Myers 2005-03-31 23:42:52 UTC
Fixed on 4.0 branch and mainline.