* [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) @ 2017-10-20 17:03 Mukesh Kapoor 2017-10-20 18:00 ` Nathan Sidwell 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-20 17:03 UTC (permalink / raw) To: gcc-patches; +Cc: Nathan Sidwell, jason [-- Attachment #1: Type: text/plain, Size: 348 bytes --] Hi, This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80955. Handle user-defined literals correctly in lex_string(). An empty string followed by an identifier is a valid user-defined literal. Don't issue a warning for this case. Bootstrapped and tested with 'make check' on x86_64-linux. New test case added. Ok for trunk? Mukesh [-- Attachment #2: patch_80955 --] [-- Type: text/plain, Size: 1355 bytes --] Index: gcc/testsuite/g++.dg/cpp0x/udlit-macros.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (revision 0) +++ gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (working copy) @@ -0,0 +1,7 @@ +// PR c++/80955 +// { dg-do compile { target c++11 } } + +using size_t = decltype(sizeof(0)); +#define _zero +int operator""_zero(const char*, size_t) { return 0; } +int main() { return ""_zero; } Index: libcpp/lex.c =================================================================== --- libcpp/lex.c (revision 253775) +++ libcpp/lex.c (working copy) @@ -2001,8 +2001,11 @@ /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + Don't do this for a user-defined literal, i.e. an + empty string followed by an identifier. + For an empty string "", (cur-base)==2. Bug 80955 */ + if (is_macro (pfile, cur) && ((cur-base) != 2)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) [-- Attachment #3: CL_80955 --] [-- Type: text/plain, Size: 347 bytes --] /libcpp 2017-10-20 Mukesh Kapoor <mukesh.kapoor@oracle.com> PR c++/80955 * lex.c (lex_string): An empty string followed by an identifier is a valid user-defined literal. Don't issue a warning for this case. /testsuite 2017-10-20 Mukesh Kapoor <mukesh.kapoor@oracle.com> PR c++/80955 * g++.dg/cpp0x/udlit-macros.C: New. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-20 17:03 [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) Mukesh Kapoor @ 2017-10-20 18:00 ` Nathan Sidwell 2017-10-20 18:19 ` Mukesh Kapoor 0 siblings, 1 reply; 17+ messages in thread From: Nathan Sidwell @ 2017-10-20 18:00 UTC (permalink / raw) To: Mukesh Kapoor, gcc-patches; +Cc: jason On 10/20/2017 12:37 PM, Mukesh Kapoor wrote: > Hi, > > This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80955. > Handle user-defined literals correctly in lex_string(). An empty string > followed by an identifier is > a valid user-defined literal. Don't issue a warning for this case. a) why do we trigger on the definition of the operator function, and not on the use site? b) Why is the empty string special cased? Doesn't the same logic apply to: int operator "bob"_zero (const char *, size_t) { return 0;} (that'd be a syntactic error in the C++ parser of course) nathan -- Nathan Sidwell ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-20 18:00 ` Nathan Sidwell @ 2017-10-20 18:19 ` Mukesh Kapoor 2017-10-20 20:56 ` Mukesh Kapoor 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-20 18:19 UTC (permalink / raw) To: Nathan Sidwell, gcc-patches; +Cc: jason Hi, On 10/20/2017 10:45 AM, Nathan Sidwell wrote: > On 10/20/2017 12:37 PM, Mukesh Kapoor wrote: >> Hi, >> >> This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80955. >> Handle user-defined literals correctly in lex_string(). An empty >> string followed by an identifier is >> a valid user-defined literal. Don't issue a warning for this case. > > a) why do we trigger on the definition of the operator function, and > not on the use site? Actually, the current compiler issues an error (incorrectly) at both places: at the definition as well as at its use. > > b) Why is the empty string special cased? Doesn't the same logic > apply to: > > int operator "bob"_zero (const char *, size_t) { return 0;} This is not a valid user-defined literal and is already reported as an error by the compiler. After my changes it's still reported as an error. The empty string immediately followed by an identifier is a special case because it's a valid user-defined literal in C++. ""_zero is a valid user-defined literal. Mukesh > > (that'd be a syntactic error in the C++ parser of course) > > nathan > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-20 18:19 ` Mukesh Kapoor @ 2017-10-20 20:56 ` Mukesh Kapoor 2017-10-24 14:07 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-20 20:56 UTC (permalink / raw) To: Nathan Sidwell, gcc-patches; +Cc: jason On 10/20/2017 11:00 AM, Mukesh Kapoor wrote: > Hi, > > On 10/20/2017 10:45 AM, Nathan Sidwell wrote: >> On 10/20/2017 12:37 PM, Mukesh Kapoor wrote: >>> Hi, >>> >>> This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80955. >>> Handle user-defined literals correctly in lex_string(). An empty >>> string followed by an identifier is >>> a valid user-defined literal. Don't issue a warning for this case. >> >> a) why do we trigger on the definition of the operator function, and >> not on the use site? > > Actually, the current compiler issues an error (incorrectly) at both > places: at the definition as well as at its use. > >> >> b) Why is the empty string special cased? Doesn't the same logic >> apply to: >> >> int operator "bob"_zero (const char *, size_t) { return 0;} > > This is not a valid user-defined literal and is already reported as an > error by the compiler. After my changes it's still reported as an error. > The empty string immediately followed by an identifier is a special > case because it's a valid user-defined literal in C++. ""_zero is a > valid user-defined literal. Sorry, I used incorrect terminology here. An empty string immediately followed by an identifier is a valid name for a literal operator; ""_zero is a valid name for a literal operator. Mukesh > > Mukesh > >> >> (that'd be a syntactic error in the C++ parser of course) >> >> nathan >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-20 20:56 ` Mukesh Kapoor @ 2017-10-24 14:07 ` Jason Merrill 2017-10-25 5:48 ` Mukesh Kapoor 0 siblings, 1 reply; 17+ messages in thread From: Jason Merrill @ 2017-10-24 14:07 UTC (permalink / raw) To: Mukesh Kapoor; +Cc: Nathan Sidwell, gcc-patches List On Fri, Oct 20, 2017 at 3:52 PM, Mukesh Kapoor <mukesh.kapoor@oracle.com> wrote: > On 10/20/2017 11:00 AM, Mukesh Kapoor wrote: >> On 10/20/2017 10:45 AM, Nathan Sidwell wrote: >>> On 10/20/2017 12:37 PM, Mukesh Kapoor wrote: >>>> >>>> This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80955. >>>> Handle user-defined literals correctly in lex_string(). An empty string >>>> followed by an identifier is >>>> a valid user-defined literal. Don't issue a warning for this case. >>> >>> a) why do we trigger on the definition of the operator function, and not >>> on the use site? >> >> Actually, the current compiler issues an error (incorrectly) at both >> places: at the definition as well as at its use. >> >>> b) Why is the empty string special cased? Doesn't the same logic apply >>> to: >>> >>> int operator "bob"_zero (const char *, size_t) { return 0;} >> >> This is not a valid user-defined literal and is already reported as an >> error by the compiler. After my changes it's still reported as an error. >> The empty string immediately followed by an identifier is a special case >> because it's a valid user-defined literal in C++. ""_zero is a valid >> user-defined literal. > > Sorry, I used incorrect terminology here. An empty string immediately > followed by an identifier is a valid name for a literal operator; ""_zero is > a valid name for a literal operator. Yes, and indeed "bob"_zero isn't. But it would be fine for the testcase to return "bob"_zero, a valid user-defined string literal which calls operator ""_zero, and that will still break after your patch. It seems like we need to handle this error recovery in the front end, where we can look to see if there's a matching operator, rather than in libcpp. Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-24 14:07 ` Jason Merrill @ 2017-10-25 5:48 ` Mukesh Kapoor 2017-10-25 11:26 ` Nathan Sidwell 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-25 5:48 UTC (permalink / raw) To: Jason Merrill; +Cc: Nathan Sidwell, gcc-patches List On 10/24/2017 7:05 AM, Jason Merrill wrote: > On Fri, Oct 20, 2017 at 3:52 PM, Mukesh Kapoor <mukesh.kapoor@oracle.com> wrote: >> On 10/20/2017 11:00 AM, Mukesh Kapoor wrote: >>> On 10/20/2017 10:45 AM, Nathan Sidwell wrote: >>>> On 10/20/2017 12:37 PM, Mukesh Kapoor wrote: >>>>> This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80955. >>>>> Handle user-defined literals correctly in lex_string(). An empty string >>>>> followed by an identifier is >>>>> a valid user-defined literal. Don't issue a warning for this case. >>>> a) why do we trigger on the definition of the operator function, and not >>>> on the use site? >>> Actually, the current compiler issues an error (incorrectly) at both >>> places: at the definition as well as at its use. >>> >>>> b) Why is the empty string special cased? Doesn't the same logic apply >>>> to: >>>> >>>> int operator "bob"_zero (const char *, size_t) { return 0;} >>> This is not a valid user-defined literal and is already reported as an >>> error by the compiler. After my changes it's still reported as an error. >>> The empty string immediately followed by an identifier is a special case >>> because it's a valid user-defined literal in C++. ""_zero is a valid >>> user-defined literal. >> Sorry, I used incorrect terminology here. An empty string immediately >> followed by an identifier is a valid name for a literal operator; ""_zero is >> a valid name for a literal operator. > Yes, and indeed "bob"_zero isn't. But it would be fine for the > testcase to return "bob"_zero, a valid user-defined string literal > which calls operator ""_zero, and that will still break after your > patch. > > It seems like we need to handle this error recovery in the front end, > where we can look to see if there's a matching operator, rather than > in libcpp. Thanks for pointing this out. Checking in the front end will be difficult because the front end gets tokens after macro expansion. I think the difficulty of fixing this bug comes because of the requirement to maintain backward compatibility with the option -Wliteral-suffix for -std=c++11. Mukesh > > Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-25 5:48 ` Mukesh Kapoor @ 2017-10-25 11:26 ` Nathan Sidwell 2017-10-25 16:13 ` Jason Merrill 2017-10-26 3:30 ` Mukesh Kapoor 0 siblings, 2 replies; 17+ messages in thread From: Nathan Sidwell @ 2017-10-25 11:26 UTC (permalink / raw) To: Mukesh Kapoor, Jason Merrill; +Cc: gcc-patches List On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: > Thanks for pointing this out. Checking in the front end will be > difficult because the front end gets tokens after macro expansion. I > think the difficulty of fixing this bug comes because of the requirement > to maintain backward compatibility with the option -Wliteral-suffix for > -std=c++11. IIUC the warning's intent is to catch cases of: printf ("some format"PRIx64 ..., ...); where there's no space between the string literals and the PRIx64 macro. I suspect it's very common for there to be a following string-literal, so perhaps the preprocessor could detect: <string-literal>NON-FN-MACRO<maybe-space><string-literal> and warn on that sequence? nathan -- Nathan Sidwell ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-25 11:26 ` Nathan Sidwell @ 2017-10-25 16:13 ` Jason Merrill 2017-10-25 16:14 ` Mukesh Kapoor 2017-10-26 3:30 ` Mukesh Kapoor 1 sibling, 1 reply; 17+ messages in thread From: Jason Merrill @ 2017-10-25 16:13 UTC (permalink / raw) To: Nathan Sidwell; +Cc: Mukesh Kapoor, gcc-patches List On Wed, Oct 25, 2017 at 7:20 AM, Nathan Sidwell <nathan@acm.org> wrote: > On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: > >> Thanks for pointing this out. Checking in the front end will be difficult >> because the front end gets tokens after macro expansion. I think the >> difficulty of fixing this bug comes because of the requirement to maintain >> backward compatibility with the option -Wliteral-suffix for -std=c++11. > > > IIUC the warning's intent is to catch cases of: > printf ("some format"PRIx64 ..., ...); > where there's no space between the string literals and the PRIx64 macro. I > suspect it's very common for there to be a following string-literal, so > perhaps the preprocessor could detect: > > <string-literal>NON-FN-MACRO<maybe-space><string-literal> > > and warn on that sequence? Or perhaps we could limit the current behavior to when the macro expands to a string literal? Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-25 16:13 ` Jason Merrill @ 2017-10-25 16:14 ` Mukesh Kapoor 0 siblings, 0 replies; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-25 16:14 UTC (permalink / raw) To: Jason Merrill, Nathan Sidwell; +Cc: gcc-patches List On 10/25/2017 9:06 AM, Jason Merrill wrote: > On Wed, Oct 25, 2017 at 7:20 AM, Nathan Sidwell <nathan@acm.org> wrote: >> On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: >> >>> Thanks for pointing this out. Checking in the front end will be difficult >>> because the front end gets tokens after macro expansion. I think the >>> difficulty of fixing this bug comes because of the requirement to maintain >>> backward compatibility with the option -Wliteral-suffix for -std=c++11. >> >> IIUC the warning's intent is to catch cases of: >> printf ("some format"PRIx64 ..., ...); >> where there's no space between the string literals and the PRIx64 macro. I >> suspect it's very common for there to be a following string-literal, so >> perhaps the preprocessor could detect: >> >> <string-literal>NON-FN-MACRO<maybe-space><string-literal> >> >> and warn on that sequence? > Or perhaps we could limit the current behavior to when the macro > expands to a string literal? To check this the macro will have to be expanded completely. A macro can be defined using other macros. The header <inttypes.h> has the following definition for PRId64: #Â define __PRI64_PREFIXÂ Â Â Â Â Â Â "l" # define PRId64Â Â Â Â Â Â Â Â __PRI64_PREFIX "d" Mukesh > > Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-25 11:26 ` Nathan Sidwell 2017-10-25 16:13 ` Jason Merrill @ 2017-10-26 3:30 ` Mukesh Kapoor 2017-10-31 16:26 ` Mukesh Kapoor 1 sibling, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-26 3:30 UTC (permalink / raw) To: Nathan Sidwell, Jason Merrill; +Cc: gcc-patches List On 10/25/2017 4:20 AM, Nathan Sidwell wrote: > On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: > >> Thanks for pointing this out. Checking in the front end will be >> difficult because the front end gets tokens after macro expansion. I >> think the difficulty of fixing this bug comes because of the >> requirement to maintain backward compatibility with the option >> -Wliteral-suffix for -std=c++11. > > IIUC the warning's intent is to catch cases of: > printf ("some format"PRIx64 ..., ...); > where there's no space between the string literals and the PRIx64 > macro. Â I suspect it's very common for there to be a following > string-literal, so perhaps the preprocessor could detect: > > <string-literal>NON-FN-MACRO<maybe-space><string-literal> > > and warn on that sequence? Yes, this can be done easily and this is also the usage mentioned in the man page. I made this change in the compiler, bootstrapped it and ran the tests. The following two tests fail after the fix: g++.dg/cpp0x/Wliteral-suffix.C g++.dg/cpp0x/warn_cxx0x4.C Both tests have code similar to the following (from Wliteral-suffix.C): #define BAR "bar" #define PLUS_ONE + 1 Â char c = '3'PLUS_ONE;Â Â // { dg-warning "invalid suffix on literal" } Â char s[] = "foo"BAR;Â Â Â // { dg-warning "invalid suffix on literal" } Other compilers don't accept this code. Maybe I should just modify these tests to have error messages instead of warnings and submit my revised fix? Mukesh > > nathan > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-26 3:30 ` Mukesh Kapoor @ 2017-10-31 16:26 ` Mukesh Kapoor 2017-11-01 20:02 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-10-31 16:26 UTC (permalink / raw) To: Nathan Sidwell, Jason Merrill; +Cc: gcc-patches List [-- Attachment #1: Type: text/plain, Size: 2642 bytes --] On 10/25/2017 6:44 PM, Mukesh Kapoor wrote: > On 10/25/2017 4:20 AM, Nathan Sidwell wrote: >> On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: >> >>> Thanks for pointing this out. Checking in the front end will be >>> difficult because the front end gets tokens after macro expansion. I >>> think the difficulty of fixing this bug comes because of the >>> requirement to maintain backward compatibility with the option >>> -Wliteral-suffix for -std=c++11. >> >> IIUC the warning's intent is to catch cases of: >> printf ("some format"PRIx64 ..., ...); >> where there's no space between the string literals and the PRIx64 >> macro.  I suspect it's very common for there to be a following >> string-literal, so perhaps the preprocessor could detect: >> >> <string-literal>NON-FN-MACRO<maybe-space><string-literal> >> >> and warn on that sequence? > > Yes, this can be done easily and this is also the usage mentioned in > the man page. I made this change in the compiler, bootstrapped it and > ran the tests. The following two tests fail after the fix: > > g++.dg/cpp0x/Wliteral-suffix.C > g++.dg/cpp0x/warn_cxx0x4.C > > Both tests have code similar to the following (from Wliteral-suffix.C): > > #define BAR "bar" > #define PLUS_ONE + 1 > >  char c = '3'PLUS_ONE;  // { dg-warning "invalid suffix on literal" } >  char s[] = "foo"BAR;   // { dg-warning "invalid suffix on literal" } > > Other compilers don't accept this code. Maybe I should just modify > these tests to have error messages instead of warnings and submit my > revised fix? Actually, according to the man page for -Wliteral-suffix, only macro names that don't start with an underscore should be considered when issuing a warning:       -Wliteral-suffix (C++ and Objective-C++ only)           Warn when a string or character literal is followed by a ud-suffix           which does not begin with an underscore... So the fix is simply to check if the macro name in is_macro() starts with an underscore. The function is_macro() is called only at three places. At two places it's used to check for the warning related to -Wliteral-suffix and the check for underscore should be made for these two cases; at one place it is used to check for the warning related to -Wc++11-compat and there is no need to check for underscore for this case. The fix is simply to pass a bool flag as an additional argument to is_macro() to decide whether the macro name starts with an underscore or not. I have tested the attached patch on x86_64-linux. Thanks. Mukesh [-- Attachment #2: CL_80955 --] [-- Type: text/plain, Size: 767 bytes --] /libcpp 2017-10-31 Mukesh Kapoor <mukesh.kapoor@oracle.com> PR c++/80955 * lex.c (lex_string): Add an argument, 'bool no_underscore', to is_macro(). If no_underscore is true, check if the macro name starts with an underscore and return false if it does. If no_underscore is false, don't check for underscore. is_macro() is called at three places. At two places it's used to check for the warning related to -Wliteral-suffix and the check for underscore is made for these two cases. At one place it's used to check for the warning related to -Wc++11-compat and the check for underscore is not made for this case. /testsuite 2017-10-31 Mukesh Kapoor <mukesh.kapoor@oracle.com> PR c++/80955 * g++.dg/cpp0x/udlit-macros.C: New. [-- Attachment #3: patch_80955 --] [-- Type: text/plain, Size: 2893 bytes --] Index: gcc/testsuite/g++.dg/cpp0x/udlit-macros.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (revision 0) +++ gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (working copy) @@ -0,0 +1,31 @@ +// PR c++/80955 +// { dg-do compile { target c++11 } } + +#define __STDC_FORMAT_MACROS +#include <inttypes.h> +#include <stdio.h> +#include <string.h> + +using size_t = decltype(sizeof(0)); +#define _zero +#define _ID _xx +int operator""_zero(const char*, size_t) { return 0; } +int operator""_ID(const char*, size_t) { return 0; } + +int main() +{ + int64_t i64 = 123; + char buf[100]; + sprintf(buf, "%"PRId64"abc", i64); // { dg-warning "invalid suffix on literal" } + return strcmp(buf, "123abc") + + ""_zero + + "bob"_zero + + R"#(raw + string)#"_zero + + "xx"_ID + + ""_ID + + R"AA(another + raw + string)AA"_ID; +} + Index: libcpp/lex.c =================================================================== --- libcpp/lex.c (revision 254048) +++ libcpp/lex.c (working copy) @@ -1576,14 +1576,17 @@ /* Returns true if a macro has been defined. + If no_underscore is true, check that the macro + name does not start with underscore. This might not work if compile with -save-temps, or preprocess separately from compilation. */ static bool -is_macro(cpp_reader *pfile, const uchar *base) +is_macro(cpp_reader *pfile, const uchar *base, bool no_underscore) { const uchar *cur = base; - if (! ISIDST (*cur)) + bool invalid_ident = (no_underscore ? ! ISALPHA (*cur) : ! ISIDST (*cur)); + if (invalid_ident) return false; unsigned int hash = HT_HASHSTEP (0, *cur); ++cur; @@ -1872,7 +1875,7 @@ a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + if (is_macro (pfile, cur, true)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) @@ -2002,7 +2005,7 @@ a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + if (is_macro (pfile, cur, true)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) @@ -2023,7 +2026,7 @@ } } else if (CPP_OPTION (pfile, cpp_warn_cxx11_compat) - && is_macro (pfile, cur) + && is_macro (pfile, cur, false) && !pfile->state.skipping) cpp_warning_with_line (pfile, CPP_W_CXX11_COMPAT, token->src_loc, 0, "C++11 requires a space " ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-10-31 16:26 ` Mukesh Kapoor @ 2017-11-01 20:02 ` Jason Merrill 2017-11-01 20:44 ` Mukesh Kapoor 0 siblings, 1 reply; 17+ messages in thread From: Jason Merrill @ 2017-11-01 20:02 UTC (permalink / raw) To: Mukesh Kapoor; +Cc: Nathan Sidwell, gcc-patches List On Tue, Oct 31, 2017 at 12:17 PM, Mukesh Kapoor <mukesh.kapoor@oracle.com> wrote: > On 10/25/2017 6:44 PM, Mukesh Kapoor wrote: >> >> On 10/25/2017 4:20 AM, Nathan Sidwell wrote: >>> >>> On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: >>> >>>> Thanks for pointing this out. Checking in the front end will be >>>> difficult because the front end gets tokens after macro expansion. I think >>>> the difficulty of fixing this bug comes because of the requirement to >>>> maintain backward compatibility with the option -Wliteral-suffix for >>>> -std=c++11. >>> >>> >>> IIUC the warning's intent is to catch cases of: >>> printf ("some format"PRIx64 ..., ...); >>> where there's no space between the string literals and the PRIx64 macro. >>> I suspect it's very common for there to be a following string-literal, so >>> perhaps the preprocessor could detect: >>> >>> <string-literal>NON-FN-MACRO<maybe-space><string-literal> >>> >>> and warn on that sequence? >> >> >> Yes, this can be done easily and this is also the usage mentioned in the >> man page. I made this change in the compiler, bootstrapped it and ran the >> tests. The following two tests fail after the fix: >> >> g++.dg/cpp0x/Wliteral-suffix.C >> g++.dg/cpp0x/warn_cxx0x4.C >> >> Both tests have code similar to the following (from Wliteral-suffix.C): >> >> #define BAR "bar" >> #define PLUS_ONE + 1 >> >> char c = '3'PLUS_ONE; // { dg-warning "invalid suffix on literal" } >> char s[] = "foo"BAR; // { dg-warning "invalid suffix on literal" } >> >> Other compilers don't accept this code. Maybe I should just modify these >> tests to have error messages instead of warnings and submit my revised fix? > > > Actually, according to the man page for -Wliteral-suffix, only macro names > that don't start with an underscore should be considered when issuing a > warning: > > -Wliteral-suffix (C++ and Objective-C++ only) > Warn when a string or character literal is followed by a > ud-suffix > which does not begin with an underscore... > > So the fix is simply to check if the macro name in is_macro() starts with an > underscore. The function is_macro() is called only at three places. At two > places it's used to check for the warning related to -Wliteral-suffix and > the check for underscore should be made for these two cases; at one place it > is used to check for the warning related to -Wc++11-compat and there is no > need to check for underscore for this case. > > The fix is simply to pass a bool flag as an additional argument to > is_macro() to decide whether the macro name starts with an underscore or > not. I have tested the attached patch on x86_64-linux. Thanks. Rather than add a mysterious parameter to is_macro, how about checking *cur != '_' before we call it? Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-11-01 20:02 ` Jason Merrill @ 2017-11-01 20:44 ` Mukesh Kapoor 2017-11-02 14:42 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-11-01 20:44 UTC (permalink / raw) To: Jason Merrill; +Cc: Nathan Sidwell, gcc-patches List [-- Attachment #1: Type: text/plain, Size: 2957 bytes --] On 11/1/2017 1:02 PM, Jason Merrill wrote: > On Tue, Oct 31, 2017 at 12:17 PM, Mukesh Kapoor > <mukesh.kapoor@oracle.com> wrote: >> On 10/25/2017 6:44 PM, Mukesh Kapoor wrote: >>> On 10/25/2017 4:20 AM, Nathan Sidwell wrote: >>>> On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: >>>> >>>>> Thanks for pointing this out. Checking in the front end will be >>>>> difficult because the front end gets tokens after macro expansion. I think >>>>> the difficulty of fixing this bug comes because of the requirement to >>>>> maintain backward compatibility with the option -Wliteral-suffix for >>>>> -std=c++11. >>>> >>>> IIUC the warning's intent is to catch cases of: >>>> printf ("some format"PRIx64 ..., ...); >>>> where there's no space between the string literals and the PRIx64 macro. >>>> I suspect it's very common for there to be a following string-literal, so >>>> perhaps the preprocessor could detect: >>>> >>>> <string-literal>NON-FN-MACRO<maybe-space><string-literal> >>>> >>>> and warn on that sequence? >>> >>> Yes, this can be done easily and this is also the usage mentioned in the >>> man page. I made this change in the compiler, bootstrapped it and ran the >>> tests. The following two tests fail after the fix: >>> >>> g++.dg/cpp0x/Wliteral-suffix.C >>> g++.dg/cpp0x/warn_cxx0x4.C >>> >>> Both tests have code similar to the following (from Wliteral-suffix.C): >>> >>> #define BAR "bar" >>> #define PLUS_ONE + 1 >>> >>> char c = '3'PLUS_ONE; // { dg-warning "invalid suffix on literal" } >>> char s[] = "foo"BAR; // { dg-warning "invalid suffix on literal" } >>> >>> Other compilers don't accept this code. Maybe I should just modify these >>> tests to have error messages instead of warnings and submit my revised fix? >> >> Actually, according to the man page for -Wliteral-suffix, only macro names >> that don't start with an underscore should be considered when issuing a >> warning: >> >> -Wliteral-suffix (C++ and Objective-C++ only) >> Warn when a string or character literal is followed by a >> ud-suffix >> which does not begin with an underscore... >> >> So the fix is simply to check if the macro name in is_macro() starts with an >> underscore. The function is_macro() is called only at three places. At two >> places it's used to check for the warning related to -Wliteral-suffix and >> the check for underscore should be made for these two cases; at one place it >> is used to check for the warning related to -Wc++11-compat and there is no >> need to check for underscore for this case. >> >> The fix is simply to pass a bool flag as an additional argument to >> is_macro() to decide whether the macro name starts with an underscore or >> not. I have tested the attached patch on x86_64-linux. Thanks. > Rather than add a mysterious parameter to is_macro, how about checking > *cur != '_' before we call it? This is a good suggestion. I have attached the revised patch. Thanks. Mukesh [-- Attachment #2: CL_80955 --] [-- Type: text/plain, Size: 426 bytes --] /libcpp 2017-10-31 Mukesh Kapoor <mukesh.kapoor@oracle.com> PR c++/80955 * lex.c (lex_string): When checking for a valid macro for the warning related to -Wliteral-suffix (CPP_W_LITERAL_SUFFIX), check that the macro name does not start with an underscore before calling is_macro(). /testsuite 2017-10-31 Mukesh Kapoor <mukesh.kapoor@oracle.com> PR c++/80955 * g++.dg/cpp0x/udlit-macros.C: New. [-- Attachment #3: patch_80955 --] [-- Type: text/plain, Size: 2374 bytes --] Index: gcc/testsuite/g++.dg/cpp0x/udlit-macros.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (revision 0) +++ gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (working copy) @@ -0,0 +1,31 @@ +// PR c++/80955 +// { dg-do compile { target c++11 } } + +#define __STDC_FORMAT_MACROS +#include <inttypes.h> +#include <stdio.h> +#include <string.h> + +using size_t = decltype(sizeof(0)); +#define _zero +#define _ID _xx +int operator""_zero(const char*, size_t) { return 0; } +int operator""_ID(const char*, size_t) { return 0; } + +int main() +{ + int64_t i64 = 123; + char buf[100]; + sprintf(buf, "%"PRId64"abc", i64); // { dg-warning "invalid suffix on literal" } + return strcmp(buf, "123abc") + + ""_zero + + "bob"_zero + + R"#(raw + string)#"_zero + + "xx"_ID + + ""_ID + + R"AA(another + raw + string)AA"_ID; +} + Index: libcpp/lex.c =================================================================== --- libcpp/lex.c (revision 254048) +++ libcpp/lex.c (working copy) @@ -1871,8 +1871,9 @@ /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + The macro name should not start with '_' for this warning. */ + if ((*cur != '_') && is_macro (pfile, cur)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) @@ -2001,8 +2002,9 @@ /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + The macro name should not start with '_' for this warning. */ + if ((*cur != '_') && is_macro (pfile, cur)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-11-01 20:44 ` Mukesh Kapoor @ 2017-11-02 14:42 ` Jason Merrill 2017-11-03 14:31 ` Paolo Carlini 0 siblings, 1 reply; 17+ messages in thread From: Jason Merrill @ 2017-11-02 14:42 UTC (permalink / raw) To: Mukesh Kapoor; +Cc: Nathan Sidwell, gcc-patches List On Wed, Nov 1, 2017 at 4:45 PM, Mukesh Kapoor <mukesh.kapoor@oracle.com> wrote: > On 11/1/2017 1:02 PM, Jason Merrill wrote: >> >> On Tue, Oct 31, 2017 at 12:17 PM, Mukesh Kapoor >> <mukesh.kapoor@oracle.com> wrote: >>> >>> On 10/25/2017 6:44 PM, Mukesh Kapoor wrote: >>>> >>>> On 10/25/2017 4:20 AM, Nathan Sidwell wrote: >>>>> >>>>> On 10/25/2017 12:03 AM, Mukesh Kapoor wrote: >>>>> >>>>>> Thanks for pointing this out. Checking in the front end will be >>>>>> difficult because the front end gets tokens after macro expansion. I >>>>>> think >>>>>> the difficulty of fixing this bug comes because of the requirement to >>>>>> maintain backward compatibility with the option -Wliteral-suffix for >>>>>> -std=c++11. >>>>> >>>>> >>>>> IIUC the warning's intent is to catch cases of: >>>>> printf ("some format"PRIx64 ..., ...); >>>>> where there's no space between the string literals and the PRIx64 >>>>> macro. >>>>> I suspect it's very common for there to be a following string-literal, >>>>> so >>>>> perhaps the preprocessor could detect: >>>>> >>>>> <string-literal>NON-FN-MACRO<maybe-space><string-literal> >>>>> >>>>> and warn on that sequence? >>>> >>>> >>>> Yes, this can be done easily and this is also the usage mentioned in the >>>> man page. I made this change in the compiler, bootstrapped it and ran >>>> the >>>> tests. The following two tests fail after the fix: >>>> >>>> g++.dg/cpp0x/Wliteral-suffix.C >>>> g++.dg/cpp0x/warn_cxx0x4.C >>>> >>>> Both tests have code similar to the following (from Wliteral-suffix.C): >>>> >>>> #define BAR "bar" >>>> #define PLUS_ONE + 1 >>>> >>>> char c = '3'PLUS_ONE; // { dg-warning "invalid suffix on literal" } >>>> char s[] = "foo"BAR; // { dg-warning "invalid suffix on literal" } >>>> >>>> Other compilers don't accept this code. Maybe I should just modify these >>>> tests to have error messages instead of warnings and submit my revised >>>> fix? >>> >>> >>> Actually, according to the man page for -Wliteral-suffix, only macro >>> names >>> that don't start with an underscore should be considered when issuing a >>> warning: >>> >>> -Wliteral-suffix (C++ and Objective-C++ only) >>> Warn when a string or character literal is followed by a >>> ud-suffix >>> which does not begin with an underscore... >>> >>> So the fix is simply to check if the macro name in is_macro() starts with >>> an >>> underscore. The function is_macro() is called only at three places. At >>> two >>> places it's used to check for the warning related to -Wliteral-suffix and >>> the check for underscore should be made for these two cases; at one place >>> it >>> is used to check for the warning related to -Wc++11-compat and there is >>> no >>> need to check for underscore for this case. >>> >>> The fix is simply to pass a bool flag as an additional argument to >>> is_macro() to decide whether the macro name starts with an underscore or >>> not. I have tested the attached patch on x86_64-linux. Thanks. >> >> Rather than add a mysterious parameter to is_macro, how about checking >> *cur != '_' before we call it? > > This is a good suggestion. I have attached the revised patch. Thanks. OK, thanks! Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-11-02 14:42 ` Jason Merrill @ 2017-11-03 14:31 ` Paolo Carlini 2017-11-04 22:38 ` Mukesh Kapoor 0 siblings, 1 reply; 17+ messages in thread From: Paolo Carlini @ 2017-11-03 14:31 UTC (permalink / raw) To: Jason Merrill, Mukesh Kapoor; +Cc: Nathan Sidwell, gcc-patches List Hi, On 02/11/2017 15:42, Jason Merrill wrote: > >> This is a good suggestion. I have attached the revised patch. Thanks. > OK, thanks! Thanks Jason. I was about to volunteer committing the patch but then noticed that the testcase includes quite a lot, eg, <inttypes.h> too, which we never include in the whole C++ testsuite. Can we have something simpler? Also, we don't need to include the whole <stdio.h> and <string.h> for a couple of declarations, we can simply provide by hand the declarations of sprintf and strcmp at the beginning of the file (plenty of examples in the testsuite). Mukesh, can you please work on that? Also, please watch out trailing blank lines. Thanks, Paolo. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-11-03 14:31 ` Paolo Carlini @ 2017-11-04 22:38 ` Mukesh Kapoor 2017-11-06 10:30 ` Paolo Carlini 0 siblings, 1 reply; 17+ messages in thread From: Mukesh Kapoor @ 2017-11-04 22:38 UTC (permalink / raw) To: Paolo Carlini, Jason Merrill; +Cc: Nathan Sidwell, gcc-patches List [-- Attachment #1: Type: text/plain, Size: 1050 bytes --] On 11/3/2017 7:31 AM, Paolo Carlini wrote: > Hi, > > On 02/11/2017 15:42, Jason Merrill wrote: >> >>> This is a good suggestion. I have attached the revised patch. Thanks. >> OK, thanks! > Thanks Jason. > > I was about to volunteer committing the patch but then noticed that > the testcase includes quite a lot, eg, <inttypes.h> too, which we > never include in the whole C++ testsuite. Can we have something > simpler? Also, we don't need to include the whole <stdio.h> and > <string.h> for a couple of declarations, we can simply provide by hand > the declarations of sprintf and strcmp at the beginning of the file > (plenty of examples in the testsuite). Mukesh, can you please work on > that? Also, please watch out trailing blank lines. I had included <inttypes.h> to get the definition of macro PRId64. I have now modified the test case to remove all includes. I have added the definition of the macro in the test case and also added declarations of functions sprintf() strcmp(). I have attached the revised patch. Thanks. Mukesh [-- Attachment #2: patch_80955 --] [-- Type: text/plain, Size: 2505 bytes --] Index: gcc/testsuite/g++.dg/cpp0x/udlit-macros.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (revision 0) +++ gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (working copy) @@ -0,0 +1,32 @@ +// PR c++/80955 +// { dg-do compile { target c++11 } } + +extern "C" int sprintf(char *__restrict s, + const char *__restrict format, ...); +extern "C" int strcmp(const char *s1, const char *s2); + +# define __PRI64_PREFIX "l" +# define PRId64 __PRI64_PREFIX "d" + +using size_t = decltype(sizeof(0)); +#define _zero +#define _ID _xx +int operator""_zero(const char*, size_t) { return 0; } +int operator""_ID(const char*, size_t) { return 0; } + +int main() +{ + int i64 = 123; + char buf[100]; + sprintf(buf, "%"PRId64"abc", i64); // { dg-warning "invalid suffix on literal" } + return strcmp(buf, "123abc") + + ""_zero + + "bob"_zero + + R"#(raw + string)#"_zero + + "xx"_ID + + ""_ID + + R"AA(another + raw + string)AA"_ID; +} Index: libcpp/lex.c =================================================================== --- libcpp/lex.c (revision 254048) +++ libcpp/lex.c (working copy) @@ -1871,8 +1871,9 @@ /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + The macro name should not start with '_' for this warning. */ + if ((*cur != '_') && is_macro (pfile, cur)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) @@ -2001,8 +2002,9 @@ /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + The macro name should not start with '_' for this warning. */ + if ((*cur != '_') && is_macro (pfile, cur)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) 2017-11-04 22:38 ` Mukesh Kapoor @ 2017-11-06 10:30 ` Paolo Carlini 0 siblings, 0 replies; 17+ messages in thread From: Paolo Carlini @ 2017-11-06 10:30 UTC (permalink / raw) To: Mukesh Kapoor, Jason Merrill; +Cc: Nathan Sidwell, gcc-patches List [-- Attachment #1: Type: text/plain, Size: 711 bytes --] Hi, On 04/11/2017 23:37, Mukesh Kapoor wrote: > I had included <inttypes.h> to get the definition of macro PRId64. I > have now modified the test case to remove all includes. I have added > the definition of the macro in the test case and also added > declarations of functions sprintf() strcmp(). I have attached the > revised patch. Thanks. Excellent. I'm now committing the patch with a couple of additional tweaks to the testcase: 1- Since you carefully constructed it to return zero at run time if we do the right thing, you want a 'dg-do run' not a 'dg-do compile'; 2- You also want a long ia64 variable, otherwise the testscase triggers a -Wformat warning. Thanks, Paolo. //////////////////// [-- Attachment #2: patch_80955_p --] [-- Type: text/plain, Size: 2507 bytes --] Index: gcc/testsuite/g++.dg/cpp0x/udlit-macros.C =================================================================== --- gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (nonexistent) +++ gcc/testsuite/g++.dg/cpp0x/udlit-macros.C (working copy) @@ -0,0 +1,31 @@ +// PR c++/80955 +// { dg-do run { target c++11 } } + +extern "C" int sprintf (char *s, const char *format, ...); +extern "C" int strcmp (const char *s1, const char *s2); + +#define __PRI64_PREFIX "l" +#define PRId64 __PRI64_PREFIX "d" + +using size_t = decltype(sizeof(0)); +#define _zero +#define _ID _xx +int operator""_zero(const char*, size_t) { return 0; } +int operator""_ID(const char*, size_t) { return 0; } + +int main() +{ + long i64 = 123; + char buf[100]; + sprintf(buf, "%"PRId64"abc", i64); // { dg-warning "invalid suffix on literal" } + return strcmp(buf, "123abc") + + ""_zero + + "bob"_zero + + R"#(raw + string)#"_zero + + "xx"_ID + + ""_ID + + R"AA(another + raw + string)AA"_ID; +} Index: libcpp/lex.c =================================================================== --- libcpp/lex.c (revision 254432) +++ libcpp/lex.c (working copy) @@ -1871,8 +1871,9 @@ lex_raw_string (cpp_reader *pfile, cpp_token *toke /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + The macro name should not start with '_' for this warning. */ + if ((*cur != '_') && is_macro (pfile, cur)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) @@ -2001,8 +2002,9 @@ lex_string (cpp_reader *pfile, cpp_token *token, c /* If a string format macro, say from inttypes.h, is placed touching a string literal it could be parsed as a C++11 user-defined string literal thus breaking the program. - Try to identify macros with is_macro. A warning is issued. */ - if (is_macro (pfile, cur)) + Try to identify macros with is_macro. A warning is issued. + The macro name should not start with '_' for this warning. */ + if ((*cur != '_') && is_macro (pfile, cur)) { /* Raise a warning, but do not consume subsequent tokens. */ if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping) ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-11-06 10:30 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-20 17:03 [C++ Patch] PR 80955 (Macros expanded in definition of user-defined literals) Mukesh Kapoor 2017-10-20 18:00 ` Nathan Sidwell 2017-10-20 18:19 ` Mukesh Kapoor 2017-10-20 20:56 ` Mukesh Kapoor 2017-10-24 14:07 ` Jason Merrill 2017-10-25 5:48 ` Mukesh Kapoor 2017-10-25 11:26 ` Nathan Sidwell 2017-10-25 16:13 ` Jason Merrill 2017-10-25 16:14 ` Mukesh Kapoor 2017-10-26 3:30 ` Mukesh Kapoor 2017-10-31 16:26 ` Mukesh Kapoor 2017-11-01 20:02 ` Jason Merrill 2017-11-01 20:44 ` Mukesh Kapoor 2017-11-02 14:42 ` Jason Merrill 2017-11-03 14:31 ` Paolo Carlini 2017-11-04 22:38 ` Mukesh Kapoor 2017-11-06 10:30 ` Paolo Carlini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).