Bug 449672 - ppc64 --track-origins=yes causes failures because of bad cmov addHRegUse
Summary: ppc64 --track-origins=yes causes failures because of bad cmov addHRegUse
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (other bugs)
Version First Reported In: 3.18.1
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-05 23:02 UTC by Mark Wielaard
Modified: 2022-02-08 15:45 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2022-02-05 23:02:26 UTC
We had an issue with some code running under memcheck with --track-origins=yes which caused a crash, it ran fine without any issues under memcheck without --track-origins=yes.

Unfortunately the issue was a nontrivial test for an security issue (which I cannot disclose as is) and it only happens deep inside glibc and only when glibc is build with GCC 11.2.1.

Julian suggested to look at the Pin_CMov reg usage and we found this:

diff --git a/VEX/priv/host_ppc_defs.c b/VEX/priv/host_ppc_defs.c
index 3ae0f6e08..4222b4786 100644
--- a/VEX/priv/host_ppc_defs.c
+++ b/VEX/priv/host_ppc_defs.c
@@ -2590,7 +2590,7 @@ void getRegUsage_PPCInstr ( HRegUsage* u, const PPCInstr* i, Bool mode64 )
       return;
    case Pin_CMov:
       addRegUsage_PPCRI(u,  i->Pin.CMov.src);
-      addHRegUse(u, HRmWrite, i->Pin.CMov.dst);
+      addHRegUse(u, HRmModify, i->Pin.CMov.dst);
       return;
    case Pin_Load:
       addRegUsage_PPCAMode(u, i->Pin.Load.src);

And that resolved it.

Since this is a conditional move the register could be both read and written (read + write = modify). This matches the dst of Pin_FpCMov and Pin_AvCMov.

This is slightly amazing, this code is from 2005 and as far as I know we never seen an issue with --track-origins=yes on power before. And I have been unable to come up with a simpler reproducer.
Comment 1 Carl Love 2022-02-07 16:20:04 UTC
Mark, thanks for letting us know about the issue.  Let us know if there is anything we can do to help verify the fix.  Off hand I have nothing to add to the fix.
Comment 2 Mark Wielaard 2022-02-08 15:45:21 UTC
(In reply to Carl Love from comment #1)
> Mark, thanks for letting us know about the issue.  Let us know if there is
> anything we can do to help verify the fix.  Off hand I have nothing to add
> to the fix.

Thanks. Pushed as:

commit fb6a77ed78876083e8ba4c2f92384db5c2e41be8 (HEAD -> master, origin/master, origin/HEAD)
Author: Mark Wielaard <mark@klomp.org>
Date:   Tue Feb 8 16:36:08 2022 +0100

    ppc64 --track-origins=yes failure because of bad cmov addHRegUse
    
    For Pin_CMov getRegUsage_PPCInstr called addHRegUse for the dst
    register with HRmWrite, but since this is a conditional move the
    register could be both read and written (read + write = modify).
    This matches the dst of Pin_FpCMov and Pin_AvCMov.
    
    In a very rare case, and only with --track-origins=yes, this
    could cause bad code generation.
    
    This is slightly amazing, this code is from 2005 and as far as
    I know we never seen an issue with --track-origins=yes on power
    before. And I have been unable to come up simple reproducer.
    
    https://bugs.kde.org/show_bug.cgi?id=449672

I have ran various tests and seen no regressions with this change. The bad code generation is also gone.
The issue is pretty hard to reproduce and I haven't been able to produce a simple reproducer. If you find
any issues with this code please add to this bug.