[PATCH 2/2] Fast tracepoint for powerpc64le

[PATCH 2/2] Fast tracepoint for powerpc64le

[Wei-cheng Wang]

Hi,

This patch adds an IPA function, jump_pad_area_hint, for giving a hint
where to map jump pad buffer.  addr = pagesize is too low for powerpr
executable.

Besides, "MAP_FIXED" flag is removed.  See MMAP(2)

    If the memory region specified by addr and len  overlaps  pages
    of any existing mapping(s), then the overlapped part of the existing
                                                            ^^^^^^^^^^^^
    mapping(s) will be discarded.
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Thanks,
Wei-cheng
---

gdb/gdbserver/ChangeLog

2015-02-20  Wei-cheng Wang  <cole945@gmail.com>

	* tracepoint.c (initialize_tracepoint): Calling jump_pad_area_hint()
	to get where to map gdb_jump_pad_buffer.  Remove MAP_FIXED.
	* tracepoint.h (jump_pad_area_hint): Add declaration.
	* linux-amd64-ipa.c (jump_pad_area_hint): New function.
	* linux-i386-ipa.c (jump_pad_area_hint): New function.
	* linux-ppc-ipa.c (jump_pad_area_hint): New function.
---
  gdb/gdbserver/linux-amd64-ipa.c | 11 +++++++++++
  gdb/gdbserver/linux-i386-ipa.c  | 11 +++++++++++
  gdb/gdbserver/linux-ppc-ipa.c   | 12 ++++++++++++
  gdb/gdbserver/tracepoint.c      |  7 +++----
  gdb/gdbserver/tracepoint.h      |  1 +
  5 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/linux-amd64-ipa.c b/gdb/gdbserver/linux-amd64-ipa.c
index c27ef21..db27d2a 100644
--- a/gdb/gdbserver/linux-amd64-ipa.c
+++ b/gdb/gdbserver/linux-amd64-ipa.c
@@ -77,6 +77,17 @@ gdb_agent_get_raw_reg (const unsigned char *raw_regs, int regnum)
    return *(ULONGEST *) (raw_regs + x86_64_ft_collect_regmap[regnum]);
  }

+/* Return the address for where to allocate buffer for jump pad.
+   The buffer should be close enough for tracepoints.  */
+
+uintptr_t
+jump_pad_area_hint (void)
+{
+  /* Allocate scratch buffer aligned on a page boundary, at a low
+     address (close to the main executable's code).  */
+  return sysconf (_SC_PAGE_SIZE);
+}
+
  #ifdef HAVE_UST

  #include <ust/processor.h>
diff --git a/gdb/gdbserver/linux-i386-ipa.c b/gdb/gdbserver/linux-i386-ipa.c
index 6381a55..1fc4bd2 100644
--- a/gdb/gdbserver/linux-i386-ipa.c
+++ b/gdb/gdbserver/linux-i386-ipa.c
@@ -114,6 +114,17 @@ gdb_agent_get_raw_reg (unsigned char *raw_regs, int regnum)
      return *(int *) (raw_regs + i386_ft_collect_regmap[regnum]);
  }

+/* Return the address for where to allocate buffer for jump pad.
+   The buffer should be close enough for tracepoints.  */
+
+uintptr_t
+jump_pad_area_hint (void)
+{
+  /* Allocate scratch buffer aligned on a page boundary, at a low
+     address (close to the main executable's code).  */
+  return sysconf (_SC_PAGE_SIZE);
+}
+
  #ifdef HAVE_UST

  #include <ust/processor.h>
diff --git a/gdb/gdbserver/linux-ppc-ipa.c b/gdb/gdbserver/linux-ppc-ipa.c
index 34b26d0..be6919b 100644
--- a/gdb/gdbserver/linux-ppc-ipa.c
+++ b/gdb/gdbserver/linux-ppc-ipa.c
@@ -105,6 +105,18 @@ gdb_agent_get_raw_reg (const unsigned char *raw_regs, int regnum)
  			+ ppc_ft_collect_regmap[regnum] * REGSZ);
  }

+/* Return the address for where to allocate buffer for jump pad.
+   The buffer should be close enough for tracepoints.  */
+
+uintptr_t
+jump_pad_area_hint (void)
+{
+  /* Main executables are normally mapped at 256MB.  Unconditional branch
+     can jump +/- 25-bit far (+/- 32MB), so allocate the buffer at 240MB
+     should be close enough in most cases.  */
+  return 0xf000000;
+}
+
  /* Initialize ipa_tdesc and others.  */

  void
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 2382a11..ad81d7a 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -7388,13 +7388,12 @@ initialize_tracepoint (void)

  #define SCRATCH_BUFFER_NPAGES 20

