* [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. @ 2019-04-19 8:43 Eli Zaretskii 2019-04-19 9:52 ` Philipp Stephani 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2019-04-19 8:43 UTC (permalink / raw) To: Philipp Stephani; +Cc: emacs-devel 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 8:43 [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken Eli Zaretskii @ 2019-04-19 9:52 ` Philipp Stephani 2019-04-19 10:08 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Philipp Stephani @ 2019-04-19 9:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Philipp Stephani, Emacs developers 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 9:52 ` Philipp Stephani @ 2019-04-19 10:08 ` Eli Zaretskii 2019-04-19 19:04 ` Paul Eggert 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2019-04-19 10:08 UTC (permalink / raw) To: Philipp Stephani; +Cc: phst, emacs-devel > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 10:08 ` Eli Zaretskii @ 2019-04-19 19:04 ` Paul Eggert 2019-04-19 19:14 ` Philipp Stephani 2019-04-19 20:14 ` Eli Zaretskii 0 siblings, 2 replies; 13+ messages in thread From: Paul Eggert @ 2019-04-19 19:04 UTC (permalink / raw) To: Eli Zaretskii, Philipp Stephani; +Cc: phst, emacs-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 19:04 ` Paul Eggert @ 2019-04-19 19:14 ` Philipp Stephani 2019-04-19 20:16 ` Eli Zaretskii 2019-04-19 20:14 ` Eli Zaretskii 1 sibling, 1 reply; 13+ messages in thread From: Philipp Stephani @ 2019-04-19 19:14 UTC (permalink / raw) To: Paul Eggert; +Cc: Philipp Stephani, Eli Zaretskii, Emacs developers 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 19:14 ` Philipp Stephani @ 2019-04-19 20:16 ` Eli Zaretskii 0 siblings, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2019-04-19 20:16 UTC (permalink / raw) To: Philipp Stephani; +Cc: phst, eggert, emacs-devel > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 19:04 ` Paul Eggert 2019-04-19 19:14 ` Philipp Stephani @ 2019-04-19 20:14 ` Eli Zaretskii 2019-04-19 23:00 ` Paul Eggert 1 sibling, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2019-04-19 20:14 UTC (permalink / raw) To: Paul Eggert; +Cc: phst, p.stephani2, emacs-devel > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 20:14 ` Eli Zaretskii @ 2019-04-19 23:00 ` Paul Eggert 2019-04-20 6:25 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Paul Eggert @ 2019-04-19 23:00 UTC (permalink / raw) To: Eli Zaretskii; +Cc: phst, p.stephani2, emacs-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-19 23:00 ` Paul Eggert @ 2019-04-20 6:25 ` Eli Zaretskii 2019-04-23 0:52 ` Paul Eggert 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2019-04-20 6:25 UTC (permalink / raw) To: Paul Eggert; +Cc: phst, p.stephani2, emacs-devel > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-20 6:25 ` Eli Zaretskii @ 2019-04-23 0:52 ` Paul Eggert 2019-04-23 6:19 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Paul Eggert @ 2019-04-23 0:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: phst, p.stephani2, emacs-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-23 0:52 ` Paul Eggert @ 2019-04-23 6:19 ` Eli Zaretskii 2019-04-23 16:56 ` Paul Eggert 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2019-04-23 6:19 UTC (permalink / raw) To: Paul Eggert; +Cc: phst, p.stephani2, emacs-devel > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-23 6:19 ` Eli Zaretskii @ 2019-04-23 16:56 ` Paul Eggert 2019-04-23 17:19 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Paul Eggert @ 2019-04-23 16:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: phst, p.stephani2, emacs-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken. 2019-04-23 16:56 ` Paul Eggert @ 2019-04-23 17:19 ` Eli Zaretskii 0 siblings, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2019-04-23 17:19 UTC (permalink / raw) To: Paul Eggert; +Cc: phst, p.stephani2, emacs-devel > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-04-23 17:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-19 8:43 [Emacs-diffs] master 74f54af: Use eassume (false) for branch that's never taken Eli Zaretskii 2019-04-19 9:52 ` Philipp Stephani 2019-04-19 10:08 ` Eli Zaretskii 2019-04-19 19:04 ` Paul Eggert 2019-04-19 19:14 ` Philipp Stephani 2019-04-19 20:16 ` Eli Zaretskii 2019-04-19 20:14 ` Eli Zaretskii 2019-04-19 23:00 ` Paul Eggert 2019-04-20 6:25 ` Eli Zaretskii 2019-04-23 0:52 ` Paul Eggert 2019-04-23 6:19 ` Eli Zaretskii 2019-04-23 16:56 ` Paul Eggert 2019-04-23 17:19 ` Eli Zaretskii
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.