Store references in SBCL-compiled code are "invisible"

Done

Details

6 participants

Danny Milosavljevic
Guillaume Le Vaillant
Leo Famulari
Ludovic Courtès
Pierre Neidhardt
Mark H Weaver

Owner: unassigned

Submitted by: Ludovic Courtès

Severity: important

Debbugs page

Ludovic Courtès wrote on 23 Dec 2018 06:19

Recipients:(name . Bug Guix)(address . bug-guix@gnu.org)

Message-ID:87r2e8jpfx.fsf@gnu.org

Hello,

As discussed with Pierre at the R-B Summit, ‘sbcl-next’ lacks a

reference to ‘next-gtk-webkit’ even though is invokes it:

Toggle snippet (13 lines)

$ guix gc --references $(type -P next) | grep next-

/gnu/store/9d66xb8wvggsp0x9pxj61mzqy007978f-sbcl-next-1.1.0

/gnu/store/pqy064fw3vkfld6lw95vi0zavj19zvrc-sbcl-next-1.1.0-lib

$ ./pre-inst-env guix run next

WARNING: Setting locale failed.

Check the following variables for correct values:

LANG=en_US.utf8

Unhandled SIMPLE-ERROR in thread #<SB-THREAD:THREAD "main thread" RUNNING

{10005885B3}>:

Couldn't execute "/gnu/store/7p6pbcmdgr53dff6033gcfl2jq0d762h-next-gtk-webkit-1.1.0/bin/next-gtk-webkit": No such file or directory

(Here ‘guix run’ runs ‘next’ in a container with exactly the closure of

‘next’, nothing more, and the ‘next’ binary is grafted.)

So the problem looks a lot like that this GCC issue we fixed a while

back: https://bugs.gnu.org/24703.

Looking at the ‘sbcl-next’ package, the reference to ‘next-gtk-webkit’

is inserted in gtk-webkit.lisp:

Toggle snippet (4 lines)

(defvar *gtk-webkit-command* "next-gtk-webkit"

"Path to the GTK-Webkit platform port executable.")

Through hexl-mode on the ‘next’ binary, we can find that reference:

Toggle snippet (27 lines)01d0bac0: 2f00 0000 6700 0000 6e00 0000 7500 0000  /...g...n...u...
01d0bad0: 2f00 0000 7300 0000 7400 0000 6f00 0000  /...s...t...o...
01d0bae0: 7200 0000 6500 0000 2f00 0000 3700 0000  r...e.../...7...
01d0baf0: 7000 0000 3600 0000 7000 0000 6200 0000  p...6...p...b...
01d0bb00: 6300 0000 6d00 0000 6400 0000 6700 0000  c...m...d...g...
01d0bb10: 7200 0000 3500 0000 3300 0000 6400 0000  r...5...3...d...
01d0bb20: 6600 0000 6600 0000 3600 0000 3000 0000  f...f...6...0...
01d0bb30: 3300 0000 3300 0000 6700 0000 6300 0000  3...3...g...c...
01d0bb40: 6600 0000 6c00 0000 3200 0000 6a00 0000  f...l...2...j...
01d0bb50: 7100 0000 3000 0000 6400 0000 3700 0000  q...0...d...7...
01d0bb60: 3600 0000 3200 0000 6800 0000 2d00 0000  6...2...h...-...
01d0bb70: 6e00 0000 6500 0000 7800 0000 7400 0000  n...e...x...t...
01d0bb80: 2d00 0000 6700 0000 7400 0000 6b00 0000  -...g...t...k...
01d0bb90: 2d00 0000 7700 0000 6500 0000 6200 0000  -...w...e...b...
01d0bba0: 6b00 0000 6900 0000 7400 0000 2d00 0000  k...i...t...-...
01d0bbb0: 3100 0000 2e00 0000 3100 0000 2e00 0000  1.......1.......
01d0bbc0: 3000 0000 2f00 0000 6200 0000 6900 0000  0.../...b...i...
01d0bbd0: 6e00 0000 2f00 0000 6e00 0000 6500 0000  n.../...n...e...
01d0bbe0: 7800 0000 7400 0000 2d00 0000 6700 0000  x...t...-...g...
01d0bbf0: 7400 0000 6b00 0000 2d00 0000 7700 0000  t...k...-...w...
01d0bc00: 6500 0000 6200 0000 6b00 0000 6900 0000  e...b...k...i...
01d0bc10: 7400 0000 0000 0000 0000 0000 0000 0000  t...............
01d0bc20: e100 0100 0000 0000 2800 0000 0000 0000  ........(.......
01d0bc30: 2a47 544b 2d57 4542 4b49 542d 434f 4d4d  *GTK-WEBKIT-COMM
01d0bc40: 414e 442a 0000 0000 0000 0000 0000 0000  AND*............

Apparently this string literal is stored as UTF-32 (UCS-4) or similar,

which prevents the reference scanner and the grafting code from finding

it, and problems ensue. :-)

Pierre, Andy: is there any way to tell SBCL to store this literal as

ASCII/UTF-8? That would be an easy fix, though we should discuss the

pros and cons and whether to enable that globally.

Thanks in advance!

Ludo’.

Pierre Neidhardt wrote on 23 Dec 2018 07:05

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87a7kwjnai.fsf@ambrevar.xyz

Thanks for looking into this, Ludo.

At first glance, I'd say that this is not a compilation option but the way

strings are encoded by default. It seems that multibyte encoding is used all

over the place by a few compilers including SBCL (and CCL I think).

One way I know around this (I'm by no mean a Common Lisp expert) is the

flexi-streams package for re-encoding.

More generally, shouldn't we make the reference scanner a bit smarter? In

particular, how does it handle non-ASCII references? Maybe it would not be

unreasonable to handle UTF-8 and UCS-4 for instance?

Pierre Neidhardt

https://ambrevar.xyz/

Mark H Weaver wrote on 23 Dec 2018 08:45

Re: bug#33848: Store references in SBCL-compiled code are "invisible"

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:877eg0i43j.fsf@netris.org

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (61 lines)> As discussed with Pierre at the R-B Summit, ‘sbcl-next’ lacks a
> reference to ‘next-gtk-webkit’ even though is invokes it:
>
> $ guix gc --references $(type -P next) | grep next-
> /gnu/store/9d66xb8wvggsp0x9pxj61mzqy007978f-sbcl-next-1.1.0
> /gnu/store/pqy064fw3vkfld6lw95vi0zavj19zvrc-sbcl-next-1.1.0-lib
> $ ./pre-inst-env guix run next
>
> WARNING: Setting locale failed.
>   Check the following variables for correct values:
>   LANG=en_US.utf8
> Unhandled SIMPLE-ERROR in thread #<SB-THREAD:THREAD "main thread" RUNNING
>                                     {10005885B3}>:
>   Couldn't execute "/gnu/store/7p6pbcmdgr53dff6033gcfl2jq0d762h-next-gtk-webkit-1.1.0/bin/next-gtk-webkit": No such file or directory
>
>
> (Here ‘guix run’ runs ‘next’ in a container with exactly the closure of
> ‘next’, nothing more, and the ‘next’ binary is grafted.)
>
> So the problem looks a lot like that this GCC issue we fixed a while
> back: <https://bugs.gnu.org/24703>.
>
> Looking at the ‘sbcl-next’ package, the reference to ‘next-gtk-webkit’
> is inserted in gtk-webkit.lisp:
>
> (defvar *gtk-webkit-command* "next-gtk-webkit"
>   "Path to the GTK-Webkit platform port executable.")
>
>
> Through hexl-mode on the ‘next’ binary, we can find that reference:
>
> 01d0bac0: 2f00 0000 6700 0000 6e00 0000 7500 0000  /...g...n...u...
> 01d0bad0: 2f00 0000 7300 0000 7400 0000 6f00 0000  /...s...t...o...
> 01d0bae0: 7200 0000 6500 0000 2f00 0000 3700 0000  r...e.../...7...
> 01d0baf0: 7000 0000 3600 0000 7000 0000 6200 0000  p...6...p...b...
> 01d0bb00: 6300 0000 6d00 0000 6400 0000 6700 0000  c...m...d...g...
> 01d0bb10: 7200 0000 3500 0000 3300 0000 6400 0000  r...5...3...d...
> 01d0bb20: 6600 0000 6600 0000 3600 0000 3000 0000  f...f...6...0...
> 01d0bb30: 3300 0000 3300 0000 6700 0000 6300 0000  3...3...g...c...
> 01d0bb40: 6600 0000 6c00 0000 3200 0000 6a00 0000  f...l...2...j...
> 01d0bb50: 7100 0000 3000 0000 6400 0000 3700 0000  q...0...d...7...
> 01d0bb60: 3600 0000 3200 0000 6800 0000 2d00 0000  6...2...h...-...
> 01d0bb70: 6e00 0000 6500 0000 7800 0000 7400 0000  n...e...x...t...
> 01d0bb80: 2d00 0000 6700 0000 7400 0000 6b00 0000  -...g...t...k...
> 01d0bb90: 2d00 0000 7700 0000 6500 0000 6200 0000  -...w...e...b...
> 01d0bba0: 6b00 0000 6900 0000 7400 0000 2d00 0000  k...i...t...-...
> 01d0bbb0: 3100 0000 2e00 0000 3100 0000 2e00 0000  1.......1.......
> 01d0bbc0: 3000 0000 2f00 0000 6200 0000 6900 0000  0.../...b...i...
> 01d0bbd0: 6e00 0000 2f00 0000 6e00 0000 6500 0000  n.../...n...e...
> 01d0bbe0: 7800 0000 7400 0000 2d00 0000 6700 0000  x...t...-...g...
> 01d0bbf0: 7400 0000 6b00 0000 2d00 0000 7700 0000  t...k...-...w...
> 01d0bc00: 6500 0000 6200 0000 6b00 0000 6900 0000  e...b...k...i...
> 01d0bc10: 7400 0000 0000 0000 0000 0000 0000 0000  t...............
> 01d0bc20: e100 0100 0000 0000 2800 0000 0000 0000  ........(.......
> 01d0bc30: 2a47 544b 2d57 4542 4b49 542d 434f 4d4d  *GTK-WEBKIT-COMM
> 01d0bc40: 414e 442a 0000 0000 0000 0000 0000 0000  AND*............
>
> Apparently this string literal is stored as UTF-32 (UCS-4) or similar,
> which prevents the reference scanner and the grafting code from finding
> it, and problems ensue.  :-)

IMO, we should consider modifying Guix to search for store references

encoded in UTF-32 and/or UTF-16. I wouldn't be surprised if some other

programs use those encodings. I'd be willing to work on it.

What do you think?

Mark

Ludovic Courtès wrote on 23 Dec 2018 09:32

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87d0psi1xo.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (2 lines)

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (8 lines)

>> Apparently this string literal is stored as UTF-32 (UCS-4) or similar,

>> which prevents the reference scanner and the grafting code from finding

>> it, and problems ensue. :-)

