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

[lldb][libc++] Adds system_clock data formatters. #78609

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

mordante
Copy link
Member

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 18, 2024

@llvm/pr-subscribers-lldb

Author: Mark de Wever (mordante)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/78609.diff

5 Files Affected:

  • (modified) lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp (+25)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp (+63-2)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.h (+8)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py (+100)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp (+52)
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index 21c73e6361904e5..f0fe6c9e06d9d47 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -1032,6 +1032,31 @@ static void LoadLibCxxFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
       TypeSummaryImplSP(new StringSummaryFormat(
           eTypeOptionHideChildren | eTypeOptionHideValue, "${var.__rep_} s")));
 
+  // Chrono time point types
+
+  AddCXXSummary(cpp_category_sp,
+                lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider,
+                "libc++ std::chrono::sys_seconds summary provider",
+                "^std::__[[:alnum:]]+::chrono::time_point<"
+                "std::__[[:alnum:]]+::chrono::system_clock, "
+                "std::__[[:alnum:]]+::chrono::duration<long long, "
+                "std::__[[:alnum:]]+::ratio<1, 1> "
+                "> >$",
+                eTypeOptionHideChildren | eTypeOptionHideValue |
+                    eTypeOptionCascade,
+                true);
+  AddCXXSummary(cpp_category_sp,
+                lldb_private::formatters::LibcxxChronoSysDaysSummaryProvider,
+                "libc++ std::chrono::sys_seconds summary provider",
+                "^std::__[[:alnum:]]+::chrono::time_point<"
+                "std::__[[:alnum:]]+::chrono::system_clock, "
+                "std::__[[:alnum:]]+::chrono::duration<int, "
+                "std::__[[:alnum:]]+::ratio<86400, 1> "
+                "> >$",
+                eTypeOptionHideChildren | eTypeOptionHideValue |
+                    eTypeOptionCascade,
+                true);
+
   // Chrono calendar types
 
   cpp_category_sp->AddTypeSummary(
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index d232a38adc029ad..042e22626969987 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -1073,18 +1073,79 @@ bool lldb_private::formatters::LibcxxWStringViewSummaryProvider(
   bool success;
   ValueObjectSP dataobj;
   size_t size;
-  std::tie( success, dataobj, size ) = LibcxxExtractStringViewData(valobj);
+  std::tie(success, dataobj, size) = LibcxxExtractStringViewData(valobj);
 
   if (!success) {
     stream << "Summary Unavailable";
     return true;
   }
 
-
   return ::LibcxxWStringSummaryProvider(valobj, stream, summary_options,
                                         dataobj, size);
 }
 
+bool lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
+    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
+  ValueObjectSP ptr_sp = valobj.GetChildMemberWithName("__d_");
+  if (!ptr_sp)
+    return false;
+  ptr_sp = ptr_sp->GetChildMemberWithName("__rep_");
+  if (!ptr_sp)
+    return false;
+
+  // The date time in the chrono library is valid in the range
+  // [-32767-01-01T00:00:00Z, 32767-12-31T23:59:59Z]. A 64-bit time_t has a
+  // larger range, the function strftime is not able to format the entire range
+  // of time_t. The exact point has not been investigated; it's limited to
+  // chrono's range.
+  const std::time_t chrono_timestamp_min =
+      -1'096'193'779'200; // -32767-01-01T00:00:00Z
+  const std::time_t chrono_timestamp_max =
+      971'890'963'199; // 32767-12-31T23:59:59Z
+
+  const std::time_t seconds = ptr_sp->GetValueAsSigned(0);
+  if (seconds < chrono_timestamp_min || seconds > chrono_timestamp_max)
+    stream.Printf("timestamp=%ld s", seconds);
+  else {
+    std::array<char, 128> str;
+    std::strftime(str.data(), str.size(), "%FT%H:%M:%SZ", gmtime(&seconds));
+    stream.Printf("date/time=%s timestamp=%ld s", str.data(), seconds);
+  }
+
+  return true;
+}
+
+bool lldb_private::formatters::LibcxxChronoSysDaysSummaryProvider(
+    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
+  ValueObjectSP ptr_sp = valobj.GetChildMemberWithName("__d_");
+  if (!ptr_sp)
+    return false;
+  ptr_sp = ptr_sp->GetChildMemberWithName("__rep_");
+  if (!ptr_sp)
+    return false;
+
+  // The date time in the chrono library is valid in the range
+  // [-32767-01-01Z, 32767-12-31Z]. A 32-bit time_t has a larger range, the
+  // function strftime is not able to format the entire range of time_t. The
+  // exact point has not been investigated; it's limited to chrono's range.
+  const int chrono_timestamp_min = -12'687'428; // -32767-01-01Z
+  const int chrono_timestamp_max = 11'248'737;  // 32767-12-31Z
+
+  const int days = ptr_sp->GetValueAsSigned(0);
+  if (days < chrono_timestamp_min || days > chrono_timestamp_max)
+    stream.Printf("timestamp=%d days", days);
+
+  else {
+    const std::time_t seconds = std::time_t(86400) * days;
+
+    std::array<char, 128> str;
+    std::strftime(str.data(), str.size(), "%FZ", gmtime(&seconds));
+    stream.Printf("date=%s timestamp=%d days", str.data(), days);
+  }
+
+  return true;
+}
+
 bool lldb_private::formatters::LibcxxChronoMonthSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   // FIXME: These are the names used in the C++20 ostream operator. Since LLVM
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
index 532d185b18543f5..72da6b2426efec8 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -261,6 +261,14 @@ SyntheticChildrenFrontEnd *
 LibcxxStdRangesRefViewSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                                lldb::ValueObjectSP);
 
