Implement -fsanitize=return (for C++ only)

On Fri, Nov 22, 2013 at 10:28:52AM +0100, Jakub Jelinek wrote:
> Hi!
> 
> Working virtually out of Pago Pago right now.
> 
> In C++, falling through the end of a function that returns a value
> without returning a value is undefined behavior (unlike C?), so this patch
> implements instrumentation of it.

I think we really want to have this in for C99+ as well, mainly
because C99 removed implicit int rule.

> --- gcc/ubsan.c.jj	2013-11-22 01:40:03.000000000 +0100
> +++ gcc/ubsan.c	2013-11-22 10:05:29.491725405 +0100
> @@ -227,8 +227,8 @@ ubsan_source_location (location_t loc)
>    xloc = expand_location (loc);
>  
>    /* Fill in the values from LOC.  */
> -  size_t len = strlen (xloc.file);
> -  tree str = build_string (len + 1, xloc.file);
> +  size_t len = xloc.file ? strlen (xloc.file) : 0;
> +  tree str = build_string (len + 1, xloc.file ? xloc.file : "");

This shouldn't be needed - the ubsan_source_location call is guarded
by if (loc != UNKNOWN_LOCATION).  But it doesn't hurt either.

> --- gcc/c-family/c-ubsan.c.jj	2013-11-19 21:56:20.000000000 +0100
> +++ gcc/c-family/c-ubsan.c	2013-11-22 09:52:31.810642606 +0100
> @@ -179,3 +179,14 @@ ubsan_instrument_vla (location_t loc, tr
>  
>    return t;
>  }
> +
> +/* Instrument missing return in C++ functions returning non-void.  */
> +
> +tree
> +ubsan_instrument_return (location_t loc)
> +{
> +  tree data = ubsan_create_data ("__ubsan_missing_return_data", loc,
> +				 NULL,NULL_TREE);

Missing space after NULL.

Otherwise the ubsan bits are ok.

Recently I tried to implement this one, too, but I instead tweaked
execute_warn_function_return in tree-cfg.c.  It was something
along the lines of (only tree-cfg.c changes here):


	Marek

On Fri, Nov 22, 2013 at 01:59:47PM +0100, Marek Polacek wrote:
> On Fri, Nov 22, 2013 at 10:28:52AM +0100, Jakub Jelinek wrote:
> > Working virtually out of Pago Pago right now.
> > 
> > In C++, falling through the end of a function that returns a value
> > without returning a value is undefined behavior (unlike C?), so this patch
> > implements instrumentation of it.
> 
> I think we really want to have this in for C99+ as well, mainly
> because C99 removed implicit int rule.

That's true, but I couldn't find in the C99 standard what the C++98 says
in [stmt.return]/2:
"Flowing off the end of a function is equivalent to a return with no value;
this results in undefined behavior in a value-returning function."

Thus,
int foo () {}
int main () { foo (); return 0; }
if I read it well is undefined behavior in C++, but is it in C (in
particular, when you don't use the calls' value)?

> > --- gcc/ubsan.c.jj	2013-11-22 01:40:03.000000000 +0100
> > +++ gcc/ubsan.c	2013-11-22 10:05:29.491725405 +0100
> > @@ -227,8 +227,8 @@ ubsan_source_location (location_t loc)
> >    xloc = expand_location (loc);
> >  
> >    /* Fill in the values from LOC.  */
> > -  size_t len = strlen (xloc.file);
> > -  tree str = build_string (len + 1, xloc.file);
> > +  size_t len = xloc.file ? strlen (xloc.file) : 0;
> > +  tree str = build_string (len + 1, xloc.file ? xloc.file : "");
> 
> This shouldn't be needed - the ubsan_source_location call is guarded
> by if (loc != UNKNOWN_LOCATION).  But it doesn't hurt either.

I got an ICE without it.  First when UBSAN tried to instrument
*this_5(D) ={v} {CLOBBER};
which it shouldn't obviously, but later on also when trying to instrument
some destructor call with &b as argument.  Would be nice if the sanopt
pass detected that the argument is always non-NULL and just didn't create
the data for __ubsan_* handler at all - I'd say that e.g. trying to fold
comparison &b == 0 would yield it is always false, then you wouldn't
instrument anything and just drop the UBSAN_NULL internal call.

> > +  tree data = ubsan_create_data ("__ubsan_missing_return_data", loc,
> > +				 NULL,NULL_TREE);
> 
> Missing space after NULL.

Oops.

> --- gcc/tree-cfg.c.mp2	2013-11-22 12:42:13.452426763 +0100
> +++ gcc/tree-cfg.c	2013-11-22 13:56:41.051581885 +0100
> @@ -8090,9 +8090,16 @@ execute_warn_function_return (void)
>  	    {
>  	      location = gimple_location (last);
>  	      if (location == UNKNOWN_LOCATION)
> -		  location = cfun->function_end_locus;
> -	      warning_at (location, OPT_Wreturn_type, "control reaches end of non-void function");
> +		location = cfun->function_end_locus;
> +	      warning_at (location, OPT_Wreturn_type, "control reaches end "
> +			  "of non-void function");
>  	      TREE_NO_WARNING (cfun->decl) = 1;
> +	      if (flag_sanitize & SANITIZE_RETURN)
> +		{
> +		  gimple_stmt_iterator gsi = gsi_for_stmt (last);
> +		  gsi_insert_before (&gsi, ubsan_instrument_return (location),
> +				     GSI_SAME_STMT);
> +		}

I guess this depends on whether it is undefined behavior in C or not (if you
don't use the value in the caller).  Plus, !targetm.warn_func_return fns
shouldn't be instrumented (though perhaps that is checked earlier already).

	Jakub

On Fri, 22 Nov 2013, Marek Polacek wrote:

> On Fri, Nov 22, 2013 at 10:28:52AM +0100, Jakub Jelinek wrote:
> > Hi!
> > 
> > Working virtually out of Pago Pago right now.
> > 
> > In C++, falling through the end of a function that returns a value
> > without returning a value is undefined behavior (unlike C?), so this patch
> > implements instrumentation of it.
> 
> I think we really want to have this in for C99+ as well, mainly
> because C99 removed implicit int rule.

In C it's undefined behavior only if the caller uses the return value of a 
particular execution where the end of the called function was reached 
(6.9.1#12 in C11, "If the } that terminates a function is reached, and the 
value of the function call is used by the caller, the behavior is 
undefined.").