Ping: PATCH to generate error for address of cast

Ping: PATCH to generate error for address of cast

[Matt Austern]

I sent this patch out a week or so ago, and it hasn't been reviewed.
Sending it again...

And as long as I was resending the patch anyway, I've also written
a couple of test cases  for it: one to make sure that I didn't break
the cast-as-lvalue extension, one to verify that the patch does what
I claim, and generates an error message when you try to take the
address of a cast.  Sorry, I don't know how to get 'cvs diff' to include
new files when I don't have write access to the repository, so I'm
just appending those two files on their own.

(By the way, you might be wondering why I'm doing all this
complicated stuff instead of just fixing lvalue_or_else.  Answer:
I found that changing lvalue_or_else breaks the case-as-lvalue
extension in a couple places.  Having a regression test suite is
nice.)

			--Matt


Index: gcc/cp/typeck.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/cp/typeck.c,v
retrieving revision 1.421
diff -u -r1.421 typeck.c
--- gcc/cp/typeck.c     9 Aug 2002 19:57:01 -0000       1.421
+++ gcc/cp/typeck.c     14 Aug 2002 17:25:58 -0000
@@ -4237,8 +4237,14 @@
          is an error.  */
        else if (TREE_CODE (argtype) != FUNCTION_TYPE
                && TREE_CODE (argtype) != METHOD_TYPE
-              && !lvalue_or_else (arg, "unary `&'"))
-       return error_mark_node;
+              && (!lvalue_p (arg) ||
+                  (TREE_CODE (arg) == NOP_EXPR &&
+                   !same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (arg)),
+                                 TYPE_MAIN_VARIANT (TREE_TYPE 
(TREE_OPERAND (arg, 0)))))))
+       {
+         error ("non-lvalue in unary `&'");
+         return error_mark_node;
+       }

        if (argtype != error_mark_node)
         argtype = build_pointer_type (argtype);




========================================
bash-2.05$ cat gcc/testsuite/g++.dg/expr/lvaddr.C
// Copyright (C) 2002 Free Software Foundation
// Contributed by Matt Austern <austern@apple.com>

// { dg-do compile }

void f()
{
   int n;
   char* p = &(char) n;          // { dg-error "non-lvalue" }
}



========================================
bash-2.05$ cat gcc/testsuite/g++.dg/expr/lvcast.C
// Copyright (C) 2002 Free Software Foundation
// Contributed by Matt Austern <austern@apple.com>

// { dg-do compile }

void f()
{
   int n;
   static_cast<char>(n) = 0;
   (char) n = 1;
}

Re: Ping: PATCH to generate error for address of cast

[Nathan Sidwell]

Matt Austern wrote:

