Skip to content

[lldb] Fix printf formatting of std::time_t seconds #81078

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

Merged

Conversation

jasonmolenda
Copy link
Collaborator

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.

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.
@llvmbot
Copy link
Member

llvmbot commented Feb 8, 2024

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

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.


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

2 Files Affected:

  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp (+3-3)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py (+14-16)
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index d0bdbe1fd4d91..b37544f6dd3ad 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;
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 9706f9e94e922..a90fb828d121a 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
@@ -54,17 +54,16 @@ def test_with_run_command(self):
             substrs=["ss_0 = date/time=1970-01-01T00:00:00Z timestamp=0 s"],
         )
 
-        # FIXME disabled temporarily, macOS is printing this as an unsigned?
-        #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_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",
@@ -77,11 +76,10 @@ def test_with_run_command(self):
             substrs=["ss_pos_seconds = timestamp=971890963200 s"],
         )
 
-        # FIXME disabled temporarily, macOS is printing this as an unsigned?
-        #self.expect(
-        #    "frame variable ss_min",
-        #    substrs=["ss_min = timestamp=-9223372036854775808 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"],

Copy link
Collaborator

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

If we switched to a streamstring or a raw_svector_ostream we could avoid having to deal with the printf specifiers altogether...

@jasonmolenda jasonmolenda merged commit f219cda into llvm:main Feb 8, 2024
@jasonmolenda jasonmolenda deleted the fix-stdtime_t-data-format-test branch February 8, 2024 17:16
@mordante
Copy link
Member

Thanks for the fix!

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)
SquallATF pushed a commit to SquallATF/llvm-project that referenced this pull request Jun 30, 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.
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.

5 participants