GNU bug report logs -
#75845
mapconcat crashes for unreasonable self-modifications
Previous Next
Full log
View this message in rfc822 format
Mattias Engdegård <mattias.engdegard <at> gmail.com> writes:
>>>>> Thanks, adding Stefan and Mattias to the discussion, in the hope that
>>>>> they could review the patch and comment.
>
> Finally I got some time to look at this, and the patch looks fine, largely.
Thanks! I'll probably apply something like it, but see below: eassert
is now officially used in places where Lisp code can trigger it, so
there's little point fixing eassert crashes in one place while new ones
are being deliberately added in others.
> Now that `mapcar1` always returns its first argument (leni), the return value can be eliminated.
Certainly, but this entire code is so strange that I don't really want
to touch it more than necessary, and this particular redundancy seems
potentially useful to me.
(If we really want to optimize it further, let's get rid of the strange
double loop in mapconcat first, which spreads the arguments then goes
back over the list in reverse order to fill in the separators).
> The change in the list loop is cheap and fine.
>
> The new tests and logic in the string loop are more expensive (5-10 %
> overhead, from a quick test) but given that strings are less common in
> calls to mapcar etc, it's probably not so bad. More significantly,
I maintain that using strings as a sequence argument to the map*
functions is so rare we should just turn them into a list or vector
instead and drop the special-casing code.
> these checks will go away entirely when we abolish size-changing
^^^^
I still think that's an "if".
> string mutation -- mentioned elsewhere but there is no need for that
I'm still not convinced abolishing string mutation is a good idea, but
if we do, we should not special-case "size-changing" mutations, because
how many bytes we use to encode each character is an implementation
detail that shouldn't be exposed to Lisp.
Indeed, the code becomes a lot simpler if we make all forms of string
mutation allocate new SDATA blocks while keeping the old ones alive, but
that needs GC changes.
> We should probably amend the documentation for mapcar & co with
> respect to the restrictions on mutating the input sequence. (Common
> Lisp has basically the same restrictions.) At least the manual would
> do well to include a short notice about this; we do mention it for
> `maphash`.
With maphash, this actually happens (it does, or did, in the nativecomp
code) by accident. I'm not sure the same is true for sequences, where
"don't crash" is probably the best we can hope for.
That said, we've since introduced some new code which crashes because
Lisp code misbehaves (608113628c2750b09b925b17c96a29b2dc9abc37, which
easserts that a Lisp function returns an integer in the desired range),
so there appears to be no consensus that Lisp code shouldn't cause
eassert crashes at this point. My idea remains that one should be able
to experiment with Lisp without running into eassert crashes (or
ordinary non-eassert-mediated ones), but if there's no consensus
supporting that, fixing mapcar while leaving readcharfun broken seems
pointless.
> We could just contend ourselves with stating that the input sequence
> shouldn't be mutated and that's it, but if you really want to spell it
I think that would be best. If you need fine-grained control, just
don't use mapcar and open-code it instead.
Pip
This bug report was last modified 210 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.