diff mbox series

[libgomp,nvptx] Fix libgomp.c/target-5.c compilation

Message ID 0ffab1d0-d202-fcaa-4d6e-658ab51d7f3a@suse.de
State New
Headers show
Series [libgomp,nvptx] Fix libgomp.c/target-5.c compilation | expand

Commit Message

Tom de Vries Dec. 13, 2018, 12:09 p.m. UTC
[ was: Re: [libgomp, nvptx] Disable
OMP_{DISPLAY_AFFINITY,AFFINITY_FORMAT} support ]

On 12-12-18 14:15, Jakub Jelinek wrote:
> On Wed, Dec 12, 2018 at 02:02:20PM +0100, Tom de Vries wrote:
>> This RFC patch implements that approach for getpid and gethostname (I
>> wonder though whether it's not possible to turn the corresponding
>> configure tests into link tests, which could also fix this for nvptx).
>>
>> Another problem we're running into is missing istty, pulled in by
>> fwrite. The nvptx newlib port does support write, but I'm unsure in what
>> form I should handle this.  Add configure tests for fwrite and write? Or
>> special case the files in the config/nvptx dir? Any advice here?
>>
>> Thanks,
>> - Tom
> 
>> [RFC][libgomp, nvptx] Fix target-5.c compilation
>>
>> Libgomp test-case libgomp.c/target-5.c is failing to compile with nvptx
>> accelerator due to missing:
>> - getpid
>> - gethostname
>> - isatty (pulled in by fwrite)
>> in the nvptx newlib.
>>
>> This demonstrator patch fixes that by minimally guarding all related locations
>> with ifndef LIBGOMP_OFFLOADED_ONLY, which allows the test-case (and others) to
>> pass.
> 
> I guess my preference would be something similar to what
> libgomp/config/mingw32/affinity-fmt.c does now, so override the file,
> define/undefine something in there and include the common affinity-fmt.c.
> Perhaps for the fwrite stuff define introduce a function that does that
> (say
> void
> gomp_print_string (const char *str, size_t len)
> {
>   fwrite (str, 1, len, stderr);
> },
> because it is in more than one file, make it hidden in libgomp.h.
> config/nvptx/error.c then has what you could also use in
> config/nvptx/affinity-fmt.c.
> And for hostname/getpid maybe just #undef those with a comment?

Done.

OK for trunk?

Thanks,
- Tom

Comments

Jakub Jelinek Dec. 13, 2018, 1:32 p.m. UTC | #1
On Thu, Dec 13, 2018 at 01:09:25PM +0100, Tom de Vries wrote:
> 2018-12-13  Tom de Vries  <tdevries@suse.de>
> 
> 	* affinity-fmt.c (gomp_print_string): New function, factored out of ...
> 	(omp_display_affinity, gomp_display_affinity_thread): ... here, and ...
> 	* fortran.c (omp_display_affinity_): ... here.
> 	* libgomp.h (gomp_print_string): Declare.
> 	* config/nvptx/affinity-fmt.c: New file.  Include affinity-fmt.c,
> 	undefining HAVE_GETPID and HAVE_GETHOSTNAME, and mapping fwrite to
> 	write.

> +/* The nvptx newlib implementation does not support fwrite, but it does support
> +   write.  Map fwrite to write for size == 1.  */
> +#undef fwrite
> +#define fwrite(ptr, size, nmemb, stream) \
> +  ((size) == 1 ? write (1, (ptr), (nmemb)) : 0)

Why not
  write (1, (ptr), (nmemb) * (size))
I know it doesn't handle the overflow case properly, but the callers are
using 1 and it actually isn't a security threat if it prints fewer chars
than it should.  We don't have anything where we'd rely on fwrite failing
when stuff overflows...

Ok with that change (plus adjust the comment).

	Jakub
diff mbox series

Patch

[libgomp, nvptx] Fix libgomp.c/target-5.c compilation

Libgomp test-case libgomp.c/target-5.c is failing to compile when building for
x86_64 with nvptx accelerator due to missing:
- getpid
- gethostname
- isatty (pulled in by fwrite)
in the nvptx newlib.

This patch fixes the build failure by:
- adding a function gomp_print_string which limits the use of fwrite to a single
  location (affinity-fmt.c), and
- creating an nvptx version of affinity-fmt.c, which:
  - implements fwrite using write
  - overrides the configure test results HAVE_GETPID and HAVE_GETHOSTNAME

Build and reg-tested on x86_64 with nvptx accelerator.

2018-12-13  Tom de Vries  <tdevries@suse.de>

	* affinity-fmt.c (gomp_print_string): New function, factored out of ...
	(omp_display_affinity, gomp_display_affinity_thread): ... here, and ...
	* fortran.c (omp_display_affinity_): ... here.
	* libgomp.h (gomp_print_string): Declare.
	* config/nvptx/affinity-fmt.c: New file.  Include affinity-fmt.c,
	undefining HAVE_GETPID and HAVE_GETHOSTNAME, and mapping fwrite to
	write.

---
 libgomp/affinity-fmt.c              | 14 +++++++---
 libgomp/config/nvptx/affinity-fmt.c | 52 +++++++++++++++++++++++++++++++++++++
 libgomp/fortran.c                   |  4 +--
 libgomp/libgomp.h                   |  1 +
 4 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/libgomp/affinity-fmt.c b/libgomp/affinity-fmt.c
index 9e5c5f754eb..1447597e7f7 100644
--- a/libgomp/affinity-fmt.c
+++ b/libgomp/affinity-fmt.c
@@ -37,6 +37,12 @@ 
 #include <sys/utsname.h>
 #endif
 
+void
+gomp_print_string (const char *str, size_t len)
+{
+  fwrite (str, 1, len, stderr);
+}
+
 void
 gomp_set_affinity_format (const char *format, size_t len)
 {
@@ -456,13 +462,13 @@  omp_display_affinity (const char *format)
   if (ret < sizeof buf)
     {
       buf[ret] = '\n';
-      fwrite (buf, 1, ret + 1, stderr);
+      gomp_print_string (buf, ret + 1);
       return;
     }
   b = gomp_malloc (ret + 1);
   ialias_call (omp_capture_affinity) (b, ret + 1, format);
   b[ret] = '\n';
-  fwrite (b, 1, ret + 1, stderr);
+  gomp_print_string (b, ret + 1);
   free (b);
 }
 
@@ -477,13 +483,13 @@  gomp_display_affinity_thread (gomp_thread_handle handle,
   if (ret < sizeof buf)
     {
       buf[ret] = '\n';
-      fwrite (buf, 1, ret + 1, stderr);
+      gomp_print_string (buf, ret + 1);
       return;
     }
   b = gomp_malloc (ret + 1);
   gomp_display_affinity (b, ret + 1, gomp_affinity_format_var,
   			 handle, ts, place);
   b[ret] = '\n';
-  fwrite (b, 1, ret + 1, stderr);
+  gomp_print_string (b, ret + 1);
   free (b);
 }
diff --git a/libgomp/config/nvptx/affinity-fmt.c b/libgomp/config/nvptx/affinity-fmt.c
new file mode 100644
index 00000000000..ff1cbfcc4f8
--- /dev/null
+++ b/libgomp/config/nvptx/affinity-fmt.c
@@ -0,0 +1,52 @@ 
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of the GNU Offloading and Multi Processing Library
+   (libgomp).
+
+   Libgomp is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3, or (at your option)
+   any later version.
+
+   Libgomp is distributed in the hope that it will be useful, but WITHOUT ANY
+   WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+   FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+   more details.
+
+   Under Section 7 of GPL version 3, you are granted additional
+   permissions described in the GCC Runtime Library Exception, version
+   3.1, as published by the Free Software Foundation.
+
+   You should have received a copy of the GNU General Public License and
+   a copy of the GCC Runtime Library Exception along with this program;
+   see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include "libgomp.h"
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+#ifdef HAVE_INTTYPES_H
+# include <inttypes.h>  /* For PRIx64.  */
+#endif
+#ifdef HAVE_UNAME
+#include <sys/utsname.h>
+#endif
+
+/* The HAVE_GETPID and HAVE_GETHOSTNAME configure tests are passing for nvptx,
+   while the nvptx newlib implementation does not support those functions.
+   Override the configure test results here.  */
+#undef HAVE_GETPID
+#undef HAVE_GETHOSTNAME
+
+/* The nvptx newlib implementation does not support fwrite, but it does support
+   write.  Map fwrite to write for size == 1.  */
+#undef fwrite
+#define fwrite(ptr, size, nmemb, stream) \
+  ((size) == 1 ? write (1, (ptr), (nmemb)) : 0)
+
+#include "../../affinity-fmt.c"
+
diff --git a/libgomp/fortran.c b/libgomp/fortran.c
index 0157baec648..328bf9c8450 100644
--- a/libgomp/fortran.c
+++ b/libgomp/fortran.c
@@ -626,7 +626,7 @@  omp_display_affinity_ (const char *format, size_t format_len)
   if (ret < sizeof buf)
     {
       buf[ret] = '\n';
-      fwrite (buf, 1, ret + 1, stderr);
+      gomp_print_string (buf, ret + 1);
     }
   else
     {
@@ -635,7 +635,7 @@  omp_display_affinity_ (const char *format, size_t format_len)
 			     format_len ? fmt : gomp_affinity_format_var,
 			     gomp_thread_self (), &thr->ts, thr->place);
       b[ret] = '\n';
-      fwrite (b, 1, ret + 1, stderr);
+      gomp_print_string (b, ret + 1);
       free (b);
     }
   if (fmt && fmt != fmt_buf)
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 7eacb45db66..8105e640e32 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -751,6 +751,7 @@  extern void gomp_display_affinity_place (char *, size_t, size_t *, int);
 
 /* affinity-fmt.c */
 
+extern void gomp_print_string (const char *str, size_t len);
 extern void gomp_set_affinity_format (const char *, size_t);
 extern void gomp_display_string (char *, size_t, size_t *, const char *,
 				 size_t);