Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2015-03-04 Thread Jeff Law 

On 03/02/15 09:19, Ilya Enkovich wrote:

On 09 Dec 23:56, Ilya Enkovich wrote:

2014-12-09 22:57 GMT+03:00 Jeff Law l...@redhat.com:


Wasn't this duplicated in the mpx-wrapper patch?


Wrappers patch introduces similar option static-libmpxwrappers.



I think this is OK on the technical side.  I need to do to some vote
tallying on the steering committee side WRT licensing, ownership of the
sources, canonical source location, etc, so please don't commit yet.


Great! Let me know when it's time to commit.

Thanks,
Ilya



jeff



Hi,

Here is an updated version of MPX runtime integration patch.  In this version 
libmpx is disabled by default and may be enabled for i386 linux targets.  May 
this version be used for trunk?

Thanks,
Ilya
--
2015-03-02  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2015-03-02  Ilya Enkovich  ilya.enkov...@intel.com

* config/i386/linux-common.h (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(CHKP_SPEC): New.
* gcc.c (CHKP_SPEC): New.
(LINK_COMMAND_SPEC): Add CHKP_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/

2015-03-02  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.
Looks good given its disabled by default.  Give the release managers 
24hrs to object and if they don't, then it's good for the trunk.


jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2015-03-02 Thread Ilya Enkovich 
On 09 Dec 23:56, Ilya Enkovich wrote:
 2014-12-09 22:57 GMT+03:00 Jeff Law l...@redhat.com:
 
  Wasn't this duplicated in the mpx-wrapper patch?
 
 Wrappers patch introduces similar option static-libmpxwrappers.
 
 
  I think this is OK on the technical side.  I need to do to some vote
  tallying on the steering committee side WRT licensing, ownership of the
  sources, canonical source location, etc, so please don't commit yet.
 
 Great! Let me know when it's time to commit.
 
 Thanks,
 Ilya
 
 
  jeff
 

Hi,

Here is an updated version of MPX runtime integration patch.  In this version 
libmpx is disabled by default and may be enabled for i386 linux targets.  May 
this version be used for trunk?

Thanks,
Ilya
--
2015-03-02  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2015-03-02  Ilya Enkovich  ilya.enkov...@intel.com

* config/i386/linux-common.h (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(CHKP_SPEC): New.
* gcc.c (CHKP_SPEC): New.
(LINK_COMMAND_SPEC): Add CHKP_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/

2015-03-02  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.
diff --git a/Makefile.def b/Makefile.def
index aa75842..e0ea2fb 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -130,6 +130,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index 00f7452..3fe423d5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -640,6 +641,27 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Enable libmpx on supported systems by request.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = xyes; then
+   AC_MSG_CHECKING([for libmpx support])
+   if (srcdir=${srcdir}/libmpx; \
+   . ${srcdir}/configure.tgt; \
+   test $LIBMPX_SUPPORTED != yes)
+   then
+   AC_MSG_RESULT([no])
+   noconfigdirs=$noconfigdirs target-libmpx
+   else
+   AC_MSG_RESULT([yes])
+   fi
+else
+   noconfigdirs=$noconfigdirs target-libmpx
+fi
+fi
+
+
+
 # Disable libquadmath for some systems.
 case ${target} in
   avr-*-*)
@@ -2672,6 +2694,11 @@ if echo  ${target_configdirs}  | grep  libvtv   
/dev/null 21 
   bootstrap_target_libs=${bootstrap_target_libs}target-libvtv,
 fi
 
+# If we are building libmpx, bootstrap it.
+if echo  ${target_configdirs}  | grep  libmpx   /dev/null 21; then
+  bootstrap_target_libs=${bootstrap_target_libs}target-libmpx,
+fi
+
 # Determine whether gdb needs tk/tcl or not.
 # Use 'maybe' since enable_gdbtk might be true even if tk isn't available
 # and in that case we want gdb to be built without tk.  Ugh!
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index b3c8cee..ac55d3c 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1041,6 +1041,9 @@ fchkp-instrument-marked-only
 C ObjC C++ ObjC++ LTO Report Var(flag_chkp_instrument_marked_only) Init(0)
 Instrument only functions marked with bnd_instrument attribute.
 
+static-libmpx
+Driver
+
 fcilkplus
 C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
 Enable Cilk Plus
diff --git a/gcc/config/i386/linux-common.h b/gcc/config/i386/linux-common.h
index f93d71d..868274f 100644
--- a/gcc/config/i386/linux-common.h
+++ b/gcc/config/i386/linux-common.h
@@ -53,3 +53,27 @@ along with GCC; see the file COPYING3.  If not see
   GNU_USER_TARGET_ENDFILE_SPEC, \
   GNU_USER_TARGET_MATHFILE_SPEC   \
   ANDROID_ENDFILE_SPEC)
