Bug 238482 – Build failure with g++ 12: DMIs before end of enclosing class

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Mike Gorse]

The build fails (again) with the most recent git revision of gcc 12:

/home/mgorse/src/WebKit/Source/JavaScriptCore/runtime/SamplingProfiler.h:135:60:   required from here
/usr/include/c++/12/type_traits:971:30: error: default member initializer for ‘JSC::SamplingProfiler::StackFrame::CodeLocation::lineNumber’ required by ‘std::is_constructible’ before the end of its enclosing class

This is not the only error of this type;
WebCore/platform/graphics/ColorInterpolationMethod.h gives a similar error.

It is Related to this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96645

[Martin Liška]

The simplest fix would be providing a default constructor as seen here:
https://paste.centos.org/view/80fc6f3c

[Mike Gorse]

Created attachment 456150 [details]
Patch.

[Michael Catanzaro]

Comment on attachment 456150 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=456150&action=review

> Source/WebCore/platform/graphics/ColorInterpolationMethod.h:64
> +        HSL(HueInterpolationMethod method)

explicit HSL(HueInterpolationMethod method)

[Michael Catanzaro]

Complaints from style bot:

ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:112:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/ColorInterpolationMethod.h:61:  Missing space before }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/ColorInterpolationMethod.h:65:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]

[Mike Gorse]

Created attachment 456169 [details]
Patch.

[Alex Christensen]

Comment on attachment 456169 [details]
Patch.

Comma location and indentation is not WebKit style even though style bot can't figure that out.

I'm not a big fan of this fix (removing initializer lists and adding more explicit constructors is a step in the wrong direction in my opinion).  Is there an alternative fix at the instantiation site?

[Jonathan Wakely]

(In reply to Martin Liška from comment #1)
> The simplest fix would be providing a default constructor as seen here:
> https://paste.centos.org/view/80fc6f3c

That paste of mine is about to expire. For posterity, the content is what Mike Gorse attached in comment 2.

[Jonathan Wakely]

(In reply to Alex Christensen from comment #6)
> I'm not a big fan of this fix (removing initializer lists and adding more
> explicit constructors is a step in the wrong direction in my opinion).  Is
> there an alternative fix at the instantiation site?

No. The problem is that the implicit default ctor needs to check if it should be defined as deleted or not, which means checking the initializers for the non-static data members.

What you can do is just add a default ctor and let the default member initializers still do the initialization:

--- webkit11.ii	2022-03-30 12:38:21.928429136 +0100
+++ webkit11.ii~	2022-03-30 12:39:46.985998048 +0100
@@ -240201,6 +240201,8 @@
         BytecodeIndex wasmOffset;
 
         struct CodeLocation {
+            CodeLocation() { }
+
             bool hasCodeBlockHash() const
             {
                 return codeBlockHash.isSet();

Now the default ctor is not implicitly-declared, so the compiler doesn't need to check if it would be well-formed or should be deleted. It's default constructible, period.

The actual initialization of the members will still be done by the default initializers defined later in the class.

[Jonathan Wakely]

Yet another option would be to replace the initializers with simpler ones. G++ just got a change to process some initializers immediately, instead of after the class is complete. So:

--- webkit11.ii	2022-03-30 12:38:21.928429136 +0100
+++ webkit11.ii~	2022-03-30 12:39:46.985998048 +0100
@@ -240218,11 +240218,11 @@
             }
 
 
-            unsigned lineNumber { std::numeric_limits<unsigned>::max() };
-            unsigned columnNumber { std::numeric_limits<unsigned>::max() };
+            unsigned lineNumber { UINT_MAX };
+            unsigned columnNumber { UINT_MAX };
             BytecodeIndex bytecodeIndex;
             CodeBlockHash codeBlockHash;
-            JITType jitType { JITType::None };
+            JITType jitType { };
             bool isRegExp { false };
         };
 
 
Using the UINT_MAX macro doesn't need to do name lookup, so can be parsed immediately. But yes, it means using a macro not numeric_limits, sorry :-(
Alternatively, use { ~0u } to get the all-ones unsigned value.

The JITType init works because value-init with {} will init to zero, which happens to be the value of JITType::None. That's a bit fragile if you ever wanted to change the value of None, or wanted to init to a different value.

My preference would be the empty default ctor in comment 8.

[Jonathan Wakely]

(In reply to Jonathan Wakely from comment #9)
> Alternatively, use { ~0u } to get the all-ones unsigned value.

Or { -1u }

[Mike Gorse]

Created attachment 456293 [details]
Patch.

I think I've fixed the indentation. Also removed the changes to the initializers.

[Alex Christensen]

Comment on attachment 456293 [details]
Patch.

This now puts redundant information into the header.  I like the suggestion in comment 8 better if that works.
While I would be fine making a reasonable change, why not just change g++12?

[Martin Liška]

So the GCC 12 change was actually reverted and will be discussed in CWG issue 2335. However, it's likely the change may land again in GCC 13.*:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96645#c24

[Alex Christensen]

Ok.  We do often make adjustments to make our code compile on the currently released or previously released and still supported gcc, clang, and msvc, though we try to make those adjustments as elegant as possible.  However, if someone wants to support a source-breaking change in a future version of a compiler, the developers of that compiler should probably consider if that's a good idea.

[Jonathan Wakely]

Even if the source being broken is ill-formed or undefined according to the C++ standard?

[Alex Christensen]

If gcc, clang, and msvc all successfully compile a piece of code and have for years, then we should consider changing the C++ standard.  I'd be happy to write a proposal if this is the case.

In this case, I don't oppose fixing our code.  I just would like to do that in a way that doesn't add redundant information.

[Mike Gorse]

Created attachment 456367 [details]
Patch.

Sorry--I'd misunderstood what Jonathan was suggesting there. I've tweaked the patch a bit.
It does not build if I don't also add the implicit constructor.
But I'm not strongly opposed to marking this wontfix, given that the gcc change has been reverted, although it may come up again in the future.

[Darin Adler]

Comment on attachment 456367 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=456367&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Build failure with g++ 12: DMIs before end of enclosing class

Is this a feature in gcc, or a bug? I am not happy with these changes!

[Alex Christensen]

This is the most acceptable version of these changes yet, but let's plan to not fix this with the possibility of revisiting if GCC13 insists that they really want to break this code.

[Mike Gorse]

The comment on the revert does suggest that the standard needs a fix.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96645#c24

[Michael Catanzaro]

Well the patch is appreciated anyway, in case other distros wind up needing it.

Nice that you caught this and let the GCC developers know, even before GCC 12 was released!