[Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

Philipp, I don't understand this change.  The function in question
should return a Lisp_Object, and AFAIU eassume doesn't do that when
ENABLE_CHECKING is not defined.  What am I missing?

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Philipp Stephani]

Am Fr., 19. Apr. 2019 um 10:43 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> Philipp, I don't understand this change.  The function in question
> should return a Lisp_Object, and AFAIU eassume doesn't do that when
> ENABLE_CHECKING is not defined.  What am I missing?
>

The default branch can never be taken. We use eassume(false) in many
other places to document this.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 19 Apr 2019 11:52:13 +0200
> Cc: Philipp Stephani <phst@google.com>, Emacs developers <emacs-devel@gnu.org>
> 
> Am Fr., 19. Apr. 2019 um 10:43 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > Philipp, I don't understand this change.  The function in question
> > should return a Lisp_Object, and AFAIU eassume doesn't do that when
> > ENABLE_CHECKING is not defined.  What am I missing?
> >
> 
> The default branch can never be taken. We use eassume(false) in many
> other places to document this.

AFAICT, all those places don't need to return anything, which is
unlike here.

When ENABLE_CHECKING is not defined, which happens in every production
build, eassume expands to code that has no side effects, so the
function will return a random value to its caller.  I don't think this
is desired.  If you want to make sure this branch is never taken even
in a production build, simply call emacs_abort there.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Paul Eggert]

On 4/19/19 3:08 AM, Eli Zaretskii wrote:
> When ENABLE_CHECKING is not defined, which happens in every production
> build, eassume expands to code that has no side effects, so the
> function will return a random value to its caller.  I don't think this
> is desired.

It looks OK to me. The default branch is impossible, so the function
cannot return a random value to its caller. Although GCC is not smart
enough to deduce this fact, eassume lets the programmer communicate it
to GCC so that GCC doesn't issue a false-alarm diagnostic (and GCC also
can generate better code). When ENABLE_CHECKING is defined, there's also
a runtime check that the impossible does not happen, but this extra
check isn't needed in ordinary production.

This would all be simpler if we replaced 'enum nonlocal_exit' with a
simple boolean. The enum seems to be more trouble than it's worth, if
it's causing this sort of bikeshedding. Not every boolean deserves an
enum just for it.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Philipp Stephani]

Am Fr., 19. Apr. 2019 um 21:04 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
>
> On 4/19/19 3:08 AM, Eli Zaretskii wrote:
> > When ENABLE_CHECKING is not defined, which happens in every production
> > build, eassume expands to code that has no side effects, so the
> > function will return a random value to its caller.  I don't think this
> > is desired.
>
> It looks OK to me. The default branch is impossible, so the function
> cannot return a random value to its caller. Although GCC is not smart
> enough to deduce this fact, eassume lets the programmer communicate it
> to GCC so that GCC doesn't issue a false-alarm diagnostic (and GCC also
> can generate better code). When ENABLE_CHECKING is defined, there's also
> a runtime check that the impossible does not happen, but this extra
> check isn't needed in ordinary production.

FWIW, I'm fine with both eassume(false) and emacs_abort.

>
> This would all be simpler if we replaced 'enum nonlocal_exit' with a
> simple boolean. The enum seems to be more trouble than it's worth, if
> it's causing this sort of bikeshedding. Not every boolean deserves an
> enum just for it.
>

I tend to disagree: In APIs, booleans should only be used for values
that have a clear false and true state (e.g. predicates). In other
cases booleans reduce readability (because "false" and "true" are
meaningless by themselves) and make extending unnecessarily hard.

I'm actually surprised that compilers warn about the lack of the
default case. The typical use for this construct is to signal that the
switch statement intends to handle all cases, causing a compiler
warning if new enumerators are added, which is very useful because it
reminds you of the callers that need to be adapted. Therefore it's
better to leave out the default case.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 19 Apr 2019 21:14:52 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, Philipp Stephani <phst@google.com>, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> FWIW, I'm fine with both eassume(false) and emacs_abort.

Then please use the latter.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

> Cc: phst@google.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 19 Apr 2019 12:04:51 -0700
> 
> On 4/19/19 3:08 AM, Eli Zaretskii wrote:
> > When ENABLE_CHECKING is not defined, which happens in every production
> > build, eassume expands to code that has no side effects, so the
> > function will return a random value to its caller.  I don't think this
> > is desired.
> 
> It looks OK to me. The default branch is impossible, so the function
> cannot return a random value to its caller.

