Bug 192361 – REGRESSION (r238764-238783): TestWTF.WTF.StringOperators is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

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

[Ryan Haddad]

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.

[Ryan Haddad]

This was a large WTF change: "Move URL from WebCore to WTF" https://trac.webkit.org/changeset/238771/webkit

[Ryan Haddad]

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

[Ryan Haddad]

I can't yet reproduce this locally, but it is failing consistently on the bots.

[Chris Dumez]

(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.

[Chris Dumez]

(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

[Alex Christensen]

I know what's going on here.  It's probably a linker issue, believe it or not.

[Alex Christensen]

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.

[Radar WebKit Bug Importer]

<rdar://problem/46524903>

[Chris Dumez]

Created attachment 356834 [details]
Patch

[Chris Dumez]

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.

[Alex Christensen]

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.

[Alexey Proskuryakov]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

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...

[Alex Christensen]

Comment on attachment 356834 [details]
Patch

What if instead we added volatile to the definition of wtfStringCopyCount?

[Chris Dumez]

(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.

[Alex Christensen]

Have you tried std::atomic?  What about inline assembly to increment and decrement the count?

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Alexey Proskuryakov]

> 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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 356964 [details]
Output with -O2

[Chris Dumez]

Created attachment 356965 [details]
Output with -Os (bad)

[Chris Dumez]

I'd be OK disabling this test in release builds like Alex suggested if Alexey prefers.

[Alexey Proskuryakov]

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.

[Ryan Haddad]

I would like this test to be disabled so that the release bots have a chance of being green.

[Chris Dumez]

(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.

[Ryan Haddad]

(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).

[WebKit Commit Bot]

Comment on attachment 356834 [details]
Patch

Clearing flags on attachment: 356834

Committed r239077: <https://trac.webkit.org/changeset/239077>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ross Kirsling]

This file is failing consistently in Mac Debug again as of r240437 (bug 193602). Ugh.