[Bug c++/52654] New: [C++11] Warn on overflow in user-defined literals

[Bug c++/52654] New: [C++11] Warn on overflow in user-defined literals

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

             Bug #: 52654
           Summary: [C++11] Warn on overflow in user-defined literals
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: marc.glisse@normalesup.org


Hello,

should there be a warning for this kind of overflow? (-Wall -Wextra is
currently silent)

int operator"" _w(unsigned long long){return 0;}
int main(){
  return 12345678901234567890123456789012345678901234567890_w;
}

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |3dw4rd at verizon dot net

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-03-29 16:32:20 UTC ---
Let's add Ed in CC. In case, should be easy to add...

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #2 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-03-30 14:30:20 UTC ---
I short-circuited the overflow check in libcpp in case the literal was resolved
in C++ FE as a raw literal.  The raw literal should be able to take any number
of digits.  Ditto the literal operator template.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-03-30 15:08:58 UTC ---
I think it depends on what the literal operator is used.  If it is a const char
* or template, then it should allow any number of chars without a warning IMHO.
But when the C++ FE needs to parse that number as unsigned long long or long
double and the literal has too many digits for that, we should warn.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #4 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-03-30 17:34:36 UTC ---
Agreed.  Testing a patch.  This will have the advantage over libcpp that long
double will also be tested for overflow.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #5 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-03-31 16:57:12 UTC ---
Created attachment 27053
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27053
This test should give no warnings.

I think I'm going to have to put off interpreting numbers until the very end
inside the call resolution.  I think I should completely remove the numeric
value from the literal tree.

Sigh.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #6 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-03-31 17:06:12 UTC ---
Created attachment 27054
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27054
Test case for overflow warnings.

This test case should give the appropriate warnings.

