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
