Bug 187737 - TestWTF.WTF_Expected.Unexpected is not MSVC-safe
Summary: TestWTF.WTF_Expected.Unexpected is not MSVC-safe
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-17 13:39 PDT by Ross Kirsling
Modified: 2018-08-22 15:28 PDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-07-17 13:39:50 PDT
The following test is compiler-dependent:

> constexpr const char* oops = "oops";
> ...
> constexpr auto s0 = makeUnexpected(oops);
> constexpr auto s1(s0);
> EXPECT_EQ(s0, s1);

It fails on Windows with the following error...

> ..\..\Tools\TestWebKitAPI\Tests\WTF\Expected.cpp:96
> Value of: s1
>   Actual: oops
> Expected: s0
> Which is: oops

...due to the fact that MSVC evidently keeps a separate string for each const char* variable when they're declared constexpr:
https://godbolt.org/g/HGC8nw
Comment 1 Ross Kirsling 2018-07-17 13:56:53 PDT
(In reply to Ross Kirsling from comment #0)
> ...due to the fact that MSVC evidently keeps a separate string for each
> const char* variable when they're declared constexpr:
> https://godbolt.org/g/HGC8nw

Dunno why this link isn't working -- I made another one here:
https://godbolt.org/g/Bbwngz

As a solution, we could drop constexpr, add const, or use a value of a type other than const char*, but I want to make sure we're not losing some important coverage in doing so. JF, what would you prefer?
Comment 2 JF Bastien 2018-07-17 14:12:34 PDT
(In reply to Ross Kirsling from comment #1)
> (In reply to Ross Kirsling from comment #0)
> > ...due to the fact that MSVC evidently keeps a separate string for each
> > const char* variable when they're declared constexpr:
> > https://godbolt.org/g/HGC8nw
> 
> Dunno why this link isn't working -- I made another one here:
> https://godbolt.org/g/Bbwngz
> 
> As a solution, we could drop constexpr, add const, or use a value of a type
> other than const char*, but I want to make sure we're not losing some
> important coverage in doing so. JF, what would you prefer?

This is a test of constexpr. Dropping constexpr defeats the purpose of the test. When I compile your second link with /O1 I get two identical functions. Seems like an MSVC bug? This is comparing the const char * pointers held in the unexpected object, they really should be the same.
Comment 3 Ross Kirsling 2018-07-17 15:29:50 PDT
(In reply to JF Bastien from comment #2)
> This is a test of constexpr. Dropping constexpr defeats the purpose of the
> test. When I compile your second link with /O1 I get two identical
> functions. Seems like an MSVC bug? This is comparing the const char *
> pointers held in the unexpected object, they really should be the same.

I agree that it seems like an MSVC bug...

It seems allowable for the compiler to decide whether to keep one or two strings for the following:
> constexpr auto foo = "oops";
> constexpr auto bar = "oops";
(In fact, MSVC has a compiler option to control this: https://msdn.microsoft.com/en-us/library/s0s0asdt.aspx)
But to convert `constexpr auto bar(foo);` into `constexpr auto bar = "oops";` and thereby treat it as a new string seems questionable.

I certainly figured that constexpr was crucial to this test case, I just wondered whether changing the type or making it constexpr const (which evidently works) would be allowable. Either way, we can try submitting an MSVC issue.
Comment 5 Ross Kirsling 2018-08-22 15:28:59 PDT
This is now resolved for Release mode as a side effect of bug 188811, as we were disabling string pooling for unclear reasons.

This will still fail in Debug though, so we can either leave this bug open to track the MSVC ticket linked above, or we could #if out the test for MSVC Debug with a FIXME comment.