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

[Vedant Kumar via lldb-commits]

There were several things off here, both in the tests and the Cocoa.cpp change. Sorry for all the breakage.

I’ve updated https://reviews.llvm.org/D80150 <https://reviews.llvm.org/D80150%C2%A0to> to address these issues and written up a summary there of what went wrong.

vedant

> On May 20, 2020, at 9:23 AM, Eric Christopher <echristo at gmail.com> wrote:
> 
> Agreed. Something is off here. My change was only to silence a few warnings, but they're definitely highlighting a conversion issue. What's up with NSDate conversions here. Does the API have a way to convert from time_t?
> 
> On Wed, May 20, 2020, 2:07 AM Pavel Labath via Phabricator via lldb-commits <lldb-commits at lists.llvm.org <mailto:lldb-commits at lists.llvm.org>> wrote:
> labath added a comment.
> 
> In D80150#2045364 <https://reviews.llvm.org/D80150#2045364 <https://reviews.llvm.org/D80150#2045364>>, @vsk wrote:
> 
> > @labath Agreed on all points, I've addressed the feedback in 82dbf4aca84 <https://reviews.llvm.org/rG82dbf4aca84ec889d0dc390674ff44e30441bcfd <https://reviews.llvm.org/rG82dbf4aca84ec889d0dc390674ff44e30441bcfd>> by moving "DataFormatters/Mock.h" to "Plugins/Language/ObjC/Utilities.h", and adding a separate LanguageObjCTests unit test.
> 
> 
> Cool. Thanks.
> 
> 
> 
> ================
> 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);
> ----------------
> 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 <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).
> 
> 
> Repository:
>   rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D80150/new/ <https://reviews.llvm.org/D80150/new/>
> 
> https://reviews.llvm.org/D80150 <https://reviews.llvm.org/D80150>
> 
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org <mailto:lldb-commits at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits <https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200522/9f7265a8/attachment.html>