> (By the way, you might be wondering why I'm doing all this
> complicated stuff instead of just fixing lvalue_or_else.  Answer:
> I found that changing lvalue_or_else breaks the case-as-lvalue
> extension in a couple places.  Having a regression test suite is
What are the semantics for cast-is-lvalue extension? (why is &(t)thing
not allowed, but some other lvalue use is ok?)

IMHO It would be better to add another parm to lvalue_or_else, and 
fix it there - there may be other places which are erroneously
allowing lvalue casts.

(I bet we can't kill the extension ... )

nathan
-- 
Dr Nathan Sidwell   ::   http://www.codesourcery.com   ::   CodeSourcery LLC
         'But that's a lie.' - 'Yes it is. What's your point?'
nathan@codesourcery.com : http://www.cs.bris.ac.uk/~nathan/ : nathan@acm.org

Re: Ping: PATCH to generate error for address of cast

[Gabriel Dos Reis]

Nathan Sidwell <nathan@codesourcery.com> writes:

| (I bet we can't kill the extension ... )

Well, we can give it a try.  Let's first deprecate it.

-- Gaby

Re: Ping: PATCH to generate error for address of cast

[Matt Austern]

On Wednesday, August 14, 2002, at 10:54 AM, Nathan Sidwell wrote:

> Matt Austern wrote:
>
>> (By the way, you might be wondering why I'm doing all this
>> complicated stuff instead of just fixing lvalue_or_else.  Answer:
>> I found that changing lvalue_or_else breaks the case-as-lvalue
>> extension in a couple places.  Having a regression test suite is
> What are the semantics for cast-is-lvalue extension? (why is &(t)thing
> not allowed, but some other lvalue use is ok?)

I don't know.  In my opinion, the cast-is-lvalue extension is poorly
documented.  But the documentation does clearly say that even
with the extension it's incorrect to take the address of a cast, and
that's how the C front end behaves.  The C++ front end was clearly
trying to do that too; as Gaby noticed, this is obviously a cut-and-
paste error.

I preferred a narrow change, rather than anything involving
lvalue_or_else or lvalue_p, precisely because I didn't understand
the complete specification of the cast-is-lvalue extension.  I wanted
to fix one very clear deviation from the closest thing we've got to a
specification.

I agree that getting rid of underspecified extensions would in
general be A Good Thing.  I'd like to keep that separate, though.

			--Matt

Re: Ping: PATCH to generate error for address of cast

[Fergus Henderson]

On 14-Aug-2002, Nathan Sidwell <nathan@codesourcery.com> wrote:
> Matt Austern wrote:
> 
> > (By the way, you might be wondering why I'm doing all this
> > complicated stuff instead of just fixing lvalue_or_else.  Answer:
> > I found that changing lvalue_or_else breaks the case-as-lvalue
> > extension in a couple places.  Having a regression test suite is
> What are the semantics for cast-is-lvalue extension? (why is &(t)thing
> not allowed, but some other lvalue use is ok?)
>
> IMHO It would be better to add another parm to lvalue_or_else, and 
> fix it there - there may be other places which are erroneously
> allowing lvalue casts.
> 
> (I bet we can't kill the extension ... )

I would oppose deprecating or removing this extension --
because my code uses it! ;-)

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.

Re: Ping: PATCH to generate error for address of cast

[Jason Merrill]

On Fri, 16 Aug 2002 00:24:57 +1000, Fergus Henderson <fjh@cs.mu.OZ.AU> wrote:

> On 14-Aug-2002, Nathan Sidwell <nathan@codesourcery.com> wrote:
>> Matt Austern wrote:
>> 
>> > (By the way, you might be wondering why I'm doing all this
>> > complicated stuff instead of just fixing lvalue_or_else.  Answer:
>> > I found that changing lvalue_or_else breaks the case-as-lvalue
>> > extension in a couple places.  Having a regression test suite is
>> What are the semantics for cast-is-lvalue extension? (why is &(t)thing
>> not allowed, but some other lvalue use is ok?)
>>
>> IMHO It would be better to add another parm to lvalue_or_else, and 
>> fix it there - there may be other places which are erroneously
>> allowing lvalue casts.
>> 
>> (I bet we can't kill the extension ... )
>
> I would oppose deprecating or removing this extension --
> because my code uses it! ;-)

The extension is obsolete in C++; references are a better solution to the
same problem.  Indeed, we already try to pedwarn about it in
build_modify_expr, but currently don't, presumably because of this same
bug.  I think that continuing to support it with an unconditional pedwarn
is the right answer.

There's certainly no reason to support it with new-style casts.

Matt, I think this bug is in lvalue_p, and should be fixed there.  Also,
there is already special handling for NOP_EXPR a few lines before, though
it seems redundant.

The tests are OK, except that tests for extensions go in g++.dg/ext, and
the positive test needs -fpermissive.

Jason

Re: Ping: PATCH to generate error for address of cast

[Matt Austern]

On Tuesday, August 20, 2002, at 10:44 AM, Jason Merrill wrote:

> Matt, I think this bug is in lvalue_p, and should be fixed there.  
> Also,
> there is already special handling for NOP_EXPR a few lines before, 
> though
> it seems redundant.
>
> The tests are OK, except that tests for extensions go in g++.dg/ext, 
> and
> the positive test needs -fpermissive.

OK, I'll change the tests as suggested.

In principle I agree with you about lvalue_p.  In practice there's
an annoying technical issue: we can't just change lvalue_p
unconditionally, because places that use this extension need the
old behavior.  We also can't change lvalue_p's interface,
because lvalue_p is declared in tree.h, i.e. it's common to all
front ends.  We want to make a C++-only change.

My proposed solution: add new functions, non_cast_lvalue_p and
non_cast_lvalue_or_else, in the C++ front end only.  Annoying and
none too pretty, but I don't see a better alternative.

And I agree with you about a pedwarn for use of this extension.
I'm not going to deal with that just yet, though.

			--Matt

Re: Ping: PATCH to generate error for address of cast

[Jason Merrill]

On Tue, 20 Aug 2002 10:57:27 -0700, Matt Austern <austern@apple.com> wrote:

> My proposed solution: add new functions, non_cast_lvalue_p and
> non_cast_lvalue_or_else, in the C++ front end only.  Annoying and
> none too pretty, but I don't see a better alternative.

Seems like a reasonable interim solution.

Jason

Re: Ping: PATCH to generate error for address of cast

[Matt Austern]

On Tuesday, August 20, 2002, at 02:52 PM, Jason Merrill wrote:

> On Tue, 20 Aug 2002 10:57:27 -0700, Matt Austern <austern@apple.com> 
> wrote:
>
>> My proposed solution: add new functions, non_cast_lvalue_p and
>> non_cast_lvalue_or_else, in the C++ front end only.  Annoying and
>> none too pretty, but I don't see a better alternative.
>
> Seems like a reasonable interim solution.

OK!  Here's my latest version, then.

			--Matt

         * gcc/cp/cp-tree.h, gcc/cp/tree.c: Introduced new functions
           non_cast_lvalue_p and non_cast_lvalue_or_else.  Same as
           lvalue_p and lvalue_or_else except that they don't
           treat casts as lvalues.  (cast-as-lvalue is a GNU extension.)
         * gcc/cp/typeck.c: Emit an error message for address of cast.

bash-2.05$ cvs diff -u
? gcc/testsuite/g++.dg/ext/lvaddr.C
? gcc/testsuite/g++.dg/ext/lvcast.C
Index: gcc/cp/cp-tree.h
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/cp/cp-tree.h,v
retrieving revision 1.744
diff -u -r1.744 cp-tree.h
--- gcc/cp/cp-tree.h	9 Aug 2002 19:57:00 -0000	1.744
+++ gcc/cp/cp-tree.h	20 Aug 2002 21:43:37 -0000
@@ -4184,6 +4184,8 @@
  extern void unshare_base_binfos			PARAMS ((tree));
  extern int member_p				PARAMS ((tree));
  extern cp_lvalue_kind real_lvalue_p		PARAMS ((tree));
+extern cp_lvalue_kind non_cast_lvalue_p		PARAMS ((tree));
+extern int non_cast_lvalue_or_else		PARAMS ((tree, const char *));
  extern tree build_min				PARAMS ((enum tree_code, tree,
  							 ...));
  extern tree build_min_nt			PARAMS ((enum tree_code, ...));
Index: gcc/cp/tree.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/cp/tree.c,v
retrieving revision 1.295
diff -u -r1.295 tree.c
--- gcc/cp/tree.c	11 Aug 2002 16:05:27 -0000	1.295
+++ gcc/cp/tree.c	20 Aug 2002 21:43:37 -0000
@@ -39,7 +39,7 @@
  static int list_hash_eq PARAMS ((const void *, const void *));
  static hashval_t list_hash_pieces PARAMS ((tree, tree, tree));
  static hashval_t list_hash PARAMS ((const void *));
-static cp_lvalue_kind lvalue_p_1 PARAMS ((tree, int));
+static cp_lvalue_kind lvalue_p_1 PARAMS ((tree, int, int));
  static tree no_linkage_helper PARAMS ((tree *, int *, void *));
  static tree build_srcloc PARAMS ((const char *, int));
  static tree mark_local_for_remap_r PARAMS ((tree *, int *, void *));
@@ -59,9 +59,10 @@
     non-zero, rvalues of class type are considered lvalues.  */

  static cp_lvalue_kind
-lvalue_p_1 (ref, treat_class_rvalues_as_lvalues)
+lvalue_p_1 (ref, treat_class_rvalues_as_lvalues, allow_cast_as_lvalue)
       tree ref;
       int treat_class_rvalues_as_lvalues;
+     int allow_cast_as_lvalue;
  {
    cp_lvalue_kind op1_lvalue_kind = clk_none;
    cp_lvalue_kind op2_lvalue_kind = clk_none;
@@ -84,16 +85,28 @@
      case WITH_CLEANUP_EXPR:
      case REALPART_EXPR:
      case IMAGPART_EXPR:
-      /* This shouldn't be here, but there are lots of places in the 
compiler
-         that are sloppy about tacking on NOP_EXPRs to the same type 
when
-	 no actual conversion is happening.  */
-    case NOP_EXPR:
        return lvalue_p_1 (TREE_OPERAND (ref, 0),
-			 treat_class_rvalues_as_lvalues);
+			 treat_class_rvalues_as_lvalues,
+			 allow_cast_as_lvalue);
+
+    case NOP_EXPR:
+      /* If expression doesn't change the type, we consider it as an
+	 lvalue even when cast_as_lvalue extension isn't selected.  That's
+	 because parts of the compiler are alleged to be sloppy about sticking
+	 in NOP_EXPR node for no good reason. */
+      if (allow_cast_as_lvalue ||
+	  same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (ref)),
+		       TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (ref, 0)))))
+	return lvalue_p_1 (TREE_OPERAND (ref, 0),
+			   treat_class_rvalues_as_lvalues,
+			   allow_cast_as_lvalue);
+      else
+	return clk_none;

      case COMPONENT_REF:
        op1_lvalue_kind = lvalue_p_1 (TREE_OPERAND (ref, 0),
-				    treat_class_rvalues_as_lvalues);
+				    treat_class_rvalues_as_lvalues,
+				    allow_cast_as_lvalue);
        if (op1_lvalue_kind
  	  /* The "field" can be a FUNCTION_DECL or an OVERLOAD in some
  	     situations.  */
@@ -134,16 +147,20 @@
      case MAX_EXPR:
      case MIN_EXPR:
        op1_lvalue_kind = lvalue_p_1 (TREE_OPERAND (ref, 0),
-				    treat_class_rvalues_as_lvalues);
+				    treat_class_rvalues_as_lvalues,
+				    allow_cast_as_lvalue);
        op2_lvalue_kind = lvalue_p_1 (TREE_OPERAND (ref, 1),
-				    treat_class_rvalues_as_lvalues);
+				    treat_class_rvalues_as_lvalues,
+				    allow_cast_as_lvalue);
        break;

      case COND_EXPR:
        op1_lvalue_kind = lvalue_p_1 (TREE_OPERAND (ref, 1),
-				    treat_class_rvalues_as_lvalues);
+				    treat_class_rvalues_as_lvalues,
+				    allow_cast_as_lvalue);
        op2_lvalue_kind = lvalue_p_1 (TREE_OPERAND (ref, 2),
-				    treat_class_rvalues_as_lvalues);
+				    treat_class_rvalues_as_lvalues,
+				    allow_cast_as_lvalue);
        break;

      case MODIFY_EXPR:
