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

[Andrea Corallo]

Pip Cet <pipcet@protonmail.com> writes:

> "Andrea Corallo" <acorallo@gnu.org> writes:
>
>> Pip Cet <pipcet@protonmail.com> writes:
>>
>>> Pip Cet <pipcet@protonmail.com> writes:
>>>> IOW, the old code happened not to run into this problem because
>>>> lambda-fixup was pure, and we never applied the sanity checks to the
>>>> pure section.
>>>
>>> Just to be clear: the code on master is fine.  I misunderstood it when
>>> modifying it for purespace removal, resulting in my bug which Gerd
>>> discovered and fixed.  The code on no-purespace is also fine now, but
>>> it's Andrea's call whether he wants some of the checking code restored,
>>> and how.
>>>
>>>> My suggestion is to fix the "sanity check" on the master branch, change
>>>> it to apply to pure relocs there, and restore the fixed check on
>>>> scratch/no-purespace afterwards.
>>>
>>> Please ignore that. My suggestion is to EXTEND the sanity check on the
>>> master branch to cover pure and impure relocs, and restore the EXTENDED
>>> check before merging scratch/no-purespace.
>>>
>>> There is no bug to fix on master.
>>
>> Right your analysis is correct, the new code in the branch just made the
>> symbol 'fixup-lambda' not compilable.
>>
>> I restored the check and applied a variant of your fix with a comment
>> around.  scratch/no-purespace work for me now.
>
> Just to summarize this:
> There's now a forbidden symbol, --lambda-fixup.  If you use this symbol
> in your code and compile the code with nativecomp, that may appear to work,
> but loading the resulting object file into another Emacs will crash that
> Emacs, if that Emacs was built with checks enabled.

Correct, maybe we should use 'comp--lambda-fixup' so it's even more
evidently private to the compiler, not sure if would be worth using a
different kind of placeholder object.

>> On master I don't think I see what we should do and the motivation for.
>
> The scratch/no-purespace branch now tests things more rigorously than
> the master branch does: master performs three checks on all impure
> relocations and a single check on pure ones, but scratch/no-purespace
> performs all three checks on all relocations.
>
> That means when we merge scratch/no-purespace, and hit one of the new
> assertions, it may be (and was, in Gerd's case) because the test would
> have failed on the master branch but was never performed there, and that
> would be unrelated to purespace removal.
>
> 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.

master doesn't have forbidden symbols that can't be stored in data_vec
so why should we test for that?