FACE_FROM_ID vs FACE_OPT_FROM_ID

FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

Paul, can you explain why we need two macros where we had only one?
Two macros with similar effect and unclear rules for when to use each
one doesn't sound like a good idea to me.  I've read the commit log
message, which attempts to explain the preference, but I don't think I
understand the criterion -- any face ID can cause FACE_FROM_ID to
produce NULL, if called in some inopportune moment.  I also looked at
the places where you used each macro, and I cannot understand why you
decided to use this or that, maybe I'm missing something.

What was the GCC 6.1 complaint that led you to this change?

Same questions for IMAGE_FROM_ID.

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

On 06/23/2016 10:03 PM, Eli Zaretskii wrote:
> What was the GCC 6.1 complaint that led you to this change?
Diagnostics like this one (there were quite a few):

> xdisp.c: In function 'fill_image_glyph_string':
> xdisp.c:24916:20: error: potential null pointer dereference 
> [-Werror=null-dereference]
>    s->font = s->face->font;
>              ~~~~~~~^~~~~~

The problem being that s->face was set from FACE_FROM_ID, which (before 
the change) might return a null pointer.

The intent is that FACE_FROM_ID returns a face (a non-null pointer), 
whereas FACE_OPT_FROM_ID returns a face option (either a face or a null 
pointer). Similarly for IMAGE_FROM_ID. I installed the attached patch to 
try to make this clearer.

[-- Attachment #2: 0001-Clarify-intent-of-FACE_FROM_ID-and-IMAGE_FROM_ID.txt --]
[-- Type: text/plain, Size: 2012 bytes --]

From b3ef03f646c8fb3dc6d55a8ed0134b6963243f86 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 24 Jun 2016 02:19:13 +0200
Subject: [PATCH] Clarify intent of FACE_FROM_ID and IMAGE_FROM_ID
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/dispextern.h (FACE_OPT_FROM_ID): Don’t use FACE_FROM_ID,
since it is intended to be used only when it returns a non-null
pointer, and here the pointer might be null.
(IMAGE_OPT_FROM_ID): Don’t use IMAGE_FROM_ID, for similar reasons.
---
 src/dispextern.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index d0fc3b2..08dcd89 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1812,7 +1812,7 @@ struct face_cache
   bool_bf menu_face_changed_p : 1;
 };
 
-/* Return a pointer to the cached face with ID on frame F.  */
+/* Return a non-null pointer to the cached face with ID on frame F.  */
 
 #define FACE_FROM_ID(F, ID)					\
   (eassert (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)),	\
@@ -1823,7 +1823,7 @@ struct face_cache
 
 #define FACE_OPT_FROM_ID(F, ID)				\
   (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)	\
-   ? FACE_FROM_ID (F, ID)				\
+   ? FRAME_FACE_CACHE (F)->faces_by_id[ID]		\
    : NULL)
 
 /* True if FACE is suitable for displaying ASCII characters.  */
@@ -3093,7 +3093,7 @@ struct image_cache
 };
 
 
