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