> IMO, we should consider modifying Guix to search for store references

> encoded in UTF-32 and/or UTF-16. I wouldn't be surprised if some other

> programs use those encodings. I'd be willing to work on it.

I don’t think we’ve encountered the problem before. This would require

fixing both the scanner and the grafting code (though eventually that

might be a single code base when the Scheme-implemented daemon is

merged) in non-trivial ways.

One issue is that users of an old daemon would get a different behavior

than users of a new daemon. It would be the first time we introduce

such a significant change in the daemon since Guix was started.

For now I lean towards looking for a way to address the issue

specifically for SBCL. I’d be tempted to generalize if and only if we

find other occurrences of the problem that would make the benefits

outweigh the development and maintenance costs.

WDYT?

I remember discussing in the past some sort of “pluggable” reference

scanning mechanism that could also work for compressed archives, etc.

That also looks like the right thing, but it has a development and

maintenance cost that’s pretty high whereas we might be able to address

the same problems in much simpler ways.

Thanks,

Ludo’.

Pierre Neidhardt wrote on 23 Dec 2018 14:01

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:874lb3kin6.fsf@ambrevar.xyz

Toggle quote (2 lines)

> I don’t think we’ve encountered the problem before.

Actually it does ring a bell for me. Didn't we have a similar issue with Fish,

or some dependency?

Toggle quote (3 lines)

> For now I lean towards looking for a way to address the issue

> specifically for SBCL.

Don't forget that we currently have 5 Lisp compilers.
Besides, it's not clear that this can be fixed on the compiler's side, it could
very well be that patches will be required  on a per-project basis.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

Ludovic Courtès wrote on 24 Dec 2018 06:55

control message for bug #33848

Recipients:(address . control@debbugs.gnu.org)

Message-ID:87a7kvgek5.fsf@gnu.org

severity 33848 important

Ludovic Courtès wrote on 24 Dec 2018 06:57

Re: bug#33848: Store references in SBCL-compiled code are "invisible"

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:875zvjgefl.fsf@gnu.org

Hi!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (9 lines)

> Thanks for looking into this, Ludo.

> At first glance, I'd say that this is not a compilation option but the way

> strings are encoded by default. It seems that multibyte encoding is used all

> over the place by a few compilers including SBCL (and CCL I think).

> One way I know around this (I'm by no mean a Common Lisp expert) is the

> flexi-streams package for re-encoding.

OK, we need to investigate.

Toggle quote (4 lines)

> More generally, shouldn't we make the reference scanner a bit smarter? In

> particular, how does it handle non-ASCII references? Maybe it would not be

> unreasonable to handle UTF-8 and UCS-4 for instance?

Store file names are always ASCII so problems arise when they are stored

as UTF-16 or UTF-32/UCS-4.

Ludo’.

Ludovic Courtès wrote on 24 Dec 2018 07:06

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87sgynezha.fsf@gnu.org

Hi Pierre,

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (5 lines)

>> I don’t think we’ve encountered the problem before.

> Actually it does ring a bell for me. Didn't we have a similar issue with Fish,

> or some dependency?

We did have a problem with Fish but I can no longer find it. Do you

remember what it was? Something with C++, no?

Toggle quote (7 lines)

>> For now I lean towards looking for a way to address the issue

>> specifically for SBCL.

> Don't forget that we currently have 5 Lisp compilers.

> Besides, it's not clear that this can be fixed on the compiler's side, it could

> very well be that patches will be required on a per-project basis.

I know little about CL but maybe we can find a solution that works for
all five compilers.  At least that would be the first approach I would
suggest following.

Thanks,
Ludo’.

Pierre Neidhardt wrote on 24 Dec 2018 09:08

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87r2e6j1hw.fsf@ambrevar.xyz

Toggle quote (3 lines)

> Store file names are always ASCII so problems arise when they are stored

> as UTF-16 or UTF-32/UCS-4.

I understand that most programs stick to ASCII filenames, but what about the odd

one using non-English, special characters?

Toggle quote (3 lines)

> We did have a problem with Fish but I can no longer find it. Do you

> remember what it was? Something with C++, no?

I think bug #30265.

Pierre Neidhardt

https://ambrevar.xyz/

Mark H Weaver wrote on 24 Dec 2018 10:12

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87tvj2yesd.fsf@netris.org

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (13 lines)> Pierre Neidhardt <mail@ambrevar.xyz> skribis:
>
>>> For now I lean towards looking for a way to address the issue
>>> specifically for SBCL.
>>
>> Don't forget that we currently have 5 Lisp compilers.
>> Besides, it's not clear that this can be fixed on the compiler's side, it could
>> very well be that patches will be required  on a per-project basis.
>
> I know little about CL but maybe we can find a solution that works for
> all five compilers.  At least that would be the first approach I would
> suggest following.

I can't imagine a solution that would work for all five compilers, but
perhaps that's a failure of imagination on my part.  Of course, you're
welcome to search for such a solution.  Can you give me a rough outline
of what you have in mind?

Of course, the usual reason to choose UTF-32 is to support non-ASCII
characters while retaining fixed-width code points, so that string
lookups are straightforward and efficient.  Using UTF-8 improves space
efficiency, but at the cost of extra code complexity.  That extra
complexity is what I guess we would need to add to each program that
currently uses UTF-32.  Alternatively, we could extend the on-disk
format to support UTF-8 and then add some kind of "load hook" that
converts the string to UTF-32 at load time.  Either way, it's likely to
be a can of worms.

Consider the case of Guile.  Years ago we agreed to switch to UTF-8 as
its sole internal string encoding, but it hasn't yet been done because
it's a big job, even for those of us already intimately familiar with
the code.

Now imagine how hard it would be for someone who barely uses Guile, but
nevertheless felt compelled to change our internal string representation
to use UTF-8.  Moreover, imagine that they hoped to find a single
solution that would work for several different Scheme implementations.

What would you say to them if they proposed to find a general solution
to convert several Scheme implementations to use UTF-8 as their string
representation, to save themselves the trouble of having to understand
each implementation individually?

I really think it would be a mistake to try to force every program and
language implementation to use our preferred string representation.  I
suspect it would be vastly easier to compromise and support a few other
popular string representations in Guix, namely UTF-16 and UTF-32.

If you don't want to change the daemon, it could be worked around in our
build-side code as follows: we could add a new phase to certain build
systems (or possibly gnu-build-system) that scans each output for
UTF-16/32 encoded store references that are never referenced in UTF-8.
If such references exist, a file with an unobtrusive name would be added
to that output containing those references encoded in UTF-8.  This would
enable our daemon's existing reference scanner to find all of the
references.

Our grafting code would then need to be extended to recognize and
transform store references encoded in UTF-16/32 as well as UTF-8.

What do you think?

      Regards,
        Mark

Pierre Neidhardt wrote on 24 Dec 2018 15:58

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87pntqiijc.fsf@ambrevar.xyz

I find Mark's points reasonable, although to be honest I have very little
knowledge of the daemon.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

Ludovic Courtès wrote on 26 Dec 2018 08:07

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87r2e4e0fu.fsf@gnu.org

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (6 lines)

>> Store file names are always ASCII so problems arise when they are stored

>> as UTF-16 or UTF-32/UCS-4.

> I understand that most programs stick to ASCII filenames, but what about the odd

> one using non-English, special characters?

That’s a separate debate. :-) Essentially this restriction on store

file names has always been there in Guix (and Nix before that). If we

were to change it, that would raise compatibility issues.

Toggle quote (5 lines)

>> We did have a problem with Fish but I can no longer find it. Do you

>> remember what it was? Something with C++, no?

> I think bug #30265.

Oh I see, UCS-4 as well.  (I can’t believe this bug is still open given
the relatively simple solutions outlined at
https://issues.guix.info/issue/30265#8.  :-))

Thanks,
Ludo’.

Ludovic Courtès wrote on 26 Dec 2018 08:14

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:877efwe04u.fsf@gnu.org

Hello!

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (20 lines)> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Pierre Neidhardt <mail@ambrevar.xyz> skribis:
>>
>>>> For now I lean towards looking for a way to address the issue
>>>> specifically for SBCL.
>>>
>>> Don't forget that we currently have 5 Lisp compilers.
>>> Besides, it's not clear that this can be fixed on the compiler's side, it could
>>> very well be that patches will be required  on a per-project basis.
>>
>> I know little about CL but maybe we can find a solution that works for
>> all five compilers.  At least that would be the first approach I would
>> suggest following.
>
> I can't imagine a solution that would work for all five compilers, but
> perhaps that's a failure of imagination on my part.  Of course, you're
> welcome to search for such a solution.  Can you give me a rough outline
> of what you have in mind?

I have nothing specific in mind, I’m just brainstorming with everyone

here. :-)

For a similar situation in C++, there’s a fairly simple and local

workaround:

https://issues.guix.info/issue/30265#8

I’m not familiar with CL but I thought that it we could achieve

something similar, that would be great—I’m not suggesting to change the

CL compilers in any non-trivial way.

For example I guess we could always store the file name as a literal

byte vector/list and add a call to turn that into a string.

Does that make sense?

Thanks,

Ludo’.

Pierre Neidhardt wrote on 27 Dec 2018 02:37

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:8736qji7c1.fsf@ambrevar.xyz

