From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82d.google.com (mail-qt1-x82d.google.com [IPv6:2607:f8b0:4864:20::82d]) by sourceware.org (Postfix) with ESMTPS id C417A385625A for ; Tue, 17 May 2022 06:20:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C417A385625A Received: by mail-qt1-x82d.google.com with SMTP id x9so13679615qts.6 for ; Mon, 16 May 2022 23:20:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=TW+fMnk02S+bzM4FXYIOopCAnIA9bY4LHKBdzCCOS5o=; b=mrYBG7G7GdqtoBg7IBjl7wrz7Th+SiCUJR/4/+N+iCb5deEHC1IVA1LIr4xqEpCo9E bDx3L2LkYEdLWi75B7ghb01Y9GO8styKIapNacZ72baKPt1K0WdVUouAOm69P6wt1J9c Gfgu3DEQmYkAKk057OTN6VnFeOcO3Ube2ch4klbUPo9oKG1gt4w6J4Foplmy75sX1/0O r6SciTIHW2cINYH/Gy5SFCrlaJTugXs0/PPGvE0vJhuopCskQvewpKO2umCGm/AE6+Jr kJCRK9TJ6Lf+S0uV4sgfNJWukulJldKeWZy1b7jPXqnO4/O7sJYk7ZLsG451dqe79/5K /yIQ== X-Gm-Message-State: AOAM532w1Sf4OGv4f0IvGuNR/rjKQDc/y2BEHAZGAntXcCQkXHzJHvKO 2TDOaDRfPGFhTDsgUqGU8+532KQVtVf3iYCExlA= X-Google-Smtp-Source: ABdhPJxNzWmRAqr+Vsj97p7zUtigaHWSF4iWUcQCWFMjyOsoD31nTuRWkclbvGNMqu7gRzZM+OdJyn1hTqvJKbco2NA= X-Received: by 2002:a05:622a:134b:b0:2f3:d11e:3865 with SMTP id w11-20020a05622a134b00b002f3d11e3865mr18379134qtk.329.1652768414245; Mon, 16 May 2022 23:20:14 -0700 (PDT) MIME-Version: 1.0 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> <73313b19-634b-c4e3-ff6e-e72511258cd3@ispras.ru> In-Reply-To: <73313b19-634b-c4e3-ff6e-e72511258cd3@ispras.ru> From: Richard Biener Date: Tue, 17 May 2022 08:20:02 +0200 Message-ID: Subject: Re: [PATCH] lto-plugin: add support for feature detection To: Alexander Monakov Cc: =?UTF-8?Q?Martin_Li=C5=A1ka?= , Rui Ueyama , Jan Hubicka , GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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 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: Tue, 17 May 2022 06:20:18 -0000 On Mon, May 16, 2022 at 5:16 PM Alexander Monakov wrot= e: > > On Mon, 16 May 2022, Martin Li=C5=A1ka 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 :) I think your comments are very useful. > > --- 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, const char * for the linker_identifier? > > + int preferred_linker_api, > > + const char **compiler_identifier, > > + int *compiler_version); I think a signed int for the version isn't a good fit so use unsigned? I miss documentation on who owns linker_identifier and *compiler_identifier= . I also miss documentation on the supported linker_apis and what they mean. Maybe we want an enum here? enum linker_api_version { /* The linker/plugin do not implement any of the API levels below, the A= PI is determined solely via the transfer vector. */ LAPI_UNSPECIFIED =3D 0, /* API level v1. The linker provides add_symbols_v3, the plugin will us= e that and not any lower versions. claim_file is thread-safe on the pl= ugin side and add_symbols on the linker side. */ LAPI_V1 =3D 1 }; > > + > > +typedef > > +enum ld_plugin_status > > +(*ld_plugin_register_get_api_version) (ld_plugin_get_api_version handl= er); > > + > > /* 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 plug= in > API seems to consist of callbacks-that-register-callbacks, and I'm with h= im > 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 t= o > 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). That sounds reasonable. It then basically extends the onload() API without introducing onload_v2(). > > 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 n= ot 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 =3D 1; > > > > static struct plugin_file_info *claimed_files =3D NULL; > > static unsigned int num_claimed_files =3D 0; > > @@ -1407,6 +1412,29 @@ process_option (const char *option) > > verbose =3D 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 er= ror out > if the intersection is empty (and otherwise return the highest version th= ey both > support as the 'negotiated' one). That sounds better indeed. As said above the API level means nothing if constraints are not thoroughly documented. > > + const char **compiler_identifier, > > + int *compiler_version) > > +{ > > + *compiler_identifier =3D "GCC"; > > + *compiler_version =3D GCC_VERSION; > > Note that I'm chiming in here because I worked on a tool that used LTO pl= ugin > API to discover symbol/section dependencies with high accuracy. So I'd li= ke to > remind that implementors/consumers of this API are not necessarily compil= ers! Indeed. For example nm/ar when auto-loading the plugin might operate on LTO IL that is produced by a compiler newer than the sources the linker-plu= gin was compiled with. So maybe it should be 'linker plugin identifier', not 'compiler identifier', likewise for version. For the GCC source based plug= in it can of course still use GCCs identifier / version. Richard. > Alexander