-/* A pointer to the image with id ID on frame F.  */
+/* A non-null pointer to the image with id ID on frame F.  */
 
 #define IMAGE_FROM_ID(F, ID)					\
   (eassert (UNSIGNED_CMP (ID, <, FRAME_IMAGE_CACHE (F)->used)),	\
@@ -3104,7 +3104,7 @@ struct image_cache
 
 #define IMAGE_OPT_FROM_ID(F, ID)				\
   (UNSIGNED_CMP (ID, <, FRAME_IMAGE_CACHE (F)->used)		\
-   ? IMAGE_FROM_ID (F, ID)					\
+   ? FRAME_IMAGE_CACHE (F)->images[ID]				\
    : NULL)
 
 /* Size of bucket vector of image caches.  Should be prime.  */
-- 
2.5.5

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 24 Jun 2016 02:23:17 +0200
> 
> > What was the GCC 6.1 complaint that led you to this change?
> Diagnostics like this one (there were quite a few):
> 
> > xdisp.c: In function 'fill_image_glyph_string':
> > xdisp.c:24916:20: error: potential null pointer dereference 
> > [-Werror=null-dereference]
> >    s->font = s->face->font;
> >              ~~~~~~~^~~~~~
> 
> The problem being that s->face was set from FACE_FROM_ID, which (before 
> the change) might return a null pointer.

Yes, and on purpose.  If the face pointer cannot be determined from
its ID, we have no other alternative but crash (or abort) anyway.

> The intent is that FACE_FROM_ID returns a face (a non-null pointer), 
> whereas FACE_OPT_FROM_ID returns a face option (either a face or a null 
> pointer).

But that's not true, see.  When the face ID is not a valid value, as
tested against FRAME_FACE_CACHE (F)->used, this macro:

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

when compiled without --enable-checking, will index the
FRAME_FACE_CACHE (F)->face_by_id[] array with an invalid index, and
either segfault or produce some random garbage that will cause trouble
elsewhere.  So we are not solving a real problem here, just shutting
up the (stupid, IMO) warning from a compiler, at a price of
introducing one more macro and making our code more complex and hard
to understand.

IOW, the protection offered by 'eassert' is ephemeral, because it
compiles to nothing in production, and we are in fact replacing code
that would surely crash in that case with code that might either crash
or continue with garbage.

So I don't like this solution.

Do we have to use -Werror=null-dereference?  Is it really that useful?
If not, I think we should just revert these changes, remove that flag
from the compiler options we use, and continue using the original
macros FACE_FROM_ID and IMAGE_FROM_ID.

If we do want to use -Werror=null-dereference, then I'd like to make
our code really better, albeit a very tiny bit so, by making the
definition of FACE_FROM_ID identical to that of FACE_OPT_FROM_ID, and
then using in all places that currently call FACE_FROM_ID something
like the following:

  struct face *face = FACE_FROM_ID (f, face_id);
  face = validate_face (f, face);

where validate_face is something like this:

struct face *
validate_face (struct frame *f, struct face *face)
{
  eassert (face);
  if (!face)
    {
      face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
      if (!face)
        emacs_abort ();
    }
  return face;
}

Where we currently call FACE_OPT_FROM_ID, we will simply call
FACE_FROM_ID, without the validation, because the following code
already deals with NULL values.

Then we can get rid of FACE_OPT_FROM_ID.

WDYT?

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert]

On 06/24/2016 10:49 AM, Eli Zaretskii wrote:
> Yes, and on purpose. If the face pointer cannot be determined from its 
> ID, we have no other alternative but crash (or abort) anyway.

Of course, and FACE_FROM_ID is intended to be used in situations like 
this. However, in some cases the caller can handle the situation where a 
face pointer cannot be determined from its ID, and FACE_OPT_FROM_ID is 
intended to be used in those cases. It is helpful to the reader (at 
least, to this reader) to easily see which case is which.

> When the face ID is not a valid value, as tested against 
> FRAME_FACE_CACHE (F)->used, ... FACE_FROM_ID(F, ID), ... when compiled 
> without --enable-checking, will index the FRAME_FACE_CACHE 
> (F)->face_by_id[] array with an invalid index, and either segfault or 
> produce some random garbage that will cause trouble elsewhere. So we 
> are not solving a real problem here

?! Yes we are. The eassert is helpful, since it reliably crashes Emacs 
when ID is out-of-range, something that is useful when debugging. On all 
platforms I know of there is no need to add an eassert that the 
resulting pointer is non-null, since the caller is about to dereference 
it anyway and that will reliably cause a crash. However, it is helpful 
to eassert that ID is in range, since most modern platforms lack 
reliable subscript checking.

With FACE_FROM_ID, the ID should never be out-of-range if Emacs is 
written correctly, so it's OK to omit the runtime check from production 
code, for performance. This is not true for FACE_OPT_FROM_ID; it is 
supposed to return NULL when ID is out of range, and callers are 
supposed to avoid dereferencing the resulting pointer if it is NULL.

> just shutting up the (stupid, IMO) warning from a compiler

It is not a stupid warning. It is a useful warning. The modern trend in 
statically-typed languages is to distinguish "possibly-null pointer to 
X" from "non-null pointer to X". That way, a compile-time check can 
reliably detect the error of dereferencing null pointers, which is a 
real problem in many applications (including Emacs). C does not have 
this notion directly and in general a C compiler cannot detect the error 
statically. That being said, GCC has reasonable heuristics to check for 
the error in many cases, and it's useful to enable this checking to 
catch silly programming errors. (Of course code that you and I write 
would never have these errors; it's always *other* people's code. :-)

> eassert (face); if (!face) { ...}

eassert (X) means that X must be nonzero, so there should never be a 
need for a runtime check !X after a call to eassert (X).

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 24 Jun 2016 11:27:26 +0200
> 
> > eassert (face); if (!face) { ...}
> 
> eassert (X) means that X must be nonzero, so there should never be a 
> need for a runtime check !X after a call to eassert (X).

Maybe I'm missing something, but my reading of this:

