Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert <eggert@cs.ucla.edu>]

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
> that.

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 ();
>
>   . Remove FACE_OPT_FROM_ID
>

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.