Revert "[libc++] Avoid constructing additional objects when using map::at" #160738
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Andrew Lazarev (alazarev) ChangesReverts llvm/llvm-project#157866 It breaks a lot of sanitizer buildbots Patch is 20.23 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/160738.diff 13 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index ddace8bf8c728..e050362abb658 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -839,7 +839,6 @@ set(files
__type_traits/is_floating_point.h
__type_traits/is_function.h
__type_traits/is_fundamental.h
- __type_traits/is_generic_transparent_comparator.h
__type_traits/is_implicit_lifetime.h
__type_traits/is_implicitly_default_constructible.h
__type_traits/is_integral.h
@@ -882,7 +881,6 @@ set(files
__type_traits/make_32_64_or_128_bit.h
__type_traits/make_const_lvalue_ref.h
__type_traits/make_signed.h
- __type_traits/make_transparent.h
__type_traits/make_unsigned.h
__type_traits/maybe_const.h
__type_traits/nat.h
diff --git a/libcxx/include/__algorithm/comp.h b/libcxx/include/__algorithm/comp.h
index 38e2fb9f5e744..ab3c598418828 100644
--- a/libcxx/include/__algorithm/comp.h
+++ b/libcxx/include/__algorithm/comp.h
@@ -11,7 +11,6 @@
#include <__config>
#include <__type_traits/desugars_to.h>
-#include <__type_traits/is_generic_transparent_comparator.h>
#include <__type_traits/is_integral.h>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -49,9 +48,6 @@ inline const bool __desugars_to_v<__less_tag, __less<>, _Tp, _Tp> = true;
template <class _Tp>
inline const bool __desugars_to_v<__totally_ordered_less_tag, __less<>, _Tp, _Tp> = is_integral<_Tp>::value;
-template <>
-inline const bool __is_generic_transparent_comparator_v<__less<> > = true;
-
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___ALGORITHM_COMP_H
diff --git a/libcxx/include/__functional/is_transparent.h b/libcxx/include/__functional/is_transparent.h
index c2c6fbce2465b..567df1a662f54 100644
--- a/libcxx/include/__functional/is_transparent.h
+++ b/libcxx/include/__functional/is_transparent.h
@@ -29,14 +29,6 @@ inline const bool __is_transparent_v<_Tp, _Key, __void_t<typename _Tp::is_transp
#endif
-// Two types are considered transparently comparable if `comparator(key, arg)` is equivalent to `comparator(key,
-// <implicit cast to KeyT>(arg))`.
-//
-// This is different from `__is_transparent_v`, which is only a property of the comparator and doesn't provide
-// additional semantic guarantees.
-template <class _Comparator, class _KeyT, class _Arg, class = void>
-inline const bool __is_transparently_comparable_v = false;
-
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___FUNCTIONAL_IS_TRANSPARENT
diff --git a/libcxx/include/__functional/operations.h b/libcxx/include/__functional/operations.h
index 7f315ca851c08..7b0ea11db5844 100644
--- a/libcxx/include/__functional/operations.h
+++ b/libcxx/include/__functional/operations.h
@@ -15,9 +15,7 @@
#include <__functional/unary_function.h>
#include <__fwd/functional.h>
#include <__type_traits/desugars_to.h>
-#include <__type_traits/is_generic_transparent_comparator.h>
#include <__type_traits/is_integral.h>
-#include <__type_traits/make_transparent.h>
#include <__utility/forward.h>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -379,14 +377,6 @@ struct less<void> {
typedef void is_transparent;
};
-template <class _Tp>
-struct __make_transparent<less<_Tp> > {
- using type _LIBCPP_NODEBUG = less<>;
-};
-
-template <>
-inline const bool __is_generic_transparent_comparator_v<less<>> = true;
-
template <class _Tp, class _Up>
inline const bool __desugars_to_v<__less_tag, less<>, _Tp, _Up> = true;
@@ -476,14 +466,6 @@ struct greater<void> {
template <class _Tp, class _Up>
inline const bool __desugars_to_v<__greater_tag, greater<>, _Tp, _Up> = true;
-
-template <class _Tp>
-struct __make_transparent<greater<_Tp>> {
- using type _LIBCPP_NODEBUG = greater<>;
-};
-
-template <>
-inline const bool __is_generic_transparent_comparator_v<greater<>> = true;
#endif
// Logical operations
diff --git a/libcxx/include/__functional/ranges_operations.h b/libcxx/include/__functional/ranges_operations.h
index dc9da061af264..df95843e7c9af 100644
--- a/libcxx/include/__functional/ranges_operations.h
+++ b/libcxx/include/__functional/ranges_operations.h
@@ -14,7 +14,6 @@
#include <__concepts/totally_ordered.h>
#include <__config>
#include <__type_traits/desugars_to.h>
-#include <__type_traits/is_generic_transparent_comparator.h>
#include <__utility/forward.h>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -109,12 +108,6 @@ inline const bool __desugars_to_v<__less_tag, ranges::less, _Tp, _Up> = true;
template <class _Tp, class _Up>
inline const bool __desugars_to_v<__greater_tag, ranges::greater, _Tp, _Up> = true;
-template <>
-inline const bool __is_generic_transparent_comparator_v<ranges::less> = true;
-
-template <>
-inline const bool __is_generic_transparent_comparator_v<ranges::greater> = true;
-
#endif // _LIBCPP_STD_VER >= 20
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index ef960d481cb7b..61c910c52c536 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -34,7 +34,6 @@
#include <__type_traits/is_same.h>
#include <__type_traits/is_specialization.h>
#include <__type_traits/is_swappable.h>
-#include <__type_traits/make_transparent.h>
#include <__type_traits/remove_const.h>
#include <__utility/forward.h>
#include <__utility/lazy_synth_three_way_comparator.h>
@@ -1750,8 +1749,7 @@ __tree<_Tp, _Compare, _Allocator>::__find_equal(const _Key& __v) {
}
__node_base_pointer* __node_ptr = __root_ptr();
- auto&& __transparent = std::__as_transparent(value_comp());
- auto __comp = __lazy_synth_three_way_comparator<__make_transparent_t<_Compare>, _Key, value_type>(__transparent);
+ auto __comp = __lazy_synth_three_way_comparator<_Compare, _Key, value_type>(value_comp());
while (true) {
auto __comp_res = __comp(__v, __nd->__get_value());
diff --git a/libcxx/include/__type_traits/is_generic_transparent_comparator.h b/libcxx/include/__type_traits/is_generic_transparent_comparator.h
deleted file mode 100644
index fd02c0b0423d1..0000000000000
--- a/libcxx/include/__type_traits/is_generic_transparent_comparator.h
+++ /dev/null
@@ -1,30 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef _LIBCPP___TYPE_TRAITS_IS_GENERIC_TRANSPARENT_COMPARATOR_H
-#define _LIBCPP___TYPE_TRAITS_IS_GENERIC_TRANSPARENT_COMPARATOR_H
-
-#include <__config>
-
-#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-#endif
-
-_LIBCPP_BEGIN_NAMESPACE_STD
-
-// This traits returns true if the given _Comparator is known to accept any two types for compaison. This is separate
-// from `__is_transparent_v`, since that only enables overloads of specific functions, but doesn't give any semantic
-// guarantees. This trait guarantess that the comparator simply calls the appropriate comparison functions for any two
-// types.
-
-template <class _Comparator>
-inline const bool __is_generic_transparent_comparator_v = false;
-
-_LIBCPP_END_NAMESPACE_STD
-
-#endif // _LIBCPP___TYPE_TRAITS_IS_GENERIC_TRANSPARENT_COMPARATOR_H
diff --git a/libcxx/include/__type_traits/make_transparent.h b/libcxx/include/__type_traits/make_transparent.h
deleted file mode 100644
index 4d3207a807fa7..0000000000000
--- a/libcxx/include/__type_traits/make_transparent.h
+++ /dev/null
@@ -1,48 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef _LIBCPP___TYPE_TRAITS_MAKE_TRANSPARENT_H
-#define _LIBCPP___TYPE_TRAITS_MAKE_TRANSPARENT_H
-
-#include <__config>
-#include <__type_traits/enable_if.h>
-#include <__type_traits/is_empty.h>
-#include <__type_traits/is_same.h>
-
-#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-#endif
-
-_LIBCPP_BEGIN_NAMESPACE_STD
-
-// __make_transparent tries to create a transparent comparator from its non-transparent counterpart, e.g. obtain
-// `less<>` from `less<T>`. This is useful in cases where conversions can be avoided (e.g. a string literal to a
-// std::string).
-
-template <class _Comparator>
-struct __make_transparent {
- using type _LIBCPP_NODEBUG = _Comparator;
-};
-
-template <class _Comparator>
-using __make_transparent_t _LIBCPP_NODEBUG = typename __make_transparent<_Comparator>::type;
-
-template <class _Comparator, __enable_if_t<is_same<_Comparator, __make_transparent_t<_Comparator> >::value, int> = 0>
-_LIBCPP_HIDE_FROM_ABI _Comparator& __as_transparent(_Comparator& __comp) {
- return __comp;
-}
-
-template <class _Comparator, __enable_if_t<!is_same<_Comparator, __make_transparent_t<_Comparator> >::value, int> = 0>
-_LIBCPP_HIDE_FROM_ABI __make_transparent_t<_Comparator> __as_transparent(_Comparator&) {
- static_assert(is_empty<_Comparator>::value);
- return __make_transparent_t<_Comparator>();
-}
-
-_LIBCPP_END_NAMESPACE_STD
-
-#endif // _LIBCPP___TYPE_TRAITS_MAKE_TRANSPARENT_H
diff --git a/libcxx/include/map b/libcxx/include/map
index 035f913bd3497..a63dfec910aae 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -600,10 +600,7 @@ erase_if(multimap<Key, T, Compare, Allocator>& c, Predicate pred); // C++20
# include <__ranges/from_range.h>
# include <__tree>
# include <__type_traits/container_traits.h>
-# include <__type_traits/desugars_to.h>
# include <__type_traits/is_allocator.h>
-# include <__type_traits/is_convertible.h>
-# include <__type_traits/make_transparent.h>
# include <__type_traits/remove_const.h>
# include <__type_traits/type_identity.h>
# include <__utility/forward.h>
@@ -669,11 +666,6 @@ public:
# endif
};
-template <class _Key, class _MapValueT, class _Compare>
-struct __make_transparent<__map_value_compare<_Key, _MapValueT, _Compare> > {
- using type _LIBCPP_NODEBUG = __map_value_compare<_Key, _MapValueT, __make_transparent_t<_Compare> >;
-};
-
# if _LIBCPP_STD_VER >= 14
template <class _MapValueT, class _Key, class _Compare>
struct __lazy_synth_three_way_comparator<__map_value_compare<_Key, _MapValueT, _Compare>, _MapValueT, _MapValueT> {
@@ -1056,24 +1048,6 @@ public:
_LIBCPP_HIDE_FROM_ABI mapped_type& operator[](key_type&& __k);
# endif
- template <class _Arg,
- __enable_if_t<__is_transparently_comparable_v<_Compare, key_type, __remove_cvref_t<_Arg> >, int> = 0>
- _LIBCPP_HIDE_FROM_ABI mapped_type& at(_Arg&& __arg) {
- auto [_, __child] = __tree_.__find_equal(__arg);
- if (__child == nullptr)
- std::__throw_out_of_range("map::at: key not found");
- return static_cast<__node_pointer>(__child)->__get_value().second;
- }
-
- template <class _Arg,
- __enable_if_t<__is_transparently_comparable_v<_Compare, key_type, __remove_cvref_t<_Arg> >, int> = 0>
- _LIBCPP_HIDE_FROM_ABI const mapped_type& at(_Arg&& __arg) const {
- auto [_, __child] = __tree_.__find_equal(__arg);
- if (__child == nullptr)
- std::__throw_out_of_range("map::at: key not found");
- return static_cast<__node_pointer>(__child)->__get_value().second;
- }
-
_LIBCPP_HIDE_FROM_ABI mapped_type& at(const key_type& __k);
_LIBCPP_HIDE_FROM_ABI const mapped_type& at(const key_type& __k) const;
@@ -1268,15 +1242,11 @@ public:
_LIBCPP_HIDE_FROM_ABI iterator find(const key_type& __k) { return __tree_.find(__k); }
_LIBCPP_HIDE_FROM_ABI const_iterator find(const key_type& __k) const { return __tree_.find(__k); }
# if _LIBCPP_STD_VER >= 14
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI iterator find(const _K2& __k) {
return __tree_.find(__k);
}
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI const_iterator find(const _K2& __k) const {
return __tree_.find(__k);
}
@@ -1292,9 +1262,7 @@ public:
# if _LIBCPP_STD_VER >= 20
_LIBCPP_HIDE_FROM_ABI bool contains(const key_type& __k) const { return find(__k) != end(); }
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI bool contains(const _K2& __k) const {
return find(__k) != end();
}
@@ -1303,16 +1271,12 @@ public:
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const key_type& __k) { return __tree_.lower_bound(__k); }
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const key_type& __k) const { return __tree_.lower_bound(__k); }
# if _LIBCPP_STD_VER >= 14
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const _K2& __k) {
return __tree_.lower_bound(__k);
}
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const _K2& __k) const {
return __tree_.lower_bound(__k);
}
@@ -1321,15 +1285,11 @@ public:
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const key_type& __k) { return __tree_.upper_bound(__k); }
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const key_type& __k) const { return __tree_.upper_bound(__k); }
# if _LIBCPP_STD_VER >= 14
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const _K2& __k) {
return __tree_.upper_bound(__k);
}
- template <typename _K2,
- enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
- int> = 0>
+ template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const _K2& __k) const {
return __tree_.upper_bound(__k);
}
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 9b0a7bb433552..dc1933324ef79 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -200,7 +200,6 @@ module std_core [system] {
header "__type_traits/is_fundamental.h"
export std_core.type_traits.integral_constant
}
- module is_generic_transparent_comparator { header "__type_traits/is_generic_transparent_comparator.h" }
module is_implicit_lifetime {
header "__type_traits/is_implicit_lifetime.h"
export std_core.type_traits.integral_constant
@@ -354,7 +353,6 @@ module std_core [system] {
module make_32_64_or_128_bit { header "__type_traits/make_32_64_or_128_bit.h" }
module make_const_lvalue_ref { header "__type_traits/make_const_lvalue_ref.h" }
module make_signed { header "__type_traits/make_signed.h" }
- module make_transparent { header "__type_traits/make_transparent.h" }
module make_unsigned { header "__type_traits/make_unsigned.h" }
module maybe_const { header "__type_traits/maybe_const.h" }
module nat { header "__type_traits/nat.h" }
diff --git a/libcxx/include/string b/libcxx/include/string
index b5b1624201e38..729a420d47598 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -600,7 +600,6 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
# include <__debug_utils/sanitizers.h>
# include <__format/enable_insertable.h>
# include <__functional/hash.h>
-# include <__functional/is_transparent.h>
# include <__functional/unary_function.h>
# include <__fwd/string.h>
# include <__iterator/bounded_iter.h>
@@ -629,7 +628,6 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
# include <__type_traits/is_allocator.h>
# include <__type_traits/is_array.h>
# include <__type_traits/is_convertible.h>
-# include <__type_traits/is_generic_transparent_comparator.h>
# include <__type_traits/is_nothrow_assignable.h>
# include <__type_traits/is_nothrow_constructible.h>
# include <__type_traits/is_replaceable.h>
@@ -2569,20 +2567,6 @@ struct __default_three_way_comparator<basic_string<_CharT, _Traits, _Alloc>, bas
};
# endif
-template <class _Comparator, class _CharT, class _Traits, class _Alloc>
-inline const bool __is_transparently_comparable_v<_Comparator,
- basic_string<_CharT, _Traits, _Alloc>,
- const _CharT*,
- __enable_if_t<__is_generic_transparent_comparator_v<_Comparator> > > =
- true;
-
-template <class _Comparator, class _CharT, class _Traits, class _Alloc, size_t _Np>
-inline const bool __is_transparently_comparable_v<_Comparator,
- basic_string<_CharT, _Traits, _Alloc>,
- _CharT[_Np],
- __enable_if_t<__is_generic_transparent_comparator_v<_Comparator> > > =
- true;
-
# if _LIBCPP_STD_VER >= 17
template <class _InputIterator,
class _CharT = __iter_value_type<_InputIterator>,
diff --git a/libcxx/test/benchmarks/containers/associative/map.bench.cpp b/libcxx/test/benchmarks/containers/associative/map.bench.cpp
index 142229ae64cad..bd664dbb56ee7 100644
--- a/libcxx/test/benchmarks/containers/associative/map.bench.cpp
+++ b/libcxx/test/benchmarks/containers/associative/map.bench.cpp
@@ -16,19 +16,6 @@
#include "../../GenerateInput.h"
#include "benchmark/benchmark.h"
-static void BM_map_find_string_literal(benchmark::State& state) {
- std::map<std::string, int> map;
- map.emplace("Something very very long to show a long string situation", 1);
- map.emplace("Something Else", 2);
-
- for (auto _ : state) {
- benchmark::DoNotOptimize(map);
- benchmark::DoNotOptimize(map.find("Something very very long to show a long string situation"));
- }
-}
-
-BENCHMARK(BM_map_find_string_literal);
-
template <class K, class V>
struct support::adapt_operations<std::map<K, V>> {
using ValueType = typename std::map<K, V>::value_type;
diff --git a/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp b/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp
index d670c531910ea..57adec2d214d4 100644
--- a/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp
+++ b/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp
@@ -15,19 +15,6 @@
#include "../../GenerateInput.h"
#include "benchmark/benchmark.h"
-static void BM_map_find_string_literal(benchmark::State& state) {
- std::unordered_map<std::string, int> map;
- map.emplace("Something very very long to show a long string situation", 1);
- map.emplace("Something Else", 2);
-
- for (auto _ : state) {
- benchmark::DoNotOptimize(map);
- benchmark::DoNotOptimize(map.find("Something very very long to show a lo...
[truncated]
|
|
@alazarev Are you volunteering to fix the issue? |
@philnik777 I guess regardless of who is fixing this we need to recover bots? Do you known what is need to be done? |
I'm happy to revert if I know who to contact about the state of the fix. Whether the person to contact is actually going to fix things or simply works on finding the right person doesn't matter to me, but I'd like to have someone to ask. Otherwise I'll never be able to re-land the patch (or break the bots in the same way again). I guess I'd also be happy if someone can tell me who the code owner is so I can pester that person.
AFAICT the |
I'm not an expert in RDF. https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy clearly indicates: |
Yes. However, the document is just as clear that there has to be a way forward for the original patch, which is unfortunately overlooked too often: "It is not considered reasonable to revert without at least the promise to provide a means for the patch author to debug the root issue.". I don't see that here so far given that I don't even have a contact to ask for further information so far (I assume, given your statement above: "I'm not an expert in RDF"). |
The quote you mentioned is about situations when the revert reason is hard to reproduce. In this particular case there are clear steps to debug. Repro steps can be taken from any of the broken buildbots. And there is a clear way forward for the original patch: fix problematic components (either by working with code owners or yourself) and re-apply the patch. It can be done AFTER the test infrastructure is restored. |
|
FWIW our toolchain builders at google are blocked as well and this revert will help us get unblocked. +1 on revert to land unconditionally. |
Buildbots have public configs. The sanitizer bots IIRC provide you a link to follow to reproduce their build. That's enough for you to reasonably debug it. sanitizer-x86_64-linux-fast doesn't seem like an unreasonable target to expect contributors to be able to debug. This revert should got through, as we're getting more reports of it breaking downstream consumers. |
I'm fixing this in #160804. |
I don't think that's in the spirit of the request. I'm not working on the backend. This is basically as-if I were to ask you to fix something in GCC. I also said above that I'd be happy if I know the code owner, which I still don't know. Would it have been so hard to provide that name? Again, I'm not working on the backend, so it's not at all clear to me which part the broken code is from. I've checked both RDF and CodeGen, but I couldn't figure out who the code owner is. I'd even been happy with "X should know who to talk to" or "I'll talk to Y, he probably knows what to do", but even that was apparently too much to ask for.
Yes, I was absolutely fine with that if I know who to talk to.
Again, I don't think this is reasonable. I'm basically working on a different code base and know almost nothing about the backend. I don't even build that part of the monorepo. All I'm asking here is for some communication what to actually do. Why is that so hard? I don't expect other people to fix things in libc++ in the rare case someone breaks something either. It's my problem as a maintainer of that part of the code base to fix things, not for them to figure out who to even talk to. |
I think most of us assume that folks who are maintainers will be familiar w/ all this. I don't think anyone thought they weren't communicating something specific to you in the threads (I certainly thought things were clear, but obviously I misread the situation ... so sorry we didn't pick up on it earlier). I don't think all the bots do this, but the sanitizer bots are pretty great, because they include reproducer instructions. If you look at the bottom of the log in the bot https://lab.llvm.org/buildbot/#/builders/169/builds/15293 theres a link to tell you what to do w/ a local llvm checkout https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild TBH its pretty awesome that its so easy. IIRC some of the old build bots always used to give you a script to run that would do something similar, which would generally be appreciated, but IIRC all bots have the CMake invocation in their logs, so it shouldn't be too hard to reproduce, generally. Hope that helps for next time :) |
That's not what I'm asking for. I feel like you don't understand what you're asking for here. From my perspective, you're basically coming in and revert while asking me to fix a problem in a project I don't work on, and know very little about. I don't think you'd find it reasonable to go to libstdc++ and say "fix my build or I'll revert your patch". I fully understand that we're in the same repository and people are building the two projects together, and I'm fine with reverting patches temporarily to reduce friction, but that's not a reason for expecting that everybody is knowledgeable on every part of the project. I'm working on a standard library, not a compiler. All I'm asking for here is that someone familiar with the broken project owns the issue and works on a fix with me instead of saying "this broke things, good luck fixing it". I'm fine if it's not immediately clear who that person is, but I'd really appreciate some effort to find someone. |
…p::at" (llvm#160738) This reverts commit b86aaac. The issue in LLVM has been fixed now.
Yes. Or find someone who fix it for you. This is how LLVM works, and I don't think it was different on my memory. And it's very reasonable. It slows down the committer but keeps llvm-project in a healthy state. With high confidence I pull "origin/main" and it will build and run for me. Me, and other contributors, don't need to figure out how to fix the issue in unfamiliar component A, because the maintainer of another unfamiliar to me component B, choose not to revert. Because you don't like to work on unfamiliar components, multiple bot owners, or users of mono repo had to deal with TWO components they are unfamiliar with.
Yes, this is what we need: if instead of "Are you volunteering to fix the issue?" The patch was just approved, @frederick-vs-ja likely fixed the issue, and we had nothing to argue.
In this case it's quite clear. If git blame/log is not enough, more effort may be needed, we have discourse discord and mailing lists. If there is no help, probably in a few days after revert, then we have issue to discuss: maybe nobody care about stuff you are breaking, maybe the patch is not worth of effort, or maybe just another ping is needed. |
…::at" (llvm#160738) Reverts llvm#157866 It breaks a lot of sanitizer buildbots
…p::at" (llvm#160738) (llvm#161485) This reverts commit b86aaac. The issue in LLVM has been fixed now.
Reverts #157866
It breaks a lot of sanitizer buildbots