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

[Akihiko Odaki]

On Thu, Feb 10, 2022 at 3:20 AM Will Cohen <wwcohen@gmail.com> wrote:
>
> On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <qemu_oss@crudebyte.com> 
> wrote:
>>
>> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
>> > > 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
>> > > invocation) ninja: build stopped: subcommand failed.
>> > > make[1]: *** [run-ninja] Error 1
>> > > make: *** [all] Error 2
>> > >
>> > > 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.
>> > __attribute__((weak_import)), which explicitly marks the function as
>> > external, is more appropriate here. It is feature-equivalent with the
>> > availability attribute when the minimum deployment target is lower
>> > than the version which introduced the function.
>>
>> Thanks for chiming in on this macOS issue Akihiko!
>>
>> Are you sure that "weak_import" is still the preferred way? From behaviour 
>> PoV
>> I do not see any difference at all. I can't even tell what the intended
>> difference was, and QEMU currently only seems to use "weak" in the entire 
>> code
>> base.
>>
>> Googling around, "weak_import" seems to be required on ancient OS X versions
>> only and that it's now deprecated in favour of the more common "weak", no?
>>
>> Best regards,
>> Christian Schoenebeck
>
>
> Either way seems reasonable to me. For reference, what I'm seeing on Google 
> and what Christian may be referring to is a circa-2016 conversation on GCC 
> bugzilla, where a tentative conclusion seems to be that the distinction 
> between the two is small and weak is probably preferred now: 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179
>

GCC doesn't maintain features specific to Apple well so we should look
at clang. Compiling QEMU for macOS with GCC would result in errors
anyway because QEMU uses clang extensions like availability checks and
blocks for Apple's ABIs/APIs. clang still distinguishes
__attribute__((weak)) and __attribute__((weak_import)).

The present uses of __attribute__((weak)) in QEMU are correct and
shouldn't be replaced with __attribute__((weak_import)) even when
targeting macOS since they have default implementations and are
statically resolved.

Regards,
Akihiko Odaki