Toggle quote (10 lines)> : > Store file names are always ASCII so problems arise when they are stored
> : > as UTF-16 or UTF-32/UCS-4.
> : 
> : I understand that most programs stick to ASCII filenames, but what about the odd
> : one using non-English, special characters?
> 
> That’s a separate debate.  :-)  Essentially this restriction on store
> file names has always been there in Guix (and Nix before that).  If we
> were to change it, that would raise compatibility issues.

But what happens if we attempt to store "á" in the store?

Toggle quote (3 lines)

> For example I guess we could always store the file name as a literal

> byte vector/list and add a call to turn that into a string.

In the case of Next, that would be a simple patch, but other programs could get

much more complicated. In the end, this approach requires a linear amount of

work. Conversely, adding UCS-* support to the scanner would fix this issue once

and for all.

Toggle quote (9 lines)

> : > We did have a problem with Fish but I can no longer find it. Do you

> : > remember what it was? Something with C++, no?

> :

> : I think bug #30265.

> Oh I see, UCS-4 as well. (I can’t believe this bug is still open given

> the relatively simple solutions outlined at

> <https://issues.guix.info/issue/30265#8>. :-))

Well, if currently only two packages out of 8500+ suffer from this, then I think

it's easier to go with Ludo's suggestion of patching the code to use ASCII

strings.

Does anyone know about more packages with this issue? It could also be that

more packages suffer from this, unbeknownst to us.

Pierre Neidhardt

https://ambrevar.xyz/

Danny Milosavljevic wrote on 27 Dec 2018 05:52

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:20181227145258.0c420eac@scratchpost.org

Hi Mark,

On Mon, 24 Dec 2018 13:12:23 -0500

Mark H Weaver <mhw@netris.org> wrote:

Toggle quote (4 lines)

> Of course, the usual reason to choose UTF-32 is to support non-ASCII

> characters while retaining fixed-width code points, so that string

> lookups are straightforward and efficient.

This kind of lookup is almost never what is necessary. There are many

people who assume character is the same as codepoint and to those people

UTF-32 brings something to the table, but it's really not useful if people

do text processing correctly, see below.

(Of course whether packages actually do this remains to be seen)

Toggle quote (3 lines)

> Using UTF-8 improves space efficiency, but at the cost of extra code

>complexity.

I agree.

Toggle quote (4 lines)

> That extra

> complexity is what I guess we would need to add to each program that

> currently uses UTF-32.

Yes, but they usually have to do stream processing even with UTF-32 (because

a character can be composed of possibly infinite number of codepoints),

so the infrastructure should be already there and the effort should be

minimal.

Toggle quote (5 lines)

> Alternatively, we could extend the on-disk

> format to support UTF-8 and then add some kind of "load hook" that

> converts the string to UTF-32 at load time. Either way, it's likely to

> be a can of worms.

If it ever came to that, a pluggable reference scanner would be

preferrable. But really, it would irk me to have so much complexity

in something so basic (the reference scanner) for no end-user gain

(as a distribution we could just mandate UTF-8 for references and the

problem would be gone for the user with no loss of functionality).

It's always easy to add special cases - but more code means more bugs

and I think if possible it's best to have only the simple case implemented

in the core - because it's less complicated which means more likely

to be correct (for the case it does handle). In the end it depends on

what would be more code, and more widely used.

Also, if we wanted to debug reference errors, we couldn't use grep anymore

because it can't handle utf-32 either (neither can any of the other UNIX tools).

Also, I really don't want to return to the time where I had to call iconv

once every three commands to be able to do anything useful on UNIX.

Also, the build daemon is written in C++ and C++ strings are widely

known to have very very bad codepoint awareness (to say nothing about

the horrible conversion facilities).

Also, if both UTF-32 and UTF-8 are used on disk, care needs to not misdetect

an UTF-8 sequence as an UTF-32 sequence of different text - or the other way

around -, but that's unlikely for ASCII strings.

Toggle quote (5 lines)

> I really think it would be a mistake to try to force every program and

> language implementation to use our preferred string representation. I

> suspect it would be vastly easier to compromise and support a few other

> popular string representations in Guix, namely UTF-16 and UTF-32.

In 1992, UTF-8 was invented.  Subsequently, most of the Internet,
all new GNU Linux distributions etc, all UNIX GUI frameworks, Subversion
etc standardized on UTF-8, with the eventual goal of standardizing all
network transfer and storage to UTF-8.  I think that by now the outliers
are the ones who need to change, otherwise these senseless encoding
conversions will never cease.  It's not like different encodings allow for
better expression of writings or anything useful to the end user.

As a distribution we can't force upstream to change, but just filing
bug reports upstream would make us see where they stand on this.

Toggle quote (9 lines)

> If you don't want to change the daemon, it could be worked around in our

> build-side code as follows: we could add a new phase to certain build

> systems (or possibly gnu-build-system) that scans each output for

> UTF-16/32 encoded store references that are never referenced in UTF-8.

> If such references exist, a file with an unobtrusive name would be added

> to that output containing those references encoded in UTF-8. This would

> enable our daemon's existing reference scanner to find all of the

> references.

I agree that that would be nice.  As a first step, even just detecting
problems like that and erroring out would be okay - in order to find them
in the first place.  Right now, it's difficult to detect and so also difficult
to say how wide-spread the problem is.  If the problem is wide-spread enough
my tune could change very quickly.

What you propose is similar to what I did in Java in Guix, only it gives
us even more advantages in the Java case (faster class loading and
eventual non-propagated inputs).

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlwk2ToACgkQ5xo1VCww

uqUUzAgApbxUHv/XlbjYMXvV4cOY0maxbx92ndZlJiukCN+bIiMqhuCd7PdEoL7Z

1d9ABxe+2oXO4Nkjpez71nhK8ym8KwRYNDuTkCSZbzUJwNEee2pF/OlU2Y+Jugz5

ICSlYGCFfwx6Buf9bZReYq1e5qjO//QSytgYC061gYURw/abtGSEyvllHWv4qrl6

DFfQuQilycHAOqrT/ACBtMgFFnsV7miHs6CrKTSPPBWKKuA3BM4STNUfHlMeb8un

gNUH3ijbDviqBgRiqDy50dZ0kbFv8zSm1LytoySX0qZ7j5oidDJGHATGbEXp4sDU

1dPmBSYeasLAVfJn4RQSQFAKba6TuA==

=gkV6

-----END PGP SIGNATURE-----

Mark H Weaver wrote on 27 Dec 2018 06:03

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87tvizvzgk.fsf@netris.org

Pierre Neidhardt <mail@ambrevar.xyz> writes:

Toggle quote (12 lines)>> : > Store file names are always ASCII so problems arise when they are stored
>> : > as UTF-16 or UTF-32/UCS-4.
>> : 
>> : I understand that most programs stick to ASCII filenames, but what about the odd
>> : one using non-English, special characters?
>> 
>> That’s a separate debate.  :-)  Essentially this restriction on store
>> file names has always been there in Guix (and Nix before that).  If we
>> were to change it, that would raise compatibility issues.
>
> But what happens if we attempt to store "á" in the store?

Indeed.  Although we might restrict the immediate entries within
/gnu/store to ASCII characters, file names deeper within those
directories may have non-ASCII characters.  More generally, store
references may occur within larger strings which might include non-ASCII
characters.

       Mark

Mark H Weaver wrote on 27 Dec 2018 06:29

Recipients:(name . Danny Milosavljevic)(address . dannym@scratchpost.org)

Message-ID:87pntnvy8e.fsf@netris.org

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> writes:

Toggle quote (21 lines)> On Mon, 24 Dec 2018 13:12:23 -0500
> Mark H Weaver <mhw@netris.org> wrote:
>
>> Of course, the usual reason to choose UTF-32 is to support non-ASCII
>> characters while retaining fixed-width code points, so that string
>> lookups are straightforward and efficient.
>
> This kind of lookup is almost never what is necessary.  There are many
> people who assume character is the same as codepoint and to those people
> UTF-32 brings something to the table, but it's really not useful if people
> do text processing correctly, see below.
>
> (Of course whether packages actually do this remains to be seen)
>
>>  That extra
>> complexity is what I guess we would need to add to each program that
>> currently uses UTF-32.
>
> Yes, but they usually have to do stream processing even with UTF-32 (because
> a character can be composed of possibly infinite number of codepoints),

I agree with you. However, as silly as it might be, the fact remains

that almost every modern programming language and string library uses

code points as the base units by which to index strings.

Toggle quote (3 lines)

> so the infrastructure should be already there and the effort should be

> minimal.

The infrastructure might or might not be there, depending on the

sophistication of the program's unicode support, but even if it _is_

there, it will most likely be a layer that expects to iterate over

strings indexed by code point to find graphemes, etc.

Anyway, if you truly believe the effort should be minimal, feel free to

investigate and propose patches to fix our 5 common lisp compilers and

Fish to avoid storing UTF-32 in the object code.

Toggle quote (4 lines)

> Also, if both UTF-32 and UTF-8 are used on disk, care needs to not misdetect

> an UTF-8 sequence as an UTF-32 sequence of different text - or the other way

> around -, but that's unlikely for ASCII strings.

This is not an issue because the substrings that the reference scanner

and grafter are looking for are ASCII-only, even if they are part of a

larger non-ASCII string. Specifically, they only need to look for the

nix hashes.

Toggle quote (11 lines)>> I really think it would be a mistake to try to force every program and
>> language implementation to use our preferred string representation.  I
>> suspect it would be vastly easier to compromise and support a few other
>> popular string representations in Guix, namely UTF-16 and UTF-32.
>
> In 1992, UTF-8 was invented.  Subsequently, most of the Internet,
> all new GNU Linux distributions etc, all UNIX GUI frameworks, Subversion
> etc standardized on UTF-8, with the eventual goal of standardizing all
> network transfer and storage to UTF-8.  I think that by now the outliers
> are the ones who need to change,

I agree that we need to standardize on Unicode. However, given the

perhaps unfortunate fact that almost everyone has standardized on code

points as the units by which to index strings, choosing UTF-32 as an

internal representation is a very reasonable choice, IMO.

Anyway, feel free to engage with the developers of the Common Lisp

implementations that use UTF-32 and try to convince them to change.

The remaining question is: what to do if upstream refuses to change? Do

we exclude that software in Guix, or do we maintain our own patches to

