eq? problem at -O2 since Guile 3.0.5

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Marius Bakke
  • Andy Wingo
Owner
unassigned
Submitted by
Marius Bakke
Severity
normal
M
M
Marius Bakke wrote on 11 May 2021 22:49
(address . bug-guile@gnu.org)
87pmxxhu7k.fsf@gnu.org
Hi!

I haven't been able to make a reproducer for this, but for illustrative
purposes, here is a code snippet that fails with -O2 on Guile 3.0.5 and
later (excerpt from GNU Shepherd):

(define (run-command socket-file action service args)
"Perform ACTION with ARGS on SERVICE, and display the result. Connect to
the daemon via SOCKET-FILE."
(with-system-error-handling
(let ((sock (open-connection socket-file))
(action* (if (and (eq? action 'detailed-status)
(memq service '(root shepherd)))
'status
action)))
;; Send the command.
(write-command (shepherd-command action* service #:arguments args)
sock)


At -O2, (eq? action 'detailed-status) evaluates to true no matter what
symbol ACTION holds.

Interestingly, adding (pk action*) between LET and WRITE-COMMAND gives
the expected result and mitigates the problem(!).

There are other issues that can be observed by running the Shepherd test
suite (i.e. make -j4 check). With -O1 all pass.
-----BEGIN PGP SIGNATURE-----

iIUEARYKAC0WIQRNTknu3zbaMQ2ddzTocYulkRQQdwUCYJrtzw8cbWFyaXVzQGdu
dS5vcmcACgkQ6HGLpZEUEHe4hQEAknNS0PVUkTkz0Wy3x/S3Qu50FowvKcMDxmKx
nsISOG8BALKLfG7ywwYVOnuC73czgpEaLT3EMzjBdGHnh7Tu6wEO
=4y51
-----END PGP SIGNATURE-----

M
M
Marius Bakke wrote on 23 May 2021 17:23
Re: bug#47172: Shepherd 0.8.1 tests fail on core-updates
(address . 48368@debbugs.gnu.org)
875yz9qxrv.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skriver:

Toggle quote (36 lines)
> Ludovic Courtès <ludo@gnu.org> skribis:
>
>> This turns out to be due to a… miscompilation bug.
>>
>> In (shepherd scripts herd), ‘run-command’ has this code:
>>
>> (let ((sock (open-connection socket-file))
>> (action* (if (and (eq? action 'detailed-status)
>> (memq service '(root shepherd)))
>> 'status
>> action)))
>> …)
>>
>> Problem is that everything works as if (eq? action 'detailed-status)
>> was omitted, such that ‘herd stop root’ is interpreted as ‘herd status
>> root’.
>
> A workaround that works with 3.0.7 is swapping the two ‘and’
> sub-expressions:
>
> diff --git a/modules/shepherd/scripts/herd.scm b/modules/shepherd/scripts/herd.scm
> index 106de1e..39d2e34 100644
> --- a/modules/shepherd/scripts/herd.scm
> +++ b/modules/shepherd/scripts/herd.scm
> @@ -126,8 +126,8 @@ of pairs."
> the daemon via SOCKET-FILE."
> (with-system-error-handling
> (let ((sock (open-connection socket-file))
> - (action* (if (and (eq? action 'detailed-status)
> - (memq service '(root shepherd)))
> + (action* (if (and (memq service '(root shepherd))
> + (eq? action 'detailed-status))
> 'status
> action)))
> ;; Send the command.

Cc'ing the relevant Guile bug:


See also commit 79be6a985799adc6d663890250f4fb7c12f015b4 on
'core-updates' that builds with -O1 as a less satisfactory workaround.
-----BEGIN PGP SIGNATURE-----

iIUEARYKAC0WIQRNTknu3zbaMQ2ddzTocYulkRQQdwUCYKpzhA8cbWFyaXVzQGdu
dS5vcmcACgkQ6HGLpZEUEHd0CgD9FsWiNMu2PxB/773BI2hOmYPKZqyX+KbAy05R
C7+xubIBAPcyjBy9TtmqfG0aCSUu1r6a8dmFKkJm4r4eb5fLEwEK
=o7Sr
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 23 May 2021 23:43
(name . Marius Bakke)(address . marius@gnu.org)
87lf853z4i.fsf@gnu.org
Hello,

Marius Bakke <marius@gnu.org> skribis:

Toggle quote (2 lines)
> Ludovic Courtès <ludo@gnu.org> skriver:

[...]

Toggle quote (23 lines)
>> A workaround that works with 3.0.7 is swapping the two ‘and’
>> sub-expressions:
>>
>> diff --git a/modules/shepherd/scripts/herd.scm b/modules/shepherd/scripts/herd.scm
>> index 106de1e..39d2e34 100644
>> --- a/modules/shepherd/scripts/herd.scm
>> +++ b/modules/shepherd/scripts/herd.scm
>> @@ -126,8 +126,8 @@ of pairs."
>> the daemon via SOCKET-FILE."
>> (with-system-error-handling
>> (let ((sock (open-connection socket-file))
>> - (action* (if (and (eq? action 'detailed-status)
>> - (memq service '(root shepherd)))
>> + (action* (if (and (memq service '(root shepherd))
>> + (eq? action 'detailed-status))
>> 'status
>> action)))
>> ;; Send the command.
>
> Cc'ing the relevant Guile bug:
>
> https://bugs.gnu.org/48368

Oh nice! (It would have saved me a bit of time to catch up on email
beforehand. :-))

Toggle quote (3 lines)
> See also commit 79be6a985799adc6d663890250f4fb7c12f015b4 on
> 'core-updates' that builds with -O1 as a less satisfactory workaround.

I found that ‘-O2 -Ono-resolve-primitives’ also does the trick.

If we manually replace ‘memq’ by two ‘eq?’ tests (which is what the
compiler does), the same problem is exhibited:
Toggle diff (14 lines)
diff --git a/modules/shepherd/scripts/herd.scm b/modules/shepherd/scripts/herd.scm
index 106de1e..513508f 100644
--- a/modules/shepherd/scripts/herd.scm
+++ b/modules/shepherd/scripts/herd.scm
@@ -127,7 +127,8 @@ the daemon via SOCKET-FILE."
(with-system-error-handling
(let ((sock (open-connection socket-file))
(action* (if (and (eq? action 'detailed-status)
- (memq service '(root shepherd)))
+ (or (eq? service 'root)
+ (eq? service 'shepherd)))
'status
action)))
;; Send the command.
‘-Ono-resolve-primitives’ also helps in this case.

‘-Ono-optimize-branch-chains’ has no effect.

So, not much progress, but at least we have a workaround.

Thanks,
Ludo’.
A
A
Andy Wingo wrote on 24 May 2021 10:14
reduced test case
(address . 48368@debbugs.gnu.org)
87lf841rb5.fsf@igalia.com
Test case:

(define (f a b)
(let ((c (if (and (eq? a 'foo)
(eq? b 'bar))
'ERROR
a)))
(pk c)))

If you run as (f 'not-foo 'bar), you get 'ERROR. Yeeps!
A
A
Andy Wingo wrote on 24 May 2021 14:19
thanks
(address . 48368-done@debbugs.gnu.org)
87cztg1fzp.fsf@igalia.com
Fixed in 17aab66e75136cf23c7f0d4942b61d6947f98f9b. Thanks for the
report :)
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 48368@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 48368
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