forked from WebKit/WebKit
-
Notifications
You must be signed in to change notification settings - Fork 52
simdutf: remove unused <iostream> from scalar/base64.h amalgamation #320
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // This directory is placed first on the include search path for non-Debug | ||
| // builds when USE_BUN_JSC_ADDITIONS is on, so `#include <iostream>` resolves | ||
| // here instead of the toolchain header. | ||
| // | ||
| // <iostream> is unlike <ostream>/<istream>/<sstream>: on libstdc++ it emits a | ||
| // reference to std::ios_base_library_init in every translation unit that | ||
| // includes it. One such reference anywhere in the link pulls globals_io.o from | ||
| // libstdc++, whose static initializer constructs cin/cout/cerr/clog and their | ||
| // wchar_t siblings before main, dragging the full std::locale facet set in | ||
| // with it. Bun never touches C++ iostreams at runtime. | ||
| // | ||
| // Use WTF::dataLog()/dataLogLn() or fputs/fprintf for diagnostics. | ||
| #ifndef BUN_ALLOW_IOSTREAM | ||
| #error "<iostream> is banned in Bun WebKit release builds: it forces std::ios_base::Init and the full std::locale facet set to run before main in every bun process. Use WTF::dataLog() or <cstdio>. See Source/WTF/wtf/bun/BannedIncludes/iostream." | ||
| #else | ||
| #include_next <iostream> | ||
| #endif |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 This
include_directories(BEFORE ...)runs at top-level project scope (OptionsJSCOnly.cmake isinclude()'d, notadd_subdirectory()'d), so the<iostream>shim propagates to every target — includingSource/ThirdParty/gtestandTools/TestWebKitAPI, which are pulled in on non-Windows viaENABLE_API_TESTS ON. gtest unconditionally includes<iostream>(gtest-port.h:291, gtest.cc:52), so a plainninja/cmake --build .(defaultalltarget) with-DUSE_BUN_JSC_ADDITIONS=ONin Release now fails ongtest-all.cc. All Bun CI/release paths build only--target jscand are unaffected, but you may want toset(ENABLE_API_TESTS OFF)underUSE_BUN_JSC_ADDITIONS, or addtarget_compile_definitions(gtest PRIVATE BUN_ALLOW_IOSTREAM), so the default target still builds.Extended reasoning...
What breaks
The new
include_directories(BEFORE "${WTF_DIR}/wtf/bun/BannedIncludes")is intended to shadow<iostream>for the WebKit sources that end up in the Bun link. However, it is applied at a scope that also covers third-party test code that legitimately uses iostreams, so building the defaultalltarget on non-Windows now fails.Why the shim leaks to gtest
OptionsJSCOnly.cmakeis not a subdirectory script — it is pulled in viainclude():CMakeLists.txt:22→include(WebKitCommon)WebKitCommon.cmake:320→include(Options${PORT})include()does not create a new directory scope, so theinclude_directories(BEFORE ...)call executes in the top-levelCMakeLists.txtdirectory scope. Everyadd_subdirectory()that follows (starting withadd_subdirectory(Source)atCMakeLists.txt:75) inherits it.A few lines further down in this same file,
ENABLE_API_TESTSis setONfor all non-Windows platforms. That causes:Source/CMakeLists.txt:27-28→add_subdirectory(ThirdParty/gtest)Tools/CMakeLists.txt:24-26→add_subdirectory(TestWebKitAPI)Neither of these is added with
EXCLUDE_FROM_ALL, so both are part of the defaultalltarget. gtest's ownCMakeLists.txtdoes not clear the inherited directory-level include list and does not defineBUN_ALLOW_IOSTREAM. And gtest unconditionally includes<iostream>:Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:291Source/ThirdParty/gtest/src/gtest.cc:52Because the shim directory is added
BEFOREand directory-level-Ipaths precede system include paths,#include <iostream>in gtest resolves to the shim and hits the#error.Step-by-step reproduction
-DPORT=JSCOnly -DUSE_BUN_JSC_ADDITIONS=ON -DCMAKE_BUILD_TYPE=Release(orRelWithDebInfo).WTF_DIRis set byWebKitFS.cmake(included atWebKitCommon.cmake:308, i.e. beforeOptions${PORT}), so the shim path expands correctly.ninja(orcmake --build .) with no explicit target.gtest-all.cc, which#includesgtest.cc→#include <iostream>→ resolves toSource/WTF/wtf/bun/BannedIncludes/iostream→#error "<iostream> is banned in Bun WebKit release builds…".Impact
Every documented and CI build path in this repo —
Dockerfile*,build.ts,mac-release.bash,release.sh,windows-release.ps1— invokescmake --build … --target jsc(or--target artifact), which does not depend on gtest. So CI, autobuild artifacts, and the shipped Bun binaries are unaffected. This only bites a developer who runs a bareninja/cmake --build .in a Release-flavoredUSE_BUN_JSC_ADDITIONStree, and the#errormessage itself points at the-DBUN_ALLOW_IOSTREAMopt-out. It's a real regression in an ostensibly-supported configuration (this file is the one that setsENABLE_API_TESTS ON), but not one that blocks the PR's goal.Suggested fixes
Any one of:
set(ENABLE_API_TESTS OFF)inside theif(USE_BUN_JSC_ADDITIONS)block — Bun doesn't ship or run these tests anyway.target_compile_definitions(gtest PRIVATE BUN_ALLOW_IOSTREAM)(e.g. via aSource/ThirdParty/gtest/PlatformJSCOnly.cmake), and similarly for TestWebKitAPI.target_include_directories(... BEFORE ...)onWTF/JavaScriptCore/bmallocinstead of the globalinclude_directories().