#ifndef ENABLE_CHECKING
# define eassert(cond) ((void) (false && (cond))) /* Check COND compiles.  */
# define eassume(cond) assume (cond)
#else /* ENABLE_CHECKING */

extern _Noreturn void die (const char *, const char *, int);

extern bool suppress_checking EXTERNALLY_VISIBLE;

# define eassert(cond)                                          \
   (suppress_checking || (cond)                                 \
    ? (void) 0                                                  \
    : die (# cond, __FILE__, __LINE__))

is that when ENABLE_CHECKING is not defined and suppress_checking is
true, eassert does nothing.

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

[Please ignore the previous message, it was confused.]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 24 Jun 2016 11:27:26 +0200
> 
> > eassert (face); if (!face) { ...}
> 
> eassert (X) means that X must be nonzero, so there should never be a 
> need for a runtime check !X after a call to eassert (X).

Maybe I'm missing something, but my reading of this:

#ifndef ENABLE_CHECKING
# define eassert(cond) ((void) (false && (cond))) /* Check COND compiles.  */

is that when ENABLE_CHECKING is not defined, eassert does nothing
useful.

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert]

On 06/24/2016 12:00 PM, Eli Zaretskii wrote:
>> my reading of this:
>>
>> #ifndef ENABLE_CHECKING
>> # define eassert(cond) ((void) (false && (cond))) /* Check COND compiles.  */
>>
>> is that when ENABLE_CHECKING is not defined, eassert does nothing
>> useful.

Yes, of course.  eassert (X) should appear only in places where X must 
be true, i.e., where Emacs is seriously broken if X is false. That's the 
standard meaning for assertions.

Another way to put it is that in general the behavior of eassert (X) is 
undefined if X is false.When (defined ENABLE_CHECKING && 
!suppress_checking), this undefined behavior happens to be a diagnostic 
and core dump. Otherwise the undefined behavior is whatever the 
underlying system does afterwards; when (!defined ENABLE_CHECKING) this 
yields better performance overall in the usual and expected case where 
Emacs is working properly.

It is not necessary to put eassert (X) every place where an expression X 
must be true. It's helpful only when we reasonably suspect there might 
be a bug in Emacs, and where platforms typically will not immediately 
fail for other reasons when X is false. So, before a pointer dereference 
*P it's not necessary to do eassert (P != NULL) since typical platforms 
immediately dump core when dereferencing a null pointer anyway. 
Conversely, before a subscript operation A[I] it can be helpful to 
eassert (0 <= I && I < N), since typical platforms lack reliable 
subscript checking.

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 24 Jun 2016 13:17:39 +0200
> 
> On 06/24/2016 12:00 PM, Eli Zaretskii wrote:
> >> my reading of this:
> >>
> >> #ifndef ENABLE_CHECKING
> >> # define eassert(cond) ((void) (false && (cond))) /* Check COND compiles.  */
> >>
> >> is that when ENABLE_CHECKING is not defined, eassert does nothing
> >> useful.
> 
> Yes, of course.  eassert (X) should appear only in places where X must 
> be true, i.e., where Emacs is seriously broken if X is false. That's the 
> standard meaning for assertions.

And that is what happens here: if the face ID is not in the face
cache, something is broken, and we cannot continue using that face ID.

In the ENABLE_CHECKING case, we abort immediately, since that's the
meaning of ENABLE_CHECKING: allow early detection of invalid data.
When ENABLE_CHECKING is not defined, which is what happens by default
in Emacs builds, we attempt to "do the best we can without crashing",
thus my suggestion to use the default face, if that is in the cache.
If even that is not in the cache, we cannot display anything, so I
suggest we abort.

If you find the code I proposed after eassert confusing, then we can
condition it on ENABLE_CHECKING being not defined.

Otherwise, I think my suggestion is better than what we have now,
because the current code, when compiled without ENABLE_CHECKING, will
attempt to use invalid face IDs.

> Another way to put it is that in general the behavior of eassert (X) is 
> undefined if X is false.When (defined ENABLE_CHECKING && 
> !suppress_checking), this undefined behavior happens to be a diagnostic 
> and core dump. Otherwise the undefined behavior is whatever the 
> underlying system does afterwards; when (!defined ENABLE_CHECKING) this 
> yields better performance overall in the usual and expected case where 
> Emacs is working properly.

My suggestion was an attempt to make the undefined behavior safer, as
much as it's possible.  That is in line with the general practice in
Emacs: to avoid crashing as much as possible, leaving the user in
charge, so she could shut down the session in an orderly fashion, thus
minimizing the risk of losing precious work.

> It is not necessary to put eassert (X) every place where an expression X 
> must be true. It's helpful only when we reasonably suspect there might 
> be a bug in Emacs, and where platforms typically will not immediately 
> fail for other reasons when X is false. So, before a pointer dereference 
> *P it's not necessary to do eassert (P != NULL) since typical platforms 
> immediately dump core when dereferencing a null pointer anyway. 

That accurately describes the FACE_FROM_ID macro we had before these
changes, and the way we used it.  We could keep that code, but you say
the GCC warning about that is useful, so I suggested a way of avoiding
the warning that also makes our code safer.  After all, if the warning
is useful, we should indeed make changes that improve our code, not
just shut up the warning.

> Conversely, before a subscript operation A[I] it can be helpful to 
> eassert (0 <= I && I < N), since typical platforms lack reliable 
> subscript checking.

And that's exactly what is missing from current uses of FACE_FROM_ID,
when ENABLE_CHECKING is not defined.

So, turning back to my suggestion, do you still object to it?

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert]

