Bug 16202

Summary: The -Wsequence-point warning misses many important instances
Product: gcc Reporter: Tom Truscott <trt>
Component: cAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: enhancement CC: gcc-bugs, gdr, giovannibajo, jsm28, manu, rth
Priority: P2 Keywords: diagnostic, patch
Version: 4.0.0   
Target Milestone: ---   
URL: http://gcc.gnu.org/ml/gcc-patches/2008-10/msg01066.html
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2006-02-26 19:27:32
Attachments: proposed patch

Description Tom Truscott 2004-06-25 18:24:43 UTC
The current -Wsequence-point handles only simple variables,
so it misses the three bugs in this test program:

    struct s { struct s *nxt; int v; } q;
     
    int x[10];
     
    void foo(int **p)
    {
       (*p) = (*p)++;
       x[3] = x[3]++;
       q.nxt->nxt->v = q.nxt->nxt->v++;
    }

With an obvious tweak to c-common.c (below), they will be caught:

    opundef.c:7: warning: operation on '*p' may be undefined
    opundef.c:8: warning: operation on 'x[3]' may be undefined
    opundef.c:9: warning: operation on 'q.nxt->nxt->v' may be undefined

I added this to gcc after it failed to catch a nasty bug,
and then it found a dozen more latent ones (in a 35Mloc source code base).

This patch to c-common.c assumes that warning ("....%E ...") works,
but unfortunately it does not.  See bugzilla # 16119

*** c-common.c.orig     Sat Jun 19 15:34:18 2004
--- c-common.c  Mon Jun 21 13:53:05 2004
***************
*** 1246,1247 ****
--- 1246,1248 ----
  static int warning_candidate_p (tree);
+ static int candidate_equal_p (tree, tree);
  static void warn_for_collisions (struct tlist *);
***************
*** 1274,1276 ****
        add->next = *to;
!       if (! exclude_writer || add->writer != exclude_writer)
        *to = copy ? new_tlist (*to, add->expr, add->writer) : add;
--- 1275,1277 ----
        add->next = *to;
!       if (! exclude_writer || ! candidate_equal_p (add->writer, exclude_writer))
        *to = copy ? new_tlist (*to, add->expr, add->writer) : add;
***************
*** 1301,1303 ****
        for (tmp2 = *to; tmp2; tmp2 = tmp2->next)
