From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id C339B3858D28 for ; Wed, 6 Jul 2022 21:08:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C339B3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.92,250,1650960000"; d="scan'208,223";a="78399858" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa4.mentor.iphmx.com with ESMTP; 06 Jul 2022 13:08:13 -0800 IronPort-SDR: OEQeMMxDmN++CeN5LagwnYSCmjjQ2cxFyRnMy0gZjSOrN35EZWnaVhkva3ccSz2LQdMYQxPR+o oWsCLH41wIT5W1dKLCxLEnw2oUr6u8IXuicmrDBv1Hj4xIbdi5KqYG7ijmWVPvCJ1PRHhzUOJK kMUFCIS7iE6qu72EZtkGJG4dYIbIwaXGwNZCgbw2EtITnUsili4AIIPN9RuIOQxwC/jXrQjQl1 Mcil/g5bxuV1vdH1+Hx04ZzBmd++E0KwbTO08s2hHTkT5cqm2Oz7+OpkCupmQqzURs7A6rUHtL AV8= From: Thomas Schwinge To: Tobias Burnus , CC: Jakub Jelinek Subject: Re: Restore 'GOMP_offload_unregister_ver' functionality (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp) In-Reply-To: <1f1aea0a-dbe8-7b95-6bee-9d157faa320e@codesourcery.com> References: <07fec82a-41cf-fdc5-6307-c068dd95ef1a@mentor.com> <7f9c91c1-a479-f94f-ac14-1d6827ce671b@codesourcery.com> <5576fa00-0ddd-8046-17c1-d1cea82bdcf5@codesourcery.com> <77331328-4961-9dab-db58-b5b03daf218c@codesourcery.com> <16ca2aa4-7e73-cf9d-9482-dd59f5b0cdae@codesourcery.com> <87tu7u4j4r.fsf@euler.schwinge.homeip.net> <1f1aea0a-dbe8-7b95-6bee-9d157faa320e@codesourcery.com> User-Agent: Notmuch/0.29.1+93~g67ed7df (https://notmuchmail.org) Emacs/27.1 (i686-pc-linux-gnu) Date: Wed, 6 Jul 2022 23:08:03 +0200 Message-ID: <87y1x6rlu4.fsf@dirichlet.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-11.mgc.mentorg.com (139.181.222.11) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jul 2022 21:08:16 -0000 --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Tobias! On 2022-07-06T15:59:59+0200, Tobias Burnus 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 unloa= ding. >> GOMP_offload_unregister_ver (unsigned version, const void *host_table, >> /* Remove image from array of pending images. */ >> + bool found =3D false; >> for (i =3D 0; i < num_offload_images; i++) >> if (offload_images[i].target_data =3D=3D target_data) >> { >> offload_images[i] =3D offload_images[--num_offload_images]; >> + found =3D true; >> break; >> } >> + assert (found); >> >> gomp_mutex_unlock (®ister_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_fa= tal. > IMHO, gomp_error is better than gomp_fatal as libgomp then continues c= leaning > 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 =3D=3D 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=C3=BC=C3=9Fe Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-Restore-GOMP_offload_unregister_ver-functionality.patch" >From 3f05e03d6cfdf723ca0556318b6a9aa37be438e7 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Tue, 5 Jul 2022 18:23:15 +0200 Subject: [PATCH] Restore 'GOMP_offload_unregister_ver' functionality The recent commit 683f11843974f0bdf42f79cdcbb0c2b43c7b81b0 "OpenMP: Move omp requires checks to libgomp" changed the 'GOMP_offload_register_ver' interface but didn't change 'GOMP_offload_unregister_ver' accordingly, so we're no longer actually unregistering. gcc/ * config/gcn/mkoffload.cc (process_obj): Clarify 'target_data' -> '[...]_data'. * config/nvptx/mkoffload.cc (process): Likewise. libgomp/ * target.c (GOMP_offload_register_ver): Clarify 'target_data' -> 'data'. (GOMP_offload_unregister_ver): Likewise. Fix up 'target_data'. --- gcc/config/gcn/mkoffload.cc | 8 ++++---- gcc/config/nvptx/mkoffload.cc | 8 ++++---- libgomp/target.c | 30 +++++++++++++++++++++++------- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/gcc/config/gcn/mkoffload.cc b/gcc/config/gcn/mkoffload.cc index b8b3fecfcb4..d2464332275 100644 --- a/gcc/config/gcn/mkoffload.cc +++ b/gcc/config/gcn/mkoffload.cc @@ -692,13 +692,13 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requires) len); fprintf (cfile, - "static const struct gcn_image_desc {\n" + "static const struct gcn_data {\n" " uintptr_t omp_requires_mask;\n" " const struct gcn_image *gcn_image;\n" " unsigned kernel_count;\n" " const struct hsa_kernel_description *kernel_infos;\n" " unsigned global_variable_count;\n" - "} target_data = {\n" + "} gcn_data = {\n" " %d,\n" " &gcn_image,\n" " sizeof (gcn_kernels) / sizeof (gcn_kernels[0]),\n" @@ -723,7 +723,7 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requires) fprintf (cfile, "static __attribute__((constructor)) void init (void)\n" "{\n" " GOMP_offload_register_ver (%#x, __OFFLOAD_TABLE__," - " %d/*GCN*/, &target_data);\n" + " %d/*GCN*/, &gcn_data);\n" "};\n", GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_GCN), GOMP_DEVICE_GCN); @@ -731,7 +731,7 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requires) fprintf (cfile, "static __attribute__((destructor)) void fini (void)\n" "{\n" " GOMP_offload_unregister_ver (%#x, __OFFLOAD_TABLE__," - " %d/*GCN*/, &target_data);\n" + " %d/*GCN*/, &gcn_data);\n" "};\n", GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_GCN), GOMP_DEVICE_GCN); diff --git a/gcc/config/nvptx/mkoffload.cc b/gcc/config/nvptx/mkoffload.cc index d8c81eb0547..0fa5f4423bf 100644 --- a/gcc/config/nvptx/mkoffload.cc +++ b/gcc/config/nvptx/mkoffload.cc @@ -310,7 +310,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) fprintf (out, "\n};\n\n"); fprintf (out, - "static const struct nvptx_tdata {\n" + "static const struct nvptx_data {\n" " uintptr_t omp_requires_mask;\n" " const struct ptx_obj *ptx_objs;\n" " unsigned ptx_num;\n" @@ -318,7 +318,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) " unsigned var_num;\n" " const struct nvptx_fn *fn_names;\n" " unsigned fn_num;\n" - "} target_data = {\n" + "} nvptx_data = {\n" " %d, ptx_objs, sizeof (ptx_objs) / sizeof (ptx_objs[0]),\n" " var_mappings," " sizeof (var_mappings) / sizeof (var_mappings[0]),\n" @@ -344,7 +344,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) fprintf (out, "static __attribute__((constructor)) void init (void)\n" "{\n" " GOMP_offload_register_ver (%#x, __OFFLOAD_TABLE__," - " %d/*NVIDIA_PTX*/, &target_data);\n" + " %d/*NVIDIA_PTX*/, &nvptx_data);\n" "};\n", GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_NVIDIA_PTX), GOMP_DEVICE_NVIDIA_PTX); @@ -352,7 +352,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) fprintf (out, "static __attribute__((destructor)) void fini (void)\n" "{\n" " GOMP_offload_unregister_ver (%#x, __OFFLOAD_TABLE__," - " %d/*NVIDIA_PTX*/, &target_data);\n" + " %d/*NVIDIA_PTX*/, &nvptx_data);\n" "};\n", GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_NVIDIA_PTX), GOMP_DEVICE_NVIDIA_PTX); diff --git a/libgomp/target.c b/libgomp/target.c index 4dac81862d7..c66c61b0621 100644 --- a/libgomp/target.c +++ b/libgomp/target.c @@ -2334,23 +2334,29 @@ gomp_requires_to_name (char *buf, size_t size, int requires_mask) /* This function should be called from every offload image while loading. It gets the descriptor of the host func and var tables HOST_TABLE, TYPE of - the target, and TARGET_DATA needed by target plugin. */ + the target, and DATA. */ void GOMP_offload_register_ver (unsigned version, const void *host_table, - int target_type, const void *target_data) + int target_type, const void *data) { int i; - int omp_req = 0; if (GOMP_VERSION_LIB (version) > GOMP_VERSION) gomp_fatal ("Library too old for offload (version %u < %u)", GOMP_VERSION, GOMP_VERSION_LIB (version)); + int omp_req; + const void *target_data; if (GOMP_VERSION_LIB (version) > 1) { - omp_req = (int) (size_t) ((void **) target_data)[0]; - target_data = &((void **) target_data)[1]; + omp_req = (int) (size_t) ((void **) data)[0]; + target_data = &((void **) data)[1]; + } + else + { + omp_req = 0; + target_data = data; } gomp_mutex_lock (®ister_lock); @@ -2413,14 +2419,24 @@ GOMP_offload_register (const void *host_table, int target_type, /* This function should be called from every offload image while unloading. It gets the descriptor of the host func and var tables HOST_TABLE, TYPE of - the target, and TARGET_DATA needed by target plugin. */ + the target, and DATA. */ void GOMP_offload_unregister_ver (unsigned version, const void *host_table, - int target_type, const void *target_data) + int target_type, const void *data) { int i; + if (GOMP_VERSION_LIB (version) > GOMP_VERSION) + gomp_fatal ("Library too old for offload (version %u < %u)", + GOMP_VERSION, GOMP_VERSION_LIB (version)); + + const void *target_data; + if (GOMP_VERSION_LIB (version) > 1) + target_data = &((void **) data)[1]; + else + target_data = data; + gomp_mutex_lock (®ister_lock); /* Unload image from all initialized devices. */ -- 2.35.1 --=-=-=--