warning when a function's address is tested?

warning when a function's address is tested?

[Andrew Morton]

We just found a silly bug in the Linux kernel:

-	if (current_is_kswapd)
+	if (current_is_kswapd())

It was there for a year.  It is a fairly easy mistake to make, and it would
be nice if the compiler could generate a warning.  I don't think there are
likely to be legitimate uses?


void foo(void)
{}

void bar(void)
{}

int main()
{
	if (foo)
		bar();
	return 0;
}

Re: warning when a function's address is tested?

[Falk Hueffner]

Andrew Morton <akpm@osdl.org> writes:

> We just found a silly bug in the Linux kernel:
> 
> -	if (current_is_kswapd)
> +	if (current_is_kswapd())
> 
> It was there for a year.  It is a fairly easy mistake to make, and
> it would be nice if the compiler could generate a warning.  I don't
> think there are likely to be legitimate uses?

Looks like a good idea. The C++ frontend already has this warning, so
it shouldn't be too hard.

-- 
	Falk

Re: warning when a function's address is tested?

[Steven Bosscher]

Op zo 12-10-2003, om 14:11 schreef Falk Hueffner:
> Andrew Morton <akpm@osdl.org> writes:
> 
> > We just found a silly bug in the Linux kernel:
> > 
> > -	if (current_is_kswapd)
> > +	if (current_is_kswapd())
> > 
> > It was there for a year.  It is a fairly easy mistake to make, and
> > it would be nice if the compiler could generate a warning.  I don't
> > think there are likely to be legitimate uses?
> 
> Looks like a good idea. The C++ frontend already has this warning, so
> it shouldn't be too hard.

It is a bit tricky since for 

void foo(void) {}
void bar(void) {}
int main() { if (foo) bar(); }

the C++ front end wraps foo in an ADDR_EXPR, but the C front end does
not (where IMHO it should...).  Something like the attached patch should
work, I'll propose it on gcc-patches after I've tested it.

Gr.
Steven

Index: c-common.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/c-common.c,v
retrieving revision 1.462
diff -c -3 -p -r1.462 c-common.c
*** c-common.c	9 Oct 2003 08:38:43 -0000	1.462
--- c-common.c	12 Oct 2003 15:59:13 -0000
*************** c_common_truthvalue_conversion (tree exp
*** 2602,2607 ****
--- 2602,2610 ----
    if (TREE_CODE (expr) == ERROR_MARK)
      return expr;
  
+   if (TREE_CODE (expr) == FUNCTION_DECL)
+     expr = build1 (ADDR_EXPR, ptr_type_node, expr);
+ 
  #if 0 /* This appears to be wrong for C++.  */
    /* These really should return error_mark_node after 2.4 is stable.
       But not all callers handle ERROR_MARK properly.  */
*************** c_common_truthvalue_conversion (tree exp
*** 2647,2663 ****
        return real_zerop (expr) ? truthvalue_false_node : truthvalue_true_node;
  
      case ADDR_EXPR:
!       /* If we are taking the address of an external decl, it might be zero
! 	 if it is weak, so we cannot optimize.  */
!       if (DECL_P (TREE_OPERAND (expr, 0))
! 	  && DECL_EXTERNAL (TREE_OPERAND (expr, 0)))
! 	break;
! 
!       if (TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 0)))
! 	return build (COMPOUND_EXPR, truthvalue_type_node,
! 		      TREE_OPERAND (expr, 0), truthvalue_true_node);
!       else
! 	return truthvalue_true_node;
  
      case COMPLEX_EXPR:
        return build_binary_op ((TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1))
--- 2650,2678 ----
        return real_zerop (expr) ? truthvalue_false_node : truthvalue_true_node;
  
      case ADDR_EXPR:
!       {
! 	if (TREE_CODE (TREE_OPERAND (expr, 0)) == FUNCTION_DECL
! 	    && ! DECL_WEAK (TREE_OPERAND (expr, 0)))
! 	  {
! 	    /* Common Ada/Pascal programmer's mistake.  We always warn
! 	       about this since it is so bad.  */
! 	    warning ("the address of `%D', will always be `true'",
! 		     TREE_OPERAND (expr, 0));
! 	    return truthvalue_true_node;
! 	  }
! 
! 	/* If we are taking the address of an external decl, it might be
! 	   zero if it is weak, so we cannot optimize.  */
! 	if (DECL_P (TREE_OPERAND (expr, 0))
! 	    && DECL_EXTERNAL (TREE_OPERAND (expr, 0)))
! 	  break;
! 
! 	if (TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 0)))
! 	  return build (COMPOUND_EXPR, truthvalue_type_node,
! 			TREE_OPERAND (expr, 0), truthvalue_true_node);
! 	else
! 	  return truthvalue_true_node;
!       }
  
      case COMPLEX_EXPR:
        return build_binary_op ((TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1))
Index: cp/cvt.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cp/cvt.c,v
retrieving revision 1.145
diff -c -3 -p -r1.145 cvt.c
*** cp/cvt.c	5 Sep 2003 08:24:19 -0000	1.145
--- cp/cvt.c	12 Oct 2003 15:59:24 -0000
*************** ocp_convert (tree type, tree expr, int c
*** 694,713 ****
  	  return error_mark_node;
  	}
        if (code == BOOLEAN_TYPE)
