forcing most functions of libiberty in plugin-enabled cc1

forcing most functions of libiberty in plugin-enabled cc1

[Basile STARYNKEVITCH]

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

Hello All,

A plugin using a function in libiberty like make_temp_file fail to be dlopen-ed, because cc1 don't have it.

We did discuss in
    http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html &
    http://gcc.gnu.org/ml/gcc/2009-07/msg00157.html
the usefulness of linking all (or at least most of) libliberty.a inside cc1 for plugin usage.

As the first message http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html demonstrates, linking several times a shared 
library like libiberty is not good and may trigger subtile bugs.

So the standard answer: plugins needing libiberty should link that library themselves is not good.

The attached patch to trunk rev 154648 is a quick & dirty trick to force cc1 when plugins are enabled to link many such 
functions. It just declares an array of function pointers.

I really don't want to recode pex_execute and make_temp_file and friends in my MELT plugin. I am sure other plugins will 
want to use libliberty. And we already have it...


###### gcc/ChangeLog ####
2009-11-25  Basile Starynkevitch  <basile@starynkevitch.net>

	* plugin.c: added inclusion of libliberty.h & md5.h
	(funptr_t) Added new type local to this file.
	(plugin_libiberty_function_table_that_should_not_be_used): Added.

#########

I did check with nm that the resulting cc1 has all the functions.

Ok for trunk?

Regards.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

[-- Attachment #2: forcelibiberty-for-plugins-rev154648.diff --]
[-- Type: text/x-patch, Size: 3395 bytes --]

Index: gcc-trunk-bstarynk/gcc/plugin.c
===================================================================
--- gcc-trunk-bstarynk/gcc/plugin.c	(revision 154648)
+++ gcc-trunk-bstarynk/gcc/plugin.c	(working copy)
@@ -42,6 +42,8 @@
 
 #ifdef ENABLE_PLUGIN
 #include "plugin-version.h"
+#include "libiberty.h"
+#include "md5.h"
 #endif
 
 /* Event names as strings.  Keep in sync with enum plugin_event.  */
@@ -686,3 +688,105 @@
     return false;
   return true;
 }
+
+
+#ifdef ENABLE_PLUGIN
+/* Reference most of libiberty functions when plugins are enabled, to
+   make them available to plugins. Currently, libiberty is not a
+   shared library, and not all of it is used by cc1. So we add
+   references to most of its functions, to make them available to
+   plugins. Linking libibery inside plugins is a bad idea. See
+   http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html &
+   http://gcc.gnu.org/ml/gcc/2009-07/msg00157.html for motivations.
+*/
+typedef void (*funptr_t) ();
+
+/* We don't declare that table static, because we don't want it to be
+   removed by optimization. However, no code should use that table, it
+   is here only to force all the functions to be linked in to be
+   available to plugins! */
+
+const funptr_t 
+plugin_libiberty_function_table_that_should_not_be_used[] = {
+
+  /* libiberty.h functions. */
+  (funptr_t) unlock_stream,
+  (funptr_t) unlock_std_streams,
+  (funptr_t) fopen_unlocked,
+  (funptr_t) fdopen_unlocked,
+  (funptr_t) freopen_unlocked,
+  (funptr_t) buildargv,
+  (funptr_t) freeargv,
+  (funptr_t) dupargv,
+  (funptr_t) expandargv,
+  (funptr_t) writeargv,
+  (funptr_t) lbasename,
+  (funptr_t) lrealpath,
+  (funptr_t) concat,
+  (funptr_t) reconcat,
+  (funptr_t) concat_length,
+  (funptr_t) concat_copy,
+  (funptr_t) concat_copy2,
+  (funptr_t) fdmatch,
+  (funptr_t) ffs,
+  (funptr_t) getpwd,
+  (funptr_t) gettimeofday,
+  (funptr_t) get_run_time,
+  (funptr_t) make_relative_prefix,
+  (funptr_t) make_relative_prefix_ignore_links,
+  (funptr_t) choose_temp_base,
+  (funptr_t) make_temp_file,
+  (funptr_t) unlink_if_ordinary,
+  (funptr_t) spaces,
+  (funptr_t) errno_max,
+  (funptr_t) strerrno,
+  (funptr_t) strtoerrno,
+  (funptr_t) xstrerror,
+  (funptr_t) signo_max, 
+  (funptr_t) strsigno,
+  (funptr_t) strtosigno,
+  (funptr_t) xatexit,
+  (funptr_t) xexit,
+  (funptr_t) xmalloc_set_program_name,
+  (funptr_t) xmalloc_failed,
+  (funptr_t) xmalloc,
+  (funptr_t) xrealloc,
+  (funptr_t) xcalloc,
+  (funptr_t) xstrdup,
+  (funptr_t) xstrndup,
+  (funptr_t) xmemdup,
+  (funptr_t) physmem_total,
+  (funptr_t) physmem_available,
+  (funptr_t) xcrc32,
+  (funptr_t) hex_init,
+  (funptr_t) pex_init,
+  (funptr_t) pex_run,
+  (funptr_t) pex_run_in_environment,
+  (funptr_t) pex_input_file,
+  (funptr_t) pex_input_pipe,
+  (funptr_t) pex_read_output,
+  (funptr_t) pex_read_err,
+  (funptr_t) pex_get_status,
+  (funptr_t) pex_get_times,
+  (funptr_t) pex_free,
+  (funptr_t) pex_one,
+  (funptr_t) pexecute,
+  (funptr_t) pwait,
+  (funptr_t) asprintf,
+  (funptr_t) vasprintf,
+  (funptr_t) snprintf,
+  (funptr_t) vsnprintf,
+  (funptr_t) strverscmp,
+
+  /* md5.h functions */
+  (funptr_t) md5_init_ctx,
+  (funptr_t) md5_process_block,
+  (funptr_t) md5_process_bytes,
+  (funptr_t) md5_finish_ctx,
+  (funptr_t) md5_read_ctx,
+  (funptr_t) md5_stream,
+  (funptr_t) md5_buffer,
+
+  (funptr_t)0
+};
+#endif

