[PATCH] Handle GOMP_NVPTX_PTXRW in libgomp nvptx plugin

[Tom de Vries <Tom_deVries@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

On 11/07/2017 03:54 PM, Jakub Jelinek wrote:
> On Tue, Nov 07, 2017 at 06:48:25AM -0800, Cesar Philippidis wrote:
>>> Changes in the patch series:
>>> - removed OPENACC_ from environment variable names
>>> - made temp files use gomp-nvptx prefix.
>>> - fixed build error due to missing _GNU_SOURCE in libgomp-nvptx.c.
>>> - merged the three GOMP_NVPTX_JIT patches into one
>>> - rewrote GOMP_NVPTX_JIT to add no extra flags to the JIT compiler
>>>    invocation if GOMP_NVPTX_JIT if not defined, removing the need for
>>>    hardcoding default values
>>> - added CU_JIT_TARGET to plugin/cuda/cuda.h
>>>
>>> Build on x86_64 with nvptx offloading enabled (using plugin/cuda/cuda.h).
>>>
>>> The patch series now looks like:
>>> 1. Handle GOMP_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
>>> 2. Handle GOMP_NVPTX_PTXRW in libgomp nvptx plugin
>>> 3. Handle GOMP_NVPTX_JIT={-O[0-4],-ori,-arch=<n>} in libgomp nvptx
>>>     plugin
>>>
>>> I'll repost the patch series in reply to this email.
>>
>> Ping.
>>
>> Can we get this patch series into trunk and og7? The ability to easily
>> modify PTX code, via GOMP_NVPTX_PTXRW, is extremely helpful. It helped
>> me isolate one problem already.
> 
> It can be helpful for debugging, but I'm afraid about having such code in
> production, I think something like this would be very easy to exploit.
> Sure, running a suid or sgid program with offloading is probably very
> dangerous anyway, but it could be just some minor priviledge escalation
> in the app (SELinux, ACLs, whatever else) and this stuff would allow anyone
> to run anything else.
> So, IMNSHO if it should be added, only enabled by non-default configure
> option.

Hi,

I've made the GOMP_NVPTX_PTXRW patch stand-alone, and added an 
off-by-default libgomp configure option 
--enable-libgomp-plugin-developer-only-options, which sets a config.h 
macro LIBGOMP_PLUGIN_DEVELOPER_ONLY_OPTIONS, which is used to 
enable/disable the GOMP_NVPTX_PTXRW functionality.

I've build this on x86_64 for nvptx accelerator, both with and without 
the configure option, and confirmed that in one case using 
GOMP_NVPTX_PTXRW=w generates a gomp-nvptx.0.ptx file, and in the other 
case it doesn't.

OK for trunk if x86_64 bootstrap and reg-test succeeds?

Thanks,
- Tom

[-- Attachment #2: 0001-Handle-GOMP_NVPTX_PTXRW-in-libgomp-nvptx-plugin.patch --]
[-- Type: text/x-patch, Size: 10663 bytes --]

Handle GOMP_NVPTX_PTXRW in libgomp nvptx plugin

2017-11-21  Tom de Vries  <tom@codesourcery.com>

	* plugin/plugin-nvptx.c (_GNU_SOURCE): Define.
	(gomp_nvptx_ptxrw): New static variable.
	(parse_gomp_nvptx_ptxrw, post_process_ptx_write, post_process_ptx_read)
	(post_process_ptx): New function.
	(link_ptx): Call post_process_ptx.
	* configure.ac: Add configure option
	--enable-libgomp-plugin-developer-only-options.
	* config.h.in: Regenerate.
	* configure: Same.

---
 libgomp/config.h.in           |   3 +
 libgomp/configure             |  32 ++++++++-
 libgomp/configure.ac          |  11 +++
 libgomp/plugin/plugin-nvptx.c | 160 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 202 insertions(+), 4 deletions(-)

diff --git a/libgomp/config.h.in b/libgomp/config.h.in
index e7bc4d97374..68cccea4186 100644
--- a/libgomp/config.h.in
+++ b/libgomp/config.h.in
@@ -118,6 +118,9 @@
 /* Define to 1 if building libgomp for an accelerator-only target. */
 #undef LIBGOMP_OFFLOADED_ONLY
 
+/* Define to 1 if libgomp plugin developer-only options are enabled. */
+#undef LIBGOMP_PLUGIN_DEVELOPER_ONLY_OPTIONS
+
 /* Define to 1 if libgomp should use POSIX threads. */
 #undef LIBGOMP_USE_PTHREADS
 
diff --git a/libgomp/configure b/libgomp/configure
index e7842b5519f..14e39d7fbec 100755
--- a/libgomp/configure
+++ b/libgomp/configure
@@ -780,6 +780,7 @@ ac_subst_files=''
 ac_user_opts='
 enable_option_checking
 enable_version_specific_runtime_libs
+enable_libgomp_plugin_developer_only_options
 enable_generated_files_in_srcdir
 enable_multilib
 enable_dependency_tracking
@@ -1434,6 +1435,9 @@ Optional Features:
   --enable-version-specific-runtime-libs
                           Specify that runtime libraries should be installed
                           in a compiler-specific directory [default=no]
+  --enable-libgomp-plugin-developer-only-options
+                          Specify that libgomp plugins should be build with
+                          additional developer-only options [default=no]
   --enable-generated-files-in-srcdir
                           put copies of generated files in source dir intended
                           for creating source tarballs for users without
@@ -2627,6 +2631,30 @@ fi
 { $as_echo "$as_me:${as_lineno-$LINENO}: result: $enable_version_specific_runtime_libs" >&5
 $as_echo "$enable_version_specific_runtime_libs" >&6; }
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for --enable-libgomp-plugin-developer-only-options" >&5
+$as_echo_n "checking for --enable-libgomp-plugin-developer-only-options... " >&6; }
+ # Check whether --enable-libgomp-plugin-developer-only-options was given.
+if test "${enable_libgomp_plugin_developer_only_options+set}" = set; then :
+  enableval=$enable_libgomp_plugin_developer_only_options;
+      case "$enableval" in
+       yes|no) ;;
+       *) as_fn_error "Unknown argument to enable/disable libgomp-plugin-developer-only-options" "$LINENO" 5 ;;
+                          esac
+
+else
+  enable_libgomp_plugin_developer_only_options=no
+fi
+
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $enable_libgomp_plugin_developer_only_options" >&5
+$as_echo "$enable_libgomp_plugin_developer_only_options" >&6; }
+if test x$enable_libgomp_plugin_developer_only_options != xno; then
+
+$as_echo "#define LIBGOMP_PLUGIN_DEVELOPER_ONLY_OPTIONS 1" >>confdefs.h
+
+fi
+
+
 # We would like our source tree to be readonly. However when releases or
 # pre-releases are generated, the flex/bison generated files as well as the
 # various formats of manuals need to be included along with the rest of the
