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

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

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

On 6/16/22 10:00, Alexander Monakov wrote:
> On Thu, 16 Jun 2022, Martin Liška wrote:
> 
>> Hi.
>>
>> I'm sending updated version of the patch where I addressed the comments.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> I noticed a typo (no objection on the substance on the patch from me):

Good!

> 
>> --- a/include/plugin-api.h
>> +++ b/include/plugin-api.h
>> @@ -483,6 +483,34 @@ 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_UNSPECIFIED = 0,
>> +
>> +   /* API level v1.  The linker provides add_symbols_v3, add_symbols_v2,
> 
> This should be '*get_*symbols_v3, add_symbols_v2'.

Sure, fixed.

> 
>> +      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
>> +};
>> +
>> +/* 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.  */
>> +
>> +typedef
>> +enum linker_api_version
>> +(*ld_plugin_get_api_version) (const char *plugin_identifier, unsigned plugin_version,
>> +			      enum linker_api_version minimal_api_supported,
>> +			      enum linker_api_version maximal_api_supported,
>> +			      const char **linker_identifier,
>> +			      unsigned *linker_version);
> 
> IIRC Richi asked to mention which side owns the strings (does the receiver need
> to 'free' or 'strdup' them). Perhaps we could say they are owned by the
> originating side, but it might be even better to say they are unchanging to
> allow simply using string literals. Perhaps add something like this to the
> comment?
> 
>     Identifier pointers remain valid as long as the plugin is loaded.

I welcome the change and I'm sending a patch that incorporates that.

> 
>>  /* Values for the tv_tag field of the transfer vector.  */
>>  
>>  enum ld_plugin_tag
>> @@ -521,6 +549,7 @@ enum ld_plugin_tag
>>    LDPT_REGISTER_NEW_INPUT_HOOK,
>>    LDPT_GET_WRAP_SYMBOLS,
>>    LDPT_ADD_SYMBOLS_V2,
>> +  LDPT_GET_API_VERSION,
>>  };
> 
> I went checking if this is in sync with Binutils header and noticed that
> get_wrap_symbols and add_symbols_v2 are not even mentioned on the wiki page with
> plugin API documentation.

Yes, I know about that. I'm going to update wiki page once we get this in.

Cheers,
Martin

> 
> Alexander

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

From 3e01823f22282c7a46771c82f9622f7ff2929b18 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.
---
 include/plugin-api.h    | 31 +++++++++++++++++++++++++++++++
 lto-plugin/lto-plugin.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..cc3a43b44a6 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,35 @@ 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_UNSPECIFIED = 0,
+
+   /* 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 = 1
+};
+
+/* 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.
+  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, unsigned plugin_version,
+			      enum linker_api_version minimal_api_supported,
+			      enum linker_api_version maximal_api_supported,
+			      const char **linker_identifier,
+			      unsigned *linker_version);
+
 /* Values for the tv_tag field of the transfer vector.  */
 
 enum ld_plugin_tag
@@ -521,6 +550,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 +586,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/lto-plugin.c b/lto-plugin/lto-plugin.c
index 13118c4983c..208b26d5c4b 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -70,6 +70,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.  */
@@ -170,6 +171,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_UNSPECIFIED if there is not negotiation.  */
+static enum linker_api_version api_version = LAPI_UNSPECIFIED;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -1415,6 +1420,33 @@ process_option (const char *option)
   verbose = verbose || debug;
 }
 
+/* Negotiate linker API version.  */
+
+static void
+negotiate_api_version (void)
+{
+  const char *linker_identifier;
+  unsigned linker_version;
+
+  api_version = get_api_version ("GCC", GCC_VERSION, LAPI_UNSPECIFIED,
+				 LAPI_V1, &linker_identifier, &linker_version);
+
+  switch (api_version)
+    {
+    case LAPI_UNSPECIFIED:
+      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\n");
+      abort ();
+    }
+}
+
 /* Called by a linker after loading the plugin. TV is the transfer vector. */
 
 enum ld_plugin_status
@@ -1481,12 +1513,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