Re: [PATCH 3/4] Add libgomp plugin for Intel MIC

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Oct 21, 2014 at 09:24:13PM +0400, Ilya Verbin wrote:
> +/* Add path specified in LD_LIBRARY_PATH to MIC_LD_LIBRARY_PATH, which is
> +   required by liboffloadmic.  */
> +__attribute__((constructor))
> +static void
> +set_mic_lib_path (void)
> +{
> +  const char *ld_lib_path = getenv (LD_LIBRARY_PATH_ENV);
> +  const char *mic_lib_path = getenv (MIC_LD_LIBRARY_PATH_ENV);
> +  char *mic_lib_path_new;
> +
> +  if (!ld_lib_path)
> +    return;
> +
> +  mic_lib_path_new = (char *) malloc ((mic_lib_path ? strlen (mic_lib_path) : 0)
> +				      + strlen (ld_lib_path) + 2);

malloc can fail, SIGSEGV in response to that is not desirable.
Can't you fallback to alloca, or use just alloca, or use alloca
with malloc fallback?
> + 
> +  if (!mic_lib_path)
> +    strcpy (mic_lib_path_new, ld_lib_path);
> +  else
> +    sprintf (mic_lib_path_new, "%s:%s", mic_lib_path, ld_lib_path);
> +  setenv (MIC_LD_LIBRARY_PATH_ENV, mic_lib_path_new, 1);
> +  free (mic_lib_path_new);

> +extern "C" void
> +GOMP_OFFLOAD_register_image (void *host_table, void *target_image)
> +{
> +  TRACE ("(host_table = %p, target_image = %p)", host_table, target_image);
> +
> +  if (num_libraries >= 1000)
> +    {
> +      fprintf (stderr,
> +	       "%s: The number of loaded shared libraries is over 1000!\n",
> +	       __FILE__);
> +      exit (1);

Where does this artificial limit come from?  Using libNNN.so library names?
Can't you use lib%d.so instead?

Also, seeing register_image, shouldn't there be
GOMP_OFFLOAD_unregister_image which would be invoked when the library
containing MIC offloading regions is dlclosed?
One could use __cxa_atexit or similar for that, something that is given
&__dso_handle.  Or is no cleanup necessary?  At least unregistering it
from translation tables, because the same addresses might be reused by a
different shared library?
With dlopen/dlclose in mind, 1000 might be easily reached, consider 10000
times dlopening/dlclosing (perhaps over longer time, by long running daemon)
a shared library containg #pragma omp target region.

> +static int first_init = 1;
> +
> +/* Load offload_target_main on target.  */
> +extern "C" void
> +GOMP_OFFLOAD_init_device (int device)
> +{
> +  TRACE ("");
> +  pthread_mutex_lock (&mutex);
> +  if (first_init)
> +    {
> +      __offload_register_image (&main_target_image);
> +      first_init = 0;
> +    }
> +  pthread_mutex_unlock (&mutex);

pthread_once instead?

	Jakub