Fix put_time() crash on invalid struct tm data

[TheStormN]

trafficstars

Fixes #4882 Fixes #924

My next PR about fixing std::put_time crashes.

If all goes well I will try to also fix the performance issues mentioned in #3575 and #1900 before VS 17.12 cut.

UPDATE: The result now outputs single ? for every invalid tm struct data based on specifier. For example if tm struct contains invalid year and the format string is "%Y-%m-%d-%H-%M", the output will be like ?-08-10-00-45.

[TheStormN]

The CI failed and it would seem that this validation is expected to be done ad-hoc based on the formatting string being passed. I.e. if there are invalid values into the tm struct, they can be ignored in case the formatting string does not require them.

I guess I underestimated the issue at hand. Doing per specifier validation should not be too hard but it will be way more verbose, Will try to alter it tomorrow.

[StephanTLavavej]

That makes sense - regardless of my open question about what we should do in the case of invalid values, I definitely didn't see any wording that suggested that invalid values that aren't needed by the formatting string are problematic in any way, so per-specifier validation does appear to be necessary. Good catch!

[TheStormN]

A single test on x86 x86 8 failed:

1: Failed Tests (1):
1:   libc++ :: std/thread/thread.condition/thread.condition.condvarany/notify_one.pass.cpp:2

However, this seems like a flaky test, unrelated to my PR. @StephanTLavavej could you manually rerun this specific step, as it seems user/pass are required and I can't do it.

[CaseyCarter]

A single test on x86 x86 8 failed:

1: Failed Tests (1):
1:   libc++ :: std/thread/thread.condition/thread.condition.condvarany/notify_one.pass.cpp:2

However, this seems like a flaky test, unrelated to my PR. @StephanTLavavej could you manually rerun this specific step, as it seems user/pass are required and I can't do it.

I've submitted #4885 to disable this test tempermanently, and rerun the failed job from your CI run.

[TheStormN]

Thanks, this is looking really good! I'm about to fall asleep so I want to take another look at this, but here are the comments I noticed.

All comments addressed. Although there seems to be an issue with the format target. Sometimes the IDE is missing some points and the CI failure says I should execute cmake --build --preset x64 --target format to fix any issues. However, cmake reports that such target does not exists.

[TheStormN]

I've discovered I'm missing some cases. Will update the PR again soon.

[TheStormN]

Ok, now I think I've got all the cases covered (and perhaps brain damage). :)

Btw, the tests are also compiling in C++14 mode. Is there any specific reason for that? It would be really nice if we can use some neat C++20 tricks there.

[StephanTLavavej]

Sometimes the IDE is missing some points

I'm not sure what that means - you should configure your editor to clang-format on save, using the same clang-format that ships in VS (ensuring that the exact same version is being used, as its behavior can change with the version). I can provide guidance for VSCode; I think full VS can do it too but I don't know how.

the CI failure says I should execute cmake --build --preset x64 --target format to fix any issues. However, cmake reports that such target does not exists.

What exactly are you seeing? This works for me:

D:\GitHub\STL>"C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvarsall.bat" x64
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.11.0-pre.7.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'

D:\GitHub\STL>cmake --preset x64
-- The CXX compiler identification is MSVC 19.41.34119.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.41.34117.0/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test WINDOWS_SDK_VERSION_CHECK
-- Performing Test WINDOWS_SDK_VERSION_CHECK - Success
-- The ASM_MASM compiler identification is MSVC
-- Found assembler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.41.34117.0/bin/Hostx64/x64/ml64.exe
-- Searching for VS clang-format
-- Searching for VS clang-format - found
-- Found Python: C:/Users/stl/AppData/Local/Programs/Python/Python312/python.exe (found suitable version "3.12.4", minimum required is "3.12") found components: Interpreter
-- Boost.Math: standalone mode ON
-- Configuring done (3.2s)
-- Generating done (0.2s)
-- Build files have been written to: D:/GitHub/STL/out/x64

D:\GitHub\STL>cmake --build --preset x64 --target format
[1328/1328] C:\WINDOWS\system32\cmd.exe /C "cd /D D:\GitHu...GitHub/STL/tools/format/../../tools/validate/validate.cpp"

Btw, the tests are also compiling in C++14 mode. Is there any specific reason for that? It would be really nice if we can use some neat C++20 tricks there.

Yes - the reason is that we need to ensure that C++14 library features (like put_time()) use only C++14 core language features in their implementation, so that users can actually use them under /std:c++14. If we tested old library features under newer language modes only, then it would be too easy to unintentionally use new compiler features in the product code. (We do test old library features under a mix of old and new modes, because we need to ensure that users can use all modes.)

The exception is that we can use a limited number of core language features in older Standard modes, when compilers allow this and emit suppressible warnings that we're using Future Technology. This is how we can use C++17 if constexpr and C++20 explicit(bool) everywhere in product code. (Test code is still subject to the warnings unless we suppress them.)

Test code can conditionally activate new logic under #if _HAS_CXX20 but it should generally do so only when testing C++20-only library features, or testing interaction of old library features with new core language features (e.g. spaceship logic, or whatever). Otherwise we do need to sometimes write more primitive test code than we would like to in an ideal world.

BTW - it isn't a huge deal, but we generally recommend that after code review has begun, you should avoid force-pushing unless it's really important. GitHub makes it more difficult to see what's changed when force pushes happen, which can interfere with incremental reviewing. (It's not the end of the world though, and sometimes force pushes can help reviewing, e.g. by reorganizing messy commit history into logical chunks, or replaying commits to get a complicated merge out of the history.)