@@ -151,7 +168,8 @@

      case COMPOUND_EXPR:
        return lvalue_p_1 (TREE_OPERAND (ref, 1),
-			 treat_class_rvalues_as_lvalues);
+			 treat_class_rvalues_as_lvalues,
+			 allow_cast_as_lvalue);

      case TARGET_EXPR:
        return treat_class_rvalues_as_lvalues ? clk_class : clk_none;
@@ -196,9 +214,20 @@
  real_lvalue_p (ref)
       tree ref;
  {
-  return lvalue_p_1 (ref, /*treat_class_rvalues_as_lvalues=*/0);
+  return lvalue_p_1 (ref, /*treat_class_rvalues_as_lvalues=*/ 0, 
/*cast*/ 1);
  }

+/* This differs from real_lvalue_p in that cast expressions are not
+   considered lvalues.  (cast-as-lvalue is a GNU extension) */
+
+cp_lvalue_kind
+non_cast_lvalue_p (ref)
+     tree ref;
+{
+  return lvalue_p_1 (ref, /*treat_class_rvalues_as_lvalues=*/ 0, 
/*cast*/ 0);
+}
+
+
  /* This differs from real_lvalue_p in that class rvalues are
     considered lvalues.  */

@@ -207,7 +236,7 @@
       tree ref;
  {
    return
-    (lvalue_p_1 (ref, /*treat_class_rvalues_as_lvalues=*/1) != 
clk_none);
+    (lvalue_p_1 (ref, /*class rvalue ok*/ 1, /*cast*/ 1) != clk_none);
  }

  /* Return nonzero if REF is an lvalue valid for this language;
@@ -218,7 +247,20 @@
       tree ref;
       const char *string;
  {
-  int win = lvalue_p (ref);
+  int ret = lvalue_p_1 (ref, /* class rvalue ok */ 1, /* cast ok */ 1);
+  int win = (ret != clk_none);
+  if (! win)
+    error ("non-lvalue in %s", string);
+  return win;
+}
+
+int
+non_cast_lvalue_or_else (ref, string)
+     tree ref;
+     const char *string;
+{
+  int ret = lvalue_p_1 (ref, /* class rvalue ok */ 1, /* cast ok */ 0);
+  int win = (ret != clk_none);
    if (! win)
      error ("non-lvalue in %s", string);
    return win;
Index: gcc/cp/typeck.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/cp/typeck.c,v
retrieving revision 1.423
diff -u -r1.423 typeck.c
--- gcc/cp/typeck.c	17 Aug 2002 12:26:01 -0000	1.423
+++ gcc/cp/typeck.c	20 Aug 2002 21:43:37 -0000
@@ -4247,7 +4247,7 @@
  	 is an error.  */
        else if (TREE_CODE (argtype) != FUNCTION_TYPE
  	       && TREE_CODE (argtype) != METHOD_TYPE
-	       && !lvalue_or_else (arg, "unary `&'"))
+	       && !non_cast_lvalue_or_else (arg, "unary `&'"))
  	return error_mark_node;

        if (argtype != error_mark_node)
