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 13:17:46 +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="186825"; 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 15:22: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 1hgULn-000mOc-LT for geb-bug-gnu-emacs@m.gmane.org; Thu, 27 Jun 2019 15:22:07 +0200 Original-Received: from localhost ([::1]:50706 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hgULk-0000BV-Rx for geb-bug-gnu-emacs@m.gmane.org; Thu, 27 Jun 2019 09:22:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:32782) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hgUIp-0005Az-TF for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 09:19:05 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hgUIo-0007Qy-0K for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 09:19:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:52352) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hgUIn-0007Qo-Sv for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 09:19:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hgUIn-0000Ec-MB for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 09:19: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 13:19: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.1561641512865 (code B ref 36370); Thu, 27 Jun 2019 13:19:01 +0000 Original-Received: (at 36370) by debbugs.gnu.org; 27 Jun 2019 13:18:32 +0000 Original-Received: from localhost ([127.0.0.1]:37663 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hgUIJ-0000Dt-GH for submit@debbugs.gnu.org; Thu, 27 Jun 2019 09:18:31 -0400 Original-Received: from mail-ot1-f68.google.com ([209.85.210.68]:39689) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hgUIH-0000Df-Go for 36370@debbugs.gnu.org; Thu, 27 Jun 2019 09:18:30 -0400 Original-Received: by mail-ot1-f68.google.com with SMTP id r21so1540119otq.6 for <36370@debbugs.gnu.org>; Thu, 27 Jun 2019 06:18:29 -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=e1qeXxLZxt1paQniviQWt2m+19gST+lcR2AvAUJrPks=; b=VwghwNpQNXXRE/12yrscAoKCyZsg9LfkGDOnr10sLrsle2sz/yj3tPGMXY3YTsoJCn wj3bVRlKhGx3f22yIkD0h1ul1ddLdsuk/iHYiOB9qe1ECihfN5Cdr7rTIrHINtK1TgAF QgQekUIIxX+wunIWQrAZ0nJRROOmVLiLouQZbtG4Dy0YE1D3sFT9vTI6S2slOJyZSG6A U+Y+Jnp8/rawrDAV4NIAscSk40xELTC6zJVfYs0n3blIUnbMlOhtB5CJh4oXzn20OhSo 6sMqZJ4/MGYJAAfp5M131PfzkHVfv1f+aPUioGWHkw4rDY0VumjghKTDvS0nQuq7f8Qg 30YQ== 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=e1qeXxLZxt1paQniviQWt2m+19gST+lcR2AvAUJrPks=; b=FoF1wLnbSQ3dKkzcIxgQ2gXgqMLNDqCLG6hG/Nak3V8rG1fNO43goJj6CMlK01LmN/ pFhR8gWhzVwKxv1njy9brwrMlXdfW8badpEkm2phC+9Gq4TA9xHyTYExu8NV+BIQAkB7 UNceyzVZI+pa1eqjGHo9qtiBQQMJ/sQsoV27d2PxTnP/OXyzzoSRRHPzAonOhbR1W78L NWVhVJP6Q77+ZNRcMtw+6xq7g2k31PVkS6/IjrgNzqCiNk4dT6dK27Uq/q9YyFu/VGoc 60CAOv6yyuJve15uMlI7MUKmqcJ5FtiDCIz6TgeQp3bRo4ypiAM2o5j6fYNvTXCjhwCn Z/4A== X-Gm-Message-State: APjAAAWURrNrKACjBIyTBCwnNj9Af0fz73BLYsqM6fG9JS4FCWViIY/5 ZX4D03tLqmNLEdYu7OWo8W+VEEL2iq3xFGze/6A= X-Google-Smtp-Source: APXvYqwFuG6cR/T51LnlDJ4vDU26ThGykb4tSQB8G0GyplaL3zChvfAsLK7CRMTp6j7QBNK+8FOwHCJQtoGWXcbcq3s= X-Received: by 2002:a9d:664c:: with SMTP id q12mr3059475otm.175.1561641503693; Thu, 27 Jun 2019 06:18:23 -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:161585 Archived-At: On Thu, Jun 27, 2019 at 8:28 AM Paul Eggert wrote: > Pip Cet wrote: > > I'd prefer if the code were written as though FIXNATs weren't FIXNUMs at all. > > I'm more of the opposite opinion. The original reason for XFIXNAT (under its old > name) was performance, because long ago Emacs put the type tag in the most > significant bits, and it was faster to mask it out than to smear the sign bit > when you knew the fixnum was nonnegative. Nowadays that original reason is > obsolete because the tags are in the least significant bits and XFIXNAT is just > an alias for XFIXNUM, and if it were up to me we'd get rid of XFIXNAT entirely, > and just use XFIXNUM. But old habits die hard.... I actually think that would be best, so we're only disagreeing about what the second-best solution is :-) > > 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. > > Sorry, you've lost me. How does valid_image_p guarantee that 'ascent' (if a > fixnum) is in the range 0..INT_MAX? Because if it's not in that range, the above > code could trap. valid_image_p only returns true if ->valid_p returns true, which only happens when parse_image_spec returns true, which only happens if :ascent is not present, Qcenter, or a fixnum in the 0..100 range. > > I'd prefer leaving the macros for now > > I actually did it that way at first, but that failed miserably as the macros > evaluated their arguments more than once when debugging , and lots of code > expects them to behave like functions and evaluate their arguments exactly once. > So functions it is. > > > 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. > > Hmm, I see I should have helped GCC more there. First, the two new eassumes > should just be easserts as the assumptions are not helpful to the compiler. > Second, I should restore the eassume that tells GCC that XFIXNAT returns a > nonnegative integer. > > > (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). > > Sorry, I don't follow. That bug report seems to be about nested C functions; why > is it relevant to Emacs, which doesn't use nested C functions? I don't think it's really relevant to this discussion, though there are similarities: ---- Just like having both XFIXNUM and XFIXNAT is confusing, so is having eassume and eassert. My idea is to define eassume as follows: #define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ? ((cond) ? 0 : __builtin_unreachable ()) : 0) That would catch most (not all) cases in which eassume() causes code to be emitted. I wanted to test this, and in particular to catch false negatives like this eassume in lisp.h: eassume (0 <= i && i < bool_vector_size (a)); (GCC isn't smart enough to derive any information from the RHS of the &&, so __builtin_constant_p (!(cond) == !(cond)) is false, but we only actually use the LHS of the &&...) So I resorted to this old trick to see whether code was being emitted: int c; asm(".pushsection .text.bad"); c = (cond); asm(".popsection"); but that hack no longer works with -g3. Instead, we can define an inner function with the relevant code, put it into a magic section, then confirm that only the trivial function actually ended up in the magic section. ---- > > I don't see how either ccl.c or image.c are buggy for out-of-range > > integer arguments. > > For example, ccl.c in master has this: > > if (FIXNUMP (AREF (status, i))) > { > i = XFIXNAT (AREF (status, 8)); > if (ccl.ic < i && i < ccl.size) > ccl.ic = i; > } > > where i is a 32-bit int. If AREF (status, 8) is the 62-bit Lisp fixnum with > value '2 * (EMACS_INT) INT_MAX + ccl.ic + 3', the assignment to i overflows, > which can trap. Even if the assignment doesn't trap but merely wraps around, the > next line won't do range checking correctly. Ah. Right. Thanks. > > 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. > > But why stop there? Because it's as far as we get only replacing characters in our code, not typing in new ones. > 0 is an obviously invalid argument for buffers. :-) True, but then we'd have to add extra characters to distinguish the buffer and string cases. > I think > this is just an area where we disagree - I want a range check where you want a > type check. It's safe either way and the range check is a tad faster (as the > range check needs to be done anyway) and results in a crisper diagnostic when it > fails. Okay, let's agree to disagree on what the second-best solution is here. > > 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. > > We can't leave it alone, as XFIXNAT is now checking that its result is > nonnegative when debugging is enabled. Do DOS machines have enough memory to run Emacs with debugging enabled? :-) You're absolutely right, of course. Thanks for the changes! I think this code is a bit hard to read: margin = image_spec_value (spec, QCmargin, NULL); if (RANGED_FIXNUMP (0, margin, INT_MAX)) img->vmargin = img->hmargin = XFIXNAT (margin); else if (CONSP (margin)) { img->hmargin = XFIXNAT (XCAR (margin)); img->vmargin = XFIXNAT (XCDR (margin)); } because range is checked for the :margin 3 case but not the :margin '(3 4) case; I'm perfectly okay with taking a list and checking it twice.