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

Add a mechanism to suppress std::source_location #1260

Merged

Conversation

dmachaj
Copy link
Contributor

@dmachaj dmachaj commented Jan 6, 2023

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 dmachaj requested a review from kennykerr January 6, 2023 04:35
@dmachaj
Copy link
Contributor Author

dmachaj commented Jan 6, 2023

@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.

Copy link
Contributor

@alvinhochun alvinhochun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the test_cpp20_no_sourcelocation target also to line 297 358 of ci.yml so that the new test is built for the mingw-w64 checks.

@kennykerr
Copy link
Collaborator

@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
Copy link
Contributor Author

dmachaj commented Jan 6, 2023

@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
Copy link
Contributor

@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:

- name: Build cppwinrt
run: |
mkdir build
cd build
if [[ "${{ matrix.arch }}" = "i686" ]]; then
skip_large_pch_arg="-DSKIP_LARGE_PCH=TRUE"
fi
cmake ../ -GNinja -DCMAKE_BUILD_TYPE=${{ matrix.config }} \
-DDOWNLOAD_WINDOWSNUMERICS=TRUE \
-DUSE_ANSI_COLOR=TRUE \
$skip_large_pch_arg
cmake --build . --target cppwinrt
- name: Build tests
run: |
cd build
cmake --build . -j2 --target test-vanilla test_cpp20 test_win7 test_old
- name: Run tests
run: |
cd build
ctest --verbose

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

@kennykerr
Copy link
Collaborator

@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.

Comment on lines 39 to 46
#if defined(__clang__)
// FIXME: Blocked on __cpp_consteval, see:
// * https://github.com/microsoft/cppwinrt/pull/1203#issuecomment-1279764927
// * https://github.com/llvm/llvm-project/issues/57094
TEST_CASE("custom_error_logger", "[!shouldfail]")
#else
TEST_CASE("custom_error_logger")
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the failure on all Clang builds (including clang-cl) is caused by this "[!shouldfail]" tag. This whole #if defined(__clang__) condition should be removed because the lack of source location support on Clang does not matter on this test, which disables source location explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I'll try that.

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dmachaj
Copy link
Contributor Author

dmachaj commented Jan 9, 2023

@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
Copy link
Collaborator

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

@dmachaj
Copy link
Contributor Author

dmachaj commented Jan 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent source location in release build
3 participants