override upstream's decision?

Toggle quote (15 lines)>> If you don't want to change the daemon, it could be worked around in our
>> build-side code as follows: we could add a new phase to certain build
>> systems (or possibly gnu-build-system) that scans each output for
>> UTF-16/32 encoded store references that are never referenced in UTF-8.
>> If such references exist, a file with an unobtrusive name would be added
>> to that output containing those references encoded in UTF-8.  This would
>> enable our daemon's existing reference scanner to find all of the
>> references.
>
> I agree that that would be nice.  As a first step, even just detecting
> problems like that and erroring out would be okay - in order to find them
> in the first place.  Right now, it's difficult to detect and so also difficult
> to say how wide-spread the problem is.  If the problem is wide-spread enough
> my tune could change very quickly.

Sure, it would be useful to have more data on what packages are

currently affected by this issue.

Regards,

Mark

Ludovic Courtès wrote on 27 Dec 2018 06:45

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87o9979gfn.fsf@gnu.org

Hello,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (20 lines)> Pierre Neidhardt <mail@ambrevar.xyz> writes:
>
>>> : > Store file names are always ASCII so problems arise when they are stored
>>> : > as UTF-16 or UTF-32/UCS-4.
>>> : 
>>> : I understand that most programs stick to ASCII filenames, but what about the odd
>>> : one using non-English, special characters?
>>> 
>>> That’s a separate debate.  :-)  Essentially this restriction on store
>>> file names has always been there in Guix (and Nix before that).  If we
>>> were to change it, that would raise compatibility issues.
>>
>> But what happens if we attempt to store "á" in the store?
>
> Indeed.  Although we might restrict the immediate entries within
> /gnu/store to ASCII characters, file names deeper within those
> directories may have non-ASCII characters.  More generally, store
> references may occur within larger strings which might include non-ASCII
> characters.

Right. For example ‘nss-certs’ contains non-ASCII, UTF-8-encoded file

names.

For “top-level” store file names, the restriction is enforced by

‘checkStoreName’ in libstore/store-api.cc.

Ludo’.

Pierre Neidhardt wrote on 27 Dec 2018 07:02

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87tvizgghs.fsf@ambrevar.xyz

Just to be sure I understand: non-toplevel, non-ASCII file names will
not be scanned properly, right?

-- 
Pierre Neidhardt
https://ambrevar.xyz/

Pierre Neidhardt wrote on 27 Dec 2018 08:15

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87r2e3gd3c.fsf@ambrevar.xyz

Danny Milosavljevic <dannym@scratchpost.org> writes:

Toggle quote (11 lines)> In 1992, UTF-8 was invented.  Subsequently, most of the Internet,
> all new GNU Linux distributions etc, all UNIX GUI frameworks, Subversion
> etc standardized on UTF-8, with the eventual goal of standardizing all
> network transfer and storage to UTF-8.  I think that by now the outliers
> are the ones who need to change, otherwise these senseless encoding
> conversions will never cease.  It's not like different encodings allow for
> better expression of writings or anything useful to the end user.
> 
> As a distribution we can't force upstream to change, but just filing
> bug reports upstream would make us see where they stand on this.

I agree with this. Reporting upstream should be a first step.

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 27 Dec 2018 09:03

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87k1juaomo.fsf@gnu.org

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (3 lines)

> Just to be sure I understand: non-toplevel, non-ASCII file names will

> not be scanned properly, right?

Every file in the store is properly scanned for references. It’s just

that users cannot create top-level items with a non-ASCII file name.

I hope this clarifies things!

Ludo’.

Pierre Neidhardt wrote on 27 Dec 2018 10:57

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87muoqhk62.fsf@ambrevar.xyz

Toggle quote (3 lines)

> Every file in the store is properly scanned for references. It’s just

> that users cannot create top-level items with a non-ASCII file name.

So if '/gnu/store/...-foo/á' is stored as UTF-8 in a binary, then it will be

found? Is it because the filesystem encoding is also UTF-8 and Guix scans over

byte arrays?

Sorry for dragging on this, I guess I should look at the code at this point but

I have very little time these days.

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 27 Dec 2018 13:54

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87zhsq8wkj.fsf@gnu.org

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (7 lines)

>> Every file in the store is properly scanned for references. It’s just

>> that users cannot create top-level items with a non-ASCII file name.

> So if '/gnu/store/...-foo/á' is stored as UTF-8 in a binary, then it will be

> found? Is it because the filesystem encoding is also UTF-8 and Guix scans over

> byte arrays?

The reference scanner, currently written in C++, traverses whole

directory trees. Being C++ it treats file names as byte arrays so it

doesn’t matter what the file name encoding is.

Note also that the reference scanner only looks for “xyz…-foo”; what

comes before and after doesn’t matter. So for example if you have

“/gnu/store/xyz…-foo/à”, what’s important is the “xyz…-foo” bit.

This is all happening in libstore/references.cc (which is surprisingly

small) and in (guix build graft) for the grafting part, which Mark wrote

a while back.

HTH,

Ludo’.

Pierre Neidhardt wrote on 27 Dec 2018 14:05

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87d0pmhbgn.fsf@ambrevar.xyz

Toggle quote (4 lines)

> The reference scanner, currently written in C++, traverses whole

> directory trees. Being C++ it treats file names as byte arrays so it

> doesn’t matter what the file name encoding is.

But what matters then is that the filename encodings on the filesystem and in the

binary match, right?

Toggle quote (4 lines)

> Note also that the reference scanner only looks for “xyz…-foo”; what

> comes before and after doesn’t matter. So for example if you have

> “/gnu/store/xyz…-foo/à”, what’s important is the “xyz…-foo” bit.

OK, makes sense, then my main worry is just moot :)

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 27 Dec 2018 14:59

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87r2e28tkv.fsf@gnu.org

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (7 lines)

>> The reference scanner, currently written in C++, traverses whole

>> directory trees. Being C++ it treats file names as byte arrays so it

>> doesn’t matter what the file name encoding is.

> But what matters then is that the filename encodings on the filesystem and in the

> binary match, right?

I’m not sure what you call “the binary”. Do you mean the nar?

Ludo’.

Pierre Neidhardt wrote on 27 Dec 2018 23:47

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:874laygkiy.fsf@ambrevar.xyz

Toggle quote (2 lines)

> I’m not sure what you call “the binary”. Do you mean the nar?

No, in this case I referred to "/bin/next" in sbcl-next.  So any file in the nar
passed to the reference scanner.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

Pierre Neidhardt wrote on 30 Mar 2021 03:15

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87lfa5eymf.fsf@ambrevar.xyz

Many grafting issues with Nyxt have been reported recently:

https://github.com/atlas-engineer/nyxt/issues/1257

Seems that glib is being grafted, which breaks cl-cffi-gtk and thus

Nyxt.

Is there a way to disable grafting for a given package? This would at

least be a local workaround for Nyxt.

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 30 Mar 2021 13:09

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87tuoscsk9.fsf@gnu.org

Hi,

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (10 lines)> Many grafting issues with Nyxt have been reported recently:
>
> https://github.com/atlas-engineer/nyxt/issues/1257
>
> Seems that glib is being grafted, which breaks cl-cffi-gtk and thus
> Nyxt.
>
> Is there a way to disable grafting for a given package?  This would at
> least be a local workaround for Nyxt.

No. I provided guidance in 2018 on how to address this issue:

https://issues.guix.gnu.org/33848

Did anyone talk to the SBCL folks? Please, let’s address this rather

than look for workarounds.

Thanks,

Ludo’.

Pierre Neidhardt wrote on 31 Mar 2021 00:10

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87sg4bdci0.fsf@ambrevar.xyz

Toggle quote (4 lines)

> I provided guidance in 2018 on how to address this issue:

> https://issues.guix.gnu.org/33848

Looks like you are linking to this very same thread :)

Toggle quote (3 lines)

> Did anyone talk to the SBCL folks? Please, let’s address this rather

> than look for workarounds.

I've just asked them for advice:

https://bugs.launchpad.net/sbcl/+bug/1922011

Pierre Neidhardt

https://ambrevar.xyz/

Pierre Neidhardt wrote on 31 Mar 2021 09:12

Recipients:

Message-ID:87im57b8u7.fsf@ambrevar.xyz

A brief discussion has ensued on SBCL bugtracker:

- It's unlikely that SBCL will change its internal string

representation.

- The main recommendation for an easy fix without updating the scanner

is that we tweaked our build system to dump the store reference to a

separate ASCII file.

Thoughts?

Guillaume?

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 31 Mar 2021 13:42

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87zgyj5a33.fsf@gnu.org

Hi,

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (2 lines)

> A brief discussion has ensued on SBCL bugtracker:

Nice, thanks for starting the discussion!

Toggle quote (3 lines)

> - It's unlikely that SBCL will change its internal string

> representation.

Of course, I would not suggest that.

What could have been nice is if there’s a way to mark specific strings

as being ASCII, or if there’s a “byte vector” data type compatible with

strings, for instance.

Toggle quote (4 lines)

> - The main recommendation for an easy fix without updating the scanner

> is that we tweaked our build system to dump the store reference to a

> separate ASCII file.

That’s a good idea: simple and efficient. We do that for the initrd,

for instance (commit b36e06c2b0889f1d0f939465589d36887ff24d33).

This could be done in ‘asdf-build-system/sbcl’ I suppose. I can see two

drawbacks:

1. Some packages like ‘nyxt’ don’t use it, so we’d have to duplicate

the phase there.

2. It may be coarse-grain compared to scanning binaries for references

(for example, we might retain references to build-only tools, such

as libraries used only for tests).

That’s probably acceptable though, and certainly better than the status quo.

Thanks,

Ludo’.

Pierre Neidhardt wrote on 31 Mar 2021 13:57

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:874kgravnm.fsf@ambrevar.xyz

Hi Ludo,

Toggle quote (4 lines)

> What could have been nice is if there’s a way to mark specific strings

> as being ASCII, or if there’s a “byte vector” data type compatible with

> strings, for instance.

It does and it could work, but according to upstream it's just simpler

to dump deps in a separate file.

Toggle quote (3 lines)

