Bug 290974

Summary:	vgdb must align pages to VKI_SHMLBA (16KB) on ARM
Product:	[Developer tools] valgrind	Reporter:	John Reiser <jreiser>
Component:	general	Assignee:	Julian Seward <jseward>
Status:	RESOLVED FIXED
Severity:	normal	CC:	philippe.waroquiers
Priority:	NOR
Version First Reported In:	3.7 SVN
Target Milestone:	---
Platform:	Compiled Sources
OS:	Linux
Latest Commit:		Version Fixed/Implemented In:
Sentry Crash Report:
Attachments:	on arm, round to SHLMBA on arm, round to SHLMBA (correct version now I hope :) on arm, round to SHLMBA (handled comments)

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