Bug 290974 - vgdb must align pages to VKI_SHMLBA (16KB) on ARM
Summary: vgdb must align pages to VKI_SHMLBA (16KB) on ARM
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.7 SVN
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-08 15:28 UTC by John Reiser
Modified: 2012-02-07 20:09 UTC (History)
1 user (show)

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


Attachments
on arm, round to SHLMBA (994 bytes, text/plain)
2012-01-24 05:02 UTC, Philippe Waroquiers
Details
on arm, round to SHLMBA (correct version now I hope :) (1000 bytes, text/plain)
2012-01-24 05:07 UTC, Philippe Waroquiers
Details
on arm, round to SHLMBA (handled comments) (1.60 KB, text/plain)
2012-01-26 19:56 UTC, Philippe Waroquiers
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Reiser 2012-01-08 15:28:38 UTC
Version:           3.7 SVN (using Devel) 
OS:                Linux

On ARM, vgdb does not align the address for mmap2(,,,MAP_FIXED,,) to 16KB (VKI_SHMLBA), and this lack often results in EINVAL and abnormal termination.  The hardware requires 16KB alignment in order to support [shared] page mapping.

Reproducible: Sometimes

Steps to Reproduce:
As reported by "Kershkovitz, Koby (Koby)" on 12/30/2011 in mailing list [Valgrind-users] "failure to run on armv6 following the armv6 legacy patches suggested by bug 276897".

Actual Results:  
strace reveals:
-----
open("/tmp/vgdb-pipe-shared-mem-vgdb-1793-by-root-on-???",
O_RDWR|O_CREAT, 0666) = 3
write(3,
"\0\0\0\0\0\0\0\0\0\0\0\0\360\240\0068\0\210\2468\240\26\0\0\4\0\0\0\350
\24\0\0", 32) = 32
mmap2(0x401d000, 32, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED, 3, 0) =
-1 EINVAL (Invalid argument)
-----
Abnormal termination follows shortly.

Note the address 0x401d000 is not divisible by 0x4000 (16KB) which is VKI_SHMLBA.

Expected Results:  
MAP_FIXED address is already a multiple of 16KB (VKI_SHMLBA), so that mmap2() succeeds and normal execution continues.

Example fix for another code path that involves mmap2 is given in patch 64147 to coregrind/m_syswrap/syswrap-generic.c [among others; correct code for this path already is present in valgrind_3.7.0].
Comment 1 Philippe Waroquiers 2012-01-08 19:08:45 UTC
I will modify VG_(m_mmap_file_float_valgrind_flags) with a fix similar to 
what was done for bug 222545.

Note that when looking at the current code, I found one strange thing
in am_mmap_file_float_valgrind_flags : the call to am_get_advisory
indicates the call is for the client, while it should be for Valgrind.
Comment 2 Philippe Waroquiers 2012-01-08 21:07:58 UTC
(In reply to comment #1)
> I will modify VG_(m_mmap_file_float_valgrind_flags) with a fix similar to 
> what was done for bug 222545.
This is still to be done.

> Note that when looking at the current code, I found one strange thing
> in am_mmap_file_float_valgrind_flags : the call to am_get_advisory
> indicates the call is for the client, while it should be for Valgrind.
Fixed the forClient arg to indicate this is a Valgrind mmap (revision 12326).
Comment 3 Philippe Waroquiers 2012-01-24 05:02:46 UTC
Created attachment 68125 [details]
on arm, round to SHLMBA 

I currently have no access to an ARM system.

John, it would be nice if you could review this patch
and validate it works/solves the problem on ARM.

Thanks
Comment 4 Philippe Waroquiers 2012-01-24 05:07:03 UTC
Created attachment 68126 [details]
on arm, round to SHLMBA (correct version now I hope :)
Comment 5 John Reiser 2012-01-24 17:01:38 UTC
(In reply to comment #4)
Works for me.

Perhaps only VKI_MAP_SHARED requires aligning to VKI_SHMLBA:

   +   if ((VKI_MAP_SHARED & flags) && (VKI_SHMLBA > VKI_PAGE_SIZE)) {
   ...
   +   if ((VKI_MAP_SHARED & flags) && (VKI_SHMLBA > VKI_PAGE_SIZE))

but just testing only (VKI_SHMLBA > VKI_PAGE_SIZE) is OK for now.
Comment 6 Julian Seward 2012-01-26 10:02:45 UTC
(In reply to comment #4)
> Created an attachment (id=68126) [details]
> on arm, round to SHLMBA (correct version now I hope :)

ok to commit w/ two small changes:

* add this
  #if !defined(VGA_arm)
  aspacem_assert(VKI_SHMLBA == VKI_PAGE_SIZE);
  #endif

* pls add { } around else clause
  req.len = length;
Comment 7 Philippe Waroquiers 2012-01-26 19:56:26 UTC
Created attachment 68206 [details]
on arm, round to SHLMBA (handled comments)

* inserted {} around else reg.len = length;
* added assert for non arm
* only round to SHMLBA for shared case

Regtested on linux x86 (still no access to an ARM system,
so a final check on ARM would be nice before commit).
Comment 8 Philippe Waroquiers 2012-02-02 22:19:32 UTC
(In reply to comment #7)
> Regtested on linux x86 (still no access to an ARM system,
> so a final check on ARM would be nice before commit).
After some limited validation on Android arm emulator,
committed in revision 12366