> 1. Some packages like ‘nyxt’ don’t use it, so we’d have to duplicate

> the phase there.

In the case of Nyxt, the reason it does not use it is because the

asdf-build-system/sbcl binary production has some drawbacks. I could

work on overhauling the build system to fix the uncanny behaviour, then

Nyxt would use it.

Toggle quote (4 lines)

> 2. It may be coarse-grain compared to scanning binaries for references

> (for example, we might retain references to build-only tools, such

> as libraries used only for tests).

The build system could easily leave out Lisp native-inputs, no?

Cheers!

Pierre Neidhardt

https://ambrevar.xyz/

Mark H Weaver wrote on 31 Mar 2021 23:03

Recipients:(address . 33848@debbugs.gnu.org)

Message-ID:87czvebky2.fsf@netris.org

Pierre Neidhardt <mail@ambrevar.xyz> writes:

Toggle quote (4 lines)

> - The main recommendation for an easy fix without updating the scanner

> is that we tweaked our build system to dump the store reference to a

> separate ASCII file.

Sounds good.  I made a similar proposal in Dec 2018, earlier in this
thread https://bugs.gnu.org/33848#31.  I wrote:

  If you don't want to change the daemon, it could be worked around in our
  build-side code as follows: we could add a new phase to certain build
  systems (or possibly gnu-build-system) that scans each output for
  UTF-16/32 encoded store references that are never referenced in UTF-8.
  If such references exist, a file with an unobtrusive name would be added
  to that output containing those references encoded in UTF-8.  This would
  enable our daemon's existing reference scanner to find all of the
  references.

  Our grafting code would then need to be extended to recognize and
  transform store references encoded in UTF-16/32 as well as UTF-8.

      Mark

Pierre Neidhardt wrote on 1 Apr 2021 00:13

Recipients:(address . 33848@debbugs.gnu.org)

Message-ID:87sg4aa35g.fsf@ambrevar.xyz

Thank you for the reminder, Mark, I had forgotten about your suggestion.

If we are going for a SBCL-specific solution, I believe that all

references are stored in plain text files at some points (the .asd files

and the .lisp files) which are often encoded in ASCII / UTF-8, so

searching these files without having to deal with their encoding might

be enough. But of course this is less general and more brittle.

Mark, Guillaume, would you have time to give this a try?

I'm a bit busy at the moment. Let me know if you can't work on it, I'll

try to find time to work on a patch.

Cheers!

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 1 Apr 2021 00:57

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87eefu30a4.fsf@gnu.org

Hi,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (20 lines)> Pierre Neidhardt <mail@ambrevar.xyz> writes:
>> - The main recommendation for an easy fix without updating the scanner
>>   is that we tweaked our build system to dump the store reference to a
>>   separate ASCII file.
>
> Sounds good.  I made a similar proposal in Dec 2018, earlier in this
> thread <https://bugs.gnu.org/33848#31>.  I wrote:
>
>   If you don't want to change the daemon, it could be worked around in our
>   build-side code as follows: we could add a new phase to certain build
>   systems (or possibly gnu-build-system) that scans each output for
>   UTF-16/32 encoded store references that are never referenced in UTF-8.
>   If such references exist, a file with an unobtrusive name would be added
>   to that output containing those references encoded in UTF-8.  This would
>   enable our daemon's existing reference scanner to find all of the
>   references.
>
>   Our grafting code would then need to be extended to recognize and
>   transform store references encoded in UTF-16/32 as well as UTF-8.

Oh thanks for the reminder.

The separate ASCII file doesn’t solve it all because, as you write, we’d

need to change the grafting code as well.

Then it might be simpler to use a “byte vector” data type for those

strings. We’ll have to wait for Pierre’s patches to get a better idea.

:-)

Thanks,

Ludo’.

Pierre Neidhardt wrote on 1 Apr 2021 01:48

Recipients:

Message-ID:87eefu9yqe.fsf@ambrevar.xyz

Hi!

Toggle quote (6 lines)

> The separate ASCII file doesn’t solve it all because, as you write, we’d

> need to change the grafting code as well.

> Then it might be simpler to use a “byte vector” data type for those

> strings.

Which strings and where would we use byte vectors?

Toggle quote (3 lines)

> We’ll have to wait for Pierre’s patches to get a better idea.

> :-)

By "Pierre's patches", you mean a patch to add a build phase that

generates an file listings all references?

Cheers!

Pierre Neidhardt

https://ambrevar.xyz/

Guillaume Le Vaillant wrote on 1 Apr 2021 02:07

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87im56l6es.fsf@yamatai

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (6 lines)

> If we are going for a SBCL-specific solution, I believe that all

> references are stored in plain text files at some points (the .asd files

> and the .lisp files) which are often encoded in ASCII / UTF-8, so

> searching these files without having to deal with their encoding might

> be enough. But of course this is less general and more brittle.

A store reference to a C library in a standalone Lisp binary can come

either from the current package or from some dependencies

(cl+ssl, cl-cffi-gtk, etc.). So we would need to scan the source

code of all the Lisp dependencies recursively to get the full list of

store refrences.

And as Mark wrote below, with the current grafting code, this list of

store references will not solve grafting for paths that are in UTF-32 or

both in ASCII and UTF-32 in the binary file.

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (31 lines)> Mark H Weaver <mhw@netris.org> skribis:
>
>> Pierre Neidhardt <mail@ambrevar.xyz> writes:
>>> - The main recommendation for an easy fix without updating the scanner
>>>   is that we tweaked our build system to dump the store reference to a
>>>   separate ASCII file.
>>
>> Sounds good.  I made a similar proposal in Dec 2018, earlier in this
>> thread <https://bugs.gnu.org/33848#31>.  I wrote:
>>
>>   If you don't want to change the daemon, it could be worked around in our
>>   build-side code as follows: we could add a new phase to certain build
>>   systems (or possibly gnu-build-system) that scans each output for
>>   UTF-16/32 encoded store references that are never referenced in UTF-8.
>>   If such references exist, a file with an unobtrusive name would be added
>>   to that output containing those references encoded in UTF-8.  This would
>>   enable our daemon's existing reference scanner to find all of the
>>   references.
>>
>>   Our grafting code would then need to be extended to recognize and
>>   transform store references encoded in UTF-16/32 as well as UTF-8.
>
> Oh thanks for the reminder.
>
> The separate ASCII file doesn’t solve it all because, as you write, we’d
> need to change the grafting code as well.
>
> Then it might be simpler to use a “byte vector” data type for those
> strings.  We’ll have to wait for Pierre’s patches to get a better idea.
> :-)

I'm not sure what you mean with the "byte vector" data type here. Could

you explain what you're thinking about with a few more details?

-----BEGIN PGP SIGNATURE-----

iIUEAREKAC0WIQTLxZxm7Ce5cXlAaz5r6CCK3yH+PwUCYGWNSw8cZ2x2QHBvc3Rl

by5uZXQACgkQa+ggit8h/j8KtgEAoyvmikRXpUHbT3CLPgewYs1QE7dXwa/fSb96

bR8rM6YBAJpnLOUegNHW8lTLfJu8ats/gdkdL+TFysYRBtjIU6Sz

=5hTk

-----END PGP SIGNATURE-----

Pierre Neidhardt wrote on 1 Apr 2021 02:13

Recipients:

Message-ID:87wntm8j18.fsf@ambrevar.xyz

Hi Guillaume!

Toggle quote (6 lines)

> A store reference to a C library in a standalone Lisp binary can come

> either from the current package or from some dependencies

> (cl+ssl, cl-cffi-gtk, etc.). So we would need to scan the source

> code of all the Lisp dependencies recursively to get the full list of

> store refrences.

I don't think there is need to scan recursively: if package A depends on

B which depends on C, then A can lists the dependency on B in a file,

and B can do the same for C. This way the relationship between A and C

is properly stored.

Am I missing something?

Toggle quote (4 lines)

> And as Mark wrote below, with the current grafting code, this list of

> store references will not solve grafting for paths that are in UTF-32 or

> both in ASCII and UTF-32 in the binary file.

Indeed, and that's the core of the issue here I believe, since grafting

is what breaks Nyxt in practice.

Cheers!

Pierre Neidhardt

https://ambrevar.xyz/

Guillaume Le Vaillant wrote on 1 Apr 2021 02:52

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87a6qil4b1.fsf@yamatai

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (15 lines)> Hi Guillaume!
>
>> A store reference to a C library in a standalone Lisp binary can come
>> either from the current package or from some dependencies
>> (cl+ssl, cl-cffi-gtk, etc.). So we would need to scan the source
>> code of all the Lisp dependencies recursively to get the full list of
>> store refrences.
>
> I don't think there is need to scan recursively: if package A depends on
> B which depends on C, then A can lists the dependency on B in a file,
> and B can do the same for C.  This way the relationship between A and C
> is properly stored.
>
> Am I missing something?

Oh, you say this file would be created for every Lisp package. I thought

it would only be for the standalone binary case, because the "regular"

asdf-build-system/sbcl used for Lisp libraries ships the sources and its

make-asdf-configuration phase creates links to the required Lisp

dependencies in '/gnu/store/...', so there should not be missing

references.

Toggle quote (9 lines)

>> And as Mark wrote below, with the current grafting code, this list of

>> store references will not solve grafting for paths that are in UTF-32 or

>> both in ASCII and UTF-32 in the binary file.

> Indeed, and that's the core of the issue here I believe, since grafting

> is what breaks Nyxt in practice.

> Cheers!

I just wondered: does the grafting code take '.fasl' files into
consideration?
If yes, I guess a Lisp library could also end up in a strange
half-grafted state if the grafting code modifies ASCII references and
not UTF-32 ones...

-----BEGIN PGP SIGNATURE-----

iIUEAREKAC0WIQTLxZxm7Ce5cXlAaz5r6CCK3yH+PwUCYGWX8g8cZ2x2QHBvc3Rl

by5uZXQACgkQa+ggit8h/j+FZAD+NJBBrb4xTqeC8wYQb956Wv1WiGyWm9LTFln5

dXAIqfEA/2O1IKrJYZG/fItBPNLo2UR5PYJwolfNVIL4fwwBHLlk

=lw/O

-----END PGP SIGNATURE-----