[TheStormN]

I use VS and on save it didn't reordered the headers. Might be something on my end. Will check again.

About force pushing. I did so exactly for readability reasons and it was for new commits after the last review.

When I get home I will show the exact errors I get for the format targets.

[TheStormN]

**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.10.5
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'

C:\Program Files\Microsoft Visual Studio\2022\Community>cd D:\Projects\Microsoft\STL

C:\Program Files\Microsoft Visual Studio\2022\Community>D:

D:\Projects\Microsoft\STL>cmake --build --preset x64
ninja: no work to do.

D:\Projects\Microsoft\STL>cmake --build --preset x64 --target format
ninja: error: unknown target 'format'

D:\Projects\Microsoft\STL>

Nope, does not work. :)

[StephanTLavavej]

Hmm, I see that you're using VS 2022 17.10.5, but we require the latest Preview (as mentioned in the README). I'm not sure if that's the root cause, though. In fact, after I merged #4824 on 2024-07-11, the STL's build will emit an error for anything older than VS 2022 17.11, so I'm not sure how you've successfully built.

Can you:

• Install VS Preview
• Show your CMake and Ninja versions
• Pull main (remember to git submodule update if necessary), clean your repo, and try again?

Here's what I have:

C:\Temp>"C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvarsall.bat" x64
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.11.0-pre.7.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'

C:\Temp>where cmake
C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe

C:\Temp>cmake --version
cmake version 3.29.5-msvc4

CMake suite maintained and supported by Kitware (kitware.com/cmake).

C:\Temp>where ninja
C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja\ninja.exe

C:\Temp>ninja --version
1.11.0

[TheStormN]

You are correct. I use VS 17.10.5. I don't like using preview versions of the IDE as often they are buggy. A simple local change to stl/inc/yvals_core.h allows me to use the latest RTM version. Given that the compiler itself have basically zero support for C++23 and that I'm also still not working on C++23 STL related items I think it should be perfectly fine to use a stable version of VS.

Also I'm not sure why there have to be a limit for developers to be forced to always use the latest VS preview in general. While I understand for latest features it is necessary, for anything else it should not be blocking.

About cleaning the repo and updating submodules - Yep I'm doing all that and it still does not work. If a VS preview is a real prerequisite for the format target, then I would be fine to skip it for now. :)

[fsb4000]

I just want to show how cmake --build --preset x64 --target format works for us:

[fsb4000]

Hmm, I see that you're using VS 2022 17.10.5, but we require the latest Preview (as mentioned in the README). I'm not sure if that's the root cause, though.

Probably. In the file: tools/format/CMakeLists.txt we have a hardcoded path for clang-format:

find_program(CLANG_FORMAT
    NAMES clang-format
    PATHS "C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/bin"
    DOC "The clang-format program to use"
    NO_DEFAULT_PATH
    NO_CMAKE_PATH
    NO_CMAKE_ENVIRONMENT_PATH
    NO_SYSTEM_ENVIRONMENT_PATH
    NO_CMAKE_SYSTEM_PATH
)

and because his clang-format has a different path, we don't add the custom target:

if(CLANG_FORMAT)
   ...
   add_custom_target(run-format)
   ...
endif()

[TheStormN]

I see, yes. It's strange to hardcode paths for tools. This will not work even for the simple case if I install the IDE in another dir.

[StephanTLavavej]

I don't like using preview versions of the IDE as often they are buggy.

You can use the production IDE, but we really do need to require the preview toolset (the command-line compiler environment). You might be getting away with hacking yvals_core.h at the moment, but this is a recipe for encountering ICEs, compiler bugs, and missing features as we continually upgrade the STL to depend on the latest compiler preview.

The production and preview versions of VS will happily coexist on your machine, they don't overwrite each other.

Also I'm not sure why there have to be a limit for developers to be forced to always use the latest VS preview in general. While I understand for latest features it is necessary, for anything else it should not be blocking.

This accelerates our development process. Remember, we're not requiring this of users at large - only STL contributors. While we regularly ship updates, our release process is deeply pipelined, with changes committed right now taking many many months to ship for production usage. By requiring the latest Preview, we shorten the time that it takes to depend on compiler fixes or features to at most 2-3 months. The STL has a ton of moving parts, and compiler changes aren't always limited to the latest Standard mode.

Basically we ask contributors to use a uniform environment so that we don't have to waste time investigating why they're seeing issues that the maintainers and PR/CI system aren't seeing, or dealing with clang-format mismatches, etc.

[TheStormN]

While I can somewhat agree with your statements, still, if I had installed VS Preview on a non-standard location, it would still not work with hardcoded path.

[StephanTLavavej]

Yeah, you've definitely found a real problem there. I'm looking into that CMakeLists.txt now, thanks! :smiley_cat:

[TheStormN]

Yeah, you've definitely found a real problem there. I'm looking into that CMakeLists.txt now, thanks! 😺

Never should've mentioned it... Got your attention away from the PR. :D

[StephanTLavavej]

Created #4888. Now back to actual work :joy_cat:

[TheStormN]

@StephanTLavavej I think we are all good here. All comments resolved. :)

[TheStormN]

Man, if we could just push a bit the UCRT team to replicate at least the validation behavior of other C libraries regarding strftime, we would save so much pain. :)

[StephanTLavavej]

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

[StephanTLavavej]

Thanks for fixing this long-standing bug and making this code so much more robust! :tada: :smile_cat: :rocket: