Summary: | REGRESSION (r238764-238783): TestWTF.WTF.StringOperators is failing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
Component: | New Bugs | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, ap, cdumez, commit-queue, jlewis3, keith_miller, ross.kirsling, tsavell, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ryan Haddad
2018-12-04 09:22:55 PST
This regressed between r238763 and r238784. The range is large because we lost testing coverage on the bots due to https://trac.webkit.org/changeset/238764/webkit, and the test was failing once they recovered in https://trac.webkit.org/changeset/238784/webkit. This was a large WTF change: "Move URL from WebCore to WTF" https://trac.webkit.org/changeset/238771/webkit Trac link for regression range: https://trac.webkit.org/log/webkit/?action=stop_on_copy&mode=stop_on_copy&rev=238783&stop_rev=238763&limit=100&verbose=on I can't yet reproduce this locally, but it is failing consistently on the bots. (In reply to Ryan Haddad from comment #4) > I can't yet reproduce this locally, but it is failing consistently on the > bots. I suspect the compiler is optimizing out the things thus the lack of copying. (In reply to Chris Dumez from comment #5) > (In reply to Ryan Haddad from comment #4) > > I can't yet reproduce this locally, but it is failing consistently on the > > bots. > > I suspect the compiler is optimizing out the things thus the lack of copying. I can reproduce I know what's going on here. It's probably a linker issue, believe it or not. WTF_STRINGTYPEADAPTER_COPIED_WTF_STRING is defined differently for different instantiations of the WTF::String functions. That test needs to link against instantiations with the correct definition. I can't reproduce the failure or I'd upload a patch. Created attachment 356834 [details]
Patch
Comment on attachment 356834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356834&action=review > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:396 > + 7C83DF381D0A590C00FEBCF3 /* StringOperators.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C01363C713C3997300EF3964 /* StringOperators.cpp */; settings = {COMPILER_FLAGS = "-O1"; }; }; -O2 would also work. It is really just -Os that fails. Instead of changing the compiler flags, could we use NDEBUG to have different expectations? I think we still want to test what our customers will actually run into, and if r238771 improved things we should have tests to make sure we don't unknowingly regress them. Comment on attachment 356834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356834&action=review > Tools/ChangeLog:10 > + Compile StringOperators.cpp with -O1 optimization level instead of -Os. This > + fixes the failures for me locally. Isn't it still a bug in WTF that its tests fail with -Os? (In reply to Alex Christensen from comment #12) > Instead of changing the compiler flags, could we use NDEBUG to have > different expectations? What does this mean? What is the expectation for NDEBUG? Clearly, it seems to depend on your compiler. > I think we still want to test what our customers > will actually run into, > and if r238771 improved things we should have tests > to make sure we don't unknowingly regress them. Improved things? Why do you think https://trac.webkit.org/changeset/238771/webkit could have improved things? I get different test results when commenting out parts of the test. E.g. if you do the first check by itself, it works. If you add the second check, the first starts failing. Seems dependent on code size somehow. (In reply to Alexey Proskuryakov from comment #13) > Comment on attachment 356834 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356834&action=review > > > Tools/ChangeLog:10 > > + Compile StringOperators.cpp with -O1 optimization level instead of -Os. This > > + fixes the failures for me locally. > > Isn't it still a bug in WTF that its tests fail with -Os? I do not think we have evidence of a WTF bug, do we? It could simply with the optimizer messing with our artificial testing that tries to figure out how many times a string is copied. (In reply to Chris Dumez from comment #14) > (In reply to Alex Christensen from comment #12) > > Instead of changing the compiler flags, could we use NDEBUG to have > > different expectations? > > What does this mean? What is the expectation for NDEBUG? Clearly, it seems > to depend on your compiler. > > > I think we still want to test what our customers > > will actually run into, This test modifies wtf/StringConcatenate.h via a define already. This is not shipping code. I only disabled -Os optimization for this specific test file. > > > and if r238771 improved things we should have tests > > to make sure we don't unknowingly regress them. > > Improved things? Why do you think > https://trac.webkit.org/changeset/238771/webkit could have improved things? > > I get different test results when commenting out parts of the test. E.g. if > you do the first check by itself, it works. If you add the second check, the > first starts failing. Seems dependent on code size somehow. This passes: diff --git a/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp b/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp index 595afb054a4..997d169544e 100644 --- a/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp +++ b/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp @@ -44,13 +44,13 @@ namespace TestWebKitAPI { TEST(WTF, StringOperators) { String string("String"); - AtomicString atomicString("AtomicString"); - ASCIILiteral literal { "ASCIILiteral"_s }; + //AtomicString atomicString("AtomicString"); + //ASCIILiteral literal { "ASCIILiteral"_s }; EXPECT_EQ(0, wtfStringCopyCount); EXPECT_N_WTF_STRING_COPIES(2, string + string); - EXPECT_N_WTF_STRING_COPIES(2, string + atomicString); + /*EXPECT_N_WTF_STRING_COPIES(2, string + atomicString); EXPECT_N_WTF_STRING_COPIES(2, atomicString + string); EXPECT_N_WTF_STRING_COPIES(2, atomicString + atomicString); @@ -181,7 +181,7 @@ TEST(WTF, StringOperators) EXPECT_N_WTF_STRING_COPIES(2, atomicString + L"wide string" + string + L"wide string"); EXPECT_N_WTF_STRING_COPIES(2, atomicString + (L"wide string" + string + L"wide string")); EXPECT_N_WTF_STRING_COPIES(2, (atomicString + L"wide string") + (string + L"wide string")); -#endif +#endif*/ } But this fails: diff --git a/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp b/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp index 595afb054a4..19463197605 100644 --- a/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp +++ b/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp @@ -45,12 +45,13 @@ TEST(WTF, StringOperators) { String string("String"); AtomicString atomicString("AtomicString"); - ASCIILiteral literal { "ASCIILiteral"_s }; + (void)atomicString; + //ASCIILiteral literal { "ASCIILiteral"_s }; EXPECT_EQ(0, wtfStringCopyCount); EXPECT_N_WTF_STRING_COPIES(2, string + string); - EXPECT_N_WTF_STRING_COPIES(2, string + atomicString); + /*EXPECT_N_WTF_STRING_COPIES(2, string + atomicString); EXPECT_N_WTF_STRING_COPIES(2, atomicString + string); EXPECT_N_WTF_STRING_COPIES(2, atomicString + atomicString); @@ -181,7 +182,7 @@ TEST(WTF, StringOperators) EXPECT_N_WTF_STRING_COPIES(2, atomicString + L"wide string" + string + L"wide string"); EXPECT_N_WTF_STRING_COPIES(2, atomicString + (L"wide string" + string + L"wide string")); EXPECT_N_WTF_STRING_COPIES(2, (atomicString + L"wide string") + (string + L"wide string")); -#endif +#endif*/ } I merely define an extra (unused) local variable... Comment on attachment 356834 [details]
Patch
What if instead we added volatile to the definition of wtfStringCopyCount?
(In reply to Alex Christensen from comment #18) > Comment on attachment 356834 [details] > Patch > > What if instead we added volatile to the definition of wtfStringCopyCount? I have already tried that and many other things, to no avail. Have you tried std::atomic? What about inline assembly to increment and decrement the count? (In reply to Alex Christensen from comment #20) > Have you tried std::atomic? What about inline assembly to increment and > decrement the count? Yes for std::atomic and it did not help. No for assembly because I do not write assembly. (In reply to Chris Dumez from comment #21) > (In reply to Alex Christensen from comment #20) > > Have you tried std::atomic? What about inline assembly to increment and > > decrement the count? > > Yes for std::atomic and it did not help. No for assembly because I do not > write assembly. I am happy to test out proposals if you cannot reproduce but I have already spent several hours testing various things and the only thing that worked is the patch that I uploaded. > I do not think we have evidence of a WTF bug, do we? It could simply with the optimizer messing with our artificial testing that tries to figure out how many times a string is copied.
I still don't get the logic here. It has to be a bug somewhere - WTF, compiler, or the test. Which one are we blaming? It kind of sounds like the test checks for something that C++ does not guarantee to be true.
(In reply to Alexey Proskuryakov from comment #23) > > I do not think we have evidence of a WTF bug, do we? It could simply with the optimizer messing with our artificial testing that tries to figure out how many times a string is copied. > > I still don't get the logic here. It has to be a bug somewhere - WTF, > compiler, or the test. Which one are we blaming? It kind of sounds like the > test checks for something that C++ does not guarantee to be true. I do not know. I have little experience with this. The fact that adding an unused local variable changes the behavior of three code makes no sense to me. Created attachment 356964 [details]
Output with -O2
Created attachment 356965 [details]
Output with -Os (bad)
I'd be OK disabling this test in release builds like Alex suggested if Alexey prefers. I don't think that I can have much of a preference when we don't even know who to blame - WTF, compiler, or the test. I would like this test to be disabled so that the release bots have a chance of being green. (In reply to Ryan Haddad from comment #29) > I would like this test to be disabled so that the release bots have a chance > of being green. I would argue that landing my patch would be better. (In reply to Chris Dumez from comment #30) > (In reply to Ryan Haddad from comment #29) > > I would like this test to be disabled so that the release bots have a chance > > of being green. > > I would argue that landing my patch would be better. Also fine with me if someone will review it (ping). Comment on attachment 356834 [details] Patch Clearing flags on attachment: 356834 Committed r239077: <https://trac.webkit.org/changeset/239077> All reviewed patches have been landed. Closing bug. This file is failing consistently in Mac Debug again as of r240437 (bug 193602). Ugh. |