Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via "Emacs development discussions." <emacs-devel@gnu.org>]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> "Stefan Kangas" <stefankangas@gmail.com> writes:
>>
>>> Pip Cet <pipcet@protonmail.com> writes:
>>>
>>>> What I think we should do doesn't really matter, but it seems quite
>>>> obvious to me that we should make the code on the master branch
>>>> perform all three checks on all relocations, as the code on
>>>> no-purespace does.
>>>
>>> Maybe.  But won't we get those checks with no additional effort once we
>>> merge no-purespace,
>>
>> Yes, we will.  (And the forbidden symbol; even if the forbidden symbol
>> doesn't cause trouble, which I think it will, it's simply very poor
>> programming practice to do things that way, particularly since the
>> crash may happen a long time after the compilation.  But, again, what I
>> think obviously doesn't matter here.  I'll just remember that
>> --enable-checking causes false positive crashes and shouldn't be used).
>
> I don't think the existence of one symbol that will crash Emacs in some
> situations means that --enable-checking should be completely avoided.
> It's still quite useful, and we're fine as long as we avoid using that
> one symbol, right?
>
> OTOH and IMHO, it would be preferable if that symbol could not crash
> Emacs.  Can we come up with a good way to fix that, while preserving the
> check that Andrea wants to keep?

That sounds like a good thing to focus on, yes.  We need to have a value
in a vector that we Fread that is distinguishable from all other values.

One cheap way of doing that is to make the structure circular, and use
references to the object being read as a placeholder value.

So instead of having

"[a b lambda-fixup c]"

in the blob, we'd have

"#1=[a b #1# c]"

Note that that makes element 2 of the vector EQ to the vector being
read, which only comes into existence during reading, so there's no way
to create such a reference in any other way.

We could also make this more explicit by using a more complex expression
involving #1# instead of #1# directly.  For example,

"#1=[a b (unresolved-lambda-fixup #1#) c]"

One side effect is that we need to make sure read-circle is t before
reading the string (which is, IIUC, already produced with print-circle
bound to t, so reading it with read-circle seems like the correct thing
to do).

For example, right now this code doesn't work:

(let ((print-circle t) (read-circle nil))
  (message "%S" (funcall (native-compile (lambda () #1=[#1#])))))

(read, of course, with read-circle bound to t)

but this does:

(let ((print-circle t) (read-circle nil))
  (message "%S" (funcall (lambda () #1=[#1#]))))

So this seems like a cheap drive-by fix.

A more complicated approach would be to add a special read syntax (if
there isn't one that I missed) producing references to objects defined
outside of the read string: in essence, we'd call Fread in a special way
that makes #s(magic-cookie x) evaluate to be eq to something passed in
as a "magic cookie".  This might be generally useful and avoids the
problems of circular data structures.

But I'll try to come up with a patch doing the simple #1# thing first.

>> That's a problem, because if we run into problems there, we'll have no
>> way of knowing whether the problem was present on the pre-merge master
>> (where we didn't check) or not.  But, again, as it's specific to
>> --enable-checking, we can simply stop using that.
>>
>>> and if so, can't it wait until then?
>>
>> Of course, but changing two things at a time makes debugging harder.
>> (And IIUC, we won't rename the symbol on master until we merge, so
>> that's three changes which can cause trouble with the nativecomp code,
>> all introduced at the same time).
>
> I still don't think I understand your argument here, sorry.
>
> The scratch/no-purespace branch contains several different changes, all
> of which have had to pass through review, testing and verification.

> Why is it particularly important to "backport" this change to master, in
> advance of the merge, but not any of the other changes on that branch?
> What am I missing?

On reflection, it was I who was missing something, that no-purespace
already has moved from "the minimum set of changes necessary for getting
rid of purespace and unexec" to "a coherent set of changes including
purespace and unexec removal and quite a few other things".  I think
that's a good change to have happened, it just took me a while to catch
up with it.

So no continued objections there.

Pip