[webkit-changes] [WebKit/WebKit] d4f69d: StringOperations.cpp test re-defines WTF::StringTy...

[Kimmo Kinnunen]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: d4f69df7d5b19c44e70860fabf379690682cf896
      https://github.com/WebKit/WebKit/commit/d4f69df7d5b19c44e70860fabf379690682cf896
  Author: Kimmo Kinnunen <kkinnunen at 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