Valgrind: PPC sync instruction L field should only be 2 bits in ISA 3.0

[bugproxy]

---Problem Description---
Valgrind, including version 3.16, mishandles the L field of the sync instruction.
More details are available at: https://bugs.kde.org/show_bug.cgi?id=422677

This is a request to backport the following Valgrind patch from upstream:

commit fb6f7abcbc92506d302fb18a2c5fc853d2929248
Author: Carl Love <email address hidden>
Date: Tue Jun 9 10:42:03 2020 -0500

    Power PC Fix extraction of the L field for sync instruction

    The L field is currently a two bit[22:21] field in ISA 3.0. The size of the
    L field has changed over time.

    Currently the ISA 3.0 Valgrind sync instruction support code sets the
    flag_L for the instruction L field to a five bit value that includes bits
    that are marked reserved the sync instruction. This patch fixes the issue for ISA 3.0
    to only setting flag_L the specified two bits.

    Valgrind bugzilla: https://bugs.kde.org/show_bug.cgi?id=422677

Machine Type = Reproducible on all POWER8 and POWER9 servers

---Steps to Reproduce---
 $ cat test-sync.c
#define __SYNC(l) ".long (0x7c0004AC | ((" #l ") << 21))"

int
main ()
{
        asm volatile(__SYNC(4) : : : "memory");
}
$ gcc test-sync.c && valgrind --tool=none ./a.out
==150073== Nulgrind, the minimal Valgrind tool
==150073== Copyright (C) 2002-2017, and GNU GPL'd, by Nicholas Nethercote.
==150073== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==150073== Command: ./a.out
==150073==
dis_memsync(ppc)(sync/lwsync,flag_L)
disInstr(ppc): unhandled instruction: 0x7C8004AC
                 primary 31(0x1F), secondary 1196(0x4AC)
==150073== valgrind: Unrecognised instruction at address 0x180788.
==150073== at 0x180788: main (in /home/tuliom/tmp/a.out)
==150073== Your program just tried to execute an instruction that Valgrind
==150073== did not recognise.
...

Userspace tool common name: Valgrind

The userspace tool has the following bit modes: 64-bit

Userspace deb: valgrind

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Frank Heimes (fheimes)]

The bug description says that the fix mentioned there is needed to close a bug in valgrind 3.16:
“Valgrind, including version 3.16, mishandles the L field of the sync instruction.”

But we are now already on 3.16.1 in groovy:
“valgrind | 1:3.16.1-1ubuntu1 | groovy | s390x”
And according to the release notes of valgrind 3.16.1:
https://www.valgrind.org/docs/manual/dist.news.html
bug ‘422677 PPC sync instruction L field should only be 2 bits in ISA 3.0’
got fixed.
Hence I’m closing this ticket as Fix Released.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2020-07-14 10:48 EDT-------
> But we are now already on 3.16.1 in groovy:
> ?valgrind | 1:3.16.1-1ubuntu1 | groovy | s390x?
> ...
> Hence I?m closing this ticket as Fix Released.

focal is still on 3.15. Are there any plans to update Valgrind on focal to v3.16.1 too?

[Andrew Cloke (andrew-cloke)]

> focal is still on 3.15. Are there any plans to update Valgrind on focal to v3.16.1 too?

There is a policy not to do wholesale version bumps on packages once released, so "no" there are no plans to update valgrind on focal to v3.16.1.

However, if there are specific patchsets that address critical issues, it may be possible to use the "SRU" process to apply these to the focal release.

This bug is currently marked as "Fix Released", so if there is a patchset that should be considered for inclusion in focal, would it be possible to raise a new bug?

Thanks.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2020-07-16 16:02 EDT-------
Hi Andrew and Frank,

Can you let us know if the fix will be released in 20.04 as well or would you rather we submit a new bug requesting this same fix in 20.04? Thanks.

[Frank Heimes (fheimes)]

Hi Luciano,
since this ticket was opened without specifying a certain Ubuntu release, it was assumed that it got opened for the Ubuntu release that is currently in development - which is in this case 20.10 / groovy.
And since groovy includes valgrind 3.16.1, that already incl. the fix mentioned above, this ticket was closed as Fix Released.

However, if this fix needs to land in 20.04 / focal's valgrind 3.15 too:
 valgrind | 1:3.15.0-1ubuntu9 | focal | ppc64el
it can be done based on Ubuntu's stable release upgrade (SRU) process (https://wiki.ubuntu.com/StableReleaseUpdates).
This process applies to Ubuntu releases that have been completed and were already published, and is therefore quite strict to avoid regressions.
Therefore we cannot simply bump the valgrind version in focal to the latest, but we may cherry-pick one or more patches/commits to fix a certain problem/bug.

Means if the above fix (fb6f7abcbc92506d302fb18a2c5fc853d2929248) can be cherry-picked to 3.15 and applies there cleanly, we may do that. If it doesn't apply cleanly a proper backport to 3.15 would be needed.

So yes, we would like to ask you to open a new bug requesting this same fix in 20.04 (since this ticket got already closed) - if you don't mind ...
(If this bug wouldn't have been reached the 'Fix Release' status yet, it could have simply marked as affecting groovy _and_ focal.)

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2020-07-17 10:42 EDT-------
Hi Frank,

We've opened LP 1887974 for 20.04 and while we are it can we include the fix in 18.04?

This bug should have been more clearer on target release(s). However, for the record the bridge does create a tag in the LP bug that starts with "targetmilestone-inin" followed by the target release 1804, 2004, etc.

Thanks for the help!

[Frank Heimes (fheimes)]

np, Luciano, I just added bionic as affected by this on the new bug, too:
LP 1887974
https://bugs.launchpad.net/ubuntu-power-systems/+bug/1887974

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2020-07-17 11:58 EDT-------
Thanks!