[Lldb-commits] [PATCH] D80150: [lldb/DataFormatter] Check for overflow when finding NSDate epoch

[Vedant Kumar via Phabricator via lldb-commits]

vsk marked 2 inline comments as done and an inline comment as not done.
vsk added inline comments.


================
Comment at: lldb/unittests/DataFormatter/MockTests.cpp:30
+  // Can't convert the date_value to a time_t.
+  EXPECT_EQ(formatDateValue(std::numeric_limits<time_t>::max() + 1),
+            llvm::None);
----------------
labath wrote:
> vsk wrote:
> > labath wrote:
> > > Isn't this actually `std::numeric_limits<time_t>::min()` (and UB due to singed wraparound) ? Did you want to convert to double before doing the `+1` ?
> > Yes, thank you! It looks like Eric caught this before I did.
> Actually, thinking about that further, (for 64-bit `time_t`s), `double(numeric_limits<time_t>::max())` is [[ https://godbolt.org/z/t3iSd7 | exactly the same value ]] as `double(numeric_limits<time_t>::max())+1.0` because `double` doesn't have enough bits to represent the value precisely. So, I have a feeling these checks are still not testing the exact thing you want to test (though I'm not sure what that is exactly).
Hm, that's right. What I'm trying to do is look for float-cast UB in two places: first, when we initially convert the `double` "date_value" to `time_t`, and second, when we add the OSXEpoch to that `time_t`.

I'll take a fresh cut at this. For the first conversion, I think I can rely on implementation-defined behavior from std::llrint to do the conversion without crashing LLDB due to a floating-point exception. For the second, we have `checkedAdd` from llvm, but the tricky part is testing it: to do that, I'm crafting a special `time_t` that should trigger overflow, and asserting that a round-trip conversion to/from `double` doesn't break the property.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80150/new/

https://reviews.llvm.org/D80150