Bug 402006 - final_tidyup should mark helper registers as defined before jumping to the freeres_wrapper
Summary: final_tidyup should mark helper registers as defined before jumping to the fr...
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-11 15:28 UTC by Mark Wielaard
Modified: 2018-12-12 13:21 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed 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 2018-12-11 15:28:31 UTC
In final_tidyup we setup the guest to call the freeres_wrapper, which will (possibly) call __gnu_cxx::__freeres() and/or __libc_freeres().

For ppc64le we do this with:

#  elif  defined(VGP_ppc64le_linux)
   /* setting GPR2 but not really needed, GPR12 is needed */
   VG_(threads)[tid].arch.vex.guest_GPR2  = freeres_wrapper;
   VG_(threads)[tid].arch.vex.guest_GPR12 = freeres_wrapper;
#  endif

And then we setup the argument as:

#  elif defined(VGA_ppc64be) || defined(VGA_ppc64le)
   VG_(threads)[tid].arch.vex.guest_GPR3 = to_run;
   VG_TRACK(post_reg_write, Vg_CoreClientReq, tid,
            offsetof(VexGuestPPC64State, guest_GPR3),
            sizeof(VG_(threads)[tid].arch.vex.guest_GPR3));

Note how in the first hunk we are not marking GPR2 and GPR12 as defined/written.

This might cause:

==21345== Use of uninitialised value of size 8
==21345==    at 0x4050794: _vgnU_freeres (vg_preloaded.c:68)
==21345==    by 0x4114A03: __run_exit_handlers (in /usr/lib64/power9/libc-2.28.so)
==21345==    by 0x4114A97: exit (in /usr/lib64/power9/libc-2.28.so)
==21345==    by 0x40F417F: (below main) (in /usr/lib64/power9/libc-2.28.so)
==21345== 
==21345== Use of uninitialised value of size 8
==21345==    at 0x40505E4: ??? (in /root/valgrind-install/lib/valgrind/vgpreload_core-ppc64le-linux.so)
==21345==    by 0x4114A03: __run_exit_handlers (in /usr/lib64/power9/libc-2.28.so)
==21345==    by 0x4114A97: exit (in /usr/lib64/power9/libc-2.28.so)
==21345==    by 0x40F417F: (below main) (in /usr/lib64/power9/libc-2.28.so)
==21345== 

This code checks r12 (first to see if a weak function symbol is set, then inside the plt entry for __libc_freeres).

So, the setup code should also mark the helper registers as defined/written for those architectures that use them:

diff --git a/coregrind/m_main.c b/coregrind/m_main.c
index 00702fc22..9052edf1e 100644
--- a/coregrind/m_main.c
+++ b/coregrind/m_main.c
@@ -2312,14 +2312,26 @@ static void final_tidyup(ThreadId tid)
    VG_(set_IP)(tid, freeres_wrapper);
 #  if defined(VGP_ppc64be_linux)
    VG_(threads)[tid].arch.vex.guest_GPR2 = r2;
+   VG_TRACK(post_reg_write, Vg_CoreClientReq, tid,
+            offsetof(VexGuestPPC64State, guest_GPR2),
+            sizeof(VG_(threads)[tid].arch.vex.guest_GPR2));
 #  elif  defined(VGP_ppc64le_linux)
    /* setting GPR2 but not really needed, GPR12 is needed */
    VG_(threads)[tid].arch.vex.guest_GPR2  = freeres_wrapper;
+   VG_TRACK(post_reg_write, Vg_CoreClientReq, tid,
+            offsetof(VexGuestPPC64State, guest_GPR2),
+            sizeof(VG_(threads)[tid].arch.vex.guest_GPR2));
    VG_(threads)[tid].arch.vex.guest_GPR12 = freeres_wrapper;
+   VG_TRACK(post_reg_write, Vg_CoreClientReq, tid,
+            offsetof(VexGuestPPC64State, guest_GPR12),
+            sizeof(VG_(threads)[tid].arch.vex.guest_GPR12));
 #  endif
    /* mips-linux note: we need to set t9 */
 #  if defined(VGP_mips32_linux) || defined(VGP_mips64_linux)
    VG_(threads)[tid].arch.vex.guest_r25 = freeres_wrapper;
+   VG_TRACK(post_reg_write, Vg_CoreClientReq, tid,
+            offsetof(VexGuestMIPS32State, guest_r25),
+            sizeof(VG_(threads)[tid].arch.vex.guest_r25));
 #  endif
 
    /* Pass a parameter to freeres_wrapper(). */

This is somewhat hard to test though because it really depends on whether or not those registers were fully defined right before. In most cases they are.
Comment 1 Mark Wielaard 2018-12-12 13:21:31 UTC
Also tested on ppc64[be] and reviewed on irc by Julian (who noticed the issue originally). Committed.

commit be7a73004583aab5d4c97cf55276ca58d5b3090b
Author: Mark Wielaard <mark@klomp.org>
Date:   Wed Dec 12 14:15:28 2018 +0100

    Mark helper regs defined in final_tidyup before freeres_wrapper call.
    
    In final_tidyup we setup the guest to call the freeres_wrapper, which
    will (possibly) call __gnu_cxx::__freeres() and/or __libc_freeres().
    
    In a couple of cases (ppc64be, ppc64le and mips32) this involves setting
    up one or more helper registers. Since we setup these guest registers
    we should make sure to mark them as fully defined. Otherwise we might
    see spurious warnings about undefined value usage if the guest register
    happened to not be fully defined before.
    
    This fixes PR402006.