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

[Alexander Monakov <amonakov@ispras.ru>]

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