Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memcheck error in ReplaceTest.NormalizeNansAndZerosMutable gtest #17610

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Fix memcheck error in ReplaceTest.NormalizeNansAndZerosMutable gtest #17610

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e0fa5b
Fix memcheck error in ReplaceTest.NormalizeNansAndZerosMutable gtest
davidwendt Dec 17, 2024
2e80d8f
Merge branch 'branch-25.02' into memcheck-norm-nans-test
davidwendt Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cpp/tests/streams/replace_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@ TEST_F(ReplaceTest, NormalizeNansAndZeros)

TEST_F(ReplaceTest, NormalizeNansAndZerosMutable)
{
auto nan = std::numeric_limits<double>::quiet_NaN();
auto input_column = cudf::test::make_type_param_vector<double>({-0.0, 0.0, -nan, nan, nan});
cudf::test::fixed_width_column_wrapper<double> input(input_column.begin(), input_column.end());
cudf::mutable_column_view mutable_view = cudf::column(input, cudf::test::get_default_stream());
cudf::normalize_nans_and_zeros(mutable_view, cudf::test::get_default_stream());
auto nan = std::numeric_limits<double>::quiet_NaN();
auto data = cudf::test::make_type_param_vector<double>({-0.0, 0.0, -nan, nan, nan});
auto input = cudf::test::fixed_width_column_wrapper<double>(data.begin(), data.end()).release();
auto view = input->mutable_view();
cudf::normalize_nans_and_zeros(view, cudf::test::get_default_stream());
Comment on lines +110 to +111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would not mind this option, but nothing wrong with current code either

Suggested change
auto view = input->mutable_view();
cudf::normalize_nans_and_zeros(view, cudf::test::get_default_stream());
cudf::normalize_nans_and_zeros(input->mutable_view(), cudf::test::get_default_stream());

Copy link
Contributor

@vyasr vyasr Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidwendt this is what I was getting at above. My interpretation of

The view must be created from a non-temporary column and also must be non-temporary itself so that it is not implicitly converted to a column_view.

was that this change would somehow break things because the mutable_column_view would be a temporary and that was not permissible here. Perhaps I was misunderstanding though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is exactly what happens. The temporary created here

cudf::normalize_nans_and_zeros(input->mutable_view(), cudf::test::get_default_stream());

causes the compiler to call the column_view const& API instead of the mutable_column_view& API.
The current code insures the appropriate API is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original change made here: #17436 was an attempt correct the API call by creating a mutable_column_view variable but inadvertently created the view to a destroyed temp column.

Copy link
Contributor

@vyasr vyasr Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does that happen? I wouldn't have expected that overload to ever be selected in this way unless the overload for the same type was actually impossible to call, but input->mutable_view() returns a (non-const) mutable_column_view that should be totally fine for this function signature.

Copy link
Contributor Author

@davidwendt davidwendt Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I'm guessing because it is a temporary and passing a non-const temporary usually makes no sense since any modifications to the object that occur inside the function are just thrown away. I suppose the compiler is trying hard to help here by finding a better API candidate to call.
I feel like this https://godbolt.org/z/hW7cnKPxW illustrates that as well.

Perhaps we should not have an implicit operator conversion from mutable_column_view to column_view
I would not expect that to be a common thing and making it explicit not be a big deal in our code base.

Copy link
Contributor

@vyasr vyasr Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this https://godbolt.org/z/hW7cnKPxW illustrates that as well.

OK I put together a slightly modified version of your example that helped me. I found your example a bit different since there is no overload of the function that actually accepts an instance of hello itself. I would have thought that would always be preferred. Your explanation of why it wouldn't be makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the changes in #17436, we were passing a mutable_column_view rvalue to cudf::normalize_nans_and_zeros. Since we cannot bind a rvalue to a non-const lvalue reference, the cudf::normalize_nans_and_zeros(mutable_column_view &) overload could not be called, and the compiler instead converted the mutable_column_view to column_view so that the overload (cudf::normalize_nans_and_zeros(column_view const &)) with the const reference parameter could be invoked.
However, while trying to create a mutable_column_view lvalue, I accidentally created the view to a rvalue which does not make sense. Thank you for the fix, @davidwendt!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we cannot bind a rvalue to a non-const lvalue reference

I forgot that this was a rule, thanks for stating it out explicitly. I guess the compiler prevents this since there's no sensible reason to allow this and it protects against user error modifying a parameter in a way that would have no effect.

}
Loading