Re: [PATCH] New version of libmpx with new memmove wrapper

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2015-11-05 13:37 GMT+03:00 Aleksandra Tsvetkova <astsvetk@gmail.com>:
> New version of libmpx was added. There is a new function get_bd() that
> allows to get bounds directory. Wrapper for memmove was modified. Now
> it moves data and then moves corresponding bounds directly from one
> bounds table to another. This approach made moving unaligned pointers
> possible. It also makes memmove function faster on sizes bigger than
> 64 bytes.

+2015-10-27  Tsvetkova Alexandra  <aleksandra.tsvetkova@intel.com>
+
+ * gcc.target/i386/mpx/memmove.c: New test for __mpx_wrapper_memmove.
+

Did you test it on different targets? It seems to me this test will
fail if you run it
on non-MPX target.  Please look at mpx-check.h and how other MPX run
tests use it.

+ * mpxrt/mpxrt.c (NUM_L1_BITS): Moved to mpxrt.h.
+ * mpxrt/mpxrt.c (REG_IP_IDX): Moved to mpxrt.h.
+ * mpxrt/mpxrt.c (REX_PREFIX): Moved to mpxrt.h.
+ * mpxrt/mpxrt.c (XSAVE_OFFSET_IN_FPMEM): Moved to mpxrt.h.
+ * mpxrt/mpxrt.c (MPX_L1_SIZE): Moved to mpxrt.h.

No need to repeat file name.

+ * libmpxwrap/mpx_wrappers.c: Rewrite __mpx_wrapper_memmove to make it faster.

You added new functions, types and modified existing function.  Make
ChangeLog more detailed.

--- /dev/null
+++ b/libmpx/mpxrt/mpxrt.h
@@ -0,0 +1,75 @@
+/* mpxrt.h                  -*-C++-*-
+ *
+ *************************************************************************
+ *
+ *  @copyright
+ *  Copyright (C) 2014, 2015, Intel Corporation
+ *  All rights reserved.

2015 only

+const uintptr_t MPX_L1_ADDR_MASK = 0xfffff000UL;
+const uintptr_t MPX_L2_ADDR_MASK = 0xfffffffcUL;
+const uintptr_t MPX_L2_VALID_MASK = 0x00000001UL;

Use defines


--- a/libmpx/mpxwrap/Makefile.am
+++ b/libmpx/mpxwrap/Makefile.am
@@ -1,4 +1,5 @@
 ALCLOCAL_AMFLAGS = -I .. -I ../config
+AM_CPPFLAGS = -I $(top_srcdir)

This is not reflected in ChangeLog

+/* The mpx_bt_entry struct represents a cell in bounds table.
+   *lb is the lower bound, *ub is the upper bound,
+   *p is the stored pointer.  */

Bounds and pointer are in lb, ub, p, not in *lb, *ub, *p. Right?

+static inline void
+alloc_bt (void *ptr)
+{
+  __asm__ __volatile__ ("bndstx %%bnd0, (%0,%0)"::"r" (ptr):"%bnd0");
+}

This should be marked as bnd_legacy.

+/* move_bounds function copies N bytes from SRC to DST.

Really?

+   It also copies bounds for all pointers inside.
+   There are 3 parts of the algorithm:
+   1) We copy everything till the end of the first bounds table SRC)

SRC is not a bounds table

+   2) In loop we copy whole bound tables till the second-last one
+   3) Data in the last bounds table is copied separately, after the loop.
+   If one of bound tables in SRC doesn't exist,
+   we skip it because there are no pointers.
+   Depending on the arrangement of SRC and DST we copy from the beginning
+   or from the end.  */
+__attribute__ ((bnd_legacy)) static void *
+move_bounds (void *dst, const void *src, size_t n)

What is returned value for?

+void *
+__mpx_wrapper_memmove (void *dst, const void *src, size_t n)
+{
+  if (n == 0)
+    return dst;
+
+  __bnd_chk_ptr_bounds (dst, n);
+  __bnd_chk_ptr_bounds (src, n);
+
+  memmove (dst, src, n);
+  move_bounds (dst, src, n);
+  return dst;
 }

You completely remove old algorithm which should be faster on small
sizes. __mpx_wrapper_memmove should become a dispatcher between old
and new implementations depending on target (32-bit or 64-bit) and N.
Since old version performs both data and bounds copy, BD check should
be moved into __mpx_wrapper_memmove to never call
it when MPX is disabled.

Thanks,
Ilya