-    /* Allocate scratch buffer aligned on a page boundary, at a low
-       address (close to the main executable's code).  */
-    for (addr = pagesize; addr != 0; addr += pagesize)
+    addr = jump_pad_area_hint ();
+    for (; addr != 0; addr += pagesize)
        {
  	gdb_jump_pad_buffer = mmap ((void *) addr, pagesize * SCRATCH_BUFFER_NPAGES,
  				    PROT_READ | PROT_WRITE | PROT_EXEC,
-				    MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
+				    MAP_PRIVATE | MAP_ANONYMOUS,
  				    -1, 0);
  	if (gdb_jump_pad_buffer != MAP_FAILED)
  	  break;
diff --git a/gdb/gdbserver/tracepoint.h b/gdb/gdbserver/tracepoint.h
index 2adcd56..05a0c2a 100644
--- a/gdb/gdbserver/tracepoint.h
+++ b/gdb/gdbserver/tracepoint.h
@@ -88,6 +88,7 @@ void supply_static_tracepoint_registers (struct regcache *regcache,
  					 CORE_ADDR pc);
  void set_trampoline_buffer_space (CORE_ADDR begin, CORE_ADDR end,
  				  char *errmsg);
+uintptr_t jump_pad_area_hint (void);

  extern const struct target_desc *ipa_tdesc;

-- 
1.9.1

Re: [PATCH 2/2] Fast tracepoint for powerpc64le

[Pedro Alves]

On 02/20/2015 06:05 PM, Wei-cheng Wang wrote:

> gdb/gdbserver/ChangeLog
> 
> 2015-02-20  Wei-cheng Wang  <cole945@gmail.com>
> 
> 	* tracepoint.c (initialize_tracepoint): Calling jump_pad_area_hint()
> 	to get where to map gdb_jump_pad_buffer.  Remove MAP_FIXED.

Write:

	* tracepoint.c (initialize_tracepoint): Call jump_pad_area_hint
	to get where to map gdb_jump_pad_buffer.  Remove MAP_FIXED.

> 	* tracepoint.h (jump_pad_area_hint): Add declaration.
> 	* linux-amd64-ipa.c (jump_pad_area_hint): New function.
> 	* linux-i386-ipa.c (jump_pad_area_hint): New function.
> 	* linux-ppc-ipa.c (jump_pad_area_hint): New function.


I'll leave the PPC-specifics to Ulrich, but otherwise this looks
fine to me, with a nit.

> 
> +/* Return the address for where to allocate buffer for jump pad.
> +   The buffer should be close enough for tracepoints.  */
> +
> +uintptr_t
> +jump_pad_area_hint (void)


> +/* Return the address for where to allocate buffer for jump pad.
> +   The buffer should be close enough for tracepoints.  */
> +
> +uintptr_t
> +jump_pad_area_hint (void)

> +/* Return the address for where to allocate buffer for jump pad.
> +   The buffer should be close enough for tracepoints.  */
> +
> +uintptr_t
> +jump_pad_area_hint (void)

Could you move this duplicated comments to the declaration
instead?  Then in the implementations' comment you can
just write:

/* See tracepoint.h.  */

Thanks,
Pedro Alves

Re: [PATCH 2/2] Fast tracepoint for powerpc64le

[Ulrich Weigand]

Wei-cheng Wang wrote:

[Note, see also Pedro's comments here:
https://sourceware.org/ml/gdb-patches/2015-03/msg00132.html ]

> This patch adds an IPA function, jump_pad_area_hint, for giving a hint
> where to map jump pad buffer.  addr = pagesize is too low for powerpr
> executable.

As a general comment, the relative branch range will impose user-visible
restrictions on the fast tracepoint feature here.  We need to jump from
the tracepoint location to the jump pad, and back again.  The jump *to*
the jump pad could use an absolute branch, as long as the jump pads are
allocated in low memory.  However, the jump back is a problem.  (In
Power8, we might be able to use a bctar, but we may not be able to rely
on the TAR register being available.)

With your change, we are able to branch back from a jump pad to the main
executable, as long as it is mapped at the default location, and does
not exceed 32 MB in code size.  We cannot place fast tracepoints in any
shared library, in any executable loaded at a non-default address (which
might be an issue with address-space randomization) or with executables
whose code size exceeds 32 MB (which is not that uncommon).

As a first implementation, I guess I'd be fine with those) restriction,
but we should be thinking of how we could lift (some of) them.  We might
want to, for example,
 - determine the actual mapping base of the main executable and place
   the jump pad close even if it is not at the default address
 - use multiple jump pads, one for the main exectuable and one for
   each shared library

Also, current code already supports two areas, one for the main jump
pads, and one for "trampolines" (the latter are already placed at
platform-specific addresses).  Maybe it would make sense to exploit
that mechanism: use an *absolute* branch at the tracepoint address
to jump to a jump pad in low memory, do most of the tracepoint
activity there, and then jump (via bctr/blr) to a trampoline that
restores the last few registers (CTR/LR and SP?) and then branches
back via relative branch to user code.  This might help fit more
tracepoints if we, say, only have a single page available between
shared library mappings ...

> Besides, "MAP_FIXED" flag is removed.  See MMAP(2)
>
>    If the memory region specified by addr and len  overlaps  pages
>    of any existing mapping(s), then the overlapped part of the existing
>                                                            ^^^^^^^^^^^^
>    mapping(s) will be discarded.
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I agree that MAP_FIXED is dangerous.  Leaving MAP_FIXED off, however,
translates the "addr" parameter into a mere *hint*, so you might get
a mapping at *some different location* returned instead.  You should
at least check for that case ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] Fast tracepoint for powerpc64le

[Wei-cheng Wang]

On 2015/3/18 上午 02:29, Ulrich Weigand wrote:
> Power8, we might be able to use a bctar, but we may not be able to rely
> on the TAR register being available.)

