Skip to content

Commit

Permalink
See Issue Morwenn#139
Browse files Browse the repository at this point in the history
Okay, so I sat with it first (before just bisecting, which will likely find that problem is gone with 2ab06c1). For many hours.

There's bad news. The problem seems traceable to the indirection projection. This is an immutable lambda, but still it ostensibly changes during execution (as part of `project_compare`).

I really can't decide yet whether that's a miscompilation, or I'm missing some source of UB lurking there.

I added a unit test `issue_139` on branch `issue_139`. The unit test does not repro the ASAN diagnostic, but it *does* manifest the source of the UB, where the captured `&proj` reference (originally include/cpp-sort/adapters/indirect_adapter.h:L123) gets clobbered.

The good news is you can see this happening. The bad news is it's not just with Clang6. ((In fact with GCC-10 + ASAN it actually reports _AddressSanitizer: stack-buffer-overflow_).

What follows is some of the thinking/observations that went into making the repro:

-----

- I figured it might have been down to lambda codegen, but rewriting it as a callable struct made no difference. (Omitted from the unit test for brevity).

- Next I figured it might be due to some weird codegen issue with empty-element `tuple<>` optimization in `projection_compare` (because replacing the tuple by it's elements **did** remove the symptom).

  But sketching an "adhoc" projection_compare in terms of `tuple` does **not** display the same symptom. [Included in the repro code in case you want to check]

Among the weirder observations (if that's not enough for you)

 - depending on how I sequence the tests, the first invocation of `projection_compare::compare()` does the right thing (`&proj == projptr`), but a second invocation fails.
 - depending on whether I phrase taking the address of `proj` as `&proj` or `std::addressof(proj)` can give different diagnostics some of the time: at times ASAN croaked about invoking a member on a null pointer when using `std::addressof`. ¯\\_(ツ)_/¯
 -  It's weird that this manifested with std_sort only. Then again, the repro case fails with other compilers/clang versions just as well, even though that never tripped ASAN before.

I fail to see something wrong with the actual code. Could it really be just... miscompilation? Pretty scary. Maybe I'm just going crazy and the error is right before our eyes. I'd appreciate a second pair of eyes. Meanwhile, I'm sleeping on it. Maybe tomorrow brings fresh ideas.

-----

Apparent workarounds:

 - capturing `proj` by value
 - replacing `tuple` projection_compare::data by its parts works
 - perhaps it is gone with 2ab06c1 (I still wish to understand the phenomenon I'm seeing)
  • Loading branch information
sehe committed Mar 22, 2021
1 parent 2b9bb7e commit fbff728
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 0 deletions.
1 change: 1 addition & 0 deletions testsuite/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ set(
utility/buffer.cpp
utility/function_constant.cpp
utility/iter_swap.cpp
utility/issue_139_repro.cpp
)

# Make one executable for the whole testsuite
Expand Down
102 changes: 102 additions & 0 deletions testsuite/utility/issue_139_repro.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* The MIT License (MIT)
*
* Copyright (c) 2016-2018 Morwenn
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
#include <catch2/catch.hpp>
#include <cpp-sort/utility/as_function.h>
#include <cpp-sort/utility/functional.h>
#include <cpp-sort/detail/projection_compare.h>

TEST_CASE( "Issue 139",
"[issue_139][projection_compare]" )
{
// minimal fixture
namespace detail = cppsort::detail;
namespace utility = cppsort::utility;

utility::identity proj;
//// same with:
// auto proj = utility::as_function(utility::identity{});

using Iterator = double const*;
double value = 0;
Iterator it = &value;

SECTION( "define iterproj based on proj" )
{
/*
* Projection taken from indirect_adapter:
*
* return Sorter{}(
* std::begin(iterators), std::end(iterators), std::move(compare),
* [&proj](RandomAccessIterator it) -> decltype(auto) { return proj(*it); });
*
* We add a `paranoia-check` projptr to check whether the reference (or
* pointer) mutates
*/

auto iterproj =
[&proj, projptr = std::addressof(proj)](Iterator it) -> decltype(auto) {

// Manifests the WTF
CHECK(std::addressof(proj) == projptr);

return 42; // (*projptr)(*it); not even required
};

SECTION( "demo iterproj is sane" ) {
iterproj(it);
}

SECTION( "copy of iterproj is sane" ) {
auto copy = iterproj;
copy(it);
}

SECTION( "using projection_compare instead" ) {
detail::projection_compare<std::less<>, decltype(iterproj)> prjcomp(
{}, iterproj);

prjcomp.projection()(it);
prjcomp.projection()(it);

SECTION( "iterproj still sane" ) { iterproj(it); }
}

SECTION(" adhoc approach ") {
// This tried to reduce projection_compare to its suspected
// culprit, but is actually behaving just fine
auto adhoc =
[data =
std::tuple<decltype(utility::as_function(std::less<>{})),
decltype(utility::as_function(iterproj))>(
std::less<>{}, iterproj)](Iterator a, Iterator b) {
return std::get<0>(data)(std::get<1>(data)(a),
std::get<1>(data)(b));
};

CHECK(not adhoc(it, it)); // invokes iterproj twice

SECTION(" iterproj still sane ") { iterproj(it); }
}
}
}

0 comments on commit fbff728

Please sign in to comment.