[patch][rfc] Giving plugins access to gold's file views

[patch][rfc] Giving plugins access to gold's file views

[Rafael Avila de Espindola]

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

The attached gold patch adds a get_view callback that plugins can used 
to request a view of a given file. There are many possible variations on 
how do to this. The implementation in the patch has the following design 
decisions:

*) It is only possible to ask for views of the entire file. The reason 
is that during LTO it is more efficient to merge files as we read them, 
so I didn't see a lot of reason to add support to mapping only parts of 
a file. In any case, I can add an offset and size argument if you want.

*) The call works when called from the claim_file hook or from the 
all_symbols_read hook. I only tested the first one, but making it work 
on both cases looked easy.

*) The plugin has no guarantees of how long the view will be valid for. 
It it wants to see the view in a subsequent callback, it has to ask for 
it again.

*) The "aligned" and "cache" arguments are not exposed. Since the 
behaviour with aligned set to true is the copy in into anonymous memory, 
the plugin can just read the file itself. The "cache" value is always 
set to false. If the plugin claims the file, it will probably use it all 
at once. If it doesn't and gold then asks for a cached view, 
find_or_make_view correctly calls set_cache.

If you are OK with this, I will finish it up (comments, interface 
documentation and ChangeLog), split the bits that have to be committed 
in gcc first and send an updated version for review.

Thanks,
Rafael


[-- Attachment #2: gold.patch --]
[-- Type: text/x-patch, Size: 4014 bytes --]

diff --git a/gold/plugin.cc b/gold/plugin.cc
index 214eff3..6845ff9 100644
--- a/gold/plugin.cc
+++ b/gold/plugin.cc
@@ -69,6 +69,9 @@ static enum ld_plugin_status
 get_input_file(const void *handle, struct ld_plugin_input_file *file);
 
 static enum ld_plugin_status
+get_view(const void *handle, const void **viewp);
+
+static enum ld_plugin_status
 release_input_file(const void *handle);
 
 static enum ld_plugin_status
@@ -130,7 +133,7 @@ Plugin::load()
   sscanf(ver, "%d.%d", &major, &minor);
 
   // Allocate and populate a transfer vector.
-  const int tv_fixed_size = 16;
+  const int tv_fixed_size = 17;
   int tv_size = this->args_.size() + tv_fixed_size;
   ld_plugin_tv* tv = new ld_plugin_tv[tv_size];
 
@@ -189,6 +192,10 @@ Plugin::load()
   tv[i].tv_u.tv_get_input_file = get_input_file;
 
   ++i;
+  tv[i].tv_tag = LDPT_GET_VIEW;
+  tv[i].tv_u.tv_get_view = get_view;
+
+  ++i;
   tv[i].tv_tag = LDPT_RELEASE_INPUT_FILE;
   tv[i].tv_u.tv_release_input_file = release_input_file;
 
@@ -635,6 +642,32 @@ Plugin_manager::release_input_file(unsigned int handle)
   return LDPS_OK;
 }
 
+ld_plugin_status
+Plugin_manager::get_view(unsigned int handle, const void **viewp)
+{
+  off_t offset;
+  size_t filesize;
+  Input_file *input_file;
+  if (this->objects_.size() == handle) {
+    // We are being called from the claim_file hook.
+    const struct ld_plugin_input_file &f = this->plugin_input_file_;
+    offset = f.offset;
+    filesize = f.filesize;
+    input_file = this->input_file_;
+  } else {
+    // An already claimed file.
+    Pluginobj* obj = this->object(handle);
+    if (obj == NULL)
+      return LDPS_BAD_HANDLE;
+    offset = obj->offset();
+    filesize = obj->filesize();
+    input_file = obj->input_file();
+  }
+  *viewp = (void*) input_file->file().get_view(offset, 0, filesize, false,
+                                               false);
+  return LDPS_OK;
+}
+
 // Add a new library path.
 
 ld_plugin_status
@@ -1247,6 +1280,15 @@ release_input_file(const void* handle)
   return parameters->options().plugins()->release_input_file(obj_index);
 }
 
+static enum ld_plugin_status
+get_view(const void *handle, const void **viewp)
+{
+  gold_assert(parameters->options().has_plugins());
+  unsigned int obj_index =
+      static_cast<unsigned int>(reinterpret_cast<intptr_t>(handle));
+  return parameters->options().plugins()->get_view(obj_index, viewp);
+}
+
 // Get the symbol resolution info for a plugin-claimed input file.
 
 static enum ld_plugin_status
