Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii <eliz@gnu.org>]

> 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?