Hi Tobias!

On 2022-07-06T15:59:59+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 06.07.22 12:42, Thomas Schwinge wrote:
>> --- a/libgomp/target.c
>> +++ b/libgomp/target.c

>>   /* This function should be called from every offload image while unloading.

>>   GOMP_offload_unregister_ver (unsigned version, const void *host_table,

>>     /* Remove image from array of pending images.  */
>> +  bool found = false;
>>     for (i = 0; i < num_offload_images; i++)
>>       if (offload_images[i].target_data == target_data)
>>         {
>>      offload_images[i] = offload_images[--num_offload_images];
>> +    found = true;
>>      break;
>>         }
>> +  assert (found);
>>
>>     gomp_mutex_unlock (&register_lock);
>>   }
>
> ... I don't like that libgomp crashes without any helpful message in that case.
>
> In my opinion:
> * Either we assume that it is unlikely to occur - ignore it.
>    (Matches the current implementation: do nothing.)
> * Or we want to have some diagnostic in case it occurs. But in that case,
>    it should be some explicit diagnostic printed by gomp_error or gomp_fatal.
>    IMHO, gomp_error is better than gomp_fatal as libgomp then continues cleaning
>    up after this error, which IMHO makes more sense that just aborting.

I'd be fine to change this into a 'gomp_error', but I don't think it's
necessary.  Maybe that wasn't obvious (and I should add a source code
comment), but my point here is that this situation really should never
arise (hence, if it does: internal error, thus 'assert').  Or, in other
words, such a check should've been present in the original implementation
already -- and would then have flagged your patch as being incomplete in
that function.

Thinking about it again, shouldn't we also add a corresponding
sanity-check ('assert') to 'GOMP_offload_register_ver', such that the
newly registered offload image must not already be present in
'offload_images'?  (Isn't that understanding also supported by the
'break' in 'if (offload_images[i].target_data == target_data)' in
'GOMP_offload_unregister_ver', as cited above: that no duplicates are
expected?)

That's at least my understanding of the situation; happy to hear if I'm
wrong.  (It's a pity that we're totally devoid of test cases for dynamic
registration/unregistration of offload images...)

Anyway: it's totally fine to address (or not, if so desired) this
sanity-check aspect independently of the other changes, so I've backed
that out, and then pushed to master branch
commit 3f05e03d6cfdf723ca0556318b6a9aa37be438e7
"Restore 'GOMP_offload_unregister_ver' functionality", see attached.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955