+
+#ifndef LIBMPX_LIBS
+#define LIBMPX_LIBS \
+ %:include(libmpx.spec)%(link_libmpx)
+#endif
+
+#ifndef LIBMPX_SPEC
+#if defined(HAVE_LD_STATIC_DYNAMIC)
+#define LIBMPX_SPEC \
+%{mmpx:%{fcheck-pointer-bounds:\
+%{static:--whole-archive -lmpx --no-whole-archive LIBMPX_LIBS }\
+%{!static:%{static-libmpx: LD_STATIC_OPTION  --whole-archive}\
+-lmpx %{static-libmpx:--no-whole-archive  LD_DYNAMIC_OPTION \
+LIBMPX_LIBS 
+#else
+#define LIBMPX_SPEC \
+%{mmpx:%{fcheck-pointer-bounds:-lmpx LIBMPX_LIBS }}
+#endif
+#endif
+
+#ifndef CHKP_SPEC
+#define CHKP_SPEC \
+%{!nostdlib:%{!nodefaultlibs: LIBMPX_SPEC }}
+#endif
diff --git a/gcc/gcc.c b/gcc/gcc.c
index 8a163a1..d956c36 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -812,6 +812,10 @@

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-12-09 Thread Ilya Enkovich 
On 24 Nov 17:02, Ilya Enkovich wrote:
 
 Right.  This works for both top level and multilib checks because failing 
 test is used and CC is usually not set when it's called by the top level 
 configure.  If we configure with CC=... then it may go wrong.  I left only 
 target check in configure.tgt and inlined test for x32 into libmpx configure.
 
  
  -- 
  Joseph S. Myers
  jos...@codesourcery.com
 
 Here is an updated version.
 
 Thanks,
 Ilya
 --

Here is an updated version.  I moved linker specs to target.  Currently mpx 
libraries are built for x86_64-*-linux* | i?86-*-linux*, so I think 
gcc/config/i386/linux-common.h is a proper place for LIBMPX delcarations.

Thanks,
Ilya
--
2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

* config/i386/linux-common.h (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(CHKP_SPEC): New.
* gcc.c (CHKP_SPEC): New.
(LINK_COMMAND_SPEC): Add CHKP_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/

2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.


diff --git a/Makefile.def b/Makefile.def
index 7c8761a..82432ef 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -129,6 +129,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index fd1bdf0..1d36f6b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -642,6 +643,25 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Disable libmpx on unsupported systems.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = x; then
+   AC_MSG_CHECKING([for libmpx support])
+   if (srcdir=${srcdir}/libmpx; \
+   . ${srcdir}/configure.tgt; \
+   test $LIBMPX_SUPPORTED != yes)
+   then
+   AC_MSG_RESULT([no])
+   noconfigdirs=$noconfigdirs target-libmpx
+   else
+   AC_MSG_RESULT([yes])
+   fi
+fi
+fi
+
+
+
 # Disable libquadmath for some systems.
 case ${target} in
   avr-*-*)
@@ -2662,6 +2682,11 @@ if echo  ${target_configdirs}  | grep  libvtv   
/dev/null 21 
   bootstrap_target_libs=${bootstrap_target_libs}target-libvtv,
 fi
 
+# If we are building libmpx, bootstrap it.
+if echo  ${target_configdirs}  | grep  libmpx   /dev/null 21; then
+  bootstrap_target_libs=${bootstrap_target_libs}target-libmpx,
+fi
+
 # Determine whether gdb needs tk/tcl or not.
 # Use 'maybe' since enable_gdbtk might be true even if tk isn't available
 # and in that case we want gdb to be built without tk.  Ugh!
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index b9f7c65..65731cc 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1020,6 +1020,9 @@ fchkp-instrument-marked-only
 C ObjC C++ ObjC++ LTO Report Var(flag_chkp_instrument_marked_only) Init(0)
 Instrument only functions marked with bnd_instrument attribute.
 
+static-libmpx
+Driver
+
 fcilkplus
 C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
 Enable Cilk Plus
diff --git a/gcc/config/i386/linux-common.h b/gcc/config/i386/linux-common.h
index 1eaf024..07cd1a2 100644
--- a/gcc/config/i386/linux-common.h
+++ b/gcc/config/i386/linux-common.h
@@ -53,3 +53,27 @@ along with GCC; see the file COPYING3.  If not see
   GNU_USER_TARGET_ENDFILE_SPEC, \
   GNU_USER_TARGET_MATHFILE_SPEC   \
   ANDROID_ENDFILE_SPEC)
+
+#ifndef LIBMPX_LIBS
+#define LIBMPX_LIBS \
+ %:include(libmpx.spec)%(link_libmpx)
+#endif
+
+#ifndef LIBMPX_SPEC
+#if defined(HAVE_LD_STATIC_DYNAMIC)
+#define LIBMPX_SPEC \
+%{mmpx:%{fcheck-pointer-bounds:\
+%{static:--whole-archive -lmpx --no-whole-archive LIBMPX_LIBS }\
+%{!static:%{static-libmpx: LD_STATIC_OPTION  --whole-archive}\
+-lmpx %{static-libmpx:--no-whole-archive  LD_DYNAMIC_OPTION \
+LIBMPX_LIBS 
+#else
+#define LIBMPX_SPEC \
+%{mmpx:%{fcheck-pointer-bounds:-lmpx LIBMPX_LIBS }}
+#endif
+#endif
+
+#ifndef CHKP_SPEC
+#define CHKP_SPEC \
+%{!nostdlib:%{!nodefaultlibs: LIBMPX_SPEC }}
+#endif
diff --git a/gcc/gcc.c b/gcc/gcc.c
index 4cb4ba3..636830e 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -809,6 +809,10 @@ proper position among the other output files.  */
 %{fvtable-verify=preinit: -lvtv

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-12-09 Thread Jeff Law 

On 12/09/14 01:24, Ilya Enkovich wrote:

On 24 Nov 17:02, Ilya Enkovich wrote:


Right.  This works for both top level and multilib checks because failing test 
is used and CC is usually not set when it's called by the top level configure.  
If we configure with CC=... then it may go wrong.  I left only target check in 
configure.tgt and inlined test for x32 into libmpx configure.



--
Joseph S. Myers
jos...@codesourcery.com


Here is an updated version.

Thanks,
Ilya
--


Here is an updated version.  I moved linker specs to target.  Currently mpx 
libraries are built for x86_64-*-linux* | i?86-*-linux*, so I think 
gcc/config/i386/linux-common.h is a proper place for LIBMPX delcarations.

Thanks,
Ilya
--
2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

* config/i386/linux-common.h (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(CHKP_SPEC): New.
* gcc.c (CHKP_SPEC): New.
(LINK_COMMAND_SPEC): Add CHKP_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/




diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index b9f7c65..65731cc 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1020,6 +1020,9 @@ fchkp-instrument-marked-only
  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_instrument_marked_only) Init(0)
  Instrument only functions marked with bnd_instrument attribute.

+static-libmpx
+Driver
+
  fcilkplus
  C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
  Enable Cilk Plus

Wasn't this duplicated in the mpx-wrapper patch?

I think this is OK on the technical side.  I need to do to some vote 
tallying on the steering committee side WRT licensing, ownership of the 
sources, canonical source location, etc, so please don't commit yet.


jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-12-09 Thread Ilya Enkovich 
2014-12-09 22:57 GMT+03:00 Jeff Law l...@redhat.com:
 On 12/09/14 01:24, Ilya Enkovich wrote:

 On 24 Nov 17:02, Ilya Enkovich wrote:


 Right.  This works for both top level and multilib checks because failing
 test is used and CC is usually not set when it's called by the top level
 configure.  If we configure with CC=... then it may go wrong.  I left only
 target check in configure.tgt and inlined test for x32 into libmpx
 configure.


 --
 Joseph S. Myers
 jos...@codesourcery.com


 Here is an updated version.

 Thanks,
 Ilya
 --


 Here is an updated version.  I moved linker specs to target.  Currently
 mpx libraries are built for x86_64-*-linux* | i?86-*-linux*, so I think
 gcc/config/i386/linux-common.h is a proper place for LIBMPX delcarations.

 Thanks,
 Ilya
 --
 2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

 * Makefile.def: Add libmpx.
 * configure.ac: Add libmpx.
 * Makefile.in: Regenerate.
 * configure: Regenerate.

 gcc/

 2014-12-09  Ilya Enkovich  ilya.enkov...@intel.com

 * config/i386/linux-common.h (LIBMPX_LIBS): New.
 (LIBMPX_SPEC): New.
 (CHKP_SPEC): New.
 * gcc.c (CHKP_SPEC): New.
 (LINK_COMMAND_SPEC): Add CHKP_SPEC.
 * c-family/c.opt (static-libmpx): New.

 libmpx/


 diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
 index b9f7c65..65731cc 100644
 --- a/gcc/c-family/c.opt
 +++ b/gcc/c-family/c.opt
 @@ -1020,6 +1020,9 @@ fchkp-instrument-marked-only
   C ObjC C++ ObjC++ LTO Report Var(flag_chkp_instrument_marked_only)
 Init(0)
   Instrument only functions marked with bnd_instrument attribute.

 +static-libmpx
 +Driver
 +
   fcilkplus
   C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
   Enable Cilk Plus

 Wasn't this duplicated in the mpx-wrapper patch?

Wrappers patch introduces similar option static-libmpxwrappers.


 I think this is OK on the technical side.  I need to do to some vote
 tallying on the steering committee side WRT licensing, ownership of the
 sources, canonical source location, etc, so please don't commit yet.

Great! Let me know when it's time to commit.

Thanks,
Ilya


 jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-24 Thread Ilya Enkovich 
On 21 Nov 23:20, Joseph Myers wrote:
 On Fri, 21 Nov 2014, Ilya Enkovich wrote:
 
  +# Disable libmpx on unsupported systems.
  +if test -d ${srcdir}/libmpx; then
  +if test x$enable_libmpx = x; then
  +   AC_MSG_CHECKING([for libmpx support])
  +   if (srcdir=${srcdir}/libmpx; \
  +   . ${srcdir}/configure.tgt; \
  +   test $LIBMPX_SUPPORTED != yes)
  +   then
  +   AC_MSG_RESULT([no])
  +   noconfigdirs=$noconfigdirs target-libmpx
  +   else
  +   AC_MSG_RESULT([yes])
  +   fi
  +fi
  +fi
 
 Using this at toplevel you can only enable or disable for all multilibs - 
 this code runs just once.  But:
 
  diff --git a/libmpx/configure.tgt b/libmpx/configure.tgt
 
  +LIBMPX_SUPPORTED=no
  +case ${target} in
  +  x86_64-*-linux* | i?86-*-linux*)
  +   # X32 doesn't support MPX.
  +   echo int i[sizeof (void *) == 4 ? 1 : -1] = { __x86_64__ };  
  conftestx.c
  +   if ${CC} ${CFLAGS} -c -o conftestx.o conftestx.c  /dev/null 21; then
  +   LIBMPX_SUPPORTED=no
  +   else
  +   LIBMPX_SUPPORTED=yes
  +   fi
  +   ;;
 
 Here you are testing something per-multilib.  Furthermore, using CC and 
 CFLAGS here is wrong when used by toplevel configure, as at toplevel 
 they'll relate to the host rather than the target.
 
 So you need to separate the two tests of (a) might MPX be supported for 
 some multilib on the target (usable at toplevel, not using CC or CFLAGS) 
 and (b) is it supported for the present multilib.  Thus the libmpx 
 directory will need configuring for all multilibs for supported targets.  
 Then, if the particular multilib doesn't support it (i.e. is x32), you'll 
 need to arrange for the directory not to build or install anything - much 
 like the libquadmath directory gets configured in some cases where it 
 doesn't build anything.

Right.  This works for both top level and multilib checks because failing test 
is used and CC is usually not set when it's called by the top level configure.  
If we configure with CC=... then it may go wrong.  I left only target check in 
configure.tgt and inlined test for x32 into libmpx configure.

 
 -- 
 Joseph S. Myers
 jos...@codesourcery.com

Here is an updated version.

Thanks,
Ilya
--
diff --git a/Makefile.def b/Makefile.def
index 40bbca9..4a535d2 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -128,6 +128,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index b27fb1d..ccb119b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -642,6 +643,25 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Disable libmpx on unsupported systems.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = x; then
+   AC_MSG_CHECKING([for libmpx support])
+   if (srcdir=${srcdir}/libmpx; \
+   . ${srcdir}/configure.tgt; \
+   test $LIBMPX_SUPPORTED != yes)
+   then
+   AC_MSG_RESULT([no])
+   noconfigdirs=$noconfigdirs target-libmpx
+   else
+   AC_MSG_RESULT([yes])
+   fi
+fi
+fi
+
+
+
 # Disable libquadmath for some systems.
 case ${target} in
   avr-*-*)
@@ -2652,6 +2672,11 @@ if echo  ${target_configdirs}  | grep  libvtv   
/dev/null 21 
   bootstrap_target_libs=${bootstrap_target_libs}target-libvtv,
 fi
 
+# If we are building libmpx, bootstrap it.
+if echo  ${target_configdirs}  | grep  libmpx   /dev/null 21; then
+  bootstrap_target_libs=${bootstrap_target_libs}target-libmpx,
+fi
+
 # Determine whether gdb needs tk/tcl or not.
 # Use 'maybe' since enable_gdbtk might be true even if tk isn't available
 # and in that case we want gdb to be built without tk.  Ugh!
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 85dcb98..8f5d76c 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1040,6 +1040,9 @@ fchkp-instrument-marked-only
 C ObjC C++ ObjC++ LTO Report Var(flag_chkp_instrument_marked_only) Init(0)
 Instrument only functions marked with bnd_instrument attribute.
 
+static-libmpx
+Driver
+
 fcilkplus
 C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
 Enable Cilk Plus
diff --git a/gcc/config/i386/cpuid.h b/gcc/config/i386/cpuid.h
index 133e356..f85cebb 100644
--- a/gcc/config/i386/cpuid.h
+++ b/gcc/config/i386/cpuid.h
@@ -72,6 +72,7 @@
 #define bit_AVX2   (1  5)
 #define bit_BMI2   (1  8)
 #define bit_RTM(1  11)

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-21 Thread Ilya Enkovich 
On 19 Nov 21:11, Ilya Enkovich wrote:
 2014-11-19 20:55 GMT+03:00 Jeff Law l...@redhat.com:
  On 11/19/14 07:15, Ilya Enkovich wrote:
 
  --
  2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com
 
  * Makefile.def: Add libmpx.
  * configure.ac: Add libmpx.
  * Makefile.in: Regenerate.
  * configure: Regenerate.
 
  gcc/
 
  2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com
 
  * gcc.c (LIBMPX_LIBS): New.
  (LIBMPX_SPEC): New.
  (MPX_SPEC): New.
  (LINK_COMMAND_SPEC): Add MPX_SPEC.
  * c-family/c.opt (static-libmpx): New.
 
  libmpx/
 
  2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com
 
  Initial commit.
 
  So I have only done a cursory peek at this code, but one thing which I did
  immediately note was the CPU feature testing stuff.  Shouldn't all that
  stuff be integrated into the feature testing bits already found in libgcc?
 
 I'll have a look at these features.
 
 
  I've asked the steering committee to vote on accepting the runtime --
  necessary given Intel is keeping copyright ownership to the best of my
  knowledge.
 
 Thanks!
 
 Ilya
 
 
  Jeff
 

Jakub objected adding CPUID checks used in MPX runtime into 
__builtin_cpu_supports.  So I just added required bits into cpuid.h and removed 
local implementation of cpuid.  Is it OK?

Thanks,
Ilya
--
2014-11-21  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-11-21  Ilya Enkovich  ilya.enkov...@intel.com

* config/i386/cpuid.h (bit_MPX): New.
(bit_BNDREGS): New.
(bit_BNDCSR): New.
* gcc.c (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(MPX_SPEC): New.
(LINK_COMMAND_SPEC): Add MPX_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/

2014-11-21  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.


diff --git a/Makefile.def b/Makefile.def
index 40bbca9..4a535d2 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -128,6 +128,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index b27fb1d..ccb119b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -642,6 +643,25 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Disable libmpx on unsupported systems.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = x; then
+   AC_MSG_CHECKING([for libmpx support])
+   if (srcdir=${srcdir}/libmpx; \
+   . ${srcdir}/configure.tgt; \
+   test $LIBMPX_SUPPORTED != yes)
+   then
+   AC_MSG_RESULT([no])
+   noconfigdirs=$noconfigdirs target-libmpx
+   else
+   AC_MSG_RESULT([yes])
+   fi
+fi
+fi
+
+
+
 # Disable libquadmath for some systems.
 case ${target} in
   avr-*-*)
@@ -2652,6 +2672,11 @@ if echo  ${target_configdirs}  | grep  libvtv   
/dev/null 21 
   bootstrap_target_libs=${bootstrap_target_libs}target-libvtv,
 fi
 
+# If we are building libmpx, bootstrap it.
+if echo  ${target_configdirs}  | grep  libmpx   /dev/null 21; then
+  bootstrap_target_libs=${bootstrap_target_libs}target-libmpx,
+fi
+
 # Determine whether gdb needs tk/tcl or not.
 # Use 'maybe' since enable_gdbtk might be true even if tk isn't available
 # and in that case we want gdb to be built without tk.  Ugh!
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 85dcb98..8f5d76c 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1040,6 +1040,9 @@ fchkp-instrument-marked-only
 C ObjC C++ ObjC++ LTO Report Var(flag_chkp_instrument_marked_only) Init(0)
 Instrument only functions marked with bnd_instrument attribute.
 
+static-libmpx
+Driver
+
 fcilkplus
 C ObjC C++ ObjC++ LTO Report Var(flag_cilkplus) Init(0)
 Enable Cilk Plus
diff --git a/gcc/config/i386/cpuid.h b/gcc/config/i386/cpuid.h
index 133e356..f85cebb 100644
--- a/gcc/config/i386/cpuid.h
+++ b/gcc/config/i386/cpuid.h
@@ -72,6 +72,7 @@
 #define bit_AVX2   (1  5)
 #define bit_BMI2   (1  8)
 #define bit_RTM(1  11)
+#define bit_MPX(1  14)
 #define bit_AVX512F(1  16)
 #define bit_AVX512DQ   (1  17)
 #define bit_RDSEED (1  18)
@@ -87,6 +88,10 @@
 /* %ecx */
 #define bit_PREFETCHWT1  (1  0)
 
+/* XFEATURE_ENABLED_MASK register bits (%eax == 13, %ecx == 0) */
+#define

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-21 Thread Joseph Myers 
On Fri, 21 Nov 2014, Ilya Enkovich wrote:

 +# Disable libmpx on unsupported systems.
 +if test -d ${srcdir}/libmpx; then
 +if test x$enable_libmpx = x; then
 +   AC_MSG_CHECKING([for libmpx support])
 +   if (srcdir=${srcdir}/libmpx; \
 +   . ${srcdir}/configure.tgt; \
 +   test $LIBMPX_SUPPORTED != yes)
 +   then
 +   AC_MSG_RESULT([no])
 +   noconfigdirs=$noconfigdirs target-libmpx
 +   else
 +   AC_MSG_RESULT([yes])
 +   fi
 +fi
 +fi

Using this at toplevel you can only enable or disable for all multilibs - 
this code runs just once.  But:

 diff --git a/libmpx/configure.tgt b/libmpx/configure.tgt

 +LIBMPX_SUPPORTED=no
 +case ${target} in
 +  x86_64-*-linux* | i?86-*-linux*)
 + # X32 doesn't support MPX.
 + echo int i[sizeof (void *) == 4 ? 1 : -1] = { __x86_64__ };  
 conftestx.c
 + if ${CC} ${CFLAGS} -c -o conftestx.o conftestx.c  /dev/null 21; then
 + LIBMPX_SUPPORTED=no
 + else
 + LIBMPX_SUPPORTED=yes
 + fi
 + ;;

Here you are testing something per-multilib.  Furthermore, using CC and 
CFLAGS here is wrong when used by toplevel configure, as at toplevel 
they'll relate to the host rather than the target.

So you need to separate the two tests of (a) might MPX be supported for 
some multilib on the target (usable at toplevel, not using CC or CFLAGS) 
and (b) is it supported for the present multilib.  Thus the libmpx 
directory will need configuring for all multilibs for supported targets.  
Then, if the particular multilib doesn't support it (i.e. is x32), you'll 
need to arrange for the directory not to build or install anything - much 
like the libquadmath directory gets configured in some cases where it 
doesn't build anything.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-21 Thread Joseph Myers 
On Wed, 19 Nov 2014, Ilya Enkovich wrote:

  If it's intended only as the latter - and this is documented - then you 
  don't have the libgcc-like requirements, and there's no point in having 
  multiple libraries.  If it's intended for both, that points the way to 
  separate libraries (where the debugging-tool library would be used in that 
  use case, but the crash-reporter-interception case would expect programs 
  to have been linked with only the minimal library).
 
 Would it be enough to mention implicit link and resulting pthread 
 dependency in -fcheck-pointer-bounds description in invoke.texi?

Well, that and say explicitly that it's intended as a debugging tool 
rather than for general hardening.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-19 Thread Ilya Enkovich 
On 13 Nov 20:56, Joseph Myers wrote:
 On Thu, 13 Nov 2014, Ilya Enkovich wrote:
 
   You can leave it as a single library - it's just that imposes libgcc-like
   constraints on what the library does and how it does things, so as to be
   usable for arbitrary programs built with MPX (e.g. using
   reserved-namespace names such as __write when available - direct syscalls
   may also be a possibility in some cases).
  
  I really don't want to make it libgcc-like and complicate its further
  development.  What is the reason for that?  Is it because library is
  linked by -mmpx?  Can it be avoided if library is linked only at
  '-fcheck-pointer-bounds -mmpx' combination?  If single -mmpx is used
 
 What's -fcheck-pointer-bounds?  I don't see any documentation of it in 
 invoke.texi.  Any options intended for users to use need documenting there 
 - did a documentation patch get missed out from the series of patches 
 posted / committed?
 
 Documentation might help understand the intent of the feature.  I think 
 there are two natural possibilities:
 
 * Use as a lightweight hardening feature enabled by default across a whole 
 distribution (or for a subset of the distribution considered 
 security-critical), by people who don't know the detailed requirements of 
 individual programs built with the option.  In that case there are 
 libgcc-like requirements (as another issue there, it might be problematic 
 to cause arbitrary previously unthreaded programs to link with libpthread; 
 at least, performance issues seem possible).  Outputting reports to files 
 determined by environment variables is probably not a sensible approach in 
 this case.  You might want reports to syslog, or might want MPX-detected 
 issues to be intercepted by crash reporters such as apport so they can 
 collect relevant information and offer to report it to the distributor.
 
 * Use as a debugging tool, by people who understand the requirements of 
 their application / library and can tell whether the things the MPX 
 library is documented as doing might be a problem in their case.  For 
 this, libgcc-like requirements don't apply and output to files seems more 
 sensible.

I think if we compare runtime libraries for these two usage models then we 
don't find much in common.  Thus it should be two independent runtime 
libraries.  This posted library corresponds to the second scenario and is to be 
used mainly for debugging. 

 
 If it's intended only as the latter - and this is documented - then you 
 don't have the libgcc-like requirements, and there's no point in having 
 multiple libraries.  If it's intended for both, that points the way to 
 separate libraries (where the debugging-tool library would be used in that 
 use case, but the crash-reporter-interception case would expect programs 
 to have been linked with only the minimal library).

Would it be enough to mention implicit link and resulting pthread dependency in 
-fcheck-pointer-bounds description in invoke.texi?

 
 -- 
 Joseph S. Myers
 jos...@codesourcery.com

Here is an updated version.  I removed printf usage in a signal handler, added 
symbol versioning, added thread safety comments for static vars, added static 
link support and -static-libmpx option (will update corresponding documentation 
patch).  Does it look OK?

Thanks,
Ilya
--
2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

* gcc.c (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(MPX_SPEC): New.
(LINK_COMMAND_SPEC): Add MPX_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/

2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.


diff --git a/Makefile.def b/Makefile.def
index 40bbca9..4a535d2 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -128,6 +128,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index b27fb1d..ccb119b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -642,6 +643,25 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Disable libmpx on unsupported systems.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = x; then
+   AC_MSG_CHECKING([for libmpx support])
+   if

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-19 Thread Jeff Law 

On 11/19/14 07:15, Ilya Enkovich wrote:


--
2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

* gcc.c (LIBMPX_LIBS): New.
(LIBMPX_SPEC): New.
(MPX_SPEC): New.
(LINK_COMMAND_SPEC): Add MPX_SPEC.
* c-family/c.opt (static-libmpx): New.

libmpx/

2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.
So I have only done a cursory peek at this code, but one thing which I 
did immediately note was the CPU feature testing stuff.  Shouldn't all 
that stuff be integrated into the feature testing bits already found in 
libgcc?


I've asked the steering committee to vote on accepting the runtime -- 
necessary given Intel is keeping copyright ownership to the best of my 
knowledge.


Jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-19 Thread Ilya Enkovich 
2014-11-19 20:55 GMT+03:00 Jeff Law l...@redhat.com:
 On 11/19/14 07:15, Ilya Enkovich wrote:

 --
 2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

 * Makefile.def: Add libmpx.
 * configure.ac: Add libmpx.
 * Makefile.in: Regenerate.
 * configure: Regenerate.

 gcc/

 2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

 * gcc.c (LIBMPX_LIBS): New.
 (LIBMPX_SPEC): New.
 (MPX_SPEC): New.
 (LINK_COMMAND_SPEC): Add MPX_SPEC.
 * c-family/c.opt (static-libmpx): New.

 libmpx/

 2014-11-19  Ilya Enkovich  ilya.enkov...@intel.com

 Initial commit.

 So I have only done a cursory peek at this code, but one thing which I did
 immediately note was the CPU feature testing stuff.  Shouldn't all that
 stuff be integrated into the feature testing bits already found in libgcc?

I'll have a look at these features.


 I've asked the steering committee to vote on accepting the runtime --
 necessary given Intel is keeping copyright ownership to the best of my
 knowledge.

Thanks!

Ilya


 Jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-13 Thread Ilya Enkovich 
2014-11-13 2:03 GMT+03:00 Joseph Myers jos...@codesourcery.com:
 On Thu, 13 Nov 2014, Ilya Enkovich wrote:

 It's hard to decide which of runtime functionality should be
 considered as basic and how it should be used.  We may say that the
 only basic thing is hardware enabling which is enable_mpx and stop
 here.  But then you get minimal but quite useless library.  Yes, it
 can enable MPX and thus make bounds violation to interrupt a program.
 But users cannot enable/disable MPX dinamycally then.  Also they
 cannot configure it.  Thus either control via environmental variables
 appears in this core library or we transform initialization function
 from constructor to interface function and use it from another
 extended MPX library which support environment variables, logging etc.
 But the core library will only be used by this extended MPX library
 and nothing else. So why not to leave it as a single library as it is?

 You can leave it as a single library - it's just that imposes libgcc-like
 constraints on what the library does and how it does things, so as to be
 usable for arbitrary programs built with MPX (e.g. using
 reserved-namespace names such as __write when available - direct syscalls
 may also be a possibility in some cases).

I really don't want to make it libgcc-like and complicate its further
development.  What is the reason for that?  Is it because library is
linked by -mmpx?  Can it be avoided if library is linked only at
'-fcheck-pointer-bounds -mmpx' combination?  If single -mmpx is used
then compiler actually doesn't generate any MPX instructions and user
doesn't even have builtins to do that.  The only option for user to
generate mpx instructions without pointer bounds checker is to write
inline assembler.  In this case he should be prepared enough to enable
MPX in his assembler :) Alternatively enable_mpx function may be put
into i386 intrin headers and used manually in programs by including
immintrin.h.  Does it sound like a viable option?

Thanks,
Ilya


 --
 Joseph S. Myers
 jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-13 Thread Joseph Myers 
On Thu, 13 Nov 2014, Ilya Enkovich wrote:

  You can leave it as a single library - it's just that imposes libgcc-like
  constraints on what the library does and how it does things, so as to be
  usable for arbitrary programs built with MPX (e.g. using
  reserved-namespace names such as __write when available - direct syscalls
  may also be a possibility in some cases).
 
 I really don't want to make it libgcc-like and complicate its further
 development.  What is the reason for that?  Is it because library is
 linked by -mmpx?  Can it be avoided if library is linked only at
 '-fcheck-pointer-bounds -mmpx' combination?  If single -mmpx is used

What's -fcheck-pointer-bounds?  I don't see any documentation of it in 
invoke.texi.  Any options intended for users to use need documenting there 
- did a documentation patch get missed out from the series of patches 
posted / committed?

Documentation might help understand the intent of the feature.  I think 
there are two natural possibilities:

* Use as a lightweight hardening feature enabled by default across a whole 
distribution (or for a subset of the distribution considered 
security-critical), by people who don't know the detailed requirements of 
individual programs built with the option.  In that case there are 
libgcc-like requirements (as another issue there, it might be problematic 
to cause arbitrary previously unthreaded programs to link with libpthread; 
at least, performance issues seem possible).  Outputting reports to files 
determined by environment variables is probably not a sensible approach in 
this case.  You might want reports to syslog, or might want MPX-detected 
issues to be intercepted by crash reporters such as apport so they can 
collect relevant information and offer to report it to the distributor.

* Use as a debugging tool, by people who understand the requirements of 
their application / library and can tell whether the things the MPX 
library is documented as doing might be a problem in their case.  For 
this, libgcc-like requirements don't apply and output to files seems more 
sensible.

If it's intended only as the latter - and this is documented - then you 
don't have the libgcc-like requirements, and there's no point in having 
multiple libraries.  If it's intended for both, that points the way to 
separate libraries (where the debugging-tool library would be used in that 
use case, but the crash-reporter-interception case would expect programs 
to have been linked with only the minimal library).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Richard Biener 
On Tue, Nov 11, 2014 at 4:34 PM, Ilya Enkovich enkovich@gmail.com wrote:
 Hi,

 This patch integrates MPX runtime library into GCC source tree.  MPX runtime 
 is responsible for initialization of MPX feature in HW, signal handling, 
 reporting etc.  Library is linked to codes compiled with -mmpx.

What happens if you omit -mmpx from the linker command-line?
What happens if you mix -mmpx and non -mmpx objects?

I wonder why this is not part of glibc and whether initialization is needed
is communicated by some ELF flag in the executable/library which
is determined by the linker.

Richard.

 Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

 Thanks,
 Ilya
 --
 2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

 * Makefile.def: Add libmpx.
 * configure.ac: Add libmpx.
 * Makefile.in: Regenerate.
 * configure: Regenerate.

 gcc/

 2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

 * gcc.c (MPX_SPEC): New.
 (LINK_COMMAND_SPEC): Add MPX_SPEC.

 libmpx/

 2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

 Initial commit.

 diff --git a/Makefile.def b/Makefile.def
 index dcbcd08..49e3c6f 100644
 --- a/Makefile.def
 +++ b/Makefile.def
 @@ -133,6 +133,9 @@ target_modules = { module= libsanitizer;
bootstrap=true;
lib_path=.libs;
raw_cxx=true; };
 +target_modules = { module= libmpx;
 +  bootstrap=true;
 +  lib_path=.libs; };
  target_modules = { module= libvtv;
bootstrap=true;
lib_path=.libs;
 diff --git a/configure.ac b/configure.ac
 index 2f0af4a..95fbab1 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -162,6 +162,7 @@ target_libraries=target-libgcc \
 target-libstdc++-v3 \
 target-libsanitizer \
 target-libvtv \
 +   target-libmpx \
 target-libssp \
 target-libquadmath \
 target-libgfortran \
 @@ -572,6 +573,25 @@ if test -d ${srcdir}/libvtv; then
  fi
  fi

 +
 +# Disable libmpx on unsupported systems.
 +if test -d ${srcdir}/libmpx; then
 +if test x$enable_libmpx = x; then
 +   AC_MSG_CHECKING([for libmpx support])
 +   if (srcdir=${srcdir}/libmpx; \
 +   . ${srcdir}/configure.tgt; \
 +   test $LIBMPX_SUPPORTED != yes)
 +   then
 +   AC_MSG_RESULT([no])
 +   noconfigdirs=$noconfigdirs target-libmpx
 +   else
 +   AC_MSG_RESULT([yes])
 +   fi
 +fi
 +fi
 +
 +
 +
  # Disable libquadmath for some systems.
  case ${target} in
avr-*-*)
 @@ -2612,6 +2632,11 @@ if echo  ${target_configdirs}  | grep  libvtv   
 /dev/null 21 
bootstrap_target_libs=${bootstrap_target_libs}target-libvtv,
  fi

 +# If we are building libmpx, bootstrap it.
 +if echo  ${target_configdirs}  | grep  libmpx   /dev/null 21; then
 +  bootstrap_target_libs=${bootstrap_target_libs}target-libmpx,
 +fi
 +
  # Determine whether gdb needs tk/tcl or not.
  # Use 'maybe' since enable_gdbtk might be true even if tk isn't available
  # and in that case we want gdb to be built without tk.  Ugh!
 diff --git a/gcc/gcc.c b/gcc/gcc.c
 index e013d52..200704b 100644
 --- a/gcc/gcc.c
 +++ b/gcc/gcc.c
 @@ -804,6 +804,13 @@ proper position among the other output files.  */
  %{fvtable-verify=preinit: -lvtv -u_vtable_map_vars_start 
 -u_vtable_map_vars_end}}
  #endif

 +#ifndef MPX_SPEC
 +#define MPX_SPEC \
 +%{!nostdlib:%{!nodefaultlibs:%{mmpx:\
 +%{static:%nMPX runtime is disabled due to -static used}\
 +%{!static:-lmpx
 +#endif
 +
  /* -u* was put back because both BSD and SysV seem to support it.  */
  /* %{static:} simply prevents an error message if the target machine
 doesn't handle -static.  */
 @@ -824,6 +831,7 @@ proper position among the other output files.  */
 %X %{o*} %{e*} %{N} %{n} %{r}\
  %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}  
 VTABLE_VERIFICATION_SPEC  \
  %{static:} %{L*} %(mfwrap) %(link_libgcc)  SANITIZER_EARLY_SPEC  %o\
 + MPX_SPEC  \
  %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
  %{fcilkplus:%:include(libcilkrts.spec)%(link_cilkrts)}\
  %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
 diff --git a/libmpx/ChangeLog b/libmpx/ChangeLog
 new file mode 100644
 index 000..2e2ea92
 --- /dev/null
 +++ b/libmpx/ChangeLog
 @@ -0,0 +1,3 @@
 +2014-11-10  Ilya Enkovich  ilya.enkov...@intel.com
 +
 +   Initial checkin.
 diff --git a/libmpx/Makefile.am b/libmpx/Makefile.am
 new file mode 100644
 index 000..c6b479f
 --- /dev/null
 +++ b/libmpx/Makefile.am
 @@ -0,0 +1,41 @@
 +ACLOCAL_AMFLAGS = -I .. -I ../config
 +
 +SUBDIRS = mpxrt
 +
 +# Work around what appears to be a GNU make bug handling MAKEFLAGS
 +# values defined in terms of make variables, as is the case for CC and
 +# friends when we are called from the top level Makefile.
 +AM_MAKEFLAGS = \
 +

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Jakub Jelinek 
On Wed, Nov 12, 2014 at 09:30:59AM +0100, Richard Biener wrote:
 On Tue, Nov 11, 2014 at 4:34 PM, Ilya Enkovich enkovich@gmail.com wrote:
  Hi,
 
  This patch integrates MPX runtime library into GCC source tree.  MPX 
  runtime is responsible for initialization of MPX feature in HW, signal 
  handling, reporting etc.  Library is linked to codes compiled with -mmpx.
 
 What happens if you omit -mmpx from the linker command-line?
 What happens if you mix -mmpx and non -mmpx objects?
 
 I wonder why this is not part of glibc and whether initialization is needed
 is communicated by some ELF flag in the executable/library which
 is determined by the linker.

Because glibc is not a kitchen sink.  And, I think most of programs in
typical uses aren't going to be built with -mmpx, so it isn't appropriate
to pay the price in all programs for something used only sometimes.

Jakub

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Ilya Enkovich 
On 11 Nov 20:25, Joseph Myers wrote:
 On Tue, 11 Nov 2014, Andi Kleen wrote:
 
  Joseph Myers jos...@codesourcery.com writes:
  
   On Tue, 11 Nov 2014, Ilya Enkovich wrote:
  
   Hi,
   
   This patch integrates MPX runtime library into GCC source tree.  MPX 
   runtime is responsible for initialization of MPX feature in HW, signal 
   handling, reporting etc.  Library is linked to codes compiled with 
   -mmpx.
   
   Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?
  
   Please use symbol versioning to control the set of exports for the 
   library; only symbols explicitly listed to be exported at a given symbol 
   version should be exported.
  
  Also I think you need some user documentation on the implications of the
  overriding of sigaction. If someone else wants to do similar tricks (and
  I'm sure some do) there would be conflicts.
 
 Indeed, I don't understand what the purpose of this overriding is.  What 
 goes wrong if you just use default signal handling in a program built with 
 MPX, or don't handle any environment variables specially at all?  That is, 
 why does this need to be linked with all programs built with MPX at all, 
 rather than being optional functionality in an independent library that 
 people can choose to build and link with if they want that extra 
 functionality?

I think there was a historical reason to make such sigaction chain.  The same 
signal is used to report bounds violation and absence of Bounds Table.  
Initiail runtime version took care of it and managed Bounds Tables.  User may 
want to catch and handle bounds violation by himsel but doubtedly he wants to 
manage Bounds Tables as well.  Thus such chain was usefull.

Currently Bounds Table management is performed in kernel and signal handler 
only handles bounds violation.  It means I may just register MPX runtime 
handler as a default one and if user wants to override it later - it's OK.  I 
remove these mpxrt-sigaction.* files from runtime and just use regular 
sigaction.

BTW no sigaction override means it should work for -static.  I'll test it and 
fix link specs appropriately soon.

MPX runtime needs to be linked with programs using MPX because it initializes 
hardware.  Without it all MPX instructions are just NOPs.  Thus it's not an 
extra functionality, but is for basic MPX functionality.

 
 My starting point is that MPX provides functionality for detecting certain 
 cases of undefined behavior in C that wouldn't otherwise be detected, and 
 it should be possible to use with any C program (including ISO C ones that 
 don't use POSIX interfaces such as sigaction and may use the sigaction 
 identifier for some other purpose).
 
 Another issue: handle_sigsegv calls functions documented in the glibc 
 manual as AS-unsafe, such as pthread_setspecific and (via do_exit) 
 fprintf.  You need to make sure everything called from a signal handler is 
 AS-safe.
 
 -- 
 Joseph S. Myers
 jos...@codesourcery.com

I also fixed other issues you mentioned in your previous comments.  Below is a 
new version.  Does it look better?

Thanks,
Ilya
--
2014-11-12  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-11-12  Ilya Enkovich  ilya.enkov...@intel.com

* gcc.c (MPX_SPEC): New.
(LINK_COMMAND_SPEC): Add MPX_SPEC.

libmpx/

2014-11-12  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.


diff --git a/Makefile.def b/Makefile.def
index dcbcd08..49e3c6f 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -133,6 +133,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index 2f0af4a..95fbab1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -572,6 +573,25 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Disable libmpx on unsupported systems.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = x; then
+   AC_MSG_CHECKING([for libmpx support])
+   if (srcdir=${srcdir}/libmpx; \
+   . ${srcdir}/configure.tgt; \
+   test $LIBMPX_SUPPORTED != yes)
+   then
+   AC_MSG_RESULT([no])
+   noconfigdirs=$noconfigdirs target-libmpx
+   else
+   AC_MSG_RESULT([yes])
+   fi
+fi
+fi
+
+
+
 # Disable libquadmath for some systems.
 case

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Jeff Law 

On 11/11/14 13:22, Ilya Enkovich wrote:



This library (some version of it) was only published in a binary form
some time ago.  AFAIK currently it is not a part of any product.
Sources were never made public before.  Sources go under the same
license as Cilk runtime and therefore require approval from steering
committee.  Should I do something to start that process?

Just want to make sure I understand the issues in play.

It sounds like GCC should be considered the canonical repository.

The code has the same copyright as Cilk+ runtime, which has already been 
OK'd.


The code is still copyrighted by Intel

Assuming I've got those key items correct, I'll raise it with the 
steering committee.  We've dealt with all the kind of stuff before, so 
it ought to be something we can work through.


jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Joseph Myers 
On Wed, 12 Nov 2014, Ilya Enkovich wrote:

 MPX runtime needs to be linked with programs using MPX because it 
 initializes hardware.  Without it all MPX instructions are just NOPs.  
 Thus it's not an extra functionality, but is for basic MPX 
 functionality.

So what if you just have the constructor that initializes the hardware - 
could everything else (handling environment variables, printing more 
detailed diagnostics, etc.) be separate?  How much is basic MPX 
functionality, how much is extra?  Basic functionality should arguably be 
like libgcc in namespace terms (so not calling any libc functions outside 
of ISO C90 namespace, using reserved-namespace versions such as __write 
instead if necessary and if such versions are available); extra 
functionality need not be so restricted.

 I also fixed other issues you mentioned in your previous comments.  
 Below is a new version.  Does it look better?

I don't see anything obvious to do symbol versioning.

If this isn't providing interfaces for the user program or 
compiler-generated code to call, then the symbol versioning could hide all 
symbols (so there are no exported symbols at all and the library operates 
purely through its initialization constructor).  Or build with 
-fvisibility=hidden to hide the symbols that way.  Either way, the dynamic 
symbol table of the shared library would end up with just undefined 
symbols, nothing exported by the library.

The issue with signal handlers calling inappropriate functions still 
applies - you're calling vfprintf from a signal handler.  Using 
pthread_mutex_lock around it doesn't help at all; vfprintf can call 
malloc, and the signal may have interrupted malloc, for example.  You 
really can't use stdio at all inside signal handlers - if you want to do 
I/O in them, you have to use write ().  (There's an argument that dprintf 
or an snprintf/write combination ought to be AS-safe, but currently they 
aren't; see https://sourceware.org/bugzilla/show_bug.cgi?id=16060; 
snprintf is probably safe than dprintf in practice.)

I mentioned the question of static writable variables.  If those variables 
are only ever modified at startup, before any threads have been created, 
could you add comments to that effect (and otherwise ensure comments on 
the variables explain how you ensure they are accessed in a thread-safe 
way)?

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Ilya Enkovich 
2014-11-13 0:40 GMT+03:00 Joseph Myers jos...@codesourcery.com:
 On Wed, 12 Nov 2014, Ilya Enkovich wrote:

 MPX runtime needs to be linked with programs using MPX because it
 initializes hardware.  Without it all MPX instructions are just NOPs.
 Thus it's not an extra functionality, but is for basic MPX
 functionality.

 So what if you just have the constructor that initializes the hardware -
 could everything else (handling environment variables, printing more
 detailed diagnostics, etc.) be separate?  How much is basic MPX
 functionality, how much is extra?  Basic functionality should arguably be
 like libgcc in namespace terms (so not calling any libc functions outside
 of ISO C90 namespace, using reserved-namespace versions such as __write
 instead if necessary and if such versions are available); extra
 functionality need not be so restricted.

It's hard to decide which of runtime functionality should be
considered as basic and how it should be used.  We may say that the
only basic thing is hardware enabling which is enable_mpx and stop
here.  But then you get minimal but quite useless library.  Yes, it
can enable MPX and thus make bounds violation to interrupt a program.
But users cannot enable/disable MPX dinamycally then.  Also they
cannot configure it.  Thus either control via environmental variables
appears in this core library or we transform initialization function
from constructor to interface function and use it from another
extended MPX library which support environment variables, logging etc.
But the core library will only be used by this extended MPX library
and nothing else. So why not to leave it as a single library as it is?


 I also fixed other issues you mentioned in your previous comments.
 Below is a new version.  Does it look better?

 I don't see anything obvious to do symbol versioning.

 If this isn't providing interfaces for the user program or
 compiler-generated code to call, then the symbol versioning could hide all
 symbols (so there are no exported symbols at all and the library operates
 purely through its initialization constructor).  Or build with
 -fvisibility=hidden to hide the symbols that way.  Either way, the dynamic
 symbol table of the shared library would end up with just undefined
 symbols, nothing exported by the library.

Currently it's just constructors.  I'll hide functions.


 The issue with signal handlers calling inappropriate functions still
 applies - you're calling vfprintf from a signal handler.  Using
 pthread_mutex_lock around it doesn't help at all; vfprintf can call
 malloc, and the signal may have interrupted malloc, for example.  You
 really can't use stdio at all inside signal handlers - if you want to do
 I/O in them, you have to use write ().  (There's an argument that dprintf
 or an snprintf/write combination ought to be AS-safe, but currently they
 aren't; see https://sourceware.org/bugzilla/show_bug.cgi?id=16060;
 snprintf is probably safe than dprintf in practice.)

I'll get rid of printf there. Will use write + own int to string
convert to avoid snprintf.


 I mentioned the question of static writable variables.  If those variables
 are only ever modified at startup, before any threads have been created,
 could you add comments to that effect (and otherwise ensure comments on
 the variables explain how you ensure they are accessed in a thread-safe
 way)?

Almost all varibles are written at process initialization only.
[out|err]_file_dirty may be written by different threads in
__mpxrt_print but the same value is always written and value is read
in process finalizer only.  Since __mpxrt_print has mutex lock anyway,
I may move the rest of fucntion body under the lock.  Remaining var is
bounds violation counter num_bnd_chk which is written in signal
handler only and read in the process finalizer.  So I believe all vars
are thread safe.  Will add appropriate comments in the code.

Thanks a lot for detailed review!
Ilya


 --
 Joseph S. Myers
 jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-12 Thread Joseph Myers 
On Thu, 13 Nov 2014, Ilya Enkovich wrote:

 It's hard to decide which of runtime functionality should be
 considered as basic and how it should be used.  We may say that the
 only basic thing is hardware enabling which is enable_mpx and stop
 here.  But then you get minimal but quite useless library.  Yes, it
 can enable MPX and thus make bounds violation to interrupt a program.
 But users cannot enable/disable MPX dinamycally then.  Also they
 cannot configure it.  Thus either control via environmental variables
 appears in this core library or we transform initialization function
 from constructor to interface function and use it from another
 extended MPX library which support environment variables, logging etc.
 But the core library will only be used by this extended MPX library
 and nothing else. So why not to leave it as a single library as it is?

You can leave it as a single library - it's just that imposes libgcc-like 
constraints on what the library does and how it does things, so as to be 
usable for arbitrary programs built with MPX (e.g. using 
reserved-namespace names such as __write when available - direct syscalls 
may also be a possibility in some cases).

-- 
Joseph S. Myers
jos...@codesourcery.com

[PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Ilya Enkovich 
Hi,

This patch integrates MPX runtime library into GCC source tree.  MPX runtime is 
responsible for initialization of MPX feature in HW, signal handling, reporting 
etc.  Library is linked to codes compiled with -mmpx.

Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

Thanks,
Ilya
--
2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

* gcc.c (MPX_SPEC): New.
(LINK_COMMAND_SPEC): Add MPX_SPEC.

libmpx/

2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

Initial commit.

diff --git a/Makefile.def b/Makefile.def
index dcbcd08..49e3c6f 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -133,6 +133,9 @@ target_modules = { module= libsanitizer;
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
diff --git a/configure.ac b/configure.ac
index 2f0af4a..95fbab1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,7 @@ target_libraries=target-libgcc \
target-libstdc++-v3 \
target-libsanitizer \
target-libvtv \
+   target-libmpx \
target-libssp \
target-libquadmath \
target-libgfortran \
@@ -572,6 +573,25 @@ if test -d ${srcdir}/libvtv; then
 fi
 fi
 
+
+# Disable libmpx on unsupported systems.
+if test -d ${srcdir}/libmpx; then
+if test x$enable_libmpx = x; then
+   AC_MSG_CHECKING([for libmpx support])
+   if (srcdir=${srcdir}/libmpx; \
+   . ${srcdir}/configure.tgt; \
+   test $LIBMPX_SUPPORTED != yes)
+   then
+   AC_MSG_RESULT([no])
+   noconfigdirs=$noconfigdirs target-libmpx
+   else
+   AC_MSG_RESULT([yes])
+   fi
+fi
+fi
+
+
+
 # Disable libquadmath for some systems.
 case ${target} in
   avr-*-*)
@@ -2612,6 +2632,11 @@ if echo  ${target_configdirs}  | grep  libvtv   
/dev/null 21 
   bootstrap_target_libs=${bootstrap_target_libs}target-libvtv,
 fi
 
+# If we are building libmpx, bootstrap it.
+if echo  ${target_configdirs}  | grep  libmpx   /dev/null 21; then
+  bootstrap_target_libs=${bootstrap_target_libs}target-libmpx,
+fi
+
 # Determine whether gdb needs tk/tcl or not.
 # Use 'maybe' since enable_gdbtk might be true even if tk isn't available
 # and in that case we want gdb to be built without tk.  Ugh!
diff --git a/gcc/gcc.c b/gcc/gcc.c
index e013d52..200704b 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -804,6 +804,13 @@ proper position among the other output files.  */
 %{fvtable-verify=preinit: -lvtv -u_vtable_map_vars_start 
-u_vtable_map_vars_end}}
 #endif
 
+#ifndef MPX_SPEC
+#define MPX_SPEC \
+%{!nostdlib:%{!nodefaultlibs:%{mmpx:\
+%{static:%nMPX runtime is disabled due to -static used}\
+%{!static:-lmpx
+#endif
+
 /* -u* was put back because both BSD and SysV seem to support it.  */
 /* %{static:} simply prevents an error message if the target machine
doesn't handle -static.  */
@@ -824,6 +831,7 @@ proper position among the other output files.  */
%X %{o*} %{e*} %{N} %{n} %{r}\
 %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}  
VTABLE_VERIFICATION_SPEC  \
 %{static:} %{L*} %(mfwrap) %(link_libgcc)  SANITIZER_EARLY_SPEC  %o\
+ MPX_SPEC  \
 %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
 %{fcilkplus:%:include(libcilkrts.spec)%(link_cilkrts)}\
 %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
diff --git a/libmpx/ChangeLog b/libmpx/ChangeLog
new file mode 100644
index 000..2e2ea92
--- /dev/null
+++ b/libmpx/ChangeLog
@@ -0,0 +1,3 @@
+2014-11-10  Ilya Enkovich  ilya.enkov...@intel.com
+
+   Initial checkin.
diff --git a/libmpx/Makefile.am b/libmpx/Makefile.am
new file mode 100644
index 000..c6b479f
--- /dev/null
+++ b/libmpx/Makefile.am
@@ -0,0 +1,41 @@
+ACLOCAL_AMFLAGS = -I .. -I ../config
+
+SUBDIRS = mpxrt
+
+# Work around what appears to be a GNU make bug handling MAKEFLAGS
+# values defined in terms of make variables, as is the case for CC and
+# friends when we are called from the top level Makefile.
+AM_MAKEFLAGS = \
+   AR_FLAGS=$(AR_FLAGS) \
+   CC_FOR_BUILD=$(CC_FOR_BUILD) \
+   CFLAGS=$(CFLAGS) \
+   CXXFLAGS=$(CXXFLAGS) \
+   CFLAGS_FOR_BUILD=$(CFLAGS_FOR_BUILD) \
+   CFLAGS_FOR_TARGET=$(CFLAGS_FOR_TARGET) \
+   INSTALL=$(INSTALL) \
+   INSTALL_DATA=$(INSTALL_DATA) \
+   INSTALL_PROGRAM=$(INSTALL_PROGRAM) \
+   INSTALL_SCRIPT=$(INSTALL_SCRIPT) \
+   JC1FLAGS=$(JC1FLAGS) \
+   LDFLAGS=$(LDFLAGS) \
+   LIBCFLAGS=$(LIBCFLAGS) \
+

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Joseph Myers 
On Tue, 11 Nov 2014, Ilya Enkovich wrote:

 Hi,
 
 This patch integrates MPX runtime library into GCC source tree.  MPX 
 runtime is responsible for initialization of MPX feature in HW, signal 
 handling, reporting etc.  Library is linked to codes compiled with 
 -mmpx.
 
 Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

Please use symbol versioning to control the set of exports for the 
library; only symbols explicitly listed to be exported at a given symbol 
version should be exported.

If the library is intended to be used implicitly by compiler-generated 
code rather than directly by users, all exported symbols should be in the 
implementation namespace - only symbols intended to be called by users 
should not start with __.

In any case, symbols not intended to be exported should be in the 
implementation namespace, or static, to avoid namespace issues for static 
linking.

The code contains unchecked malloc calls.  You always need to allow for 
errors from library functions.

The code contains unchecked sprintf to static buffers using values from 
the environment.  Use snprintf.

The code contains calls to getenv to choose files to open.  You need to 
use __secure_getenv / secure_getenv when available (probably for all 
getenv calls, unless you have a clear reason certain calls are safe even 
for setuid programs).

You have lots of static writable variables.  Are you sure all those 
variables are handled in a thread-safe way (e.g. only modified before any 
threads start)?

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Joseph Myers 
On Tue, 11 Nov 2014, Joseph Myers wrote:

 You have lots of static writable variables.  Are you sure all those 
 variables are handled in a thread-safe way (e.g. only modified before any 
 threads start)?

Another general multi-thread issue for libraries: files should be opened 
with O_CLOEXEC (that's the e flag to fopen when using glibc) unless you 
have a clear reason not to do so.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Andi Kleen 
Joseph Myers jos...@codesourcery.com writes:

 On Tue, 11 Nov 2014, Ilya Enkovich wrote:

 Hi,
 
 This patch integrates MPX runtime library into GCC source tree.  MPX 
 runtime is responsible for initialization of MPX feature in HW, signal 
 handling, reporting etc.  Library is linked to codes compiled with 
 -mmpx.
 
 Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

 Please use symbol versioning to control the set of exports for the 
 library; only symbols explicitly listed to be exported at a given symbol 
 version should be exported.

Also I think you need some user documentation on the implications of the
overriding of sigaction. If someone else wants to do similar tricks (and
I'm sure some do) there would be conflicts.

Really it would be far better if the signal handler chaining was done
in glibc.

-Andi


-- 
a...@linux.intel.com -- Speaking for myself only

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Jeff Law 

On 11/11/14 08:34, Ilya Enkovich wrote:

Hi,

This patch integrates MPX runtime library into GCC source tree.  MPX runtime is 
responsible for initialization of MPX feature in HW, signal handling, reporting 
etc.  Library is linked to codes compiled with -mmpx.

Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

Thanks,
Ilya
--
2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

* Makefile.def: Add libmpx.
* configure.ac: Add libmpx.
* Makefile.in: Regenerate.
* configure: Regenerate.

gcc/

2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

* gcc.c (MPX_SPEC): New.
(LINK_COMMAND_SPEC): Add MPX_SPEC.
Separate from Joseph's comments, is this library strictly a part of GCC, 
or is it shared with other implementations such as Clang/LLVM or ICC? 
If shared, which copy is the master?  If shared, where should patches 
for the library be posted?


I haven't looked closely, but what license is the actual runtime covered 
by?  BSD-ish, LGPL, something else?


Depending on the answers, we may need approval from the steering 
committee to accept.  It's a process we've worked through for various 
other libraries (Cilk, Sanitizers, etc).



Jeff

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread H.J. Lu 
On Tue, Nov 11, 2014 at 10:49 AM, Andi Kleen a...@firstfloor.org wrote:
 Joseph Myers jos...@codesourcery.com writes:

 On Tue, 11 Nov 2014, Ilya Enkovich wrote:

 Hi,

 This patch integrates MPX runtime library into GCC source tree.  MPX
 runtime is responsible for initialization of MPX feature in HW, signal
 handling, reporting etc.  Library is linked to codes compiled with
 -mmpx.

 Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

 Please use symbol versioning to control the set of exports for the
 library; only symbols explicitly listed to be exported at a given symbol
 version should be exported.

 Also I think you need some user documentation on the implications of the
 overriding of sigaction. If someone else wants to do similar tricks (and
 I'm sure some do) there would be conflicts.

 Really it would be far better if the signal handler chaining was done
 in glibc.


It is similar to libsanitizer.  Put it in glibc isn't going to work well
for MPX.

-- 
H.J.

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Ilya Enkovich 
2014-11-11 23:02 GMT+03:00 Jeff Law l...@redhat.com:
 On 11/11/14 08:34, Ilya Enkovich wrote:

 Hi,

 This patch integrates MPX runtime library into GCC source tree.  MPX
 runtime is responsible for initialization of MPX feature in HW, signal
 handling, reporting etc.  Library is linked to codes compiled with -mmpx.

 Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?

 Thanks,
 Ilya
 --
 2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

 * Makefile.def: Add libmpx.
 * configure.ac: Add libmpx.
 * Makefile.in: Regenerate.
 * configure: Regenerate.

 gcc/

 2014-11-11  Ilya Enkovich  ilya.enkov...@intel.com

 * gcc.c (MPX_SPEC): New.
 (LINK_COMMAND_SPEC): Add MPX_SPEC.

 Separate from Joseph's comments, is this library strictly a part of GCC, or
 is it shared with other implementations such as Clang/LLVM or ICC? If
 shared, which copy is the master?  If shared, where should patches for the
 library be posted?

 I haven't looked closely, but what license is the actual runtime covered by?
 BSD-ish, LGPL, something else?

 Depending on the answers, we may need approval from the steering committee
 to accept.  It's a process we've worked through for various other libraries
 (Cilk, Sanitizers, etc).


 Jeff

This library (some version of it) was only published in a binary form
some time ago.  AFAIK currently it is not a part of any product.
Sources were never made public before.  Sources go under the same
license as Cilk runtime and therefore require approval from steering
committee.  Should I do something to start that process?

Thanks,
Ilya

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Ilya Enkovich 
2014-11-11 21:09 GMT+03:00 Joseph Myers jos...@codesourcery.com:
 On Tue, 11 Nov 2014, Joseph Myers wrote:

 You have lots of static writable variables.  Are you sure all those
 variables are handled in a thread-safe way (e.g. only modified before any
 threads start)?

 Another general multi-thread issue for libraries: files should be opened
 with O_CLOEXEC (that's the e flag to fopen when using glibc) unless you
 have a clear reason not to do so.

Hello Joseph,

Thanks a lot for your comments! I'll fix these problems and be back
with more safe version.

Ilya


 --
 Joseph S. Myers
 jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Joseph Myers 
On Tue, 11 Nov 2014, Andi Kleen wrote:

 Joseph Myers jos...@codesourcery.com writes:
 
  On Tue, 11 Nov 2014, Ilya Enkovich wrote:
 
  Hi,
  
  This patch integrates MPX runtime library into GCC source tree.  MPX 
  runtime is responsible for initialization of MPX feature in HW, signal 
  handling, reporting etc.  Library is linked to codes compiled with 
  -mmpx.
  
  Bootstrap is OK for x86_64-unknown-linux-gnu.  OK for trunk?
 
  Please use symbol versioning to control the set of exports for the 
  library; only symbols explicitly listed to be exported at a given symbol 
  version should be exported.
 
 Also I think you need some user documentation on the implications of the
 overriding of sigaction. If someone else wants to do similar tricks (and
 I'm sure some do) there would be conflicts.

Indeed, I don't understand what the purpose of this overriding is.  What 
goes wrong if you just use default signal handling in a program built with 
MPX, or don't handle any environment variables specially at all?  That is, 
why does this need to be linked with all programs built with MPX at all, 
rather than being optional functionality in an independent library that 
people can choose to build and link with if they want that extra 
functionality?

My starting point is that MPX provides functionality for detecting certain 
cases of undefined behavior in C that wouldn't otherwise be detected, and 
it should be possible to use with any C program (including ISO C ones that 
don't use POSIX interfaces such as sigaction and may use the sigaction 
identifier for some other purpose).

Another issue: handle_sigsegv calls functions documented in the glibc 
manual as AS-unsafe, such as pthread_setspecific and (via do_exit) 
fprintf.  You need to make sure everything called from a signal handler is 
AS-safe.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Andi Kleen 
 It is similar to libsanitizer.  Put it in glibc isn't going to work well
 for MPX.

Can you explain it more please? 

-Andi

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread H.J. Lu 
On Tue, Nov 11, 2014 at 1:01 PM, Andi Kleen a...@firstfloor.org wrote:
 It is similar to libsanitizer.  Put it in glibc isn't going to work well
 for MPX.

 Can you explain it more please?


Are you suggesting putting MPX run-time in glibc?  Will we have
2 glibc, one with MPX run-time and one without MPX run-time?


-- 
H.J.

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Andi Kleen 
On Tue, Nov 11, 2014 at 01:04:42PM -0800, H.J. Lu wrote:
 On Tue, Nov 11, 2014 at 1:01 PM, Andi Kleen a...@firstfloor.org wrote:
  It is similar to libsanitizer.  Put it in glibc isn't going to work well
  for MPX.
 
  Can you explain it more please?
 
 
 Are you suggesting putting MPX run-time in glibc?  Will we have
 2 glibc, one with MPX run-time and one without MPX run-time?

No, I just think signal chaining should be in glibc.

Then there would be no conflicts, and the problem Joseph pointed
out with unconditionally taking away sigaction from the user program
also wouldn't happen.

-Andi

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Joseph Myers 
On Tue, 11 Nov 2014, Andi Kleen wrote:

 On Tue, Nov 11, 2014 at 01:04:42PM -0800, H.J. Lu wrote:
  On Tue, Nov 11, 2014 at 1:01 PM, Andi Kleen a...@firstfloor.org wrote:
   It is similar to libsanitizer.  Put it in glibc isn't going to work well
   for MPX.
  
   Can you explain it more please?
  
  
  Are you suggesting putting MPX run-time in glibc?  Will we have
  2 glibc, one with MPX run-time and one without MPX run-time?
 
 No, I just think signal chaining should be in glibc.

I do wonder whether the signal handler needs to be enabled by default or 
whether there should simply be a function __mpx_handle_sigsegv provided 
with instructions to the user on how it can be used to get extra 
MPX-related information when SIGSEGV occurs (in which case it's entirely 
the user's responsibility to write a handler calling their own handler and 
the MPX one, if desired).  Or an LD_PRELOAD library like libSegFault.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread H.J. Lu 
On Tue, Nov 11, 2014 at 1:33 PM, Joseph Myers jos...@codesourcery.com wrote:
 On Tue, 11 Nov 2014, Andi Kleen wrote:

 On Tue, Nov 11, 2014 at 01:04:42PM -0800, H.J. Lu wrote:
  On Tue, Nov 11, 2014 at 1:01 PM, Andi Kleen a...@firstfloor.org wrote:
   It is similar to libsanitizer.  Put it in glibc isn't going to work well
   for MPX.
  
   Can you explain it more please?
  
 
  Are you suggesting putting MPX run-time in glibc?  Will we have
  2 glibc, one with MPX run-time and one without MPX run-time?

 No, I just think signal chaining should be in glibc.

Currently mpx-runtime uses

dlsym(RTLD_NEXT, sigaction);

to get the original sigaction in liibc.so.  Glibc does provides __sigaction
for the original sigaction.  But dlsym(RTLD_NEXT, is more portable.

 I do wonder whether the signal handler needs to be enabled by default or
 whether there should simply be a function __mpx_handle_sigsegv provided
 with instructions to the user on how it can be used to get extra
 MPX-related information when SIGSEGV occurs (in which case it's entirely
 the user's responsibility to write a handler calling their own handler and
 the MPX one, if desired).  Or an LD_PRELOAD library like libSegFault.


It is certainly an option.

-- 
H.J.

Re: [PATCH, MPX runtime 1/2] Integrate MPX runtime library

2014-11-11 Thread Joseph Myers 
On Tue, 11 Nov 2014, H.J. Lu wrote:

 Currently mpx-runtime uses
 
 dlsym(RTLD_NEXT, sigaction);
 
 to get the original sigaction in liibc.so.  Glibc does provides __sigaction
 for the original sigaction.  But dlsym(RTLD_NEXT, is more portable.

I guess there's a question of what the portability targets for this code 
are.  E.g. setting O_CLOEXEC when opening files (not later with fcntl 
FD_CLOEXEC if there might be other threads running at the time) is good 
practice for writing libraries - but while O_CLOEXEC is in current POSIX, 
setting it from fopen with the e flag is a GNU extension.

-- 
Joseph S. Myers
jos...@codesourcery.com