vsk updated this revision to Diff 265829. vsk added a comment. Apologies for all the breakage. I think I've identified the issues in the initial patch which caused breakage on the bots, and which caused the tests to not target what they meant to test.
I've reworked this patch to use std::llrint, instead of casting the max `time_t` value to `double` as part of an overflow check. The cast to `double` was not precise, and could crash on some machines (presumably due to a floating point exception?). For the tests, I've added asserts to make sure that the specially-crafted values that are meant to trigger clamping behavior in std::llrint, or an overflow in `llvm::checkedAdd`, actually do satisfy those conditions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80150/new/ https://reviews.llvm.org/D80150 Files: lldb/include/lldb/DataFormatters/FormattersHelpers.h lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/Cocoa.cpp lldb/source/Plugins/Language/ObjC/Utilities.h lldb/unittests/Language/AppleObjC/CMakeLists.txt lldb/unittests/Language/AppleObjC/UtilitiesTests.cpp lldb/unittests/Language/CMakeLists.txt
Index: lldb/unittests/Language/CMakeLists.txt =================================================================== --- lldb/unittests/Language/CMakeLists.txt +++ lldb/unittests/Language/CMakeLists.txt @@ -1,2 +1,6 @@ add_subdirectory(CPlusPlus) add_subdirectory(Highlighting) + +if (APPLE) + add_subdirectory(AppleObjC) +endif() Index: lldb/unittests/Language/AppleObjC/UtilitiesTests.cpp =================================================================== --- /dev/null +++ lldb/unittests/Language/AppleObjC/UtilitiesTests.cpp @@ -0,0 +1,59 @@ +//===-- UtilitiesTests.cpp ------------------------------------------------===// +// +// 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 "Plugins/Language/ObjC/Utilities.h" +#include "lldb/Utility/StreamString.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/StringRef.h" +#include "gtest/gtest.h" +#include <string> +#include <cmath> + +using namespace lldb_private; + +// Try to format the date_value field of a NSDate. +static llvm::Optional<std::string> formatDateValue(double date_value) { + StreamString s; + bool succcess = formatters::NSDate::FormatDateValue(date_value, s); + if (succcess) + return std::string(s.GetData()); + return llvm::None; +} + +TEST(DataFormatterTest, NSDate) { + EXPECT_EQ(formatDateValue(-63114076800), + std::string("0001-12-30 00:00:00 +0000")); + + // Assert that std::llrint clamps values greater/lesser than the max/min + // time_t values. If it doesn't, it won't make sense to proceed, as we're + // relying on this behavior to avoid float-cast-overflow UB. + const double greater_than_max_time = 1e200; + const double lesser_than_min_time = -1e200; + ASSERT_LE(static_cast<time_t>(std::llrint(greater_than_max_time)), + std::numeric_limits<time_t>::max()); + ASSERT_LE(static_cast<time_t>(std::llrint(lesser_than_min_time)), + std::numeric_limits<time_t>::min()); + + // If the date_value `double` cannot be represented in a `time_t`, give up. + EXPECT_EQ(formatDateValue(greater_than_max_time), llvm::None); + EXPECT_EQ(formatDateValue(lesser_than_min_time), llvm::None); + + // If the date_value `double` plus the epoch cannot be represented in a + // `time_t`, give up. Try to craft a double that will trigger this scenario: + // if we can't do it, don't proceed. + const time_t date_value_to_trigger_overflow = + std::numeric_limits<time_t>::max() - formatters::GetOSXEpoch() + 1; + const double date_value_to_trigger_overflow_as_double = + static_cast<double>(date_value_to_trigger_overflow); + ASSERT_LE(date_value_to_trigger_overflow, + static_cast<time_t>(date_value_to_trigger_overflow_as_double)); + EXPECT_EQ(formatDateValue(date_value_to_trigger_overflow), llvm::None); + + EXPECT_EQ(formatDateValue(0), std::string("2001-01-01 00:00:00 UTC")); + EXPECT_EQ(formatDateValue(1024), std::string("2001-01-01 00:17:04 UTC")); +} Index: lldb/unittests/Language/AppleObjC/CMakeLists.txt =================================================================== --- /dev/null +++ lldb/unittests/Language/AppleObjC/CMakeLists.txt @@ -0,0 +1,6 @@ +add_lldb_unittest(LanguageAppleObjCTests + UtilitiesTests.cpp + + LINK_LIBS + lldbPluginObjCLanguage + ) Index: lldb/source/Plugins/Language/ObjC/Utilities.h =================================================================== --- /dev/null +++ lldb/source/Plugins/Language/ObjC/Utilities.h @@ -0,0 +1,32 @@ +//===-- Utilities.h ---------------------------------------------*- 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_PLUGINS_LANGUAGE_OBJC_UTILITIES_H +#define LLDB_PLUGINS_LANGUAGE_OBJC_UTILITIES_H + +#include <time.h> + +namespace lldb_private { + +class Stream; + +namespace formatters { + +/// Return an epoch time_t adjusted to the reference date set by the Cocoa +/// framework. +time_t GetOSXEpoch(); + +namespace NSDate { +/// Format the date_value field of a NSDate. +bool FormatDateValue(double date_value, Stream &stream); +} // namespace NSDate + +} // namespace formatters +} // namespace lldb_private + +#endif // LLDB_PLUGINS_LANGUAGE_OBJC_UTILITIES_H Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp =================================================================== --- lldb/source/Plugins/Language/ObjC/Cocoa.cpp +++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp @@ -8,6 +8,7 @@ #include "Cocoa.h" +#include "Plugins/Language/ObjC/Utilities.h" #include "Plugins/TypeSystem/Clang/TypeSystemClang.h" #include "lldb/Core/Mangled.h" #include "lldb/Core/ValueObject.h" @@ -27,11 +28,14 @@ #include "llvm/ADT/APInt.h" #include "llvm/ADT/bit.h" +#include "llvm/Support/CheckedArithmetic.h" #include "Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.h" #include "NSString.h" +#include <cmath> + using namespace lldb; using namespace lldb_private; using namespace lldb_private::formatters; @@ -785,6 +789,64 @@ return llvm::bit_cast<double>(decodedBits); } +// POSIX has an epoch on Jan-1-1970, but Cocoa prefers Jan-1-2001 +// this call gives the POSIX equivalent of the Cocoa epoch +time_t lldb_private::formatters::GetOSXEpoch() { + static time_t epoch = 0; +#ifdef __APPLE__ + if (!epoch) { + tzset(); + tm tm_epoch; + tm_epoch.tm_sec = 0; + tm_epoch.tm_hour = 0; + tm_epoch.tm_min = 0; + tm_epoch.tm_mon = 0; + tm_epoch.tm_mday = 1; + tm_epoch.tm_year = 2001 - 1900; + tm_epoch.tm_isdst = -1; + tm_epoch.tm_gmtoff = 0; + tm_epoch.tm_zone = nullptr; + epoch = timegm(&tm_epoch); + } +#endif // __APPLE__ + return epoch; +} + +bool lldb_private::formatters::NSDate::FormatDateValue(double date_value, + Stream &stream) { + if (date_value == -63114076800) { + stream.Printf("0001-12-30 00:00:00 +0000"); + return true; + } + + // Rely on implementation-defined behavior from std::llrint to clamp an + // out-of-range `double` value to a value representable by `time_t` without + // crashing LLDB with a floating-point exception. + // + // We're not distinguishing between the case where date_value "is" the max + // `time_t` value, and the case where std::llrint simply returns that value + // due to clamping, although technically we should be able to by inspecting + // the `math_errhandling` macro (which is set on a domain/overflow error). + const time_t date_value_as_time_t = + static_cast<time_t>(std::llrint(date_value)); + time_t epoch = GetOSXEpoch(); + if (auto osx_epoch = llvm::checkedAdd(epoch, date_value_as_time_t)) + epoch = *osx_epoch; + else + return false; + + tm *tm_date = gmtime(&epoch); + if (!tm_date) + return false; + std::string buffer(1024, 0); + if (strftime(&buffer[0], 1023, "%Z", tm_date) == 0) + return false; + stream.Printf("%04d-%02d-%02d %02d:%02d:%02d %s", tm_date->tm_year + 1900, + tm_date->tm_mon + 1, tm_date->tm_mday, tm_date->tm_hour, + tm_date->tm_min, tm_date->tm_sec, buffer.c_str()); + return true; +} + bool lldb_private::formatters::NSDateSummaryProvider( ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) { ProcessSP process_sp = valobj.GetProcessSP(); @@ -828,6 +890,16 @@ if (descriptor->GetTaggedPointerInfo(&info_bits, &value_bits)) { date_value_bits = ((value_bits << 8) | (info_bits << 4)); memcpy(&date_value, &date_value_bits, sizeof(date_value_bits)); + + // Accomodate the __NSTaggedDate format introduced in Foundation 1600. + if (class_name == g___NSTaggedDate) { + auto *apple_runtime = llvm::dyn_cast_or_null<AppleObjCRuntime>( + ObjCLanguageRuntime::Get(*process_sp)); + if (!apple_runtime) + return false; + if (apple_runtime->GetFoundationVersion() >= 1600) + date_value = decodeTaggedTimeInterval(value_bits << 4); + } } else { llvm::Triple triple( process_sp->GetTarget().GetArchitecture().GetTriple()); @@ -850,34 +922,7 @@ } else return false; - if (date_value == -63114076800) { - stream.Printf("0001-12-30 00:00:00 +0000"); - return true; - } - - // Accomodate for the __NSTaggedDate format introduced in Foundation 1600. - if (class_name == g___NSTaggedDate) { - auto *runtime = llvm::dyn_cast_or_null<AppleObjCRuntime>( - ObjCLanguageRuntime::Get(*process_sp)); - if (runtime && runtime->GetFoundationVersion() >= 1600) - date_value = decodeTaggedTimeInterval(value_bits << 4); - } - - // this snippet of code assumes that time_t == seconds since Jan-1-1970 this - // is generally true and POSIXly happy, but might break if a library vendor - // decides to get creative - time_t epoch = GetOSXEpoch(); - epoch = epoch + (time_t)date_value; - tm *tm_date = gmtime(&epoch); - if (!tm_date) - return false; - std::string buffer(1024, 0); - if (strftime(&buffer[0], 1023, "%Z", tm_date) == 0) - return false; - stream.Printf("%04d-%02d-%02d %02d:%02d:%02d %s", tm_date->tm_year + 1900, - tm_date->tm_mon + 1, tm_date->tm_mday, tm_date->tm_hour, - tm_date->tm_min, tm_date->tm_sec, buffer.c_str()); - return true; + return NSDate::FormatDateValue(date_value, stream); } bool lldb_private::formatters::ObjCClassSummaryProvider( @@ -1098,29 +1143,6 @@ return true; } -// POSIX has an epoch on Jan-1-1970, but Cocoa prefers Jan-1-2001 -// this call gives the POSIX equivalent of the Cocoa epoch -time_t lldb_private::formatters::GetOSXEpoch() { - static time_t epoch = 0; - if (!epoch) { -#ifndef _WIN32 - tzset(); - tm tm_epoch; - tm_epoch.tm_sec = 0; - tm_epoch.tm_hour = 0; - tm_epoch.tm_min = 0; - tm_epoch.tm_mon = 0; - tm_epoch.tm_mday = 1; - tm_epoch.tm_year = 2001 - 1900; - tm_epoch.tm_isdst = -1; - tm_epoch.tm_gmtoff = 0; - tm_epoch.tm_zone = nullptr; - epoch = timegm(&tm_epoch); -#endif - } - return epoch; -} - template bool lldb_private::formatters::NSDataSummaryProvider<true>( ValueObject &, Stream &, const TypeSummaryOptions &); Index: lldb/source/Plugins/Language/ObjC/CF.cpp =================================================================== --- lldb/source/Plugins/Language/ObjC/CF.cpp +++ lldb/source/Plugins/Language/ObjC/CF.cpp @@ -8,6 +8,7 @@ #include "CF.h" +#include "Plugins/Language/ObjC/Utilities.h" #include "Plugins/TypeSystem/Clang/TypeSystemClang.h" #include "lldb/Core/ValueObject.h" #include "lldb/Core/ValueObjectConstResult.h" Index: lldb/include/lldb/DataFormatters/FormattersHelpers.h =================================================================== --- lldb/include/lldb/DataFormatters/FormattersHelpers.h +++ lldb/include/lldb/DataFormatters/FormattersHelpers.h @@ -58,8 +58,6 @@ lldb::ValueObjectSP GetValueOfLibCXXCompressedPair(ValueObject &pair); -time_t GetOSXEpoch(); - struct InferiorSizedWord { InferiorSizedWord(const InferiorSizedWord &word) : ptr_size(word.ptr_size) {
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
