* FACE_FROM_ID vs FACE_OPT_FROM_ID @ 2016-06-23 20:03 Eli Zaretskii 2016-06-24 0:23 ` Paul Eggert 0 siblings, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2016-06-23 20:03 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-23 20:03 FACE_FROM_ID vs FACE_OPT_FROM_ID Eli Zaretskii @ 2016-06-24 0:23 ` Paul Eggert 2016-06-24 8:49 ` Eli Zaretskii 0 siblings, 1 reply; 12+ messages in thread From: Paul Eggert @ 2016-06-24 0:23 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 0:23 ` Paul Eggert @ 2016-06-24 8:49 ` Eli Zaretskii 2016-06-24 9:27 ` Paul Eggert 0 siblings, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2016-06-24 8:49 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 8:49 ` Eli Zaretskii @ 2016-06-24 9:27 ` Paul Eggert 2016-06-24 9:57 ` Eli Zaretskii 2016-06-24 10:00 ` Eli Zaretskii 0 siblings, 2 replies; 12+ messages in thread From: Paul Eggert @ 2016-06-24 9:27 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel 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). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 9:27 ` Paul Eggert @ 2016-06-24 9:57 ` Eli Zaretskii 2016-06-24 10:00 ` Eli Zaretskii 1 sibling, 0 replies; 12+ messages in thread From: Eli Zaretskii @ 2016-06-24 9:57 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 9:27 ` Paul Eggert 2016-06-24 9:57 ` Eli Zaretskii @ 2016-06-24 10:00 ` Eli Zaretskii 2016-06-24 11:17 ` Paul Eggert 1 sibling, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2016-06-24 10:00 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel [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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 10:00 ` Eli Zaretskii @ 2016-06-24 11:17 ` Paul Eggert 2016-06-24 13:43 ` Eli Zaretskii 0 siblings, 1 reply; 12+ messages in thread From: Paul Eggert @ 2016-06-24 11:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 11:17 ` Paul Eggert @ 2016-06-24 13:43 ` Eli Zaretskii 2016-06-24 21:34 ` Paul Eggert 0 siblings, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2016-06-24 13:43 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 13:43 ` Eli Zaretskii @ 2016-06-24 21:34 ` Paul Eggert 2016-06-25 7:48 ` Eli Zaretskii 0 siblings, 1 reply; 12+ messages in thread From: Paul Eggert @ 2016-06-24 21:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-24 21:34 ` Paul Eggert @ 2016-06-25 7:48 ` Eli Zaretskii 2016-06-25 21:34 ` Paul Eggert 0 siblings, 1 reply; 12+ messages in thread From: Eli Zaretskii @ 2016-06-25 7:48 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-25 7:48 ` Eli Zaretskii @ 2016-06-25 21:34 ` Paul Eggert 2016-07-02 9:50 ` Eli Zaretskii 0 siblings, 1 reply; 12+ messages in thread From: Paul Eggert @ 2016-06-25 21:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FACE_FROM_ID vs FACE_OPT_FROM_ID 2016-06-25 21:34 ` Paul Eggert @ 2016-07-02 9:50 ` Eli Zaretskii 0 siblings, 0 replies; 12+ messages in thread From: Eli Zaretskii @ 2016-07-02 9:50 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-07-02 9:50 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-23 20:03 FACE_FROM_ID vs FACE_OPT_FROM_ID Eli Zaretskii 2016-06-24 0:23 ` Paul Eggert 2016-06-24 8:49 ` Eli Zaretskii 2016-06-24 9:27 ` Paul Eggert 2016-06-24 9:57 ` Eli Zaretskii 2016-06-24 10:00 ` Eli Zaretskii 2016-06-24 11:17 ` Paul Eggert 2016-06-24 13:43 ` Eli Zaretskii 2016-06-24 21:34 ` Paul Eggert 2016-06-25 7:48 ` Eli Zaretskii 2016-06-25 21:34 ` Paul Eggert 2016-07-02 9:50 ` 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.