Re: ATTRIBUTE_NORETURN on sets_cc0_p

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Kaveh R. Ghazi]

 > From: Mark Mitchell <mark@markmitchell.com>
 > 
 > On x86-linux-gnu (which defines HAVE_cc0), I'm seeing:
 > 
 > ../../gcc/jump.c: In function `sets_cc0_p':
 > ../../gcc/jump.c:3335: warning: function declared `noreturn' has a
 > 	`return' statement
 > 
 > That's not good; this function does return and we don't want the
 > compiler assuming it doesn't.  That could be very dangerous.
 > 
 > I see that rtl.h has:
 > 
 > extern int sets_cc0_p			PROTO ((rtx))
 > #ifndef HAVE_cc0
 >   ATTRIBUTE_NORETURN
 > #endif
 >   ;
 > 
 > Perhaps HAVE_cc0 is not yet be set at that point?
 > -- 
 > Mark Mitchell 			mark@markmitchell.com

	Right.  HAVE_cc0 is defined in insn-config.h which appears after
rtl.h in jump.c.  We have to move insn-config.h above rtl.h, but in
*every* file, not just jump.c.  The harmful scenario is other files
which incorrectly assume sets_cc0_p() doesn't return, (and don't even
complain about it.)

	The idea I have is to make genconfig put something like this in
insn-config.h

 > #ifdef RTX_CODE
 >  #error "insn-config.h must be included before rtl.h."
 > #endif

and then see what breaks. :-)  That'll show us all the spots, and
(importantly) leaving it there will ensure this header file ordering bug
doesn't creep back in.  I'll try to crank out a patch tomorrow. 

		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Icon CMT Corp.

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < 199810150408.AAA26944@caip.rutgers.edu >you write:
  > 	Right.  HAVE_cc0 is defined in insn-config.h which appears after
  > rtl.h in jump.c.  We have to move insn-config.h above rtl.h, but in
  > *every* file, not just jump.c.  The harmful scenario is other files
  > which incorrectly assume sets_cc0_p() doesn't return, (and don't even
  > complain about it.)
Ugh.  Note this includes backends, possibly the insn*.c files, and possibly
even front-ends since many want to include rtl.h (including front-ends we
don't control yet like ada, pascal, & modula).

Another thought would be to include insn-config via rtl.h.  We've tried to
avoid too much of this in the gcc sources though.

I'm not sure which is the cleaner solution.

I'm very tempted to punt it for now since marking sets_cc0_p as noreturn is
strictly an optimization.

jeff

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Mark Mitchell]

>>>>> "Jeffrey" == Jeffrey A Law <law@cygnus.com> writes:

    Jeffrey> I'm very tempted to punt it for now since marking
    Jeffrey> sets_cc0_p as noreturn is strictly an optimization.

And a pretty unimportant one.  It's only noreturn because all it does
is call `abort()' in the non-HAVE-cc0 case.  We could just call abort
directly in that case via some macro magic if it's that important to
let the compiler know the call will not return.

I don't really think the `function not marked noreturn but doesn't
return' warning is very good.  This warnings means that to get gcc to
shut up, I *must* use a gcc extension on perfectly valid, unambiguous,
unconfusing code.  That's unreasonable.  This warning attempts to
force me to give the compiler an additional optimization hint, not to
in any way clarify the function.  (It's not like signed/unsigned
comparison, for example, which is well-defined but wicked confusing.)

I think the right solution is no pull this warning out of -W -Wall,
and only enable it with a special -Wnoreturn flag or some such.

I don't have a problem with the opposite warning (`function uses
noreturn but returns anyhow').

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Richard Earnshaw]

