From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: FACE_FROM_ID vs FACE_OPT_FROM_ID Date: Fri, 24 Jun 2016 11:49:51 +0300 Message-ID: <8360sz2cpc.fsf@gnu.org> References: <83d1n73c5z.fsf@gnu.org> <576C7D75.4070401@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1466758276 29701 80.91.229.3 (24 Jun 2016 08:51:16 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 24 Jun 2016 08:51:16 +0000 (UTC) Cc: emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jun 24 10:51:16 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1bGMpV-0001dQ-OC for ged-emacs-devel@m.gmane.org; Fri, 24 Jun 2016 10:51:13 +0200 Original-Received: from localhost ([::1]:42060 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGMpV-000443-2A for ged-emacs-devel@m.gmane.org; Fri, 24 Jun 2016 04:51:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:45648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGMoz-00043g-BT for emacs-devel@gnu.org; Fri, 24 Jun 2016 04:50:42 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGMou-0003rq-UE for emacs-devel@gnu.org; Fri, 24 Jun 2016 04:50:40 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:55160) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGMou-0003rm-NH; Fri, 24 Jun 2016 04:50:36 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:3826 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bGMot-0001yz-TM; Fri, 24 Jun 2016 04:50:36 -0400 In-reply-to: <576C7D75.4070401@cs.ucla.edu> (message from Paul Eggert on Fri, 24 Jun 2016 02:23:17 +0200) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:204720 Archived-At: > Cc: emacs-devel@gnu.org > From: Paul Eggert > 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?