Reverts around time_zone #95058
Reverts around time_zone #95058
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Florian Mayer (fmayer) ChangesI intend to submit those as separate changes, not as one squashed change. Patch is 141.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/95058.diff 35 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index d65b7ce782ebd..cfe1f44777bca 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -250,7 +250,6 @@ set(files
__chrono/convert_to_tm.h
__chrono/day.h
__chrono/duration.h
- __chrono/exception.h
__chrono/file_clock.h
__chrono/formatter.h
__chrono/hh_mm_ss.h
@@ -277,7 +276,6 @@ set(files
__chrono/year_month.h
__chrono/year_month_day.h
__chrono/year_month_weekday.h
- __chrono/zoned_time.h
__compare/common_comparison_category.h
__compare/compare_partial_order_fallback.h
__compare/compare_strong_order_fallback.h
diff --git a/libcxx/include/__chrono/exception.h b/libcxx/include/__chrono/exception.h
deleted file mode 100644
index 75fd0615b7e08..0000000000000
--- a/libcxx/include/__chrono/exception.h
+++ /dev/null
@@ -1,129 +0,0 @@
-// -*- C++ -*-
-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-// For information see https://libcxx.llvm.org/DesignDocs/TimeZone.html
-
-#ifndef _LIBCPP___CHRONO_EXCEPTION_H
-#define _LIBCPP___CHRONO_EXCEPTION_H
-
-#include <version>
-// Enable the contents of the header only when libc++ was built with experimental features enabled.
-#if !defined(_LIBCPP_HAS_NO_EXPERIMENTAL_TZDB)
-
-# include <__chrono/calendar.h>
-# include <__chrono/local_info.h>
-# include <__chrono/time_point.h>
-# include <__config>
-# include <__configuration/availability.h>
-# include <__verbose_abort>
-# include <format>
-# include <stdexcept>
-# include <string>
-
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
-
-_LIBCPP_BEGIN_NAMESPACE_STD
-
-# if _LIBCPP_STD_VER >= 20
-
-namespace chrono {
-
-class nonexistent_local_time : public runtime_error {
-public:
- template <class _Duration>
- _LIBCPP_HIDE_FROM_ABI nonexistent_local_time(const local_time<_Duration>& __time, const local_info& __info)
- : runtime_error{__create_message(__time, __info)} {
- // [time.zone.exception.nonexist]/2
- // Preconditions: i.result == local_info::nonexistent is true.
- // The value of __info.result is not used.
- _LIBCPP_ASSERT_PEDANTIC(__info.result == local_info::nonexistent,
- "creating an nonexistent_local_time from a local_info that is not non-existent");
- }
-
- _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI ~nonexistent_local_time() override; // exported as key function
-
-private:
- template <class _Duration>
- _LIBCPP_HIDE_FROM_ABI string __create_message(const local_time<_Duration>& __time, const local_info& __info) {
- return std::format(
- R"({} is in a gap between
-{} {} and
-{} {} which are both equivalent to
-{} UTC)",
- __time,
- local_seconds{__info.first.end.time_since_epoch()} + __info.first.offset,
- __info.first.abbrev,
- local_seconds{__info.second.begin.time_since_epoch()} + __info.second.offset,
- __info.second.abbrev,
- __info.first.end);
- }
-};
-
-template <class _Duration>
-_LIBCPP_NORETURN _LIBCPP_AVAILABILITY_TZDB _LIBCPP_HIDE_FROM_ABI void __throw_nonexistent_local_time(
- [[maybe_unused]] const local_time<_Duration>& __time, [[maybe_unused]] const local_info& __info) {
-# ifndef _LIBCPP_HAS_NO_EXCEPTIONS
- throw nonexistent_local_time(__time, __info);
-# else
- _LIBCPP_VERBOSE_ABORT("nonexistent_local_time was thrown in -fno-exceptions mode");
-# endif
-}
-
-class ambiguous_local_time : public runtime_error {
-public:
- template <class _Duration>
- _LIBCPP_HIDE_FROM_ABI ambiguous_local_time(const local_time<_Duration>& __time, const local_info& __info)
- : runtime_error{__create_message(__time, __info)} {
- // [time.zone.exception.ambig]/2
- // Preconditions: i.result == local_info::ambiguous is true.
- // The value of __info.result is not used.
- _LIBCPP_ASSERT_PEDANTIC(__info.result == local_info::ambiguous,
- "creating an ambiguous_local_time from a local_info that is not ambiguous");
- }
-
- _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI ~ambiguous_local_time() override; // exported as key function
-
-private:
- template <class _Duration>
- _LIBCPP_HIDE_FROM_ABI string __create_message(const local_time<_Duration>& __time, const local_info& __info) {
- return std::format(
- // There are two spaces after the full-stop; this has been verified
- // in the sources of the Standard.
- R"({0} is ambiguous. It could be
-{0} {1} == {2} UTC or
-{0} {3} == {4} UTC)",
- __time,
- __info.first.abbrev,
- __time - __info.first.offset,
- __info.second.abbrev,
- __time - __info.second.offset);
- }
-};
-
-template <class _Duration>
-_LIBCPP_NORETURN _LIBCPP_AVAILABILITY_TZDB _LIBCPP_HIDE_FROM_ABI void __throw_ambiguous_local_time(
- [[maybe_unused]] const local_time<_Duration>& __time, [[maybe_unused]] const local_info& __info) {
-# ifndef _LIBCPP_HAS_NO_EXCEPTIONS
- throw ambiguous_local_time(__time, __info);
-# else
- _LIBCPP_VERBOSE_ABORT("ambiguous_local_time was thrown in -fno-exceptions mode");
-# endif
-}
-
-} // namespace chrono
-
-# endif // _LIBCPP_STD_VER >= 20
-
-_LIBCPP_END_NAMESPACE_STD
-
-#endif // !defined(_LIBCPP_HAS_NO_EXPERIMENTAL_TZDB)
-
-#endif // _LIBCPP___CHRONO_EXCEPTION_H
diff --git a/libcxx/include/__chrono/time_zone.h b/libcxx/include/__chrono/time_zone.h
index de11dac1eef0c..91ddab8903fe2 100644
--- a/libcxx/include/__chrono/time_zone.h
+++ b/libcxx/include/__chrono/time_zone.h
@@ -16,16 +16,12 @@
// Enable the contents of the header only when libc++ was built with experimental features enabled.
#if !defined(_LIBCPP_HAS_NO_EXPERIMENTAL_TZDB)
-# include <__chrono/calendar.h>
# include <__chrono/duration.h>
-# include <__chrono/exception.h>
-# include <__chrono/local_info.h>
# include <__chrono/sys_info.h>
# include <__chrono/system_clock.h>
# include <__compare/strong_order.h>
# include <__config>
# include <__memory/unique_ptr.h>
-# include <__type_traits/common_type.h>
# include <string_view>
# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -42,8 +38,6 @@ _LIBCPP_BEGIN_NAMESPACE_STD
namespace chrono {
-enum class choose { earliest, latest };
-
class _LIBCPP_AVAILABILITY_TZDB time_zone {
_LIBCPP_HIDE_FROM_ABI time_zone() = default;
@@ -69,91 +63,12 @@ class _LIBCPP_AVAILABILITY_TZDB time_zone {
return __get_info(chrono::time_point_cast<seconds>(__time));
}
- template <class _Duration>
- [[nodiscard]] _LIBCPP_HIDE_FROM_ABI local_info get_info(const local_time<_Duration>& __time) const {
- return __get_info(chrono::time_point_cast<seconds>(__time));
- }
-
- // We don't apply nodiscard here since this function throws on many inputs,
- // so it could be used as a validation.
- template <class _Duration>
- _LIBCPP_HIDE_FROM_ABI sys_time<common_type_t<_Duration, seconds>> to_sys(const local_time<_Duration>& __time) const {
- local_info __info = get_info(__time);
- switch (__info.result) {
- case local_info::unique:
- return sys_time<common_type_t<_Duration, seconds>>{__time.time_since_epoch() - __info.first.offset};
-
- case local_info::nonexistent:
- chrono::__throw_nonexistent_local_time(__time, __info);
-
- case local_info::ambiguous:
- chrono::__throw_ambiguous_local_time(__time, __info);
- }
-
- // TODO TZDB The Standard does not specify anything in these cases.
- _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
- __info.result != -1, "cannot convert the local time; it would be before the minimum system clock value");
- _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
- __info.result != -2, "cannot convert the local time; it would be after the maximum system clock value");
-
- return {};
- }
-
- template <class _Duration>
- [[nodiscard]] _LIBCPP_HIDE_FROM_ABI sys_time<common_type_t<_Duration, seconds>>
- to_sys(const local_time<_Duration>& __time, choose __z) const {
- local_info __info = get_info(__time);
- switch (__info.result) {
- case local_info::unique:
- case local_info::nonexistent: // first and second are the same
- return sys_time<common_type_t<_Duration, seconds>>{__time.time_since_epoch() - __info.first.offset};
-
- case local_info::ambiguous:
- switch (__z) {
- case choose::earliest:
- return sys_time<common_type_t<_Duration, seconds>>{__time.time_since_epoch() - __info.first.offset};
-
- case choose::latest:
- return sys_time<common_type_t<_Duration, seconds>>{__time.time_since_epoch() - __info.second.offset};
-
- // Note a value out of bounds is not specified.
- }
- }
-
- // TODO TZDB The standard does not specify anything in these cases.
- _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
- __info.result != -1, "cannot convert the local time; it would be before the minimum system clock value");
- _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
- __info.result != -2, "cannot convert the local time; it would be after the maximum system clock value");
-
- return {};
- }
-
- template <class _Duration>
- [[nodiscard]] _LIBCPP_HIDE_FROM_ABI local_time<common_type_t<_Duration, seconds>>
- to_local(const sys_time<_Duration>& __time) const {
- using _Dp = common_type_t<_Duration, seconds>;
-
- sys_info __info = get_info(__time);
-
- _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
- __info.offset >= chrono::seconds{0} || __time.time_since_epoch() >= _Dp::min() - __info.offset,
- "cannot convert the system time; it would be before the minimum local clock value");
-
- _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
- __info.offset <= chrono::seconds{0} || __time.time_since_epoch() <= _Dp::max() - __info.offset,
- "cannot convert the system time; it would be after the maximum local clock value");
-
- return local_time<_Dp>{__time.time_since_epoch() + __info.offset};
- }
-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI const __impl& __implementation() const noexcept { return *__impl_; }
private:
[[nodiscard]] _LIBCPP_EXPORTED_FROM_ABI string_view __name() const noexcept;
[[nodiscard]] _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI sys_info __get_info(sys_seconds __time) const;
- [[nodiscard]] _LIBCPP_AVAILABILITY_TZDB _LIBCPP_EXPORTED_FROM_ABI local_info __get_info(local_seconds __time) const;
unique_ptr<__impl> __impl_;
};
diff --git a/libcxx/include/__chrono/zoned_time.h b/libcxx/include/__chrono/zoned_time.h
deleted file mode 100644
index c6084426ad72b..0000000000000
--- a/libcxx/include/__chrono/zoned_time.h
+++ /dev/null
@@ -1,55 +0,0 @@
-// -*- C++ -*-
-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-// For information see https://libcxx.llvm.org/DesignDocs/TimeZone.html
-
-#ifndef _LIBCPP___CHRONO_ZONED_TIME_H
-#define _LIBCPP___CHRONO_ZONED_TIME_H
-
-#include <version>
-// Enable the contents of the header only when libc++ was built with experimental features enabled.
-#if !defined(_LIBCPP_HAS_NO_EXPERIMENTAL_TZDB)
-
-# include <__chrono/time_zone.h>
-# include <__chrono/tzdb_list.h>
-# include <__config>
-# include <__fwd/string_view.h>
-
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
-
-_LIBCPP_BEGIN_NAMESPACE_STD
-
-# if _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
- !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-
-namespace chrono {
-
-template <class>
-struct zoned_traits {};
-
-template <>
-struct zoned_traits<const time_zone*> {
- [[nodiscard]] _LIBCPP_HIDE_FROM_ABI static const time_zone* default_zone() { return chrono::locate_zone("UTC"); }
- [[nodiscard]] _LIBCPP_HIDE_FROM_ABI static const time_zone* locate_zone(string_view __name) {
- return chrono::locate_zone(__name);
- }
-};
-
-} // namespace chrono
-
-# endif // _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM)
- // && !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-
-_LIBCPP_END_NAMESPACE_STD
-
-#endif // !defined(_LIBCPP_HAS_NO_EXPERIMENTAL_TZDB)
-
-#endif // _LIBCPP___CHRONO_ZONED_TIME_H
diff --git a/libcxx/include/chrono b/libcxx/include/chrono
index c1a92595ff1f5..96a3e92faa81f 100644
--- a/libcxx/include/chrono
+++ b/libcxx/include/chrono
@@ -724,10 +724,6 @@ const time_zone* current_zone()
const tzdb& reload_tzdb(); // C++20
string remote_version(); // C++20
-// [time.zone.exception], exception classes
-class nonexistent_local_time; // C++20
-class ambiguous_local_time; // C++20
-
// [time.zone.info], information classes
struct sys_info { // C++20
sys_seconds begin;
@@ -767,28 +763,10 @@ class time_zone {
template<class Duration>
sys_info get_info(const sys_time<Duration>& st) const;
-
- template<class Duration>
- local_info get_info(const local_time<Duration>& tp) const;
-
- template<class Duration>
- sys_time<common_type_t<Duration, seconds>>
- to_sys(const local_time<Duration>& tp) const;
-
- template<class Duration>
- sys_time<common_type_t<Duration, seconds>>
- to_sys(const local_time<Duration>& tp, choose z) const;
-
- template<class Duration>
- local_time<common_type_t<Duration, seconds>>
- to_local(const sys_time<Duration>& tp) const;
};
bool operator==(const time_zone& x, const time_zone& y) noexcept; // C++20
strong_ordering operator<=>(const time_zone& x, const time_zone& y) noexcept; // C++20
-// [time.zone.zonedtraits], class template zoned_traits
-template<class T> struct zoned_traits; // C++20
-
// [time.zone.leap], leap second support
class leap_second { // C++20
public:
@@ -934,7 +912,6 @@ constexpr chrono::year operator ""y(unsigned lo
#if _LIBCPP_STD_VER >= 20
# include <__chrono/calendar.h>
# include <__chrono/day.h>
-# include <__chrono/exception.h>
# include <__chrono/hh_mm_ss.h>
# include <__chrono/literals.h>
# include <__chrono/local_info.h>
@@ -962,7 +939,6 @@ constexpr chrono::year operator ""y(unsigned lo
# include <__chrono/time_zone_link.h>
# include <__chrono/tzdb.h>
# include <__chrono/tzdb_list.h>
-# include <__chrono/zoned_time.h>
# endif
#endif
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 892d2c6b4af3c..48391b2a12095 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -1101,7 +1101,6 @@ module std_private_chrono_duration [system] {
header "__chrono/duration.h"
export std_private_type_traits_is_convertible
}
-module std_private_chrono_exception [system] { header "__chrono/exception.h" }
module std_private_chrono_file_clock [system] { header "__chrono/file_clock.h" }
module std_private_chrono_formatter [system] {
header "__chrono/formatter.h"
@@ -1114,10 +1113,7 @@ module std_private_chrono_high_resolution_clock [system] {
}
module std_private_chrono_leap_second [system] { header "__chrono/leap_second.h" }
module std_private_chrono_literals [system] { header "__chrono/literals.h" }
-module std_private_chrono_local_info [system] {
- header "__chrono/local_info.h"
- export std_private_chrono_sys_info
-}
+module std_private_chrono_local_info [system] { header "__chrono/local_info.h" }
module std_private_chrono_month [system] { header "__chrono/month.h" }
module std_private_chrono_month_weekday [system] { header "__chrono/month_weekday.h" }
module std_private_chrono_monthday [system] { header "__chrono/monthday.h" }
@@ -1159,7 +1155,6 @@ module std_private_chrono_year [system] { header "__chrono/yea
module std_private_chrono_year_month [system] { header "__chrono/year_month.h" }
module std_private_chrono_year_month_day [system] { header "__chrono/year_month_day.h" }
module std_private_chrono_year_month_weekday [system] { header "__chrono/year_month_weekday.h" }
-module std_private_chrono_zoned_time [system] { header "__chrono/zoned_time.h" }
module std_private_compare_common_comparison_category [system] { header "__compare/common_comparison_category.h" }
module std_private_compare_compare_partial_order_fallback [system] { header "__compare/compare_partial_order_fallback.h" }
diff --git a/libcxx/modules/std/chrono.inc b/libcxx/modules/std/chrono.inc
index 87e32afbe4bdc..813322a1797f6 100644
--- a/libcxx/modules/std/chrono.inc
+++ b/libcxx/modules/std/chrono.inc
@@ -209,12 +209,13 @@ export namespace std {
using std::chrono::reload_tzdb;
using std::chrono::remote_version;
-# endif // !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
- // !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-
+# if 0
// [time.zone.exception], exception classes
using std::chrono::ambiguous_local_time;
using std::chrono::nonexistent_local_time;
+# endif // if 0
+# endif // !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
+ // !defined(_LIBCPP_HAS_NO_LOCALIZATION)
// [time.zone.info], information classes
using std::chrono::local_info;
@@ -223,14 +224,18 @@ export namespace std {
# if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) && \
!defined(_LIBCPP_HAS_NO_LOCALIZATION)
+# if 0
// [time.zone.timezone], class time_zone
using std::chrono::choose;
+# endif // if 0
+
using std::chrono::time_zone;
+# if 0
+
// [time.zone.zonedtraits], class template zoned_traits
using std::chrono::zoned_traits;
-# if 0
// [time.zone.zonedtime], class template zoned_time
using std::chrono::zoned_time;
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index 9e6c70335a794..65e6ce2c4da43 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -340,9 +340,6 @@ if (LIBCXX_ENABLE_LOCALIZATION AND LIBCXX_ENABLE_FILESYSTEM AND LIBCXX_ENABLE_TI
include/tzdb/types_private.h
include/tzdb/tzdb_list_private.h
include/tzdb/tzdb_private.h
- # TODO TZDB The exception could be moved in chrono once the TZDB library
- # is no longer experimental.
- chrono_exception.cpp
time_zone.cpp
tzdb.cpp
tzdb_list.cpp
diff --git a/libcxx/src/chrono_exception.cpp b/libcxx/src/chrono_exception.cpp
deleted file mode 100644
index bea2ad110310a..0000000000000
--- a/libcxx/src/chrono_exception.cpp
+++ /dev/null
@@ -1,22 +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
-//
-//===----------------------------------------------------------------------===//
-
-#include <chrono>
-
-_LIBCPP_BEGIN_NAMESPACE_STD
-
-namespace chrono {
-
-_LIBCPP_AVAILABILITY_TZDB
-_LIBCPP_EXPORTED_FROM_ABI nonexistent_local_time::~nonexistent_local_time() = default; // key function
-_LIBCPP_AVAILABILITY_TZDB
-_LIBCPP_EXPORTED_FROM_ABI ambiguous_local_time::~ambiguous_local_time() = default; // key function
-
-} // namespace chrono
-
-_LIBCPP_END_NAMESPACE_STD
diff --git a/li...
[truncated]
|
|
Can you please provide context for why you want to revert these patches? |
See the comments in #89537. This broke multiple buildbots. |
|
I'd like @mordante to chime in before these patches are reverted. It's not clear to me that we currently have a path to fixing this after the revert, so this feels a lot like a "lowest effort revert back to green without enough information to then move forward" kind of thing. By the way, these configurations are also not visible in our pre-commit CI. Please add them there, that is our policy to get libc++ support. |
This is not only our bot, but others as well. Also, quoting LLVM policy:
It's not about effort, it's about not stacking breakages on top of each other. It's infeasible to add all these configurations to pre-commit UI. Some of our bots take hours to compile multi-stage LLVM with sanitizers. |
It does not work with different timezones. Revert and re-land is very trivial. Why do we need to have exception for libc++ when this approach works just fine for the rest of LLVM.
Do you have arm hardware? |
The HWSAN is fixed by disabling the test in the CI right? I got an e-mail of the sanitizer failure, however in my experience the sanitizer builds tend to be flaky so I typically look whether they fail for multiple builds. (I spend a lot of time in the past to investigate build failures and the next build was green again.) I have not gotten e-mails of other build failures.
Louis asked how we can then move forward. That is part of the same policy "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. If a situation arises where a public reproducer can not be shared for some reason (e.g. requires hardware patch author doesn’t have access to, sharp regression in compile time of internal workload, etc.), the reverter is expected to be proactive about working with the patch author to debug and test candidate patches." So I would like to know how we can move ahead before reverting. Since these changes can depend on the system configuration it's hard to debug them on these systems. |
It's been continuously failing since yesterday.
You can run the sanitizer buildbot configuration, as long as you have arm64 hardware. It says in the buildbot error message:
|
Nothing in sanitizers that can cause flakiness on it's own.
Technically it should be enough to reproduce. But if it's just a slow test, as we have an evidence for that, can you just split it some how soon, to cut slowest parts e.g. in half?
Somehow it's not disabled. Probably something in libc++ test infra didn't propagate hwasan feature. |
@fmayer Just noticed, XFAIL does not fix timouts, as XFAIL executes test anyway. It must be UNSUPPORTED |
|
It's easy to quote stuff from the Developer Policy out of context. The Developer Policy basically states that it's OK to revert to a green state to keep the CI afloat, and it describes provisions for how people can work together to make progress. It doesn't condone reverting a project's work without a reasonable path to fixing the bots, nor does it state that arbitrary people can set up arbitrary build bot configurations (on arbitrary hardware and arbitrary machines) and then hold project contributors accountable for failures on these bots to the point of not being able to make progress because of them. Libc++ has a documented support policy and these bots seem to be falling outside of that. I'm not going to play games with you and pretend that we don't have to fix the issues on your bots: we definitely want to get that fixed ASAP. However, since these setups fall outside of our CI, are inaccessible to us and are clearly a bit unusual, we need your help to investigate the issue or provide easy access to the machine so we can reasonably investigate it ourselves. Without that, we'll basically revert these patches, folks will disappear and we'll be left without the ability to re-land the patches. Once this is over: We work really hard to make our CI as good as possible and covers all of our supported usage scenarios. Please work with us to include your usage scenarios into our pre-commit CI, these events are a huge pain for everyone involved and can be solved with technical means. |
|
Note that if you really just want to turn off this whole feature on your configuration while we're figuring out a path forward, I think you could just pass This would have the added benefit that the issue can be reproduced on your setup by just re-enabling the configuration. |
It's not that at all. It's about reverting, figuring out what to do, and re-landing. Which is the norm in (at least non-libcxx) LLVM. I had no intention of reverting it and just disappearing. Sorry if that is the impression that you got. |
I don't have access to ARM64 hardware. I also noticed this page states "You need system similar to the buildbot, details can be found in worker info on the build page, e.g. sanitizer-x86_64-linux-fast" at the linked site I don't see the requirements how to build this. I would be helpful if that page can be improved. Since this error is a time-out it might not be possible to reproduce on a different machine. As @vitalybuka mentioned the test should be
Yes that is the option to disable this. |
I already submitted this. I will note that even on my workstation without sanitizers, this test is slow. This revert wasn't mainly for sanitizers in the first place, but because other people also reported issues on the PR so I proposed this revert while we figure out the various issues. |
Isn't should be the opposite, if way forward is not clear - revert, then figured, including working with bots maintainers. I always help if to debug on my bots, I can't promise to do that immediately. So revert and debug AFTER We do this all the time on the rest of LLVM, but somehow it's a problem for libcxx.
I am rely grateful to people who to setup and maintain those bots. I would rather be slowed down just after landing the patch, then to look into that in 6 month when it reached release on that specific config, and now I am out of context and need to switch from something else. I think those arbitrarily bots very helpful in long run.
Yes, that default, sorry if it's not clear.
Happy to help with CI, but missing CI should not be excuse for no-revert. |
|
I guess in general we arguing about these two approaches:
and
To me @mordante and @fmayer time investment of Revert <= No-Revert, but No1 is less likely annoy the community and downstream users. So I strongly advocate for No1 and revert when in doubt. |
|
I think that's a false dilemma. My understanding is that there are three failures reported:
The first two issues don't have a path to being resolved because the reporters have not provided a ways to reproduce the failures or investigated them. Therefore, they are not grounds for asking a revert of the patch. This is especially true given that TZDB is a library that is inherently coupled with the system in use, which means that system differences and quirks are intended. This means that the only viable option might very well be to The last issue is a timeout. The fact that you run a build bot with instrumentation that greatly slows down runtime tests and can cause them to timeout is also not grounds to revert the patch. The right solution here is to mark the test as unsupported under that configuration, or to use a faster machine, or to make the tests faster, or to improve hwasan to have less runtime impact. It doesn't make the code that was committed wrong as in "incorrect". To further explain my point: if I enable a CI machine that is vastly underpowered and causes a bunch of tests to start timing out, whose fault is it? Can I then go to patch authors and ask them to revert their patches under the Dev Policy because of my bot? I think we'll all agree that it doesn't make sense. Instead, I should figure out a way to make my bot robust to longer-running tests and take it upon myself to deal with the implications of running such a bot. So, to summarize: When we ship a bug and we can't quickly fix it forward, we revert. We all agree on that, it's the LLVM way. However, many things don't qualify as "a bug" yet still break CI. This is a prime example of such a situation, and I am really exhausted of folks trying to strong-arm others into reverting patches blindly just to get their bots green again with minimal effort while citing the LLVM policy. @vitalybuka Is there even still a problem? You marked the test as unsupported under hwasan, so it's not running under your configuration. Did that work? A better fix would be to use |
no one strong-armed anyone. From out point of view: this breaks our bot and at least two others, we don't have enough context into what you are doing to fix forward, or to know whether this is indicative a bug, so we proposed a revert. That's it. I don't know what the big fuss is about, submitting and reverting the revert would have taken less time than this meta-discussion. It will always be the case that buildbot maintainers do not have the same level of context into the change, so they will err more on the side of reverts than the author would. |
I consider selectively quoting the part of the LLVM Developer Policy that says "reverts are fine" to be a form of that. This fuss is about the dynamics in LLVM that make some people think that reverting a month of work over a test that's running too slowly under a sanitizer and should be addressed with a Lit annotation is normal. It happens a lot in libc++ due to the nature of the project running under many configurations and on many platforms, and it's exhausting. It also doesn't help that it comes almost exclusively from corporate actors and is imposed upon contributors who (in libc++) are mostly enthusiasts, leading to a bad contribution experience. |
|
Again, the revert was because it broke three buildbots, not only for the lit annotation. We don't want to mask problems, and if multiple bots fail, maybe something is actually wrong. If you say the other two buildbots are running unsupported configurations, that's okay with me, but there is no way for me to know this, it wasn't called out in the PR. For the record I will say that I too have worked on runtime components, which are by definition hard to test in all configurations, and also got my changes reverted sometimes. Please don't assume bad intent based on whether someone is a "corporate actor". |
I did not assume bad intent based on that. BTW I am employed full-time working on LLVM myself. What I meant is simply that what can appear as acceptable to someone working on LLVM as their full-time job in a corporate context may be perceived completely differently by someone who is doing this as an enthusiast outside of a corporate setting, and that can lead to contributor alienation for those people. Libc++ has a much higher concentration of enthusiasts than any other area of LLVM (at least in my experience). |
|
@ldionne I am not sure why you are on such defense on reverts.
It's not about blaming who is this fault. It's about doing necessary to maintain the code health. It's not ONLY about correctness. Our build bots are way to keep compiler from regressing. And time to time revert of correct code is needed to keep bots functioning, maybe just for upgrading or re-configuring build steps. Keeping attention to bots is the price to pay to have good diverse test coverage and detect REAL bugs early. For problematic bots:
But it's not the case we have here. That bot is consistently green and well maintained.
And until proven there is always safe to assume that there is a bug than CI failure. In particular case possibility of 'deadlock' what is concerned me. And my point that threshold for revert should be significantly lower than TRUE bug. Some times it's need to be done to figure out how to proceed further. If it's CI issue, it's better to revert, bot maintainers may need some time catching up, e.g. when we upgrade some deps requirement, we may revert back and forth that patch, until we upgrade all bots. It's way better then we broke all bots and fix forward them next few weeks keeping then non-functional. Reverting is a trivial thing, it's not some terrible thing that must never happens. E.g. here @mordante was able quickly guess problems, and if we reverted that yesterday, we'd already had it relanded with UNSUPPORTED spending less time than we spent arguing here. It works for the rest of LLVM. works for large OSS projects, e.g. Chromium, why libcxx needs to be special? To summarize:
Yes, it works.
Thanks! I didn't know. We will use this. Then disabling asan is unnecessary, This particular issue is resolved. My main concern is that reverts in libcxx are often an issue and more complicated than in the rest of LLVM, making bot maintenance difficult. Can we do better? |
As mentioned before, the sanitizer bots have false positives every now and then. I agree with @ldionne that a time-out in a bot is not per-se an error in our code.
Reverting and relanding is not always trivial. Temporary disabling tests on configurations that do not work is a solution we use in libc++.
We try to do better, in libc++ we have a pre-commit CI to test all our supported configurations. This means patches in libc++ typically have been tested in the CI more than typical patches in the LLVM project. My personal issue with reverting directly is that it sometimes is easier to add a patch that temporary disables a configuration. Especially since buildbot maintainers sometimes don't have time to look at an issue in a timely fashion. That means development of a feature is blocked until that can be resolved. (Using stacked patches in GitHub, unlike in Phabricator, is quite painful. This is already makes development of larger features harder than previously.) Note that in cases where I expect issues in the code due to CI issues I revert my patches. In this case I only got 1 e-mail of a failing build, not of all 3 failing builds. |
In this case it was ~10 consecutive failures, which is definitely not a flake. I will be very concerned about health of the project if we start treating bot issues as not "out code".
Yes. But most of the time it's trivial.
CI does best effort but can't cover all supported configurations. E.g. I don't see anything except x86.
That all true, but reverting should still be treated as trivial thing.
"Blocked" is exaggeration. We all work we a lot of branches and commits in local checkouts.
https://github.com/getcord/spr not the same but pretty close. BTW. It would be nice to at least pause landing the stack after the firs bot email.
If you believed that this is CI issue from the beginning, it could be helpful to comment. Somehow it didn't click for me from the beginning that this is just a long test, and not deadlock, which means a bug in the compiler or libcxx. But again it's not the point. If we do reverts easily, we had this patch re-landed with test disabled in no time, without the drama. |
This broke multiple buildbots.
See the comments in #89537.
I intend to submit those as separate changes, not as one squashed change.