This is the same as 250631, right?

I'd assumed that the page-of-kernel-made-up-code-in-userspace
thing was some nasty kludge that had been done anway with in more
recent kernels, which is why I got rid of support for this in
r10973 (see https://bugs.kde.org/show_bug.cgi?id=250631#c5),
and that seemed to be ok for Ubuntu 10.04 and 10.10.

If it's still alive in a Linaro kernel, are you saying effectively
that it's officially blessed by Linaro, and needs to be supported
indefinitely?

Actually, having now read coregrind/m_aspacemgr/aspacemgr-linux.c we already do special-case this, and the problem is just that the kernel now does expose this magic page in /proc/*/maps and this confuses the workaround valgrind had for that kernel bug.

I think the fix is simply to change the value of ARM_LINUX_FAKE_COMMPAGE_END1 to match what the kernel is actually providing:

Index: coregrind/m_aspacemgr/aspacemgr-linux.c
===================================================================
--- coregrind/m_aspacemgr/aspacemgr-linux.c     (revision 11449)
+++ coregrind/m_aspacemgr/aspacemgr-linux.c     (working copy)
@@ -344,7 +344,7 @@
    we have to fake it up, in parse_procselfmaps. */
 #if defined(VGP_arm_linux)
 #  define ARM_LINUX_FAKE_COMMPAGE_START 0xFFFF0000
-#  define ARM_LINUX_FAKE_COMMPAGE_END1  0xFFFFF000
+#  define ARM_LINUX_FAKE_COMMPAGE_END1  0xFFFF1000
 #endif
 
 
With this fix I can valgrind /bin/ls on both the machine where [vectors] does not appear in maps and the machine where it does.

You might also want to update the comments complaining about this kernel bug to note that it is fixed in newer kernels...

(That comment was a mid-air collision so doesn't take account of your #1.)

> I got rid of support for this in r10973 

I don't think this is the same as that bug. The problem here is that the attempt in aspacemgr-linux.c to special case this mapping (which is still present in trunk) is not firing because the guess it makes about the length of the mapping is wrong.

> I'd assumed that the page-of-kernel-made-up-code-in-userspace
> thing was some nasty kludge that had been done away with in more
> recent kernels

I'm told it's a standard part of the ARM kernel ABI for userspace. This isn't Linaro specific at all.

Binaries compiled for eg armv7 may well not need to use as many of the helpers as armv4-compatible binaries, but as you see even they will still use some of them.

Do any of them do anything that prevents us from translating and executing like any other bit of userspace code?

> Do any of them do anything that prevents us from translating and executing
> like any other bit of userspace code?

Having looked at the kernel implementations for 2.6.35 I think the answer is "no" for __kernel_dmb(), "no if armv6 or better" for __kernel_cmpxchg() and "maybe" for __kernel_get_tls(). [The tls code is patched at kernel startup to just do a coprocessor register access if the cpu supports it and you haven't set some odd config options; so I think the usual armv7 case will be 'no'.]

(Technically the kernel could change to break us without advance warning because the ABI is only the entry points and their results, and the actual code executed could change...)

(In reply to comment #2)
> With this fix I can valgrind /bin/ls on both the machine where [vectors] does
> not appear in maps and the machine where it does.

There are two different but related issues here:

(1) whether the commpage is properly mapped/permission'd in m_aspacemgr

(2) whether guest attempts to set/get its TLS pointer work correctly.

Your patch addresses (1).  As it stands, Valgrind assumes that the
guest keeps its TLS in TPIDRURO (guest_TPIDRURO in the ARM guest
state).  Guest syscalls to ARM_set_tls are intercepted, the specified
value is parked in guest_TPIDRURO, and Valgrind doesn't let the call
go through to the kernel, because that would result in the kernel
setting the host TPIDRURO, which isn't what we want.

I am concerned that allowing the commpage to be executable could
result in mishandling of TLS values.  That said, if all the commpage
does in response to a call __kernel_get_tls is to read TPIDRURO, then
that's OK because it will merely read the value parked there by
sys_set_tls in syswrap-arm-linux.c.  If the TLS is stored anywhere
else then we're in deep trouble.

Re the patch, I would be more convinced (given the above) if you are
successfully able to run, with --tool=memcheck, non-trivial
threaded apps on both kernel configurations.  (viz, Firefox and
OpenOffice).  A useful small test case might be
helgrind/tests/tc17_sembar: 
gcc -g -o tc17 helgrind/tests/tc17_sembar.c -DVGO_linux

I can confirm that firefox, openoffice and the tc17 test case all work correctly with this patch. (This is as expected, since the patch simply makes kernels which report [vectors] in maps behave the same way as kernels which don't report [vectors], and we already know from the pegatron that "just execute the commpage" works for armv7 kernels not configured for software TLS.)

Things will go wrong on kernels not configured for s/w TLS, but they do already: that's bug 250631. (You will note that the reported failure mode there is not "tried to execute in the commpage but could not", but "executed in the commpage and got the wrong answer and failed later on". We can deduce that the submitter's kernel config doesn't report [vectors] in maps and has s/w TLS.)

I agree that it would be cleaner for valgrind to intercept all three of the commpage entry points and emulate/wrap them the same way it does system calls. If you do that then this patch would become moot; if you're not going to do that for 3.6.0 then I think we should definitely put this patch in for the moment.

> Things will go wrong on kernels not configured for s/w TLS

Gah. I meant "on kernels configured for s/w TLS". Sorry.

Thanks for the extra testing.  I'll commit the patch shortly.

(The following is clarification)  I thought more about the TLS issue.
The difficulty lies in the asymmetry between how it is set and how it
is fetched.

To get it, the guest either reads TPIDRURO directly, or calls the
commpage, which does whatever it does to produce the value.

To set it, the guest does a ARM_set_tls syscall.  Problem is that
Valgrind doesn't know what that syscall does, and so it doesn't know
what to do with the supplied value.  In this case we assume that the
kernel merely parks it in TPIDRURO, so we copy the value into the
simulated TPIDRURO.

But the kernel _could_ do absolutely anything with the value.  It
could use it to alter the number of pink elephants in orbit around the
moon, provided only that the get_tls routine in the commpage somehow
manages to come up with the right value when called.

If the TLS were both read and written via syscalls, fine -- then the
required value would go back and forth across the kernel boundary,
directly into guest registers or memory (don't care which).

If the TLS was handled entirely in userspace, by setting some
register, also fine .. we'd just simulate the register like all the
rest, and reads and writes of it would be handed naturally.

In the asymmetric case, we have to make some assumption about how
ARM_set_tls alters the guest state, and if we get that wrong then it
fails a la bug 250631.

Well so long as we can assume that a value set by set_tls is only ever fetched by calling the commpage routine then we can stash the value wherever we like when set_tls is called so long as we also replace the commpage routine with one which fetches it from the same place.

We already replace some of the commpage routines on x86 so we have the technology to do that.

Committed w/ further comments, r11460.  Thanks for the analysis
and testing.

> Well so long as we can assume that a value set by set_tls is only ever fetched
> by calling the commpage routine

I believe that the ABI allows a program to also read the TLS by executing the MRC instruction to read the register directly, so valgrind would need to support both commpage call and MRC to read the TLS value. (The idea being that if you're compiling for armv6k or better you go ahead and use the MRC, if you're compiling for armv4/v5 for back-compatibility then you emit a call to the commpage, and either way will work on a kernel that's compiled for v6k or better.) Doesn't invalidate the approach, just a wrinkle to remember.