On 06/24/2016 03:43 PM, Eli Zaretskii wrote:
> When ENABLE_CHECKING is not defined, which is what happens by default
> in Emacs builds, we attempt to "do the best we can without crashing",
No, assertions should be always true; if an assertion fails Emacs should 
just abort, and should not try to patch around the bug. This is the 
longstanding intent of assertions, both in Emacs and in traditional 
computer science. If X is a side-effect-free expression, then after an 
eassert (X) there is no point to testing whether X is true, because X is 
already known to be true. The code 'eassert (X); if (!X) foo (X);' is 
redundant and somewhat silly, just as the code 'while (X) if (!X) foo 
(X);' is redundant and somewhat silly.

I suppose Emacs could have a different kind of check, for an expression 
that must be true when debugging but that might not be true in 
production. We could call it 'check_when_debugging (X)', say. Its 
semantics would be that Emacs aborts when debugging is enabled and X is 
false; but that X might be either true or false in production code.

However, I don't think check_when_debugging (X) would be helpful. Unlike 
eassert (X), it won't help the reader or simplify later code, because 
later code won't be able to assume that X is true. Worse, it would cause 
Emacs behavior in debugging mode to diverge further from production 
mode, and the production-only code paths would not be easily debuggable. 
The whole thing would be considerably more confusing than what Emacs 
does now.

It could be that the font and image caches are special cases that have a 
lot of bugs, and that we therefore want some sort of belt-and-suspenders 
treatment for them that goes beyond ordinary assertions. I don't see 
offhand why that would be, though. Are we getting an unusual number of 
bugs in that area, and if so what sort of bugs are they?

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 24 Jun 2016 23:34:56 +0200
> 
> On 06/24/2016 03:43 PM, Eli Zaretskii wrote:
> > When ENABLE_CHECKING is not defined, which is what happens by default
> > in Emacs builds, we attempt to "do the best we can without crashing",
> No, 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.

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

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?

> It could be that the font and image caches are special cases that have a 
> lot of bugs, and that we therefore want some sort of belt-and-suspenders 
> treatment for them that goes beyond ordinary assertions. I don't see 
> offhand why that would be, though. Are we getting an unusual number of 
> bugs in that area, and if so what sort of bugs are they?

I don't know how to measure "unusual", so I cannot answer that
question.  I can tell that 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.  I believe the known cases are now solved in the development
sources, but I won't be surprised to hear about similar problems that
still exist in even more rare use cases.

As I wrote above, I'm okay with unconditionally aborting if
FACE_FROM_ID ever yields NULL, and the code after that is written
under the assumption that it cannot happen.  However, the current
FACE_FROM_ID doesn't ensure that, so we should make some follow-up
changes to ensure that.

Also, I don't want two macros (and don't see how this could be
achieved by 2 macros anyway).  I read the related code too frequently
to allow confusing macros into it.

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

OK?

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert]

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.

Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 25 Jun 2016 23:34:52 +0200
> 
> >   . 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.)

OK, thanks.  I took all these arguments under consideration, and made
the corresponding changes in master commit 55d38fc.  Most of it is
just renaming FACE_OPT_FROM_ID to FACE_FROM_ID_OR_NULL (the latter is
IMO more telling about its purpose), but some of the changes change
the macro we call, either because the calling code can cope with a
NULL face pointer, or because it cannot, but the dereference was too
far for GCC 6 to notice it.

I decided not to abort when ENABLE_CHECKING is not defined, because
you convinced me that is unnecessary.