bugGNU Octave - Bugs: bug #62212, Issues when casting large integers...

 
 

bug #62212: Issues when casting large integers to floating point

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Wed 23 Mar 2022 02:25:47 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 7.0.92 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 30 Mar 2022 04:09:17 PM UTC, comment #19: 

I haven't checked the standard myself, but https://en.cppreference.com/w/cpp/language/types says:

Prior to C++20, the C++ Standard allowed any signed integer representation, and the minimum guaranteed range of N-bit signed integers was from −(2N−1−1) to +2N−1−1

(e.g. -127 to 127 for a signed 8-bit type), which corresponds to the limits of ones' complement or sign-and-magnitude.

However, all C++ compilers use two's complement representation, and as of C++20, it is the only representation allowed by the standard, with the guaranteed range from −2N−1
to +2N−1−1

(e.g. -128 to 127 for a signed 8-bit type).

8-bit ones' complement and sign-and-magnitude representations for char have been disallowed since C++11 (via CWG 1759), because a UTF-8 code unit of value 0x80 used in a UTF-8 string literal must be storable in a char element object.

John W. Eaton <jwe>
Group administrator
Wed 30 Mar 2022 02:49:06 PM UTC, comment #18: 

Thanks Arun for testing.
Also no new failures with our CI since this change.