Re: forcing most functions of libiberty in plugin-enabled cc1

[Richard Guenther]

On Wed, Nov 25, 2009 at 4:47 PM, Basile STARYNKEVITCH
<basile@starynkevitch.net> wrote:
> Hello All,
>
> A plugin using a function in libiberty like make_temp_file fail to be
> dlopen-ed, because cc1 don't have it.
>
> We did discuss in
>   http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html &
>   http://gcc.gnu.org/ml/gcc/2009-07/msg00157.html
> the usefulness of linking all (or at least most of) libliberty.a inside cc1
> for plugin usage.
>
> As the first message http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html
> demonstrates, linking several times a shared library like libiberty is not
> good and may trigger subtile bugs.
>
> So the standard answer: plugins needing libiberty should link that library
> themselves is not good.
>
> The attached patch to trunk rev 154648 is a quick & dirty trick to force cc1
> when plugins are enabled to link many such functions. It just declares an
> array of function pointers.
>
> I really don't want to recode pex_execute and make_temp_file and friends in
> my MELT plugin. I am sure other plugins will want to use libliberty. And we
> already have it...
>
>
> ###### gcc/ChangeLog ####
> 2009-11-25  Basile Starynkevitch  <basile@starynkevitch.net>
>
>        * plugin.c: added inclusion of libliberty.h & md5.h
>        (funptr_t) Added new type local to this file.
>        (plugin_libiberty_function_table_that_should_not_be_used): Added.
>
> #########
>
> I did check with nm that the resulting cc1 has all the functions.
>
> Ok for trunk?

This is completely broken.  Instead link libiberty.a with --whole-archive
when plugins are enabled.

Richard.

> Regards.
> --
> Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
> email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
> 8, rue de la Faiencerie, 92340 Bourg La Reine, France
> *** opinions {are only mines, sont seulement les miennes} ***
>

Re: forcing most functions of libiberty in plugin-enabled cc1

[Basile STARYNKEVITCH]

Richard Guenther wrote:
> On Wed, Nov 25, 2009 at 4:47 PM, Basile STARYNKEVITCH
> <basile@starynkevitch.net> wrote:
>> Hello All,
>>
>> A plugin using a function in libiberty like make_temp_file fail to be
>> dlopen-ed, because cc1 don't have it.
>>
>> We did discuss in
>>   http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html &
>>   http://gcc.gnu.org/ml/gcc/2009-07/msg00157.html
>> the usefulness of linking all (or at least most of) libliberty.a inside cc1
>> for plugin usage.
> 
> This is completely broken.  Instead link libiberty.a with --whole-archive
> when plugins are enabled.


That won't work on non-binutils systems (AFAIK --whole-archive is a binutils options to the linker), and the point of 
libiberty is precisely to provide a portability layer.
http://gcc.gnu.org/ml/gcc/2009-07/msg00174.html
http://gcc.gnu.org/ml/gcc/2009-07/msg00175.html

I do agree that the patch is quick & dirty. But:

  * it is portable AFAIK (since they is no language tricks)

  * it should work on any (even obscure) plateform

  * the only thing impacted is the resulting cc1 binary size. No additionnal code is executed!

I would like some comments by people familiar with non-linux systems (and systems whose linkers is not the binutils one) 
providing a dlopen. Of course, if we only targeted linux (which I would be happy with) the proposed patch is crap.

Regards.


-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: forcing most functions of libiberty in plugin-enabled cc1

[Basile STARYNKEVITCH]

Basile STARYNKEVITCH wrote:
> Richard Guenther wrote:
>> On Wed, Nov 25, 2009 at 4:47 PM, Basile STARYNKEVITCH
>> <basile@starynkevitch.net> wrote:
>>> Hello All,
>>>
>>> A plugin using a function in libiberty like make_temp_file fail to be
>>> dlopen-ed, because cc1 don't have it.
>>>
>>> We did discuss in
>>>   http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html &
>>>   http://gcc.gnu.org/ml/gcc/2009-07/msg00157.html
>>> the usefulness of linking all (or at least most of) libliberty.a 
>>> inside cc1
>>> for plugin usage.
>>
>> This is completely broken.  Instead link libiberty.a with --whole-archive
>> when plugins are enabled.
> 
> 
> That won't work on non-binutils systems (AFAIK --whole-archive is a 
> binutils options to the linker), and the point of libiberty is precisely 
> to provide a portability layer.
> http://gcc.gnu.org/ml/gcc/2009-07/msg00174.html
> http://gcc.gnu.org/ml/gcc/2009-07/msg00175.html
> 
> I do agree that the patch is quick & dirty. But:
> 
>  * it is portable AFAIK (since they is no language tricks)
> 
>  * it should work on any (even obscure) plateform
> 
>  * the only thing impacted is the resulting cc1 binary size. No 
> additionnal code is executed!
> 
> I would like some comments by people familiar with non-linux systems 
> (and systems whose linkers is not the binutils one) providing a dlopen. 
> Of course, if we only targeted linux (which I would be happy with) the 
> proposed patch is crap.


The more I am thinking about it, the more I feel the patch is serious and should be considered by reviewers.

Of course, there may be more elegant ways of solving the issue that the patch try to solve. However,

1. We could decide (in my opinion that would be a big mistake) that libiberty is not usable by plugins. If we do decide 
that, we should at least document that fact seriously in plugins.texi. My opinion is that, since libiberty is a 
portability wrapper, it should be available to plugins. Otherwise I don't see the point of having it in systemes (like 
Linux) which don't require most of it.

2. Otherwise, libiberty should be usable by plugin. My patch is indeed quite ugly, but I would believe it have the same 
virtue as libiberty: it should be highly portable (because I believe it is quite standard C), and I believe it is the 
only way of achieving easily that goal. If some has some ideas for making libiberty available to plugins, particularily 
on non-linux systems, I would be delighted.

