Patchwork [Trunk] patch for GNU-friendly gengtype

login
register
mail settings
Submitter Basile Starynkevitch
Date June 16, 2010, 1:11 p.m.
Message ID <20100616131134.GA5966@hector.lesours>
Download mbox | patch
Permalink /patch/55883/
State New
Headers show

Comments

Basile Starynkevitch - June 16, 2010, 1:11 p.m.
Hello All

We need to improve gengtype, in particular so that it will (later) be
installed -eg as gcc-gengtype- and be usable in plugin mode without
requiring the entire GCC build or source tree.

See http://gcc.gnu.org/ml/gcc/2009-06/msg00416.html and more.

A first step is to make gengtype more GNU friendly, that is accept
--help & --version program arguments, and change its program
invocation conventions.

The attached patch to trunk revison 160833 seems to work (on
x86-64/Linux).

gcc/Changelog entry:
2010-06-16  Basile Starynkevitch  <basile@starynkevitch.net>

	* Makefile.in (gengtype.o): Pass version information strings as
	-DGENGTYPE_* preprocessor flags.

	* gengtype.c: include getopt.h.
	(do_dump): Added static variable moved from main.
	(inputlist): Ditto.
	(plugin_output_filename): Ditto.
	(gengtype_long_options): New variable.
	(print_usage): Added function.
	(bug_report_url, version_string, pkgversion_string): Added variable.
	(print_version): Added function.
	(parse_program_options): Added new function.
	(main): Changed program invocation convention. Call
	parse_program_options. Removed old argument parsing.

##############

I was not able to add a dependency to version.o for build/gengtype.o
(this caused circularities in make), so I pasted code from version.c
to gengtype.c

Comments are welcome.

Bootstrapped on x86-64/Linux (Ubuntu/Lucid).

Ok for trunk?

Cheers
Basile Starynkevitch - June 19, 2010, 12:33 p.m.
Hello All, & Joseph, Diego, Gerald who in the past did review some of my
gengtype patches, & Laurynas who also works on gengtype.

I am respectfully pinging the patch
http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01621.html

On Wed, 2010-06-16 at 15:11 +0200, Basile Starynkevitch wrote:
> 
> We need to improve gengtype, in particular so that it will (later) be
> installed -eg as gcc-gengtype- and be usable in plugin mode without
> requiring the entire GCC build or source tree.

This patch is needed for further gengtype enhancement.

We need the gengtype of 4.6 to be:

1. compatible with plugins, at least as 4.5's gengtype is, but better if
possible

