Bug 192361 - REGRESSION (r238764-238783): TestWTF.WTF.StringOperators is failing
Summary: REGRESSION (r238764-238783): TestWTF.WTF.StringOperators is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-04 09:22 PST by Ryan Haddad
Modified: 2019-01-24 15:25 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.38 KB, patch)
2018-12-07 14:01 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Output with -O2 (2.52 MB, text/plain)
2018-12-10 09:11 PST, Chris Dumez
no flags Details
Output with -Os (bad) (1.77 MB, text/plain)
2018-12-10 09:12 PST, Chris Dumez
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2018-12-04 09:22:55 PST
The following API test is consistently failing on the bots:

    TestWTF.WTF.StringOperators
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:52
        Expected equality of these values:
          2
          wtfStringCopyCount
            Which is: 0
        string + string
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:54
        Expected equality of these values:
          2
          wtfStringCopyCount
            Which is: 0
        atomicString + string
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:57
        Expected equality of these values:
          1
          wtfStringCopyCount
            Which is: 0
        "C string" + string

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/8229/steps/run-api-tests/logs/stdio
Comment 1 Ryan Haddad 2018-12-04 09:25:37 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.
Comment 2 Ryan Haddad 2018-12-04 09:28:19 PST
This was a large WTF change: "Move URL from WebCore to WTF" https://trac.webkit.org/changeset/238771/webkit
Comment 4 Ryan Haddad 2018-12-04 10:57:00 PST
I can't yet reproduce this locally, but it is failing consistently on the bots.
Comment 5 Chris Dumez 2018-12-04 11:00:05 PST
(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.
Comment 6 Chris Dumez 2018-12-04 11:00:18 PST
(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
Comment 7 Alex Christensen 2018-12-04 11:30:57 PST
I know what's going on here.  It's probably a linker issue, believe it or not.
Comment 8 Alex Christensen 2018-12-04 11:34:26 PST
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.
Comment 9 Radar WebKit Bug Importer 2018-12-06 09:03:38 PST
<rdar://problem/46524903>
Comment 10 Chris Dumez 2018-12-07 14:01:43 PST
Created attachment 356834 [details]
Patch
Comment 11 Chris Dumez 2018-12-07 14:28:52 PST
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.
Comment 12 Alex Christensen 2018-12-07 14:45:21 PST
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 13 Alexey Proskuryakov 2018-12-07 14:45:41 PST
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?
Comment 14 Chris Dumez 2018-12-07 14:56:08 PST
(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.
Comment 15 Chris Dumez 2018-12-07 14:58:12 PST
(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.
Comment 16 Chris Dumez 2018-12-07 14:59:28 PST
(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.
Comment 17 Chris Dumez 2018-12-07 15:06:18 PST
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 18 Alex Christensen 2018-12-07 16:48:19 PST
Comment on attachment 356834 [details]
Patch

What if instead we added volatile to the definition of wtfStringCopyCount?
Comment 19 Chris Dumez 2018-12-07 16:49:50 PST
(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.
Comment 20 Alex Christensen 2018-12-07 16:59:04 PST
Have you tried std::atomic?  What about inline assembly to increment and decrement the count?
Comment 21 Chris Dumez 2018-12-07 17:01:41 PST
(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.
Comment 22 Chris Dumez 2018-12-07 17:08:47 PST
(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.
Comment 23 Alexey Proskuryakov 2018-12-07 17:19:25 PST
> 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.
Comment 24 Chris Dumez 2018-12-07 18:07:35 PST
(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.
Comment 25 Chris Dumez 2018-12-10 09:11:54 PST
Created attachment 356964 [details]
Output with -O2
Comment 26 Chris Dumez 2018-12-10 09:12:25 PST
Created attachment 356965 [details]
Output with -Os (bad)
Comment 27 Chris Dumez 2018-12-10 12:57:36 PST
I'd be OK disabling this test in release builds like Alex suggested if Alexey prefers.
Comment 28 Alexey Proskuryakov 2018-12-10 17:37:50 PST
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.
Comment 29 Ryan Haddad 2018-12-11 10:22:28 PST
I would like this test to be disabled so that the release bots have a chance of being green.
Comment 30 Chris Dumez 2018-12-11 10:25:38 PST
(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.
Comment 31 Ryan Haddad 2018-12-11 10:39:52 PST
(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 32 WebKit Commit Bot 2018-12-11 11:10:56 PST
Comment on attachment 356834 [details]
Patch

Clearing flags on attachment: 356834

Committed r239077: <https://trac.webkit.org/changeset/239077>
Comment 33 WebKit Commit Bot 2018-12-11 11:10:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Ross Kirsling 2019-01-24 15:25:05 PST
This file is failing consistently in Mac Debug again as of r240437 (bug 193602). Ugh.