Valgrind utility is missing bug fixes since 3.13.0 release

[bugproxy]

[Impact]

 * Valgrind is unable to execute certain binaries, or executes them incorrectly on ppc64el platform in bionic.

[Test Case]

 * Compile the following test cases, they should execute correctly normally and under valgrind; without illegal opcodes and returning the same output lengths for timebase.

int main()
{
asm ("mtspr 3,0");
}

int main()
{
asm ("xvnegsp 33,34");
}

#include <stdio.h>
#include <sys/platform/ppc.h>

int main(int argc, char** argv)
{
uint64_t tb = __ppc_get_timebase();
printf("timebase = %lx\n", tb);
sleep(15);
tb = __ppc_get_timebase();
printf("timebase = %lx\n", tb);

return 0;
}

[Regression Potential]

 * These are upstream included and tested patches, cherrypicked into bionic, affect power code-paths only, and tested/released in cosmic.

[Other Info]

 * Original bug report.

---Problem Description---
Valgrind is missing the PPC64 bug fixes and fixes for missing support since the Valgrind 3.13.0 release in June of 2017. This is causing users trying to use Valgrind to get internal Valgrind errors. This bugzilla is to get the latest PPC64 patches added to the Ubuntu 18.04 release.

---uname output---
Linux genoa 4.4.0-130-generic #156-Ubuntu

Machine Type = Power 9

---Steps to Reproduce---
 Valgrind ./user_application
Where the user_application includes vpermr instructions

Need to have this bugzilla mirred to the Ubuntu Launchpad so we can get the right people from Ubuntu to discuss how to address getting the needed updates into the long term release support for 18.04.

[bugproxy (bugproxy)]

Default Comment by Bridge

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-07-13 10:45 EDT-------
The following is from the Valgrind mailing list on July 13 with regards to the next release from the maintainer.

I propose the following:

* Up to 23 Aug: fix as many bugs as we can
* Fri 23 Aug: code freeze. After this, only "important" bug fixes to land
* Mon 10 Sept: release

At an absolute minimum I would like to have the s390/z13 fixes in, and the
current PT_LOAD problem(s) resolved by then. Plus of course as many other
fixes as we have time for. I will create a docs/internals/3_13_BUGSTATUS.txt
file in the next day or so, so as to get a top-level overview of the bug
situation.

J

[Launchpad Janitor (janitor)]

This bug was fixed in the package valgrind - 1:3.13.0-2ubuntu4

---------------
valgrind (1:3.13.0-2ubuntu4) cosmic; urgency=medium

  * Apply post 3.13 PPC64 related patches. LP: #1781128.

 -- Matthias Klose <email address hidden> Thu, 12 Jul 2018 09:40:44 +0200

[Dimitri John Ledkov (xnox)]

For the SRU we will need a test case. Is there a binary in the Ubuntu Bionic archive that already uses the relevant instructions?

"
Valgrind ./user_application
Where the user_application includes vpermr instructions
"

Is there bionic application that already does it? e.g. would something like numpy use that instruction?

[Dimitri John Ledkov (xnox)]

Is there an example affected app please?

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-08-23 15:50 EDT-------
We had various users reporting issues about instructions in their application were not supported by valgrind. The users were able to isolate the failing instructions and provided the following simple cases:

int main()
{
asm ("mtspr 3,0");
}

int main()
{
asm ("xvnegsp 33,34");
}

There was another user who's application was trying to read the time base register. Unfortunately, valgrind only returned 32-bits not the required 64-bits. The simple test case is:

#include <stdio.h>
#include <sys/platform/ppc.h>

int main(int argc, char** argv)
{
uint64_t tb = __ppc_get_timebase();
printf("timebase = %lx\n", tb);
sleep(15);
tb = __ppc_get_timebase();
printf("timebase = %lx\n", tb);

return 0;
}

[Brian Murray (brian-murray)]

Hello bugproxy, or anyone else affected,

Accepted valgrind into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/valgrind/1:3.13.0-2ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Dimitri John Ledkov (xnox)]

Reproduced the issue with valgrind 1:3.13.0-2ubuntu2, two core dumps and short timebase output (instead of long one when executed not under valgrind).

Upgrading to 1:3.13.0-2ubuntu2.1 first and third test case now pass. Thus it is an improvement.

The following test case is still generating Illegal opcode. So something is still missing to support xvnegsp instruction in valgrind in bionic.
int main()
{
asm ("xvnegsp 33,34");
}

I shall open a new issue for xvnegsp but imho this SRU should be released, as it does fix the mtspr and the __ppc_get_timebase support.

[Robie Basak (racb)]

