Skip to content

Commit 800752b

Browse files
Fix the double forwarding bug in function fusion (#168)
Function fusion technically runs std::forward on the same arguments multiple times, in theory this could mean that the second time a function gets those arguments they could have been moved away. This one was interesting as I realised that I wasn't even forwarding the arguments all the way down. At the last level they were left as lvalue. After fixing this issue the actual bug could be fixed so that shared arguments will always be passed through as lvalues, while unique variables will only ever be sent to a single function and thus, forwarding them multiple times is fine as only one function will ever "see" the final value.
1 parent 9d60bd4 commit 800752b

7 files changed

Lines changed: 349 additions & 47 deletions

File tree

‎src/PowerPlant.hpp‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -311,24 +311,6 @@ class PowerPlant {
311311
FusionFunction::call(*this, data, std::forward<Arguments>(args)...);
312312
}
313313

314-
template <template <typename> class First, template <typename> class... Remainder, typename... Arguments>
315-
void emit_shared(Arguments&&... args) {
316-
317-
using Functions = std::tuple<First<void>, Remainder<void>...>;
318-
using ArgumentPack = decltype(std::forward_as_tuple(*this, std::forward<Arguments>(args)...));
319-
using CallerArgs = std::tuple<>;
320-
using FusionFunction = util::FunctionFusion<Functions, ArgumentPack, EmitCaller, CallerArgs, 1>;
321-
322-
// Provide a check to make sure they are passing us the right stuff
323-
static_assert(
324-
FusionFunction::value,
325-
"There was an error with the arguments for the emit function, Check that your scope and arguments "
326-
"match what you are trying to do.");
327-
328-
// Fuse our emit handlers and call the fused function
329-
FusionFunction::call(*this, std::forward<Arguments>(args)...);
330-
}
331-
332314
/// Our TaskScheduler that handles distributing task to the pool threads
333315
threading::scheduler::Scheduler scheduler;
334316
/// Our vector of Reactors, will get destructed when this vector is

‎src/Reactor.hpp‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,9 @@ namespace dsl {
135135
template <typename WatchdogGroup, typename RuntimeType>
136136
struct WatchdogServicer;
137137
template <typename WatchdogGroup, typename RuntimeType>
138-
WatchdogServicer<WatchdogGroup, RuntimeType> ServiceWatchdog(RuntimeType&& data);
138+
std::unique_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>> ServiceWatchdog(RuntimeType&& data);
139139
template <typename WatchdogGroup>
140-
WatchdogServicer<WatchdogGroup, void> ServiceWatchdog();
140+
std::unique_ptr<WatchdogServicer<WatchdogGroup, void>> ServiceWatchdog();
141141
} // namespace emit
142142
} // namespace word
143143
} // namespace dsl
@@ -272,10 +272,7 @@ class Reactor {
272272

273273
/// @copydoc dsl::word::emit::ServiceWatchdog
274274
template <typename WatchdogGroup, typename... Arguments>
275-
auto ServiceWatchdog(Arguments&&... args)
276-
// THIS IS VERY IMPORTANT, the return type must be dependent on the function call
277-
// otherwise it won't check it's valid in SFINAE
278-
-> decltype(dsl::word::emit::ServiceWatchdog<WatchdogGroup>(std::forward<Arguments>(args)...)) {
275+
auto ServiceWatchdog(Arguments&&... args) {
279276
return dsl::word::emit::ServiceWatchdog<WatchdogGroup>(std::forward<Arguments>(args)...);
280277
}
281278

@@ -368,7 +365,7 @@ class Reactor {
368365
util::CallbackGenerator<DSL, Function>(std::forward<Function>(callback)));
369366

370367
// Get our tuple from binding our reaction
371-
auto tuple = DSL::bind(reaction, std::get<Index>(args)...);
368+
auto tuple = DSL::bind(reaction, std::move(std::get<Index>(args))...);
372369

373370
auto handle = threading::ReactionHandle(reaction);
374371
reactor.reaction_handles.push_back(handle);
@@ -379,7 +376,8 @@ class Reactor {
379376
}
380377

381378
public:
382-
Binder(Reactor& r, Arguments&&... args) : reactor(r), args(args...) {}
379+
template <typename... Args>
380+
Binder(Reactor& r, Args&&... args) : reactor(r), args(std::forward<Args>(args)...) {}
383381

384382
template <typename Label, typename Function>
385383
auto then(Label&& label, Function&& callback) {

‎src/dsl/word/emit/Watchdog.hpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,8 @@ namespace dsl {
125125
* @return A WatchdogServicer object which will update the service time of the specified watchdog
126126
*/
127127
template <typename WatchdogGroup, typename RuntimeType>
128-
WatchdogServicer<WatchdogGroup, RuntimeType> ServiceWatchdog(RuntimeType&& data) {
129-
return WatchdogServicer<WatchdogGroup, RuntimeType>(std::forward<RuntimeType>(data));
128+
std::unique_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>> ServiceWatchdog(RuntimeType&& data) {
129+
return std::make_unique<WatchdogServicer<WatchdogGroup, RuntimeType>>(std::forward<RuntimeType>(data));
130130
}
131131

132132
/**
@@ -137,8 +137,8 @@ namespace dsl {
137137
* @return WatchdogServicer<WatchdogGroup, void>
138138
*/
139139
template <typename WatchdogGroup>
140-
WatchdogServicer<WatchdogGroup, void> ServiceWatchdog() {
141-
return WatchdogServicer<WatchdogGroup, void>();
140+
std::unique_ptr<WatchdogServicer<WatchdogGroup, void>> ServiceWatchdog() {
141+
return std::make_unique<WatchdogServicer<WatchdogGroup, void>>();
142142
}
143143

144144
/**
@@ -158,9 +158,9 @@ namespace dsl {
158158

159159
template <typename WatchdogGroup, typename RuntimeType>
160160
static void emit(const PowerPlant& /*powerplant*/,
161-
WatchdogServicer<WatchdogGroup, RuntimeType>& servicer) {
161+
const std::shared_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>>& servicer) {
162162
// Update our service time
163-
servicer.service();
163+
servicer->service();
164164
}
165165
};
166166

‎src/util/FunctionFusion.hpp‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@
2020
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
2121
*/
2222

23-
#ifndef NUCLEAR_UTIL_FUNCTIONFUSION_HPP
24-
#define NUCLEAR_UTIL_FUNCTIONFUSION_HPP
23+
#ifndef NUCLEAR_UTIL_FUNCTION_FUSION_HPP
24+
#define NUCLEAR_UTIL_FUNCTION_FUSION_HPP
2525

2626
#include "Sequence.hpp"
2727
#include "tuplify.hpp"
@@ -51,8 +51,16 @@ namespace util {
5151
auto apply_function_fusion_call(const std::tuple<Arguments...>& args,
5252
const Sequence<Shared...>& /*shared*/,
5353
const Sequence<Selected...>& /*selected*/)
54-
-> decltype(Function::call(std::get<Shared>(args)..., std::get<Selected>(args)...)) {
55-
return Function::call(std::get<Shared>(args)..., std::get<Selected>(args)...);
54+
-> decltype(Function::call(
55+
std::get<Shared>(args)...,
56+
static_cast<std::tuple_element_t<Selected, std::tuple<Arguments...>>>(std::get<Selected>(args))...)) {
57+
58+
return Function::call(
59+
// By not moving/forwarding the shared arguments they will always end up as lvalues even if they were
60+
// rvalues originally. That allows them to be used in multiple function calls without the first one being
61+
// able to steal them via a move constructor etc
62+
std::get<Shared>(args)...,
63+
static_cast<std::tuple_element_t<Selected, std::tuple<Arguments...>>>(std::get<Selected>(args))...);
5664
}
5765

5866
/**
@@ -138,13 +146,14 @@ namespace util {
138146
*
139147
* @return the result of calling this specific function
140148
*/
141-
template <typename Function, int Start, int End>
149+
template <typename Function, int Start, int End, typename... Args>
142150
// It is forwarded as a tuple
143-
// NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved)
144-
static auto call_one(const Sequence<Start, End>& /*e*/, Arguments&&... args)
145-
-> decltype(apply_function_fusion_call<Function, Shared, Start, End>(std::forward_as_tuple(args...))) {
151+
static auto call_one(const Sequence<Start, End>& /*e*/, Args&&... args)
152+
-> decltype(apply_function_fusion_call<Function, Shared, Start, End>(
153+
std::forward_as_tuple(std::forward<Args>(args)...))) {
146154

147-
return apply_function_fusion_call<Function, Shared, Start, End>(std::forward_as_tuple(args...));
155+
return apply_function_fusion_call<Function, Shared, Start, End>(
156+
std::forward_as_tuple(std::forward<Args>(args)...));
148157
}
149158

150159
/**
@@ -177,12 +186,14 @@ namespace util {
177186
NextStep::call(std::forward<Args>(args)...))) {
178187

179188
// Call each on a separate line to preserve order of execution
180-
// TODO(thouliston) this is a legitimate bug, fix it in a future PR and add a test to ensure it doesn't
181-
// regress
182-
// NOLINTNEXTLINE(bugprone-use-after-move)
183-
auto current = tuplify(call_one<CurrentFunction>(CurrentRange(), std::forward<Args>(args)...));
189+
// The std::forwards here are fine even though they are passed to multiple functions.
190+
// As part of function fusion before, all of the shared arguments are enforced to be lvalues and only the
191+
// selected arguments will be potentially rvalues. These rvalues are then only passed to a single target
192+
// function, even if they are forwarded multiple times to the functions which select the right arguments.
193+
// As shown in the moving test, casting to an rvalue, but then not using it won't change the result.
184194
// NOLINTNEXTLINE(bugprone-use-after-move)
185-
auto remainder = NextStep::call(std::forward<Args>(args)...);
195+
auto current = tuplify(call_one<CurrentFunction>(CurrentRange(), std::forward<Args>(args)...));
196+
auto remainder = NextStep::call(std::forward<Args>(args)...); // NOLINT(bugprone-use-after-move)
186197

187198
return std::tuple_cat(std::move(current), std::move(remainder));
188199
}
@@ -365,4 +376,4 @@ namespace util {
365376
} // namespace util
366377
} // namespace NUClear
367378

368-
#endif // NUCLEAR_UTIL_FUNCTIONFUSION_HPP
379+
#endif // NUCLEAR_UTIL_FUNCTION_FUSION_HPP

‎src/util/Sequence.hpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
#ifndef NUCLEAR_UTIL_SEQUENCE_HPP
2424
#define NUCLEAR_UTIL_SEQUENCE_HPP
2525

26+
#include <type_traits>
27+
2628
namespace NUClear {
2729
namespace util {
2830

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* MIT License
3+
*
4+
* Copyright (c) 2022 NUClear Contributors
5+
*
6+
* This file is part of the NUClear codebase.
7+
* See https://github.com/Fastcode/NUClear for further info.
8+
*
9+
* Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
10+
* documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
11+
* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to
12+
* permit persons to whom the Software is furnished to do so, subject to the following conditions:
13+
*
14+
* The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
15+
* Software.
16+
*
17+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
18+
* WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
19+
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
20+
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
*/
22+
23+
#include "util/FunctionFusion.hpp"
24+
25+
#include <catch2/catch_test_macros.hpp>
26+
#include <tuple>
27+
#include <utility>
28+
#include <vector>
29+
30+
31+
// There are several linting rules from clang-tidy that will trigger in this code.
32+
// However as the point of these tests is to show what the behaviour is in these situations they are suppressed for this
33+
// file
34+
// NOLINTBEGIN(bugprone-use-after-move)
35+
36+
namespace { // Make everything here internal linkage
37+
38+
/**
39+
* This struct is used to test what is passed from FunctionFusion to the call function.
40+
*
41+
* It has four different versions of append to see which one is called
42+
* The ones that take rvalue references will move the arguments into new temporaries to ensure the original arguments
43+
* will be empty after the function runs.
44+
*/
45+
struct Appender {
46+
47+
/// rvalue/rvalue
48+
static std::vector<char> append(std::vector<char>&& x, std::vector<char>&& y) {
49+
const std::vector<char> x1 = std::move(x);
50+
const std::vector<char> y1 = std::move(y);
51+
52+
std::vector<char> out;
53+
out.push_back('r');
54+
out.push_back('r');
55+
out.insert(out.end(), x1.begin(), x1.end()); // HERE
56+
out.insert(out.end(), y1.begin(), y1.end());
57+
58+
return out;
59+
}
60+
61+
/// rvalue/lvalue
62+
static std::vector<char> append(std::vector<char>&& x, const std::vector<char>& y) {
63+
const std::vector<char> x1 = std::move(x);
64+
const auto& y1 = y;
65+
66+
std::vector<char> out;
67+
out.push_back('r');
68+
out.push_back('l');
69+
out.insert(out.end(), x1.begin(), x1.end());
70+
out.insert(out.end(), y1.begin(), y1.end());
71+
72+
return out;
73+
}
74+
75+
/// lvalue/rvalue
76+
static std::vector<char> append(const std::vector<char>& x, std::vector<char>&& y) {
77+
const auto& x1 = x;
78+
const std::vector<char> y1 = std::move(y);
79+
80+
std::vector<char> out;
81+
out.push_back('l');
82+
out.push_back('r');
83+
out.insert(out.end(), x1.begin(), x1.end()); // HERE
84+
out.insert(out.end(), y1.begin(), y1.end());
85+
86+
return out;
87+
}
88+
89+
/// lvalue/lvalue
90+
static std::vector<char> append(const std::vector<char>& x, const std::vector<char>& y) {
91+
const auto& x1 = x;
92+
const auto& y1 = y;
93+
94+
std::vector<char> out;
95+
out.push_back('l');
96+
out.push_back('l');
97+
out.insert(out.end(), x.begin(), x.end());
98+
out.insert(out.end(), y.begin(), y.end());
99+
100+
return out;
101+
}
102+
};
103+
104+
template <typename T>
105+
struct AppendCaller {
106+
template <typename... Args>
107+
static auto call(Args&&... args) -> decltype(T::append(std::forward<Args>(args)...)) {
108+
return T::append(std::forward<Args>(args)...);
109+
}
110+
};
111+
112+
template <int Shared, typename... Args>
113+
auto do_fusion(Args&&... args) {
114+
return NUClear::util::FunctionFusion<std::tuple<Appender, Appender>,
115+
decltype(std::forward_as_tuple(std::forward<Args>(args)...)),
116+
AppendCaller,
117+
std::tuple<>,
118+
Shared>::call(std::forward<Args>(args)...);
119+
}
120+
121+
} // namespace
122+
123+
SCENARIO("Shared arguments to FunctionFusion are not moved", "[util][FunctionFusion][shared][move]") {
124+
125+
WHEN("calling append with 1 shared and 1 selected arguments") {
126+
std::vector<char> shared({'s'});
127+
std::vector<char> arg1({'1'});
128+
std::vector<char> arg2({'2'});
129+
130+
auto result = do_fusion<1>(std::move(shared), std::move(arg1), std::move(arg2));
131+
132+
const auto& r1 = std::get<0>(result);
133+
const auto& r2 = std::get<1>(result);
134+
135+
THEN("the results are correct") {
136+
CHECK(r1 == std::vector<char>({'l', 'r', 's', '1'}));
137+
CHECK(r2 == std::vector<char>({'l', 'r', 's', '2'}));
138+
}
139+
THEN("the shared argument is not moved") {
140+
CHECK(shared == std::vector<char>({'s'}));
141+
}
142+
THEN("the selected arguments are moved") {
143+
CHECK(arg1.empty());
144+
CHECK(arg2.empty());
145+
}
146+
}
147+
148+
WHEN("calling append with 0 shared and 2 selected arguments") {
149+
std::vector<char> arg1({'1'});
150+
std::vector<char> arg2({'2'});
151+
std::vector<char> arg3({'3'});
152+
std::vector<char> arg4({'4'});
153+
154+
auto result = do_fusion<0>(std::move(arg1), std::move(arg2), std::move(arg3), std::move(arg4));
155+
156+
const auto& r1 = std::get<0>(result);
157+
const auto& r2 = std::get<1>(result);
158+
159+
THEN("the results are correct") {
160+
CHECK(r1 == std::vector<char>({'r', 'r', '1', '2'}));
161+
CHECK(r2 == std::vector<char>({'r', 'r', '3', '4'}));
162+
}
163+
THEN("the selected arguments are moved") {
164+
CHECK(arg1.empty());
165+
CHECK(arg2.empty());
166+
CHECK(arg3.empty());
167+
CHECK(arg4.empty());
168+
}
169+
}
170+
}
171+
172+
// NOLINTEND(bugprone-use-after-move)

0 commit comments

Comments
 (0)