diff --git a/gold/plugin.h b/gold/plugin.h
index c26414d..87747bf 100644
--- a/gold/plugin.h
+++ b/gold/plugin.h
@@ -245,6 +245,9 @@ class Plugin_manager
   ld_plugin_status
   get_input_file(unsigned int handle, struct ld_plugin_input_file* file);
 
+  ld_plugin_status
+  get_view(unsigned int handle, const void **viewp);
+
   // Release an input file.
   ld_plugin_status
   release_input_file(unsigned int handle);
diff --git a/include/plugin-api.h b/include/plugin-api.h
index 956df00..7450a9e 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -203,6 +203,10 @@ enum ld_plugin_status
 (*ld_plugin_get_input_file) (const void *handle,
                              struct ld_plugin_input_file *file);
 
+typedef
+enum ld_plugin_status
+(*ld_plugin_get_view) (const void *handle, const void **viewp);
+
 /* The linker's interface for releasing the input file.  */
 
 typedef
@@ -269,7 +273,8 @@ enum ld_plugin_tag
   LDPT_ADD_INPUT_LIBRARY,
   LDPT_OUTPUT_NAME,
   LDPT_SET_EXTRA_LIBRARY_PATH,
-  LDPT_GNU_LD_VERSION
+  LDPT_GNU_LD_VERSION,
+  LDPT_GET_VIEW
 };
 
 /* The plugin transfer vector.  */
@@ -289,6 +294,7 @@ struct ld_plugin_tv
     ld_plugin_add_input_file tv_add_input_file;
     ld_plugin_message tv_message;
     ld_plugin_get_input_file tv_get_input_file;
+    ld_plugin_get_view tv_get_view;
     ld_plugin_release_input_file tv_release_input_file;
     ld_plugin_add_input_library tv_add_input_library;
     ld_plugin_set_extra_library_path tv_set_extra_library_path;

Re: [patch][rfc] Giving plugins access to gold's file views

[Ian Lance Taylor]

Rafael Avila de Espindola <respindola@mozilla.com> writes:

> *) It is only possible to ask for views of the entire file. The reason
> is that during LTO it is more efficient to merge files as we read
> them, so I didn't see a lot of reason to add support to mapping only
> parts of a file. In any case, I can add an offset and size argument if
> you want.
>
> *) The call works when called from the claim_file hook or from the
> all_symbols_read hook. I only tested the first one, but making it work
> on both cases looked easy.
>
> *) The plugin has no guarantees of how long the view will be valid
> for. It it wants to see the view in a subsequent callback, it has to
> ask for it again.
>
> *) The "aligned" and "cache" arguments are not exposed. Since the
> behaviour with aligned set to true is the copy in into anonymous
> memory, the plugin can just read the file itself. The "cache" value is
> always set to false. If the plugin claims the file, it will probably
> use it all at once. If it doesn't and gold then asks for a cached
> view, find_or_make_view correctly calls set_cache.
>
> If you are OK with this, I will finish it up (comments, interface
> documentation and ChangeLog), split the bits that have to be committed
> in gcc first and send an updated version for review.

I'm OK with this approach.

We already expose the file descriptor in the plugin interface, so this
is just for efficiency, right?

> +  if (this->objects_.size() == handle) {
> +    // We are being called from the claim_file hook.
> +    const struct ld_plugin_input_file &f = this->plugin_input_file_;
> +    offset = f.offset;
> +    filesize = f.filesize;
> +    input_file = this->input_file_;
> +  } else {
> +    // An already claimed file.
> +    Pluginobj* obj = this->object(handle);
> +    if (obj == NULL)
> +      return LDPS_BAD_HANDLE;
> +    offset = obj->offset();
> +    filesize = obj->filesize();
> +    input_file = obj->input_file();
> +  }

Braces need to be GNU style.

Ian

Re: [patch][rfc] Giving plugins access to gold's file views

[Rafael Avila de Espindola]

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

On 11-03-21 07:22 PM, Ian Lance Taylor wrote:
> Rafael Avila de Espindola<respindola@mozilla.com>  writes:
>
>> *) It is only possible to ask for views of the entire file. The reason
>> is that during LTO it is more efficient to merge files as we read
>> them, so I didn't see a lot of reason to add support to mapping only
>> parts of a file. In any case, I can add an offset and size argument if
>> you want.
>>
>> *) The call works when called from the claim_file hook or from the
>> all_symbols_read hook. I only tested the first one, but making it work
>> on both cases looked easy.
>>
>> *) The plugin has no guarantees of how long the view will be valid
>> for. It it wants to see the view in a subsequent callback, it has to
>> ask for it again.
>>
>> *) The "aligned" and "cache" arguments are not exposed. Since the
>> behaviour with aligned set to true is the copy in into anonymous
>> memory, the plugin can just read the file itself. The "cache" value is
>> always set to false. If the plugin claims the file, it will probably
>> use it all at once. If it doesn't and gold then asks for a cached
>> view, find_or_make_view correctly calls set_cache.
>>
>> If you are OK with this, I will finish it up (comments, interface
>> documentation and ChangeLog), split the bits that have to be committed
>> in gcc first and send an updated version for review.
>
> I'm OK with this approach.
>
> We already expose the file descriptor in the plugin interface, so this
> is just for efficiency, right?

Correct. Running strace on a link I noticed many consecutive calls to 
mmap on the same fd, one from gold, one from the plugin.

>> +  if (this->objects_.size() == handle) {
>> +    // We are being called from the claim_file hook.
>> +    const struct ld_plugin_input_file&f = this->plugin_input_file_;
>> +    offset = f.offset;
>> +    filesize = f.filesize;
>> +    input_file = this->input_file_;
>> +  } else {
>> +    // An already claimed file.
>> +    Pluginobj* obj = this->object(handle);
>> +    if (obj == NULL)
>> +      return LDPS_BAD_HANDLE;
>> +    offset = obj->offset();
>> +    filesize = obj->filesize();
>> +    input_file = obj->input_file();
>> +  }
>
> Braces need to be GNU style.

Fixed. The two patches are attached.

2010-03-22  Rafael Ávila de Espíndola <respindola@mozilla.com>

	* plugin-api.h (ld_plugin_get_view): New.
	(ld_plugin_tag): Add LDPT_GET_VIEW.
	(ld_plugin_tv): Add tv_get_view.

2010-03-22  Rafael Ávila de Espíndola <respindola@mozilla.com>

	* plugin.cc (get_view): New.
	(Plugin::load): Pass get_view to the plugin.
	(Plugin_manager::get_view): New.

> Ian

Cheers,
Rafael

[-- Attachment #2: include.patch --]
[-- Type: text/x-patch, Size: 1085 bytes --]

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 956df00..7450a9e 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -203,6 +203,10 @@ enum ld_plugin_status
 (*ld_plugin_get_input_file) (const void *handle,
                              struct ld_plugin_input_file *file);
 
+typedef
+enum ld_plugin_status
+(*ld_plugin_get_view) (const void *handle, const void **viewp);
+
 /* The linker's interface for releasing the input file.  */
 
 typedef
@@ -269,7 +273,8 @@ enum ld_plugin_tag
   LDPT_ADD_INPUT_LIBRARY,
   LDPT_OUTPUT_NAME,
   LDPT_SET_EXTRA_LIBRARY_PATH,
-  LDPT_GNU_LD_VERSION
+  LDPT_GNU_LD_VERSION,
+  LDPT_GET_VIEW
 };
 
 /* The plugin transfer vector.  */
@@ -289,6 +294,7 @@ struct ld_plugin_tv
     ld_plugin_add_input_file tv_add_input_file;
     ld_plugin_message tv_message;
     ld_plugin_get_input_file tv_get_input_file;
+    ld_plugin_get_view tv_get_view;
     ld_plugin_release_input_file tv_release_input_file;
     ld_plugin_add_input_library tv_add_input_library;
     ld_plugin_set_extra_library_path tv_set_extra_library_path;

