From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11495 invoked by alias); 10 Dec 2015 12:06:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 11478 invoked by uid 89); 10 Dec 2015 12:06:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.1 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Thu, 10 Dec 2015 12:06:32 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 0F9D9AC3B for ; Thu, 10 Dec 2015 12:06:29 +0000 (UTC) Subject: Re: [hsa 3/10] HSA libgomp plugin [part 2/2] To: gcc-patches@gcc.gnu.org References: <20151207111758.GA24234@virgil.suse.cz> <20151207112049.GD24234@virgil.suse.cz> <20151209121555.GQ5675@tucnak.redhat.com> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <56696AC4.5030505@suse.cz> Date: Thu, 10 Dec 2015 12:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151209121555.GQ5675@tucnak.redhat.com> Content-Type: multipart/mixed; boundary="------------090107080904030709060101" X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg01115.txt.bz2 This is a multi-part message in MIME format. --------------090107080904030709060101 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Content-length: 5091 On 12/09/2015 01:15 PM, Jakub Jelinek wrote: > On Mon, Dec 07, 2015 at 12:20:49PM +0100, Martin Jambor wrote: >> +#include >> +#include >> +#include >> +#include >> +#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 --------------090107080904030709060101 Content-Type: text/x-patch; name="0002-HSA-add-configure-check-of-secure_getenv-in-libgomp.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0002-HSA-add-configure-check-of-secure_getenv-in-libgomp.pat"; filename*1="ch" Content-length: 2252 >From 412f0429295e38e853f5a1246a2b2711165309f4 Mon Sep 17 00:00:00 2001 From: marxin 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 --------------090107080904030709060101--