2. remove the dirty requirement that a plugin needing gengtype (there
are some, notably MELT http://gcc.gnu.org/wiki/MELT and possibly others)
have to access to both source & build trees of the GCC for which the
plugin is compiled. This kind of dependency is unmodular, dirty, and
breaks most Linux distributions packaging systems. We really should
remove the silly requirement
http://gcc.gnu.org/onlinedocs/gccint/Plugins.html end of page: 
"Plugins needing to use gengtype require a GCC build directory for the
same version of GCC that they will be linked against." 

3. since the future 4.6 gengtype will be needed to build some 4.6 GCC
plugins, I believe that it should be installed, probably as gcc-gengtype
[with possibly the program suffix that has been ./configure-d, so it
will be gcc-gengtype-4.6 when GCC 4.6 will be installed as gcc-4.6 etc
etc.].



So my plan is


a. make gengtype more GNU friendly. This is what the proposed patch
http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01621.html provides. When
this patch is accepted, gengtype will be more easy to use (accepting
--help & --version ...) and a little less hard to extend (we will need
extra program arguments, and a GNU getopt_long framework will help).


b. extend gengtype to be able to
  * save, during GCC build time, the entire state in some textual
format. This state should be enough to re-run later gengtype in plugin
mode without requiring any GCC build or source tree, as it does in 4.5!
This state file should also be installed.

  * load & use, during plugin builds, the above saved state to be able
to generate GCC marking routines for plugin needing them (e.g. MELT)
without the silly requirement of needing both GCC build & source tree to
build a plugin. 

Notice that Luarynas already added some debug dump facilities inside
gengtype which are a good starting point for this save & restore state
feature.


c. enhance the installation process to install at some suitable place
the gengtype (probably as gcc-gengtype), the saved state from b. aboce
etc.




This is why I hope that my patch
http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01621.html suitably
improved if needed, will be accepted for 4.6. As I try to explain here,
it is the first step of a gengtype improvement.

Comments to this patch [which I won't repeat here] are welcome.

Regards.
Laurynas Biveinis - June 21, 2010, 5:55 a.m.
Hello Basile -

> gcc/Changelog entry:
> 2010-06-16  Basile Starynkevitch  <basile@starynkevitch.net>
>
>        * Makefile.in (gengtype.o): Pass version information strings as
>        -DGENGTYPE_* preprocessor flags.
>
>        * gengtype.c: include getopt.h.

Capitalize "Include".

> I was not able to add a dependency to version.o for build/gengtype.o
> (this caused circularities in make), so I pasted code from version.c
> to gengtype.c

Ugh. How is version.o currently dependent on gengtype? IMHO it
shouldn't be, can you look into breaking that dependency?

+static void
+print_usage (void)
+{
+  fprintf (stderr, "Usage: %s\n", progname);

I believe the usage information should be displayed on stdout, not
stderr (quick grep in gcc.c confirms this). Same with --version.

+/* Parse the program options using getopt_long... */
+static void
+parse_program_options (int argc, char**argv)
+{
+  int opt = -1;
+  while ((opt = getopt_long (argc, argv, "hvdP:S:I:",

I would use const static char gengtype_short_options[] = "hvdP:S:I:"
and put it right next to gengtype_long_options definition.

+  progname = (argc>0)?argv[0]:"gengtype";

Some whitespace around the operators here?

@@ -3875,7 +3884,7 @@
 build/genautomata$(build_exeext) : BUILD_LIBS += -lm

 # These programs are not linked with the MD reader.
-build/gengtype$(build_exeext) : build/gengtype-lex.o build/gengtype-parse.o
+build/gengtype$(build_exeext) : build/gengtype-lex.o build/gengtype-parse.o

Redudant patch chunk, i.e. whitespace change only?

I believe the patch with Makefile circular dependency issue resolved
is good, but I cannot approve it. Thanks for working on this.
Basile Starynkevitch - June 21, 2010, 6:04 a.m.
On Mon, 2010-06-21 at 07:55 +0200, Laurynas Biveinis wrote:
> Hello Basile -

[[thanks for the review, I will apply the correction this afternoon and
resubmit the http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01621.html
patch]]


> I believe the patch with Makefile circular dependency issue resolved
> is good, but I cannot approve it. Thanks for working on this.

Sorry, I wrote something unclear. What I want, is to have gengtype
accept a --version program argument. I first tried to link version.o
inside build/gengtype, but doing that did add a circular dependency, &
make issued a warning about that.

So I removed version.o from the objects making build/gengtype, and I had
to manually add version information strings as -DGENGTYPE_* preprocessor
flags.

The result was my patch. I am not extremely happy with the idea of
passing version information thru -DGENGTYPE_* flags, but this work and
does not introduce circular dependencies.

Cheers.
Laurynas Biveinis - June 21, 2010, 6:13 a.m.
2010/6/21 Basile Starynkevitch <basile@starynkevitch.net>:
> Sorry, I wrote something unclear. What I want, is to have gengtype
> accept a --version program argument. I first tried to link version.o
> inside build/gengtype, but doing that did add a circular dependency, &
> make issued a warning about that.

Exactly. Why is this happening? The circular dependency means that
there is a pre-existing version.o dependency on build/gengtype, so
when you add the dependency the other way around, you complete the
circle. But this pre-existing dependency is IMHO wrong, version.o
should not depend on build/gengtype, or there is something else going
on with Makefile which I don't understand.

Can you look if there is indeed version.o dependency on
build/gengtype, if yes, can you break it?
Paolo Bonzini - June 21, 2010, 9:53 a.m.
On 06/21/2010 08:13 AM, Laurynas Biveinis wrote:
> 2010/6/21 Basile Starynkevitch<basile@starynkevitch.net>:
>> Sorry, I wrote something unclear. What I want, is to have gengtype
>> accept a --version program argument. I first tried to link version.o
>> inside build/gengtype, but doing that did add a circular dependency,&
>> make issued a warning about that.
>
> Exactly. Why is this happening? The circular dependency means that
> there is a pre-existing version.o dependency on build/gengtype, so
> when you add the dependency the other way around, you complete the
> circle. But this pre-existing dependency is IMHO wrong, version.o
> should not depend on build/gengtype, or there is something else going
> on with Makefile which I don't understand.

# In order for parallel make to really start compiling the expensive
# objects from $(OBJS-common) as early as possible, build all their
# prerequisites strictly before all objects.
$(ALL_HOST_OBJS) : | $(generated_files)

In any case, using version.o would break cross-compilation.  You need to 
add a build/version.o and link _that_ with gengtype.  Rules to compile 
build/version.o using the same compiler as gengtype are implicitly used.

Paolo
Laurynas Biveinis - June 21, 2010, 1:58 p.m.
2010/6/21 Paolo Bonzini <bonzini@gnu.org>:
> In any case, using version.o would break cross-compilation.  You need to add
> a build/version.o and link _that_ with gengtype.

I absolutely have forgotten it, this indeed should be a
build/version.o and then all Makefile issues should go away.

Thanks,

Patch

Index: gcc/gengtype.c
===================================================================
--- gcc/gengtype.c	(revision 160833)
+++ gcc/gengtype.c	(working copy)
@@ -21,9 +21,11 @@ 
 #include "bconfig.h"
 #include "system.h"
 #include "gengtype.h"
+#include "getopt.h"
 #include "errors.h"	/* for fatal */
 #include "double-int.h"
 #include "hashtab.h"
+#include "version.h"    /* for version_string & pkgversion_string */
 
 /* Data types, macros, etc. used only in this file.  */
 
@@ -145,10 +147,12 @@ 
 /* The list of output files.  */
 static outf_p output_files;
 
-/* The plugin input files and their number; in that case only
-   a single file is produced.  */
+/* The plugin input files and their number; in that case only a single
+   file is produced.  Each element of plugin_files should have an all
+   zero lang_bitmap. */
 static char** plugin_files;
 static size_t nb_plugin_files;
+
 /* the generated plugin output name & file */
 static outf_p plugin_output;
 
@@ -162,6 +166,10 @@ 
 /* Length of srcdir name.  */
 static size_t srcdir_len = 0;
 
+static int do_dump = 0;
+static char* inputlist = NULL;
+static char* plugin_output_filename = NULL;
+
 static outf_p create_file (const char *, const char *);
 
 static const char * get_file_basename (const char *);
@@ -4158,54 +4166,129 @@ 
 }
 
 
+/* Option specification for getopt_long.  */
+static const struct option gengtype_long_options[] = 
+{
+  { "help",      no_argument, NULL, 'h' },
+  { "version",   no_argument, NULL, 'v' },
+  { "dump",      no_argument, NULL, 'd' },
+  { "plugin",    required_argument, NULL, 'P' },
+  { "srcdir",    required_argument, NULL, 'S' },
+  { "inputs",    required_argument, NULL, 'I' },
+  { NULL,        no_argument, NULL, 0   },
+};
+
+
+static void
+print_usage (void)
+{
+  fprintf (stderr, "Usage: %s\n", progname);
+  fprintf (stderr, "\t -h | --help  \t# Give this help.\n");
+  fprintf (stderr, "\t -v | --version  \t# Give version information.\n");
+  fprintf (stderr, "\t -d | --dump  \t# Dump state for debugging.\n");
+  fprintf (stderr, "\t -P | --plugin <output-file>  \t#  Generate for a plugin.\n");
+  fprintf (stderr, "\t -S | --srcdir <GCC-directory>  \t#  Specify the GCC source directory.\n");
+  fprintf (stderr, "\t -I | --inputs <input-list>  \t#  Specify the file with source files list.\n");
+}
+
+
+/* This is the location of the online document giving instructions for
+   reporting bugs.  If you distribute a modified version of GCC,
+   please configure with --with-bugurl pointing to a document giving
+   instructions for reporting bugs to you, not us.  (You are of course
+   welcome to forward us bugs reported to you, if you determine that
+   they are not bugs in your modifications.)  */
+const char bug_report_url[] = GENGTYPE_BUGURL;
+/* The complete version string, assembled from several pieces.
+   GENGTYPE_BASEVER, GENGTYPE_DATESTAMP, GENGTYPE_DEVPHASE, and
+   GENGTYPE_REVISION are defined by the Makefile.  */
+
+const char version_string[] = GENGTYPE_BASEVER GENGTYPE_DATESTAMP GENGTYPE_DEVPHASE GENGTYPE_REVISION;
+const char pkgversion_string[] = GENGTYPE_PKGVERSION;
+
+static void
+print_version (void)
+{
+  /* Since we cannot depend upon version.o, we have to duplicate
+     version information here. */
+  fprintf (stderr, "%s %s%s\n", progname, pkgversion_string, version_string);
+  fprintf (stderr, "Report bugs: %s\n", bug_report_url);
+}
+
+
+/* Parse the program options using getopt_long... */
+static void
+parse_program_options (int argc, char**argv)
+{
+  int opt = -1;
+  while ((opt = getopt_long (argc, argv, "hvdP:S:I:",
+			     gengtype_long_options, NULL)) >= 0)
+    {
+      switch (opt)
+	{
+	case 'h': /* --help */
+	  print_usage ();
+	  break;
+	case 'v': /* --version */
+	  print_version ();
+	  break;
+	case 'd': /* --dump */
+	  do_dump = 1;
+	  break;
+	case 'P': /* --plugin */
+	  if (optarg)
+	    plugin_output_filename = optarg;
+	  else
+	    fatal ("missing plugin output file name");
+	  break;
+	case 'S': /* --srcdir */
+	  if (optarg)
+	    srcdir = optarg;
+	  else
+	    fatal ("missing source directory");
+	  srcdir_len = strlen (srcdir);
+	  break;
+	case 'I': /* --inputs */
+	  if (optarg)
+	    inputlist = optarg;
+	  else
+	    fatal ("missing input list");
+	  break;
+	default:
+	  fprintf (stderr, "%s: unknown flag '%c'\n", progname, opt);
+	  print_usage ();
+	  fatal ("unexpected flag");
+	}
+    };
+  if (plugin_output_filename)
+    {
+      /* In plugin mode we require some input files. */
+      int i = 0;
+      if (optind >= argc)
+	fatal("no source files given in plugin mode");
+      nb_plugin_files = argc - optind;
+      for  (i = 0; i < (int) nb_plugin_files; i++)
+	{
+	  /* Place an all zero lang_bitmap before the plugin file
+	     name.  */
+	  char *name = argv[i + optind];
+	  int len = strlen(name) + 1 + sizeof (lang_bitmap);
+	  plugin_files[i] = XCNEWVEC (char, len) + sizeof (lang_bitmap);
+	  strcpy (plugin_files[i], name);
+	}
+    }
+}
+
 int
 main (int argc, char **argv)
 {
   size_t i;
   static struct fileloc pos = { this_file, 0 };
-  char* inputlist = 0;
-  int do_dump = 0;
   outf_p output_header;
-  char* plugin_output_filename = NULL;
   /* fatal uses this */
-  progname = "gengtype";
+  progname = (argc>0)?argv[0]:"gengtype";
+  parse_program_options (argc, argv);
 
-  if (argc >= 2 && !strcmp (argv[1], "-d"))
-    {
-      do_dump = 1;
-      argv = &argv[1];
-      argc--;
-    }
-
-  if (argc >= 6 && !strcmp (argv[1], "-P"))
-    {
-      plugin_output_filename = argv[2];
-      plugin_output = create_file ("GCC", plugin_output_filename);
-      srcdir = argv[3];
-      inputlist = argv[4];
-      nb_plugin_files = argc - 5;
-      plugin_files = XCNEWVEC (char *, nb_plugin_files);
-      for (i = 0; i < nb_plugin_files; i++)
-      {
-        /* Place an all zero lang_bitmap before the plugin file
-	   name.  */
-        char *name = argv[i + 5];
-        int len = strlen(name) + 1 + sizeof (lang_bitmap);
-        plugin_files[i] = XCNEWVEC (char, len) + sizeof (lang_bitmap);
-        strcpy (plugin_files[i], name);
-      }
-    }
-  else if (argc == 3)
-    {
-      srcdir = argv[1];
-      inputlist = argv[2];
-    }
-  else
-    fatal ("usage: gengtype [-d] [-P pluginout.h] srcdir input-list "
-           "[file1 file2 ... fileN]");
-
-  srcdir_len = strlen (srcdir);
-
   read_input_list (inputlist);
   if (hit_error)
     return 1;
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in	(revision 160833)
+++ gcc/Makefile.in	(working copy)
@@ -3745,7 +3745,7 @@ 
 
 s-gtype: build/gengtype$(build_exeext) $(filter-out [%], $(GTFILES)) \
 	 gtyp-input.list
-	$(RUN_GEN) build/gengtype$(build_exeext) $(srcdir) gtyp-input.list
+	$(RUN_GEN) build/gengtype$(build_exeext) -S $(srcdir) -I gtyp-input.list
 	$(STAMP) s-gtype
 
 generated_files = config.h tm.h $(TM_P_H) $(TM_H) multilib.h \
@@ -3831,8 +3831,17 @@ 
 build/gengtype-lex.o : gengtype-lex.c gengtype.h $(BCONFIG_H) $(SYSTEM_H)
 build/gengtype-parse.o : gengtype-parse.c gengtype.h $(BCONFIG_H)	\
   $(SYSTEM_H)
+
+# gengtype cannot depend of version.o, otherwise a circular make
+# dependency appears.
 build/gengtype.o : gengtype.c $(BCONFIG_H) $(SYSTEM_H) gengtype.h 	\
-  rtl.def insn-notes.def errors.h double-int.h $(HASHTAB_H)
+  rtl.def insn-notes.def errors.h double-int.h $(HASHTAB_H) \
+   $(REVISION) $(DATESTAMP) $(BASEVER) 
+	$(COMPILER) $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) \
+	-DGENGTYPE_BASEVER=$(BASEVER_s) -DGENGTYPE_DATESTAMP=$(DATESTAMP_s) \
+	-DGENGTYPE_REVISION=$(REVISION_s) \
+	-DGENGTYPE_DEVPHASE=$(DEVPHASE_s) -DGENGTYPE_PKGVERSION=$(PKGVERSION_s) \
+	-DGENGTYPE_BUGURL=$(BUGURL_s) -c $(srcdir)/gengtype.c $(OUTPUT_OPTION)
 build/genmddeps.o: genmddeps.c $(BCONFIG_H) $(SYSTEM_H) coretypes.h	\
   errors.h $(READ_MD_H)
 build/genmodes.o : genmodes.c $(BCONFIG_H) $(SYSTEM_H) errors.h		\
@@ -3875,7 +3884,7 @@ 
 build/genautomata$(build_exeext) : BUILD_LIBS += -lm
 
 # These programs are not linked with the MD reader.
-build/gengtype$(build_exeext) : build/gengtype-lex.o build/gengtype-parse.o
+build/gengtype$(build_exeext) : build/gengtype-lex.o build/gengtype-parse.o 
 
 # Generated source files for gengtype.
 gengtype-lex.c : gengtype-lex.l