On 6/6/22 15:56, enh wrote:
> it's your mktime(), so you tell me :-)
tzdb mktime doesn't make any attempt to preserve errno on success. Even
on a platform like bionic with primitives following a
preserve-errno-on-success philosophy, tzdb functions can succeed after
calling other functions that may fail and set errno. For example,
tzset_unlocked can recover from a failed zoneinit (which sets errno) by
calling zoneinit again.
> in bionic we use RAII to make functions "errno clean" all
> the time to avoid this kind of problem because it's too hard to reason
> about...)
If we wanted to make tzdb localtime.c and strftime.c "errno clean" it'd
require some careful work. It'd be doable but I expect it's not a
one-hour job (though perhaps you could prove me wrong :-).
i think worse than that, it's just too hard to reason about --- we'd bitrot anyway.
it's really not obvious that there's a good fix (beyond "wait for 32-bit to die") at all...
i don't think we can easily fix mktime64() --- i think we'd basically need to fix it differently, so we built tzcode twice (once with the public ABI 32-bit time_t and again with the 64-bit time_t), which at the very least would be quite a behavior change for any code using the 64-bit stuff. we'd also have to change the const-ness of a few arguments, which could be a breaking change. (i mean, it _is_ a breaking change; the question is really "how many apps would break?".)
i don't think we can really go the errno route. you've convinced me of that.
i don't see how we can save the localtime_r() comparisons --- our fundamental problem is that we don't have anything from mktime64() to compare against.
i think the safest choice for bionic is probably just "disable the new check and preserve the old behavior". given that you can only get into this mess by passing a bad `struct tm` to strftime(), and given that most use cases i'm aware of have literally just asked us to fill out that `struct tm` from a `time_t`, so -1 is almost certain to be "legit -1" rather than "error -1", i think i'm fine with that? especially since we haven't seen/heard any complaints about the existing behavior (we haven't yet shipped
https://github.com/eggert/tz/commit/8b238ec54c09556eb2aa405c1741eedfd12c4a87, despite it being from 2020 --- it's mingaleev's work trying to get back in sync that uncovered this).
it sounds from the checkin comment like there wasn't a motivating example for this behavior change, but let us know if you can think of an actual downside to not taking this change...