[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


From: Paul Eggert
Date: Sat, 25 Jun 2016 23:34:52 +0200
User-agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.8.0

On 06/25/2016 09:48 AM, Eli Zaretskii wrote:
assertions should be always true; if an assertion fails Emacs should
just abort, and should not try to patch around the bug.
That's not how we've been using eassert in Emacs.  Just look around,
and you will see it.

Look around where? Certainly eassert is used that way when ENABLE_CHECKING is defined. And it would be used that way even if ENABLE_CHECKING were not defined, if the runtime cost were zero. Performance concerns should be the only reason we don't have runtime checking enabled in Emacs in production builds. This is true not only for eassert, but also for things like subscript checking, uninitialized variable checking, etc.

The current FACE_FROM_ID doesn't do that, either, when compiled with 
ENABLE_CHECKING not defined.

Sure, but that's merely a performance optimization. The intent is that the predicate must be true in production, just as it must be true when debugging. Enabling checking should not change the behavior of Emacs, other than possibly causing core dumps earlier and possibly hurting performance.

We could, of course, make validate_face I proposed unconditionally
call emacs_abort.  Or we could add such code in-line in all places
that don't already have code to deal with a NULL value coming out of
FACE_FROM_ID.  Would you agree to such a change?

I'm not quite following the suggestion, but if you're suggesting that FACE_FROM_ID should eassert that its result is nonnull, that would be logically OK. In practice, I expect there would be little practical benefit from this, as callers of FACE_FROM_ID invariably dereference the result right away, and this reliably dumps core on typical Emacs platforms anyway. Hence adding the eassert won't catch bugs significantly earlier than they're already caught.

(Note added in rereading this email before sending it: this idea is further analyzed below....)

there were a couple of subtle problems in
the display code that under some rare and complicated use cases caused
FACE_FROM_ID to yield NULL where the subsequent code didn't expect

Were these because the ID argument was out of range, or because ID was in range but the resulting pointer was null? If the former, the current code already catches that if ENABLE_CHECKING is true; if the latter, typical platforms already catch that immediately now, as described above.

So, to summarize, here's the proposal.

  . Define FACE_FROM_ID as it was before:

    #define FACE_FROM_ID(F, ID)                         \
        (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used) \
         ? FRAME_FACE_CACHE (F)->faces_by_id[ID]             \
         : NULL)

  . In every place that calls FACE_FROM_ID and doesn't already include
    code to deal with a NULL value, do this:

      face = FACE_FROM_ID (f, face_id);
      if (!face)
        emacs_abort ();


If we go this route, I suggest having a function NONNULL_FACE_FROM_ID that packages up those three lines of code (which will occur in many places). That is, instead of this:

  face = FACE_FROM_ID (f, face_id);
  if (!face)
    emacs_abort ();

we should write this:

  face = NONNULL_FACE_FROM_ID (f, face_id);

to mean the same thing. (Or perhaps you can think of a better name than NONNULL_FACE_FROM_ID.)

This sort of thing should also suffice to remove the warning. A downside is that it inserts unnecessary runtime checks in production code, as the "if (!face) emacs_abort ();" won't detect errors in production usefully earlier than the existing code does when it dereferences the pointer. In practice the unnecessary checks probably won't cost that much, so it's OK if nobody else cares about the performance degradation in production code. (I tried hard to avoid slowing down Emacs merely to pacify GCC, and if we go this route we'd be departing slightly from that tradition.)

If you like this idea I can prepare a more-detailed proposal along these lines.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]