! 	{
! 	  tree fn = NULL_TREE;
  
- 	  /* Common Ada/Pascal programmer's mistake.  We always warn
-              about this since it is so bad.  */
- 	  if (TREE_CODE (expr) == FUNCTION_DECL)
- 	    fn = expr;
- 	  else if (TREE_CODE (expr) == ADDR_EXPR 
- 		   && TREE_CODE (TREE_OPERAND (expr, 0)) == FUNCTION_DECL)
- 	    fn = TREE_OPERAND (expr, 0);
- 	  if (fn && !DECL_WEAK (fn))
- 	    warning ("the address of `%D', will always be `true'", fn);
- 	  return cp_truthvalue_conversion (e);
- 	}
        return fold (convert_to_integer (type, e));
      }
    if (POINTER_TYPE_P (type) || TYPE_PTR_TO_MEMBER_P (type))
--- 694,701 ----
  	  return error_mark_node;
  	}
        if (code == BOOLEAN_TYPE)
! 	return cp_truthvalue_conversion (e);
  
        return fold (convert_to_integer (type, e));
      }
    if (POINTER_TYPE_P (type) || TYPE_PTR_TO_MEMBER_P (type))

Re: warning when a function's address is tested?

[Zack Weinberg]

Steven Bosscher <s.bosscher@student.tudelft.nl> writes:

> Something like the attached patch should work, I'll propose it on
> gcc-patches after I've tested it.

Informally speaking, the patch looks good - just a couple of notes:

1) there's now code in the C front end that handles a bare
   FUNCTION_DECL - it should be found and removed.

2) this sentence 

> ! 	    warning ("the address of `%D', will always be `true'",
> ! 		     TREE_OPERAND (expr, 0));

is awkward English - suggest removing the comma and the quotes around 'true'.

zw

Re: warning when a function's address is tested?

[Gabriel Dos Reis]

"Zack Weinberg" <zack@codesourcery.com> writes:

| > ! 	    warning ("the address of `%D', will always be `true'",
| > ! 		     TREE_OPERAND (expr, 0));
| 
| is awkward English - suggest removing the comma and the quotes around 'true'.


   "the address of '%D' will always evaluate to true"

?

-- Gaby

Re: warning when a function's address is tested?

[Zack Weinberg]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> "Zack Weinberg" <zack@codesourcery.com> writes:
>
> | > ! 	    warning ("the address of `%D', will always be `true'",
> | > ! 		     TREE_OPERAND (expr, 0));
> | 
> | is awkward English - suggest removing the comma and the quotes around 'true'.
>
>
>    "the address of '%D' will always evaluate to true"

That's a better choice of verb, but then I think you want to put the
quotes around 'true' back, and maybe use 'as' instead of 'to'.

zw

Re: warning when a function's address is tested?

[Gabriel Dos Reis]

"Zack Weinberg" <zack@codesourcery.com> writes:

| Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
| 
| > "Zack Weinberg" <zack@codesourcery.com> writes:
| >
| > | > ! 	    warning ("the address of `%D', will always be `true'",
| > | > ! 		     TREE_OPERAND (expr, 0));
| > | 
| > | is awkward English - suggest removing the comma and the quotes around 'true'.
| >
| >
| >    "the address of '%D' will always evaluate to true"
| 
| That's a better choice of verb, but then I think you want to put the
| quotes around 'true' back, and maybe use 'as' instead of 'to'.

I agree with your suggestion.  

Thanks,

-- Gaby

Re: warning when a function's address is tested?

[Jeff Sturm]

On Sun, 12 Oct 2003, Andrew Morton wrote:
> -	if (current_is_kswapd)
> +	if (current_is_kswapd())
>
> It was there for a year.  It is a fairly easy mistake to make, and it would
> be nice if the compiler could generate a warning.  I don't think there are
> likely to be legitimate uses?

One legitimate use is to test for undefined weak symbols.  That's not an
argument against the warning however... more often than not this is surely
a mistake.

Jeff

Re: warning when a function's address is tested?

[John Levon]

On Sun, Oct 12, 2003 at 12:00:15PM -0400, Jeff Sturm wrote:

> One legitimate use is to test for undefined weak symbols.  That's not an
> argument against the warning however... more often than not this is surely
> a mistake.

And the code in cp/cvt.c explicitly checks for this and doesn't warn
anyway.

john
-- 
Khendon's Law:
If the same point is made twice by the same person, the thread is over.

Re: warning when a function's address is tested?

[Falk Hueffner]

Jeff Sturm <jsturm@one-point.com> writes:

> On Sun, 12 Oct 2003, Andrew Morton wrote:
> > -	if (current_is_kswapd)
> > +	if (current_is_kswapd())
> >
> > It was there for a year.  It is a fairly easy mistake to make, and it would
> > be nice if the compiler could generate a warning.  I don't think there are
> > likely to be legitimate uses?
> 
> One legitimate use is to test for undefined weak symbols.

But gcc knows that from __attribute__((weak)) and can suppress the
warning in this case (like the C++ frontend does).

-- 
	Falk

Re: warning when a function's address is tested?

[Jeff Sturm]

On 12 Oct 2003, Falk Hueffner wrote:
> > One legitimate use is to test for undefined weak symbols.
>
> But gcc knows that from __attribute__((weak)) and can suppress the
> warning in this case (like the C++ frontend does).

Thanks, didn't know c++ did that.  So, never mind...

Jeff