New issue
Advanced search Search tips

Issue 825352 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Template bug found by VS 15.7 pre-release compiler, missed by clang?

Project Member Reported by brucedaw...@chromium.org, Mar 23 2018

Issue description

In third_party\WebKit\Source\platform\wtf\ThreadSpecific.h we have this function:

template <typename T>
inline ThreadSpecific<T>::~ThreadSpecific() {
  // Does not invoke destructor functions. They will be called from
  // ThreadSpecificThreadExit when the thread is detached.
  TlsFree(tlsKeys()[m_index]);
}

The last line references a non-existent function and a non-existent variable. Presumably it has gone undetected because of VC++'s odd way of evaluating templates. The code should look like this:
  TlsFree(TlsKeys()[index_]);

I assume that this was missed by clang because it was trying to maintain VC++ compatibility. Perhaps it is time to move towards greater C++ compatibility in this area now that VC++ is catching up?

I will fix the bad code.

 
Relevant changes:

crrev.com/c/978849 is the fix for this particular issue
crrev.com/c/734101 considered turning on -fno-delayed-template-parsing

It looks like turning on -fno-delayed-template-parsing should become practical and desirable soon if it is not already.

In a similar vein, compiling with clang with -fno-delayed-template-parsing reveals these template errors:

FAILED: obj/base/base/utf_string_conversion_utils.obj 
../../base/strings/utf_string_conversion_utils.cc(125,15):  error: duplicate explicit instantiation of 'PrepareForUTF8Output<wchar_t>' ignored as a Microsoft extension [-Werror,-Wmicrosoft-template]
template void PrepareForUTF8Output(const char16*, size_t, std::string*);
              ^
../../base/strings/utf_string_conversion_utils.cc(124,15):  note: previous explicit instantiation is here
template void PrepareForUTF8Output(const wchar_t*, size_t, std::string*);
              ^
../../base/strings/utf_string_conversion_utils.cc(146,15):  error: duplicate explicit instantiation of 'PrepareForUTF16Or32Output<std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > >' ignored as a Microsoft extension [-Werror,-Wmicrosoft-template]
template void PrepareForUTF16Or32Output(const char*, size_t, string16*);
              ^
../../base/strings/utf_string_conversion_utils.cc(145,15):  note: previous explicit instantiation is here
template void PrepareForUTF16Or32Output(const char*, size_t, std::wstring*);
              ^
2 errors generated.

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3effe184c834005b0728716fb9993ee6c38b40e8

commit 3effe184c834005b0728716fb9993ee6c38b40e8
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Tue Mar 27 05:54:53 2018

Remove duplicated template functions on Windows

Because wchar_t is the same as char16 on Windows, and by extension
std::wstring is the same as string16, instantiating templates using both
of these types leads to duplicate instantiations. This error is only
reported when building with -fno-delayed-template-parsing, but it is
always dodgy/wrong.

Microsoft is working towards supporting correct template parsing which
means that we can also (because they are fixing their headers).

The (simplified) error messages were:
error: duplicate explicit instantiation of 'PrepareForUTF8Output<wchar_t>'
ignored as a Microsoft extension
error: duplicate explicit instantiation of 'PrepareForUTF16Or32Output<>
ignored as a Microsoft extension

Bug:  825352 
Change-Id: I5360b14f48277d56c3800238cc77077f13ef9acf
Reviewed-on: https://chromium-review.googlesource.com/981083
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546034}
[modify] https://crrev.com/3effe184c834005b0728716fb9993ee6c38b40e8/base/strings/utf_string_conversion_utils.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db526e73638b6815327ab97eabda50e03bd1f148

commit db526e73638b6815327ab97eabda50e03bd1f148
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Thu Mar 29 03:11:32 2018

Fix references to non-existent function and variable

All unchecked code will contain errors and apparently, due to quirks of
VC++ template compilation, emulated by clang-cl, the ThreadSpecific<T>
destructor was never *really* compiled. It referenced two functions and
one variable and only one of these three things actually existed. This
was reported by Microsoft who use chromium as a compiler torture test.

Bug:  825352 
Change-Id: I5dd1868acda760ef33dde518d72e3d7694fad9a8
Reviewed-on: https://chromium-review.googlesource.com/978849
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546720}
[modify] https://crrev.com/db526e73638b6815327ab97eabda50e03bd1f148/third_party/WebKit/Source/platform/wtf/ThreadSpecific.h

Status: Fixed (was: Assigned)
This issue is fixed. The only remaining sibling issue would be building with -fno-delayed-template-parsing which is a clang-team issue that can't be done until we switch to the (not yet shipped) Spring Update SDK for Windows 10.

Sign in to add a comment