Re: [PATCH] libcpp: Add -Winvalid-utf8 warning [PR106655]
On 8/29/22 17:35, Jakub Jelinek wrote: On Mon, Aug 29, 2022 at 05:15:26PM -0400, Jason Merrill wrote: On 8/29/22 04:15, Jakub Jelinek wrote: Hi! The following patch introduces a new warning - -Winvalid-utf8 similarly to what clang now has - to diagnose invalid UTF-8 byte sequences in comments. In identifiers and in string literals it should be diagnosed already but comment content hasn't been really verified. I'm not sure if this is enough to say P2295R6 is implemented or not. The problem is that in the most common case, people don't use -finput-charset= option and the sources often are UTF-8, but sometimes could be some ASCII compatible single byte encoding where non-ASCII characters only appear in comments. So having the warning off by default is IMO desirable. Now, if people use explicit -finput-charset=UTF-8, perhaps we could make the warning on by default for C++23 and use pedwarn instead of warning, because then the user told us explicitly that the source is UTF-8. From the paper I understood one of the implementation options is to claim that the implementation supports 2 encodings, UTF-8 and UTF-8 like encodings where invalid UTF-8 characters in comments are replaced say by spaces, where the latter could be the default and the former only used if -finput-charset=UTF-8 -Werror=invalid-utf8 options are used. Thoughts on this? That sounds good to me. The pedwarn on -std=c++23 -finput-charset=UTF-8 or just documenting that "conforming" UTF-8 is only -finput-charset=UTF-8 -Werror=invalid-utf8 ? The former. +static const uchar * +_cpp_warn_invalid_utf8 (cpp_reader *pfile) +{ + cpp_buffer *buffer = pfile->buffer; + const uchar *cur = buffer->cur; + + if (cur[0] < utf8_signifier + || cur[1] < utf8_continuation || cur[1] >= utf8_signifier) +{ + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, +pfile->line_table->highest_line, +CPP_BUF_COL (buffer), +"invalid UTF-8 character <%x> in comment", +cur[0]); + return cur + 1; +} + else if (cur[2] < utf8_continuation || cur[2] >= utf8_signifier) Unicode table 3-7 says that the second byte is sometimes restricted to less than this range. That is true and I've tried to include tests for all of those cases in the testcase and all of them get a warning. Some of them are through: /* Make sure the shortest possible encoding was used. */ if (c <= 0x7F && nbytes > 1) return EILSEQ; if (c <= 0x7FF && nbytes > 2) return EILSEQ; if (c <=0x && nbytes > 3) return EILSEQ; if (c <= 0x1F && nbytes > 4) return EILSEQ; if (c <= 0x3FF && nbytes > 5) return EILSEQ; and others are through: /* Make sure the character is valid. */ if (c > 0x7FFF || (c >= 0xD800 && c <= 0xDFFF)) return EILSEQ; All I had to do outside of what one_utf8_to_cppchar already implements was: + if (_cpp_valid_utf8 (pfile, &pstr, buffer->rlimit, 0, NULL, &s) + && s <= 0x0010) the <= 0x0010 check, because one_utf8_to_cppchar as written happily supports up to 6 bytes long UTF-8 which can encode up to 7FFF: -007F 0xxx 0080-07FF 110x 10xx 0800- 1110 10xx 10xx 0001-001F 0xxx 10xx 10xx 10xx 0020-03FF 10xx 10xx 10xx 10xx 10xx 0400-7FFF 110x 10xx 10xx 10xx 10xx 10xx while 3-7 only talks about encoding 0..D7FF and D800..10 in up to 4 bytes. I guess I should try what happens with 0x11 and 0x7fff in identifiers and string literals. Jakub
Re: [PATCH] libcpp: Add -Winvalid-utf8 warning [PR106655]
On Mon, Aug 29, 2022 at 11:35:44PM +0200, Jakub Jelinek wrote: > I guess I should try what happens with 0x11 and 0x7fff in > identifiers and string literals. It is rejected in identifiers, but happily accepted in string literals: const char32_t *a = U""; const char32_t *b = U"��"; int ab = 1; int c��d = 2; Jakub
Re: [PATCH] libcpp: Add -Winvalid-utf8 warning [PR106655]
On Mon, Aug 29, 2022 at 05:15:26PM -0400, Jason Merrill wrote: > On 8/29/22 04:15, Jakub Jelinek wrote: > > Hi! > > > > The following patch introduces a new warning - -Winvalid-utf8 similarly > > to what clang now has - to diagnose invalid UTF-8 byte sequences in > > comments. In identifiers and in string literals it should be diagnosed > > already but comment content hasn't been really verified. > > > > I'm not sure if this is enough to say P2295R6 is implemented or not. > > > > The problem is that in the most common case, people don't use > > -finput-charset= option and the sources often are UTF-8, but sometimes > > could be some ASCII compatible single byte encoding where non-ASCII > > characters only appear in comments. So having the warning off by default > > is IMO desirable. Now, if people use explicit -finput-charset=UTF-8, > > perhaps we could make the warning on by default for C++23 and use pedwarn > > instead of warning, because then the user told us explicitly that the source > > is UTF-8. From the paper I understood one of the implementation options > > is to claim that the implementation supports 2 encodings, UTF-8 and UTF-8 > > like encodings where invalid UTF-8 characters in comments are replaced say > > by spaces, where the latter could be the default and the former only > > used if -finput-charset=UTF-8 -Werror=invalid-utf8 options are used. > > > > Thoughts on this? > > That sounds good to me. The pedwarn on -std=c++23 -finput-charset=UTF-8 or just documenting that "conforming" UTF-8 is only -finput-charset=UTF-8 -Werror=invalid-utf8 ? > > +static const uchar * > > +_cpp_warn_invalid_utf8 (cpp_reader *pfile) > > +{ > > + cpp_buffer *buffer = pfile->buffer; > > + const uchar *cur = buffer->cur; > > + > > + if (cur[0] < utf8_signifier > > + || cur[1] < utf8_continuation || cur[1] >= utf8_signifier) > > +{ > > + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, > > +pfile->line_table->highest_line, > > +CPP_BUF_COL (buffer), > > +"invalid UTF-8 character <%x> in comment", > > +cur[0]); > > + return cur + 1; > > +} > > + else if (cur[2] < utf8_continuation || cur[2] >= utf8_signifier) > > Unicode table 3-7 says that the second byte is sometimes restricted to less > than this range. That is true and I've tried to include tests for all of those cases in the testcase and all of them get a warning. Some of them are through: /* Make sure the shortest possible encoding was used. */ if (c <= 0x7F && nbytes > 1) return EILSEQ; if (c <= 0x7FF && nbytes > 2) return EILSEQ; if (c <=0x && nbytes > 3) return EILSEQ; if (c <= 0x1F && nbytes > 4) return EILSEQ; if (c <= 0x3FF && nbytes > 5) return EILSEQ; and others are through: /* Make sure the character is valid. */ if (c > 0x7FFF || (c >= 0xD800 && c <= 0xDFFF)) return EILSEQ; All I had to do outside of what one_utf8_to_cppchar already implements was: > > + if (_cpp_valid_utf8 (pfile, &pstr, buffer->rlimit, 0, NULL, &s) > > + && s <= 0x0010) the <= 0x0010 check, because one_utf8_to_cppchar as written happily supports up to 6 bytes long UTF-8 which can encode up to 7FFF: -007F 0xxx 0080-07FF 110x 10xx 0800- 1110 10xx 10xx 0001-001F 0xxx 10xx 10xx 10xx 0020-03FF 10xx 10xx 10xx 10xx 10xx 0400-7FFF 110x 10xx 10xx 10xx 10xx 10xx while 3-7 only talks about encoding 0..D7FF and D800..10 in up to 4 bytes. I guess I should try what happens with 0x11 and 0x7fff in identifiers and string literals. Jakub
Re: [PATCH] libcpp: Add -Winvalid-utf8 warning [PR106655]
On 8/29/22 04:15, Jakub Jelinek wrote: Hi! The following patch introduces a new warning - -Winvalid-utf8 similarly to what clang now has - to diagnose invalid UTF-8 byte sequences in comments. In identifiers and in string literals it should be diagnosed already but comment content hasn't been really verified. I'm not sure if this is enough to say P2295R6 is implemented or not. The problem is that in the most common case, people don't use -finput-charset= option and the sources often are UTF-8, but sometimes could be some ASCII compatible single byte encoding where non-ASCII characters only appear in comments. So having the warning off by default is IMO desirable. Now, if people use explicit -finput-charset=UTF-8, perhaps we could make the warning on by default for C++23 and use pedwarn instead of warning, because then the user told us explicitly that the source is UTF-8. From the paper I understood one of the implementation options is to claim that the implementation supports 2 encodings, UTF-8 and UTF-8 like encodings where invalid UTF-8 characters in comments are replaced say by spaces, where the latter could be the default and the former only used if -finput-charset=UTF-8 -Werror=invalid-utf8 options are used. Thoughts on this? That sounds good to me. 2022-08-29 Jakub Jelinek PR c++/106655 libcpp/ * include/cpplib.h (struct cpp_options): Implement C++23 P2295R6 - Support for UTF-8 as a portable source file encoding. Add cpp_warn_invalid_utf8 field. (enum cpp_warning_reason): Add CPP_W_INVALID_UTF8 enumerator. * init.cc (cpp_create_reader): Initialize cpp_warn_invalid_utf8. * lex.cc (utf8_continuation): New const variable. (utf8_signifier): Move earlier in the file. (_cpp_warn_invalid_utf8): New function. (_cpp_skip_block_comment): Handle -Winvalid-utf8 warning. (skip_line_comment): Likewise. gcc/ * doc/invoke.texi (-Winvalid-utf8): Document it. gcc/c-family/ * c.opt (-Winvalid-utf8): New warning. gcc/testsuite/ * c-c++-common/cpp/Winvalid-utf8-1.c: New test. --- libcpp/include/cpplib.h.jj 2022-08-25 14:25:23.866912426 +0200 +++ libcpp/include/cpplib.h 2022-08-27 12:17:55.185022807 +0200 @@ -560,6 +560,9 @@ struct cpp_options cpp_bidirectional_level. */ unsigned char cpp_warn_bidirectional; + /* True if libcpp should warn about invalid UTF-8 characters in comments. */ + bool cpp_warn_invalid_utf8; + /* Dependency generation. */ struct { @@ -666,7 +669,8 @@ enum cpp_warning_reason { CPP_W_CXX11_COMPAT, CPP_W_CXX20_COMPAT, CPP_W_EXPANSION_TO_DEFINED, - CPP_W_BIDIRECTIONAL + CPP_W_BIDIRECTIONAL, + CPP_W_INVALID_UTF8 }; /* Callback for header lookup for HEADER, which is the name of a --- libcpp/init.cc.jj 2022-08-24 09:55:44.571876638 +0200 +++ libcpp/init.cc 2022-08-27 12:18:54.559246323 +0200 @@ -227,6 +227,7 @@ cpp_create_reader (enum c_lang lang, cpp CPP_OPTION (pfile, ext_numeric_literals) = 1; CPP_OPTION (pfile, warn_date_time) = 0; CPP_OPTION (pfile, cpp_warn_bidirectional) = bidirectional_unpaired; + CPP_OPTION (pfile, cpp_warn_invalid_utf8) = 0; /* Default CPP arithmetic to something sensible for the host for the benefit of dumb users like fix-header. */ --- libcpp/lex.cc.jj2022-08-26 09:24:12.089615949 +0200 +++ libcpp/lex.cc 2022-08-27 13:43:40.560769087 +0200 @@ -1704,6 +1704,59 @@ maybe_warn_bidi_on_char (cpp_reader *pfi bidi::on_char (kind, ucn_p, loc); } +static const cppchar_t utf8_continuation = 0x80; +static const cppchar_t utf8_signifier = 0xC0; > +/* Emit -Winvalid-utf8 warning on invalid UTF-8 character starting + at PFILE->buffer->cur. Return a pointer after the diagnosed + invalid character. */ + +static const uchar * +_cpp_warn_invalid_utf8 (cpp_reader *pfile) +{ + cpp_buffer *buffer = pfile->buffer; + const uchar *cur = buffer->cur; + + if (cur[0] < utf8_signifier + || cur[1] < utf8_continuation || cur[1] >= utf8_signifier) +{ + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, +pfile->line_table->highest_line, +CPP_BUF_COL (buffer), +"invalid UTF-8 character <%x> in comment", +cur[0]); + return cur + 1; +} + else if (cur[2] < utf8_continuation || cur[2] >= utf8_signifier) Unicode table 3-7 says that the second byte is sometimes restricted to less than this range. Hmm, it looks like one_utf8_to_cppchar doesn't check that, either. Did you consider adding error reporting to one_utf8_to_cppchar? It might be better to localize the utf8 logic. +{ + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, +pfile->line_table->highest_line, +CPP_BUF_COL (buffer), +"invalid UTF-8 character <%x><%x> in comment",
[PATCH] libcpp: Add -Winvalid-utf8 warning [PR106655]
Hi! The following patch introduces a new warning - -Winvalid-utf8 similarly to what clang now has - to diagnose invalid UTF-8 byte sequences in comments. In identifiers and in string literals it should be diagnosed already but comment content hasn't been really verified. I'm not sure if this is enough to say P2295R6 is implemented or not. The problem is that in the most common case, people don't use -finput-charset= option and the sources often are UTF-8, but sometimes could be some ASCII compatible single byte encoding where non-ASCII characters only appear in comments. So having the warning off by default is IMO desirable. Now, if people use explicit -finput-charset=UTF-8, perhaps we could make the warning on by default for C++23 and use pedwarn instead of warning, because then the user told us explicitly that the source is UTF-8. From the paper I understood one of the implementation options is to claim that the implementation supports 2 encodings, UTF-8 and UTF-8 like encodings where invalid UTF-8 characters in comments are replaced say by spaces, where the latter could be the default and the former only used if -finput-charset=UTF-8 -Werror=invalid-utf8 options are used. Thoughts on this? 2022-08-29 Jakub Jelinek PR c++/106655 libcpp/ * include/cpplib.h (struct cpp_options): Implement C++23 P2295R6 - Support for UTF-8 as a portable source file encoding. Add cpp_warn_invalid_utf8 field. (enum cpp_warning_reason): Add CPP_W_INVALID_UTF8 enumerator. * init.cc (cpp_create_reader): Initialize cpp_warn_invalid_utf8. * lex.cc (utf8_continuation): New const variable. (utf8_signifier): Move earlier in the file. (_cpp_warn_invalid_utf8): New function. (_cpp_skip_block_comment): Handle -Winvalid-utf8 warning. (skip_line_comment): Likewise. gcc/ * doc/invoke.texi (-Winvalid-utf8): Document it. gcc/c-family/ * c.opt (-Winvalid-utf8): New warning. gcc/testsuite/ * c-c++-common/cpp/Winvalid-utf8-1.c: New test. --- libcpp/include/cpplib.h.jj 2022-08-25 14:25:23.866912426 +0200 +++ libcpp/include/cpplib.h 2022-08-27 12:17:55.185022807 +0200 @@ -560,6 +560,9 @@ struct cpp_options cpp_bidirectional_level. */ unsigned char cpp_warn_bidirectional; + /* True if libcpp should warn about invalid UTF-8 characters in comments. */ + bool cpp_warn_invalid_utf8; + /* Dependency generation. */ struct { @@ -666,7 +669,8 @@ enum cpp_warning_reason { CPP_W_CXX11_COMPAT, CPP_W_CXX20_COMPAT, CPP_W_EXPANSION_TO_DEFINED, - CPP_W_BIDIRECTIONAL + CPP_W_BIDIRECTIONAL, + CPP_W_INVALID_UTF8 }; /* Callback for header lookup for HEADER, which is the name of a --- libcpp/init.cc.jj 2022-08-24 09:55:44.571876638 +0200 +++ libcpp/init.cc 2022-08-27 12:18:54.559246323 +0200 @@ -227,6 +227,7 @@ cpp_create_reader (enum c_lang lang, cpp CPP_OPTION (pfile, ext_numeric_literals) = 1; CPP_OPTION (pfile, warn_date_time) = 0; CPP_OPTION (pfile, cpp_warn_bidirectional) = bidirectional_unpaired; + CPP_OPTION (pfile, cpp_warn_invalid_utf8) = 0; /* Default CPP arithmetic to something sensible for the host for the benefit of dumb users like fix-header. */ --- libcpp/lex.cc.jj2022-08-26 09:24:12.089615949 +0200 +++ libcpp/lex.cc 2022-08-27 13:43:40.560769087 +0200 @@ -1704,6 +1704,59 @@ maybe_warn_bidi_on_char (cpp_reader *pfi bidi::on_char (kind, ucn_p, loc); } +static const cppchar_t utf8_continuation = 0x80; +static const cppchar_t utf8_signifier = 0xC0; + +/* Emit -Winvalid-utf8 warning on invalid UTF-8 character starting + at PFILE->buffer->cur. Return a pointer after the diagnosed + invalid character. */ + +static const uchar * +_cpp_warn_invalid_utf8 (cpp_reader *pfile) +{ + cpp_buffer *buffer = pfile->buffer; + const uchar *cur = buffer->cur; + + if (cur[0] < utf8_signifier + || cur[1] < utf8_continuation || cur[1] >= utf8_signifier) +{ + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, +pfile->line_table->highest_line, +CPP_BUF_COL (buffer), +"invalid UTF-8 character <%x> in comment", +cur[0]); + return cur + 1; +} + else if (cur[2] < utf8_continuation || cur[2] >= utf8_signifier) +{ + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, +pfile->line_table->highest_line, +CPP_BUF_COL (buffer), +"invalid UTF-8 character <%x><%x> in comment", +cur[0], cur[1]); + return cur + 2; +} + else if (cur[3] < utf8_continuation || cur[3] >= utf8_signifier) +{ + cpp_warning_with_line (pfile, CPP_W_INVALID_UTF8, +pfile->line_table->highest_line, +CPP_BUF_COL (buffer), +"invalid UTF-8 character <