microsoft / cppwinrt

C++/WinRT
MIT License
1.67k stars 239 forks source link

Add a mechanism to suppress std::source_location #1260

Closed dmachaj closed 1 year ago

dmachaj commented 1 year ago

Closes #1259

Why is this change being made?

Because some users who have moved forward to C++20 do not want source location information to end up in their release binaries. This exposes some information regarding source code layout to the public. It also increases binary size somewhat.

Briefly summarize what changed

If WINRT_NO_SOURCE_LOCATION is defined before <winrt/base.h> is first included then the macros for std::source_location will treat it as if it is C++17 and will not include any source code data. Users may choose to define this only for release builds, or for all builds. It is their choice.

To validate these changes I had to split off a version of the test_cpp20 project that will define this new define. It is going to be global to the project so I don't see another reasonable way to provide coverage without a new project. The only difference is that WINRT_NO_SOURCE_LOCATION is a preprocessor define. Only custom_error.cpp is included in this project. The other C++20 test collateral is not repeated.

One small aspect that is not fully contained by the define is the #include <source_location> in base_includes.h. That will still be included but will not be used. This ends up in base.h before base_macros.h so the preprocessor logic will not have run when this is parsed. I could duplicate the check here but decided simpler is better and kept it unchanged. I am open to changing that based on feedback.

How was this change tested?

The primary test coverage is the new test binary and test case. This compiles with the new define (catching some build breaks along the way). It also confirms that the source information is lacking like in C++17 mode.

build_test_all.cmd succeeds and the tests pass.

I did my best to follow the pattern for CMake support but am not sure how to test that locally so I will have to rely on the CI build to catch any errors.

dmachaj commented 1 year ago

@kennykerr I feel like there is probably some documentation that I should update, but I did not see anything in this repo. Can you please point me in the right direction for that? Thanks.

kennykerr commented 1 year ago

@kennykerr I feel like there is probably some documentation that I should update, but I did not see anything in this repo. Can you please point me in the right direction for that? Thanks.

@stevewhims has thus far been kind enough to add docs for us.

dmachaj commented 1 year ago

@alvinhochun are there any docs for building the mingw variant locally? I don't have past experience with mingw and am unsure how to replicate the failures locally. Thanks.

alvinhochun commented 1 year ago

@alvinhochun are there any docs for building the mingw variant locally? I don't have past experience with mingw and am unsure how to replicate the failures locally. Thanks.

Not at this time. The easiest way to test should be through MSYS2 -- after installing MSYS2, start the "MSYS2 MinGW Clang x64" shell and run pacman -S mingw-w64-clang-x86_64-toolchain mingw-w64-clang-x86_64-cmake mingw-w64-clang-x86_64-ninja to install the build tools, then you should be able to follow roughly these commands to build with mingw-w64 Clang: https://github.com/microsoft/cppwinrt/blob/fe304096fa30583f3cc2ebfdbf564d2b4081e2ad/.github/workflows/ci.yml#L342-L363

(The above is for the CLANG64 environment. The UCRT64 environment shown on their homepage gives you GCC instead.)

kennykerr commented 1 year ago

@alvinhochun can you help out with the build? Although I'm comfortable cross compiling Rust, most of us have zero experience with cross-compiling C++.

I also noticed that all the (great) work you've been doing on CI validation is in a single workflow. That's a little problematic since we can't easily disable individual workflows that may be failing and blocking developers. By comparison, for Rust I have a half dozen individual workflows and can easily temporarily disable cross-compilation testing if needed:

https://github.com/microsoft/windows-rs/tree/master/.github/workflows

Ideally, we can move to a more granular model otherwise I may be forced to disable the entire workflow which obviously won't be ideal.

dmachaj commented 1 year ago

@kennykerr @alvinhochun that change seems to have done the trick. Unfortunately 2 false positives hit. Is there a "rerun failed tasks" button that I'm missing or do I need to push an empty change to get a fresh run? Thanks.

kennykerr commented 1 year ago

@dmachaj - upped your permissions so you should be able to rerun.

dmachaj commented 1 year ago

@kennykerr that did the trick 👍. Two re-runs and the checks are green.