Re: [PATCH v5 09/11] 9p: darwin: Implement compatibility for mknodat

[Christian Schoenebeck]

On Dienstag, 8. Februar 2022 23:57:32 CET Will Cohen wrote:
> > On a second thought: this case a bit special. Are we worried that
> > pthread_fchdir_np() is "not yet" available on macOS, or "no longer"
> > available.
> > Probably both, right?
> > 
> > So maybe it would make sense to replace the API_AVAILABLE() attribute
> > directly
> > with a __attribute__((weak)) attribute. Then the runtime check with the
> > proposed error message would also trigger if a bleeding edge macOS version
> > no
> > longer has pthread_fchdir_np().
> > 
> > Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED
> > and
> > MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at
> > compile
> > time to wrap deployment target dependent code accordingly.
> > 
> > On doubt you could just make some tests there by simply "inventing" a non-
> > existent function.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> I like the idea of switching it to __attribute__((weak)). I should note
> that I'm not sure that I can actually fully test this out since I'm getting
> stuck with the linker noting my undefined fake function during the build,
> but the idea does make logical sense to me for the future fail case and the
> happy case builds again when implemented with actual pthread_fchdir_np:
> 
> [1075/2909] Linking target qemu-nbd
> FAILED: qemu-nbd
> cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
> -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> libio.fa -fstack-protector-strong
> -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa
> libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms
> /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0
> -lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib
> -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> -lgmodule-2.0 -lglib-2.0 -lintl
> /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework
> CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib
> -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
> Undefined symbols for architecture x86_64:
>   "_pthread_fchdir_npfoo", referenced from:
>       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see

Even when "weak" linking, the respective symbol must still at least exist at 
compile time. To make this more clear, the following test works for me:

$ cat a.c
#include <stdio.h>

void aaa(void) {
    printf("aaa\n");
}
$ cat ab.c
#include <stdio.h>

void aaa(void) {
    printf("aaa\n");
}

void bbb(void) {
    printf("bbb\n");
}
$ cat main.c
#include <stdio.h>

void aaa(void);
void bbb(void) __attribute__((weak));

int main() {
    aaa();
    if (bbb)
        bbb();
    else
        printf("bbb() not supported\n");
    return 0;
}
$ clang -dynamiclib ab.c -o foo.1.dylib
$ clang main.c -o weaktest foo.1.dylib
$ ./weaktest
aaa
bbb
$ clang -dynamiclib a.c -o foo.1.dylib
$ ./weaktest
aaa
bbb() not supported
$

> With that caveat re testing in mind, unless there's another recommended
> path forward, I think it makes sense to stick with __attribute__((weak))
> and prepare v6 which incorporates this and all the other feedback from this
> round.

Just make this clear with an appropriate comment why it is weakly linked: to 
guard that function pthread_fchdir_np might simply disappear in future macOS 
versions, as it is simply a private API.

Best regards,
Christian Schoenebeck