If I clobbered TAR register, won't it affect the user code?

> As a first implementation, I guess I'd be fine with those) restriction,
> but we should be thinking of how we could lift (some of) them.  We might
> want to, for example,
>   - determine the actual mapping base of the main executable and place
>     the jump pad close even if it is not at the default address

How about this?  AT_PHDR should be at the very beginning of the first
loadable segment of the executable.  This should work for PIE.

uintptr_t
jump_pad_area_hint (void)
{
   /* Use AT_PHDR address to guess where the main executable is mapped,
      and try to map the jump pad before it.  The jump pad should be
      closed enough to the executable for unconditional branch (+/- 32MB). */

   uintptr_t base = getauxval (AT_PHDR);
   uintptr_t pagesz = sysconf (_SC_PAGE_SIZE);
   uintptr_t hint = (base & ~(pagesz - 1)) - 1 * 1024 * 1024;

   /* Is wrap around?  */
   if (hint > base)
     hint = pagesz;

   return hint;
}

Thanks,
Wei-cheng

Re: [PATCH 2/2] Fast tracepoint for powerpc64le

[Ulrich Weigand]

Wei-cheng Wang wrote:
> On 2015/3/18 上午 02:29, Ulrich Weigand wrote:
> > Power8, we might be able to use a bctar, but we may not be able to rely
> > on the TAR register being available.)
> 
> If I clobbered TAR register, won't it affect the user code?

Well, the ABI defines that user code must never use TAR, and must not
rely on it not being clobbered.  The intent is specifically to allow
it to be used by system code for purposes just like this.

There's still a bit of uncertainty what exactly "system code" is and
whether use of TAR by a gdbserver IPA stub might interfere with use
of TAR by *other* parts of the system, so it's probably best to
leave it alone for now.  But definitely something to revisit later.

> > As a first implementation, I guess I'd be fine with those) restriction,
> > but we should be thinking of how we could lift (some of) them.  We might
> > want to, for example,
> >   - determine the actual mapping base of the main executable and place
> >     the jump pad close even if it is not at the default address
> 
> How about this?  AT_PHDR should be at the very beginning of the first
> loadable segment of the executable.  This should work for PIE.
> 
> uintptr_t
> jump_pad_area_hint (void)
> {
>    /* Use AT_PHDR address to guess where the main executable is mapped,
>       and try to map the jump pad before it.  The jump pad should be
>       closed enough to the executable for unconditional branch (+/- 32MB). */
> 
>    uintptr_t base = getauxval (AT_PHDR);
>    uintptr_t pagesz = sysconf (_SC_PAGE_SIZE);
>    uintptr_t hint = (base & ~(pagesz - 1)) - 1 * 1024 * 1024;
> 
>    /* Is wrap around?  */
>    if (hint > base)
>      hint = pagesz;
> 
>    return hint;
> }

Looks OK to me.  Note that getauxval was only introduced with
a relatively recent glibc release, so might warrant a configure
check to avoid breaking the build on older releases ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com