bash-2.05$ cat gcc/testsuite/g++.dg/ext/lvaddr.C
// Copyright (C) 2002 Free Software Foundation
// Contributed by Matt Austern <austern@apple.com>

// { dg-do compile }

void f()
{
   int n;
   char* p = &(char) n;		// { dg-error "non-lvalue" }
}
bash-2.05$ cat gcc/testsuite/g++.dg/ext/lvcast.C
// Copyright (C) 2002 Free Software Foundation
// Contributed by Matt Austern <austern@apple.com>

// { dg-do compile }
// { dg-options -fpermissive }

void f()
{
   int n;
   static_cast<char>(n) = 0;
   (char) n = 1;
}
bash-2.05$

Re: Ping: PATCH to generate error for address of cast

[Jason Merrill]

On Tue, 20 Aug 2002 14:59:51 -0700, Matt Austern <austern@apple.com> wrote:

> bash-2.05$ cat gcc/testsuite/g++.dg/ext/lvcast.C
> // Copyright (C) 2002 Free Software Foundation
> // Contributed by Matt Austern <austern@apple.com>
>
> // { dg-do compile }
> // { dg-options -fpermissive }
>
> void f()
> {
>    int n;
>    static_cast<char>(n) = 0;
>    (char) n = 1;
> }

Please remove the static_cast from this testcase; I don't want to support
this extension with new-style casts.  Otherwise, the patch is OK.  It was
corrupted by your mailer, though, so I can't apply it; you should get one
of the other Apple folks with write privs to apply it for you.  We should
also get you set up for write on approval access.