> >>>>> "Jeffrey" == Jeffrey A Law <law@cygnus.com> writes:
> 
>     Jeffrey> I'm very tempted to punt it for now since marking
>     Jeffrey> sets_cc0_p as noreturn is strictly an optimization.
> 
> And a pretty unimportant one.  It's only noreturn because all it does
> is call `abort()' in the non-HAVE-cc0 case.  We could just call abort
> directly in that case via some macro magic if it's that important to
> let the compiler know the call will not return.
> 
> I don't really think the `function not marked noreturn but doesn't
> return' warning is very good.  This warnings means that to get gcc to
> shut up, I *must* use a gcc extension on perfectly valid, unambiguous,
> unconfusing code.  That's unreasonable.  This warning attempts to
> force me to give the compiler an additional optimization hint, not to
> in any way clarify the function.  (It's not like signed/unsigned
> comparison, for example, which is well-defined but wicked confusing.)
> 
> I think the right solution is no pull this warning out of -W -Wall,
> and only enable it with a special -Wnoreturn flag or some such.
> 
> I don't have a problem with the opposite warning (`function uses
> noreturn but returns anyhow').
> 

The documentation in extend.texi is very clear on this one:

  The @code{noreturn} keyword tells the compiler to assume that
  @code{fatal} cannot return.  It can then optimize without regard to what
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  would happen if @code{fatal} ever did return.  This makes slightly
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  better code.  More importantly, it helps avoid spurious warnings of
  uninitialized variables.

  Do not assume that registers saved by the calling function are
  restored before calling the @code{noreturn} function.

  It does not make sense for a @code{noreturn} function to have a return
  type other than @code{void}.

Seeing the noreturn attribute on a function will cause gcc to insert a barrier after the call, so that flow can see that insns after that will not be used.  Programs would therefore behave in an undefined manner if they did return.

Additionally, the ARM port also uses this to avoid having to push all the call-saved registers onto the stack (after all, they will never get unpushed).

Please lets not devalue this attribute into just a "lets shut up a warning" message.

Richard.

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < vyzzpatkl8h.fsf@issan.cs.uni-dortmund.de >you write:
  > Jeffrey A Law <law@cygnus.com> writes:
  > 
  > |>   In message < vyzvhlkykg9.fsf@issan.cs.uni-dortmund.de >you write:
  > |>   > It looks pretty bogus to me to have a function that returns a value 
  > but
  > |>   > does not return.  ATTRIBUTE_NORETURN does not make sense on a non-vo
  > id
  > |>   > function and is most likely misplaced here.
  > |> For a non-cc0 machine sets_cc0_p unconditionally aborts.
  > 
  > In other words, for non-cc0 machine it is never called, is it?  Thus the
  > actual declaration doesn't matter at all then?
Correct.  If it ever gets called by a non-cc0 machine, that's a bug.

jeff

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Andreas Schwab]

Jeffrey A Law <law@cygnus.com> writes:

|>   In message < vyzvhlkykg9.fsf@issan.cs.uni-dortmund.de >you write:
|>   > It looks pretty bogus to me to have a function that returns a value but
|>   > does not return.  ATTRIBUTE_NORETURN does not make sense on a non-void
|>   > function and is most likely misplaced here.
|> For a non-cc0 machine sets_cc0_p unconditionally aborts.

In other words, for non-cc0 machine it is never called, is it?  Thus the
actual declaration doesn't matter at all then?

