Re: [PATCH 3/3] lto-plugin: implement LDPT_GET_API_VERSION

["Martin Liška" <mliska@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

On 7/11/22 11:55, Richard Biener wrote:
> On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>>
>> On Mon, 11 Jul 2022, Rui Ueyama wrote:
>>
>>>> but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
>>>> is also wrong. It really should check how given [min; max] range intersects
>>>> with its own range of supported versions.
>>>
>>> Currently only one version is defined which is LAPI_V1. I don't think
>>> LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
>>> value. No ordering should be defined between a defined value and an
>>> unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
>>> LAPI_V0.
>>
>> You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
>> advertise it (thread safety of claim_file in this particular case).
> 

Hi.

All right, I think we should rename LAPI_UNSPECIFIED to LAPI_V0 in order
to support minimal_api_supported == LAPI_V0.

> So with LAPI_UNSPECIFIED all the plugin gets is the linker name and version.
> Clarifying the documentation on LAPI_UNSPECIFIED might be nice, also
> what the expectation is on the linker when the plugin returns
> LAPI_UNSPECIFIED when it speficied minimal_api_supported == V1.

I've clarified that linker should return a value that is in range
[minimal_api_supported, maximal_api_supported] and added an abort
if it's not the case.

Having that, mold should respect if maximal_api_supported == LAPI_V0 is returned
by a plug-in (happens now as we miss locking for some targets).

Martin

> "minimal_api_supported == LAPI_UNSPECIFIED" does not make much
> sense if using Ruis reading of this value?
> 
>> And you still should check the intersection of supported API ranges
>> and give a sane diagnostic when min_api_supported advertised by the plugin
>> exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
>> case).
>>
>> Alexander

[-- Attachment #2: 0001-lto-plugin-implement-LDPT_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 6597 bytes --]

From 7905b58c062774deffcd3d5adfa893e17bcc0f26 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] lto-plugin: implement LDPT_GET_API_VERSION

include/ChangeLog:

	* plugin-api.h (enum linker_api_version): New enum.
	(ld_plugin_get_api_version): New.
	(enum ld_plugin_tag): Add LDPT_GET_API_VERSION.
	(struct ld_plugin_tv): Add tv_get_api_version.

lto-plugin/ChangeLog:

	* lto-plugin.c (negotiate_api_version): New.
	(onload): Negotiate API version.
	* Makefile.am: Add -DBASE_VERSION.
	* Makefile.in: Regenerate.
---
 include/plugin-api.h    | 33 +++++++++++++++++++++++++++++
 lto-plugin/Makefile.am  |  2 +-
 lto-plugin/Makefile.in  |  2 +-
 lto-plugin/lto-plugin.c | 47 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..e4ed1d139bb 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,37 @@ enum ld_plugin_level
   LDPL_FATAL
 };
 
+/* Contract between a plug-in and a linker.  */
+
+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_V0,
+
+   /* API level v1.  The linker provides get_symbols_v3, add_symbols_v2,
+      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
+};
+
+/* The linker's interface for API version negotiation.  A plug-in calls
+  the function (with its IDENTIFIER and VERSION), plus minimal and maximal
+  version of linker_api_version is provided.  Linker then returns selected
+  API version and provides its IDENTIFIER and VERSION.  The returned value
+  by linker must be in range [MINIMAL_API_SUPPORTED, MAXIMAL_API_SUPPORTED].
+  Identifier pointers remain valid as long as the plugin is loaded.  */
+
+typedef
+enum linker_api_version
+(*ld_plugin_get_api_version) (const char *plugin_identifier,
+			      const char *plugin_version,
+			      enum linker_api_version minimal_api_supported,
+			      enum linker_api_version maximal_api_supported,
+			      const char **linker_identifier,
+			      const char **linker_version);
+
 /* Values for the tv_tag field of the transfer vector.  */
 
 enum ld_plugin_tag
@@ -521,6 +552,7 @@ enum ld_plugin_tag
   LDPT_REGISTER_NEW_INPUT_HOOK,
   LDPT_GET_WRAP_SYMBOLS,
   LDPT_ADD_SYMBOLS_V2,
+  LDPT_GET_API_VERSION,
 };
 
 /* The plugin transfer vector.  */
@@ -556,6 +588,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_get_api_version tv_get_api_version;
   } tv_u;
 };
 
diff --git a/lto-plugin/Makefile.am b/lto-plugin/Makefile.am
index 81362eafc36..482946e4dd5 100644
--- a/lto-plugin/Makefile.am
+++ b/lto-plugin/Makefile.am
@@ -8,7 +8,7 @@ target_noncanonical := @target_noncanonical@
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/Makefile.in b/lto-plugin/Makefile.in
index 2033dd9b7c2..9453bc7d607 100644
--- a/lto-plugin/Makefile.in
+++ b/lto-plugin/Makefile.in
@@ -343,7 +343,7 @@ AUTOMAKE_OPTIONS = no-dependencies
 gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER)
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 7927dca60a4..e9afd2fb76d 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -180,6 +180,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_get_api_version get_api_version;
+
+/* By default, use version LAPI_V0 if there is not negotiation.  */
+static enum linker_api_version api_version = LAPI_V0;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -1428,6 +1432,43 @@ process_option (const char *option)
   verbose = verbose || debug;
 }
 
+/* Negotiate linker API version.  */
+
+static void
+negotiate_api_version (void)
+{
+  const char *linker_identifier;
+  const char *linker_version;
+
+  enum linker_api_version supported_api = LAPI_V0;
+#if HAVE_PTHREAD_LOCKING
+  supported_api = LAPI_V1;
+#endif
+
+  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V0,
+				 supported_api, &linker_identifier, &linker_version);
+  if (api_version > supported_api)
+    {
+      fprintf (stderr, "requested an unsupported API version (%d)\n", api_version);
+      abort ();
+    }
+
+  switch (api_version)
+    {
+    case LAPI_V0:
+      break;
+    case LAPI_V1:
+      check (get_symbols_v3, LDPL_FATAL,
+	     "get_symbols_v3 required for API version 1");
+      check (add_symbols_v2, LDPL_FATAL,
+	     "add_symbols_v2 required for API version 1");
+      break;
+    default:
+      fprintf (stderr, "unsupported API version (%d)\n", api_version);
+      abort ();
+    }
+}
+
 /* Called by a linker after loading the plugin. TV is the transfer vector. */
 
 enum ld_plugin_status
@@ -1496,12 +1537,18 @@ onload (struct ld_plugin_tv *tv)
 	  /* We only use this to make user-friendly temp file names.  */
 	  link_output_name = p->tv_u.tv_string;
 	  break;
+	case LDPT_GET_API_VERSION:
+	  get_api_version = p->tv_u.tv_get_api_version;
+	  break;
 	default:
 	  break;
 	}
       p++;
     }
 
+  if (get_api_version)
+    negotiate_api_version ();
+
   check (register_claim_file, LDPL_FATAL, "register_claim_file not found");
   check (add_symbols, LDPL_FATAL, "add_symbols not found");
   status = register_claim_file (claim_file_handler);
-- 
2.36.1