!       if (tmp2->expr == add->expr)
          {
--- 1302,1304 ----
        for (tmp2 = *to; tmp2; tmp2 = tmp2->next)
!       if (candidate_equal_p (tmp2->expr, add->expr))
          {
***************
*** 1329,1331 ****
    for (tmp = warned_ids; tmp; tmp = tmp->next)
!     if (tmp->expr == written)
        return;
--- 1330,1332 ----
    for (tmp = warned_ids; tmp; tmp = tmp->next)
!     if (candidate_equal_p (tmp->expr, written))
        return;
***************
*** 1334,1337 ****
      {
!       if (list->expr == written
!         && list->writer != writer
          && (! only_writes || list->writer))
--- 1335,1338 ----
      {
!       if (candidate_equal_p (list->expr, written)
!         && ! candidate_equal_p (list->writer, writer)
          && (! only_writes || list->writer))
***************
*** 1339,1342 ****
          warned_ids = new_tlist (warned_ids, written, NULL_TREE);
!         warning ("operation on `%s' may be undefined",
!                  IDENTIFIER_POINTER (DECL_NAME (list->expr)));
        }
--- 1340,1342 ----
          warned_ids = new_tlist (warned_ids, written, NULL_TREE);
!         warning ("operation on %qE may be undefined", written);
        }
***************
*** 1366,1368 ****
  {
!   return TREE_CODE (x) == VAR_DECL || TREE_CODE (x) == PARM_DECL;
  }
--- 1366,1375 ----
  {
!   return lvalue_p (x);
! }
!
! /* Return nonzero if X and Y appear to be the same candidate (or NULL) */
! static int
! candidate_equal_p (tree x, tree y)
! {
!   return (x == y) || (x && y && operand_equal_p (x, y, 0));
  }
***************
*** 1412,1417 ****
    if (warning_candidate_p (x))
!     {
!       *pno_sp = new_tlist (*pno_sp, x, writer);
!       return;
!     }
   
--- 1419,1421 ----
    if (warning_candidate_p (x))
!     *pno_sp = new_tlist (*pno_sp, x, writer);
   
***************
*** 1521,1523 ****
        for (t = save_expr_cache; t; t = t->next)
!         if (t->expr == x)
            break;
--- 1525,1527 ----
        for (t = save_expr_cache; t; t = t->next)
!         if (candidate_equal_p (t->expr, x))
            break;
Comment 1 Andrew Pinski 2004-06-25 21:01:27 UTC
Confirmed, Patches goto gcc-patches@.
Comment 2 Gabriel Dos Reis 2004-10-03 03:02:24 UTC
silly bugzilla.
Comment 3 Giovanni Bajo 2004-10-04 12:07:11 UTC
JSM, can you have a look at this patch? It is said to be fully functional 
already, so it would be a shame if it did not make it into 4.0 just for lack of 
attention.
Comment 4 Joseph S. Myers 2004-10-04 12:44:16 UTC
Subject: Re:  The -Wsequence-point warnng misses many important
 instances

On Mon, 4 Oct 2004, giovannibajo at libero dot it wrote:

> JSM, can you have a look at this patch? It is said to be fully 
> functional already, so it would be a shame if it did not make it into 
> 4.0 just for lack of attention.

I don't see either a testcase or a patch to the existing 
gcc.dg/sequence-pt-1.c to remove XFAILs in this patch.  A complete 
submission including tests should be sent to gcc-patches.

Comment 5 Tom Truscott 2004-10-06 18:35:58 UTC
Created attachment 7299 [details]
proposed patch

I've attached a more official-looking patch, with a testsuite change to
gcc.dg/sequence-pt-1.c and a new sequence-pt-2.c

C++ now invokes this code, and the C++ variant of lvalue_p(x) in cp/tree.c
crashes on TREE_LIST nodes.  I tried checking EXPR_P(x) but it looks like it
really has to be (EXPR_P (x) || DECL_P (x)) so I just check for NULL TREE_TYPE
(x) which is what the C++ lvalue_p() is crashing on.  I consider this a
work-around for a C++ lvalue_p() bug.
Comment 6 Manuel López-Ibáñez 2007-01-09 14:57:52 UTC
Another Wsequence-point bug is PR 17880.
Comment 7 Manuel López-Ibáñez 2007-01-16 22:02:41 UTC
(In reply to comment #5)
> Created an attachment (id=7299) [edit]
> proposed patch
> 
> I've attached a more official-looking patch, with a testsuite change to
> gcc.dg/sequence-pt-1.c and a new sequence-pt-2.c
> 

Could you send your patch to gcc-patches@gcc.gnu.org, please ? Thanks.

You can find more info on contributing to GCC development at:
http://gcc.gnu.org/contribute.html
Comment 8 Manuel López-Ibáñez 2007-01-16 22:35:24 UTC
(In reply to comment #5)
> Created an attachment (id=7299) [edit]
> proposed patch
> 
> I've attached a more official-looking patch, with a testsuite change to
> gcc.dg/sequence-pt-1.c and a new sequence-pt-2.c

This patch does not even compile on current mainline, since lvalue_p was moved out of c-common.c to c-typeck.c and to cp/tree.c :( 


Comment 9 Tom Truscott 2007-01-17 18:15:37 UTC
I made lvalue_p a global function in my personal gcc.

I've proposed a dozen different warnings-related things for gcc, and never made headway on any of them.  I'm just a random user and don't know the secret handshake.  The people who do seem utterly uninterested in gcc diagnostics.

If you, or anyone, would get this patch working and into gcc, I would be delighted.
Comment 10 Manuel López-Ibáñez 2007-01-17 19:12:19 UTC
I am also new, my first patch was just a few months ago, so let me say that I understand your situation. On the other hand, I got patches committed, so also let me say that it is not as bad as you may think.

The secret handshake is quite simple actually:

1) Have a copyright assignment: http://gcc.gnu.org/contribute.html#legal

2) Post patches to gcc-patches@gcc.gnu.org (you can also add them to the patch queue http://www.dberlin.org/patchdirections.html but posting to gcc-patches is essential, bugzilla attachments are not considered serious proposals). 

3) Add testcases to the patch and bootstrap and regression test for a recent revision, and say that you have done so. Add a Changelog to your mail (but not to the patch). More info at http://gcc.gnu.org/contribute.html

4) Wait, wait, wait, make a comment about your patch pending review (if you follow the list, you will see that people do this very often), then wait, wait and wait a bit more, perhaps make another comment and eventually your patch will be reviewed and surely you will have to make changes. In that case, goto 2.

5) Ask for someone to commit the patch for yourself or ask for write-after-approval rights. The wait-wait-wait thing may apply here again. Patience, you can have as many patches pending review as you wish, just ping them once every while.

Easy, isn't it?

I think the first step is to get subscribed to gcc-patches to check out how people submit and get reviewed patches. What are you waiting for? We look forward to have you on board!

Cheers,

Manuel.
Comment 11 Manuel López-Ibáñez 2008-10-24 17:21:23 UTC
Patch for GCC 4.5 :

http://gcc.gnu.org/ml/gcc-patches/2008-10/msg01066.html
Comment 12 Manuel López-Ibáñez 2009-04-21 07:47:32 UTC
Subject: Bug 16202

Author: manu
Date: Tue Apr 21 07:47:13 2009
New Revision: 146472

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=146472
Log:
2009-04-21  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR 16202
	* c-typeck.c (lvalue_p): Move declaration ...
	* c-common.h (lvalue_p): ... to here.
	* c-common.c (candidate_equal_p): New.
	(add_tlist): Use it.
	(merge_tlist): Use it.
	(warn_for_collisions_1): Likewise.
	(warning_candidate_p): Accept more candidates.
	(verify_tree): A warning candidate can be an expression. Use
	candidate_equal_p.
cp/
	* tree.c (lvalue_p_1): Use const_tree.
	Use CONST_CAST_TREE to avoid warning.
	(lvalue_p): Returns bool, receives const_tree.
testsuite/
	* gcc.dg/sequence-pt-1.c: Remove XFAILs.
	* gcc.dg/sequence-pt-2.c: New.
	* gcc.dg/sequence-pt-3.c: New.
	* g++.dg/warn/sequence-pt-1.C: Remove XFAILs.
	* g++.dg/warn/sequence-pt-2.c: New.
	* g++.dg/warn/sequence-pt-3.c: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/sequence-pt-2.C
    trunk/gcc/testsuite/g++.dg/warn/sequence-pt-3.C
    trunk/gcc/testsuite/gcc.dg/sequence-pt-2.c
    trunk/gcc/testsuite/gcc.dg/sequence-pt-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-common.c
    trunk/gcc/c-common.h
    trunk/gcc/c-typeck.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-tree.h
    trunk/gcc/cp/tree.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/warn/sequence-pt-1.C
    trunk/gcc/testsuite/gcc.dg/sequence-pt-1.c

Comment 13 Manuel López-Ibáñez 2009-04-21 07:48:42 UTC
FIXED in GCC 4.5