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.

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

Support gcc 12 as the C++ compiler #13316

Merged

Conversation

robertmaynard
Copy link
Contributor

Description

Corrects compilation issues found by gcc 12 and the new warnings it provides.

Three major classes of issues was found:

  • incorrect lambda captures of the wrong container for the iterators being used
  • incorrect implicit conversions
  • warnings generated by using std::transform and std::initializer_lists

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@robertmaynard robertmaynard added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels May 9, 2023
@robertmaynard robertmaynard requested a review from a team as a code owner May 9, 2023 15:08
@robertmaynard robertmaynard requested review from harrism and vyasr May 9, 2023 15:08
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 9, 2023
@robertmaynard robertmaynard requested a review from a team as a code owner May 9, 2023 17:41
@github-actions github-actions bot added the CMake CMake build issue label May 9, 2023
if constexpr (std::is_unsigned_v<TypeParam>) { return static_cast<TypeParam>(std::abs(e)); }
return static_cast<TypeParam>(e);
});
std::vector<T> input{init_list};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to copy the init_list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reproducer:

#include <algorithm>
#include <cstdint>
#include <vector>
#include <thrust/host_vector.h>


template <typename TypeParam, typename T>
thrust::host_vector<TypeParam>
make_type_param_vector(std::initializer_list<T> const& init_list) {
  // std::vector<T> input{init_list};
  std::vector<TypeParam> vec(init_list.size()); //or thrust::host_vector
  std::transform(std::cbegin(init_list), std::cend(init_list), std::begin(vec), [](auto const& e) {
    if constexpr (std::is_unsigned_v<TypeParam>) { return static_cast<TypeParam>(std::abs(e)); }
    return static_cast<TypeParam>(e);
  });
  return vec;
}


template <typename T> bool validate_A() {
  auto const input_column_valid_a = make_type_param_vector<uint8_t>({1, 0});
  auto const input_column_valid_b = make_type_param_vector<uint8_t>({0, 0});
  auto const input_column_valid_c = make_type_param_vector<T>({15, 16});
  return 0;
}
int main() {
  validate_A<float>();
  validate_A<double>();
  validate_A<int8_t>();
  validate_A<int16_t>();
  validate_A<int32_t>();
  validate_A<int64_t>();
  validate_A<uint8_t>();
  validate_A<uint16_t>();
  validate_A<uint32_t>();
  return 0;
}

Will generate warnings ( as errors ) of:

    inlined from 'thrust::host_vector<TypeParam> make_type_param_vector(const std::initializer_list<_Value>&) [with TypeParam = unsigned char; T = int]' at /home/rmaynard/Work/temp/sample2.cpp:11:34:
/home/rmaynard/miniconda3/envs/cuda_tk12/x86_64-conda-linux-gnu/include/c++/12.2.0/bits/new_allocator.h:137:55: note: at offset [29, 4611686018427387917] into destination object of size 3 allocated by 'operator new'
  137 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
In function '_OIter std::transform(_IIter, _IIter, _OIter, _UnaryOperation) [with _IIter = const int*; _OIter = thrust::detail::normal_iterator<unsigned char*>; _UnaryOperation = make_type_param_vector<unsigned char, int>(const std::initializer_list<int>&)::<lambda(const auto:1&)>]',
    inlined from 'thrust::host_vector<TypeParam> make_type_param_vector(const std::initializer_list<_Value>&) [with TypeParam = unsigned char; T = int]' at /home/rmaynard/Work/temp/sample2.cpp:12:17:
/home/rmaynard/miniconda3/envs/cuda_tk12/x86_64-conda-linux-gnu/include/c++/12.2.0/bits/stl_algo.h:4263:19: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
 4263 |         *__result = __unary_op(*__first);

I believe that this is a bug in gcc as it is easit to trigger when we have at least 3 calls to make_type_param_vector with input of the same length with no other calls of a differing length to make_type_param_vector. Copying the initializer_list to a std::vector avoids this bug.

@harrism
Copy link
Member

harrism commented May 9, 2023

Why so many reviewers assigned?

@robertmaynard
Copy link
Contributor Author

Why so many reviewers assigned?

I believe that you and ttnghia got auto assigned, and everyone else wanted to see the gcc-12 changes.

Previously the `nulls_from_nullptrs` would iterate over the
passed in vector but capture a copy of the vector in the lambda.

This made it look valid on initial read but is actually a bug
since the iterator doesn't match the capture.
Previous to gcc-12 it looks like the explicit conversion in fixed_point
would be used for implicit conversions. Now that the bug has been fixed
we need to use syntax that allows for explicit conversions to occur.
The original code would generate ( maybe false positive ) warnings
when the initializer_list was used for the transform iterators.
Copying to a local vector resolves the issue.
@robertmaynard robertmaynard force-pushed the bug/support_gcc_12_compilation branch from df10576 to 2488a84 Compare May 11, 2023 20:01
@robertmaynard
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 98122d3 into rapidsai:branch-23.06 May 11, 2023
@robertmaynard robertmaynard deleted the bug/support_gcc_12_compilation branch May 15, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants