From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by sourceware.org (Postfix) with ESMTPS id 90F5A385780F for ; Mon, 16 May 2022 15:16:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 90F5A385780F Received: from [10.10.3.121] (unknown [10.10.3.121]) by mail.ispras.ru (Postfix) with ESMTPS id 461984076253; Mon, 16 May 2022 15:16:51 +0000 (UTC) Date: Mon, 16 May 2022 18:16:51 +0300 (MSK) From: Alexander Monakov To: =?ISO-8859-15?Q?Martin_Li=A8ka?= cc: Richard Biener , Rui Ueyama , Jan Hubicka , GCC Patches Subject: Re: [PATCH] lto-plugin: add support for feature detection In-Reply-To: <35c2871d-4bab-efb4-317f-acac44591040@suse.cz> Message-ID: <73313b19-634b-c4e3-ff6e-e72511258cd3@ispras.ru> References: <7788e58e-ce41-d25b-eefe-5f9c966a2ff2@ispras.ru> <97c39ec3-33f-36e1-ebd7-498e3772c9a3@ispras.ru> <245cc396-ad40-cb98-b171-e56e3d9c1c2@ispras.ru> <69142cd0-1947-e7d3-50ad-d9010f46fbf0@suse.cz> <35c2871d-4bab-efb4-317f-acac44591040@suse.cz> MIME-Version: 1.0 X-Spam-Status: No, score=-8.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, 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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Mon, 16 May 2022 15:16:59 -0000 On Mon, 16 May 2022, Martin Liška wrote: > I've implemented first version of the patch, please take a look. I'll comment on the patch, feel free to inform me when I should back off with forcing my opinion in this thread :) > --- a/include/plugin-api.h > +++ b/include/plugin-api.h > @@ -483,6 +483,18 @@ enum ld_plugin_level > LDPL_FATAL > }; > > +/* The linker's interface for API version negotiation. */ > + > +typedef > +int (*ld_plugin_get_api_version) (char *linker_identifier, int linker_version, > + int preferred_linker_api, > + const char **compiler_identifier, > + int *compiler_version); > + > +typedef > +enum ld_plugin_status > +(*ld_plugin_register_get_api_version) (ld_plugin_get_api_version handler); > + > /* Values for the tv_tag field of the transfer vector. */ > > enum ld_plugin_tag > @@ -521,6 +533,7 @@ enum ld_plugin_tag > LDPT_REGISTER_NEW_INPUT_HOOK, > LDPT_GET_WRAP_SYMBOLS, > LDPT_ADD_SYMBOLS_V2, > + LDPT_REGISTER_GET_API_VERSION, > }; > > /* The plugin transfer vector. */ > @@ -556,6 +569,7 @@ struct ld_plugin_tv > ld_plugin_get_input_section_size tv_get_input_section_size; > ld_plugin_register_new_input tv_register_new_input; > ld_plugin_get_wrap_symbols tv_get_wrap_symbols; > + ld_plugin_register_get_api_version tv_register_get_api_version; > } tv_u; > }; Here I disagree with the overall design. Rui already pointed out how plugin API seems to consist of callbacks-that-register-callbacks, and I'm with him on that, let's not make that worse. On a more serious note, this pattern: * the linker provides register_get_api_version entrypoint * the plugin registers its get_api_version implementation * the linker uses the provided function pointer is problematic because the plugin doesn't know when the linker is going to invoke its callback (or maybe the linker won't do that at all). I'd recommend to reduce the level of indirection, remove the register_ callback, and simply require that if LDPT_GET_API_VERSION is provided, the plugin MUST invoke it before returning from onload, i.e.: * the linker invokes onload with LDPT_GET_API_VERSION in 'transfer vector' * the plugin iterates over the transfer vector and notes if LDPT_GET_API_VERSION is seen * if not, the plugin knows the linker is predates its introduction * if yes, the plugin invokes it before returning from onload * the linker now knows the plugin version (either one provided via LDPT_GET_API_VERSION, or 'old' if the callback wasn't invoked). > diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c > index 00b760636dc..49484decd89 100644 > --- a/lto-plugin/lto-plugin.c > +++ b/lto-plugin/lto-plugin.c > @@ -69,6 +69,7 @@ along with this program; see the file COPYING3. If not see > #include "../gcc/lto/common.h" > #include "simple-object.h" > #include "plugin-api.h" > +#include "ansidecl.h" > > /* We need to use I64 instead of ll width-specifier on native Windows. > The reason for this is that older MS-runtimes don't support the ll. */ > @@ -166,6 +167,10 @@ static ld_plugin_add_input_file add_input_file; > static ld_plugin_add_input_library add_input_library; > static ld_plugin_message message; > static ld_plugin_add_symbols add_symbols, add_symbols_v2; > +static ld_plugin_register_get_api_version register_get_api_version; > + > +/* By default, use version 1 if there is not negotiation. */ > +static int used_api_version = 1; > > static struct plugin_file_info *claimed_files = NULL; > static unsigned int num_claimed_files = 0; > @@ -1407,6 +1412,29 @@ process_option (const char *option) > verbose = verbose || debug; > } > > +static int > +get_api_version (char *linker_identifier, int linker_version, > + int preferred_linker_api, The 'preferred' qualifier seems vague. If you go with my suggestion above, I'd suggest to pass lowest and highest supported version number, and the linker can check if that intersects with its range of supported versions, and error out if the intersection is empty (and otherwise return the highest version they both support as the 'negotiated' one). > + const char **compiler_identifier, > + int *compiler_version) > +{ > + *compiler_identifier = "GCC"; > + *compiler_version = GCC_VERSION; Note that I'm chiming in here because I worked on a tool that used LTO plugin API to discover symbol/section dependencies with high accuracy. So I'd like to remind that implementors/consumers of this API are not necessarily compilers! Alexander