Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<xloctime>: apply two-digit year logic only if exactly two digits are read #2666

Merged
merged 6 commits into from
May 1, 2022

Conversation

MattStephanson
Copy link
Contributor

Fixes #2618.

ABI note: this replaces a call to the separately-compiled function _Getint with a call to the header-only function _Getint_v2. The two functions are virtually identical, so _Getint is rewritten as a wrapper that discards the number of digits read. I think both of these changes are safe, but want to highlight them for closer scrutiny.

@MattStephanson MattStephanson requested a review from a team as a code owner April 23, 2022 03:41
@MattStephanson
Copy link
Contributor Author

MattStephanson commented Apr 23, 2022

libc++ applies the two-digit logic even when one digit is read:

https://github.com/llvm/llvm-project/blob/b8d38e8b4fcab071c5c4cb698e154023d06de69e/libcxx/test/std/localization/locale.categories/category.time/locale.time.get/locale.time.get.members/get_year.pass.cpp#L40-L48

I'm not sure I agree with that, but should be go with it for compatibility?

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 26, 2022
@StephanTLavavej
Copy link
Member

libc++ applies the two-digit logic even when one digit is read:
I'm not sure I agree with that, but should be go with it for compatibility?

I agree that seems questionable. I'd recommend checking libstdc++'s behavior (not their source code) - if it does the same thing, then we may as well avoid implementation divergence. If libc++ and libstdc++ disagree, I think the compatibility argument is less compelling, and we should instead probably skip the affected test for now, and report it to libc++.

@MattStephanson
Copy link
Contributor Author

The libc++ logic appears to be as follows:

  • %y: read up to 4 digits, apply 2-digit logic regardless of how many digits are read.
  • %Y: read up to 4 digits, no post-processing
  • time_get::do_get_year: same as %y

MSVC is:

  • %y read up to 2 digits, apply 2-digit logic
  • %Y: call get_year
  • time_get::do_get_year: read up to 4 digits, apply 2-digit logic regardless of how many digits are read.

Here's the behavior of %y that I'm seeing (https://godbolt.org/z/P5zcjMb35):

String libc++ libstdc++ strptime
"1" 2001 2001 2001
"01" 2001 2001 2001
"85" 1985 1985 1985
"085" 1985 85 2008
"111" 111 111 2011

There's unanimous agreement about applying the 2-digit rule for single-digit results, so I'll follow that.

More surprising to me is that both libc++ and libstdc++ will read more than two digits for %y. If I were working from a blank slate, I'd propose:

  • %y read up to 2 digits, always apply 2-digit logic
  • %Y: read up to 4 digits, no post-processing
  • time_get::do_get_year: read up to 4 digits, apply 2-digit logic if two or fewer digits read (including leading zeros).

I think this is close to libstdc++ behavior, except for how many digits are read for %y. I don't see anything in the older or newer strptime wording that supports reading more than two digits.

@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and @CaseyCarter, @strega-nil-ms, and I all agree with your "If I were working from a blank slate" proposed behavior. 👍

tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
stl/inc/xloctime Outdated Show resolved Hide resolved
tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved
@ghost

This comment was marked as outdated.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Apr 29, 2022

Looks great - I pushed changes for a few tiny things I noticed (followed by a trivial comment change to rerun the CLA Bot).

@StephanTLavavej StephanTLavavej self-assigned this Apr 30, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@CaseyCarter CaseyCarter added the affects redist Results in changes to separately compiled bits label May 1, 2022
@StephanTLavavej StephanTLavavej merged commit b965db4 into microsoft:main May 1, 2022
@StephanTLavavej
Copy link
Member

Thanks for investigating and fixing this runtime correctness bug while preserving ABI! 🐞 🔍 😻

@fsb4000
Copy link
Contributor

fsb4000 commented May 2, 2022

I informed libstdc++ and libc++ developers: GCC-105450 and LLVM-55220

Oops, gcc already knew about it: GCC-103612 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects redist Results in changes to separately compiled bits bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<xloctime>: get_time does not return correct year in tm.tm_year if year is 1
5 participants