Fkill_emacs NO_RETURN

Fkill_emacs NO_RETURN

[Eli Zaretskii]

This change:

    2006-04-09  Dan Nicolaescu  <dann@ics.uci.edu>

	    * puresize.h (pure_write_error): Mark as NO_RETURN.

	    * lisp.h (args_out_of_range, args_out_of_range_3)
	    (Fkill_emacs): Likewise.

marks Fkill_emacs as __attribute__((no_return)) for those versions of
GCC which support that.  I think this change is for the worse: now GCC
whines that a function that is marked no-return returns a value.  This
is because Fkill_emacs has this at its end:

      exit (INTEGERP (arg) ? XINT (arg) : EXIT_SUCCESS);
      /* NOTREACHED */
      return Qnil;

The ``NOTREACHED return'' is there because DEFUN declares a function
that returns a Lisp_Object, and some compilers will complain if
there's no return statement in such a function.

So either we find a clean way to condition `return Qnil' on NO_RETURN
being defined to nothing, or we should revert this change.  (Why was
it made, anyway?)

Re: Fkill_emacs NO_RETURN

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

  > This change:
  > 
  >     2006-04-09  Dan Nicolaescu  <dann@ics.uci.edu>
  > 
  > 	    * puresize.h (pure_write_error): Mark as NO_RETURN.
  > 
  > 	    * lisp.h (args_out_of_range, args_out_of_range_3)
  > 	    (Fkill_emacs): Likewise.
  > 
  > marks Fkill_emacs as __attribute__((no_return)) for those versions of
  > GCC which support that.  I think this change is for the worse: now GCC
  > whines that a function that is marked no-return returns a value.  This
  > is because Fkill_emacs has this at its end:
  > 
  >       exit (INTEGERP (arg) ? XINT (arg) : EXIT_SUCCESS);
  >       /* NOTREACHED */
  >       return Qnil;
  > 
  > The ``NOTREACHED return'' is there because DEFUN declares a function
  > that returns a Lisp_Object, and some compilers will complain if
  > there's no return statement in such a function.
  > 
  > So either we find a clean way to condition `return Qnil' on NO_RETURN
  > being defined to nothing, or we should revert this change.  

How about just deleting the return statement?
gcc has supported the noreturn attribute since at least gcc-2.95. 
Other compilers might just warn if they don't realize that "exit" does
not return. I think quite a few modern compilers know that.

  > (Why was it made, anyway?)

One of the Coverity reports was due to the fact that the tool did not
realize that Fkill_emacs cannot return. 

IMO marking the function as not returning will prevent having to
analyze similar errors in the future with other checking tools.

Re: Fkill_emacs NO_RETURN

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dan Nicolaescu <dann@ics.uci.edu>
> Date: Sun, 09 Apr 2006 12:13:36 -0700
> 
>   >       exit (INTEGERP (arg) ? XINT (arg) : EXIT_SUCCESS);
>   >       /* NOTREACHED */
>   >       return Qnil;
>   > 
>   > The ``NOTREACHED return'' is there because DEFUN declares a function
>   > that returns a Lisp_Object, and some compilers will complain if
>   > there's no return statement in such a function.
>   > 
>   > So either we find a clean way to condition `return Qnil' on NO_RETURN
>   > being defined to nothing, or we should revert this change.  
> 
> How about just deleting the return statement?

I thought I explained above why this was not a good idea.

> gcc has supported the noreturn attribute since at least gcc-2.95. 
> Other compilers might just warn if they don't realize that "exit" does
> not return. I think quite a few modern compilers know that.

You'd be surprised how many modern compilers whine about a non-void
function without a return statement.

>   > (Why was it made, anyway?)
> 
> One of the Coverity reports was due to the fact that the tool did not
> realize that Fkill_emacs cannot return. 
> 
> IMO marking the function as not returning will prevent having to
> analyze similar errors in the future with other checking tools.

That's fine with me, but the solution needs to be clean, and in
particular it shouldn't introduce new warning from GCC.

Re: Fkill_emacs NO_RETURN

[Richard Stallman]

I deleted the NO_RETURN on Fkill_emacs,
because that was clearly the safest fix.

Another safe fix is to conditionalize the return statement with
#ifndef __GNUC__.  But I don't see a need to do so.  We should not
twist our code in knots just to satisfy a lint program.

Re: Fkill_emacs NO_RETURN

[Dan Nicolaescu]

Richard Stallman <rms@gnu.org> writes:

  > I deleted the NO_RETURN on Fkill_emacs,
  > because that was clearly the safest fix.
  > 
  > Another safe fix is to conditionalize the return statement with
  > #ifndef __GNUC__.  But I don't see a need to do so.  We should not
  > twist our code in knots just to satisfy a lint program.

Also consider just deleting the return statement. Why? 
 - gcc would not have a problem with that, it would not warn for a
   missing return for a NO_RETURN function.  
 - Other compilers might warn about it, but the warning would be
   incorrect anyway 

Marking functions NO_RETURN is desirable from a few points of view:
 -it helps gcc do a better job for the -Wuninitialized warning
 -it helps lint type tools, it will avoid analyzing the same code over
 and over when a new tool warns about it only to discover there's no
 problem. 
 -it helps code generation: for example by just marking
 `wrong_type_argument' as NO_RETURN the text size decreases from 
  1483168 bytes to 1474080 bytes (ie ~9KB) on my x86 system using
  gcc-4.1
 -gcc has a -Wmissing-noreturn flag that helps identify candidate
 functions. There are around 20 of them in emacs now. 