+bool LibcxxChronoSysSecondsSummaryProvider(
+    ValueObject &valobj, Stream &stream,
+    const TypeSummaryOptions &options); // libc++ std::chrono::sys_seconds
+
+bool LibcxxChronoSysDaysSummaryProvider(
+    ValueObject &valobj, Stream &stream,
+    const TypeSummaryOptions &options); // libc++ std::chrono::sys_days
+
 bool LibcxxChronoMonthSummaryProvider(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &options); // libc++ std::chrono::month
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
index d4bc140015fbb71..67372572dc4464e 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
@@ -32,6 +32,106 @@ def test_with_run_command(self):
         self.expect("frame variable m", substrs=["m = 4321 months"])
         self.expect("frame variable y", substrs=["y = 321 years"])
 
+        self.expect(
+            "frame variable ss_tp",
+            substrs=["ss_tp = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
+        )
+        self.expect(
+            "frame variable ss_tp_d",
+            substrs=["ss_tp_d = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
+        )
+        self.expect(
+            "frame variable ss_tp_d_r",
+            substrs=["ss_tp_d_r = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
+        )
+        self.expect(
+            "frame variable ss_tp_d_r2",
+            substrs=["ss_tp_d_r2 = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
+        )
+
+        self.expect(
+            "frame variable ss_0",
+            substrs=["ss_0 = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
+        )
+
+        self.expect(
+            "frame variable ss_neg_date_time",
+            substrs=[
+                "ss_neg_date_time = date/time=-32767-01-01T00:00:00Z timestamp=-1096193779200 s"
+            ],
+        )
+        self.expect(
+            "frame variable ss_neg_seconds",
+            substrs=["ss_neg_seconds = timestamp=-1096193779201 s"],
+        )
+
+        self.expect(
+            "frame variable ss_pos_date_time",
+            substrs=[
+                "ss_pos_date_time = date/time=32767-12-31T23:59:59Z timestamp=971890963199 s"
+            ],
+        )
+        self.expect(
+            "frame variable ss_pos_seconds",
+            substrs=["ss_pos_seconds = timestamp=971890963200 s"],
+        )
+
+        self.expect(
+            "frame variable ss_min",
+            substrs=["ss_min = timestamp=-9223372036854775808 s"],
+        )
+        self.expect(
+            "frame variable ss_max",
+            substrs=["ss_max = timestamp=9223372036854775807 s"],
+        )
+
+        self.expect(
+            "frame variable sd_tp",
+            substrs=["sd_tp = date=1970-01-01Z timestamp=0 days"],
+        )
+        self.expect(
+            "frame variable sd_tp_d_r",
+            substrs=["sd_tp_d_r = date=1970-01-01Z timestamp=0 days"],
+        )
+        self.expect(
+            "frame variable sd_tp_d_r2",
+            substrs=["sd_tp_d_r2 = date=1970-01-01Z timestamp=0 days"],
+        )
+
+        self.expect(
+            "frame variable sd_0", substrs=["sd_0 = date=1970-01-01Z timestamp=0 days"]
+        )
+        self.expect(
+            "frame variable sd_neg_date",
+            substrs=["sd_neg_date = date=-32767-01-01Z timestamp=-12687428 days"],
+        )
+        self.expect(
+            "frame variable sd_neg_days",
+            substrs=["sd_neg_days = timestamp=-12687429 days"],
+        )
+
+        self.expect(
+            "frame variable sd_pos_date",
+            substrs=["sd_pos_date = date=32767-12-31Z timestamp=11248737 days"],
+        )
+        self.expect(
+            "frame variable sd_pos_days",
+            substrs=["sd_pos_days = timestamp=11248738 days"],
+        )
+
+        self.expect(
+            "frame variable sd_min",
+            substrs=["sd_min = timestamp=-2147483648 days"],
+        )
+        self.expect(
+            "frame variable sd_max",
+            substrs=["sd_max = timestamp=2147483647 days"],
+        )
+
+        # self.expect("frame variable sd_min", substrs=["sd_min = date=1970-01-01Z timestamp=0 days"])
+        # self.expect("frame variable sd_0", substrs=["sd_0 = date=1970-01-01Z timestamp=0 days"])
+        # self.expect("frame variable XXXtp_d_0", substrs=["XXXtp_d_0 = date=1970-01-01Z timestamp=0 days"])
+
         self.expect("frame variable d_0", substrs=["d_0 = day=0"])
         self.expect("frame variable d_1", substrs=["d_1 = day=1"])
         self.expect("frame variable d_31", substrs=["d_31 = day=31"])
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp
index 57215aaf343f649..315c88a787d823d 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp
@@ -15,6 +15,58 @@ int main() {
   std::chrono::months m{4321};
   std::chrono::years y{321};
 
+  // sys_seconds aliasses
+  std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
+      ss_tp{std::chrono::seconds{0}};
+  std::chrono::time_point<std::chrono::system_clock,
+                          std::chrono::duration<long long>>
+      ss_tp_d{std::chrono::seconds{0}};
+  std::chrono::time_point<std::chrono::system_clock,
+                          std::chrono::duration<long long, std::ratio<1>>>
+      ss_tp_d_r{std::chrono::seconds{0}};
+  std::chrono::time_point<std::chrono::system_clock,
+                          std::chrono::duration<long long, std::ratio<1>>>
+      ss_tp_d_r2{std::chrono::seconds{0}};
+
+  // sys_seconds
+  std::chrono::sys_seconds ss_0{std::chrono::seconds{0}};
+  std::chrono::sys_seconds ss_neg_date_time{
+      std::chrono::seconds{-1'096'193'779'200}};
+  std::chrono::sys_seconds ss_neg_seconds{
+      std::chrono::seconds{-1'096'193'779'201}};
+  std::chrono::sys_seconds ss_pos_date_time{
+      std::chrono::seconds{971'890'963'199}};
+  std::chrono::sys_seconds ss_pos_seconds{
+      std::chrono::seconds{971'890'963'200}};
+  std::chrono::sys_seconds ss_min{
+      std::chrono::seconds{std::numeric_limits<long long>::min()}};
+  std::chrono::sys_seconds ss_max{
+      std::chrono::seconds{std::numeric_limits<long long>::max()}};
+
+  // sys_days aliasses
+  std::chrono::time_point<std::chrono::system_clock, std::chrono::days> sd_tp{
+      std::chrono::days{0}};
+  std::chrono::time_point<std::chrono::system_clock,
+                          std::chrono::duration<int, std::ratio<86400>>>
+      sd_tp_d_r{std::chrono::days{0}};
+  std::chrono::time_point<std::chrono::system_clock,
+                          std::chrono::duration<int, std::ratio<86400, 1>>>
+      sd_tp_d_r2{std::chrono::days{0}};
+
+  // sys_days
+  std::chrono::sys_days sd_0{std::chrono::days{0}};
+
+  std::chrono::sys_days sd_neg_date{std::chrono::days{-12'687'428}};
+  std::chrono::sys_days sd_neg_days{std::chrono::days{-12'687'429}};
+
+  std::chrono::sys_days sd_pos_date{std::chrono::days{11'248'737}};
+  std::chrono::sys_days sd_pos_days{std::chrono::days{11'248'738}};
+
+  std::chrono::sys_days sd_min{
+      std::chrono::days{std::numeric_limits<int>::min()}};
+  std::chrono::sys_days sd_max{
+      std::chrono::days{std::numeric_limits<int>::max()}};
+
   std::chrono::day d_0{0};
   std::chrono::day d_1{1};
   std::chrono::day d_31{31};

const std::time_t seconds = std::time_t(86400) * days;

std::array<char, 128> str;
std::strftime(str.data(), str.size(), "%FZ", gmtime(&seconds));
Copy link
Member

Choose a reason for hiding this comment

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

is it worth checking the return of strftime here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll them, but they should never trigger.

substrs=["sd_max = timestamp=2147483647 days"],
)

# self.expect("frame variable sd_min", substrs=["sd_min = date=1970-01-01Z timestamp=0 days"])
Copy link
Member

Choose a reason for hiding this comment

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

Why are these commented?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch, leftover copy-pasted

Copy link
Member

@Michael137 Michael137 left a comment

Choose a reason for hiding this comment

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

LGTM (with minor questions)

@mordante mordante merged commit bfd12f3 into llvm:main Jan 22, 2024
4 checks passed
@mordante mordante deleted the lldb_system_clock_data_formatters branch January 22, 2024 19:25
@jasonmolenda
Copy link
Collaborator

@mordante I'm seeing failures on the macOS bots (and on my desktop) with TestDataFormatterLibcxxChrono.py, the test

    self.expect(
        "frame variable ss_neg_date_time",
        substrs=[
            "ss_neg_date_time = date/time=-32767-01-01T00:00:00Z timestamp=-1096193779200 s"
        ],
    )

but we're getting an unsigned timestamp from lldb instead:

(lldb) fr v ss_neg_date_time
(std::chrono::sys_seconds) ss_neg_date_time = date/time=-32767-01-01T00:00:00Z timestamp=18446742977515772416 s

so the test fails, from the %ld formater in formatters::LibcxxChronoSysSecondsSummaryProvider I suppose. Both of these are formattings of the value 0xffffff00c5c25a00, I'm honestly not sure why it's formatted as an unsigned value on macOS for me, I'm guessing it isn't on linux or some platform?

jasonmolenda added a commit that referenced this pull request Jan 26, 2024
On macOS, the formatter is printing signed values as
unsigned, it seems, and the tests are expecting correctly
signed values.  These tests were added in
#78609
@jasonmolenda
Copy link
Collaborator

I commented out the two tests that are failing on macOS temporarily so you'd have a chance to look at this. If you don't have access to macOS (I assume this is working on Linux or whatever), @Michael137 or I can look into it. Thanks.

commit ba45ad160e3f329aeb02c19eaf18af27fa423d85
Author: Jason Molenda <jmolenda@apple.com>
Date:   Thu Jan 25 16:30:14 2024 -0800

    Temporarily disable two libcxx chrono formatter tests

@mordante
Copy link
Member Author

mordante commented Feb 4, 2024

@jasonmolenda I'm not using a mac, but based on your results I expect that std::size_t is a 64-bit unsigned integer type. Is that correct?

@Michael137
Copy link
Member

Michael137 commented Feb 4, 2024

tbf, we are casting the value to an uint64_t and printing it with a PRIu64, right?:

stream.Printf("timestamp=%" PRIu64 " s", static_cast<uint64_t>(seconds))

So how would we ever expect a -1096193779200 here? Are you sure this was working correctly on Linux @mordante ? (I don't have a linux machine set up right now to quickly test this on)

@jasonmolenda
Copy link
Collaborator

Agreed, if I change this to

diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index d0bdbe1fd4d..b37544f6dd3 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -1105,7 +1105,7 @@ bool lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
 
   const std::time_t seconds = ptr_sp->GetValueAsSigned(0);
   if (seconds < chrono_timestamp_min || seconds > chrono_timestamp_max)
-    stream.Printf("timestamp=%" PRIu64 " s", static_cast<uint64_t>(seconds));
+    stream.Printf("timestamp=%" PRId64 " s", static_cast<int64_t>(seconds));
   else {
     std::array<char, 128> str;
     std::size_t size =
@@ -1113,8 +1113,8 @@ bool lldb_private::formatters::LibcxxChronoSysSecondsSummaryProvider(
     if (size == 0)
       return false;
 
-    stream.Printf("date/time=%s timestamp=%" PRIu64 " s", str.data(),
-                  static_cast<uint64_t>(seconds));
+    stream.Printf("date/time=%s timestamp=%" PRId64 " s", str.data(),
+                  static_cast<int64_t>(seconds));
   }
 
   return true;

the tests pass on Darwin. On Darwin time_t is long, a signed value - and given that this can represent negative times, that makes sense. The test is checking that PRIu64 prints a negative value but we're casting this time_t to a uint64_t, I don't understand why this would print as a negative value on Linux (and pass the tests).

jasonmolenda added a commit to jasonmolenda/llvm-project that referenced this pull request Feb 8, 2024
This formatter
llvm#78609
was originally passing the signed seconds (which can refer to times
in the past) with an unsigned printf formatter, and had tests that
expected to see negative values from the printf which always failed
on macOS.  I'm not clear how they ever passed on any platform.

Fix the printf to print seconds as a signed value, and re-enable
the tests.
@jasonmolenda
Copy link
Collaborator

Created a new PR with my proposed fix #81078

jasonmolenda added a commit that referenced this pull request Feb 8, 2024
This formatter
#78609
was originally passing the signed seconds (which can refer to times in
the past) with an unsigned printf formatter, and had tests that expected
to see negative values from the printf which always failed on macOS. I'm
not clear how they ever passed on any platform.

Fix the printf to print seconds as a signed value, and re-enable the
tests.
Michael137 pushed a commit to Michael137/llvm-project that referenced this pull request Feb 16, 2024
Michael137 pushed a commit to Michael137/llvm-project that referenced this pull request Feb 16, 2024
This formatter
llvm#78609
was originally passing the signed seconds (which can refer to times in
the past) with an unsigned printf formatter, and had tests that expected
to see negative values from the printf which always failed on macOS. I'm
not clear how they ever passed on any platform.

Fix the printf to print seconds as a signed value, and re-enable the
tests.

(cherry picked from commit f219cda)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants