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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't know how widespread the llrint behavior is. Clamping to the largest/smallest int seems reasonable, so it might be reasonable to depend on that -- though I don't know what will that do for NaNs...

I actually don't think the previous approach was wrong. IIUC, it only became a problem when Eric added an explicit int cast to the input value -- it should have been done the other way around, casting the integer to the float type.  Then we'd just need to be careful in the test to select a number that is sufficiently bigger than UINT64_MAX, so that the difference is observable after conversion to floating point.

In  a way, I think that the real problem here is that we're using host floating point arithmetic on numbers that are: a) untrusted; b) come from a system which has potentially different floating point encoding/semantics.  A more principled approach would be to do it like the compiler, and work with APFloats with semantics explicitly configured from based on the target.



================
Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:796
+  static time_t epoch = 0;
+#ifdef __APPLE__
+  if (!epoch) {
----------------
I think that changing !_WIN32 to __APPLE__ is a step in the wrong direction. Ideally, this function should work the same everywhere. The way to achieve that would be to have a host function which selects between `gmtime` and `_mkgmtime` depending on the platform. But failing that, I don't see a reason to restrict the scope of this further.


================
Comment at: lldb/unittests/Language/AppleObjC/UtilitiesTests.cpp:43
+  // If the date_value `double` cannot be represented in a `time_t`, give up.
+  EXPECT_EQ(formatDateValue(greater_than_max_time), llvm::None);
+  EXPECT_EQ(formatDateValue(lesser_than_min_time), llvm::None);
----------------
What about infinities and NaNs? (I can believe this will do the right thing for the former, but I have no clue what will happen in the NaN case).


================
Comment at: lldb/unittests/Language/CMakeLists.txt:4-6
+if (APPLE)
+  add_subdirectory(AppleObjC)
+endif()
----------------
I don't think the distinction between AppleObjC and ObjC is very clear, but if the code under test lives in `ObjC`, I think it makes sense to call the test folder the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80150