The answer use --whole-archive is not adequate, unless the --whole-archive option is available on every system on which 
GCC wants to permit plugins. If that is the case (I really don't know), then we should *force* that option, or any other 
way of making libiberty available, into the GCC core! This probably means a Makefile.in patch at least, and probably a 
configure.ac patch also.

Regards


-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: forcing most functions of libiberty in plugin-enabled cc1

[Rainer Orth]

Basile STARYNKEVITCH <basile@starynkevitch.net> writes:

> The answer use --whole-archive is not adequate, unless the --whole-archive
> option is available on every system on which GCC wants to permit
> plugins. If that is the case (I really don't know), then we should *force*
> that option, or any other way of making libiberty available, into the GCC
> core! This probably means a Makefile.in patch at least, and probably a
> configure.ac patch also.

Why not just build PIC objects from libiberty, create (say) a
libiberty_pic.a from those (or pic/libiberty.a, whatever), and link the
plugin against that one?  Just as portable, far less ugly, and no
special code in GCC to maintain, listing every function in libiberty.
Btw., the list of functions available in libiberty varies from platform
to platform, so you would have to generate your list a libiberty build
time instead of having a statical list.

     Rainer 

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: forcing most functions of libiberty in plugin-enabled cc1

[Basile STARYNKEVITCH]

Rainer Orth wrote:
> Basile STARYNKEVITCH <basile@starynkevitch.net> writes:
> 
>> The answer use --whole-archive is not adequate, unless the --whole-archive
>> option is available on every system on which GCC wants to permit
>> plugins. If that is the case (I really don't know), then we should *force*
>> that option, or any other way of making libiberty available, into the GCC
>> core! This probably means a Makefile.in patch at least, and probably a
>> configure.ac patch also.
> 
> Why not just build PIC objects from libiberty, 

This was already explained by  Ralf Wildenhues
http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html
In short: mixing a statically linked function with a dynamically linked variant of the same function accessing static 
variables is an "oportunity for subtle bugs". And the point is that libiberty provide several such functions.

Regards.

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: forcing most functions of libiberty in plugin-enabled cc1

[H.J. Lu]

On Thu, Nov 26, 2009 at 11:53 AM, Basile STARYNKEVITCH
<basile@starynkevitch.net> wrote:
> Rainer Orth wrote:
>>
>> Basile STARYNKEVITCH <basile@starynkevitch.net> writes:
>>
>>> The answer use --whole-archive is not adequate, unless the
>>> --whole-archive
>>> option is available on every system on which GCC wants to permit
>>> plugins. If that is the case (I really don't know), then we should
>>> *force*
>>> that option, or any other way of making libiberty available, into the GCC
>>> core! This probably means a Makefile.in patch at least, and probably a
>>> configure.ac patch also.
>>
>> Why not just build PIC objects from libiberty,
>
> This was already explained by  Ralf Wildenhues
> http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html
> In short: mixing a statically linked function with a dynamically linked
> variant of the same function accessing static variables is an "oportunity
> for subtle bugs". And the point is that libiberty provide several such
> functions.
>

Why not build/install libiberty.so for plugin.



-- 
H.J.

Re: forcing most functions of libiberty in plugin-enabled cc1

[Basile STARYNKEVITCH]

H.J. Lu wrote:
> On Thu, Nov 26, 2009 at 11:53 AM, Basile STARYNKEVITCH
> <basile@starynkevitch.net> wrote:
>> Rainer Orth wrote:
>>> Basile STARYNKEVITCH <basile@starynkevitch.net> writes:
>>>
>>>> The answer use --whole-archive is not adequate, unless the
>>>> --whole-archive
>>>> option is available on every system on which GCC wants to permit
>>>> plugins. 
> 
> Why not build/install libiberty.so for plugin.

I am not sure to understand your proposal (which seems ambigous).

The good way would be, when GCC has plugin enabled, to install libiberty.so and to have *cc1 linked* to this shared 
library. Unfortunately I believe this solution has been considered and not accepted (because it would add yet another 
dependency to executing cc1).

What won't work is to install libiberty.so but still have cc1 linking the static libiberty.a, as explained in
http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html

So what did you (H.J.Lu) meant by "install libiberty for the plugin"?

It really should be installed by the GCC installation!

Regards
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: forcing most functions of libiberty in plugin-enabled cc1

[H.J. Lu]

On Thu, Nov 26, 2009 at 10:40 PM, Basile STARYNKEVITCH
<basile@starynkevitch.net> wrote:
> H.J. Lu wrote:
>>
>> On Thu, Nov 26, 2009 at 11:53 AM, Basile STARYNKEVITCH
>> <basile@starynkevitch.net> wrote:
>>>
>>> Rainer Orth wrote:
>>>>
>>>> Basile STARYNKEVITCH <basile@starynkevitch.net> writes:
>>>>
>>>>> The answer use --whole-archive is not adequate, unless the
>>>>> --whole-archive
>>>>> option is available on every system on which GCC wants to permit
>>>>> plugins.
>>
>> Why not build/install libiberty.so for plugin.
>
> I am not sure to understand your proposal (which seems ambigous).
>
> The good way would be, when GCC has plugin enabled, to install libiberty.so
> and to have *cc1 linked* to this shared library. Unfortunately I believe
> this solution has been considered and not accepted (because it would add yet
> another dependency to executing cc1).
>
> What won't work is to install libiberty.so but still have cc1 linking the
> static libiberty.a, as explained in
> http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html
>
> So what did you (H.J.Lu) meant by "install libiberty for the plugin"?
>

You can install libiberty.so without linking it against cc1 while your
plugin can link against libiberty.so if needed.


-- 
H.J.

Re: forcing most functions of libiberty in plugin-enabled cc1

[Ralf Wildenhues]

* Basile STARYNKEVITCH wrote on Thu, Nov 26, 2009 at 08:18:18PM CET:
> The answer use --whole-archive is not adequate, unless the
> --whole-archive option is available on every system on which GCC
> wants to permit plugins.

Some linkers have a different way to spell it (e.g., -zallextract).
With linkers that don't have it, one could enumerate all objects
libiberty.a is comprised of.  libtool does this when it creates a shared
library that links against a convenience archive.

(Note I'm not proposing to use libtool for linking cc1, but the
algorithm could be copied from there.)

Other possibilities would be to either make libiberty a proper shared
library, including a stable API (which was already rejected), or stick
to making libiberty clean for multiple instances of it inside a single
executable.  (This is a restriction for code inside libiberty.)

Cheers,
Ralf

Re: forcing most functions of libiberty in plugin-enabled cc1

[Ralf Wildenhues]

* H.J. Lu wrote on Fri, Nov 27, 2009 at 07:49:32AM CET:
> 
> You can install libiberty.so without linking it against cc1 while your
> plugin can link against libiberty.so if needed.

That would still leave you with more than one instance of static data in
libiberty, which is only ok if you accept to keep libiberty safe for it.
(IOW, at least it would need some big documentation warning.)

Cheers,
Ralf

Re: forcing most functions of libiberty in plugin-enabled cc1

[Basile STARYNKEVITCH]

H.J. Lu wrote:
> On Thu, Nov 26, 2009 
>> What won't work is to install libiberty.so but still have cc1 linking the
>> static libiberty.a, as explained in
>> http://gcc.gnu.org/ml/gcc/2009-07/msg00166.html
>>
>> So what did you (H.J.Lu) meant by "install libiberty for the plugin"?
>>
> 
> You can install libiberty.so without linking it against cc1 while your
> plugin can link against libiberty.so if needed.

It won't work, in particular for libiberty functions having static data, which are already linked inside cc1.
Take for example make_temp_file in libiberty/make-temp-file.c. This is a function I would like to access from plugins. 
In MELT plugin, I had to code a function for temporary files without it.

make_temp_file uses a static variable memoized_tmpdir (line 91 of libiberty/make-temp-file.c).

If it where linked both in cc1 statically and in libiberty.so from a plugin, we would have two instances of the
make_temp_file function, each using their own memoized_tmpdir.

Likewise, a random number generator (like the one in libiberty/random.c) would behave wrongly if linked statically from 
cc1 and also dynamically from a plugin: the random numbers used by cc1 will be correlated to the ones used by the plugin.

We *really* need most libiberty functions linked inside cc1. For MELT, I am particularily concerned with xstrndup (which 
does not appear in cc1 the last time I checked in ocotber 2009), pex_init, pex_run, pex_get_status and friends (I had to 
use system(3) in melt.so, and that means doing all the dirty work of escaping spaces & other charaters in program 
arguments), and make_temp_file.

My point is that we really should settle this issue before 4.5 release. Should I open a PR?

And besides, I believe that for newbies coding plugins, we have an incoherent talk: we are saying: hey, we have a 
libiberty to solve your portability issues, but you should not use it in your plugins. They will rightly langth at us.

libierty should be fully usable from plugins. And that means it should either be entirely linked into cc1 (as my dirty 
patch proposed), or that cc1 should link dynamically, when plugins are enabled, a libiberty.so.

Regards.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: forcing most functions of libiberty in plugin-enabled cc1

[Richard Guenther]

On Fri, Nov 27, 2009 at 7:54 AM, Ralf Wildenhues <Ralf.Wildenhues@gmx.de> wrote:
> * H.J. Lu wrote on Fri, Nov 27, 2009 at 07:49:32AM CET:
>>
>> You can install libiberty.so without linking it against cc1 while your
>> plugin can link against libiberty.so if needed.
>
> That would still leave you with more than one instance of static data in
> libiberty, which is only ok if you accept to keep libiberty safe for it.
> (IOW, at least it would need some big documentation warning.)

Btw you should be able to link libiberty statically into both cc1
and plugins.  You just need to make sure to not export its symbols
from cc1 (may be as easy/complicated/portable as linking
the whole archive).

Note that I do not think Basiles patch is acceptable - it's a gross
hack that involves an API.

Richard.