--- Begin Message ---
|
Subject: |
27.0.50; XFIXNAT called on negative numbers |
|
Date: |
Tue, 25 Jun 2019 05:36:15 +0000 |
I just spent some time reviewing the use of XFIXNAT, after being
bitten by this code in fileio.c:
if (FIXNUMP (tem))
nextpos = XFIXNAT (tem);
In the following cases, negative numbers passed to APIs which aren't
strictly internal or low-level cause us to call XFIXNAT on a negative
number:
(indent-to -100 -100)
(set-text-properties -1 -1 nil "")
(make-network-process :name "name" :family 'ipv6 :remote [-1 0 0 0 0 0 0 0 0])
(end-kbd-macro -1 nil)
(previous-single-property-change (point) 'dummy nil -1)
I think those six cases are worth fixing, but more generally, I think
it would help avoid such errors if XFIXNAT were "always" paired with
FIXNATP, XFIXNUM with FIXNUMP, and XCHARACTER (which would be new)
with CHARACTERP.
So I'm going to be brave here and admit that in many cases, while
XFIXNUM was correct, it took me a long time to see that because the
check was performed somewhere else.
I understand there is a very slight performance hit on some platforms,
but is that really still an issue? Also, consistent pairing would
catch a few cases in which we call XFIXNUM but could get away with an
XFIXNAT, such as this code in self-insert-command:
if (!CHARACTERP (c))
bitch_at_user ();
else {
int character = translate_char (Vtranslation_table_for_input,
XFIXNUM (c));
I also think it would make sense to eassume (FIXNUMP (a)) in both
XFIXNAT and XFIXNUM (but not XHASH, or XUFIXNUM if that's what XHASH
uses). It might catch bugs like
(set-text-properties nil nil nil "")
currently not throwing an error.
The patch I'm attaching fixes only things I'm reasonably sure are bugs.
xfixnat.diff
Description: Text Data
--- End Message ---
--- Begin Message ---
|
Subject: |
Re: bug#36370: 27.0.50; XFIXNAT called on negative numbers |
|
Date: |
Thu, 27 Jun 2019 12:38:10 -0700 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
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 :-)
Removing XFIXNAT would be outside the scope of this patch. However, if
we're already fixing the code for some other reason and if the XFIXNATs
are confusing that code, we might as well replace them with XFIXNUMs.
The attached patch does that.
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.
Thanks for explaining. The attached patch removes the unnecessary range
checks that I proposed.
My idea is to define eassume as follows:
#define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
((cond) ? 0 : __builtin_unreachable ()) : 0)
That would generate worse code in some cases, since after (say) "eassume
(i >= 0); return i/2;" where i is a variable, GCC would not be able to
optimize i/2 into i>>1 because GCC would not know that i is nonnegative.
The main point of eassume (as opposed to eassert) is to enable
optimizations like that.
think this code is a bit hard to read:
The attached patch changes that to use XFIXNUM instead of XFIXNAT, along
the lines discussed above.
Thanks for the review. In addition to the already-mentioned patches, I
installed the attached and am closing the bug report.
0001-Omit-a-few-minor-unnecessary-range-checks.patch
Description: Text Data
--- End Message ---