[libc++] Resolve missing table_size symbol

[muiez]

Summary

The aim of this patch is to resolve the missing table_size symbol (see reduced test case). That const variable is declared and defined in libcxx/include/locale; however, the test case suggests that the symbol is missing. This is due to a C++ pitfall (highlighted here). In summary, assigning the reference of table_size doesn't enforce the const-ness and expects to find table_size in the DLL. The fix is to use constexpr or have an out-of-line definition in the src (for consistency).

[muiez]

To reproduce:

Compile (with -std=c++17) and link the following program:

#include <locale>

int main() {
  const size_t *okay = &std::ctype<char>::table_size;
  return 0;
}

[ldionne]

In D110647#3028307, @muiez wrote:

To reproduce:

Compile (with -std=c++17) and link the following program:

Can you please add a test case then?

[muiez]

Add test case

[muiez]

In D110647#3032038, @ldionne wrote:

In D110647#3028307, @muiez wrote:

To reproduce:

Compile (with -std=c++17) and link the following program:

Can you please add a test case then?

Yup, added a test case in updated revision.

[muiez]

ping

[jloser]

libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp

24

Do we need the pointer? Or can we just say

assert(F::table_size >= 256);

[jloser]

libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp

24

It could be a static_assert even, right?

[muiez]

libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp

24

Yes, we need the pointer since the error under test occurs when we have a reference to F::table_size. The assert is there to avoid getting an unused variable warning (G).

[• Quuxplusone]

Adding my own distracting comment. ;) I figure @ldionne is the only one who can really give this the green light.

libcxx/include/__locale

712–718 ↗

