Re: JSON/YAML/TOML/etc. parsing performance

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4019 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Do., 5. Okt. 2017 um 09:12 Uhr:

> > Cc: p.stephani2@gmail.com, emacs-devel@gnu.org
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Wed, 4 Oct 2017 14:24:59 -0700
> >
> > On 10/04/2017 12:38 PM, Eli Zaretskii wrote:
> > > if we did use size_t for the arguments which can clearly only be
> > > non-negative, the problems which we are discussing would not have
> > > happened
> > Sure, but we would also have worse problems, as size_t is inherently
> > more error-prone. ptrdiff_t overflows are reliably diagnosed when Emacs
> > is compiled with suitable GCC compiler options. size_t overflows cannot
> > be diagnosed, are all too common, and can cause serious trouble.
>
> If ptrdiff_t overflows are reliably diagnosed, then why do we have to
> test for them explicitly in our code, as in the proposed json.c?
> AFAIU, ptrdiff_t overflows are the _only_ reason for json.c checks
> whether a size_t value is too large, because similar checks for
> ptrdiff_t values are already in the low-level subroutines involved in
> creating Lisp objects.  So why couldn't those checks be avoided by
> simply assigning to a ptrdiff_t variables?


Signed integer overflow is only diagnosed when compiling with -ftrapv or
-fsanitize=undefined, which normally doesn't happen in production builds.
IIUC lossy numeric conversions are never diagnosed because they are not
undefined behavior, so we always need to check for them explicitly.
We also need to strictly distinguish between the case where an overflow is
a bug (incorrect assumptions in the Emacs source code itself) and the case
where it is a legitimate outcome due to user input (in this case, return
values from Jansson that overflow ptrdiff_t). The two cases need to be
treated differently: In the first case we can say it's UB and rely on
(hopefully regular and comprehensive) UBSan runs, in the second case we
need explicit checks and normal signalling as opposed to assertions.


>
> > The Emacs internals occasionally use size_t because underlying
> > primitives like 'malloc' do, so we do make some exceptions. Perhaps
> > there should be an exception here, for convenience with the JSON
> > library. The code snippets I've seen so far in this thread are not
> > enough context to judge whether an exception would be helpful in this
> > case. Generally speaking, though, unsigned types should be avoided
> > because they are more error-prone. This has long been the style in Emacs
> > internals, and it's served us well.
>
> I'm not arguing for general replacement of ptrdiff_t with size_t, only
> for doing that in those primitives where negative values are a clear
> mistake/bug.
>
> For example, let's take this case from your proposed changes:
>
>    static Lisp_Object
>   -json_make_string (const char *data, ptrdiff_t size)
>   +json_make_string (const char *data, size_t size)
>    {
>   +  if (PTRDIFF_MAX < size)
>   +    string_overflow ();
>      return make_specified_string (data, -1, size, true);
>    }
>
> If we were to change make_specified_string (and its subroutines, like
> make_uninit_multibyte_string etc.) to accept a size_t value in its 3rd
> argument, the need for the above check against PTRDIFF_MAX would
> disappear.
>

It wouldn't disappear, it would merely be shifted around. Arguments could
be made for either choice, but somewhere the check needs to happen.

  It would also make the higher-level code more
> reliable, because application-level programmers will not need to
> understand all the non-trivial intricacies of this stuff.


Every C programmer needs to understand these issues, no matter what
codebase they are working with. Lossy integral conversions are fundamental
design choices of the C language that can't be avoided.

C is a nasty language full of traps. You can try to hide some of the traps,
but you can't remove them.
(Arguably I've yet to come across a language that doesn't have nasty
integer types. Python 3, with integers auto-upgrading to bignums, might be
closest.)

[-- Attachment #2: Type: text/html, Size: 5117 bytes --]