| Message ID | 20100616131134.GA5966@hector.lesours |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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?
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
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,
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