[libc++] Use intptr_t instead of ptrdiff_t for messages_base::catalog

[ldionne]

Summary

On GLibc, FreeBSD and macOS systems nl_catd is a pointer type, and
round-tripping this in a variable of ptrdiff_t is not portable.
In fact such a round-trip yields a non-dereferenceable pointer on
CHERI-enabled architectures such as Arm Morello. There pointers (and
therefore intptr_t) are twice the size of ptrdiff_t, which means casting
to ptrdiff_t strips the high (metadata) bits (as well as a hidden pointer
validity bit).

Since catalog is now guaranteed to be the same size or larger than nl_catd,
we can store all return values safely and the shifting workaround from
commit 0c68ed006d4f38c3cdcab6a565aa3e208015895f should not be needed
anymore (this is also not portable to CHERI systems on since shifting a
valid pointer right will create a massively out-of-bounds pointer that
may not be representable).

This can be fixed by using intptr_t which should be the same type as
ptrdiff_t on all currently supported architectures.

See also: https://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2028

[Mordante]

libcxx/include/locale

3458

Did you check these types are the same on all supported platforms.
I noticed intptr_t is not guaranteed to be available on all platforms. Is that an issue?

3533

Is this code used in C++03? static_assert is a C++11 feature.

[philnik]

libcxx/include/locale

3533

We have static_assert in C++03. We #define static_assert(...) _Static_assert(__VA_ARGS__) in __config.

[arichardson]

libcxx/include/locale

3458

I've looked at clang and for almost all targetinfo classes PtrDiffType uses the same type as IntPtrType. The only exception I can see is MIPS O32 ABI which uses Int for ptrdiff_t but long for intptr_t (those are the same size just different integer types). In case this is a problem, I'm not sure if MIPS O32 ABI is actually supported - if 32-bit is supported I would assume it's only N32 ABI.

FreeBSD stddef.h checks SIZEOF_PTRDIFF_T and SIZEOF_POINTER, so if those match inside clang they will use the same types (which is true for all FreeBSD architectures other than CHERI-extended ones, which are not supported upstream)

I think in theory they could be different with other OS/compiler-provided stddef.h, but I'd be surprised if they were since we don't yet support any architectures that don't have a flat address space (which should imply size_t==intptr_t).

[arichardson]

libcxx/include/locale

3458

I guess this could change mangled names for 32-bit MIPS, but https://libcxx.llvm.org/#platform-and-compiler-support does not list MIPS anywhere, so maybe this is okay? Otherwise I could add ifdefs (but I'd prefer not to).

[arichardson]

rebase for CI

[Mordante]

LGTM, but I think it would be good for @ldionne to have a look too.

libcxx/include/locale

3458

Thanks for the info, then it seems it's not an issue to make this change.

3533

Thanks.

[arichardson]

@ldionne do you think this is acceptable, or should I add additional ifdefs to avoid potential name mangling changes on 32-bit MIPS? To be honest I'm not sure if this type ever leaks into mangled names or if it's only ever used in inlined functions.

[arichardson]

ping?

[arichardson]

ping @ldionne . Would be nice to land this so that I can successfully run the libc++ testsuite on Morello.

[arichardson]

ping @ldionne. I am pretty certain this is ABI compatible with all supported architectures, but I can add a release note if you like?

[arichardson]

Is there any chance someone else who has the ability to approve this for #libc could do so since it appears @ldionne is not interested in this change. I get that this currently does not break any production targets, but IMO it does clean up the code for glibc and macOS as well.

[ldionne]

In D134420#4485499, @arichardson wrote:

Is there any chance someone else who has the ability to approve this for #libc could do so since it appears @ldionne is not interested in this change. I get that this currently does not break any production targets, but IMO it does clean up the code for glibc and macOS as well.

I'm not disinterested, I just didn't see it amongst the ton of changes that I get pinged on. Let me take a look now.

[ldionne]

This seems like a good change to me. I had ABI impact concerns but I read and re-read your commit + the discussion in the thread, and I am fine with this.

LGTM but we need to add a few tests, rebase this and get a green CI run.

libcxx/include/locale

3450

Can we add a libc++ specific test that asserts is_same<messages_base::catalog, intptr_t>?

Also a non-libc++-specific test to check for the implementation of LWG2028? It could assert that is_signed<messages_base::catalog>.

[ldionne]

Oh and thanks for the cleanup BTW, this is a lot better than what we had before.

[arichardson]

In D134420#4485745, @ldionne wrote:

Oh and thanks for the cleanup BTW, this is a lot better than what we had before.

