{"id":106182,"date":"2022-01-21T07:00:20","date_gmt":"2022-01-21T15:00:20","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/oldnewthing\/?p=106182"},"modified":"2022-01-21T06:30:34","modified_gmt":"2022-01-21T14:30:34","slug":"fixing-the-crash-that-seems-to-be-on-a-stdmove-operation","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20220121-20\/?p=106182","title":{"rendered":"Fixing the crash that seems to be on a std::move<\/CODE> operation"},"content":{"rendered":"

Last time, we looked at a crash that was root-caused to an order of evaluation bug if compiled as C++14<\/a>. One solution to the problem is to switch to C++17 mode, but presumably the customer isn’t willing to make that drastic a change to their product yet. Maybe there’s something we can do that is less disruptive.<\/p>\n

    \/\/ std::shared_ptr<Test> test;\r\n    test<\/span>->harness->callAndReport([test2 = std::move(test)<\/span>]() ...);\r\n<\/pre>\n
The problem here is that we move the pointer out of the test<\/code> when building the lambda, while at the same time fetching the pointer from the test<\/code> in order to locate the function to call. In C++14, these two operations are not sequenced, so there is no guarantee on the order of evaluation, even though our code depends on it:<\/p>\n\n\n\n\n\n\n\n\n\n\n\n\n\n
test<\/code><\/td>\n\u2190 need this to happen<\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n <\/td>\n<\/tr>\n
operator-><\/code><\/td>\n <\/td>\n <\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n <\/td>\n<\/tr>\n
harness<\/code><\/td>\n <\/td>\nbefore this happens<\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
operator-><\/code><\/td>\n <\/td>\ntest2 = std::move(test)<\/code><\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
callAndReport<\/code><\/td>\n <\/td>\nlambda constructed<\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
function call<\/td>\n<\/tr>\n<\/tbody>\n<\/table>\n
We’ll have to break the large expression (with unspecified order of evaluation) into two expressions that force the desired order of evaluation.<\/p>\n
We need the retrieval of the object referenced by test<\/code> to happen before the evaluation of test2 = std::move(test)<\/code>. So let’s write it out explicitly in the order we want the operations to happen, as two separate statements to enforce sequencing.<\/p>\n
There are a few ways of doing this, depending on where we want to break the chain in the above diagram.<\/p>\n
We could break it as soon as possible:<\/p>\n
    auto& original_test = *test; \/\/ get this before we std::move(test)\r\n    original_test.<\/span>harness->callAndReport([test2 = std::move(test)]() ...);\r\n<\/pre>\n
An equivalent, perhaps less weird version, is this:<\/p>\n
    auto test_ptr = test.get(); \/\/ get this before we std::move(test)\r\n    test_ptr<\/span>->harness->callAndReport([test2 = std::move(test)]() ...);\r\n<\/pre>\n
Both of these force the sequencing of the “figure out what test<\/code> points to” to happen before the rest of the expression:<\/p>\n\n\n\n\n\n\n\n\n\n\n\n\n\n
test<\/code><\/td>\n <\/td>\n <\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n <\/td>\n<\/tr>\n
operator-><\/code><\/td>\n <\/td>\n<\/tr>\n
\n
\n<\/td>\n<\/tr>\n
harness<\/code><\/td>\n <\/td>\n <\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n <\/td>\n<\/tr>\n
operator-><\/code><\/td>\n <\/td>\ntest2 = std::move(test)<\/code><\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
callAndReport<\/code><\/td>\n <\/td>\nlambda constructed<\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
function call<\/td>\n<\/tr>\n<\/tbody>\n<\/table>\n
Perhaps even less weird-looking is sequencing after the acquisition of the harness<\/code>.<\/p>\n
    auto& harness = test->harness; \/\/ get this before we std::move(test)\r\n    harness<\/span>->callAndReport([test2 = std::move(test)]() ...);\r\n<\/pre>\n
This moves the explicit sequencing a little later in the evaluation of the left column:<\/p>\n\n\n\n\n\n\n\n\n\n\n\n\n\n
test<\/code><\/td>\n <\/td>\n <\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n <\/td>\n<\/tr>\n
operator-><\/code><\/td>\n <\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n <\/td>\n<\/tr>\n
harness<\/code><\/td>\n <\/td>\n <\/td>\n<\/tr>\n
\n
\n<\/td>\n<\/tr>\n
operator-><\/code><\/td>\n <\/td>\ntest2 = std::move(test)<\/code><\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
callAndReport<\/code><\/td>\n <\/td>\nlambda constructed<\/td>\n<\/tr>\n
\u2193<\/td>\n <\/td>\n\u2193<\/td>\n<\/tr>\n
function call<\/td>\n<\/tr>\n<\/tbody>\n<\/table>\n
It doesn’t really matter where we put it, as long as it definitely sequences the read of test<\/code> ahead of its modification by move-assignment.<\/p>\n
But maybe we’re trying too hard.<\/p>\n
The problem with the original code was that it was being too clever, using std::move<\/code> to transfer the std::shared_ptr<\/code> into the lambda and avoid bumping and then dropping the reference count. But what did we really save?<\/p>\n
Let’s look at the entire (repaired) function and evaluate its effect on the std::shared_ptr<\/code>:<\/p>\n
void polarity_test(std::shared_ptr<Test> test)\r\n{\r\n    auto& harness = test->harness; \/\/ get this before we std::move(test)\r\n    harness->callAndReport([test2 = std::move(test)]() mutable\r\n    {\r\n        ...\r\n    });\r\n}\r\n<\/pre>\n
The test<\/code> is destructed at the end of the function, and that’s unavoidable.<\/p>\n
The test2<\/code> is copy-constructed from test<\/code>, and it destructs when the lambda is destroyed.<\/p>\n
I checked gcc, clang, MSVC, and icc, and none of them optimize out the test<\/code> destructor, not even in simple cases like this:<\/p>\n
void simple(std::shared_ptr<int> p)\r\n{\r\n    p.reset();\r\n    \/\/ run the destructor now\r\n}\r\n<\/pre>\n
For gcc, clang, and icc, the reason is that the call site is responsible for destructing parameters, and the call site doesn’t know what the ultimate fate of the shared_ptr<\/code> is. (That changes if the call is inlined, though.)<\/p>\n
So all you’re really saving by moving the test<\/code> into the lambda is the increment of the reference count. It turns out incrementing the reference count of a shared_ptr<\/code> isn’t that bad. It’s a null pointer test and an interlocked increment.<\/p>\n
This doesn’t appear to be a performance-sensitive code path. Indeed, it looks like a test! The simplest solution is probably just to avoid the optimization and copy the shared_ptr<\/code>.<\/p>\n
    test->harness->callAndReport([test2 = test<\/span>]() ...);\r\n<\/pre>\n
Reminder<\/b>: C++17 strengthened the order of evaluation rules and now requires that for function calls, determining the function to call must be performed before the arguments are evaluated.<\/p>\n","protected":false},"excerpt":{"rendered":"
Getting the order of evaluation to be what you want.<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-106182","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
Getting the order of evaluation to be what you want.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/106182","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=106182"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/106182\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=106182"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=106182"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=106182"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}