Internal linkage in Picolib breaks libcxx module tests

[mordante]

At least since C11 "Any declaration of a library function shall have external linkage." This is important for C++20 modules. Named declarations with internal linkage can't be exported, which prevents using these declarations in the C++23 std and std.compat module.

Several declarations in Picolib have internal linkage, for example static __inline int isblank(int c). See https://buildkite.com/llvm-project/libcxx-ci/builds/32428#018c9247-50ba-4728-b13d-fb064eba3707 for more issues. This prevents using this library in the C++ modules

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Mark de Wever (mordante)

At least since [C11](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf#%5B%7B%22num%22%3A429%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C-27%2C816%2Cnull%5D) "Any declaration of a library function shall have external linkage." This is important for C++20 modules. Named declarations with internal linkage can't be exported, which prevents using these declarations in the C++23 `std` and `std.compat` module.

Several declarations in Picolib have internal linkage, for example static __inline int isblank(int c). See https://buildkite.com/llvm-project/libcxx-ci/builds/32428#018c9247-50ba-4728-b13d-fb064eba3707 for more issues. This prevents using this library in the C++ modules

[DavidSpickett]

To reproduce, check out the branch https://github.com/llvm/llvm-project/tree/users/mordante/adds_module_testing and remove the PicoLib xfail:

diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index 1939d553d72d..9a676f98d104 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -324,7 +324,7 @@ DEFAULT_FEATURES = [
         # This is not allowed per C11 7.1.2 Standard headers/6
         #  Any declaration of a library function shall have external linkage.
         when=lambda cfg: "__ANDROID__" in compilerMacros(cfg)
-        or "__PICOLIBC__" in compilerMacros(cfg)
+#        or "__PICOLIBC__" in compilerMacros(cfg)
         or platform.system().lower().startswith("aix")
         # Avoid building on platforms that don't support modules properly.
         or not hasCompileFlag(cfg, "-Wno-reserved-module-identifier"),

Then make sure you have clang 16.x or higher, and qemu-system-arm 8.1.3 or greater.

~/modules-llvm-project$ ./libcxx/utils/ci/run-buildbot armv7m-picolibc

********************
Failed Tests (5):
  armv7m-picolibc-libc++.cfg.in :: libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
  armv7m-picolibc-libc++.cfg.in :: libcxx/selftest/modules/std-module.sh.cpp
  armv7m-picolibc-libc++.cfg.in :: libcxx/selftest/modules/std.compat-module.sh.cpp
  armv7m-picolibc-libc++.cfg.in :: std/modules/std.compat.pass.cpp
  armv7m-picolibc-libc++.cfg.in :: std/modules/std.pass.cpp

The builder downloads the picolib release each time, might have to hack in a local path for that as you make changes. The version in use now is 1.8.5.

[DavidSpickett]

I've brought this to the attention of the team at Arm handling this area.

[mordante]

I've brought this to the attention of the team at Arm handling this area.

Thanks! Note that this issue is not just needed for libc++, it's required for conformance with the C standard too.

[domin144]

I am trying to sort out the isblank issue in upstream.

There are other errors related to std::get_deleter. I think these are caused by disabled RTTI in one of the picolibc variants.

[keith-packard]

Where available, picolibc uses the gnu_inline attribute to make this work while still providing inline acceleration of these operations.

Example:

file-1.c:

#include <stdlib.h>
#include <stdio.h>

extern int foo(int y);

extern __inline int __attribute((gnu_inline, always_inline)) foo(int y)
{
	return y + 1;
}

int main(int argc, char **argv)
{
	printf("%d\n", foo(atoi(argv[1])));
	return 0;
}

file-2.c:

extern int foo(int y);

extern __inline int __attribute((gnu_inline, always_inline)) foo(int y)
{
	return y + 1;
}

int
foo(int y)
{
	return y + 2;
}

Note that the inline and extern versions generate different results (which is "wrong", but useful for testing).

$ cc file-1.c file-2.c
$ nm a.out | grep foo
0000000000001191 T foo
$ ./a.out 10
11
$

As you can see, foo is a real function, but when invoked, the application uses the inline version.

[mordante]

I am trying to sort out the isblank issue in upstream.

Great to hear.

There are other errors related to std::get_deleter. I think these are caused by disabled RTTI in one of the picolibc variants.

I noticed that too, I just added a fix here f8bf61f

@keith-packard is this the current situation or the proposed change? In the example static __inline int isblank(int c) uses
static not extern __inline int __attribute((gnu_inline, always_inline)) isblank(int c).

[keith-packard]

@keith-packard is this the current situation or the proposed change? In the example static __inline int isblank(int c) uses static not extern __inline int __attribute((gnu_inline, always_inline)) isblank(int c).

isblank is an exception, and this represents a bug. I'm working on fixing this, but there's a bigger mess in the fenv support that needs clearing out as well. The current status of picolibc/picolibc#668 shows the proposed fix for isblank stack on top of a general cleanup of the use of gnu_inline. I'll merge that in and then continue the fenv cleanup on top.

[mordante]

With the picolib updates this issue is resolved and modules are now tested with Picolib in the CI.