Closing as fixed (minus the possible issues on exotic hardware described in comment #17).

Markus Mützel <mmuetzel>
Group administrator
Tue 29 Mar 2022 09:35:33 AM UTC, comment #17: 

Forgot to mention: There might still be places in the code base where floating point values are compared to int_min. That should be ok on platforms that use two's complement for negative integers (the vast majority afaict). They might need to be adapted if Octave runs on a platform that uses ones' complement for negative integers. (Are those even still around?)
Since all of our CI runs on systems with two's complement, the compiler is silent about those potential errors. (At least, I presume that is the reason.)
I only changed it where I "accidentally" stumbled across it.

We have checks for ones' complement in some locations. If someone cares, they are welcome to try and track down where we would need changes for those platforms. Those are likely locations where we cast int_min to a floating point.

Markus Mützel <mmuetzel>
Group administrator
Mon 28 Mar 2022 06:10:30 PM UTC, comment #16: 

Passes tests for me as before. Zero failures in make check with clang.

Arun Giridhar <arungiridhar>
Group Member
Mon 28 Mar 2022 05:55:00 PM UTC, comment #15: 

I pushed an updated changeset to the stable branch here:
https://hg.savannah.gnu.org/hgweb/octave/rev/014030798d5e

Afaict, a few of those changes avoid possible issues at corner cases.

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 09:03:59 PM UTC, comment #14: 

Your patch fixes the vast majority of the warnings. There are only 14 left like that, all in oct-inttypes.cc like you said.

Good to push to stable?

Arun Giridhar <arungiridhar>
Group Member
Wed 23 Mar 2022 07:22:03 PM UTC, comment #13: 

I'm attaching an updated patch with a short commit message.
There are still a few warnings of that category from oct-inttypes.cc. I haven't looked into those yet. But it's already getting late in Europe.

This is most likely only an issue with rare corner cases. So, there is probably no rush, and we could push something like this to the default branch.
On the other hand, we could handle this as a bug which can be fixed without breaking the API. So, it would also be ok to push on stable imho.

What do you think?



(file #53013)

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 06:28:50 PM UTC, comment #12: 

I agree. Just casting explicitly probably doesn't help a lot. We'll need to check how  those expressions are used and how to adapt them so they will do the "right thing".

I'm attaching a wip patch that makes it hopefully a little clearer how that could work.

(file #53012)

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 06:24:18 PM UTC, comment #11: 

If we want to eliminate the warnings, I think we should carefully consider each individually and not just apply casts to silence the warnings.

John W. Eaton <jwe>
Group administrator
Wed 23 Mar 2022 06:20:12 PM UTC, comment #10: 

For X > flintmax, I would expect X-1 == X.  Don't we need to account for eps(X) here?

I understand that my original assumption that X > intmax('UT') would work was wrong but I'm still not sure exactly how to perform this check "correctly".

John W. Eaton <jwe>
Group administrator
Wed 23 Mar 2022 06:12:09 PM UTC, comment #9: 

I'm currently looking into eliminating those warnings.
There are quite a few also in other files.

It looks like it doesn't warn for explicit casts.
But rearranging the code might make sense, too. (That might also help to avoid that the compiler "optimizes our fix away".)
I hope I can upload a patch soon.

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 06:06:17 PM UTC, comment #8: 

With that change, Clang gives more compiler warnings than before but it does build and pass all tests. The range test output:


>> intmin ("int64") : -2*double (intmin ("int64")) : intmax ("int64")
ans = -9223372036854775808


Arun Giridhar <arungiridhar>
Group Member
Wed 23 Mar 2022 05:30:23 PM UTC, comment #7: 

Next attempt of fixing this:
https://hg.savannah.gnu.org/hgweb/octave/rev/df26decca96b

If this works, maybe we could use similar expressions where clang warns that implicit conversions change values.

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 04:58:03 PM UTC, comment #6: 

Looks like it is undefined behavior:
https://en.cppreference.com/w/cpp/language/implicit_conversion

> Floating–integral conversions
> A prvalue of floating-point type can be converted to a prvalue of any integer type. The fractional part is truncated, that is, the fractional part is discarded. If the value cannot fit into the destination type, the behavior is undefined (even when the destination type is unsigned, modulo arithmetic does not apply).

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 04:32:51 PM UTC, comment #5: 

Sorry. Crossed with your post.
Is this really undefined behavior? I thought the standard was pretty clear when it comes to unsigned integer overflow. And I thought this is one of those cases of integer overflow. Maybe not...

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 04:30:00 PM UTC, comment #4: 

Hmm..
g++ only gets this exactly right when compiling without optimization (which might be the default on Ubuntu?):

$ g++ -O0 -otest_cast test_cast.cc
$ ./test_cast
db_large_integer: 1.84467e+19
ui64_large_integer: 0
$ g++ -O1 -otest_cast test_cast.cc
$ ./test_cast
db_large_integer: 1.84467e+19
ui64_large_integer: 18446744073709551615


... and the same for `-O2` and `-O3`. Maybe the default optimization level is different for your system.
Do we compile Octave with `-O0`?

clang++ gets it always wrong (but differently depending on optimization level):

$ clang++ -O0 -otest_cast test_cast.cc
$ ./test_cast
db_large_integer: 1.84467e+19
ui64_large_integer: 9223372036854775808
$ clang++ -O1 -otest_cast test_cast.cc
$ ./test_cast
db_large_integer: 1.84467e+19
ui64_large_integer: 4202531


... and the same for `-O2` and `-O3`.
All of those are too far off for the test case.

I wonder if there are tricks to avoid optimizations here or use different code paths when the compiler generates code.

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 04:18:37 PM UTC, comment #3: 

I can now reproduce your numerical results for GCC and Clang. Took some debugging but got there in the end.

With the sanitizer flags, the difference in behavior is more apparent:


$ g++ -fsanitize=address,undefined clangtest.cpp && ./a.out
db_large_integer: 1.84467e+19
ui64_large_integer: 0

$ clang++ -fsanitize=address,undefined clangtest.cpp && ./a.out
db_large_integer: 1.84467e+19
clangtest.cpp:8:33: runtime error: 1.84467e+19 is outside the range of representable values of type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clangtest.cpp:8:33 in
ui64_large_integer: 9223372036854775808


Also to note: GCC gives "ui64_large_integer: 18446744073709551615" for any optimization flag like -O1, -O2 and -O3, anything other than -O0. With any of those flags, Clang gives the junk value near 2^46 which moreover changes with every execution.

Arun Giridhar <arungiridhar>
Group Member
Wed 23 Mar 2022 03:56:16 PM UTC, comment #2: 

I'm very confused by the minimum reproducer you posted. I named it clangtest.cpp, and look at what I get:


$ echo $CFLAGS $CXXFLAGS $FFLAGS

$ g++ --version
g++ (GCC) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ g++ clangtest.cpp && ./a.out
db_large_integer: 1.84467e+19
ui64_large_integer: 18446744073709551615

$ clang++ --version
clang version 13.0.1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ clang++ clangtest.cpp && ./a.out
db_large_integer: 1.84467e+19
ui64_large_integer: 93882097565731


For me, GCC 11.2 returns 18446744073709551615 not 0, and Clang 13.0.1 returns the completely inexplicable 93882097565731 which is not even the correct order of magnitude! It is only some 2^46 or 2^47, not even close to 2^64.

What am I doing wrong?

Arun Giridhar <arungiridhar>
Group Member
Wed 23 Mar 2022 02:37:18 PM UTC, comment #1: 

I tagged the tests that are failing with clang 13 here:
https://hg.savannah.gnu.org/hgweb/octave/rev/9b3b0dbb4eba

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Mar 2022 02:25:47 PM UTC, original submission:  

I mainly open this report to get a bug number to tag failing tests with clang (at least version 13).

This can lead to incorrect range operator results for corner case inputs. Other places in the code might also be affected.

"Minimal" reproducer:

#include <cmath>
#include <iostream>

int main (void)
{
  double db_large_integer = std::pow (2., 64.);
  std::cout << "db_large_integer: " << db_large_integer << std::endl;
  uint64_t ui64_large_integer = db_large_integer;
  std::cout << "ui64_large_integer: " << ui64_large_integer << std::endl;
  return 0;
}


With Ubuntu 21.10:

$ g++ --version
g++ (Ubuntu 11.2.0-7ubuntu2) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ g++ -otest_cast test_cast.cc
$ ./test_cast
db_large_integer: 1.84467e+19
ui64_large_integer: 0
$ clang++ --version
Ubuntu clang version 13.0.0-2
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ clang++ -otest_cast test_cast.cc
$ ./test_cast
db_large_integer: 1.84467e+19
ui64_large_integer: 9223372036854775808


IIUC, the overflow behavior of unsigned integers is defined in the standard. Afaict, gcc gets the correct answer, clang doesn’t.

Markus Mützel <mmuetzel>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-03-30 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-03-28 mmuetzel StatusIn Progress Ready For Test
        Releasedev 7.0.92
        SummaryWrong unsigned integer overflow with clang Issues when casting large integers to floating point
    2022-03-23 mmuetzel Attached File- Added bug62212_large_int_to_float-v2.patch, #53013
        CategoryNone Interpreter
        StatusNone In Progress
    2022-03-23 mmuetzel Attached File- Added bug62212_large_int_to_float.patch, #53012

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code