On 12/09/2015 01:15 PM, Jakub Jelinek wrote:
> On Mon, Dec 07, 2015 at 12:20:49PM +0100, Martin Jambor wrote:
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <pthread.h>
>> +#include "libgomp-plugin.h"
>> +#include "gomp-constants.h"
>> +#include "hsa.h"
>> +#include "hsa_ext_finalize.h"
>
> If these 2 headers are coming from outside of gcc, better use <> for them
> instead of "", and better place them above the libgomp specific ones.
>
>> +#include "dlfcn.h"
>
> Ditto.
>
>> +/* Flag to decide whether print to stderr information about what is going
>> on.
>> + Set in init_debug depending on environment variables. */
>> +
>> +static bool debug;
>> +
>> +/* Flag to decide if the runtime should suppress a possible fallback to host
>> + execution. */
>> +
>> +static bool suppress_host_fallback;
>> +
>> +/* Initialize debug and suppress_host_fallback according to the
>> environment. */
>> +
>> +static void
>> +init_enviroment_variables (void)
>> +{
>> + if (getenv ("HSA_DEBUG"))
>> + debug = true;
>> + else
>> + debug = false;
>> +
>> + if (getenv ("HSA_SUPPRESS_HOST_FALLBACK"))
>> + suppress_host_fallback = true;
>> + else
>> + suppress_host_fallback = false;
>
> Wonder whether we want these custom env vars in suid programs, allowing
> users to change behavior might be undesirable. So perhaps use secure_getenv
> instead of getenv if found by configure?
>> +
>> + const char* hsa_error;
>
> Space before * instead of after it (multiple spots).
>
>> + hsa_status_string (status, &hsa_error);
>> +
>> + unsigned l = strlen (hsa_error);
>> +
>> + char *err = GOMP_PLUGIN_malloc (sizeof (char) * l);
>
> sizeof (char) is 1, please don't multiply by it, that is just confusing.
> It makes sense in macros where you don't know what the type really is, but
> not here.
>
>> + memcpy (err, hsa_error, l - 1);
>> + err[l] = '\0';
>> +
>> + fprintf (stderr, "HSA warning: %s (%s)\n", str, err);
>
> Why do you allocate err at all, if you free it right away? Can't you use
> hsa_error directly instead?
>> +
>> + free (err);
>
>> +static void
>> +hsa_fatal (const char *str, hsa_status_t status)
>> +{
>> + const char* hsa_error;
>> + hsa_status_string (status, &hsa_error);
>> + GOMP_PLUGIN_fatal ("HSA fatal error: %s (%s)", str, hsa_error);
>
> Like you do e.g. here?
>
>> +/* Callback of dispatch queues to report errors. */
>> +
>> +static void
>> +queue_callback(hsa_status_t status, hsa_queue_t* queue __attribute__
>> ((unused)),
>
> Missing space before (.
>> +/* Callback of hsa_agent_iterate_regions. Determine if a memory REGION can
>> be
>> + used for kernarg allocations and if so write it to the memory pointed to
>> by
>> + DATA and break the query. */
>> +
>> +static hsa_status_t get_kernarg_memory_region (hsa_region_t region, void*
>> data)
>
> Missing newline between return type and function name.
>
>> + hsa_region_t* ret = (hsa_region_t*) data;
>
> Two spots with wrong formatting above.
>> +{
>> + if (agent->first_module)
>> + agent->first_module->prev = module;
>
> Wrong indentation.
>
>> + unsigned size = end - start;
>> + char *buf = (char *) malloc (size);
>> + memcpy (buf, start, size);
>
> malloc could return NULL and you crash. Should this use GOMP_PLUGIN_malloc
> instead?
>
>> + struct GOMP_hsa_kernel_dispatch *shadow = GOMP_PLUGIN_malloc_cleared
>> + (sizeof (struct GOMP_hsa_kernel_dispatch));
>
> Formatting, = should go on the next line, and if even that doesn't help,
> maybe use sizeof (*shadow)?
>> +
>> + shadow->queue = agent->command_q;
>> + shadow->omp_data_memory = omp_data_size > 0
>> + ? GOMP_PLUGIN_malloc (omp_data_size) : NULL;
>
> = should go on the next line.
>
>> + unsigned dispatch_count = kernel->dependencies_count;
>> + shadow->kernel_dispatch_count = dispatch_count;
>> +
>> + shadow->children_dispatches = GOMP_PLUGIN_malloc
>> + (dispatch_count * sizeof (struct GOMP_hsa_kernel_dispatch *));
>
> Likewise.
>> +indent_stream (FILE *f, unsigned indent)
>> +{
>> + for (int i = 0; i < indent; i++)
>> + fputc (' ', f);
>
> Perhaps fprintf (f, "%*s", indent, "");
> instead?
>
>> + struct GOMP_hsa_kernel_dispatch *shadow = create_single_kernel_dispatch
>> + (kernel, omp_data_size);
>
> Formatting issues again.
>
>> + shadow->omp_num_threads = 64;
>> + shadow->debug = 0;
>> + shadow->omp_level = kernel->gridified_kernel_p ? 1 : 0;
>> +
>> + /* Create kernel dispatch data structures. We do not allow to have
>> + a kernel dispatch with depth bigger than one. */
>> + for (unsigned i = 0; i < kernel->dependencies_count; i++)
>> + {
>> + struct kernel_info *dependency = get_kernel_for_agent
>> + (kernel->agent, kernel->dependencies[i]);
>> + shadow->children_dispatches[i] = create_single_kernel_dispatch
>> + (dependency, omp_data_size);
>> + shadow->children_dispatches[i]->queue =
>
> Never put = at the end of line.
>> + kernel->agent->kernel_dispatch_command_q;
>
> Jakub
>
Hi.
The second part introduces usage of 'secure_getenv' routine.
Martin
>From 412f0429295e38e853f5a1246a2b2711165309f4 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 10 Dec 2015 11:17:15 +0100
Subject: [PATCH 2/2] HSA: add configure check of secure_getenv in libgomp
---
libgomp/configure | 2 +-
libgomp/configure.ac | 2 +-
libgomp/plugin/plugin-hsa.c | 12 ++++++++++--
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/libgomp/configure b/libgomp/configure
index a9c421e..47ced17 100755
--- a/libgomp/configure
+++ b/libgomp/configure
@@ -15459,7 +15459,7 @@ _ACEOF
# Check for functions needed.
-for ac_func in getloadavg clock_gettime strtoull
+for ac_func in getloadavg clock_gettime strtoull secure_getenv __secure_getenv
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/libgomp/configure.ac b/libgomp/configure.ac
index 2e41ca8..b5a5b37 100644
--- a/libgomp/configure.ac
+++ b/libgomp/configure.ac
@@ -205,7 +205,7 @@ esac
m4_include([plugin/configfrag.ac])
# Check for functions needed.
-AC_CHECK_FUNCS(getloadavg clock_gettime strtoull)
+AC_CHECK_FUNCS(getloadavg clock_gettime strtoull secure_getenv __secure_getenv)
# Check for broken semaphore implementation on darwin.
# sem_init returns: sem_init error: Function not implemented.
diff --git a/libgomp/plugin/plugin-hsa.c b/libgomp/plugin/plugin-hsa.c
index 46b9c03..df7a4c4 100644
--- a/libgomp/plugin/plugin-hsa.c
+++ b/libgomp/plugin/plugin-hsa.c
@@ -37,6 +37,14 @@
#include "libgomp-plugin.h"
#include "gomp-constants.h"
+#ifndef HAVE_SECURE_GETENV
+# ifdef HAVE___SECURE_GETENV
+# define secure_getenv __secure_getenv
+# else
+# define secure_getenv getenv
+# endif
+#endif
+
/* Part of the libgomp plugin interface. Return the name of the accelerator,
which is "hsa". */
@@ -87,12 +95,12 @@ static bool suppress_host_fallback;
static void
init_enviroment_variables (void)
{
- if (getenv ("HSA_DEBUG"))
+ if (secure_getenv ("HSA_DEBUG"))
debug = true;
else
debug = false;
- if (getenv ("HSA_SUPPRESS_HOST_FALLBACK"))
+ if (secure_getenv ("HSA_SUPPRESS_HOST_FALLBACK"))
suppress_host_fallback = true;
else
suppress_host_fallback = false;
--
2.6.3