Andreas.

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < 199810161826.LAA07779@smtp.earthlink.net >you write:
  > Sure.  g++.robertl/eb132.C on x86-linux-gnu trips this warning for me.
  > You can run `runtest -tool g++ old-deja.exp=eb132.C' and then look at
  > the log to see the right set of flags to pass to cc1plus.
Well, "this" is showing up as a VAR_DECL, not a PARM_DECL probably has a lot
to do with the problem.  I don't know enough about the G++ front end to decide
if that should be happening or not.

  > That's what I meant.  A VAR_DECL with DECL_INITIAL or a PARM_DECL
  > should never be warned about.  That's not a complete solution, but it
  > would be a failsafe, and would get rid of some of the most obvious
  > false positives. 
OK.  We never warn for a PARM_DECL (see uninitialized_vars_warning).

I just tweaked that same code to not warn for a VAR_DECL if it has a nonzero
DECL_INITIAL.

jeff

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < vyzvhlkykg9.fsf@issan.cs.uni-dortmund.de >you write:
  > It looks pretty bogus to me to have a function that returns a value but
  > does not return.  ATTRIBUTE_NORETURN does not make sense on a non-void
  > function and is most likely misplaced here.
For a non-cc0 machine sets_cc0_p unconditionally aborts.

For a cc0 machine it returns a value.



jeff

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Mark Mitchell]

>>>>> "Jeffrey" == Jeffrey A Law <law@cygnus.com> writes:

    Jeffrey>   In message
    Jeffrey> < 199810161737.KAA07180@smtp.earthlink.net >you write:
    >> OK.  But, those dang uninitialized variable warnings have been
    >> in such bad shape of late that I expect that we'd be better off
    >> investigating them directly.
    Jeffrey> Well, I know where most of them are comming from.  It's
    Jeffrey> not an easy problem to resolve.


    >> At this point, one C++ regression test is failing because the
    >> compiler complains that `this' might be used uninitialized even
    >> though it's a parameter and therefore always initialized!

    Jeffrey> That one is a little more interesting.  Can you get me a
    Jeffrey> testcase I can look at?  Even C++ is OK :-)

Sure.  g++.robertl/eb132.C on x86-linux-gnu trips this warning for me.
You can run `runtest -tool g++ old-deja.exp=eb132.C' and then look at
the log to see the right set of flags to pass to cc1plus.

    >> I think the warning code should have some basic failsafe that
    >> checks DECL_INITIAL and doesn't warn for PARM_DECLs to avoid
    >> getting itself confused, but I haven't suggested this yet
    >> because implementing this failsafe may obscure some the real
    >> problems with the uninitialized-variable code.

    Jeffrey> Interesting idea.  If a VAR_DECL has a DECL_INITIAL, then
    Jeffrey> it can't be uninitialized.  It doesn't solve all the
    Jeffrey> problems, but would kill some of the false positives
    Jeffrey> created by global optimizations.

    Jeffrey> I'm less sure about PARM_DECLs.  Seems to me that a
    Jeffrey> PARM_DECL is always implicitly initialized by the caller
    Jeffrey> and we should never warn about them.

That's what I meant.  A VAR_DECL with DECL_INITIAL or a PARM_DECL
should never be warned about.  That's not a complete solution, but it
would be a failsafe, and would get rid of some of the most obvious
false positives. 

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Mark Mitchell]

>>>>> "Kaveh" == Kaveh R Ghazi <ghazi@caip.rutgers.edu> writes:

    Kaveh> Mark,

    Kaveh> 	Please let me clear up a couple of things.  The
    Kaveh> warning *has not been installed in egcs*.  Its only in my
    Kaveh> local sources. 

Ah, that was my main misunderstanding.

    Kaveh> 	Second, the detection patch *does not put this warning
    Kaveh> in -W -Wall*.  It only gets turned on when specifically
    Kaveh> asked for via -Wmissing-noreturn.  (I don't recall if I
    Kaveh> submitted the final version or not, but it does exactly as
    Kaveh> you ask.  Ie, only warn when asked for.)

Very good.

    Kaveh> 	Third, I'm not doing this for the trivial optimization
    Kaveh> it affords.  My reason is because adding noreturn gives
    Kaveh> more information to flow analysis which helps remove some
    Kaveh> of the uninitialized variable warnings.

OK.  But, those dang uninitialized variable warnings have been in such
bad shape of late that I expect that we'd be better off investigating
them directly.  At this point, one C++ regression test is failing
because the compiler complains that `this' might be used uninitialized
even though it's a parameter and therefore always initialized!  I
think the warning code should have some basic failsafe that checks
DECL_INITIAL and doesn't warn for PARM_DECLs to avoid getting itself
confused, but I haven't suggested this yet because implementing this
failsafe may obscure some the real problems with the
uninitialized-variable code.

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < 199810161737.KAA07180@smtp.earthlink.net >you write:
  > OK.  But, those dang uninitialized variable warnings have been in such
  > bad shape of late that I expect that we'd be better off investigating
  > them directly.