Pierre Neidhardt wrote on 1 Apr 2021 03:06

Recipients:(name . Guillaume Le Vaillant)(address . glv@posteo.net)

Message-ID:87czvez5c1.fsf@ambrevar.xyz

Guillaume Le Vaillant <glv@posteo.net> writes:

Toggle quote (7 lines)

> Oh, you say this file would be created for every Lisp package. I thought

> it would only be for the standalone binary case, because the "regular"

> asdf-build-system/sbcl used for Lisp libraries ships the sources and its

> make-asdf-configuration phase creates links to the required Lisp

> dependencies in '/gnu/store/...', so there should not be missing

> references.

Right.

The only case where there could be a missing reference is if the source

code contains an FFI reference stored in non-ASCII / UTF-8.

So we need to parse other encodings too as Mark suggested if I

understand correctly.

Toggle quote (6 lines)

> I just wondered: does the grafting code take '.fasl' files into

> consideration?

> If yes, I guess a Lisp library could also end up in a strange

> half-grafted state if the grafting code modifies ASCII references and

> not UTF-32 ones...

Absolutely, .fasls suffer from the same problem since they may encode
strings as UTF-32.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

Pierre Neidhardt wrote on 1 Apr 2021 03:07

Recipients:(name . Guillaume Le Vaillant)(address . glv@posteo.net)

Message-ID:87a6qiz5b3.fsf@ambrevar.xyz

I'm not familiar with the grafting code, so anyone who is (Mark? Ludo?)
might be able to fix this much quicker than me! :)

-- 
Pierre Neidhardt
https://ambrevar.xyz/

Ludovic Courtès wrote on 1 Apr 2021 08:24

Recipients:(name . Pierre Neidhardt)(address . mail@ambrevar.xyz)

Message-ID:87pmze58p6.fsf@gnu.org

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

Toggle quote (3 lines)

> I'm not familiar with the grafting code, so anyone who is (Mark? Ludo?)

> might be able to fix this much quicker than me! :)

There’s ‘%graft-hooks’ in (guix build graft). One could add a hook that

would do nothing except for SBCL packages; for SBCL packages, it would

to the UCS-4 rewriting “somehow”.

The other option, which might be easier, would be to arrange to not use

UCS-4 in the first place, by choosing a bytevector data type for string

literals known to contain a store reference. It can be done if we know

where those references come from—e.g., they’re introduced by

‘substitute*’ on the source.

I hope this makes sense!

Ludo’.

Mark H Weaver wrote on 1 Apr 2021 10:23

Recipients:

Message-ID:875z15c3zx.fsf@netris.org

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (4 lines)

> What could have been nice is if there’s a way to mark specific strings

> as being ASCII, or if there’s a “byte vector” data type compatible with

> strings, for instance.

Do we know that all strings containing store references will be

representable in ASCII?

Mark

Mark H Weaver wrote on 1 Apr 2021 10:33

Recipients:

Message-ID:871rbtc3j5.fsf@netris.org

Pierre Neidhardt <mail@ambrevar.xyz> writes:

Toggle quote (3 lines)

> I'm not familiar with the grafting code, so anyone who is (Mark? Ludo?)

> might be able to fix this much quicker than me! :)

I'll think about what would be required to modify our grafting code to

support UCS-4.

Mark

Ludovic Courtès wrote on 2 Apr 2021 08:13

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87im541zzg.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (8 lines)

> Ludovic Courtès <ludo@gnu.org> writes:

>> What could have been nice is if there’s a way to mark specific strings

>> as being ASCII, or if there’s a “byte vector” data type compatible with

>> strings, for instance.

> Do we know that all strings containing store references will be

> representable in ASCII?