But what bothers me is what happens when the "impossible" becomes
possible, i.e. if the value is neither of the two handled explicitly.
This is what the 'default' branch is for, no?  When this happens in
the production build, the function should still return some
predictable value to the caller.

Otherwise, why have the default branch at all?

> this sort of bikeshedding

Thanks a lot!  A great incentive to trying to keep our sources clean.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Paul Eggert]

Eli Zaretskii wrote:
> But what bothers me is what happens when the "impossible" becomes
> possible, i.e. if the value is neither of the two handled explicitly.

It's not possible. Really.

> why have the default branch at all?

It's there only to pacify GCC. It has no other function.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, phst@google.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 19 Apr 2019 16:00:50 -0700
> 
> Eli Zaretskii wrote:
> > But what bothers me is what happens when the "impossible" becomes
> > possible, i.e. if the value is neither of the two handled explicitly.
> 
> It's not possible. Really.

Anything's possible with bugs.  AFAIU, that's what this discussion is
about.  Because if it's _really_ impossible, then eassume has no place
there, either.

[Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Paul Eggert]

Eli Zaretskii wrote:
> Anything's possible with bugs.  AFAIU, that's what this discussion is
> about.  Because if it's_really_  impossible, then eassume has no place
> there, either.

Let me try to explain. eassume is designed for the situation where the
programmer knows something that the compiler does not infer on its own,
and where this knowledge can help the compiler produce better
diagnostics or better code.

Here is a toy example. Suppose GCC was so amazingly dumb that if you did
this:

   {
      int i = 27;
      return 1000 / i;
   }

then GCC warned "possible integer division by zero" and inserted a
runtime check (just before the 'return' statement) that 'i' is nonzero.
And suppose you could disable the warning (and improve performance) by
doing this instead:

   {
      int i = 27;
      eassume (i == 27);
      return 1000 / i;
   }

Would we reject this solution because "if it's _really_ impossible, then
eassume has no place there"?  No, because it really *is* impossible for
i != 27 there; but in this (very contrived) example, eassume *does* have
a place, namely to pacify the amazingly dumb compiler.

The case that started this thread is similar, except that GCC is not as
dumb as in the contrived example above.

One might at first think that because the programmer might have made a
mistake and it's better to be safe than sorry, we should replace
instances of 'eassume (X);' with 'if (!X) emacs_abort ();' so that there
is always a runtime check, even in production. But that would be
overkill, for the same reason that replacing all instances of 'eassert
(X);' with 'if (!X) emacs_abort ();' would be overkill.

By the way, now that we have -fsanitize=undefined, it would be realistic
to simplify Emacs by dropping 'eassume' and replacing all uses with
plain 'assume', as modern compilers will do the runtime check for us
automatically (if we use -fsanitize=reachable), and older compilers are
kind of lost causes anyway.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: p.stephani2@gmail.com, phst@google.com, emacs-devel@gnu.org
> Date: Mon, 22 Apr 2019 17:52:32 -0700
> 
>    {
>       int i = 27;
>       eassume (i == 27);
>       return 1000 / i;
>    }
> 
> Would we reject this solution because "if it's _really_ impossible, then
> eassume has no place there"?  No, because it really *is* impossible for
> i != 27 there; but in this (very contrived) example, eassume *does* have
> a place, namely to pacify the amazingly dumb compiler.
> 
> The case that started this thread is similar, except that GCC is not as
> dumb as in the contrived example above.
> 
> One might at first think that because the programmer might have made a
> mistake and it's better to be safe than sorry, we should replace
> instances of 'eassume (X);' with 'if (!X) emacs_abort ();' so that there
> is always a runtime check, even in production. But that would be
> overkill, for the same reason that replacing all instances of 'eassert
> (X);' with 'if (!X) emacs_abort ();' would be overkill.

My mental model of using assertions in Emacs is slightly different.
In my model, we use eassert for things that "cannot happen", but can
be tolerated in some sense in a production build.  "Tolerate" here
means that the result could be incorrect display or some strange error
message or a crash in some unrelated place.  If something that "cannot
happen" causes an immediate problem, i.e. the code simply cannot
continue, then we should call emacs_abort instead.

eassume is the same as eassert, it just helps the compiler generate
better code in optimized builds, so conceptually it isn't different,
to my mind.

I learned long ago that in programming, "things which cannot happen,
will", so I don't consider calling emacs_abort overkill where
appropriate.

