[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[debbugs-tracker] bug#36370: closed (27.0.50; XFIXNAT called on negative

From: GNU bug Tracking System
Subject: [debbugs-tracker] bug#36370: closed (27.0.50; XFIXNAT called on negative numbers)
Date: Thu, 27 Jun 2019 19:39:02 +0000

Your message dated Thu, 27 Jun 2019 12:38:10 -0700
with message-id <address@hidden>
and subject line Re: bug#36370: 27.0.50; XFIXNAT called on negative numbers
has caused the debbugs.gnu.org bug report #36370,
regarding 27.0.50; XFIXNAT called on negative numbers
to be marked as done.

(If you believe you have received this mail in error, please contact

36370: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=36370
GNU Bug Tracking System
Contact address@hidden with problems
--- 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

(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)

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.

Attachment: 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.

Attachment: 0001-Omit-a-few-minor-unnecessary-range-checks.patch
Description: Text Data

--- End Message ---

reply via email to

[Prev in Thread] Current Thread [Next in Thread]