From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.bugs Subject: bug#36370: 27.0.50; XFIXNAT called on negative numbers Date: Thu, 27 Jun 2019 01:28:30 -0700 Organization: UCLA Computer Science Department Message-ID: References: Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------3410BBC0447F81BE707959FE" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="146522"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.1 Cc: 36370@debbugs.gnu.org To: Pip Cet Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Jun 27 10:29:13 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 1hgPmK-000bwq-D1 for geb-bug-gnu-emacs@m.gmane.org; Thu, 27 Jun 2019 10:29:12 +0200 Original-Received: from localhost ([::1]:47568 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hgPmJ-000477-Eb for geb-bug-gnu-emacs@m.gmane.org; Thu, 27 Jun 2019 04:29:11 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:51205) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hgPmC-00046w-FN for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 04:29:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hgPmA-0007FB-Ha for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 04:29:04 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:52065) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hgPm9-0007Eg-Vr for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 04:29:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hgPm9-0003Mr-OQ for bug-gnu-emacs@gnu.org; Thu, 27 Jun 2019 04:29:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Paul Eggert Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 27 Jun 2019 08:29: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.156162412312920 (code B ref 36370); Thu, 27 Jun 2019 08:29:01 +0000 Original-Received: (at 36370) by debbugs.gnu.org; 27 Jun 2019 08:28:43 +0000 Original-Received: from localhost ([127.0.0.1]:37376 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hgPlq-0003MK-Dw for submit@debbugs.gnu.org; Thu, 27 Jun 2019 04:28:42 -0400 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:59232) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hgPlm-0003M4-QG for 36370@debbugs.gnu.org; Thu, 27 Jun 2019 04:28:40 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id C31EA161A49; Thu, 27 Jun 2019 01:28:32 -0700 (PDT) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id PdmYkMFWsicR; Thu, 27 Jun 2019 01:28:31 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 33DDF161C38; Thu, 27 Jun 2019 01:28:31 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id sqrgOfTtkGaA; Thu, 27 Jun 2019 01:28:31 -0700 (PDT) Original-Received: from [192.168.1.9] (cpe-23-242-74-103.socal.res.rr.com [23.242.74.103]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 0A0F1161A49; Thu, 27 Jun 2019 01:28:31 -0700 (PDT) In-Reply-To: Content-Language: en-US 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:161564 Archived-At: This is a multi-part message in MIME format. --------------3410BBC0447F81BE707959FE Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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.... > 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. > I'd prefer AREF (status, 8) here, for what readability the code gives us. Sure, sounds good. See attached further patch. > 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? >> * 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. No, as the range-checking is done after the XFIXNAT, which means the XFIXNAT could have an assertion failure. >> 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. 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. > 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? 0 is an obviously invalid argument for buffers. :-) 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. > 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. However, we could instead change the XFIXNATs to XFIXNUMs: this will generate exactly the same machine code on that platform when debugging is disabled, and will not trap when debugging is enabled. See attached patch. --------------3410BBC0447F81BE707959FE Content-Type: text/plain; charset=UTF-8; name="0001-Improve-XFIXNUM-cleanup-a-bit.txt" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="0001-Improve-XFIXNUM-cleanup-a-bit.txt" RnJvbSAzZjgwOTBlOTExMjVkZTk4Nzk4ZDRiZmI1ZjkxOTM0YmNhYzk2MjZiIE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQYXVsIEVnZ2VydCA8ZWdnZXJ0QGNzLnVjbGEuZWR1 PgpEYXRlOiBUaHUsIDI3IEp1biAyMDE5IDAxOjE0OjUzIC0wNzAwClN1YmplY3Q6IFtQQVRD SF0gSW1wcm92ZSBYRklYTlVNIGNsZWFudXAgYSBiaXQKTUlNRS1WZXJzaW9uOiAxLjAKQ29u dGVudC1UeXBlOiB0ZXh0L3BsYWluOyBjaGFyc2V0PVVURi04CkNvbnRlbnQtVHJhbnNmZXIt RW5jb2Rpbmc6IDhiaXQKCkJhc2VkIG9uIFBpcCBDZXTigJlzIHJldmlldyAoQnVnIzM2Mzcw IzEzKS4KKiBzcmMvY2NsLmMgKEZjY2xfZXhlY3V0ZV9vbl9zdHJpbmcpOiBVc2UgY2xlYXJl ciBpbmRleGluZy4KKiBzcmMvZG9zZm5zLmMgKEZpbnQ4NiwgRmRvc19tZW1wdXQpOgpBdm9p ZCBydW50aW1lIGNoZWNrcyBmb3IgbmVnYXRpdmUgZml4bnVtcyB3aGVuIGRlYnVnZ2luZy4K VGhpcyByZXN0b3JlcyB0aGUgZWFybGllciBtYWNoaW5lIGNvZGUuCiogc3JjL2xpc3AuaCAo WEZJWE5VTSwgWFVGSVhOVU0pOiBVc2UgZWFzc2VydCwgbm90IGVhc3N1bWUuCihYRklYTkFU KTogQXQgdGhlIHN0YXJ0LCBtZXJlbHkgZWFzc2VydCBGSVhOVU1QIHJhdGhlcgp0aGFuIGVh c3N1bWluZyBGSVhOQVRQLiAgQXQgdGhlIGVuZCwgZWFzc3VtZSB0aGF0IHRoZQpyZXN1bHQg aXMgbm9ubmVnYXRpdmUuICBUaGlzIHJlc3RvcmVzIGhlbHAgdG8gdGhlIGNvbXBpbGVyCnRo YXQgdGhlIHByZXZpb3VzIHBhdGNoIG1pc3Rha2VubHkgcmVtb3ZlZC4KLS0tCiBzcmMvY2Ns LmMgICAgfCAgNCArKy0tCiBzcmMvZG9zZm5zLmMgfCAyMiArKysrKysrKysrKy0tLS0tLS0t LS0tCiBzcmMvbGlzcC5oICAgfCAxMCArKysrKystLS0tCiAzIGZpbGVzIGNoYW5nZWQsIDE5 IGluc2VydGlvbnMoKyksIDE3IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9jY2wu YyBiL3NyYy9jY2wuYwppbmRleCBmMWQ0YzI4ZGYxLi5mZjQyYzZmMjVmIDEwMDY0NAotLS0g YS9zcmMvY2NsLmMKKysrIGIvc3JjL2NjbC5jCkBAIC0yMDYyLDkgKzIwNjIsOSBAQCAjZGVm aW5lIENDTF9FWEVDVVRFX0JVRl9TSVpFIDEwMjQKICAgICAgIGlmIChUWVBFX1JBTkdFRF9G SVhOVU1QIChpbnQsIEFSRUYgKHN0YXR1cywgaSkpKQogCWNjbC5yZWdbaV0gPSBYRklYTlVN IChBUkVGIChzdGF0dXMsIGkpKTsKICAgICB9Ci0gIGlmIChGSVhOVU1QIChBUkVGIChzdGF0 dXMsIGkpKSkKKyAgaWYgKEZJWE5VTVAgKEFSRUYgKHN0YXR1cywgOCkpKQogICAgIHsKLSAg ICAgIEVNQUNTX0lOVCBpYyA9IFhGSVhOVU0gKEFSRUYgKHN0YXR1cywgaSkpOworICAgICAg RU1BQ1NfSU5UIGljID0gWEZJWE5VTSAoQVJFRiAoc3RhdHVzLCA4KSk7CiAgICAgICBpZiAo Y2NsLmljIDwgaWMgJiYgaWMgPCBjY2wuc2l6ZSkKIAljY2wuaWMgPSBpYzsKICAgICB9CmRp ZmYgLS1naXQgYS9zcmMvZG9zZm5zLmMgYi9zcmMvZG9zZm5zLmMKaW5kZXggZmI1YmNjOWFk My4uNjM1ZjI5YmQ2NSAxMDA2NDQKLS0tIGEvc3JjL2Rvc2Zucy5jCisrKyBiL3NyYy9kb3Nm bnMuYwpAQCAtNzIsMTYgKzcyLDE2IEBAIERFRlVOICgiaW50ODYiLCBGaW50ODYsIFNpbnQ4 NiwgMiwgMiwgMCwKICAgaWYgKG5vIDwgMCB8fCBubyA+IDB4ZmYgfHwgQVNJWkUgKHJlZ2lz dGVycykgIT0gOCkKICAgICByZXR1cm4gUW5pbDsKICAgZm9yIChpID0gMDsgaSA8IDg7IGkr KykKLSAgICBDSEVDS19GSVhOQVQgKEFSRUYgKHJlZ2lzdGVycywgaSkpOworICAgIENIRUNL X0ZJWE5VTSAoQVJFRiAocmVnaXN0ZXJzLCBpKSk7CiAKLSAgaW5yZWdzLnguYXggICAgPSAo dW5zaWduZWQgbG9uZykgWEZJWE5BVCAoQVJFRiAocmVnaXN0ZXJzLCAwKSk7Ci0gIGlucmVn cy54LmJ4ICAgID0gKHVuc2lnbmVkIGxvbmcpIFhGSVhOQVQgKEFSRUYgKHJlZ2lzdGVycywg MSkpOwotICBpbnJlZ3MueC5jeCAgICA9ICh1bnNpZ25lZCBsb25nKSBYRklYTkFUIChBUkVG IChyZWdpc3RlcnMsIDIpKTsKLSAgaW5yZWdzLnguZHggICAgPSAodW5zaWduZWQgbG9uZykg WEZJWE5BVCAoQVJFRiAocmVnaXN0ZXJzLCAzKSk7Ci0gIGlucmVncy54LnNpICAgID0gKHVu c2lnbmVkIGxvbmcpIFhGSVhOQVQgKEFSRUYgKHJlZ2lzdGVycywgNCkpOwotICBpbnJlZ3Mu eC5kaSAgICA9ICh1bnNpZ25lZCBsb25nKSBYRklYTkFUIChBUkVGIChyZWdpc3RlcnMsIDUp KTsKLSAgaW5yZWdzLnguY2ZsYWcgPSAodW5zaWduZWQgbG9uZykgWEZJWE5BVCAoQVJFRiAo cmVnaXN0ZXJzLCA2KSk7Ci0gIGlucmVncy54LmZsYWdzID0gKHVuc2lnbmVkIGxvbmcpIFhG SVhOQVQgKEFSRUYgKHJlZ2lzdGVycywgNykpOworICBpbnJlZ3MueC5heCAgICA9ICh1bnNp Z25lZCBsb25nKSBYRklYTlVNIChBUkVGIChyZWdpc3RlcnMsIDApKTsKKyAgaW5yZWdzLngu YnggICAgPSAodW5zaWduZWQgbG9uZykgWEZJWE5VTSAoQVJFRiAocmVnaXN0ZXJzLCAxKSk7 CisgIGlucmVncy54LmN4ICAgID0gKHVuc2lnbmVkIGxvbmcpIFhGSVhOVU0gKEFSRUYgKHJl Z2lzdGVycywgMikpOworICBpbnJlZ3MueC5keCAgICA9ICh1bnNpZ25lZCBsb25nKSBYRklY TlVNIChBUkVGIChyZWdpc3RlcnMsIDMpKTsKKyAgaW5yZWdzLnguc2kgICAgPSAodW5zaWdu ZWQgbG9uZykgWEZJWE5VTSAoQVJFRiAocmVnaXN0ZXJzLCA0KSk7CisgIGlucmVncy54LmRp ICAgID0gKHVuc2lnbmVkIGxvbmcpIFhGSVhOVU0gKEFSRUYgKHJlZ2lzdGVycywgNSkpOwor ICBpbnJlZ3MueC5jZmxhZyA9ICh1bnNpZ25lZCBsb25nKSBYRklYTlVNIChBUkVGIChyZWdp c3RlcnMsIDYpKTsKKyAgaW5yZWdzLnguZmxhZ3MgPSAodW5zaWduZWQgbG9uZykgWEZJWE5V TSAoQVJFRiAocmVnaXN0ZXJzLCA3KSk7CiAKICAgaW50ODYgKG5vLCAmaW5yZWdzLCAmb3V0 cmVncyk7CiAKQEAgLTEzOSw4ICsxMzksOCBAQCBERUZVTiAoIm1zZG9zLW1lbXB1dCIsIEZk b3NfbWVtcHV0LCBTZG9zX21lbXB1dCwgMiwgMiwgMCwKIAogICBmb3IgKGkgPSAwOyBpIDwg bGVuOyBpKyspCiAgICAgewotICAgICAgQ0hFQ0tfRklYTkFUIChBUkVGICh2ZWN0b3IsIGkp KTsKLSAgICAgIGJ1ZltpXSA9ICh1bnNpZ25lZCBjaGFyKSBYRklYTkFUIChBUkVGICh2ZWN0 b3IsIGkpKSAmIDB4RkY7CisgICAgICBDSEVDS19GSVhOVU0gKEFSRUYgKHZlY3RvciwgaSkp OworICAgICAgYnVmW2ldID0gKHVuc2lnbmVkIGNoYXIpIFhGSVhOVU0gKEFSRUYgKHZlY3Rv ciwgaSkpICYgMHhGRjsKICAgICB9CiAKICAgZG9zbWVtcHV0IChidWYsIGxlbiwgb2Zmcyk7 CmRpZmYgLS1naXQgYS9zcmMvbGlzcC5oIGIvc3JjL2xpc3AuaAppbmRleCAwNzdkMjM2MDY1 Li5hMDYxOWU2NGYyIDEwMDY0NAotLS0gYS9zcmMvbGlzcC5oCisrKyBiL3NyYy9saXNwLmgK QEAgLTExOTUsNyArMTE5NSw3IEBAIFhGSVhOVU1fUkFXIChMaXNwX09iamVjdCBhKQogSU5M SU5FIEVNQUNTX0lOVAogWEZJWE5VTSAoTGlzcF9PYmplY3QgYSkKIHsKLSAgZWFzc3VtZSAo RklYTlVNUCAoYSkpOworICBlYXNzZXJ0IChGSVhOVU1QIChhKSk7CiAgIHJldHVybiBYRklY TlVNX1JBVyAoYSk7CiB9CiAKQEAgLTEyMDksNyArMTIwOSw3IEBAIFhVRklYTlVNX1JBVyAo TGlzcF9PYmplY3QgYSkKIElOTElORSBFTUFDU19VSU5UCiBYVUZJWE5VTSAoTGlzcF9PYmpl Y3QgYSkKIHsKLSAgZWFzc3VtZSAoRklYTlVNUCAoYSkpOworICBlYXNzZXJ0IChGSVhOVU1Q IChhKSk7CiAgIHJldHVybiBYVUZJWE5VTV9SQVcgKGEpOwogfQogCkBAIC0yODI4LDkgKzI4 MjgsMTEgQEAgRklYTkFUUCAoTGlzcF9PYmplY3QgeCkKIElOTElORSBFTUFDU19JTlQKIFhG SVhOQVQgKExpc3BfT2JqZWN0IGEpCiB7Ci0gIGVhc3N1bWUgKEZJWE5BVFAgKGEpKTsKKyAg ZWFzc2VydCAoRklYTlVNUCAoYSkpOwogICBFTUFDU19JTlQgaW50MCA9IExpc3BfSW50MDsK LSAgcmV0dXJuIFVTRV9MU0JfVEFHID8gWEZJWE5VTSAoYSkgOiBYTEkgKGEpIC0gKGludDAg PDwgVkFMQklUUyk7CisgIEVNQUNTX0lOVCByZXN1bHQgPSBVU0VfTFNCX1RBRyA/IFhGSVhO VU0gKGEpIDogWExJIChhKSAtIChpbnQwIDw8IFZBTEJJVFMpOworICBlYXNzdW1lICgwIDw9 IHJlc3VsdCk7CisgIHJldHVybiByZXN1bHQ7CiB9CiAKIElOTElORSBib29sCi0tIAoyLjIx LjAKCg== --------------3410BBC0447F81BE707959FE--