Could someone please check that valgrind still works correctly on non-power? I see that a dep8 test checks /bin/true, but that comes out as:

==1276== HEAP SUMMARY:
==1276== in use at exit: 0 bytes in 0 blocks
==1276== total heap usage: 0 allocs, 0 frees, 0 bytes allocated

so possibly doesn't actually exercise most of valgrind?

[Robie Basak (racb)]

> I shall open a new issue for xvnegsp but imho this SRU should be released, as it does fix the mtspr and the __ppc_get_timebase support.

Does this SRU include an attempt to fix the second test case, or is just that the patch that fixes it was missed?

[Dimitri John Ledkov (xnox)]

I expect /bin/true to use to heap. As it should be effectively `int main(){return 0;}`.
Valgrind in general does work, with heap allocating apps.

I'm not sure yet what's up with xvnegsp. As i don't see anything explicit to either fix it, or break it, or otherwise missing to support it.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-10-31 14:12 EDT-------
(In reply to comment #15)
> I expect /bin/true to use to heap. As it should be effectively `int
> main(){return 0;}`.
> Valgrind in general does work, with heap allocating apps.
>
> I'm not sure yet what's up with xvnegsp. As i don't see anything explicit to
> either fix it, or break it, or otherwise missing to support it.

Digging for this one a bit... xvnegsp should have been fixed with the upstream commit as referenced in the kde bguzilla:
https://bugs.kde.org/show_bug.cgi?id=395709
(june 21 2018, with some testcase updates occurring a few days later.)

Should have been contained in the changes provided with this bugz (July 2018).

Carl?

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-10-31 17:07 EDT-------
Will:

I don't seem to have an old Valgrind tree around that used the svn revision control to examine.

But according to my notes, the functional fix was committed in svn revision 395709 on June 21, 2018. The functional fix adds the support for the instruction to Valgrind and the support for the instruction to the functional test test_isa_2_06_part3.c to the test suite. However, the expected result file changes didn't get included in that commit.

It looks like Valgrind switched to the Git repository right after that. I see the expected results for the test case were committed in the git repository as:

commit 1f69ed86e994ee152a0e6e3ee7031d0105f02a7e
Author: Carl Love <email address hidden>
Date: Mon Jun 25 16:24:14 2018 -0500
Fix ppc32 results for test_isa_2_06_part3.c.
The ppc32 results were not updated when the xvnegsp instruction support
was added. Add the xvnegsp 32-bit results to
ppc/test_isa_2_06_part3.stdout.exp.

So, all of the functional Valgrind and test case changes went in on the first commit. Hopefully that clears up your question?

Carl Love

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-11-01 09:52 EDT-------
(In reply to comment #15)
> I expect /bin/true to use to heap. As it should be effectively `int
> main(){return 0;}`.
> Valgrind in general does work, with heap allocating apps.
>
> I'm not sure yet what's up with xvnegsp. As i don't see anything explicit to
> either fix it, or break it, or otherwise missing to support it.

Looking through the tarball that was provided via this bugzilla, it looks to me like the xvnegsp fix was not included.

The functional fix for xvnegsp would need to be pulled in. this is upstream commit:
commit 9c5d762904862db0d90fb0142ac1b12d5647f607
Author: Carl Love <email address hidden>
Date: Thu Jun 21 17:27:40 2018 -0500
PPC64, add support for the xvnegsp instruction. Add test case for the instruction.

Also recommend the testcase output checking changes, which is upstream commit:
commit 1f69ed86e994ee152a0e6e3ee7031d0105f02a7e
Author: Carl Love <email address hidden>
Date: Mon Jun 25 16:24:14 2018 -0500
Fix ppc32 results for test_isa_2_06_part3.c.

[Dimitri John Ledkov (xnox)]

I did retest of the xvnegsp test case.
It's broken in bionic, bionic-proposed, cosmic, and disco.
Disco-proposed has updated valgrind, but it FTBFS.

So to answer robbies question - no, this SRU was never meant to fix the second testcase as that is still broken everywhere. But this SRU did meant to, and has fixed the first and third testcase.

I think from here we should release bionic SRU; work on upgrading valgrind in disco; and then cherrypick the above identified things into cosmic/bionic to fixup valgrinding xvnegsp instruction.

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for valgrind has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package valgrind - 1:3.13.0-2ubuntu2.1

---------------
valgrind (1:3.13.0-2ubuntu2.1) bionic; urgency=medium

  [ Matthias Klose ]
  * Apply post 3.13 PPC64 related patches. LP: #1781128.

 -- Dimitri John Ledkov <email address hidden> Mon, 22 Oct 2018 13:43:55 +0100