| Message ID | c36670ee86bc781cd1ad57a111bba19d0790a8bb.1623941441.git.wschmidt@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Replace the Power target-specific builtin machinery | expand |
Hi! On Thu, Jun 17, 2021 at 10:18:54AM -0500, Bill Schmidt wrote: > * config/rs6000/rs6000-gen-builtins.c (rbtree.h): New #include. > (num_bifs): New variable. > (num_ovld_stanzas): Likewise. > (num_ovlds): Likewise. > (parse_codes): New enum. > (bif_rbt): New variable. > (ovld_rbt): Likewise. > (fntype_rbt): Likewise. > (bifo_rbt): Likewise. > (parse_bif): New stub function. > (create_bif_order): Likewise. > (parse_ovld): Likewise. > (write_header_file): Likewise. > (write_init_file): Likewise. > (write_defines_file): Likewise. > (delete_output_files): New function. > (main): Likewise. > +/* Parse the built-in file. */ > +static parse_codes > +parse_bif (void) > +{ > + return PC_OK; > +} Baby steps :-) > +/* Write everything to the header file (rs6000-builtins.h). */ > +static int > +write_header_file (void) > +{ > + return 1; > +} What does the return value mean? Please document it in a comment. Same for other functions (where the function name does not make it obvious what the return value is). > +static void > +delete_output_files (void) > +{ > + /* Depending on whence we're called, some of these may already be > + closed. Don't check for errors. */ > + fclose (header_file); > + fclose (init_file); > + fclose (defines_file); > + > + unlink (header_path); > + unlink (init_path); > + unlink (defines_path); > +} What are header_path etc.? It is a very good idea to make sure this is never something terrible to call unlink on (including making sure the delete_output_files function is *obviously* never called if creating the files didn't succeed). > +/* Main program to convert flat files into built-in initialization code. */ > +int > +main (int argc, const char **argv) > +{ > + if (argc != 6) > + { > + fprintf (stderr, > + "Five arguments required: two input file and three output " > + "files.\n"); Two input file_s_ :-) (Or s/file //). > + pgm_path = argv[0]; This isn't portable (depending on what you use it for -- argv[0] is not necessarily a path at all). > + bif_file = fopen (bif_path, "r"); > + if (!bif_file) > + { > + fprintf (stderr, "Cannot find input built-in file '%s'.\n", bif_path); > + exit (1); > + } Say s/find/open/ in the error? > + fprintf (stderr, "Cannot find input overload file '%s'.\n", ovld_path); (more) Okay with those trivialities, and the unlink stuff looked at. Thanks! Segher
Hi Segher, On 7/19/21 2:15 PM, Segher Boessenkool wrote: > Hi! > > On Thu, Jun 17, 2021 at 10:18:54AM -0500, Bill Schmidt wrote: >> * config/rs6000/rs6000-gen-builtins.c (rbtree.h): New #include. >> (num_bifs): New variable. >> (num_ovld_stanzas): Likewise. >> (num_ovlds): Likewise. >> (parse_codes): New enum. >> (bif_rbt): New variable. >> (ovld_rbt): Likewise. >> (fntype_rbt): Likewise. >> (bifo_rbt): Likewise. >> (parse_bif): New stub function. >> (create_bif_order): Likewise. >> (parse_ovld): Likewise. >> (write_header_file): Likewise. >> (write_init_file): Likewise. >> (write_defines_file): Likewise. >> (delete_output_files): New function. >> (main): Likewise. >> +/* Parse the built-in file. */ >> +static parse_codes >> +parse_bif (void) >> +{ >> + return PC_OK; >> +} > Baby steps :-) > >> +/* Write everything to the header file (rs6000-builtins.h). */ >> +static int >> +write_header_file (void) >> +{ >> + return 1; >> +} > What does the return value mean? Please document it in a comment. Same > for other functions (where the function name does not make it obvious > what the return value is). > >> +static void >> +delete_output_files (void) >> +{ >> + /* Depending on whence we're called, some of these may already be >> + closed. Don't check for errors. */ >> + fclose (header_file); >> + fclose (init_file); >> + fclose (defines_file); >> + >> + unlink (header_path); >> + unlink (init_path); >> + unlink (defines_path); >> +} > What are header_path etc.? It is a very good idea to make sure this is > never something terrible to call unlink on (including making sure the > delete_output_files function is *obviously* never called if creating the > files didn't succeed). See the main function. All three files are guaranteed to have been opened for writing when this is called, but some of them may have already been closed. So the fclose calls may fail to do anything, but the unlinks will always delete the output files. This is done to avoid leaving garbage lying around after a parsing failure. > >> +/* Main program to convert flat files into built-in initialization code. */ >> +int >> +main (int argc, const char **argv) >> +{ >> + if (argc != 6) >> + { >> + fprintf (stderr, >> + "Five arguments required: two input file and three output " >> + "files.\n"); > Two input file_s_ :-) (Or s/file //). > >> + pgm_path = argv[0]; > This isn't portable (depending on what you use it for -- argv[0] is not > necessarily a path at all). The only thing it's used for is as a documentation string in the output files, indicating the path to the program that built them. So long as argv[0] is a NULL-terminated string, which it had better be, this is harmless. ISO C11: "If the value of|argc|is greater than zero, the string pointed to by|argv[0]|represents the program name;|argv[0][0]|shall be the null character if the program name is not available from the host environment." So I think we're good here. > >> + bif_file = fopen (bif_path, "r"); >> + if (!bif_file) >> + { >> + fprintf (stderr, "Cannot find input built-in file '%s'.\n", bif_path); >> + exit (1); >> + } > Say s/find/open/ in the error? > >> + fprintf (stderr, "Cannot find input overload file '%s'.\n", ovld_path); > (more) > > Okay with those trivialities, and the unlink stuff looked at. Thanks! I'll get this cleaned up and post what I commit. Thanks! Bill > > > Segher
Hi! On Tue, Jul 20, 2021 at 05:19:54PM -0500, Bill Schmidt wrote: > See the main function. All three files are guaranteed to have been > opened for writing when this is called, but some of them may have > already been closed. So the fclose calls may fail to do anything, but > the unlinks will always delete the output files. This is done to avoid > leaving garbage lying around after a parsing failure. That is much worse actually! From the C spec: The value of a pointer to a FILE object is indeterminate after the associated file is closed so this is undefined behaviour. Please fix that? Just assign 0 after closing, and guard the fclose on error with that? > >>+ pgm_path = argv[0]; > >This isn't portable (depending on what you use it for -- argv[0] is not > >necessarily a path at all). > > The only thing it's used for is as a documentation string in the output > files, indicating the path to the program that built them. So long as > argv[0] is a NULL-terminated string, which it had better be, this is > harmless. It is allowed to be a null pointer as well. (gfortran does not work on systems that do that, so I wouldn't worry to much about it, but still). > ISO C11: "If the value of|argc|is greater than zero, the string pointed > to by|argv[0]|represents the program name;|argv[0][0]|shall be the null > character if the program name is not available from the host environment." > > So I think we're good here. Yup, we'll survive, argc > 0 pretty much everywhere (but technically it isn't even required by POSIX afaics). Thanks, Segher
On 7/20/21 6:22 PM, Segher Boessenkool wrote: > Hi! > > On Tue, Jul 20, 2021 at 05:19:54PM -0500, Bill Schmidt wrote: >> See the main function. All three files are guaranteed to have been >> opened for writing when this is called, but some of them may have >> already been closed. So the fclose calls may fail to do anything, but >> the unlinks will always delete the output files. This is done to avoid >> leaving garbage lying around after a parsing failure. > That is much worse actually! From the C spec: > The value of a pointer to a FILE object is indeterminate after the > associated file is closed > so this is undefined behaviour. > > Please fix that? Just assign 0 after closing, and guard the fclose on > error with that? No, you're misunderstanding. unlink doesn't use a pointer to a FILE object. It takes a string representing the path and deletes that name from the filesystem. If nobody has the file open, the file is then deleted. In this case the files are all always closed before unlink is called. The names are removed from the filesystem, and the files are deleted. If somehow the file managed to remain open (really impossible), the file would not be deleted, but the name would be. No undefined behavior. Thanks, Bill >>>> + pgm_path = argv[0]; >>> This isn't portable (depending on what you use it for -- argv[0] is not >>> necessarily a path at all). >> The only thing it's used for is as a documentation string in the output >> files, indicating the path to the program that built them. So long as >> argv[0] is a NULL-terminated string, which it had better be, this is >> harmless. > It is allowed to be a null pointer as well. (gfortran does not work on > systems that do that, so I wouldn't worry to much about it, but still). > >> ISO C11: "If the value of|argc|is greater than zero, the string pointed >> to by|argv[0]|represents the program name;|argv[0][0]|shall be the null >> character if the program name is not available from the host environment." >> >> So I think we're good here. > Yup, we'll survive, argc > 0 pretty much everywhere (but technically it > isn't even required by POSIX afaics). > > Thanks, > > > Segher
On Tue, Jul 20, 2021 at 08:51:58PM -0500, Bill Schmidt wrote: > On 7/20/21 6:22 PM, Segher Boessenkool wrote: > >On Tue, Jul 20, 2021 at 05:19:54PM -0500, Bill Schmidt wrote: > >>See the main function. All three files are guaranteed to have been > >>opened for writing when this is called, but some of them may have > >>already been closed. So the fclose calls may fail to do anything, but > >>the unlinks will always delete the output files. This is done to avoid > >>leaving garbage lying around after a parsing failure. > >That is much worse actually! From the C spec: > > The value of a pointer to a FILE object is indeterminate after the > > associated file is closed > >so this is undefined behaviour. > > > >Please fix that? Just assign 0 after closing, and guard the fclose on > >error with that? > > No, you're misunderstanding. > > unlink doesn't use a pointer to a FILE object. It takes a string > representing the path and deletes that name from the filesystem. If > nobody has the file open, the file is then deleted. Ah, "the fclose calls may fail to do anything" confused me. That should never happen (it can get an error, maybe you meant that?) > In this case the files are all always closed before unlink is called. > The names are removed from the filesystem, and the files are deleted. > If somehow the file managed to remain open (really impossible), the file > would not be deleted, but the name would be. No undefined behavior. Calling fclose on the same FILE * twice is UB. You said you do that, but that is probably not true? Segher
On 7/21/21 10:43 AM, Segher Boessenkool wrote: > On Tue, Jul 20, 2021 at 08:51:58PM -0500, Bill Schmidt wrote: >> On 7/20/21 6:22 PM, Segher Boessenkool wrote: >>> On Tue, Jul 20, 2021 at 05:19:54PM -0500, Bill Schmidt wrote: >>>> See the main function. All three files are guaranteed to have been >>>> opened for writing when this is called, but some of them may have >>>> already been closed. So the fclose calls may fail to do anything, but >>>> the unlinks will always delete the output files. This is done to avoid >>>> leaving garbage lying around after a parsing failure. >>> That is much worse actually! From the C spec: >>> The value of a pointer to a FILE object is indeterminate after the >>> associated file is closed >>> so this is undefined behaviour. >>> >>> Please fix that? Just assign 0 after closing, and guard the fclose on >>> error with that? >> No, you're misunderstanding. >> >> unlink doesn't use a pointer to a FILE object. It takes a string >> representing the path and deletes that name from the filesystem. If >> nobody has the file open, the file is then deleted. > Ah, "the fclose calls may fail to do anything" confused me. That should > never happen (it can get an error, maybe you meant that?) > >> In this case the files are all always closed before unlink is called. >> The names are removed from the filesystem, and the files are deleted. >> If somehow the file managed to remain open (really impossible), the file >> would not be deleted, but the name would be. No undefined behavior. > Calling fclose on the same FILE * twice is UB. You said you do that, > but that is probably not true? That is unfortunately true. I guess I'll have to track which files have been closed, or otherwise make this cleaner. I had misremembered that duplicate fclose was ignored. :/ Bill > > > Segher
On 7/21/21 11:08 AM, Bill Schmidt wrote: > On 7/21/21 10:43 AM, Segher Boessenkool wrote: >> On Tue, Jul 20, 2021 at 08:51:58PM -0500, Bill Schmidt wrote: >>> On 7/20/21 6:22 PM, Segher Boessenkool wrote: >>>> On Tue, Jul 20, 2021 at 05:19:54PM -0500, Bill Schmidt wrote: >>>>> See the main function. All three files are guaranteed to have been >>>>> opened for writing when this is called, but some of them may have >>>>> already been closed. So the fclose calls may fail to do anything, but >>>>> the unlinks will always delete the output files. This is done to avoid >>>>> leaving garbage lying around after a parsing failure. >>>> That is much worse actually! From the C spec: >>>> The value of a pointer to a FILE object is indeterminate after the >>>> associated file is closed >>>> so this is undefined behaviour. >>>> >>>> Please fix that? Just assign 0 after closing, and guard the fclose on >>>> error with that? >>> No, you're misunderstanding. >>> >>> unlink doesn't use a pointer to a FILE object. It takes a string >>> representing the path and deletes that name from the filesystem. If >>> nobody has the file open, the file is then deleted. >> Ah, "the fclose calls may fail to do anything" confused me. That should >> never happen (it can get an error, maybe you meant that?) >> >>> In this case the files are all always closed before unlink is called. >>> The names are removed from the filesystem, and the files are deleted. >>> If somehow the file managed to remain open (really impossible), the file >>> would not be deleted, but the name would be. No undefined behavior. >> Calling fclose on the same FILE * twice is UB. You said you do that, >> but that is probably not true? > That is unfortunately true. I guess I'll have to track which files have > been closed, or otherwise make this cleaner. I had misremembered that > duplicate fclose was ignored. :/ I'll just move all the fclose calls to the end and avoid the problem. Bill > > Bill > >> >> Segher
diff --git a/gcc/config/rs6000/rs6000-gen-builtins.c b/gcc/config/rs6000/rs6000-gen-builtins.c index b964dc2298f..69345b9de70 100644 --- a/gcc/config/rs6000/rs6000-gen-builtins.c +++ b/gcc/config/rs6000/rs6000-gen-builtins.c @@ -163,6 +163,7 @@ along with GCC; see the file COPYING3. If not see #include <string.h> #include <assert.h> #include <unistd.h> +#include "rbtree.h" /* Input and output file descriptors and pathnames. */ static FILE *bif_file; @@ -249,6 +250,29 @@ struct typeinfo char *val2; }; +static int num_bifs; +static int num_ovld_stanzas; +static int num_ovlds; + +/* Return codes for parsing routines. */ +enum parse_codes +{ + PC_OK, + PC_EOFILE, + PC_EOSTANZA, + PC_PARSEFAIL +}; + +/* The red-black trees for built-in function identifiers, built-in + overload identifiers, and function type descriptors. */ +static rbt_strings bif_rbt; +static rbt_strings ovld_rbt; +static rbt_strings fntype_rbt; + +/* Another red-black tree containing a mapping from built-in function + identifiers to the order in which they were encountered. */ +static rbt_strings bifo_rbt; + /* Pointer to a diagnostic function. */ static void (*diag) (const char *, ...) __attribute__ ((format (printf, 1, 2))); @@ -865,3 +889,190 @@ match_type (typeinfo *typedata, int voidok) return 1; } + +/* Parse the built-in file. */ +static parse_codes +parse_bif (void) +{ + return PC_OK; +} + +/* Create a mapping from function IDs in their final order to the order + they appear in the built-in function file. */ +static void +create_bif_order (void) +{ +} + +/* Parse the overload file. */ +static parse_codes +parse_ovld (void) +{ + return PC_OK; +} + +/* Write everything to the header file (rs6000-builtins.h). */ +static int +write_header_file (void) +{ + return 1; +} + +/* Write everything to the initialization file (rs6000-builtins.c). */ +static int +write_init_file (void) +{ + return 1; +} + +/* Write everything to the include file (rs6000-vecdefines.h). */ +static int +write_defines_file (void) +{ + return 1; +} + +/* Close and delete output files after any failure, so that subsequent + build dependencies will fail. */ +static void +delete_output_files (void) +{ + /* Depending on whence we're called, some of these may already be + closed. Don't check for errors. */ + fclose (header_file); + fclose (init_file); + fclose (defines_file); + + unlink (header_path); + unlink (init_path); + unlink (defines_path); +} + +/* Main program to convert flat files into built-in initialization code. */ +int +main (int argc, const char **argv) +{ + if (argc != 6) + { + fprintf (stderr, + "Five arguments required: two input file and three output " + "files.\n"); + exit (1); + } + + pgm_path = argv[0]; + bif_path = argv[1]; + ovld_path = argv[2]; + header_path = argv[3]; + init_path = argv[4]; + defines_path = argv[5]; + + bif_file = fopen (bif_path, "r"); + if (!bif_file) + { + fprintf (stderr, "Cannot find input built-in file '%s'.\n", bif_path); + exit (1); + } + ovld_file = fopen (ovld_path, "r"); + if (!ovld_file) + { + fprintf (stderr, "Cannot find input overload file '%s'.\n", ovld_path); + exit (1); + } + header_file = fopen (header_path, "w"); + if (!header_file) + { + fprintf (stderr, "Cannot open header file '%s' for output.\n", + header_path); + exit (1); + } + init_file = fopen (init_path, "w"); + if (!init_file) + { + fprintf (stderr, "Cannot open init file '%s' for output.\n", init_path); + exit (1); + } + defines_file = fopen (defines_path, "w"); + if (!defines_file) + { + fprintf (stderr, "Cannot open defines file '%s' for output.\n", + defines_path); + exit (1); + } + + /* Initialize the balanced trees containing built-in function ids, + overload function ids, and function type declaration ids. */ + rbt_new (&bif_rbt); + rbt_new (&ovld_rbt); + rbt_new (&fntype_rbt); + + /* Initialize another balanced tree that contains a map from built-in + function ids to the order in which they were encountered. */ + rbt_new (&bifo_rbt); + + /* Parse the built-in function file. */ + num_bifs = 0; + line = 0; + if (parse_bif () == PC_PARSEFAIL) + { + fprintf (stderr, "Parsing of '%s' failed, aborting.\n", bif_path); + delete_output_files (); + exit (1); + } + fclose (bif_file); + + /* Create a mapping from function IDs in their final order to + the order they appear in the built-in function file. */ + create_bif_order (); + +#ifdef DEBUG + fprintf (stderr, "\nFunction ID list:\n"); + rbt_dump (&bif_rbt, bif_rbt.rbt_root); + fprintf (stderr, "\n"); +#endif + + /* Parse the overload file. */ + num_ovld_stanzas = 0; + num_ovlds = 0; + line = 0; + if (parse_ovld () == PC_PARSEFAIL) + { + fprintf (stderr, "Parsing of '%s' failed, aborting.\n", ovld_path); + delete_output_files (); + exit (1); + } + fclose (ovld_file); + +#ifdef DEBUG + fprintf (stderr, "\nFunction type decl list:\n"); + rbt_dump (&fntype_rbt, fntype_rbt.rbt_root); + fprintf (stderr, "\n"); +#endif + + /* Write the header file and the file containing initialization code. */ + if (!write_header_file ()) + { + fprintf (stderr, "Output to '%s' failed, aborting.\n", header_path); + delete_output_files (); + exit (1); + } + fclose (header_file); + if (!write_init_file ()) + { + fprintf (stderr, "Output to '%s' failed, aborting.\n", init_path); + delete_output_files (); + exit (1); + } + fclose (init_file); + + /* Write the defines file to be included into altivec.h. */ + if (!write_defines_file ()) + { + fprintf (stderr, "Output to '%s' failed, aborting.\n", defines_path); + delete_output_files (); + exit (1); + } + fclose (defines_file); + + return 0; +}