Skip to content

Conversation

@philkuz
Copy link

@philkuz philkuz commented Aug 25, 2025

We pull in the mlir package for the gimlet-api python package. We compile this with manylinux 2.31 to be compatible with more environments. However we also compile with C++20 because we need to support new language features.

manylinux uses libstdc++10 which has early, but buggy/restrictive support for C++20, especially around iterator concepts like std::inserter. Namely the std::inserter expects that iterators are default constructible, which is not the case for SmallPtrSetIterator. Explored trying to work-around this, but template substitution was an impassible barrier for this problem.

So to get around this, we're fixing the one usage that caused problems by switching away from an iterator implementation.

Signed-off-by: Phillip Kuznetsov philkuz@gimletlabs.ai

…nt/bug in libstdc++10

Signed-off-by: Phillip Kuznetsov <philkuz@gimletlabs.ai>
@philkuz
Copy link
Author

philkuz commented Aug 25, 2025

The call:

       // Find the set of underlying pointers that this pointer is based on.
       SmallPtrSet<Value, 4> basedOnPointers;
       for (Value pointer : pointerArgs) {
         FailureOr<SmallVector<Value>> underlyingObjectSet =
             getUnderlyingObjectSet(pointer);
         if (failed(underlyingObjectSet))
           return;
         llvm::copy(*underlyingObjectSet,
                    std::inserter(basedOnPointers, basedOnPointers.begin()));
       }

The bad template:

  template<typename _Container>
    constexpr insert_iterator<_Container>
    inserter(_Container& __x, std::__detail::__range_iter_t<_Container> __i)
    { return insert_iterator<_Container>(__x, __i); }
The failing template argument:
    // Implementation of std::ranges::iterator_t, without using ranges::begin.
    template<typename _Tp>
      using __range_iter_t
	= decltype(__detail::__ranges_begin(std::declval<_Tp&>()));
ranges begin:
    // Simplified version of std::ranges::begin that only supports lvalues,
    // for use by __range_iter_t below.
    template<typename _Tp>
      requires is_array_v<_Tp> || __member_begin<_Tp&> || __adl_begin<_Tp&>
      auto
      __ranges_begin(_Tp& __t)
      {

that requries call is the problem. SmallPtrSet is not an array, so fails the first condition.

So member_begin and adl_begin are here, mostly fine, except for the input_or_output_iterator fails to match


    template<typename _Tp>
      concept __member_begin = requires(_Tp& __t)
	{
	  { __detail::__decay_copy(__t.begin()) } -> input_or_output_iterator;
	};

    void begin(auto&) = delete;
    void begin(const auto&) = delete;

    template<typename _Tp>
      concept __adl_begin = __class_or_enum<remove_reference_t<_Tp>>
	&& requires(_Tp& __t)
	{
	  { __detail::__decay_copy(begin(__t)) } -> input_or_output_iterator;
	};

input_or_output_iterator:

  template<typename _Iter>
    concept input_or_output_iterator
      = requires(_Iter __i) { { *__i } -> __detail::__can_reference; }
	&& weakly_incrementable<_Iter>;
and in weakly_incrementable is the issue, default_initializable.
  template<typename _Iter>
    concept weakly_incrementable = default_initializable<_Iter>
      && movable<_Iter>
      && requires(_Iter __i)
      {

basically default_initializable requires new T() to be valid for the object. SmallPtrSetIterator does not have a default constructor

in libstdc++12 that requirement was dropped:

  /// Requirements on types that can be incremented with ++.
  template<typename _Iter>
    concept weakly_incrementable = movable<_Iter>
      && requires(_Iter __i)
      {

@philkuz philkuz merged commit a074091 into gimlet_patches/20241118 Aug 25, 2025
@philkuz philkuz deleted the philkuz/replace_insert branch August 25, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants