| Message ID | 339c02294ee06570958bc71c772f33736a103fa9.1619537141.git.wschmidt@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Replace the Power target-specific built-in machinery | expand |
Hi! On Tue, Apr 27, 2021 at 10:32:41AM -0500, Bill Schmidt via Gcc-patches wrote: > +/* Used as a sentinel for range constraints on integer fields. No field can > + be 32 bits wide, so this is a safe sentinel value. */ > +#define MININT INT32_MIN INT32_MIN is for value of type int32_t. You use it for "int" though. There is INT_MIN just for that :-) > +/* Exit codes for the shell. */ > +enum exit_codes { > + EC_INTERR > +}; Please define this with some specified value (so append " = 1" or such), or just write "exit(1);" etc. directly. As it is, a compiler is free to do "exit(0);" for this error value! Not good. All these exit codes are externally visible, so please just make them explicit. There isn't much point in having symbolic names for it either, because users will see the raw numbers (reported by their shell), instead. Most generator programs allow to use fatal() etc., this is a much richer environment, no need to reinvent stuff? Include "errors.h" and link with error.o, and that is all you need AFAIK. > +static void > +consume_whitespace () Please say (void), empty argument lists have different meanings in different language versions, and it looks like you just forgot to fill int the type. Besides, it is just "what we do" :-) > +/* Get the next nonblank, noncomment line, returning 0 on EOF, 1 otherwise. */ > +static int > +advance_line (FILE *file) > +{ > + while (1) > + { > + /* Read ahead one line and check for EOF. */ > + if (!fgets (linebuf, sizeof(linebuf), file)) sizeof is an operator, not a function, and even if it was a function it would have a space before the opening parenthesis :-) > + return 0; So it also returns 0 on any error? This may not be a good idea. > +/* Match an integer and return its value, or MININT on failure. */ > +static int > +match_integer () > +{ > + int startpos = pos; > + if (linebuf[pos] == '-') > + safe_inc_pos (); > + > + int lastpos = pos - 1; > + while (isdigit (linebuf[lastpos + 1])) > + if (++lastpos >= LINELEN - 1) > + { > + (*diag) ("line length overrun in match_integer.\n"); > + exit (EC_INTERR); > + } > + > + if (lastpos < pos) > + return MININT; > + > + pos = lastpos + 1; > + char *buf = (char *) malloc (lastpos - startpos + 2); > + memcpy (buf, &linebuf[startpos], lastpos - startpos + 1); > + buf[lastpos - startpos + 1] = '\0'; > + > + int x; > + sscanf (buf, "%d", &x); > + return x; > +} Can't you just use strtol? > +static const char * > +match_to_right_bracket () This needs a function comment. > +{ > + int lastpos = pos - 1; > + while (linebuf[lastpos + 1] != ']') > + if (++lastpos >= LINELEN - 1) Please don't use side effects in "if" conditions. > + { > + (*diag) ("line length overrun.\n"); > + exit (EC_INTERR); > + } I don't think you shoulod check for line length overrun in any of these functions, btw? Just check where you read them in, and that is plenty? > + if (lastpos < pos) > + return 0; > + > + char *buf = (char *) malloc (lastpos - pos + 2); > + memcpy (buf, &linebuf[pos], lastpos - pos + 1); > + buf[lastpos - pos + 1] = '\0'; > + > + pos = lastpos + 1; > + return buf; > +} Are there no utility routines you can use? It would be useful to have something that all gen* can use (something less bare than what there is now...) Segher
On 5/21/21 1:51 PM, Segher Boessenkool wrote: > Hi! > > On Tue, Apr 27, 2021 at 10:32:41AM -0500, Bill Schmidt via Gcc-patches wrote: >> +/* Used as a sentinel for range constraints on integer fields. No field can >> + be 32 bits wide, so this is a safe sentinel value. */ >> +#define MININT INT32_MIN > INT32_MIN is for value of type int32_t. You use it for "int" though. > There is INT_MIN just for that :-) OK. > >> +/* Exit codes for the shell. */ >> +enum exit_codes { >> + EC_INTERR >> +}; > Please define this with some specified value (so append " = 1" or such), > or just write "exit(1);" etc. directly. As it is, a compiler is free to > do "exit(0);" for this error value! Not good. > > All these exit codes are externally visible, so please just make them > explicit. There isn't much point in having symbolic names for it > either, because users will see the raw numbers (reported by their > shell), instead. > > Most generator programs allow to use fatal() etc., this is a much richer > environment, no need to reinvent stuff? Include "errors.h" and link > with error.o, and that is all you need AFAIK. I'll dump the exit codes, which were an early idea that didn't prove useful in the end. exit (1) will suffice... However, I'd like to keep my approach to diagnostics. I wrote it this way so that I would have a centralized place to produce the file, line, and column information, which was really helpful while debugging these large input files. Putting wrappers around the errors.c functions seems at least as messy. > >> +static void >> +consume_whitespace () > Please say (void), empty argument lists have different meanings in > different language versions, and it looks like you just forgot to fill > int the type. Besides, it is just "what we do" :-) OK. > >> +/* Get the next nonblank, noncomment line, returning 0 on EOF, 1 otherwise. */ >> +static int >> +advance_line (FILE *file) >> +{ >> + while (1) >> + { >> + /* Read ahead one line and check for EOF. */ >> + if (!fgets (linebuf, sizeof(linebuf), file)) > sizeof is an operator, not a function, and even if it was a function it > would have a space before the opening parenthesis :-) OK. > >> + return 0; > So it also returns 0 on any error? This may not be a good idea. match_identifier returns 0 when the string doesn't match a legal identifier, as described in the function note. Otherwise the string is copied into a dynamic buffer and its address is returned. Not clear to me what you're objecting to here. >> +/* Match an integer and return its value, or MININT on failure. */ >> +static int >> +match_integer () >> +{ >> + int startpos = pos; >> + if (linebuf[pos] == '-') >> + safe_inc_pos (); >> + >> + int lastpos = pos - 1; >> + while (isdigit (linebuf[lastpos + 1])) >> + if (++lastpos >= LINELEN - 1) >> + { >> + (*diag) ("line length overrun in match_integer.\n"); >> + exit (EC_INTERR); >> + } >> + >> + if (lastpos < pos) >> + return MININT; >> + >> + pos = lastpos + 1; >> + char *buf = (char *) malloc (lastpos - startpos + 2); >> + memcpy (buf, &linebuf[startpos], lastpos - startpos + 1); >> + buf[lastpos - startpos + 1] = '\0'; >> + >> + int x; >> + sscanf (buf, "%d", &x); >> + return x; >> +} > Can't you just use strtol? I could, but (a) it's more overhead because of tracking the base, and (b) I then have to calculate lastpos afterwards. /shrug > >> +static const char * >> +match_to_right_bracket () > This needs a function comment. OK. > >> +{ >> + int lastpos = pos - 1; >> + while (linebuf[lastpos + 1] != ']') >> + if (++lastpos >= LINELEN - 1) > Please don't use side effects in "if" conditions. Really? Is it actually better to write while (linebuf[lastpos + a] != ']') { ++lastpos; if (lastpos >= LINELEN - 1) ... } Frankly I don't see it...and I don't see anything in the GNU or GCC coding conventions about this. I'd rather keep what I have. >> + { >> + (*diag) ("line length overrun.\n"); >> + exit (EC_INTERR); >> + } > I don't think you shoulod check for line length overrun in any of these > functions, btw? Just check where you read them in, and that is plenty? Yes -- I think if I check in advance_line for a line that doesn't end in \n, that will make a lot of those things superfluous. I've been a little reluctant to do that, since eventually I want to support escape-newline processing to avoid long input lines, but I can still work around that when I get to it. > >> + if (lastpos < pos) >> + return 0; >> + >> + char *buf = (char *) malloc (lastpos - pos + 2); >> + memcpy (buf, &linebuf[pos], lastpos - pos + 1); >> + buf[lastpos - pos + 1] = '\0'; >> + >> + pos = lastpos + 1; >> + return buf; >> +} > Are there no utility routines you can use? It would be useful to have > something that all gen* can use (something less bare than what there is > now...) I didn't find anything great as I was poking around, hence I wrote my own low level utilities. It goes back to my desire to track line/pos information for debug. Thanks for the review! Bill > > > Segher
Hi! On Fri, May 21, 2021 at 03:56:09PM -0500, Bill Schmidt wrote: > On 5/21/21 1:51 PM, Segher Boessenkool wrote: > >>+/* Exit codes for the shell. */ > >>+enum exit_codes { > >>+ EC_INTERR > >>+}; > >Please define this with some specified value (so append " = 1" or such), > >or just write "exit(1);" etc. directly. As it is, a compiler is free to > >do "exit(0);" for this error value! Not good. > > > >All these exit codes are externally visible, so please just make them > >explicit. There isn't much point in having symbolic names for it > >either, because users will see the raw numbers (reported by their > >shell), instead. > > > >Most generator programs allow to use fatal() etc., this is a much richer > >environment, no need to reinvent stuff? Include "errors.h" and link > >with error.o, and that is all you need AFAIK. > > I'll dump the exit codes, which were an early idea that didn't prove > useful in the end. exit (1) will suffice... > > However, I'd like to keep my approach to diagnostics. I wrote it this > way so that I would have a centralized place to produce the file, line, > and column information, which was really helpful while debugging these > large input files. Putting wrappers around the errors.c functions seems > at least as messy. Yes, wrappers is a no-go. But you could just have added the features you need to the generic code? Was there a technical reason not to do that? It sounds useful in many places, not just here. > >>+static int > >>+match_integer () > >>+{ > >>+ int startpos = pos; > >>+ if (linebuf[pos] == '-') > >>+ safe_inc_pos (); > >>+ > >>+ int lastpos = pos - 1; > >>+ while (isdigit (linebuf[lastpos + 1])) > >>+ if (++lastpos >= LINELEN - 1) > >>+ { > >>+ (*diag) ("line length overrun in match_integer.\n"); > >>+ exit (EC_INTERR); > >>+ } > >>+ > >>+ if (lastpos < pos) > >>+ return MININT; > >>+ > >>+ pos = lastpos + 1; > >>+ char *buf = (char *) malloc (lastpos - startpos + 2); > >>+ memcpy (buf, &linebuf[startpos], lastpos - startpos + 1); > >>+ buf[lastpos - startpos + 1] = '\0'; > >>+ > >>+ int x; > >>+ sscanf (buf, "%d", &x); > >>+ return x; > >>+} > >Can't you just use strtol? > I could, but (a) it's more overhead because of tracking the base, and How so? If you give base 0 it handles all of decimal, hexadecimal, and octal (with the usual 0x and 0 prefixes). > (b) I then have to calculate lastpos afterwards. /shrug It can stores it in where its second arg points to! === char *endp; const char *str = where_the_number_starts_or_some_whitespace_before_it; errno = 0; long n = strtol (str, &endp, 0); if (!*str || errno) whoops (...); === ... and now endp points to the first char that is not part of the number. It's one of the cases where errno is *helpful* :-) > >>+ int lastpos = pos - 1; > >>+ while (linebuf[lastpos + 1] != ']') > >>+ if (++lastpos >= LINELEN - 1) > >Please don't use side effects in "if" conditions. > > Really? Is it actually better to write > > while (linebuf[lastpos + a] != ']') > { > ++lastpos; > if (lastpos >= LINELEN - 1) > ... > } for (int lastpost = pos - 1; linebuf[lastpos + 1] != ']'; lastpos++) { if (lastpos >= LINELEN - 1) ... ... } > Frankly I don't see it...and I don't see anything in the GNU or GCC > coding conventions about this. I'd rather keep what I have. The most used side effect in conditionals is assignment. This saves a few keystrokes, and maybe a whole line in the code, but it makes the code a lot harder to comprehend. In your code it does not matter so very much, since you exit if there is an error anyway, but it makes it a lot harder to verify what the code does in all cases, and to check that that is what is wanted. > >>+ { > >>+ (*diag) ("line length overrun.\n"); > >>+ exit (EC_INTERR); > >>+ } > >I don't think you shoulod check for line length overrun in any of these > >functions, btw? Just check where you read them in, and that is plenty? > > Yes -- I think if I check in advance_line for a line that doesn't end in > \n, that will make a lot of those things superfluous. > > I've been a little reluctant to do that, since eventually I want to > support escape-newline processing to avoid long input lines, but I can > still work around that when I get to it. Aha, that explains. Yeah you should be able to check the length again when concatenating two lines, and that is all you need? Checking for errors repeatedly is so error-prone :-( Segher
On 21 May 2021 22:56:09 CEST, Bill Schmidt via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >>> + if (lastpos < pos) >>> + return 0; >>> + >>> + char *buf = (char *) malloc (lastpos - pos + 2); >>> + memcpy (buf, &linebuf[pos], lastpos - pos + 1); >>> + buf[lastpos - pos + 1] = '\0'; >>> + >>> + pos = lastpos + 1; >>> + return buf; >>> +} >> Are there no utility routines you can use? It would be useful to >have >> something that all gen* can use (something less bare than what there >is >> now...) > >I didn't find anything great as I was poking around, hence I wrote my >own low level utilities. It goes back to my desire to track line/pos >information for debug. > >Thanks for the review! You saw the unchecked usage of the malloc return value, did you? We certainly warn about that, I'd hope. thanks,
On Mon, May 24, 2021 at 12:37:30AM +0200, Bernhard Reutner-Fischer wrote: > On 21 May 2021 22:56:09 CEST, Bill Schmidt via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > >>> + char *buf = (char *) malloc (lastpos - pos + 2); > >>> + memcpy (buf, &linebuf[pos], lastpos - pos + 1); > >>> + buf[lastpos - pos + 1] = '\0'; > You saw the unchecked usage of the malloc return value, did you? Yes, and it is Good. We do not assert on things that will fail on the next statement anyway, in general. Also, this is not part of the compiler, this is a tool used to *build* the compiler, so it is fine to have less user-friendly errors anyway. > We certainly warn about that, I'd hope. Maybe I just don't see what you mean? In general, it is good that we do *not* do superfluous checks normally. There is nothing useful we could say about an out-of-memory situation. If this was in GCC itself we would get a helpful ICE as-is. Since this is in a generator file we can assume whoever debugs this knows how to fire up GDB for it, so it is fine as well. There are thousands of ways a developer can crash the generators by giving bad inputs. An out-of-memory condition is not likely at all, compared to that. Segher
On 5/21/21 6:43 PM, Segher Boessenkool wrote: > > Yes, wrappers is a no-go. But you could just have added the features > you need to the generic code? Was there a technical reason not to do > that? It sounds useful in many places, not just here. I agree it would be nice if all the gen* tools had line/column reporting. Maybe we could look at that as a follow-up? I was trying to keep the patch series as simple as possible, since it's already pretty large. > >>>> +static int >>>> +match_integer () >>>> +{ >>>> + int startpos = pos; >>>> + if (linebuf[pos] == '-') >>>> + safe_inc_pos (); >>>> + >>>> + int lastpos = pos - 1; >>>> + while (isdigit (linebuf[lastpos + 1])) >>>> + if (++lastpos >= LINELEN - 1) >>>> + { >>>> + (*diag) ("line length overrun in match_integer.\n"); >>>> + exit (EC_INTERR); >>>> + } >>>> + >>>> + if (lastpos < pos) >>>> + return MININT; >>>> + >>>> + pos = lastpos + 1; >>>> + char *buf = (char *) malloc (lastpos - startpos + 2); >>>> + memcpy (buf, &linebuf[startpos], lastpos - startpos + 1); >>>> + buf[lastpos - startpos + 1] = '\0'; >>>> + >>>> + int x; >>>> + sscanf (buf, "%d", &x); >>>> + return x; >>>> +} >>> Can't you just use strtol? >> <...my extraneous response and follow-ups deleted...> Sorry, I wrote this a long time ago and forgot what I was doing here. Yes, that would be a reasonable thing to do. Even more reasonable would be to do what I thought I remembered doing... It would be best to just save these as strings, not integers, since I'm just going to fprintf them back to a file later anyway. All of these are strings of at most two digits (with possible leading minus), so just using isdigit to find the strings is efficient. Using sscanf (or strtol) and fprintf is just silly. I'll rework to remove that. >> Frankly I don't see it...and I don't see anything in the GNU or GCC >> coding conventions about this. I'd rather keep what I have. > The most used side effect in conditionals is assignment. This saves a > few keystrokes, and maybe a whole line in the code, but it makes the > code a lot harder to comprehend. > > In your code it does not matter so very much, since you exit if there > is an error anyway, but it makes it a lot harder to verify what the code > does in all cases, and to check that that is what is wanted. ok. Not worth a big argument. ;-) > >>>> + { >>>> + (*diag) ("line length overrun.\n"); >>>> + exit (EC_INTERR); >>>> + } >>> I don't think you shoulod check for line length overrun in any of these >>> functions, btw? Just check where you read them in, and that is plenty? >> Yes -- I think if I check in advance_line for a line that doesn't end in >> \n, that will make a lot of those things superfluous. >> >> I've been a little reluctant to do that, since eventually I want to >> support escape-newline processing to avoid long input lines, but I can >> still work around that when I get to it. > Aha, that explains. Yeah you should be able to check the length again > when concatenating two lines, and that is all you need? Mostly. I'll have to make sure the line/column reporting is still sensible, which is the tricky bit. But that's for later... Thanks again for the review! Bill > > Checking for errors repeatedly is so error-prone :-( > > > Segher
diff --git a/gcc/config/rs6000/rs6000-gen-builtins.c b/gcc/config/rs6000/rs6000-gen-builtins.c index 0e8b315208b..f3e1d31c225 100644 --- a/gcc/config/rs6000/rs6000-gen-builtins.c +++ b/gcc/config/rs6000/rs6000-gen-builtins.c @@ -164,6 +164,10 @@ along with GCC; see the file COPYING3. If not see #include <assert.h> #include <unistd.h> +/* Used as a sentinel for range constraints on integer fields. No field can + be 32 bits wide, so this is a safe sentinel value. */ +#define MININT INT32_MIN + /* Input and output file descriptors and pathnames. */ static FILE *bif_file; static FILE *ovld_file; @@ -186,6 +190,11 @@ static char linebuf[LINELEN]; static int line; static int pos; +/* Exit codes for the shell. */ +enum exit_codes { + EC_INTERR +}; + /* Pointer to a diagnostic function. */ void (*diag) (const char *, ...) __attribute__ ((format (printf, 1, 2))) = NULL; @@ -210,3 +219,115 @@ ovld_diag (const char * fmt, ...) vfprintf (stderr, fmt, args); va_end (args); } + +/* Pass over unprintable characters and whitespace (other than a newline, + which terminates the scan). */ +static void +consume_whitespace () +{ + while (pos < LINELEN && isspace(linebuf[pos]) && linebuf[pos] != '\n') + pos++; + return; +} + +/* Get the next nonblank, noncomment line, returning 0 on EOF, 1 otherwise. */ +static int +advance_line (FILE *file) +{ + while (1) + { + /* Read ahead one line and check for EOF. */ + if (!fgets (linebuf, sizeof(linebuf), file)) + return 0; + line++; + pos = 0; + consume_whitespace (); + if (linebuf[pos] != '\n' && linebuf[pos] != ';') + return 1; + } +} + +static inline void +safe_inc_pos () +{ + if (pos++ >= LINELEN) + { + (*diag) ("line length overrun.\n"); + exit (EC_INTERR); + } +} + +/* Match an identifier, returning NULL on failure, else a pointer to a + buffer containing the identifier. */ +static char * +match_identifier () +{ + int lastpos = pos - 1; + while (isalnum (linebuf[lastpos + 1]) || linebuf[lastpos + 1] == '_') + if (++lastpos >= LINELEN - 1) + { + (*diag) ("line length overrun.\n"); + exit (EC_INTERR); + } + + if (lastpos < pos) + return 0; + + char *buf = (char *) malloc (lastpos - pos + 2); + memcpy (buf, &linebuf[pos], lastpos - pos + 1); + buf[lastpos - pos + 1] = '\0'; + + pos = lastpos + 1; + return buf; +} + +/* Match an integer and return its value, or MININT on failure. */ +static int +match_integer () +{ + int startpos = pos; + if (linebuf[pos] == '-') + safe_inc_pos (); + + int lastpos = pos - 1; + while (isdigit (linebuf[lastpos + 1])) + if (++lastpos >= LINELEN - 1) + { + (*diag) ("line length overrun in match_integer.\n"); + exit (EC_INTERR); + } + + if (lastpos < pos) + return MININT; + + pos = lastpos + 1; + char *buf = (char *) malloc (lastpos - startpos + 2); + memcpy (buf, &linebuf[startpos], lastpos - startpos + 1); + buf[lastpos - startpos + 1] = '\0'; + + int x; + sscanf (buf, "%d", &x); + return x; +} + +static const char * +match_to_right_bracket () +{ + int lastpos = pos - 1; + while (linebuf[lastpos + 1] != ']') + if (++lastpos >= LINELEN - 1) + { + (*diag) ("line length overrun.\n"); + exit (EC_INTERR); + } + + if (lastpos < pos) + return 0; + + char *buf = (char *) malloc (lastpos - pos + 2); + memcpy (buf, &linebuf[pos], lastpos - pos + 1); + buf[lastpos - pos + 1] = '\0'; + + pos = lastpos + 1; + return buf; +}