(On Diff #376343)

Peanut gallery says: This does fix the linker problem IIUC, but wouldn't it be more consistent to put an out-of-line definition in libcxx/src/locale.cpp? We do that for

locale::id ctype<char>::id;

(cf. line 712 above), and also for

const ctype_base::mask ctype_base::space;
const ctype_base::mask ctype_base::print;
const ctype_base::mask ctype_base::cntrl;
~~~

Vice versa, if we're finally able to use constexpr for this stuff so that it doesn't need to be in locale.cpp, then maybe can we use constexpr consistently for all of the above stuff too? or we can't do that because ABI?

[muiez]

Using out-of-line definition instead of constexpr. This way is more consistent and allows the test case to pass for standards prior to c++17.

[zibi]

LGTM

[muiez]

Any updates @ldionne? Comments have been addressed.

[muiez]

Clang-format

[muiez]

Ping

[muiez]

Any insight as to why the CI is red? It seems to complain about a new symbol being added _ZNSt3__15ctypeIcE10table_sizeE, which is defined. However, that is the intention since we want to resolve the missing symbol.

[ldionne]

In D110647#3122226, @muiez wrote:

Any insight as to why the CI is red? It seems to complain about a new symbol being added _ZNSt3__15ctypeIcE10table_sizeE, which is defined. However, that is the intention since we want to resolve the missing symbol.

The CI is testing that there are no ABI changes. This patch contains an ABI change, since it adds a symbol. The change is not an ABI *break* so it is acceptable, but it is nonetheless a change. You need to update the ABI lists to contain that symbol.

You can see how to do that here: https://libcxx.llvm.org/Contributing.html#exporting-new-symbols-from-the-library. Basically, go to https://buildkite.com/llvm-project/libcxx-ci/builds/6180#75b25b94-130a-472d-a409-72a7f5507f17 and click on Artifacts. The libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist file will contain the new symbol for table_size -- add it to the corresponding file in libcxx/. You'll then need to do the same on macOS (look at the macOS ABI lists in the macOS jobs once the CI gets that far).

Finally, please also add an entry in libcxx/lib/abi/CHANGELOG.TXT.

libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp

12

Please add a comment explaining what the test is checking. Something like this would suffice:

// Make sure we can reference std::ctype<char>::table_size.

13

You will need to add something like this:

// Before https://llvm.org/D110647, the shared library did not contain
// std::ctype<char>::table_size, so this test fails with a link error.
// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{11|12|13}}

Technically you would also need similar annotations for other vendors who ship libc++ as the system library, but I don't think they test back-deployment, so we don't have annotations for them.

24

I think it is good to have the assert since std::ctype<char>::table_size is specified as being at least 256.

[muiez]

Added symbol in abilists. More detail in test case.

[muiez]

In D110647#3122355, @ldionne wrote:

In D110647#3122226, @muiez wrote:

Any insight as to why the CI is red? It seems to complain about a new symbol being added _ZNSt3__15ctypeIcE10table_sizeE, which is defined. However, that is the intention since we want to resolve the missing symbol.

The CI is testing that there are no ABI changes. This patch contains an ABI change, since it adds a symbol. The change is not an ABI *break* so it is acceptable, but it is nonetheless a change. You need to update the ABI lists to contain that symbol.

You can see how to do that here: https://libcxx.llvm.org/Contributing.html#exporting-new-symbols-from-the-library. Basically, go to https://buildkite.com/llvm-project/libcxx-ci/builds/6180#75b25b94-130a-472d-a409-72a7f5507f17 and click on Artifacts. The libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist file will contain the new symbol for table_size -- add it to the corresponding file in libcxx/. You'll then need to do the same on macOS (look at the macOS ABI lists in the macOS jobs once the CI gets that far).

Finally, please also add an entry in libcxx/lib/abi/CHANGELOG.TXT.

Thanks for the explanation. I will add the commit hash to the libcxx/lib/abi/CHANGELOG.TXT once this lands. Comments have been addressed.

[ldionne]

LGTM once CI passes and comments have been addressed.

libcxx/lib/abi/CHANGELOG.TXT

21

Let's be verbose on the explanations.

libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist

1649

I think this should be {'name': '__ZNSt3__15ctypeIcE10table_sizeE', 'type': 'OBJECT', 'is_defined': True, 'size': 0} instead. For some reason, mangled names on Darwin start with two underscores, and one underscore on Linux. I should investigate why that is.

libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist

1649

Same.

[muiez]

Verbose description, added correct symbol on abilist(s).

[ldionne]

Ship it once CI is green!

libcxx/lib/abi/CHANGELOG.TXT

 `generate-cxx-abilist` target, and look at the diff.
 
 New entries should be added directly below the "Version" header.
 
 ------------
 Version 14.0
 ------------
 
+* [libc++] Resolve missing table_size symbol
+
+  This commit added an out-of-line definition for `table_size` in the library.
+  This is not an ABI break since we are just adding a symbol to the library.
+  In fact, any program that would have attempted to refer to that symbol would
+  have previously failed to link.
+
+  x86_64-apple-apple-darwin
+  -------------------------
+  Symbol added: _ZNSt3__15ctypeIcE10table_sizeE
+
+  x86_64-unknown-linux-gnu
+  ------------------------
+  Symbol added: _ZNSt3__15ctypeIcE10table_sizeE
+
 * [libc++] Always define a key function for std::bad_function_call in the dylib
 
   This commit added a new explicit instantiation of std::bad_function_call's
   vtable in the shared library. This change is not an ABI break as it only
   adds symbols to the library. Programs compiled against the library will not
   rely on the vtable being defined in it by default. Reliance on the vtable
   being in the shared library can be enabled by defining the
   _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION macro.

libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist

 {'is_defined': True, 'name': '__ZNSt3__14stofERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stofERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stoiERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stoiERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stolERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stolERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14wcinE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__15alignEmmRPvRm', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__15ctypeIcE10table_sizeE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcE13classic_tableEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcE2idE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcEC1EPKjbm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcEC2EPKjbm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcED0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcED1Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcED2Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIwE2idE', 'size': 0, 'type': 'OBJECT'}

 {'is_defined': True, 'name': '___cxa_vec_delete2', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_delete3', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_dtor', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new2', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new3', 'type': 'I'}
 {'is_defined': True, 'name': '___dynamic_cast', 'type': 'I'}
 {'is_defined': True, 'name': '___gxx_personality_v0', 'type': 'I'}
 No newline at end of file

libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist

 {'is_defined': True, 'name': '__ZNSt3__14stofERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stofERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stoiERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stoiERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stolERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14stolERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__14wcinE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__15alignEmmRPvRm', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__15ctypeIcE10table_sizeE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcE13classic_tableEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcE2idE', 'size': 0, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcEC1EPKjbm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcEC2EPKjbm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcED0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcED1Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIcED2Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__15ctypeIwE2idE', 'size': 0, 'type': 'OBJECT'}

 {'is_defined': True, 'name': '___cxa_vec_delete2', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_delete3', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_dtor', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new2', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new3', 'type': 'I'}
 {'is_defined': True, 'name': '___dynamic_cast', 'type': 'I'}
 {'is_defined': True, 'name': '___gxx_personality_v0', 'type': 'I'}
 No newline at end of file

libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist

 {'is_defined': True, 'name': '_ZNSt3__14stofERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__14stofERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__14stoiERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__14stoiERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__14stolERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__14stolERKNS_12basic_stringIwNS_11char_traitsIwEENS_9allocatorIwEEEEPmi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__14wcinE', 'size': 168, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '_ZNSt3__15alignEmmRPvRm', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__15ctypeIcE10table_sizeE', 'size': 8, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcE13classic_tableEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcE21__classic_lower_tableEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcE21__classic_upper_tableEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcE2idE', 'size': 16, 'type': 'OBJECT'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcEC1EPKtbm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcEC2EPKtbm', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcED0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__15ctypeIcED1Ev', 'type': 'FUNC'}

 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__113basic_ostreamIcNS_11char_traitsIcEEED0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__113basic_ostreamIcNS_11char_traitsIcEEED1Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__113basic_ostreamIwNS_11char_traitsIwEEED0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__113basic_ostreamIwNS_11char_traitsIwEEED1Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__114basic_iostreamIcNS_11char_traitsIcEEED0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__114basic_iostreamIcNS_11char_traitsIcEEED1Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__19strstreamD0Ev', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZTv0_n24_NSt3__19strstreamD1Ev', 'type': 'FUNC'}
 No newline at end of file

libcxx/src/locale.cpp

ctype<wchar_t>::do_narrow(const char_type* low, const char_type* high, char dfault, char* dest) const
     return low;
 }
 #endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS
 
 // template <> class ctype<char>;
 
 locale::id ctype<char>::id;
 
+const size_t ctype<char>::table_size;
+
 ctype<char>::ctype(const mask* tab, bool del, size_t refs)
     : locale::facet(refs),
       __tab_(tab),
       __del_(del)
 {
   if (__tab_ == 0)
       __tab_ = classic_table();
 }

libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <locale>
+
+// template <> class ctype<char>
+
+// Make sure we can reference std::ctype<char>::table_size.
+
+// Before https://llvm.org/D110647, the shared library did not contain
+// std::ctype<char>::table_size, so this test fails with a link error.
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{11|12|13}}
+
+#include <locale>
+#include <cassert>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+  typedef std::ctype<char> F;
+  const size_t* G = &F::table_size;
+  assert(*G >= 256);
+
+  return 0;
+}