[-- Attachment #3: gold.patch --]
[-- Type: text/x-patch, Size: 2969 bytes --]

diff --git a/gold/plugin.cc b/gold/plugin.cc
index 214eff3..7e259e0 100644
--- a/gold/plugin.cc
+++ b/gold/plugin.cc
@@ -69,6 +69,9 @@ static enum ld_plugin_status
 get_input_file(const void *handle, struct ld_plugin_input_file *file);
 
 static enum ld_plugin_status
+get_view(const void *handle, const void **viewp);
+
+static enum ld_plugin_status
 release_input_file(const void *handle);
 
 static enum ld_plugin_status
@@ -130,7 +133,7 @@ Plugin::load()
   sscanf(ver, "%d.%d", &major, &minor);
 
   // Allocate and populate a transfer vector.
-  const int tv_fixed_size = 16;
+  const int tv_fixed_size = 17;
   int tv_size = this->args_.size() + tv_fixed_size;
   ld_plugin_tv* tv = new ld_plugin_tv[tv_size];
 
@@ -189,6 +192,10 @@ Plugin::load()
   tv[i].tv_u.tv_get_input_file = get_input_file;
 
   ++i;
+  tv[i].tv_tag = LDPT_GET_VIEW;
+  tv[i].tv_u.tv_get_view = get_view;
+
+  ++i;
   tv[i].tv_tag = LDPT_RELEASE_INPUT_FILE;
   tv[i].tv_u.tv_release_input_file = release_input_file;
 
@@ -635,6 +642,35 @@ Plugin_manager::release_input_file(unsigned int handle)
   return LDPS_OK;
 }
 
+ld_plugin_status
+Plugin_manager::get_view(unsigned int handle, const void **viewp)
+{
+  off_t offset;
+  size_t filesize;
+  Input_file *input_file;
+  if (this->objects_.size() == handle)
+    {
+      // We are being called from the claim_file hook.
+      const struct ld_plugin_input_file &f = this->plugin_input_file_;
+      offset = f.offset;
+      filesize = f.filesize;
+      input_file = this->input_file_;
+    }
+  else
+    {
+      // An already claimed file.
+      Pluginobj* obj = this->object(handle);
+      if (obj == NULL)
+        return LDPS_BAD_HANDLE;
+      offset = obj->offset();
+      filesize = obj->filesize();
+      input_file = obj->input_file();
+    }
+  *viewp = (void*) input_file->file().get_view(offset, 0, filesize, false,
+                                               false);
+  return LDPS_OK;
+}
+
 // Add a new library path.
 
 ld_plugin_status
@@ -1247,6 +1283,15 @@ release_input_file(const void* handle)
   return parameters->options().plugins()->release_input_file(obj_index);
 }
 
+static enum ld_plugin_status
+get_view(const void *handle, const void **viewp)
+{
+  gold_assert(parameters->options().has_plugins());
+  unsigned int obj_index =
+      static_cast<unsigned int>(reinterpret_cast<intptr_t>(handle));
+  return parameters->options().plugins()->get_view(obj_index, viewp);
+}
+
 // Get the symbol resolution info for a plugin-claimed input file.
 
 static enum ld_plugin_status
diff --git a/gold/plugin.h b/gold/plugin.h
index c26414d..87747bf 100644
--- a/gold/plugin.h
+++ b/gold/plugin.h
@@ -245,6 +245,9 @@ class Plugin_manager
   ld_plugin_status
   get_input_file(unsigned int handle, struct ld_plugin_input_file* file);
 
+  ld_plugin_status
+  get_view(unsigned int handle, const void **viewp);
+
   // Release an input file.
   ld_plugin_status
   release_input_file(unsigned int handle);

Re: [patch][rfc] Giving plugins access to gold's file views

[Ian Lance Taylor]

Rafael Avila de Espindola <respindola@mozilla.com> writes:

> Fixed. The two patches are attached.
>
> 2010-03-22  Rafael Ávila de Espíndola <respindola@mozilla.com>
>
> 	* plugin-api.h (ld_plugin_get_view): New.
> 	(ld_plugin_tag): Add LDPT_GET_VIEW.
> 	(ld_plugin_tv): Add tv_get_view.
>
> 2010-03-22  Rafael Ávila de Espíndola <respindola@mozilla.com>
>
> 	* plugin.cc (get_view): New.
> 	(Plugin::load): Pass get_view to the plugin.
> 	(Plugin_manager::get_view): New.


> +  *viewp = (void*) input_file->file().get_view(offset, 0, filesize, false,
> +                                               false);

Use static_cast<>.

These patches are OK with that change if they're OK with Cary.

Please also update http://gcc.gnu.org/wiki/whopr/driver .

Ian

Re: [patch][rfc] Giving plugins access to gold's file views

[Cary Coutant]

> These patches are OK with that change if they're OK with Cary.
>
> Please also update http://gcc.gnu.org/wiki/whopr/driver .

They're OK with me, too.

-cary