managarm / mlibc

Portable C standard library
Other
851 stars 130 forks source link

meson: hide all libc.so C++ symbols via a linker script #1119

Closed ArsenArsen closed 1 month ago

ArsenArsen commented 2 months ago

result: https://gist.github.com/ArsenArsen/570e87410c48f5b17f46d579269f10c2

btw, I noticed that we have a C++ function that probably should not be a C++ function:

0000000000068a58 T seed48(unsigned short*)
000000000008354a T login_tty(int)
000000000009f478 T wcstoimax(wchar_t const*, wchar_t**, int)
000000000009f4b7 T wcstoumax(wchar_t const*, wchar_t**, int)

this would imply we lack tests for those..

I recall some discussion about how to detect such cases before in the mlibc channel, but I do not recall the result. if you can find it/remember it, please share

no92 commented 2 months ago

Detecting such errors might also be a convenient extra feature for a possible ABI checker.

mintsuki commented 2 months ago

LGTM

ArsenArsen commented 2 months ago

obviously the abi break is expected

no92 commented 1 month ago

As is, this needs some changes; managarm uses __mlibc_getPassthrough in a few places, so this PR currently breaks boot.

ArsenArsen commented 1 month ago

that shouldn't break, since this only excludes _Z*? or am I missing something?

qookei commented 1 month ago

It's not marked extern "C" :^)

ArsenArsen commented 1 month ago

that's a silly name for a C++ function.. but, OK, we can export it also

qookei commented 1 month ago

Marking it extern "C" would be simpler, and I suppose more correct for a libc

ArsenArsen commented 1 month ago

and an abi break, which might be problematic depending on what it's used for

Geertiebear commented 1 month ago

Does that actually get used by other binaries?

ArsenArsen commented 1 month ago

I imagine boot wouldn't fail without it if it wasn't

ArsenArsen commented 1 month ago

based on the other __mlibc_ functions not being mangled I'm guessing that this is unintentionally not extern C. anyway, try this. might require that we start passing --no-undefined-version at some point

ArsenArsen commented 1 month ago

don't merge yet, need to add a dep to the link_deps of the .so

edit: done, pls test

no92 commented 1 month ago

With managarm/managarm#715, the need to make __mlibc_getPassthrough be extern "C" is avoided. It no longer needs to be declared with C linkage or be excepted in the linker script.

ArsenArsen commented 1 month ago

okay, I'll revert that

no92 commented 1 month ago

Works on my machine™