The basename part of /gnu/store/* is guaranteed to be pure ASCII. Now,

you’re right that file names that come after could be non-ASCII, though

that’s very unlikely. We’d have to look at concrete examples I guess.

Ludo’.

Mark H Weaver wrote on 2 Apr 2021 15:46

Recipients:

Message-ID:87r1js9udv.fsf@netris.org

Here's a preliminary draft patch to add support for UTF-32 and UTF-16

references to our grafting code. I haven't yet measured the efficiency

impact of these changes, but I suspect it's not too bad.

I'd be curious to know whether it fixes the Nyxt graft.

Mark

Attachment: 0001-DRAFT-grafts-Add-support-for-UTF-16-and-UTF-32-store.patch

Pierre Neidhardt wrote on 2 Apr 2021 23:51

Recipients:

Message-ID:87sg47vp16.fsf@ambrevar.xyz

Wow, that was fast, thank you Mark!

Any idea how I can test this, i.e. how I can force a graft?

Pierre Neidhardt

https://ambrevar.xyz/

Mark H Weaver wrote on 3 Apr 2021 13:10

Recipients:

Message-ID:875z139liy.fsf@netris.org

Pierre Neidhardt <mail@ambrevar.xyz> writes:

Toggle quote (4 lines)

> Wow, that was fast, thank you Mark!

> Any idea how I can test this, i.e. how I can force a graft?

Just apply the patch to a git checkout of Guix, build it, and then use

it to build anything you like, e.g. "./pre-inst-env guix build nyxt".

With this patch applied, all graft derivations will be rebuilt, but

*only* grafts. When it's ready (i.e. when it has better comments,

docstrings, etc), this change is perfectly appropriate for 'master'.

FYI, I've already applied this new grafting code to my private branch of

Guix, switched to a system and user profile built using it, rebooted

into the new system, and I'm typing this email on it. I've also checked

that none of the processes running on my system include executables or

shared libraries from ungrafted store items (after removing my ibus

.cache files, see https://bugs.gnu.org/47576).

Mark

Ludovic Courtès wrote on 5 Apr 2021 12:45

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87ft04sefs.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (4 lines)

> Pierre Neidhardt <mail@ambrevar.xyz> writes:

>> Wow, that was fast, thank you Mark!

Yup, thumbs up!

Toggle quote (9 lines)

>> Any idea how I can test this, i.e. how I can force a graft?

> Just apply the patch to a git checkout of Guix, build it, and then use

> it to build anything you like, e.g. "./pre-inst-env guix build nyxt".

> With this patch applied, all graft derivations will be rebuilt, but

> *only* grafts. When it's ready (i.e. when it has better comments,

> docstrings, etc), this change is perfectly appropriate for 'master'.

The GC’s scanner still gets it wrong though. I wonder whether having

the grafting code more capable than the scanner could lead to bad

surprises. WDYT?

Thank you for looking into this!

Ludo’.

Mark H Weaver wrote on 5 Apr 2021 16:04

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:8735w472oo.fsf@netris.org

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (10 lines)> Mark H Weaver <mhw@netris.org> skribis:
>
>> With this patch applied, all graft derivations will be rebuilt, but
>> *only* grafts.  When it's ready (i.e. when it has better comments,
>> docstrings, etc), this change is perfectly appropriate for 'master'.
>
> The GC’s scanner still gets it wrong though.  I wonder whether having
> the grafting code more capable than the scanner could lead to bad
> surprises.  WDYT?

I've thought about it, and I've been unable to think of any

disadvantage to making the grafter more capable than the scanner.

It seems to me a pure win.

That the scanner fails to find all references is clearly an important

problem that should be fixed ASAP, but as far as I can tell, improving

the grafter would not make that problem any worse or create any new

problems.

Improving the grafter should have the following effects:

(1) Reducing the number of cases where ungrafted code with security

flaws is being used on our systems.

(2) Fixing problems in our Fish, Nyxt, and Common Lisp packages.

Improving the scanner, or adding phases to selected packages or build

systems to copy hidden references to an ASCII file, should have the

following effects:

(1) Reducing the number of cases where run-time dependencies are not

known to Guix, which could lead to dependencies being prematurely

GC'd or excluded from things like "guix pack".

So, it seems to me that we should persue both of these improvements

concurrently, and I see no practical advantage to postponing one for the

sake of rolling them both out at the same time. Of course, I welcome

other opinions on this.

What do you think?

Thanks,

Mark

Ludovic Courtès wrote on 6 Apr 2021 01:16

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87v98zomjv.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (12 lines)> That the scanner fails to find all references is clearly an important
> problem that should be fixed ASAP, but as far as I can tell, improving
> the grafter would not make that problem any worse or create any new
> problems.
>
> Improving the grafter should have the following effects:
>
> (1) Reducing the number of cases where ungrafted code with security
>     flaws is being used on our systems.
>
> (2) Fixing problems in our Fish, Nyxt, and Common Lisp packages.

These are cases where the scanner may get the references wrong in the

first place though.

OTOH, there are also cases where the scanner gets the references right.

For example, earlier Pierre mentioned that grafting breaks the reference

of nyxt to glib:

https://issues.guix.gnu.org/33848#26

It turns out that the scanner does find a reference to glib “by chance”:

Toggle snippet (6 lines)

$ guix gc --references $(guix build nyxt --no-grafts) | grep glib-2

/gnu/store/4vmhbc31cpbnlw3c96kcc094ihmaf7dv-glib-2.62.6

$ grep -r 4vmhbc31cpbnlw3c96kcc094ihmaf7dv $(guix build nyxt --no-grafts)

Duuma dosiero /gnu/store/5pgh0cn1kzyajaanj7f1iyp91hd917d6-nyxt-2-pre-release-6/bin/.nyxt-real kongruas

So in this case, the fixed grafter is a net win.

After applying the patch you sent, I confirm that Nyxt starts just fine

when running:

./pre-inst-env guix environment --ad-hoc nyxt -CN -E ^DISPLAY --share=/tmp/.X11-unix -- nyxt

… whereas on master it fails to start with:

Toggle snippet (8 lines)

Unhandled SIMPLE-ERROR in thread #<SB-THREAD:THREAD "main thread" RUNNING

{10010D0103}>:

Problem running initialization hook GLIB::RUN-INITIALIZERS:

Unable to load any of the alternatives:

("/gnu/store/4vmhbc31cpbnlw3c96kcc094ihmaf7dv-glib-2.62.6/lib/libglib-2.0.so.0"

"/gnu/store/4vmhbc31cpbnlw3c96kcc094ihmaf7dv-glib-2.62.6/lib/libglib-2.0.so")

Toggle quote (13 lines)> Improving the scanner, or adding phases to selected packages or build
> systems to copy hidden references to an ASCII file, should have the
> following effects:
>
> (1) Reducing the number of cases where run-time dependencies are not
>     known to Guix, which could lead to dependencies being prematurely
>     GC'd or excluded from things like "guix pack".
>
> So, it seems to me that we should persue both of these improvements
> concurrently, and I see no practical advantage to postponing one for the
> sake of rolling them both out at the same time.  Of course, I welcome
> other opinions on this.

As always I worry about added complexity. In this case, I think you’re

right: the UTF-{16,32}-capable grafter would most likely fix a number of

issues right away, including this Nyxt issue.

Thanks!

Ludo’.

Pierre Neidhardt wrote on 6 Apr 2021 01:23

Recipients:

Message-ID:87wntfrfd1.fsf@ambrevar.xyz

Hi Mark, hi Ludo,

Toggle quote (16 lines)> After applying the patch you sent, I confirm that Nyxt starts just fine
> when running:
>
>   ./pre-inst-env guix environment --ad-hoc nyxt -CN -E ^DISPLAY --share=/tmp/.X11-unix -- nyxt
>
> … whereas on master it fails to start with:
>
> --8<---------------cut here---------------start------------->8---
> Unhandled SIMPLE-ERROR in thread #<SB-THREAD:THREAD "main thread" RUNNING
>                                     {10010D0103}>:
>   Problem running initialization hook GLIB::RUN-INITIALIZERS:
>   Unable to load any of the alternatives:
>    ("/gnu/store/4vmhbc31cpbnlw3c96kcc094ihmaf7dv-glib-2.62.6/lib/libglib-2.0.so.0"
>     "/gnu/store/4vmhbc31cpbnlw3c96kcc094ihmaf7dv-glib-2.62.6/lib/libglib-2.0.so")
> --8<---------------cut here---------------end--------------->8---

Fantastic, this looks like it's fixing this grafting issue indeed!

I also agree this looks like a net win even though the `guix gc` issue

is not fix.

The latter seems to be relatively easier to fix, doesn't it?

Cheers!

Pierre Neidhardt

https://ambrevar.xyz/

Mark H Weaver wrote on 6 Apr 2021 04:19

Recipients:

Message-ID:87h7kj7j7x.fsf@netris.org

Here's a revised draft of the patch, which updates the comments and

refactors the code a bit to (hopefully) make it a bit more readable.

Mark

Attachment: 0001-DRAFT-grafts-Support-rewriting-UTF-16-and-UTF-32-sto.patch

Leo Famulari wrote on 6 Apr 2021 10:23

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:YGyZLj1PXbz9Pe+z@jasmine.lan

On Mon, Apr 05, 2021 at 09:45:43PM +0200, Ludovic Courtès wrote:

Toggle quote (4 lines)

> The GC’s scanner still gets it wrong though. I wonder whether having

> the grafting code more capable than the scanner could lead to bad

> surprises. WDYT?

I'm going off-topic, but I've wished we had a generic fast string-search

(and replace?) procedure.

The go-build-system has a slow "one byte a time" implementation because

I couldn't figure out how to re-use the code in (guix build grafts):

https://git.savannah.gnu.org/cgit/guix.git/tree/guix/build/go-build-system.scm?h=v1.2.0#n269

There are probably some other places we could use a fast procedure. It

might even be something to add to Guile.

Mark H Weaver wrote on 6 Apr 2021 16:21

Recipients:

Message-ID:87mtubngmi.fsf@netris.org

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

Toggle quote (8 lines)

> On Mon, Apr 05, 2021 at 09:45:43PM +0200, Ludovic Courtès wrote:

>> The GC’s scanner still gets it wrong though. I wonder whether having

>> the grafting code more capable than the scanner could lead to bad

>> surprises. WDYT?

> I'm going off-topic, but I've wished we had a generic fast string-search

> (and replace?) procedure.

Guile has several functions to help with this, e.g. 'string-contains',

'string-replace', 'string-replace-substring', and of course the regexp

functions.

The grafting code can't use these things because (1) we are not

searching for a single string, but rather for arbitrary nix hashes, and

(2) we can't easily use the regexp functions because there could be NULs

present.

Toggle quote (5 lines)

> The go-build-system has a slow "one byte a time" implementation because

> I couldn't figure out how to re-use the code in (guix build grafts):

> https://git.savannah.gnu.org/cgit/guix.git/tree/guix/build/go-build-system.scm?h=v1.2.0#n269

From a glance, I would think that 'replace-store-references' from

(guix build grafts) is directly applicable here. Do you remember what

the difficulty was in using it?

Alternatively, since you are only searching for a single string to

replace, I think you could read the entire file into a single string

(e.g. using 'get-string-all' with "ISO-8859-1" encoding) and use

'string-replace-substring'.

What do you think?

Mark

Ludovic Courtès wrote on 8 Apr 2021 03:13

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87ft01axta.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (10 lines)> From 6eec36e66d20d82fe02c6de793422875477b90d8 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <mhw@netris.org>
> Date: Fri, 2 Apr 2021 18:36:23 -0400
> Subject: [PATCH] DRAFT: grafts: Support rewriting UTF-16 and UTF-32 store
>  references.
>
> * guix/build/graft.scm (replace-store-references): Add support for
> finding and rewriting UTF-16 and UTF-32 store references.
> * tests/grafts.scm: Add tests.

Please add a “Fixes” line in the commit log.

I’m not reviewing the code in depth and I trust your judgment.

The risks of bugs I can think of are: missed ASCII references (a

regression, whereby some ASCII references would not get rewritten), and

unrelated UTF-{16,32}-base32-looking references getting rewritten.

I guess the latter is very unlikely because only sequences found in the

replacement table may be rewritten, right?

The former should be caught by ‘tests/grafts.scm’ but we could also

check the closure of real-world systems, for instance, to make sure it

doesn’t refer to ungrafted things.

Do you know how this affects performance?

Some superficial comments:

Toggle quote (18 lines)> +(define (possible-utf16-hash? buffer i w)
> +  (and (<= (* 2 hash-length) (- i w))
> +       (let loop ((j (+ 1 (- i (* 2 hash-length)))))
> +         (or (>= j i)
> +             (and (zero? (bytevector-u8-ref buffer j))
> +                  (loop (+ j 2)))))))
> +
> +(define (possible-utf32-hash? buffer i w)
> +  (and (<= (* 4 hash-length) (- i w))
> +       (let loop ((j (+ 1 (- i (* 4 hash-length)))))
> +         (or (>= j i)
> +             (and (zero? (bytevector-u8-ref buffer j))
> +                  (zero? (bytevector-u8-ref buffer (+ j 1)))
> +                  (zero? (bytevector-u8-ref buffer (+ j 2)))
> +                  (loop (+ j 4)))))))
> +
> +(define (insert-nuls char-size bv)

Perhaps add short docstrings for clarity.

Toggle quote (62 lines)> +(for-each
> + (lambda (char-size1)
> +   (for-each
> +    (lambda (char-size2)
> +      (for-each
> +       (lambda (gap)
> +        (for-each
> +         (lambda (offset)
> +           (test-equal (format #f "replace-store-references, char-sizes ~a ~a, gap ~s, offset ~a"
> +                               char-size1 char-size2 gap offset)
> +             (string-append (make-string offset #\=)
> +                            (nul-expand (string-append "/gnu/store/"
> +                                                       (make-string 32 #\6)
> +                                                       "-BlahBlaH")
> +                                        char-size1)
> +                            gap
> +                            (nul-expand (string-append "/gnu/store/"
> +                                                       (make-string 32 #\8)
> +                                                       "-SoMeTHiNG")
> +                                        char-size2)
> +                            (list->string (map integer->char (iota 77 33))))
> +
> +             ;; Create input data where the right-hand-size of the dash ("-something"
> +             ;; here) goes beyond the end of the internal buffer of
> +             ;; 'replace-store-references'.
> +             (let* ((content     (string-append (make-string offset #\=)
> +                                                (nul-expand (string-append "/gnu/store/"
> +                                                                           (make-string 32 #\5)
> +                                                                           "-blahblah")
> +                                                            char-size1)
> +                                                gap
> +                                                (nul-expand (string-append "/gnu/store/"
> +                                                                           (make-string 32 #\7)
> +                                                                           "-something")
> +                                                            char-size2)
> +                                                (list->string
> +                                                 (map integer->char (iota 77 33)))))
> +                    (replacement (alist->vhash
> +                                  `((,(make-string 32 #\5)
> +                                     . ,(string->utf8 (string-append
> +                                                       (make-string 32 #\6)
> +                                                       "-BlahBlaH")))
> +                                    (,(make-string 32 #\7)
> +                                     . ,(string->utf8 (string-append
> +                                                       (make-string 32 #\8)
> +                                                       "-SoMeTHiNG")))))))
> +               (call-with-output-string
> +                 (lambda (output)
> +                   ((@@ (guix build graft) replace-store-references)
> +                    (open-input-string content) output
> +                    replacement
> +                    "/gnu/store"))))))
> +         ;; offsets to test
> +         (map (lambda (i) (- buffer-size (* 40 char-size1) i))
> +              (iota 30))))
> +       ;; gaps
> +       '("" "-" " " "a")))
> +    ;; char-size2 values to test
> +    '(1 2)))
> + ;; char-size1 values to test
> + '(1 2 4))

For clarity, perhaps you can define a top-level procedure for the test

and call it from ‘for-each’.

Modulo these very minor issues, it looks like it’s ready to go!

Thank you,

Ludo’.

Mark H Weaver wrote on 13 Apr 2021 13:06

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87k0p6rlt5.fsf@netris.org

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (2 lines)

> Please add a “Fixes” line in the commit log.

Done, thanks.

Toggle quote (5 lines)

> I’m not reviewing the code in depth and I trust your judgment.

> The risks of bugs I can think of are: missed ASCII references (a

> regression, whereby some ASCII references would not get rewritten),

This is indeed a risk whenever we modify the grafting code, which is

written for efficiency, not clarity. I've tried to be careful, and have

checked that my newly grafted system and user profiles do not retain

references to ungrafted code, modulo the following pre-existing issues:

https://bugs.gnu.org/47576

(ibus-daemon launches ungrafted subprocesses)

https://bugs.gnu.org/47614

(Chunked store references in .zo files in Racket 8)

Toggle quote (5 lines)

> and unrelated UTF-{16,32}-base32-looking references getting rewritten.

> I guess the latter is very unlikely because only sequences found in the

> replacement table may be rewritten, right?

Yes, that's correct.

Toggle quote (4 lines)

> The former should be caught by ‘tests/grafts.scm’ but we could also

> check the closure of real-world systems, for instance, to make sure it

> doesn’t refer to ungrafted things.

As I mention above, I've already done so for my own GNOME-based Guix

system.

Toggle quote (2 lines)

> Do you know how this affects performance?

I have not yet measured it, but subjectively, I do not notice any

obvious difference in speed.

I expect that the main performance impact is that large blocks of NULs

will be processed more slowly by the current draft version of the new

grafting code. That's because NULs can now be part of a nix hash, and

therefore the new grafting code can only advance 1 byte position when

seeing a NUL, whereas previously it would skip ahead 33 positions in

that case.

If desired, the handling of NULs could be made more efficient, at the

cost of a bit more complexity. When seeing a NUL, we could check the

adjacent bytes to see if this is part of a nix-base32 character in

UTF-16 or UTF-32 encoding. If not, we could skip ahead.

Toggle quote (2 lines)

> Perhaps add short docstrings for clarity.

Done.

Toggle quote (10 lines)>> +(for-each
>> + (lambda (char-size1)
>> +   (for-each
>> +    (lambda (char-size2)
>> +      (for-each
>> +       (lambda (gap)
>> +        (for-each
>> +         (lambda (offset)
>> +           (test-equal (format #f "replace-store-references, char-sizes ~a ~a, gap ~s, offset ~a"
>> +                               char-size1 char-size2 gap offset)

[...]

Toggle quote (13 lines)>> +         ;; offsets to test
>> +         (map (lambda (i) (- buffer-size (* 40 char-size1) i))
>> +              (iota 30))))
>> +       ;; gaps
>> +       '("" "-" " " "a")))
>> +    ;; char-size2 values to test
>> +    '(1 2)))
>> + ;; char-size1 values to test
>> + '(1 2 4))
>
> For clarity, perhaps you can define a top-level procedure for the test
> and call it from ‘for-each’.

Good idea. I'd like to optimize the tests a bit before pushing this.

They take a fairly long time to run, and lead to a huge 1.5G grafts.log

file. It might be sufficient to avoid 'test-equal' here.

Toggle quote (2 lines)

> Modulo these very minor issues, it looks like it’s ready to go!

Sounds good. Thanks for the review!

Below, I've attached my current revision of this draft patch, which

incorporates your suggestions regarding the main code. What remains is

to improve the tests.

Regards,

Mark

Attachment: 0001-DRAFT-grafts-Support-rewriting-UTF-16-and-UTF-32-sto.patch

Ludovic Courtès wrote on 14 Apr 2021 03:55

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:87eefdf840.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (14 lines)>> The risks of bugs I can think of are: missed ASCII references (a
>> regression, whereby some ASCII references would not get rewritten),
>
> This is indeed a risk whenever we modify the grafting code, which is
> written for efficiency, not clarity.  I've tried to be careful, and have
> checked that my newly grafted system and user profiles do not retain
> references to ungrafted code, modulo the following pre-existing issues:
>
>   <https://bugs.gnu.org/47576>
>   (ibus-daemon launches ungrafted subprocesses)
>
>   <https://bugs.gnu.org/47614>
>   (Chunked store references in .zo files in Racket 8)

OK.

Toggle quote (14 lines)>> and unrelated UTF-{16,32}-base32-looking references getting rewritten.
>>
>> I guess the latter is very unlikely because only sequences found in the
>> replacement table may be rewritten, right?
>
> Yes, that's correct.
>
>> The former should be caught by ‘tests/grafts.scm’ but we could also
>> check the closure of real-world systems, for instance, to make sure it
>> doesn’t refer to ungrafted things.
>
> As I mention above, I've already done so for my own GNOME-based Guix
> system.

Yes, we should be safe.

Toggle quote (17 lines)>> Do you know how this affects performance?
>
> I have not yet measured it, but subjectively, I do not notice any
> obvious difference in speed.
>
> I expect that the main performance impact is that large blocks of NULs
> will be processed more slowly by the current draft version of the new
> grafting code.  That's because NULs can now be part of a nix hash, and
> therefore the new grafting code can only advance 1 byte position when
> seeing a NUL, whereas previously it would skip ahead 33 positions in
> that case.
>
> If desired, the handling of NULs could be made more efficient, at the
> cost of a bit more complexity.  When seeing a NUL, we could check the
> adjacent bytes to see if this is part of a nix-base32 character in
> UTF-16 or UTF-32 encoding.  If not, we could skip ahead.

Let’s keep it this way for now; we can always revisit later if we feel

the need for it.

Toggle quote (7 lines)

>> For clarity, perhaps you can define a top-level procedure for the test

>> and call it from ‘for-each’.

> Good idea. I'd like to optimize the tests a bit before pushing this.

> They take a fairly long time to run, and lead to a huge 1.5G grafts.log

> file. It might be sufficient to avoid 'test-equal' here.

Ah yes, rather use ‘test-assert’ or some such to avoid the huge log

file. :-)

Toggle quote (4 lines)

> Below, I've attached my current revision of this draft patch, which

> incorporates your suggestions regarding the main code. What remains is

> to improve the tests.

LGTM! Feel free to push this version or an improved one. I think it’s

good to have it in the upcoming release, and if it’s pushed sooner,

we’ll have more time to react in case something’s wrong.

Thank you!

Ludo’.

Ludovic Courtès wrote on 14 Apr 2021 08:19

control message for bug #47297

Recipients:(address . control@debbugs.gnu.org)

Message-ID:871rbcgagq.fsf@gnu.org

block 47297 by 33848

quit

Leo Famulari wrote on 14 Apr 2021 15:37

Re: bug#33848: Store references in SBCL-compiled code are "invisible"

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:YHduuR+fX4/nRI4Z@jasmine.lan

On Wed, Apr 14, 2021 at 12:55:43PM +0200, Ludovic Courtès wrote:

Toggle quote (4 lines)

> LGTM! Feel free to push this version or an improved one. I think it’s

> good to have it in the upcoming release, and if it’s pushed sooner,

> we’ll have more time to react in case something’s wrong.

Just FYI to everyone reading, we aim to release 1.2.1 this Sunday, April

18.

Mark H Weaver wrote on 15 Apr 2021 00:26

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:871rbcj9dn.fsf@netris.org

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (4 lines)

> LGTM! Feel free to push this version or an improved one. I think it’s

> good to have it in the upcoming release, and if it’s pushed sooner,

> we’ll have more time to react in case something’s wrong.

I pushed an improved version of the patch to 'master' as commit

1bab9b9f17256a9e4f45f5b0cceb8b52e0a1b1ed.

Thanks,

Mark

Pierre Neidhardt wrote on 16 Apr 2021 02:44

Recipients:

Message-ID:87v98mva15.fsf@ambrevar.xyz

Just tested like Ludo did: Nyxt works now without the --no-grafts

option!

Thank you so much, Mark, this is a huge step forward for Nyxt (and Guix

for course)! :)

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 22 Apr 2021 06:36

control message for bug #33848

Recipients:(address . control@debbugs.gnu.org)

Message-ID:877dku1lx7.fsf@gnu.org

tags 33848 fixed
close 33848 
quit

Mark H Weaver wrote on 30 Apr 2021 13:03

Re: bug#33848: Store references in SBCL-compiled code are "invisible"

Recipients:

Message-ID:878s4zo7z2.fsf@netris.org

Hi Pierre, and Ludovic,

Pierre Neidhardt <mail@ambrevar.xyz> writes:

Toggle quote (4 lines)

> I also agree this looks like a net win even though the `guix gc` issue

> is not fix.

> The latter seems to be relatively easier to fix, doesn't it?

Yes. I now have a plan to finally fix this bug for good.

I intend to write a scanner in Scheme that is able to find Nix hashes

encoded in ASCII, UTF-16, or UTF-32. Using that, I'll write a procedure

that, for each package output, finds all store references that are found

encoded in UTF-16 or UTF-32 but never in ASCII, and write those

references to a file (if that set is nonempty). This procedure can then

be used by selected packages and/or build-systems.

However, there's one thing I will need: the set of all transitive inputs

(and native-inputs, including implicit inputs) available to the build,

i.e. the set of possible hashes that might legitimately be found by the

scanner.

Ludovic: what's the best way to get that list from the build-side code?

Thanks,

Mark

Disinformation flourishes because many people care deeply about injustice

but very few check the facts. Ask me about https://stallmansupport.org.

Pierre Neidhardt wrote on 1 May 2021 02:22

Recipients:

Message-ID:87fsz6st9w.fsf@ambrevar.xyz

Hi Mark,

thanks for the update and thanks for getting your hands dirty, this is a

very valuable contribution to the core of Guix!

Cheers!

Pierre Neidhardt

https://ambrevar.xyz/

Ludovic Courtès wrote on 11 May 2021 01:46

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:878s4l4q0i.fsf@gnu.org

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

Toggle quote (14 lines)> I intend to write a scanner in Scheme that is able to find Nix hashes
> encoded in ASCII, UTF-16, or UTF-32.  Using that, I'll write a procedure
> that, for each package output, finds all store references that are found
> encoded in UTF-16 or UTF-32 but never in ASCII, and write those
> references to a file (if that set is nonempty).  This procedure can then
> be used by selected packages and/or build-systems.
>
> However, there's one thing I will need: the set of all transitive inputs
> (and native-inputs, including implicit inputs) available to the build,
> i.e. the set of possible hashes that might legitimately be found by the
> scanner.
>
> Ludovic: what's the best way to get that list from the build-side code?

You can use #:references-graphs for that.

Sorry for the delay!

Ludo’.

Your comment

This issue is archived.

To comment on this conversation send an email to 33848@patchwise.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 33848

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date