Fix put_time() crash on invalid format specifier

[TheStormN]

Fixes #4820. The test case also verifies the situation where an invalid specifier is given when a modifier is being used. For example "%E% some text"

This is my first PR, so sorry if something is wrong in formatting or ABI terms.

[TheStormN]

@StephanTLavavej Sorry for the ping, but I see that not only this, but quite few more PRs are waiting for reviews and eventual merge for considerable time. I'm wondering what is the workflow in general? How much time it usually takes and do you have dedicated time for reviewing community supplied PRs?

[StephanTLavavej]

Sorry for the ping, but I see that not only this, but quite few more PRs are waiting for reviews and eventual merge for considerable time. I'm wondering what is the workflow in general? How much time it usually takes and do you have dedicated time for reviewing community supplied PRs?

My apologies for the delay - yes, the backlog has been accumulating. (This is something that I monitor with the STL Status Chart.)

The problem is that I am currently the only STL maintainer able to spend up to 100% of their time on the STL. (See our Maintainer Priorities issue #4700 for weekly updates.) Everyone else is working on high-priority projects like ASAN and even requesting small amounts of their time to review my own PRs is difficult (but necessary as all code must be reviewed by at least one other person, not even I am allowed to check in whatever I want).

I occasionally have non-STL tasks to deal with (ConcRT, ATL/MFC every other quarter). Additionally, I just took a few days off for vacation (my niece's cat-themed 6th birthday party), during which the backlog further accumulated, and as soon as I got back I was slammed with high-priority STL-specific but non-review work - I had to fix a regression on Win11 24H2 (#4844), revert my glorious changes to drop Win7 (#4857), update our LLVM submodule to fix internal "credential scanner" failures (#4862), and fix /analyze warnings for bounds safety (#4856). Today I thought I'd finally have time to get back to reviews, but upon checking the STL-ASan-CI history to record occurrences of sporadic x86 crashes that we missed in #4324 (because I don't get email notifications of STL-ASan-CI failures), I noticed that the x64 tests failed right before I left for vacation. I find sporadic test failures to be absolutely intolerable and in this case I actually knew how to fix them, since in #3922 I had fixed a similar failure with x86 SSE2 instructions and enhanced the ASan machinery to print out unrecognized bytes. So I've spent all day fixing that (and bogus comments) in LLVM ASan, and now validating it with a build. I also have to review an important STL document that my double boss sent to me yesterday.

When the world isn't on fire and I can sit down and review PRs, usually I can dedicate most of my day to it (excluding only meetings and email). The time that it takes to review a PR varies, depending on how complicated the problem and solution are, how familiar I am with the area, how well-structured the PR's commit history and description are, and how much the PR has already been reviewed by other contributors. If the PR is clean and I know the area deeply, I can easily power through a thousand lines in an hour. If the area is complicated, or I notice that something is messed up, a few dozen lines can take longer to either fix or write up what's wrong.

When there are no major distractions, I can usually keep up with or slightly exceed the rate of incoming PRs, which is how we've been able to drain the formerly-numbering-80 PR backlog. But as queueing theory explains, when a source and drain are closely balanced with little excess capacity, any increase in source throughput or reduction in drain throughput causes a massive increase in the backlog (aka why store checkout lines can be massive, etc.).

I'm hoping to make review progress next week, although I can't promise anything. (I try to review high-priority PRs first, but the order in which I review PRs is highly variable - I also like to review easy PRs first, until I'm in the zone and can really focus on complicated changes.)

[TheStormN]

I can see, why PR reviews can take so much time. You not only do the reviews, but also fix your own comments. :)

[StephanTLavavej]

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

[StephanTLavavej]

Thanks for fixing this longstanding bug with severe impact, and congratulations on your first microsoft/STL commit! 😻 🛠️ 🥳

This is expected to ship in VS 2022 17.12 Preview 3.

[TheStormN]

Thanks for fixing this longstanding bug with severe impact, and congratulations on your first microsoft/STL commit! 😻 🛠️ 🥳

This is expected to ship in VS 2022 17.12 Preview 3.

Thank you! What is the dead line to catch the 17.12 RTM version as I plan to have two more PR's regarding put_time? :)

P.S. Also, just out of curiosity, why there is an 'internal repo' on which you mirror the PRs?

[StephanTLavavej]

Changes will stop flowing from our development branch into 17.12 Preview 3 on 2024-08-30 (3 weeks from now). That's not the release date, it's just when an internal merge from prod/fe (the branch that the compiler front-end and libraries share) to MSVC main happens. After that point, changes will need to be manually ported with special boss approval to get into 17.12 Preview 3, or any later Preview, up to and including 17.12 General Availability. That process is intentionally tedious to avoid devs injecting risk late into a release cycle, so we avoid porting changes except for critical bugs.

Aug 30 is the date that the branch is "snapped" but we need at least a couple of days to review and merge a change, so I'd say that a PR has to be completely ready by Mon, Aug 26 if it's to have a chance of making it through the review and merge process in time.

The internal repo is where all of the MSVC toolset (including the compiler front-end, back-end, linker, and other libraries) is built. While 99.9% of STL development happens on GitHub and is then mirrored to MSVC, the MSVC-internal repo builds more flavors that we haven't ported to our CMake build system yet (notably /clr and /clr:pure - we just need to find the time but haven't found it in the last 5 years) and other variants such as OneCore and Spectre (ditto). While all of the STL's own tests are on GitHub, the MSVC-internal repo has tons of compiler tests that also exercise the STL to some extent, and all of the tests (both the ones on GitHub and the ones that are internal-only) are also run with internal "checked" builds of the compiler with assertions enabled, which catch compiler bugs that aren't always visible in the publicly available "retail" compiler. The internal repo is responsible for packaging up the VCRedist, and ensuring that headers and libs and sources are installed to the right directories in the VS Installer. Finally, only the MSVC-internal build generates the normal msvcp140.dll name (GitHub intentionally builds msvcp140_oss.dll to avoid conflicts), and can officially sign it as Microsoft (which we could never do for our "unofficial" OSS builds).

Although all of that internal stuff is happening, we ensure that our GitHub repo is binary-identical at nearly all times to a subdirectory of our internal repo (src/vctools/crt/github), with temporary divergence typically only when the compiler back-end devs want to make changes in their prod/be branch and code takes some time to flow over. This is how GitHub remains the source of truth for development, even though the MSVC repo is where the bits are built. (Also incentivizing us to do 99.9% of our development in public is that the GitHub build and test systems are way better and faster than the internal equivalents.)

[TheStormN]

Thank you for the detailed explanations! I will try my best to catch the deadline.