From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Pip Cet Newsgroups: gmane.emacs.bugs Subject: bug#36370: 27.0.50; XFIXNAT called on negative numbers Date: Thu, 27 Jun 2019 06:16:25 +0000 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="102746"; mail-complaints-to="usenet@blaine.gmane.org" Cc: 36370@debbugs.gnu.org To: Paul Eggert Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Jun 27 08:18:10 2019 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1hgNjV-000QdT-W4 for geb-bug-gnu-emacs@m.gmane.org; Thu, 27 Jun 2019 08:18:10 +0200 Original-Received: from localhost ([::1]:46678 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hgNjU-0001KA-UT for geb-bug-gnu-emacs@m.gmane.org; Thu, 27 Jun 2019 02:18:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:39697) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hgNjP-0001Js-L6 for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 02:18:04 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hgNjO-0003az-4m for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 02:18:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:51998) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hgNjO-0003ar-1C for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 02:18:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hgNjN-00007J-SD for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 02:18:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Pip Cet Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 27 Jun 2019 06:18:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 36370 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 36370-submit@debbugs.gnu.org id=B36370.1561616232390 (code B ref 36370); Thu, 27 Jun 2019 06:18:01 +0000 Original-Received: (at 36370) by debbugs.gnu.org; 27 Jun 2019 06:17:12 +0000 Original-Received: from localhost ([127.0.0.1]:37309 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hgNiZ-00006D-OO for submit@debbugs.gnu.org; Thu, 27 Jun 2019 02:17:12 -0400 Original-Received: from mail-ot1-f45.google.com ([209.85.210.45]:46475) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hgNiW-00005n-5K for 36370@debbugs.gnu.org; Thu, 27 Jun 2019 02:17:08 -0400 Original-Received: by mail-ot1-f45.google.com with SMTP id z23so1066087ote.13 for <36370@debbugs.gnu.org>; Wed, 26 Jun 2019 23:17:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4b8IY4y2RuYfORbRCXkWUJJExOrxIXV+Px+OouuCh88=; b=lGf9mMS8Q5MR+0fseDC/PmvD7fQTxnOadxTO8bntodZP3lNYW5NwsrMswW1DS9l61m uzde4IVGbe/ZOOe0zhTWp+N6faIN2oNaJJTNK7YG9VPfGfMjSpzu21/eYCv2OOs8vaqw frGdPCnMV9wUSPAJoa1GYOy0zoPUVq7Rd0lDkZ7fzPGrGBFtKj/pJOM4Yxo/zTvWAv2w 7AfWLfw8XOTuZFFPsEYxKbregp7Q18irsfYjeXEOKwQvYXdU+I3wz34CnayBWVM85jrj yjvvfDtE95s7BBAEyTfC2ehWKbkv8gfpddDOw1+5uZxxxibiu80LNihkR4jJUT4Q443X pG3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4b8IY4y2RuYfORbRCXkWUJJExOrxIXV+Px+OouuCh88=; b=PIb6T5IVGesVrhPo1uCQltmieMMnAKh6lu76VRKQhO6LKa29yAM4MxbXDLM0GpP4Ww VtKZxcdEgZRc26hyagGDuisdn4KM+CsYqkCj0F7aN4iKrQIC/NakkZXlYdG5tfvxJCTK jbHSOiPVn8XC17wnrzmT3rMSZOKIjZGkrfNuLMftOvFRMpd211P0D9WEQLztJLwgEDSd liti6iHMlOQOhPRzBXyVFdsGRgEAXfSxoGTHPQ0MSqd3mYGX70mpAGWEfAIbHhR+8us0 poWJbeDYHm8+SanBLbLP4mnK0rzL0b5+D/5aUhayEkuhysZpiyPuR+hPKOf57RCW+PIS l1IA== X-Gm-Message-State: APjAAAU2lqPakSFt490NczAq/juDdp4Q7H8s7yNnjNFi2DLaNK1o3IRB /MEcq7KSZ+1NxhVRXAvXmbdIYLh8EshXTH7rwo4= X-Google-Smtp-Source: APXvYqwWgZF6MPutwjVcC70wSYytVYSZX5Sa2QfGtNaM3joJUkTS4aaZWU3582oHxwHknB9W6ZE+iM92nHB83UgaKWY= X-Received: by 2002:a9d:664c:: with SMTP id q12mr1802698otm.175.1561616222402; Wed, 26 Jun 2019 23:17:02 -0700 (PDT) In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.51.188.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:161560 Archived-At: On Thu, Jun 27, 2019 at 1:11 AM Paul Eggert wrote: > Thanks for looking into this. Cleaning this up has been on my to-do list > for a while, and something along these lines has been discussed on the > mailing list. Some comments on your fixes: I really think I should reiterate that readability is much much more important than whatever performance tweaks XFIXNAT might allow. I'd prefer if the code were written as though FIXNATs weren't FIXNUMs at all: pair FIXNATP with XFIXNAT, and provide FIXNAT versions of CHECK_RANGED_FIXNUM and CHECK_FIXNUM_COERCE_MARKER. Hmm. I'm not sure it's a problem for you, but I'm finding it rather hard to check in all cases whether XFIXNAT is safe. For example, ascent = image_spec_value (spec, QCascent, NULL); if (FIXNUMP (ascent)) img->ascent = XFIXNAT (ascent); in image.c is safe, because lookup_image is called only after valid_image_p. if (FIXNUMP (AREF (status, i))) { - i = XFIXNAT (AREF (status, 8)); - if (ccl.ic < i && i < ccl.size) - ccl.ic = i; + EMACS_INT ic = XFIXNUM (AREF (status, i)); + if (ccl.ic < ic && ic < ccl.size) + ccl.ic = ic; } I'd prefer AREF (status, 8) here, for what readability the code gives us. # define XHASH(a) lisp_h_XHASH (a) # if USE_LSB_TAG # define make_fixnum(n) lisp_h_make_fixnum (n) -# define XFIXNAT(a) lisp_h_XFIXNAT (a) -# define XFIXNUM(a) lisp_h_XFIXNUM (a) +# define XFIXNUM_RAW(a) lisp_h_XFIXNUM_RAW (a) # define XTYPE(a) lisp_h_XTYPE (a) # endif #endif I'd prefer leaving the macros for now; eassume (inline_function (x)) doesn't work very well on current GCC, while eassume (MACRO (x)) is fine, so I'm sometimes building with DEFINE_KEY_OPS_AS_MACROS. (The eassume/eassert situation is something else I've been planning to look at in more detail, though debugging it will be easier when https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91005 is fixed). > * The ccl.c, fileio.c, image.c, and process.c fixes should use XFIXNUM, > not XFIXNAT, since the code is range-checking after extraction anyway > and the latter range-checking should do the right thing. Come to think That's an argument that either XFIXNUM or XFIXNAT are fine, and in this case XFIXNAT seems, to me, much more readable. > of it, the ccl.c and image.c code is buggy regardless of the > XFIXNUM/XFIXNAT business since the Emacs integer might not fit into int > range, and that should be fixed too. I don't see how either ccl.c or image.c are buggy for out-of-range integer arguments. > * Instead of adding a new macro CHECK_FIXNAT_COERCE_MARKER, the code > should stick with CHECK_FIXNUM_COERCE_MARKER and do the usual > range-checking on the results. This should yield more-precise > diagnostics. Most of the range-checking is there already. I disagree, obviously. When you want a position, you want a FIXNAT, not a negative number, so CHECK_FIXNAT_COERCE_MARKER everywhere would catch those cases where someone passes an obviously invalid argument. > * The set_text_properties fix can be done more efficiently via EQ. I suspect your change results in a few more insns being emitted, but I'll have to check. > * I'm mildly inclined to think that w32term.c and xterm.c should treate > negative minimum values as 0 when the actual value must be nonnegative. > That is, a negative minimum should act like a minimum of 0, not as a > minimum of 1 as in your proposed patch. Doesn't matter, so I decided not to fiddle with this code. > To help prevent similar problems in the future, XFIXNUM, XFIXNAT etc. > should check that their arguments are of the proper type when > ENABLE_CHECKING is nonzero. > > A proposed patch is attached; it should do all the above. I'd prefer not touching the dosfns.c code, for the simple reason that if anyone still uses it, they might rely on the broken behavior. > PS. At some point we should also check that make_fixnum is invoked only > on values in fixnum range, but that's a matter for a later patch. Definitely.