Well, I know where most of them are comming from.  It's not an easy problem to
resolve.


  > At this point, one C++ regression test is failing
  > because the compiler complains that `this' might be used uninitialized
  > even though it's a parameter and therefore always initialized!
That one is a little more interesting.  Can you get me a testcase I can look
at?  Even C++ is OK :-)

  > I think the warning code should have some basic failsafe that checks
  > DECL_INITIAL and doesn't warn for PARM_DECLs to avoid getting itself
  > confused, but I haven't suggested this yet because implementing this
  > failsafe may obscure some the real problems with the
  > uninitialized-variable code.
Interesting idea.  If a VAR_DECL has a DECL_INITIAL, then it can't be
uninitialized.  It doesn't solve all the problems, but would kill some of the
false positives created by global optimizations.

I'm less sure about PARM_DECLs.  Seems to me that a PARM_DECL is always
implicitly initialized by the caller and we should never warn about them.

Another thought is to perform the necessary analysis before gcse so that
gcse's actions don't confuse things.  That's a pretty heavyweight solution
though.

jeff

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Kaveh R. Ghazi]

 > From: Mark Mitchell <mark@markmitchell.com>
 > 
 > >>>>> "Jeffrey" == Jeffrey A Law <law@cygnus.com> writes:
 > 
 >     Jeffrey> I'm very tempted to punt it for now since marking
 >     Jeffrey> sets_cc0_p as noreturn is strictly an optimization.
 > 
 > I don't really think the `function not marked noreturn but doesn't
 > return' warning is very good.  This warnings means that to get gcc to
 > shut up, I *must* use a gcc extension on perfectly valid, unambiguous,
 > unconfusing code.  That's unreasonable.  This warning attempts to
 > force me to give the compiler an additional optimization hint, not to
 > in any way clarify the function.  (It's not like signed/unsigned
 > comparison, for example, which is well-defined but wicked confusing.)
 > 
 > I think the right solution is no pull this warning out of -W -Wall,
 > and only enable it with a special -Wnoreturn flag or some such.
 > -- 
 > Mark Mitchell 			mark@markmitchell.com


Mark,

	Please let me clear up a couple of things.  The warning *has not
been installed in egcs*.  Its only in my local sources.  I've just been
cleaning up the spots it detects. 

	Second, the detection patch *does not put this warning in -W
-Wall*.  It only gets turned on when specifically asked for via
-Wmissing-noreturn.  (I don't recall if I submitted the final version or
not, but it does exactly as you ask.  Ie, only warn when asked for.)

	Third, I'm not doing this for the trivial optimization it
affords.  My reason is because adding noreturn gives more information to
flow analysis which helps remove some of the uninitialized variable
warnings. 

	I'll submit the detection patch to egcs-patches so you can see
what I mean.  I hope this helps allay your concerns. 

		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Icon CMT Corp.

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Kaveh R. Ghazi]

 > From: Jeffrey A Law <law@cygnus.com>
 > 
 >   In message < 199810150408.AAA26944@caip.rutgers.edu >you write:
 >   > 	Right.  HAVE_cc0 is defined in insn-config.h which appears after
 >   > rtl.h in jump.c.  We have to move insn-config.h above rtl.h, but in
 >   > *every* file, not just jump.c.  The harmful scenario is other files
 >   > which incorrectly assume sets_cc0_p() doesn't return, (and don't even
 >   > complain about it.)
 > Ugh.  Note this includes backends, possibly the insn*.c files, and possibly
 > even front-ends since many want to include rtl.h (including front-ends we
 > don't control yet like ada, pascal, & modula).
 > 
 > Another thought would be to include insn-config via rtl.h.  We've tried to
 > avoid too much of this in the gcc sources though.


	I tried this, but it gave me a circular dependency.  Roughly:

rtl.h -> insn-config.h -> genconfig -> genconfig.o -> rtl.h.

So that's why I suggested my alternative.



 > I'm not sure which is the cleaner solution.
 > 
 > I'm very tempted to punt it for now since marking sets_cc0_p as noreturn is
 > strictly an optimization.
 > jeff

	I agree.  Do I have your approval to revert the noreturn on
sets_cc0_p?

		--Kaveh

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < 199810161528.LAA21281@caip.rutgers.edu >you write:
  > rtl.h -> insn-config.h -> genconfig -> genconfig.o -> rtl.h.
  > 
  > So that's why I suggested my alternative.
Ugh.

  > 	I agree.  Do I have your approval to revert the noreturn on
  > sets_cc0_p?
Yes.  Let's remove it.  We can revisit later.

jeff

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Andreas Schwab]

Jeffrey A Law <law@cygnus.com> writes:

|>   In message < 199810150200.TAA14764@smtp.earthlink.net >you write:
|>   > 
|>   > On x86-linux-gnu (which defines HAVE_cc0), I'm seeing:
|>   > 
|>   > ../../gcc/jump.c: In function `sets_cc0_p':
|>   > ../../gcc/jump.c:3335: warning: function declared `noreturn' has a `return'
|>   >  statement
|>   > 
|>   > That's not good; this function does return and we don't want the
|>   > compiler assuming it doesn't.  That could be very dangerous.
|>   > 
|>   > I see that rtl.h has:
|>   > 
|>   > extern int sets_cc0_p			PROTO ((rtx))
|>   > #ifndef HAVE_cc0
|>   >   ATTRIBUTE_NORETURN
|>   > #endif
|>   >   ;
|>   > 
|>   > Perhaps HAVE_cc0 is not yet be set at that point?
|> That would be my guess.

It looks pretty bogus to me to have a function that returns a value but
does not return.  ATTRIBUTE_NORETURN does not make sense on a non-void
function and is most likely misplaced here.

Andreas.

Re: ATTRIBUTE_NORETURN on sets_cc0_p

[Jeffrey A Law]

  In message < 199810150200.TAA14764@smtp.earthlink.net >you write:
  > 
  > On x86-linux-gnu (which defines HAVE_cc0), I'm seeing:
  > 
  > ../../gcc/jump.c: In function `sets_cc0_p':
  > ../../gcc/jump.c:3335: warning: function declared `noreturn' has a `return'
  >  statement
  > 
  > That's not good; this function does return and we don't want the
  > compiler assuming it doesn't.  That could be very dangerous.
  > 
  > I see that rtl.h has:
  > 
  > extern int sets_cc0_p			PROTO ((rtx))
  > #ifndef HAVE_cc0
  >   ATTRIBUTE_NORETURN
  > #endif
  >   ;
  > 
  > Perhaps HAVE_cc0 is not yet be set at that point?
That would be my guess.

Perhaps we should avoid trying to do this right now :-)

jeff

ATTRIBUTE_NORETURN on sets_cc0_p

[Mark Mitchell]

On x86-linux-gnu (which defines HAVE_cc0), I'm seeing:

../../gcc/jump.c: In function `sets_cc0_p':
../../gcc/jump.c:3335: warning: function declared `noreturn' has a `return' statement

That's not good; this function does return and we don't want the
compiler assuming it doesn't.  That could be very dangerous.

I see that rtl.h has:

extern int sets_cc0_p			PROTO ((rtx))
#ifndef HAVE_cc0
  ATTRIBUTE_NORETURN
#endif
  ;

Perhaps HAVE_cc0 is not yet be set at that point?

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com