Jason

Re: Ping: PATCH to generate error for address of cast

[Matt Austern]

On Wednesday, August 21, 2002, at 12:17 PM, Jason Merrill wrote:

> Please remove the static_cast from this testcase; I don't want to 
> support
> this extension with new-style casts.

I have no problem removing the static_cast from the
positive test case.  However, in the interests of
being as explicit as possible about just what this
extension does and doesn't do, I think that if we
decide it doesn't apply to new-style casts then we
should probably have the compiler issue an error
message.

(That's a general gripe: language specification is
hard, and I think that a lot of extensions are
underspecified.  We really shouldn't ever put in an
extension unless we're willing to do the work of
figuring out what it does in all circumstances.)

> Otherwise, the patch is OK.  It was
> corrupted by your mailer, though, so I can't apply it;

Sorry about that.  Would it help if I sent patches as
attachments instead, or is that contrary to mailing list
policy?

			--Matt

Re: Ping: PATCH to generate error for address of cast

[Jason Merrill]

On Wed, 21 Aug 2002 12:28:35 -0700, Matt Austern <austern@apple.com> wrote:

> On Wednesday, August 21, 2002, at 12:17 PM, Jason Merrill wrote:
>
>> Please remove the static_cast from this testcase; I don't want to support
>> this extension with new-style casts.
>
> I have no problem removing the static_cast from the positive test case.
> However, in the interests of being as explicit as possible about just
> what this extension does and doesn't do, I think that if we decide it
> doesn't apply to new-style casts then we should probably have the
> compiler issue an error message.

