Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: d4f69df7d5b19c44e70860fabf379690682cf896
https://github.com/WebKit/WebKit/commit/d4f69df7d5b19c44e70860fabf379690682cf896
Author: Kimmo Kinnunen <kkinnu...@apple.com>
Date: 2022-10-29 (Sat, 29 Oct 2022)
Changed paths:
M Source/WTF/wtf/text/StringConcatenate.h
M Source/WTF/wtf/text/StringView.cpp
M Tools/TestWebKitAPI/CMakeLists.txt
M Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
M Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp
R Tools/TestWebKitAPI/WTFStringUtilities.cpp
M Tools/TestWebKitAPI/WTFStringUtilities.h
Log Message:
-----------
StringOperations.cpp test re-defines WTF::StringTypeAdapter implementation,
undefined behaviour
https://bugs.webkit.org/show_bug.cgi?id=247121
rdar://problem/101669240
Reviewed by Antti Koivisto.
In C++, one definition rule states that in one program each definition must
consists out of
same tokens in all compilation units in the program.
https://en.cppreference.com/w/cpp/language/definition
...
There can be more than one definition in a program of each of the
following: class type,
enumeration type, inline function, inline variable (since C++17), templated
entity
(template or member of template, but not full template specialization), as
long as all
of the following is true:
...
- each definition consists of the same sequence of tokens (typically,
appears in the same header file)
...
if the definition is for a template, then all these requirements apply to
both names at the point of
definition and dependent names at the point of instantiation
If all these requirements are satisfied, the program behaves as if there is
only one definition in the
entire program. Otherwise, the program is ill-formed, no diagnostic
required.
In other words: you cannot have two implementations of same template member
function, one 100 bytes and
one 1000mbs in size and select between them based on an #define before #include
in different .cpp files
in the same program.
The tests would alter the StringView operator+ used templates via custom define
before include.
This results in undefined behavior, since libWTF.a would contain one
implementation and use
of the templates, while the StringOperators.cpp test would intend to contain a
different
implementation. Also, any other test in TestWTF would use the same version as
StringOperators.cpp,
due to how the includes are currently written.
Using the hack only for StringOperators.cpp would preserve the UB but be the
minimalistic change.
However, this does not work:
The requirement that WTFStringUtilities.h needs to be included before
StringView.h
is messing up the include order in tests. When trying to fix the include order
for all
other files except the StringOperators.cpp, the omission of the hack include
will cause new, non-hacked
template definition for the same templates that are hacked for
StringOperators.cpp. This in turn
causes non-deterministic selection of the template: the non-hacked, correct
template is selected
for all instantiations, including the ones in StringOperators.cpp. This causes
the test to fail, as
the hack is not present.
This means that this sort of UB hack cannot be used for StringOperators.cpp
while using the correct
templates for other instantiations in the binary. This means that almost all
WTFString-using tests
must use WTFStringUtilities.h to get the hack. This has the problem:
- UB
- Messed up includes
Fix this properly by not using the hack, rather putting the debug logic into
the Debug build and
disabling the test for non-Debug builds.
Use atomic<int> to count the copies so that it doesn't show up in TSAN or
similar tools when running
normally for other purposes. The test itself of course does not need the atomic.
This is work that is prerequisite to fix tests to obey normal WebKit include
rules.
* Source/WTF/wtf/text/StringConcatenate.h:
* Source/WTF/wtf/text/StringView.cpp:
* Tools/TestWebKitAPI/CMakeLists.txt:
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:
(TestWebKitAPI::TEST):
* Tools/TestWebKitAPI/WTFStringUtilities.cpp: Removed.
* Tools/TestWebKitAPI/WTFStringUtilities.h:
Canonical link: https://commits.webkit.org/256131@main
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