> By the way, now that we have -fsanitize=undefined, it would be realistic
> to simplify Emacs by dropping 'eassume' and replacing all uses with
> plain 'assume', as modern compilers will do the runtime check for us
> automatically (if we use -fsanitize=reachable), and older compilers are
> kind of lost causes anyway.

If non-production builds use -fsanitize=reachable, and if doing so
will cause an abort when the condition is violated, then yes, maybe we
should do that.  We still have eassert, though.  And it doesn't help
that with current build machinery one needs to manually specify all
the compiler switches, instead of using some simple configure switch
that automatically does that for us.  Using one more switch increases
that burden slightly.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Paul Eggert]

On 4/22/19 11:19 PM, Eli Zaretskii wrote:
> My mental model of using assertions in Emacs is slightly different.
> In my model, we use eassert for things that "cannot happen", but can
> be tolerated in some sense in a production build.  "Tolerate" here
> means that the result could be incorrect display or some strange error
> message or a crash in some unrelated place.

This is not a model I'm familiar with, and many (most?) executions of
eassert don't behave that way. For example, when XCAR (via XCONS) uses
eassert to check that its argument is tagged as a cons, any assertion
failure means Emacs is in a seriously bad state. Quite possibly Emacs
will crash immediately; but even if Emacs lucks out and doesn't crash
immediately it's not something that should be tolerated.

> If something that "cannot
> happen" causes an immediate problem, i.e. the code simply cannot
> continue, then we should call emacs_abort instead.

Again, that's not what I would expect. Many (most?) executions of 'if
(!X) emacs_abort ();' won't necessarily prevent an immediate problem.
For example, string_bytes has such a test, even though string_bytes
won't crash immediately if the test is omitted.

In practice, I think the more accurate characterization is that we use
eassert for runtime checks done in testing but not in production, and we
use emacs_abort for runtime checks always done even in production. We're
more likely to prefer emacs_abort to eassert if the runtime check is
cheap or is rarely needed, or if the failure is more likely or has worse
effects. Whether the failure would occur immediately after the check is
not that relevant.


> If non-production builds use -fsanitize=reachable, and if doing so
> will cause an abort when the condition is violated, then yes, maybe we
> should do that.

-fsanitize=undefined implies -fsanitize=reachable, so if we encourage
the use of -fsanitize=undefined then we should be on a good path here.


> And it doesn't help
> that with current build machinery one needs to manually specify all
> the compiler switches, instead of using some simple configure switch
> that automatically does that for us.  Using one more switch increases
> that burden slightly.

We could have --enable-checking default to -fsanitize=undefined on
platforms that support it.

Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken.

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, phst@google.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 23 Apr 2019 09:56:15 -0700
> 
> On 4/22/19 11:19 PM, Eli Zaretskii wrote:
> > My mental model of using assertions in Emacs is slightly different.
> > In my model, we use eassert for things that "cannot happen", but can
> > be tolerated in some sense in a production build.  "Tolerate" here
> > means that the result could be incorrect display or some strange error
> > message or a crash in some unrelated place.
> 
> This is not a model I'm familiar with, and many (most?) executions of
> eassert don't behave that way. For example, when XCAR (via XCONS) uses
> eassert to check that its argument is tagged as a cons, any assertion
> failure means Emacs is in a seriously bad state. Quite possibly Emacs
> will crash immediately;

You just said in different words what I described.

> but even if Emacs lucks out and doesn't crash immediately it's not
> something that should be tolerated.

My model disagrees with "should" there.  IMO, it's a judgment call
when to tolerate that and when not.

> For example, string_bytes has such a test, even though string_bytes
> won't crash immediately if the test is omitted.

I didn't say my model is followed consistently throughout our
sources.  It's possible that string_bytes needs to be changed
(assuming that I will convince you to adopt my model ;-).

> In practice, I think the more accurate characterization is that we use
> eassert for runtime checks done in testing but not in production, and we
> use emacs_abort for runtime checks always done even in production.

That is also consistent with what I said.

> We're more likely to prefer emacs_abort to eassert if the runtime
> check is cheap or is rarely needed, or if the failure is more likely
> or has worse effects. Whether the failure would occur immediately
> after the check is not that relevant.

Like I said, it's a judgment call.  What you describe are all valid
considerations, but they don't contradict my model.

> > And it doesn't help
> > that with current build machinery one needs to manually specify all
> > the compiler switches, instead of using some simple configure switch
> > that automatically does that for us.  Using one more switch increases
> > that burden slightly.
> 
> We could have --enable-checking default to -fsanitize=undefined on
> platforms that support it.

If it doesn't tremendously slow down Emacs, I think we should, yes.