REGRESSION(r277744): Broke build on s390x mainframes

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Michael Catanzaro]

r277744 broke the build on s390x mainframes (yes, these run WebKit). The failure occurs when linking JSC's LLIntSettingsExtractor:

Source/WTF/wtf/CMakeFiles/WTF.dir/./text/StringBuilderJSON.cpp.o:StringBuilderJSON.cpp:function WTF::StringBuilder::appendQuotedJSONString(WTF::String const&): error: undefined reference to 'void WTF::StringBuilder::allocateBuffer<char16_t, unsigned char>(unsigned char const*, unsigned int)'
collect2: error: ld returned 1 exit status

I wound up asking for help and was told to suspect template instantiation. Indeed, we have templates defined in the source file, but of course templates belong in the header file. Apparently s390x's GCC is more or less aggressive about inlining. Fortunately it's easy to fix by just moving the template function definitions to the header. (Note there's also another template function defined in StringBuilderJSON.cpp, but this one is OK because it's private to that file, only used in that one place.)

[Michael Catanzaro]

Created attachment 430259 [details]
Patch

[Darin Adler]

Is this a substandard compiler that doesn’t correctly implement modern C++, or is it a mistake in our code?

Can we work around this with explicit instantiatiation rather than moving everything to the header?

I’m not thrilled with putting so much more code in the header.

[Michael Catanzaro]

