gengtype improvements for plugins. patch 1/N

Hello All, Join work by Basile Starynkevitch & Jeremie Salvucci, taking into account the feedback we got previously. See http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02047.html and many other mails, notably http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02069.html [whichs list more of them] I've finally got the bug in our gengtype-state.c file. For Jeremie (who is currently in holidays for only a week) and other people curious about the bug we spent a month to track, the next field of type_p structures inside gengtype has two uses. First and mostly, it makes a big list (going into the structures variable) of all the GTY-ed struct & union-s. There is also a param_structs variable which is a smaller linked list of parametrized GTY-ed types. But the next field is also used within lang_struct-s to make a linked list of homonymous types which are specific to a given GCC front-end language. Honestly, gengtype is a very dirty code. Perhaps it might be the first piece of GCC to be recoded in C++ (I don't like much C++, but I do admit that its container templates would help a lot to make gengtype cleaner). Of course, this patch serie is not a rewriting of gengype in C++! We (Jeremie & Basile) now have a patch which is able to persist gengtype state in a textual file which can be used in plugin mode. So if our serie of patches is accepted in 4.6, plugins could use GTY without requiring (as the 4.5 kludge) availability of GCC source and build trees. I just uploaded the entire work as one big diff on http://starynkevitch.net/Basile/gengtype-r163961-07sept2010.diff - if people want to test the code, they could apply that as a big patch against trunk 163961. At least, that big patch demonstrates that a persistent gengtype is doable. In addition, if I am hit by a bus tommorrow, perhaps some bright people could do what I am starting now again: splitting it into manageable pieces. There are some dirt remaining there (in particular some #warning which I am removing right now). However, I am very scared and even a bit discouraged. The previous patches got no comments by any person able to OK them. It seems that no reviewer is interested in reviewing this work. If nobody reviews it, how is it possible to push it into 4.6? I have the very uncomfortable feeling that people find the goals of this patch interesting, but nobody understand well enough gengtype and have enough time to review it and OK it. So far, all the few nice comments I've got have been from people not authorized or not willing to review that patch and give enough feedback or an OK to commit it. The person most interested in this serie of patches is probably Laurynas Biveinis, and I thank him warmly for his feeedback & support. Unfortunately, he has no authority to OK any commit! I don't understand how can that work be pushed into 4.6 if no reviewer cares! And 4.6 stage 1 is ending soon!!! I do understand however, why no reviewer is interested: first, this patch is only really useful for plugins using GTY (and I know of no such plugin except my MELT; and perhaps many old GCC people are still against plugins.). Second, gengtype is really crappy code, and few people understand it well (otherwise, some other people than Laurynas would have answered my technical questions). And third, this patch does not improve the code produced by GCC, since it does not change it at all, since all the gengtype generated C code is not changed by this work. I will now splice again that patch in several chunks, perhaps improving slightly our code during this splitting. I will try to take into account every comment we got so far. If I did forgot a comment please tell. The intended splits are as before (except that we now add an entirely working gengtype-state.c file). I gave a mnemotechnic name to each patch in brackets: patch 1 [declprog], like http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02047.html moves many private definitions, types, functions from gengtype.c to gengtype.h & provide a GNU friendly gengtype program invocation. We have to declare publicly most internal types of gengtype since we really want the persistent machinery to be in a separate file gengtype-state.c and because we feel that gengtype.c is messy and big enough. patch 2 [verbosity], like http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02060.html, add a verbose flag -v to the gengtype program. When given (it can be given more than once to increase verbosity), it explains what gengtype is doing. This flag should be useful to any gengtype user. patch 3 [inputfile], like http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02063.html provides an input_file structure remove the horrible disgusting hack of encoding the set of languages in a bitmap in four bytes before the file path. Honestly, I cannot understand why the old gengtype had such an horrible coding, and how could it have been accepted in the first place, nearly nine or ten years ago. patch 4 [filesrules] like http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02065.html provides a better machinery to associate an output file to each input_file. So it is improving the get_output_file_with_visibility function with a rule & regular-expression based machinery. I did thought a few seconds about adding a convention in comments -for instance requiring that gimple.h has a comment /*@@ GENGTYPEOUTPUT gt-gimple.h */, and tree.h have /*@@ GENGTYPEOUTPUT gt-tree.h */ etc, and have gengtype parse such comments. However, such a patch would require me to patch, by adding just a comment, almost every file of GCC, and I am sadly pretty sure nobody will review such a work; the social rules of GCC make such an idea impractical. patch 5 [typedopt] like http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02068.html is making each option a disciminated union. This is needed to be able to persist gengtype state. patch 6 [wstate] see http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02069.html gives the gengtype-state.c which has all the persistency machinery and enable its use. patch 7 [doc] improves the documentation gty.texi Maybe another patch will send some more improvements. In particular, I want to check that mark_hook GTY is still working, and I would like to send later a small patch which adds conditional #if in the generated marking code. Maybe a convention should be firmly defined of how the gengtype program is named (perhaps gcc-gengtype is better) and where is the default state file (perhaps in the plugins directory). As a general rule, Jeremie & me Basile find gengtype to be of poor quality. We prefer much to increase its readability & maintainability, even if it runs a bit slower. We feel that making gengtype easier to debug & to enhance is much more important than its speed. Remember that gengtype is run very rarely in practice! By the way, I have the feeling that unchanged trunk rev.163961 crashes: I am building it on a AMD64 Debian testing in /usr/src/Lang/_GccTrunkObj build tree and /usr/src/Lang/gcc-trunk-bstarynk source tree. Configure options are '/usr/src/Lang/gcc-trunk-bstarynk/configure' '--enable-plugins' '--program-suffix=-plaintrunk' '--enable-maintainer-mode' '--enable-checks=tree,gc' '--disable-bootstrap' '--disable-multilib' '--enable-version-specific-runtime-libs' 'CFLAGS=-g' '--enable-languages=c,c++,lto,objc' The build fails with: /usr/src/Lang/gcc-trunk-bstarynk/libgcc/../gcc/libgcc2.c: In function '__multi3': /usr/src/Lang/gcc-trunk-bstarynk/libgcc/../gcc/libgcc2.c:558:1: internal compiler error: Segmentation fault My patch crashes at the same moment since it generates the same gt*. [ch] files! The script below is used to check that the gengtype generated files are same in trunk and with the patch. ########################### comparegengtypefiles script ############################### #! /bin/bash ## The trunk build directory gcctrunkobjdir=$1 ## The patched GCC build directory gccgengtypeobjdir=$2 if [ ! -d "$gcctrunkobjdir" ]; then echo $0: no Trunk obj dir 1st arg > /dev/stderr exit 1 fi if [ ! -d "$gccgengtypeobjdir" ]; then echo $0: no Gengtype obj dir 2nd arg > /dev/stderr exit 1 fi if [ ! -x $gcctrunkobjdir/build/gengtype ]; then echo $0: no $gcctrunkobjdir/build/gengtype > /dev/stderr exit 1 fi if [ ! -x $gccgengtypeobjdir/build/gengtype ]; then echo $0: no $gccgengtypeobjdir/build/gengtype > /dev/stderr exit 1 fi if [ ! -f $gcctrunkobjdir/gtype-desc.c ]; then echo $0: no $gcctrunkobjdir/gtype-desc.c > /dev/stderr exit 1 fi if [ ! -f $gccgengtypeobjdir/gtype-desc.c ]; then echo $0: no $gccgengtypeobjdir/gtype-desc.c > /dev/stderr exit 1 fi gtfiles=$(cd $gcctrunkobjdir; echo gt*.[ch]) noteqfiles="" samefiles='' for f in $gtfiles; do (diff -u -b $gcctrunkobjdir/$f $gccgengtypeobjdir/$f | head -21) if cmp $gcctrunkobjdir/$f $gccgengtypeobjdir/$f ; then samefiles="$samefiles $f" else noteqfiles="$noteqfiles $f" echo fi done echo $0 same files= $samefiles ################################################################################# I still hope our patches will be reviewed by someone authorized to send an OK when they will be ready enough. I also hope that 4.6 will be released with our patches (suitably improved) included! But I am very scared that no reviewer really care about that. What is the correct frequency to ping such a serie of patches? Who, amongst the reviewers, is particularily interested or motivated to review that work? If you are, please tell! Cheers.

Message ID	1283012591.3067.17.camel@glinka
State	New
Headers	show Return-Path: <gcc-patches-return-271504-incoming=patchwork.ozlabs.org@gcc.gnu.org> Subject: gengtype improvements for plugins. patch 1/N From: Basile Starynkevitch <basile@starynkevitch.net> Reply-To: basile@starynkevitch.net, jeremie.salvucci@free.fr Cc: gcc-patches@gcc.gnu.org Content-Type: multipart/mixed; boundary="=-u1hp5ye2WuyDxbVgpHTp" Date: Sat, 28 Aug 2010 18:23:11 +0200 Message-ID: <1283012591.3067.17.camel@glinka> Mime-Version: 1.0 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk Sender: gcc-patches-owner@gcc.gnu.org

Index: gcc/gengtype.c =================================================================== --- gcc/gengtype.c (revision 163612) +++ gcc/gengtype.c (working copy) @@ -20,10 +20,12 @@ #include "bconfig.h" #include "system.h" -#include "gengtype.h" #include "errors.h" /* for fatal */ +#include "getopt.h" #include "double-int.h" #include "hashtab.h" +#include "version.h" /* for version_string & pkgversion_string */ +#include "gengtype.h" /* Data types, macros, etc. used only in this file. */ @@ -39,8 +41,6 @@ enum typekind { TYPE_PARAM_STRUCT }; -typedef unsigned lang_bitmap; - /* A way to pass data through to the output end. */ struct options { @@ -120,16 +120,6 @@ struct type || (x)->kind == TYPE_STRUCT \ || (x)->kind == TYPE_LANG_STRUCT) -/* Structure representing an output file. */ -struct outf -{ - struct outf *next; - const char *name; - size_t buflength; - size_t bufused; - char *buf; -}; -typedef struct outf * outf_p; /* An output file, suitable for definitions, that can see declarations made in INPUT_FILE and is linked into every language that uses @@ -142,26 +132,37 @@ const char *get_output_file_name (const char *); static void oprintf (outf_p o, const char *S, ...) ATTRIBUTE_PRINTF_2; -/* The list of output files. */ -static outf_p output_files; +/* Rhe name of the file containing the list of input files. */ +static char* inputlist = 0; + /* The plugin input files and their number; in that case only a single file is produced. */ static char** plugin_files; static size_t nb_plugin_files; -/* the generated plugin output name & file */ + +/* The generated plugin output file & name. */ static outf_p plugin_output; +static char* plugin_output_filename; -/* The output header file that is included into pretty much every - source file. */ -static outf_p header_file; +/* Our output files and output header file. */ +outf_p output_files; +outf_p header_file; -/* Source directory. */ -static const char *srcdir; +/* Our source directory & its name. */ +const char *srcdir; +size_t srcdir_len; -/* Length of srcdir name. */ -static size_t srcdir_len = 0; +/* Variables used for reading and writing the state. Not yet + implemented feature! */ +char *read_state_filename; +char *write_state_filename; + +/* Variables to help debugging. */ +int do_dump; +int do_debug; + static outf_p create_file (const char *, const char *); static const char * get_file_basename (const char *); @@ -4209,32 +4210,140 @@ dump_everything (void) } +/* 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' }, + { "debug", no_argument, NULL, 'D' }, + { "plugin", required_argument, NULL, 'P' }, + { "srcdir", required_argument, NULL, 'S' }, + { "inputs", required_argument, NULL, 'I' }, + { "read-state", required_argument, NULL, 'r' }, + { "write-state", required_argument, NULL, 'w' }, + { NULL, no_argument, NULL, 0 }, +}; + + +static void +print_usage (void) +{ + printf ("Usage: %s\n", progname); + printf ("\t -h | --help \t# Give this help.\n"); + printf ("\t -D | --debug \t# Lots of debug output to debug gengtype itself.\n"); + printf ("\t -V | --version \t# Give version information.\n"); + printf ("\t -d | --dump \t# Dump state for debugging.\n"); + printf ("\t -P | --plugin <output-file> \t# Generate for a plugin.\n"); + printf ("\t -S | --srcdir <GCC-directory> \t# Specify the GCC source directory.\n"); + printf ("\t -I | --inputs <input-list> \t# Specify the file with source files list.\n"); + printf ("\t -w | --write-state <state-file> \t# Write a state file (for plugin use).\n"); + printf ("\t -r | --read-state <state-file> \t# Read a state file.\n"); +} + +static void +print_version (void) +{ + printf ("%s %s%s\n", progname, pkgversion_string, version_string); + printf ("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:w:r:D", + 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 'D': /* --debug */ + do_debug = 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; + case 'r': /* --read-state */ + if (optarg) + read_state_filename = optarg; + else + fatal ("missing read state file"); + dbgprintf ("read state %s\n", optarg); + break; + case 'w': /* --write-state */ + dbgprintf ("write state %s\n", optarg); + if (optarg) + write_state_filename = optarg; + else + fatal ("missing write state file"); + 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++) + { + char *name = argv[i + optind]; + 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"; - if (argc >= 2 && !strcmp (argv[1], "-d")) - { - do_dump = 1; - argv = &argv[1]; - argc--; - } + parse_program_options (argc, argv); - if (argc >= 6 && !strcmp (argv[1], "-P")) + if (read_state_filename) + fatal ("read state of %s not implemented yet", read_state_filename); + + if (write_state_filename) + fatal ("write state of %s not implemented yet", write_state_filename); + + if (plugin_output_filename) { - 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++) { @@ -4246,17 +4355,8 @@ main (int argc, char **argv) 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); - + dbgprintf ("inputlist %s", inputlist); read_input_list (inputlist); if (hit_error) return 1; Index: gcc/gengtype.h =================================================================== --- gcc/gengtype.h (revision 163612) +++ gcc/gengtype.h (working copy) @@ -20,6 +20,10 @@ along with GCC; see the file COPYING3. If not see #ifndef GCC_GENGTYPE_H #define GCC_GENGTYPE_H +/* Sets of accepted source languages like C, C++, Ada ... are + represented by a bitmap. */ +typedef unsigned lang_bitmap; + /* A file position, mostly for error messages. The FILE element may be compared using pointer equality. */ struct fileloc { @@ -37,6 +41,35 @@ typedef struct options *options_p; extern int lexer_toplevel_done; extern struct fileloc lexer_line; +/* Structure representing an output file. */ +struct outf +{ + struct outf *next; + const char *name; + size_t buflength; + size_t bufused; + char *buf; +}; +typedef struct outf * outf_p; + +/* The list of output files. */ +extern outf_p output_files; + +/* The output header file that is included into pretty much every + source file. */ +extern outf_p header_file; + +/* Source directory. */ +extern const char *srcdir; + +/* Length of srcdir name. */ +extern size_t srcdir_len; + +/* Variable used for reading and writing the state. */ +extern char *read_state_filename; +extern char *write_state_filename; + + /* Print an error message. */ extern void error_at_line (const struct fileloc *pos, const char *msg, ...) ATTRIBUTE_PRINTF_2; @@ -110,4 +143,19 @@ enum { a meaningful value to be printed. */ FIRST_TOKEN_WITH_VALUE = PARAM_IS }; -#endif + + +/* For debugging purposes of gengtype itself! */ +extern int do_dump; +extern int do_debug; + +#if ENABLE_CHECKING +#define dbgprintf(Fmt,...) do {if(do_debug) \ + fprintf(stderr, "%s:%d: " Fmt "\n", \ + lbasename(__FILE__),__LINE__, ##__VA_ARGS__);} while(0) +#else +#define dbgprintf(Fmt,...) do {/*nodebugprintf*/} while(0) +#endif /*ENABLE_CHECKING*/ + +#endif /*GCC_GENGTYPE_H*/ + Index: gcc/Makefile.in =================================================================== --- gcc/Makefile.in (revision 163612) +++ gcc/Makefile.in (working copy) @@ -3804,7 +3804,7 @@ s-gtyp-input: Makefile 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 \ @@ -3892,6 +3892,18 @@ build/gengtype-parse.o : gengtype-parse.c gengtype $(SYSTEM_H) build/gengtype.o : gengtype.c $(BCONFIG_H) $(SYSTEM_H) gengtype.h \ rtl.def insn-notes.def errors.h double-int.h $(HASHTAB_H) +## the build/version.o is needed at least for gengtype. +ifdef REVISION_c +build/version.o: version.c version.h $(REVISION) $(DATESTAMP) $(BASEVER) $(DEVPHASE) +else +build/version.o: version.c version.h $(DATESTAMP) $(BASEVER) $(DEVPHASE) +endif + $(COMPILER) $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) \ + -DBASEVER=$(BASEVER_s) -DDATESTAMP=$(DATESTAMP_s) \ + -DREVISION=$(REVISION_s) \ + -DDEVPHASE=$(DEVPHASE_s) -DPKGVERSION=$(PKGVERSION_s) \ + -DBUGURL=$(BUGURL_s) -c $(srcdir)/version.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 \ @@ -3935,7 +3947,9 @@ $(genprog:%=build/gen%$(build_exeext)): $(BUILD_ER 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 \ + build/version.o + build/genhooks$(build_exeext) : $(BUILD_ERRORS) # Generated source files for gengtype.

gengtype improvements for plugins. patch 1/N

Commit Message

Comments

Patch