Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener <richard.guenther@gmail.com>]

On Mon, May 16, 2022 at 5:16 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> 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 :)

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 API
       is determined solely via the transfer vector.  */
   LAPI_UNSPECIFIED = 0,
   /* API level v1.  The linker provides add_symbols_v3, the plugin will use
      that and not any lower versions.  claim_file is thread-safe on the plugin
      side and add_symbols on the linker side.  */
   LAPI_V1 = 1
};

> > +
> > +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).

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 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).

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 = "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!

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-plugin
was compiled with.  So maybe it should be 'linker plugin identifier', not
'compiler identifier', likewise for version.  For the GCC source based plugin
it can of course still use GCCs identifier / version.

Richard.

> Alexander