(In reply to Darin Adler from comment #2)
> Is this a substandard compiler that doesn’t correctly implement modern C++,
> or is it a mistake in our code?

Well it's GCC 11-point-something. I think it's a mistake in our code. I was taught that templates have to be defined in headers, and I'm actually more surprised that other platforms did not fail than that GCC did fail.

The tip I received was:

"""
if C++ templates are in play, then it's likely an incorrect C++ code that a different optimization on s390x discovers

if you add "--param=inline-min-speedup=2 --param=max-inline-insns-auto=80" to compiler flags you might be able to reproduce the issue on other arches as well
"""

Haven't tried it though.

> Can we work around this with explicit instantiatiation rather than moving
> everything to the header?
> 
> I’m not thrilled with putting so much more code in the header.

Yes, it should be possible to do with explicit instantiation. See https://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html:

"""
Duplicate instances of a template can be avoided by defining an explicit instantiation in one object file, and preventing the compiler from doing implicit instantiations in any other object files by using an explicit instantiation declaration, using the extern template syntax:

extern template int max (int, int);

This syntax is defined in the C++ 2011 standard, but has been supported by G++ and other compilers since well before 2011.

Explicit instantiations can be used for the largest or most frequently duplicated instances, without having to know exactly which other instances are used in the rest of the program. You can scatter the explicit instantiations throughout your program, perhaps putting them in the translation units where the instances are used or the translation units that define the templates themselves; you can put all of the explicit instantiations you need into one big file; or you can create small files like

#include "Foo.h"
#include "Foo.cc"

template class Foo<int>;
template ostream& operator <<
                (ostream&, const Foo<int>&);

for each of the instances you need, and create a template instantiation library from those.

This is the simplest option, but also offers flexibility and fine-grained control when necessary. It is also the most portable alternative and programs using this approach will work with most modern compilers.
"""

I can try this and figure out how complex it turns out to be.

[Adrian Perez]

(In reply to Darin Adler from comment #2)
> Is this a substandard compiler that doesn’t correctly implement modern C++,
> or is it a mistake in our code?
> 
> Can we work around this with explicit instantiatiation rather than moving
> everything to the header?
> 
> I’m not thrilled with putting so much more code in the header.

A couple of times in the past while fixing non-unified builds I've seen
cases in which templates in .cpp files would work fine because by chance
the .cpp file where they were defined and where they were used both ended
up in the same unified translation unit. I haven't checked if this was
the case here, but I can confirm this also fixed the build for me with
with GCC 11.1.0 on x86_64 😶️

[Darin Adler]

This class is StringBuilder, used across multiple frameworks. Seems unlikely that the issue has to do with unified translation units. Pretty sure these are used outside WTF, and therefore definitely outside the translation unit.

Ideally I’d like to know more about exactly what’s wrong before we make the change. I understand that "putting it in the header makes the bug go away" but I’d like to go a little deeper.

[Michael Catanzaro]

Comment on attachment 430259 [details]
Patch

The problem is the template 'void WTF::StringBuilder::allocateBuffer<char16_t, unsigned char>(unsigned char const*, unsigned int)' is not instantiated in the object file StringBuilderJSON.cpp.o. If using templates in source files rather than header files, we have to explicitly instantiate the templates in the desired object files, which we have not done here. Otherwise, toolchain decides for itself which object file the template will be instantiated in, and either we get lucky and it builds, or not. When defining templates in headers -- that's normal practice, it's what we do for huge WTF headers like Vector.h and HashTable.h, for example -- then this problem goes away because the templates get instantiated separately in each object file. That probably bloats binary size, but it works.

I've actually never used -- or even heard of -- explicit template instantiation before. I've just always defined templates in header files, following the general rule "never define templates in source files" that most C++ programmers learn. At least, that is what we were taught at university back before C++11 was finished. I suspect it's going to be ugly, but I need to investigate more and see for myself. I'll try to get to this soon, hopefully tomorrow (because the longer we go failing s390x CI, the more likely some big endian regression will sneak in, and we won't know which day it happened).

(In reply to Adrian Perez from comment #4)
> A couple of times in the past while fixing non-unified builds I've seen
> cases in which templates in .cpp files would work fine because by chance
> the .cpp file where they were defined and where they were used both ended
> up in the same unified translation unit. I haven't checked if this was
> the case here, but I can confirm this also fixed the build for me with
> with GCC 11.1.0 on x86_64 😶️

Weird, it works fine for us on x86_64. *shrug* Anyway, I guess that's sufficient proof that it's not some weird s390x bug.

[Darin Adler]

(In reply to Michael Catanzaro from comment #6)
> The problem is the template 'void
> WTF::StringBuilder::allocateBuffer<char16_t, unsigned char>(unsigned char
> const*, unsigned int)' is not instantiated in the object file
> StringBuilderJSON.cpp.o. If using templates in source files rather than
> header files, we have to explicitly instantiate the templates in the desired
> object files, which we have not done here.

OK, I think I figured it out.

My guess is that this is caused by the call to allocateBuffer in StringBuilderJSON.cpp. I hate to move these templates into the header just so it can be shared by another part of the StringBuilder implementation. All the callers are inside StringBuilder itself, it’s just that is divided across two source files.

Here are some different fixes:

1) Move these function template definitions into the StringBuilder.h.

2) Explicitly instantiate this function template in StringBuilder.cpp.

3) Merge StringBuilderJSON.cpp into StringBuilder.cpp.

4) Move these function template definitions into a StringBuilderInternals.h header file, included only by StringBuilder.cpp and StringBuilderJSON.cpp.

I prefer solutions other than (1). I like (2), (3), and (4).

I am probably not in as much of a rush as you are to get compiling on s390x again, and I am sorry for that.

[Darin Adler]

Hah, I just noticed you already mention StringBuilderJSON. I was so proud of figuring it out, but I just didn’t read carefully enough!

[Michael Catanzaro]

I wrote a very long comment attempting to explain explicit template instantiation, quoting from Bjarne Stroustrup's "The C++ Programming Language, Fourth Edition" but I'm just going to throw it out because yes, it's caused by the call to allocateBuffer in StringBuilderJSON.cpp, and yes, all four of your proposed solutions would fix this.

Of your proposals, (1) (which I implemented) and (4) are the most robust solutions. But yes, (1) is slowest to compile. (2) would fix this particular linker error, but other errors may occur in the future, so it's the most fragile solution. So (3) and (4) seem more preferable than (2) IMO. I think I prefer (4), simply because defining templates in headers is more natural to me, so this is probably what I'll implement tomorrow.

By the way, I *will* quote this one part of "The C++ Programming Language" section 26.2.2 "Manual Control of Instantiation" where Bjarne explains why we might not want to do (1):

"The link-time and recompilation efficiency impact of instantiation requests can be significant. I have seen examples in which bundling most template instantiations into a single compilation unit cut the compile time from a number of hours to the equivalent number of minutes."

Which echos what you've said already. (Sadly, that would be impractical to do across WebKit. I assume that C++ modules will be the eventual practical solution for our compilation time woes....)

[Darin Adler]

The solutions other than (1) are also better for someone working on StringBuilder who doesn’t want to rebuild the world after each change.

[Michael Catanzaro]

Created attachment 430405 [details]
Patch

[Darin Adler]

Comment on attachment 430405 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilder.cpp:31
> +#include <wtf/text/StringBuilderInternals.h>
>  
>  #include <wtf/dtoa.h>

Should probably be grouped in with the other includes below, not the special "class header" paragraph.

> Source/WTF/wtf/text/StringBuilderJSON.cpp:16
> +#include <wtf/text/StringBuilderInternals.h>
>  
>  #include <wtf/text/WTFString.h>

Should probably be grouped in with the other includes below, not the special "class header" paragraph.

[Michael Catanzaro]

Created attachment 430461 [details]
Patch for landing

[EWS]

Committed r278404 (238429@main): <https://commits.webkit.org/238429@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430461 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78811017>