Remove undefined behavior from npy_datetimestruct_to_datetime

[WillAyd]

This helps the readability of things as is, but also helps us better implement bounds checking in a later PR to avoid overflow

[jbrockmendel]

The current implementation is vendored from numpy, which recently expose this for us in their C API so we can de-duplicate it once our minimum version catches up.

[WillAyd]

I think we can upstream this again once we have the UB cleaned up. It's a few years out and numpy doesn't have the amount of tests hitting this as we do right?

[WillAyd]

@jbrockmendel I took this a bit further so you can see where this would head. gcc/clang/msvc all provide builtin functions to check for overflow, which can even be dispatched down at the hardware level. By leveraging those functions, we can avoid introducing any undefined behavior.

The next evolution of this could get rid of the check_dts_bounds function and just rely on the libraries / standard Python error handling, which should help rip out a lot of code and potentially avoid some errors in the checks

[WillAyd]

Hmm so going through this it turns out that our current bounds checking isn't quite correct.

>>> pd.Timestamp.min
Timestamp('1677-09-21 00:12:43.145224193')

This gets stored as a struct with the following members / values:

{year = 1677, month = 9, day = 21, hour = 0, min = 12, sec = 43, us = 145224, ps = 193000, as = 0}

On main the switch case to convert this to a nanosecond resolution is:

            case NPY_FR_ps:
                ret = ((((days * 24 + dts->hour) * 60 + dts->min) * 60 +
                        dts->sec) *
                           1000000 +
                       dts->us) *
                          1000000 +
                      dts->ps;

Converting this to a Python function with the appropriate values:

>>> days = -106752
>>> hour = 0
>>> min = 12
>>> sec = 43
>>> us = 145224
>>> ((((days * 24 + hour) * 60 + min) * 60 + sec) * 1000000 + us) * 1000
-9223372036854776000

Which exceeds the limits of an int64.

Since the numpy datetimestruct does not store ns but we instead need to multiply us * 1000 to get that value, I think we need to adjust our Timestamp.min / Timestamp.max to be +1000/-1000 ns respectively to avoid overflow

[jbrockmendel]

nitpick e->err. i dislike 1-letter names

[jbrockmendel]

just have pydate_to_dt64 raise OutOfBoundsDatetime to avoid having to re-raise?

[WillAyd]

Yea giving the error handling a better pass as we speak. Thanks for the idea

[jbrockmendel]

On main the switch case to convert this to a nanosecond resolution is: [...] case NPY_FR_ps

we dont use FR_ps anywhere. is this reachable?

[jbrockmendel]

big picture im pretty happy to defer to @WillAyd on this, would like someone with stronger c-fu than me to review the c edits.

[WillAyd]

we dont use FR_ps anywhere. is this reachable?

Yea sorry typo on my end. But the problem exists in the NPY_FR_ns block as well. Essentially multiplying us * 1000 when you are within 1 microsecond of the theoretical storage limit will overflow

[jbrockmendel]

I'll prioritize taking another look this afternoon

[jbrockmendel]

might be worth doing this as a preliminary. id be much more comfortable merging that as a standalone without the C stuff

[WillAyd]

Fair enough. I just took care of that here because its pretty small, but can split out if a deal breaker

[jbrockmendel]

Not a deal breaker, just easier for me to merge promptly

[mroeschke]

LGTM

[WillAyd]

Any objections on this one @jbrockmendel or good to merge?

[jbrockmendel]

taking another look now

[jbrockmendel]

should the return here be str?

[WillAyd]

No reason - didn't think much of it. Makes sense though - just pushed this up

[jbrockmendel]

One nitpick, no objections. Haven't looked at the C code that closely

[mroeschke]

Thanks @WillAyd

[jbrockmendel]

I just started seeing a bunch of

  ../../pandas/_libs/src/vendored/numpy/datetime/np_datetime.c:355:40: warning: incompatible pointer types passing 'int64_t *' (aka 'long long *') to parameter of type 'long *' [-Wincompatible-pointer-types]
    return checked_int64_sub(year, 1970, result);
                                         ^~~~~~
  ../../pandas/_libs/src/vendored/numpy/datetime/np_datetime.c:45:69: note: expanded from macro 'checked_int64_sub'
  #define checked_int64_sub(a, b, res) __builtin_ssubl_overflow(a, b, res)
                                                                      ^~~
  ../../pandas/_libs/src/vendored/numpy/datetime/np_datetime.c:359:39: warning: incompatible pointer types passing 'int64_t *' (aka 'long long *') to parameter of type 'long *' [-Wincompatible-pointer-types]
    return checked_int64_mul(years, 12, result);
                                        ^~~~~~
  ../../pandas/_libs/src/vendored/numpy/datetime/np_datetime.c:46:69: note: expanded from macro 'checked_int64_mul'
  #define checked_int64_mul(a, b, res) __builtin_smull_overflow(a, b, res)

[truncated]

can/should we do something about this?

[WillAyd]

Yea we should. What platform are you on?

[jbrockmendel]

Mac (pre-M1)

[WillAyd]

Ok thanks. The pre-processor condition on line 43 has to be to blame. Will see what I can find for macOS (hoping it's in CI logs too).

In your case if you deleted that #if directive and kept the true branch I think it would resolve your issues

[WillAyd]

#55977 should be a more permanent fix

[jbrockmendel]

Two other follow-up requests that are not time-sensitive but i want to make sure don't fall through the cracks: 1) try to upstream this to numpy so our versions dont diverge from theirs, 2) deduplicate the "catch OverflowError and re-raise as OutOfBoundsDatetime"

update: would it be difficult to define OutOfBoundsDatetime in the C code instead of in the cython?

[WillAyd]

Yea I agree moving that exception down to the extension would be great - just need to spend more time on that