Thanks for the review, will try to address your comments ASAP

[ldionne]

[Github PR transition cleanup]

Commandeering to finish.

[ldionne]

Address comments and rebase.

[arichardson]

Thanks for updating this change and apologies for not getting to it myself!

[ldionne]

No worries!

Shipping now, the CI failure is something else.

libcxx/include/locale

money_put<_CharT, _OutputIterator>::do_put(iter_type __s, bool __intl,
     this->__format(__mb, __mi, __me, __iob.flags(),
                    __digits.data(), __digits.data() + __digits.size(), __ct,
                    __neg, __pat, __dp, __ts, __grp, __sym, __sn, __fd);
     return std::__pad_and_output(__s, __mb, __mi, __me, __iob, __fl);
 }
 
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS money_put<char>;
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS money_put<wchar_t>;
 #endif
 
 // messages
 
 class _LIBCPP_EXPORTED_FROM_ABI messages_base
 {
 public:
-    typedef ptrdiff_t catalog;
+    typedef intptr_t catalog;
 
     _LIBCPP_INLINE_VISIBILITY messages_base() {}
 };
 
 template <class _CharT>
 class _LIBCPP_TEMPLATE_VIS messages
     : public locale::facet,
       public messages_base

 locale::id
 messages<_CharT>::id;
 
 template <class _CharT>
 typename messages<_CharT>::catalog
 messages<_CharT>::do_open(const basic_string<char>& __nm, const locale&) const
 {
 #ifdef _LIBCPP_HAS_CATOPEN
-    catalog __cat = (catalog)catopen(__nm.c_str(), NL_CAT_LOCALE);
-    if (__cat != -1)
-        __cat = static_cast<catalog>((static_cast<size_t>(__cat) >> 1));
-    return __cat;
+    return (catalog)catopen(__nm.c_str(), NL_CAT_LOCALE);
 #else // !_LIBCPP_HAS_CATOPEN
     (void)__nm;
     return -1;
 #endif // _LIBCPP_HAS_CATOPEN
 }
 
 template <class _CharT>
 typename messages<_CharT>::string_type
 messages<_CharT>::do_get(catalog __c, int __set, int __msgid,
                          const string_type& __dflt) const
 {
 #ifdef _LIBCPP_HAS_CATOPEN
     string __ndflt;
     __narrow_to_utf8<sizeof(char_type)*__CHAR_BIT__>()(std::back_inserter(__ndflt),
                                                        __dflt.c_str(),
                                                        __dflt.c_str() + __dflt.size());
-    if (__c != -1)
-        __c <<= 1;
     nl_catd __cat = (nl_catd)__c;
+    static_assert(sizeof(catalog) >= sizeof(nl_catd), "Unexpected nl_catd type");
     char* __n = catgets(__cat, __set, __msgid, __ndflt.c_str());
     string_type __w;
     __widen_from_utf8<sizeof(char_type)*__CHAR_BIT__>()(std::back_inserter(__w),
                                                         __n, __n + _VSTD::strlen(__n));
     return __w;
 #else // !_LIBCPP_HAS_CATOPEN
     (void)__c;
     (void)__set;
     (void)__msgid;
     return __dflt;
 #endif // _LIBCPP_HAS_CATOPEN
 }
 
 template <class _CharT>
 void
 messages<_CharT>::do_close(catalog __c) const
 {
 #ifdef _LIBCPP_HAS_CATOPEN
-    if (__c != -1)
-        __c <<= 1;
-    nl_catd __cat = (nl_catd)__c;
-    catclose(__cat);
+    catclose((nl_catd)__c);
 #else // !_LIBCPP_HAS_CATOPEN
     (void)__c;
 #endif // _LIBCPP_HAS_CATOPEN
 }
 
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS messages<char>;
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS messages<wchar_t>;

libcxx/test/std/localization/locale.categories/category.messages/locale.messages/messages_base.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>
 
 // class messages_base
 // {
 // public:
 //     typedef unspecified catalog;
 // };
 
+#include <cstdint>
 #include <locale>
 #include <type_traits>
 
-#include "test_macros.h"
+#include "assert_macros.h"
 
-int main(int, char**)
-{
-    std::messages_base mb;
+#ifdef _LIBCPP_VERSION
+ASSERT_SAME_TYPE(std::messages_base::catalog, std::intptr_t);
+#endif
+
+// Check that we implement LWG2028
+static_assert(std::is_signed<std::messages_base::catalog>::value, "");
+static_assert(std::is_integral<std::messages_base::catalog>::value, "");
+
+int main(int, char**) {
 
   return 0;
 }