Certainly.  Care to add it, or at least an XFAILed error test?

> (That's a general gripe: language specification is hard, and I think that
> a lot of extensions are underspecified.  We really shouldn't ever put in
> an extension unless we're willing to do the work of figuring out what it
> does in all circumstances.)

Yep.

>> Otherwise, the patch is OK.  It was corrupted by your mailer, though, so
>> I can't apply it;
>
> Sorry about that.  Would it help if I sent patches as attachments
> instead, or is that contrary to mailing list policy?

Yes, that would be better.

Jason

Re: Ping: PATCH to generate error for address of cast

[Matt Austern]

On Wednesday, August 21, 2002, at 12:37 PM, Jason Merrill wrote:

> On Wed, 21 Aug 2002 12:28:35 -0700, Matt Austern <austern@apple.com> 
> wrote:
>
>> On Wednesday, August 21, 2002, at 12:17 PM, Jason Merrill wrote:
>>
>>> Please remove the static_cast from this testcase; I don't want to 
>>> support
>>> this extension with new-style casts.
>>
>> I have no problem removing the static_cast from the positive test 
>> case.
>> However, in the interests of being as explicit as possible about just
>> what this extension does and doesn't do, I think that if we decide it
>> doesn't apply to new-style casts then we should probably have the
>> compiler issue an error message.
>
> Certainly.  Care to add it, or at least an XFAILed error test?

Separately, I think.  I'll have to learn how to distinguish
between old-style and new-style casts in the IR.  I'll also
add a note to the documentation saying that the extension
doesn't apply to new-style casts.  A couple days.

			--Matt

Re: Ping: PATCH to generate error for address of cast

[Jason Merrill]

On Wed, 21 Aug 2002 15:51:20 -0700, Matt Austern <austern@apple.com> wrote:

> Separately, I think.  I'll have to learn how to distinguish
> between old-style and new-style casts in the IR.

I don't think there's currently any distinction.

Jason

Re: Ping: PATCH to generate error for address of cast

[Matt Austern]

On Thursday, August 22, 2002, at 06:33 AM, Jason Merrill wrote:

> On Wed, 21 Aug 2002 15:51:20 -0700, Matt Austern <austern@apple.com> 
> wrote:
>
>> Separately, I think.  I'll have to learn how to distinguish
>> between old-style and new-style casts in the IR.
>
> I don't think there's currently any distinction.

That would explain why I couldn't find the distinction,
in that case.
			--Matt

Re: Ping: PATCH to generate error for address of cast

[Nathan Sidwell]

Jason Merrill wrote:
> 
> On Wed, 21 Aug 2002 15:51:20 -0700, Matt Austern <austern@apple.com> wrote:
> 
> > Separately, I think.  I'll have to learn how to distinguish
> > between old-style and new-style casts in the IR.
> 
> I don't think there's currently any distinction.
No there isn't. static_cast goes through some extra initial checks
to filter out the staticky cast differences, verifies old cast
will DTRT and then calls the old cast code. Put the extra test
in build_static_cast or thereabouts.

nathan
-- 
Dr Nathan Sidwell   ::   http://www.codesourcery.com   ::   CodeSourcery LLC
         'But that's a lie.' - 'Yes it is. What's your point?'
nathan@codesourcery.com : http://www.cs.bris.ac.uk/~nathan/ : nathan@acm.org