Re: [PATCH] Implement GOMP_OFFLOAD_unload_image in intelmic plugin

[Ilya Verbin <iverbin at gmail dot com>]

On Thu, Nov 19, 2015 at 14:33:06 +0100, Jakub Jelinek wrote:
> On Mon, Nov 16, 2015 at 08:33:28PM +0300, Ilya Verbin wrote:
> > diff --git a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
> > index 772e198..6ee585e 100644
> > --- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
> > +++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
> > @@ -65,6 +65,17 @@ typedef std::vector<AddrVect> DevAddrVect;
> >  /* Addresses for all images and all devices.  */
> >  typedef std::map<const void *, DevAddrVect> ImgDevAddrMap;
> >  
> > +/* Image descriptor needed by __offload_[un]register_image.  */
> > +struct TargetImageDesc {
> > +  int64_t size;
> > +  /* 10 characters is enough for max int value.  */
> > +  char name[sizeof ("lib0000000000.so")];
> > +  char data[];
> > +} __attribute__ ((packed));
> 
> Why the packed attribute?  I know it is preexisting, but with int64_t
> being the first and then just char, there is no padding in between fields.

Hmmm, I can't remember, but I definitely have added this attribute 2 years ago,
because liboffloadmic failed to register the image.  Anyway, now everything
works fine without it.

> And to determine the size without data, you can just use offsetof.

I will add this:

diff --git a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
index 6ee585e..f8c1725 100644
--- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
+++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
@@ -71,7 +71,7 @@ struct TargetImageDesc {
   /* 10 characters is enough for max int value.  */
   char name[sizeof ("lib0000000000.so")];
   char data[];
-} __attribute__ ((packed));
+};
 
 /* Image descriptors, indexed by a pointer obtained from libgomp.  */
 typedef std::map<const void *, TargetImageDesc *> ImgDescMap;
@@ -313,9 +313,8 @@ offload_image (const void *target_image)
 	 target_image, image_start, image_end);
 
   int64_t image_size = (uintptr_t) image_end - (uintptr_t) image_start;
-  TargetImageDesc *image
-    = (TargetImageDesc *) malloc (sizeof (int64_t) + sizeof ("lib0000000000.so")
-				  + image_size);
+  TargetImageDesc *image = (TargetImageDesc *) malloc (offsetof (TargetImageDesc, data)
+						       + image_size);
   if (!image)
     {
       fprintf (stderr, "%s: Can't allocate memory\n", __FILE__);


> > @@ -217,13 +231,27 @@ offload (const char *file, uint64_t line, int device, const char *name,
> >  }
> >  
> >  static void
> > +unregister_main_image ()
> > +{
> > +  __offload_unregister_image (&main_target_image);
> > +}
> > +
> > +static void
> >  register_main_image ()
> >  {
> > +  /* Do not check the return value, because old versions of liboffloadmic did
> > +     not have return values.  */
> >    __offload_register_image (&main_target_image);
> >  
> >    /* liboffloadmic will call GOMP_PLUGIN_target_task_completion when
> >       asynchronous task on target is completed.  */
> >    __offload_register_task_callback (GOMP_PLUGIN_target_task_completion);
> > +
> > +  if (atexit (unregister_main_image) != 0)
> > +    {
> > +      fprintf (stderr, "%s: atexit failed\n", __FILE__);
> > +      exit (1);
> > +    }
> >  }
> 
> What is the point of this hunk?  Is there any point in unregistering the
> main target image?  I mean at that point the process is exiting anyway.
> The importance of unregistering target images registered from shared
> libraries is that they should be unregistered when they are dlclosed.

liboffloadmic performs correct finalization of the target process in
__offload_fini_library, which is called only during unregistration of the main
target image.
Without this finalization the target process will be destroyed after unloading
libcoi_host.so.  And then some DSO may call GOMP_offload_unregister_ver from its
destructor, which will try to unload target image from the already destroyed
process.  This issue is reproducible only using real COI.

  -- Ilya