Also, what about this:

 -3_w;

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #7 from Marc Glisse <marc.glisse at normalesup dot org> 2012-03-31 17:18:37 UTC ---
(In reply to comment #6)
> Also, what about this:
> 
>  -3_w;

What about it? IIUC, it is just -(3_w), I don't think it requires a particular
treatment.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #8 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-03-31 18:29:45 UTC ---
I think it's actually (-3)_w.  The tokenizer would pick the - up and pass -3
along.

The result of applying a literal operator may not be numeric at all i.e.
operator- would mean nothing.

I guess it would be taken care of however -3 would be for unsigned long long.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #9 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-04-05 19:30:41 UTC ---
Created attachment 27103
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27103
Patch including testcases - warn and only when necessary on literal overflow.

Pass a flag indicating overflow (or underflow) through the preprocessor and
lexer down to the C++ parser.  Only if a numeric argument literal operator is
found do we finally warn.  Otherwise block all warnings for over/under-flow of
user-defined literals in the preprocessor and lexer.

Waiting for a complete regtest.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #10 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-04-06 04:52:13 UTC ---
I made the warnings on by default.  Any opinion on whether I should have made
them depend on OPT_Woverflow instead?

I guess I thought the warning is slightly different in this case than the case
of just a regular numeric literal because the warning might cause the
programmer to examine a different type of operator as a solution - say template
or raw.

Still regtesting.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27103|0                           |1
           is patch|                            |
  Attachment #27103|application/octet-stream    |text/plain
          mime type|                            |

--- Comment #11 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-06 11:23:35 UTC ---
Comment on attachment 27103
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27103
Patch including testcases - warn and only when necessary on literal overflow.

Mark the patch as a patch.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-04-06
                 CC|                            |manu at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #12 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-06 11:45:43 UTC ---
(In reply to comment #10)
> I made the warnings on by default.  Any opinion on whether I should have made
> them depend on OPT_Woverflow instead?
> 
> I guess I thought the warning is slightly different in this case than the case
> of just a regular numeric literal because the warning might cause the
> programmer to examine a different type of operator as a solution - say template
> or raw.
> 

Some comments:

* Is overflow anything but -1, 0, and 1? If so, could you just make it an enum?
This will have the added benefit of not making calls like whatever(...,0,...)
in which it is very far from clear what 0 means. With the enum you can use
"NO_OVERFLOW". It may also save some space with respect to using 'int'.

* I don't get the reasoning for not using OPT_Woverflow. It is a warning about
overflow of numbers. Adding warnings that cannot be disabled by any option is
bad. The warning could be moved to a new different option later if people
clamor for it.

* Please use warning_at, even if you don't have a better location than
input_location. Diagnostic functions without location are DEPRECATED, but
nobody has bothered to do a full-conversion of the existing ones (I did with
pedwarn, inform and permerror, but there are too many warning/error calls).

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Ed Smith-Rowland <3dw4rd at verizon dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27054|0                           |1
        is obsolete|                            |
  Attachment #27103|0                           |1
        is obsolete|                            |

--- Comment #13 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-04-06 15:32:39 UTC ---
Created attachment 27105
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27105
New patch incorporating recent suggestions.

Using warning_at, OPT_Woverflow, etc.
Added a new enum for overflow type and used it everywhere.

regtested fine on x86_64-linux-gnu.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27105|0                           |1
           is patch|                            |
  Attachment #27105|application/octet-stream    |text/plain
          mime type|                            |

--- Comment #14 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-06 15:36:53 UTC ---
Comment on attachment 27105
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27105
New patch incorporating recent suggestions.

Set type to patch.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #15 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-06 16:40:42 UTC ---
(In reply to comment #13)
> Created attachment 27105 [details]
> New patch incorporating recent suggestions.
> 
> Using warning_at, OPT_Woverflow, etc.
> Added a new enum for overflow type and used it everywhere.
> 

Well, sorry if I am nitpicking, but the code would be clearer if the enum was
declared in real.h, used by real_from_string and everywhere else, so one would
not need to do "overflow < 0" but "overflow == UNDERFLOW" or something similar
like "UNDERFLOWED". And OT_NONE is not very easy to understand, because I don't
know what OT is the build_ call, I would suggest "NO_OVERFLOW". Or perhaps:

real.h:
/* Enumerate the types of overflow that may occur.  */
enum overflow_type {
  overflow_underflowed = -1,
  overflow_none,
  overflow_overflowed
};

GCC source code is already hard enough to read. Every little thing helps.
But this is merely a suggestion. You should send your patch to gcc-patches and
wait and see what the maintainers say. I don't have the power to approve (or
reject) patches.

Also, I may be misunderstanding how the current code works and what you are
trying to achieve, but it is strange to me that you parse the number first as a
number and then, if it is indeed converted to a number warn. So why don't just
parse the number as a string, and if it is indeed converted to a number, then
convert the string to a number and warn? That is, in c-lex.c build the userdef
before any interpret_ is called, and store the raw string. Do not try to
interpret it. Then, in cp_parser_userdef_numeric_literal, when you know what
type you are expecting, you can call interpret_* and do all the magic to decide
whether this is actually a number or a string. It seems to me that this will 
do the work in one place, instead of moving stuff up and down the parser and
the lexer. Or maybe you already tried this and it didn't work for some reason?

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #16 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-04-06 17:40:27 UTC ---
Thank you for your comments.

I was trying to follow the style of enum that I saw in the vicinity of the code
I was editing.  I was not able to discern a single style.  If lower-case is
more modern (I like it) then that's good for me.  I have no problem moving it
to real.h and using it all over either (I think just one return type).  I also
used the enum in interpret_integer though.  Maybe that's not a problem really.

Maybe I could go as far as changing cpp_number.overflow to use this enum as
well instead of a bool? - no real is not part of libcpp by design it seems.

As far as just storing a string and parsing it later you may be right.  Up to
now it was just convenient to keep the numeric values.  I tried to figure out
how to run interpret_integer, etc in parser but I got stuck trying to feed
cpp_token in parser in the c++ fe when all I could see was cp_token.  Is there
a way I can get the preprocessor token from the C++ token or is the former
stored somewhere?  If so I'll do that in a heartbeat.  OK, I'll try
parse_in->cut_token.

I thought about breaking up interpret_ into separate pieces that took strings
but that seemed like more trouble than it was worth.

Thank you for your comments.

Ed

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #17 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2012-04-06 18:38:51 UTC ---
(In reply to comment #16)
> Thank you for your comments.
> 
> I was trying to follow the style of enum that I saw in the vicinity of the code
> I was editing.  I was not able to discern a single style.  If lower-case is
> more modern (I like it) then that's good for me.  I have no problem moving it

Actually, I don't know what is the best style, I just looked at the other enum
defined in real.h

> to real.h and using it all over either (I think just one return type).  I also
> used the enum in interpret_integer though.  Maybe that's not a problem really.
> 
> Maybe I could go as far as changing cpp_number.overflow to use this enum as
> well instead of a bool? - no real is not part of libcpp by design it seems.

Well, libcpp seems not to care about the difference between underflow and
overflow, so why bother to modify it? Also real and libcpp are independent, so
this seems to me adding a dependence that is not needed.

> As far as just storing a string and parsing it later you may be right.  Up to
> now it was just convenient to keep the numeric values.  I tried to figure out
> how to run interpret_integer, etc in parser but I got stuck trying to feed
> cpp_token in parser in the c++ fe when all I could see was cp_token.  Is there
> a way I can get the preprocessor token from the C++ token or is the former
> stored somewhere?  If so I'll do that in a heartbeat.  OK, I'll try
> parse_in->cut_token.

Oh, I see the problem now. So I guess that what you did is the most
straightforward way then. Unless someone more knowledgeable proposes a nice way
to avoid passing this info around.

> I thought about breaking up interpret_ into separate pieces that took strings
> but that seemed like more trouble than it was worth.

Indeed. I see your point. It would be nice to be able to call these functions
from the C++ FE to avoid passing all this info around, and also avoid
interpreting the numbers if they are not actually numbers, and avoid checking
for user-def literals in CPP. 

Is parse_in still valid at the moment that cp_parser_userdef_ functions are
called? In fact, cpp_interpret_integer only needs a valid cpp_reader, it
doesn't use its token argument for anything but the string.

Again, I am not maintainer, so it would be nice if some maintainer expressed
their opinion.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Ed Smith-Rowland <3dw4rd at verizon dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at redhat dot com

--- Comment #18 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-06-06 15:03:16 UTC ---
Jason,

Could you check out the trail starting here and render an opinion.
I'll make sure the patch still works.
I wanted to get this in 4.7.1 but it'll have to wait until 4.7.2 - I just
didn't have time.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Thanks,
Ed

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[3dw4rd at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #19 from Ed Smith-Rowland <3dw4rd at verizon dot net> 2012-11-28 03:05:27 UTC ---
Created attachment 28814
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28814
Patch.

Here is a final patch for this.

Ultimately we should probably revisit this and 54413 by parsing the literal
numbers in the C++ front end.  That won't happen for 4.8.

This builds and passes on x86_64-linux.

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[emsr at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

--- Comment #20 from emsr at gcc dot gnu.org 2012-11-29 02:30:48 UTC ---
Author: emsr
Date: Thu Nov 29 02:30:44 2012
New Revision: 193918

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193918
Log:
gcc/c-family/

2012-11-29  Ed Smith-Rowland  <3dw4rd@verizon.net>

    PR c++/52654
    * c-common.h (overflow_type): New enum.
    (build_userdef_literal): Add overflow_type argument.
    (tree_userdef_literal): Add overflow_type.
    (USERDEF_LITERAL_OVERFLOW): New access macro.
    * c-common.c (build_userdef_literal): Add overflow_type
    argument.
    * c-lex.c (c_lex_with_flags): Add overflow_type to
    build_userdef_literal calls.
    (interpret_integer, interpret_float): Add overflow_type argument.


gcc/cp/

2012-11-29  Ed Smith-Rowland  <3dw4rd@verizon.net>

    PR c++/52654
    * parser.c (cp_parser_string_literal): Add overflow_type arg.
    (cp_parser_userdef_numeric_literal): Warn on numeric overflow.


gcc/testsuite/

2012-11-29  Ed Smith-Rowland  <3dw4rd@verizon.net>

    PR c++/52654
    * g++.dg/cpp0x/udlit-overflow.C: New.
    * g++.dg/cpp0x/udlit-overflow-neg.C: New.


Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/udlit-overflow-neg.C
    trunk/gcc/testsuite/g++.dg/cpp0x/udlit-overflow.C
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/c-family/c-lex.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/parser.c
    trunk/gcc/testsuite/ChangeLog

[Bug c++/52654] [C++11] Warn on overflow in user-defined literals

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52654

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.8.0

--- Comment #21 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-11-29 10:33:25 UTC ---
Fixed.