On Mon, Jun 6, 2022 at 5:49 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
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...