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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