Re: Fkill_emacs NO_RETURN

[Richard Stallman]

I deleted the NO_RETURN for Fkill_emacs to avoid hassles.

    Marking functions NO_RETURN is desirable from a few points of view:
     -it helps gcc do a better job for the -Wuninitialized warning

Surely Fkill_emacs makes no difference to this.

     -it helps lint type tools, it will avoid analyzing the same code over
     and over when a new tool warns about it only to discover there's no
     problem. 

Not terribly important.

     -it helps code generation: for example by just marking
     `wrong_type_argument' as NO_RETURN the text size decreases from 
      1483168 bytes to 1474080 bytes (ie ~9KB) on my x86 system using
      gcc-4.1

That may be significant for wrong_type_argument, but not for Fkill_emacs.

Perhaps it is worth adding the NO_RETURN for wrong_type_argument.
It is called a lot more than Fkill_emacs.

Re: Fkill_emacs NO_RETURN

[Dan Nicolaescu]

Richard Stallman <rms@gnu.org> writes:

  > I deleted the NO_RETURN for Fkill_emacs to avoid hassles.
  > 
  >     Marking functions NO_RETURN is desirable from a few points of view:
  >      -it helps gcc do a better job for the -Wuninitialized warning
  > 
  > Surely Fkill_emacs makes no difference to this.
  > 
  >      -it helps lint type tools, it will avoid analyzing the same code over
  >      and over when a new tool warns about it only to discover there's no
  >      problem. 
  > 
  > Not terribly important.
  > 
  >      -it helps code generation: for example by just marking
  >      `wrong_type_argument' as NO_RETURN the text size decreases from 
  >       1483168 bytes to 1474080 bytes (ie ~9KB) on my x86 system using
  >       gcc-4.1
  > 
  > That may be significant for wrong_type_argument, but not for Fkill_emacs.
  > 
  > Perhaps it is worth adding the NO_RETURN for wrong_type_argument.
  > It is called a lot more than Fkill_emacs.

The reason I referred to wrong_type_argument is that it has the same
issue as Fkill_emacs: it has a return statement, so marking it as
NO_RETURN would mean having to do something about the return statement.

So please make a decision about marking wrong_type_argument and/or
Fkill_emacs as NO_RETURN.

Re: Fkill_emacs NO_RETURN

[Richard Stallman]

    The reason I referred to wrong_type_argument is that it has the same
    issue as Fkill_emacs: it has a return statement, so marking it as
    NO_RETURN would mean having to do something about the return statement.

For Fkill_emacs it's not worth the trouble.
For wrong_type_argument, I think it is worth the trouble.
So please add NO_RETURN for wrong_type_argument,
and please conditionalize its return statements
so that all compilers will be happy.

Re: Fkill_emacs NO_RETURN

[Stefan Monnier]

>       exit (INTEGERP (arg) ? XINT (arg) : EXIT_SUCCESS);
>       /* NOTREACHED */
>       return Qnil;

> The ``NOTREACHED return'' is there because DEFUN declares a function
> that returns a Lisp_Object, and some compilers will complain if
> there's no return statement in such a function.

> So either we find a clean way to condition `return Qnil' on NO_RETURN
> being defined to nothing, or we should revert this change.  (Why was
> it made, anyway?)

I guess we'd need to mark `exit' as NORETRUN.
Or Coverity should improve/implement the automatic inference of
NORETURN annotations.


        Stefan

Re: Fkill_emacs NO_RETURN

[Eli Zaretskii]

> Cc: Dan Nicolaescu <dann@ics.uci.edu>,  emacs-devel@gnu.org
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 09 Apr 2006 20:35:05 -0400
> 
> I guess we'd need to mark `exit' as NORETRUN.

Did you look at your stdlib.h?  `exit' is already marked as NO_RETURN
(or at least it should be).

Re: Fkill_emacs NO_RETURN

[Stefan Monnier]

>> I guess we'd need to mark `exit' as NORETRUN.
> Did you look at your stdlib.h?  `exit' is already marked as NO_RETURN
> (or at least it should be).

Then the problem is with Coverity.


        Stefan