[libcxx-commits] [PATCH] D110647: Resolve missing table_size symbol

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D110647#3122226 <https://reviews.llvm.org/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 symbols.

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`.



================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp:11
+
+// template <> class ctype<char>
+
----------------
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.
```


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp:12
+// template <> class ctype<char>
+
+#include <locale>
----------------
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.


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/table_size.pass.cpp:23
+  typedef std::ctype<char> F;
+  const size_t *G = &F::table_size;
+  assert(*G >= 256);
----------------
muiez wrote:
> jloser wrote:
> > jloser wrote:
> > > Do we need the pointer? Or can we just say
> > > ```
> > > assert(F::table_size >= 256);
> > > ```
> > It could be a `static_assert` even, right?
> 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`).
I think it is good to have the assert since `std::ctype<char>::table_size` is specified as being at least 256.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110647/new/

https://reviews.llvm.org/D110647