@@ -11158,7 +11186,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11161 "configure"
+#line 11189 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -11264,7 +11292,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11267 "configure"
+#line 11295 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/libgomp/configure.ac b/libgomp/configure.ac
index 4e0bc8166a9..c6cfb1f0796 100644
--- a/libgomp/configure.ac
+++ b/libgomp/configure.ac
@@ -15,6 +15,17 @@ LIBGOMP_ENABLE(version-specific-runtime-libs, no, ,
    permit yes|no)
 AC_MSG_RESULT($enable_version_specific_runtime_libs)
 
+AC_MSG_CHECKING([for --enable-libgomp-plugin-developer-only-options])
+LIBGOMP_ENABLE(libgomp-plugin-developer-only-options, no, ,
+   [Specify that libgomp plugins should be build with additional developer-only options],
+   permit yes|no)
+AC_MSG_RESULT($enable_libgomp_plugin_developer_only_options)
+if test x$enable_libgomp_plugin_developer_only_options != xno; then
+  AC_DEFINE(LIBGOMP_PLUGIN_DEVELOPER_ONLY_OPTIONS, 1,
+            [Define to 1 if libgomp plugin developer-only options are enabled.])
+fi
+
+
 # We would like our source tree to be readonly. However when releases or
 # pre-releases are generated, the flex/bison generated files as well as the
 # various formats of manuals need to be included along with the rest of the
