From: Paul Eggert <eggert@cs.ucla.edu>
To: Philipp Stephani <p.stephani2@gmail.com>, Eli Zaretskii <eliz@gnu.org>
Cc: emacs-devel@gnu.org
Subject: Re: JSON/YAML/TOML/etc. parsing performance
Date: Tue, 3 Oct 2017 13:52:54 -0700 [thread overview]
Message-ID: <43520b71-9e25-926c-d744-78098dad6441@cs.ucla.edu> (raw)
In-Reply-To: <CAArVCkSR5w5B4_E0UU8uJtZJtr1oP51md4-i-J+NSRCGpGP_9A@mail.gmail.com>
On 10/03/2017 05:26 AM, Philipp Stephani wrote:
> > +#if __has_attribute (warn_unused_result)
> > +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__
> ((__warn_unused_result__))
> > +#else
> > +# define ATTRIBUTE_WARN_UNUSED_RESULT
> > +#endif
>
> Hmm... why do we need this attribute? You use it with 2 static
> functions, so this sounds like a left-over from the development stage?
>
> It's not strictly needed (and if you don't like it, I can remove it),
> but it helps catching memory leaks.
I've found __warn_unused_result__ to be more trouble than it's worth in
library functions. Emacs has lib/ignore-value.h in order to work around
__warn_unused_result__ brain damage, for example. For static functions
the problem is less, but still, I mildly favor leaving it out.
>
> And btw, how can size be greater than SIZE_MAX in this case? This is
> a valid Lisp object, isn't it? (There are more such tests in the
> patch, e.g. in lisp_to_json, and I think they, too, are redundant.)
>
>
> Depends on the range of ptrdiff_t and size_t. IIUC nothing in the C
> standard guarantees PTRDIFF_MAX <= SIZE_MAX. If we want to guarantee
> that, we should probably add at least a static assertion.
There should be no need for that. No Lisp object can exceed min
(PTRDIFF_MAX, SIZE_MAX) bytes; alloc.c guarantees this, so that Emacs
should work even on oddball platforms where SIZE_MAX < PTRDIFF_MAX, and
there is no need for a runtime check here.
The main practical problem here, by the way, is objects whose sizes
exceed PTRDIFF_MAX on mainstream 32-bit platforms. This Does Not Work
because you cannot subtract pointers within such objects reliably,
sometimes even when the true difference is representable as ptrdiff_t!
This is the main reason alloc.c prohibits creating such large objects,
and that Emacs should reject any attempt to support such objects (even
non-Lisp objects).
> > + if (buffer_and_size->size > PTRDIFF_MAX)
> > + xsignal1 (Qoverflow_error, build_string ("buffer too large"));
> > + insert (buffer_and_size->buffer, buffer_and_size->size);
>
> I don't think we need this test here, as 'insert' already has the
> equivalent test in one of its subroutines.
>
> It can't, because it takes the byte length as ptrdiff_t. We need to
> check before whether the size is actually in the valid range of ptrdiff_t.
A PTRDIFF_MAX test is needed if the JSON library can return strings
longer than PTRDIFF_MAX. Please just use buffer_overflow () to signal
the error, though.
> > + case JSON_INTEGER:
> > + {
> > + json_int_t value = json_integer_value (json);
> > + if (FIXNUM_OVERFLOW_P (value))
> > + xsignal1 (Qoverflow_error,
> > + build_string ("JSON integer is too large"));
> > + return make_number (value);
>
> This overflow test is also redundant, as make_number already does it.
>
> It can't, because json_int_t can be larger than EMACS_INT. Also,
> make_number doesn't contain any checks.
You're right that a test is needed. However, elsewhere we just use
xsignal0 (Qoverflow_error) for this sort of thing, and I suggest being
consistent and doing that here as well. Similarly for other calls to
xsignal1 with Qoverflow_error.
> > + case JSON_STRING:
> > + {
> > + size_t size = json_string_length (json);
> > + if (FIXNUM_OVERFLOW_P (size))
> > + xsignal1 (Qoverflow_error, build_string ("JSON string is
> too long"));
> > + return json_make_string (json_string_value (json), size);
>
> Once again, the overflow test is redundant, as make_specified_string
> (called by json_make_string) already includes an equivalent test.
>
> And once again, we need to check at least whether the size fits into
> ptrdiff_t.
You're right, a test is needed. However, I suggest using string_overflow
() to signal string overflows.
next prev parent reply other threads:[~2017-10-03 20:52 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-16 15:54 JSON/YAML/TOML/etc. parsing performance Ted Zlatanov
2017-09-16 16:02 ` Mark Oteiza
2017-09-17 0:02 ` Richard Stallman
2017-09-17 3:13 ` Mark Oteiza
2017-09-18 0:00 ` Richard Stallman
2017-09-17 0:02 ` Richard Stallman
2017-09-18 13:46 ` Ted Zlatanov
2017-09-17 18:46 ` Philipp Stephani
2017-09-17 19:05 ` Eli Zaretskii
2017-09-17 20:27 ` Philipp Stephani
2017-09-17 22:41 ` Mark Oteiza
2017-09-18 13:53 ` Ted Zlatanov
2017-09-17 21:17 ` Speed of Elisp (was: JSON/YAML/TOML/etc. parsing performance) Stefan Monnier
2017-09-18 13:26 ` JSON/YAML/TOML/etc. parsing performance Philipp Stephani
2017-09-18 13:58 ` Mark Oteiza
2017-09-18 14:14 ` Philipp Stephani
2017-09-18 14:28 ` Mark Oteiza
2017-09-18 14:36 ` Philipp Stephani
2017-09-18 15:02 ` Eli Zaretskii
2017-09-18 16:14 ` Philipp Stephani
2017-09-18 17:33 ` Eli Zaretskii
2017-09-18 19:57 ` Thien-Thi Nguyen
2017-09-18 14:57 ` Eli Zaretskii
2017-09-18 15:07 ` Mark Oteiza
2017-09-18 15:51 ` Eli Zaretskii
2017-09-18 16:22 ` Philipp Stephani
2017-09-18 18:08 ` Eli Zaretskii
2017-09-19 19:32 ` Richard Stallman
2017-09-18 17:26 ` Glenn Morris
2017-09-18 18:16 ` Eli Zaretskii
2017-09-18 16:08 ` Philipp Stephani
2017-09-19 8:18 ` Philipp Stephani
2017-09-19 19:09 ` Eli Zaretskii
2017-09-28 21:19 ` Philipp Stephani
2017-09-28 21:27 ` Stefan Monnier
2017-09-29 19:55 ` Eli Zaretskii
2017-09-30 22:02 ` Philipp Stephani
2017-10-01 18:06 ` Eli Zaretskii
2017-10-03 12:26 ` Philipp Stephani
2017-10-03 15:31 ` Eli Zaretskii
2017-10-03 15:52 ` Philipp Stephani
2017-10-03 16:26 ` Eli Zaretskii
2017-10-03 17:10 ` Eli Zaretskii
2017-10-03 18:37 ` Philipp Stephani
2017-10-03 20:52 ` Paul Eggert [this message]
2017-10-04 5:33 ` Eli Zaretskii
2017-10-04 6:41 ` Paul Eggert
2017-10-04 8:03 ` Eli Zaretskii
2017-10-04 17:51 ` Paul Eggert
2017-10-04 19:38 ` Eli Zaretskii
2017-10-04 21:24 ` Paul Eggert
2017-10-05 1:48 ` Paul Eggert
2017-10-05 7:14 ` Eli Zaretskii
2017-10-08 22:52 ` Philipp Stephani
2017-10-09 5:54 ` Paul Eggert
2017-10-29 20:48 ` Philipp Stephani
2017-10-09 6:38 ` Eli Zaretskii
2017-10-05 7:12 ` Eli Zaretskii
2017-10-06 1:58 ` Paul Eggert
2017-10-06 7:40 ` Eli Zaretskii
2017-10-06 19:36 ` Paul Eggert
2017-10-06 21:03 ` Eli Zaretskii
2017-10-08 23:09 ` Philipp Stephani
2017-10-09 6:19 ` Paul Eggert
2017-10-29 20:48 ` Philipp Stephani
2017-10-29 22:49 ` Philipp Stephani
2017-12-09 23:05 ` Philipp Stephani
2017-12-10 7:08 ` Eli Zaretskii
2017-12-10 13:26 ` Philipp Stephani
2017-12-10 13:32 ` Ted Zlatanov
2017-10-08 23:04 ` Philipp Stephani
2017-10-09 6:47 ` Eli Zaretskii
2017-10-08 17:58 ` Philipp Stephani
2017-10-08 18:42 ` Eli Zaretskii
2017-10-08 23:14 ` Philipp Stephani
2017-10-09 6:53 ` Eli Zaretskii
2017-10-29 20:41 ` Philipp Stephani
2017-10-09 6:22 ` Paul Eggert
2017-10-01 18:38 ` Eli Zaretskii
2017-10-03 12:12 ` Philipp Stephani
2017-10-03 14:54 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=43520b71-9e25-926c-d744-78098dad6441@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=eliz@gnu.org \
--cc=emacs-devel@gnu.org \
--cc=p.stephani2@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).