Re: [LTO merge][12/15] Add linker plugin

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Mon, 28 Sep 2009, Diego Novillo wrote:

> This adds a new toplevel directory named lto-plugin which is a
> gold plugin so that lto1 can get information out of the linker
> and pull LTO object files out of archives.

We know the plugin and gold are not yet optimally portable to different 
hosts, so I won't note most portability issues here.  However, this patch 
is the one adding plugin-api.h and one of the open LTO bugs (40790) is 
that this unconditionally includes stdint.h, which will make GCC fail to 
build on some hosts if not fixed (this header is included in some LTO 
files in core GCC that are unconditionally built into GCC), which is a 
regression.

> diff -rdupN --exclude=.svn --exclude=.git --exclude='*.diff*' --exclude='autom4te*' --exclude=tags --exclude=ChangeLog.lto --exclude=configure /usr/local/google/homedirs/dnovillo/gcc/trunk/include/lto-symtab.h /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/include/lto-symtab.h
> --- /usr/local/google/homedirs/dnovillo/gcc/trunk/include/lto-symtab.h	1969-12-31 19:00:00.000000000 -0500
> +++ /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/include/lto-symtab.h	2009-09-23 11:34:40.000000000 -0400
> @@ -0,0 +1,41 @@
> +/* Data types used in the IL symbol table.
> +   Copyright (C) 1999, 2000, 2002, 2003, 2004 Free Software Foundation, Inc.
> +   Contributed by Rafael Espindola <espindola@google.com>

I am very sceptical of those copyright dates.

> diff -rdupN --exclude=.svn --exclude=.git --exclude='*.diff*' --exclude='autom4te*' --exclude=tags --exclude=ChangeLog.lto --exclude=configure /usr/local/google/homedirs/dnovillo/gcc/trunk/include/plugin-api.h /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/include/plugin-api.h
> --- /usr/local/google/homedirs/dnovillo/gcc/trunk/include/plugin-api.h	1969-12-31 19:00:00.000000000 -0500
> +++ /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/include/plugin-api.h	2009-09-23 11:34:41.000000000 -0400
> @@ -0,0 +1,263 @@
[...]

> +enum ld_plugin_status
> +{
> +  LDPS_OK = 0,
> +  LDPS_NO_SYMS,         /* Attempt to get symbols that haven't been added. */
> +  LDPS_ERR
> +  /* Additional Error codes TBD.  */
> +};

The version of this header in the src repository has an additional 
LDPS_BAD_HANDLE here.  You need to make sure the version you add to GCC 
trunk is up to date with all changes that have been made to this file in 
the src repository; when it appears on GCC trunk, the GCC version becomes 
the master and any changes only in src may be lost.

> diff -rdupN --exclude=.svn --exclude=.git --exclude='*.diff*' --exclude='autom4te*' --exclude=tags --exclude=ChangeLog.lto --exclude=configure /usr/local/google/homedirs/dnovillo/gcc/trunk/lto-plugin/Makefile.am /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/lto-plugin/Makefile.am
> --- /usr/local/google/homedirs/dnovillo/gcc/trunk/lto-plugin/Makefile.am	1969-12-31 19:00:00.000000000 -0500
> +++ /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/lto-plugin/Makefile.am	2009-09-23 11:34:52.000000000 -0400
> @@ -0,0 +1,26 @@
> +# Makefile.am is used by automake 1.9.6 to generate Makefile.in.

The point about using the same auto* versions now used by the rest of GCC 
has already been made.

> +AM_CPPFLAGS = -I$(top_srcdir)/../include -D_LARGEFILE_SOURCE \
> +	-D_FILE_OFFSET_BITS=64 $(LIBELFINC)

See my previous comments 
<http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01391.html> regarding using 
these flags being potentially dangerous.  In any case, AC_SYS_LARGEFILE 
should be used if you want these options instead of hardcoding the defines 
used on some particular system.

> +bin_PROGRAMS = ltosymtab

If you put a program in bindir it is a public interface to GCC.  What does 
it do?  How and when should GCC users use it?  These things need 
documenting in the GCC manual.  It needs to support --help and --version 
following the GNU Coding Standards and using ACX_PKGVERSION and 
ACX_BUGURL.  If it's not a public interface - if it's used internally by 
other programs - put it in GCC's libexecsubdir and you don't need to care 
about these issues.

> diff -rdupN --exclude=.svn --exclude=.git --exclude='*.diff*' --exclude='autom4te*' --exclude=tags --exclude=ChangeLog.lto --exclude=configure /usr/local/google/homedirs/dnovillo/gcc/trunk/lto-plugin/lto-plugin.c /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/lto-plugin/lto-plugin.c
> --- /usr/local/google/homedirs/dnovillo/gcc/trunk/lto-plugin/lto-plugin.c	1969-12-31 19:00:00.000000000 -0500
> +++ /usr/local/google/homedirs/dnovillo/gcc/trunk.lto/lto-plugin/lto-plugin.c	2009-09-23 11:34:52.000000000 -0400
> @@ -0,0 +1,674 @@
> +/* LTO plugin for gold.
> +   Copyright (C) 2008 Free Software Foundation, Inc.
> +   Contributed by Rafael Avila de Espindola (espindola@google.com).

Watch those copyright dates; 2009 should generally be included.  Likewise 
in other files.

> +You should have received a copy of the GNU General Public License
> +along with this program; if not, write to the Free Software
> +Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */

Standard GCC GPLv3 notices use a URL rather than a postal address.  
Likewise in other files.

> +static void
> +write_resolution (void)
> +{
> +  unsigned int i;
> +  FILE *f;
> +  /* FIXME: Disabled for now since we are not using the resolution file. */
> +  return;
> +
> +
> +  /* FIXME: This should be a temporary file. */
> +  f = fopen ("resolution", "w");

Fixed filenames like this are typically security holes.  The return above 
means this isn't, but it might be best to remove this function altogether 
until you are ready to add a version that actually does something.

> +/* Pass files generated by the lto-wrapper to the linker. FD is lto-wrapper's
> +   stdout. */
> +
> +static void
> +add_output_files (FILE *f)
> +{
> +  char fname[1000]; /* FIXME: Is this big enough? */

I don't know what sort of strings go there, but if they can be filenames 
with user-controlled components then the GNU Coding Standards say to avoid 
arbitrary limits.

> +      output_files = realloc (output_files, num_output_files * sizeof (char *));
> +      output_files[num_output_files - 1] = strdup (s);

Use xrealloc and xstrdup.  Other places have the same issue with realloc 
or calloc or strdup.

> +  /* Write argv to a file to avoid a command line that is too long. */
> +  t = asprintf (&at_args, "@%s/arguments", temp_obj_dir_name);
> +  assert (t >= 0);

This is an example of inappropriate use of assert for things that may be 
valid error conditions not a program bug.  It looks like there are others 
in this plugin.  Some cleanup work is needed.

-- 
Joseph S. Myers
joseph@codesourcery.com