diff --git a/libgomp/plugin/plugin-nvptx.c b/libgomp/plugin/plugin-nvptx.c
index 71630b57355..0cbc8fc197d 100644
--- a/libgomp/plugin/plugin-nvptx.c
+++ b/libgomp/plugin/plugin-nvptx.c
@@ -31,6 +31,7 @@
    is not clear as to what that state might be.  Or how one might
    propagate it from one thread to another.  */
 
+#define _GNU_SOURCE
 #include "openacc.h"
 #include "config.h"
 #include "libgomp-plugin.h"
@@ -138,6 +139,8 @@ init_cuda_lib (void)
 # define init_cuda_lib() true
 #endif
 
+#include "secure_getenv.h"
+
 /* Convenience macros for the frequently used CUDA library call and
    error handling sequence as well as CUDA library calls that
    do the error checking themselves or don't do it at all.  */
@@ -876,6 +879,156 @@ notify_var (const char *var_name, const char *env_var)
     GOMP_PLUGIN_debug (0, "%s: '%s'\n", var_name, env_var);
 }
 
+#ifdef LIBGOMP_PLUGIN_DEVELOPER_ONLY_OPTIONS
+static int gomp_nvptx_ptxrw = -1;
+#else
+static int gomp_nvptx_ptxrw = 0;
+#endif
+
+/* Parse environment variable GOMP_NVPTX_PTXRW=[WwRr].  */
+
+static void
+parse_gomp_nvptx_ptxrw (void)
+{
+  gomp_nvptx_ptxrw = 0;
+
+  const char *var_name = "GOMP_NVPTX_PTXRW";
+  const char *env_var = secure_getenv (var_name);
+  notify_var (var_name, env_var);
+
+  if (env_var == NULL)
+    ;
+  else if ((env_var[0] == 'w' || env_var[0] == 'W')
+	   && env_var[1] == '\0')
+    gomp_nvptx_ptxrw = 1;
+  else if ((env_var[0] == 'r' || env_var[0] == 'R')
+	   && env_var[1] == '\0')
+    gomp_nvptx_ptxrw = 2;
+  else
+    GOMP_PLUGIN_error ("Error parsing %s", var_name);
+}
+
+/* Write CODE with length SIZE to file FILE_NAME.  */
+
+static void
+post_process_ptx_write (char *file_name, const char *code, size_t size)
+{
+  FILE *ptx_file = fopen (file_name, "w");
+  if (ptx_file == NULL)
+    {
+      GOMP_PLUGIN_debug (0, "Opening %s failed\n", file_name);
+      return;
+    }
+
+  int res = fprintf (ptx_file, "%s", code);
+  unsigned int write_succeeded = res == size - 1;
+  if (!write_succeeded)
+    GOMP_PLUGIN_debug (0,
+		       "Writing %s failed: written %d but expected %zu\n",
+		       file_name, res, size - 1);
+
+  res = fclose (ptx_file);
+  if (res != 0)
+    GOMP_PLUGIN_debug (0, "Closing %s failed\n", file_name);
+}
+
+/* Read the contents of FILE_NAME into *RES_CODE and save the file size
+   in *RES_SIZE.  */
+
+static void
+post_process_ptx_read (char *file_name, const char **res_code, size_t *res_size)
+{
+  FILE *ptx_file = fopen (file_name, "r");
+  if (ptx_file == NULL)
+    {
+      GOMP_PLUGIN_debug (0, "Opening %s failed\n", file_name);
+      return;
+    }
+
+  if (fseek (ptx_file, 0L, SEEK_END) != 0)
+    {
+      GOMP_PLUGIN_debug (0, "Seeking end of %s failed\n", file_name);
+      return;
+    }
+
+  long bufsize = ftell (ptx_file);
+  if (bufsize == -1)
+    {
+      GOMP_PLUGIN_debug (0, "ftell of %s failed\n", file_name);
+      return;
+    }
+
+  if (fseek (ptx_file, 0L, SEEK_SET) != 0)
+    {
+      GOMP_PLUGIN_debug (0, "Seeking start of %s failed\n", file_name);
+      return;
+    }
+
+  char *new_code = GOMP_PLUGIN_malloc (sizeof (char) * (bufsize + 1));
+
+  size_t new_size = fread (new_code, sizeof (char), bufsize, ptx_file);
+  if (ferror (ptx_file) != 0)
+    {
+      GOMP_PLUGIN_debug (0, "Reading %s failed\n", file_name);
+      return;
+    }
+
+  assert (new_size < bufsize + 1);
+  new_code[new_size++] = '\0';
+
+  int res = fclose (ptx_file);
+  if (res != 0)
+    {
+      GOMP_PLUGIN_debug (0, "Closing %s failed\n", file_name);
+      return;
+    }
+
+  *res_code = new_code;
+  *res_size = new_size;
+}
+
+/* If environment variable GOMP_NVPTX_PTXRW=[Ww], write *RES_CODE to file
+   gomp-nvptx.<NUM>.ptx.  If it is [Rr], read *RES_CODE from file
+   instead.  */
+
+static void
+post_process_ptx (unsigned num, const char **res_code, size_t *res_size)
+{
+  if (gomp_nvptx_ptxrw == -1)
+    parse_gomp_nvptx_ptxrw ();
+
+  if (gomp_nvptx_ptxrw == 0)
+    return;
+
+  const char *code = *res_code;
+  size_t size = *res_size;
+
+  const char *prefix = "gomp-nvptx.";
+  const char *postfix = ".ptx";
+  const int len = (strlen (prefix)
+		   + 10 /* %u.  */
+		   + strlen (postfix)
+		   + 1  /* '\0'.  */);
+  char file_name[len];
+  int res = snprintf (file_name, len, "%s%u%s", prefix,
+		      num, postfix);
+  assert (res < len); /* Assert there's no truncation.  */
+
+  GOMP_PLUGIN_debug (0, "%s %s \n",
+		     (gomp_nvptx_ptxrw == 1 ? "Writing" : "Reading"),
+		     file_name);
+
+  switch (gomp_nvptx_ptxrw)
+    {
+    case 1:
+      post_process_ptx_write (file_name, code, size);
+      break;
+    case 2:
+      post_process_ptx_read (file_name, res_code, res_size);
+      break;
+    }
+}
+
 static bool
 link_ptx (CUmodule *module, const struct targ_ptx_obj *ptx_objs,
 	  unsigned num_objs)
@@ -912,11 +1065,14 @@ link_ptx (CUmodule *module, const struct targ_ptx_obj *ptx_objs,
 
   for (; num_objs--; ptx_objs++)
     {
+      const char *ptx_code = ptx_objs->code;
+      size_t ptx_size = ptx_objs->size;
+      post_process_ptx (num_objs, &ptx_code, &ptx_size);
+      GOMP_PLUGIN_debug (0, "Loading:\n---\n%s\n---\n", ptx_code);
       /* cuLinkAddData's 'data' argument erroneously omits the const
 	 qualifier.  */
-      GOMP_PLUGIN_debug (0, "Loading:\n---\n%s\n---\n", ptx_objs->code);
       r = CUDA_CALL_NOCHECK (cuLinkAddData, linkstate, CU_JIT_INPUT_PTX,
-			     (char *) ptx_objs->code, ptx_objs->size,
+			     (char *) ptx_code, ptx_size,
 			     0, 0, 0, 0);
       if (r != CUDA_SUCCESS)
 	{