Bug 90920 - [9 Regression] ABI incompatibility in std::rotate
Summary: [9 Regression] ABI incompatibility in std::rotate
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 9.1.0
: P2 normal
Target Milestone: 9.2
Assignee: Jonathan Wakely
URL:
Keywords: ABI
Depends on:
Blocks:
 
Reported: 2019-06-18 19:43 UTC by Jonathan Wakely
Modified: 2019-07-13 02:04 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 10.0, 8.3.0
Known to fail: 9.1.0
Last reconfirmed: 2019-06-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2019-06-18 19:43:49 UTC
r263433 moved some checking from std::__rotate to its caller, std::rotate. It's no longer safe to combine code compiled before and after that change:

tmp$ cat rot1.cc
#include <algorithm>

namespace std {
// explicit instantiation definition to simulate the linker choosing the
// definition from this file.
template int* __rotate(int*, int*, int*, random_access_iterator_tag);
}
tmp$ cat rot2.cc
#include <algorithm>

namespace std {
// explicit instantiation definition to simulate the linker choosing a
// definition from another file.
extern template int* __rotate(int*, int*, int*, random_access_iterator_tag);
}
int main()
{
  int i = 0;
  std::rotate(&i, &i, &i+1);
}
tmp$ /home/jwakely/gcc/9.1.0/bin/g++ -c rot1.cc
tmp$ /home/jwakely/gcc/8.3.0/bin/g++ -c rot2.cc
tmp$ /home/jwakely/gcc/9.1.0/bin/g++ rot1.o rot2.o
tmp$ ./a.out
Floating point exception (core dumped)
Comment 1 Jonathan Wakely 2019-06-18 19:52:17 UTC
The problem case happens when std::rotate is compiled with the old headers and std::__rotate is compiled with the new code, and so neither function checks for an empty range.

Reverting the patch is easy, but that doesn't help code already compiled with GCC 9.1, which has non-checking instantiations of std::__rotate.
Comment 2 Jonathan Wakely 2019-06-19 22:57:33 UTC
Author: redi
Date: Wed Jun 19 22:57:02 2019
New Revision: 272489

URL: https://gcc.gnu.org/viewcvs?rev=272489&root=gcc&view=rev
Log:
PR libstdc++/90920 restore previous checks for empty ranges

The change in r263433 broke the contract of the __rotate functions, by no
longer accepting empty ranges. That means that callers which inlined the
old version of std::rotate (without checks) that end up linking to a new
definition of std::__rotate (also without checks) could perform a divide
by zero and crash.

This restores the old contract of the __rotate overloads.

	PR libstdc++/90920 partially revert r263433
	* include/bits/stl_algo.h (__rotate): Restore checks for empty ranges.
	(rotate): Remove checks.
	* testsuite/25_algorithms/rotate/90920.cc: New test.

Added:
    trunk/libstdc++-v3/testsuite/25_algorithms/rotate/90920.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/stl_algo.h
Comment 3 Richard Biener 2019-06-20 16:35:12 UTC
Fixed on trunk.  I think I rather prefer the one-off incompatibility in 9.1.
Unless you can think of a solution not breaking compatibility with 9.1 of course.
Comment 4 Jonathan Wakely 2019-06-20 16:42:10 UTC
I did experiment with putting the range checks in *both* places, the std::rotate function and the std::__rotate helpers it calls. But there's still no guarantee you won't get a "bad" combination  of definitions from objects built with e.g. 9.1 and 8.3

If we change the mangled names to make the "new" functions different that doesn't help, because the "old" functions will still be present in already-built objects.

Basically nothing we can do will fix the code in already compiled objects. The only guaranteed way to avoid the problem is to recompile anything built with 9.1, and if you have to do that anyway, then the fix I put on trunk works fine.

So I think I'll just backport the same fix, and 9.1 will be a blip.
Comment 5 rguenther@suse.de 2019-06-21 06:56:08 UTC
On June 20, 2019 6:42:10 PM GMT+02:00, "redi at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90920
>
>--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
>I did experiment with putting the range checks in *both* places, the
>std::rotate function and the std::__rotate helpers it calls. But
>there's still
>no guarantee you won't get a "bad" combination  of definitions from
>objects
>built with e.g. 9.1 and 8.3
>
>If we change the mangled names to make the "new" functions different
>that
>doesn't help, because the "old" functions will still be present in
>already-built objects.
>
>Basically nothing we can do will fix the code in already compiled
>objects. The
>only guaranteed way to avoid the problem is to recompile anything built
>with
>9.1, and if you have to do that anyway, then the fix I put on trunk
>works fine.
>
>So I think I'll just backport the same fix, and 9.1 will be a blip.

Sounds good. Please document this on changes. Html in some prominent place (caveats section).
Comment 6 Jonathan Wakely 2019-06-21 17:37:38 UTC
Author: redi
Date: Fri Jun 21 17:37:07 2019
New Revision: 272558

URL: https://gcc.gnu.org/viewcvs?rev=272558&root=gcc&view=rev
Log:
PR libstdc++/90920 restore previous checks for empty ranges

The change in r263433 broke the contract of the __rotate functions, by no
longer accepting empty ranges. That means that callers which inlined the
old version of std::rotate (without checks) that end up linking to a new
definition of std::__rotate (also without checks) could perform a divide
by zero and crash.

This restores the old contract of the __rotate overloads.

Backport from mainline
2019-06-19  Jonathan Wakely  <jwakely@redhat.com>

	PR libstdc++/90920 partially revert r263433
	* include/bits/stl_algo.h (__rotate): Restore checks for empty ranges.
	(rotate): Remove checks.
	* testsuite/25_algorithms/rotate/90920.cc: New test.

Added:
    branches/gcc-9-branch/libstdc++-v3/testsuite/25_algorithms/rotate/90920.cc
Modified:
    branches/gcc-9-branch/libstdc++-v3/ChangeLog
    branches/gcc-9-branch/libstdc++-v3/include/bits/stl_algo.h
Comment 7 Jonathan Wakely 2019-06-21 18:48:20 UTC
Fixed for 9.2
Comment 8 Jonathan Wakely 2019-06-21 18:48:37 UTC
.