Created attachment 231376 [details] git diff in audio/jack Similar problem as fixed in https://cgit.freebsd.org/ports/commit/?id=159b44b2fb70889722fbf810f22aea435848684d but on aarch64. - tested in poudriere on rpi4/aarch64/14-CURRENT
I'm certainly fine with the workaround at the moment. But I'd like to solve this problem with Clang eventually - could you provide a build log of the error? I'm particularly interested in the compiler version and compile flags, AFAIK there are successful builds on MacOS with Clang / aarch64. The original PR causing this is found here: https://github.com/jackaudio/jack2/pull/761 I may open a new one, thus not adding that to the URL field.
(In reply to Florian Walpen from comment #1) It is in the official pkg builders of FreeBSD. http://www.ipv6proxy.net/go.php?u=http%3A%2F%2Fampere2.nyi.freebsd.org%2Fdata%2Fmain-arm64-default%2Fpd8f8cc3a8823_s4f0e50b293%2Flogs%2Ferrors%2Fjackit-1.9.20.log&b=0&f=norefer
NB: it builds fine on 13.0. http://ampere1.nyi.freebsd.org/data/130arm64-default/7201e24c4c91/logs/jackit-1.9.16_1.log (link is ipv6 only, use ipv6proxy.net if needed)
(In reply to Ronald Klop from comment #3) Good pointer, I'll try to reproduce this on 14.0-CURRENT too.
Actually I'd like to give maintainer-approval to the patch, but cannot set the flag. @Ronald, is it possible to add me (dev@submerge.ch) as maintainer to the patch? Maybe that helps.
Update: I can't reproduce the error. Neither with base Clang on 14.0-CURRENT, cross-compiling to arm64 and powerpc, nor with any other combination I tried. Looks like this is specific to the native base compilers of arm64 and powerpc*.
It's still an issue. Can we import https://github.com/0EVSG/jack2/commit/1e2dc2a029259960dbe4eb698bb3d048262ac321 ?
Created attachment 231447 [details] v0
(In reply to Mikael Urankar from comment #7) The GCC workaround hasn't been committed yet, or what do you mean by still an issue? Regarding the github patch, it will not go upstream and I don't want to guarantee its existence forever - I'd prefer an offline patch in audio/jack/files for that. But I don't even know whether it's effective, since I can't reproduce the error. Someone with Arm64 or PowerPC hardware has to test. I still think we should try to identify the problem with Clang / LLVM on these platforms. It doesn't make sense to require 8 byte alignment in this case. Can someone with the hardware try to build with llvm* from ports for comparison?
(In reply to Florian Walpen from comment #9) On rpi4/aarch64/14-CURRENT: Building with gcc10 works. Building with llvm12 from ports gives the alignment errors: In file included from ../common/JackEngineProfiling.cpp:23: ../common/JackEngineControl.h:67:5: error: requested alignment is less than minimum alignment of 8 for type 'Jack::JackTransportEngine' alignas(UInt32) JackTransportEngine fTransport; ^ ../common/JackEngineControl.h:89:5: error: requested alignment is less than minimum alignment of 8 for type 'Jack::JackFrameTimer' alignas(UInt32) JackFrameTimer fFrameTimer; ^ 2 errors generated.
@Ronald: Thanks for testing! @Mikael: You have commit bits, right? I would suggest to just use the patch provided by Ronald for now - and build with GCC. We probably won't have a better answer in good time. Thanks for your efforts! What bothers me is that I cannot reproduce the error when cross-compiling. In principle, that should produce ABI compatible code and thus run into the same alignment problem. For the records, I'm following the method described here: https://mcilloni.ovh/2021/02/09/cxx-cross-clang/ On 14.0-CURRENT and x86_64, download a sysroot from the official 14.0-CURRENT snapshot for arm64 (or powerpc). Then cross-compile, mimicking the compile flags from the build logs as closely as possible: c++ --target=aarch64-unknown-freebsd14.0 --sysroot=/tmp/sysroot/arm64 -mcpu=cortex-a72 -stdlib=libc++ -O2 -pipe -fPIC -fstack-protector-strong -fno-strict-aliasing -Wall -Wno-invalid-offsetof -std=gnu++11 -fPIC -Ifreebsd -I../freebsd -Iposix -I../posix -Icommon -I../common -Icommon/jack -I../common/jack -I. -I.. -I../compat/alloca -I/usr/local/include/opus -I/usr/local/include -I/usr/local/include/dbus-1.0 -I/usr/local/lib/dbus-1.0/include -DEXECINFO=1 -DHAVE_LIBSYSINFO=1 -DHAVE_DOXYGEN=0 -DHAVE_ALSA=0 -DHAVE_FIREWIRE=0 -DHAVE_IIO=0 -DHAVE_PORTAUDIO=0 -DHAVE_WINMME=0 -DHAVE_CELT=0 -DHAVE_EXAMPLE_TOOLS=0 -DHAVE_OPUS_OPUS_CUSTOM_H=1 -DHAVE_OPUS_PKG=1 -DHAVE_OPUS=1 -DHAVE_SAMPLERATE=1 -DHAVE_SNDFILE=0 -DHAVE_READLINE=0 -DHAVE_SYSTEMD=0 -DHAVE_DB_H=1 -DHAVE_DB=0 -DHAVE_ZALSA=0 -DHAVE_PPOLL=1 -DHAVE_EXECINFO_H=1 -DJACK_VERSION="[]" -DHAVE_DBUS_1=1 -DHAVE_EXPAT=1 -DUSE_LIBDBUS_AUTOLAUNCH=1 -DCLIENT_NUM=256 -DPORT_NUM_FOR_CLIENT=2048 -DADDON_DIR="/home/flo/Install/lib/jack" -DJACK_LOCATION="/home/flo/Install/bin" -DUSE_POSIX_SHM=1 -DJACKMP=1 -DJACK_DBUS=1 -DHAVE_CONFIG_H ../common/JackDebugClient.cpp -c -o/usr/home/flo/Source/jack2/build/common/JackDebugClient.cpp.1.o -I/usr/local/include Am I missing something? I played with target and cpu quite a bit, no luck. I'll test with qemu when I get around to it, but that will take some time.
(In reply to Florian Walpen from comment #11) Yes I have commit right but I'd prefer to not depend on gcc and get jack fixed instead.
Just a pointer to the upstream issue: https://github.com/jackaudio/jack2/issues/839 . And tested llvm11 and llvm10 on aarch64/14-CURRENT. Same error. So something native aarch64. Not a recent regression in llvm.
(In reply to Mikael Urankar from comment #12) Fair enough, but given that * Clang and GCC produce different alignments, not to mention cross-build * the classes in question are in shared memory (client server ABI) * some client ports require GCC to build solving this only on the JACK side is not possible with my github patch. One solution could be: * Explicitly set the alignments to 8 bytes instead of 4. * Bump all client ports for ABI compatibility. * Lots of testing, expect some strange bug reports. Again, FreeBSD would be the only one going that route, upstream is very unlikely to follow unless that problem also manifests on other platforms. That's why I focus on finding the cause of the odd 8 byte alignment requirement by Clang now. Obviously I can't give a time estimate for that.
CC'ing dim@ maybe he can shed some lights on this issue.
(In reply to Mikael Urankar from comment #15) As far as I can see, class instances must be aligned to the minimum 'natural' alignment for the largest class member. So for example: struct Foo { long l; }; struct Bar { alignas(int) Foo foo; }; Even on amd64 this gives: % clang -c foo.cpp foo.cpp:6:3: error: requested alignment is less than minimum alignment of 8 for type 'Foo' alignas(int) Foo foo; ^ 1 error generated. For some reason, gcc does not care about this...
Did the qemu excercise in zen patience, now I can reproduce the error. And I think I found the culprit. In JACK headers, the packed attribute #define POST_PACKED_STRUCTURE __attribute__((__packed__)) gets undefined explicitly for arm, mips and powerpc architectures - except when on MacOS. It looks like cross-compilation doesn't define "__aarch64__" or "__powerpc__" by itself, which is why I couldn't reproduce the error there. I will talk to upstream and find out why they undefined it in the first place.
Created attachment 231505 [details] Replace alignas(UInt32) with aligned(4) (In reply to Dimitry Andric from comment #16) Ok, if it isn't possible to change the existing alignas() values, and the members *must* be aligned to 4 bytes, the solution is to avoid using alignas(), as it can never be used to 'misalign' members, and use __attribute__((__aligned__())) instead. For example, as in the attached patch.
Created attachment 231525 [details] Fix build, let compiler choose alignment for non-packed classes. This supposedly fixes the alignment errors on arm64 and powerpc platforms. Use "git am" in ports to apply the patch. The alignment errors were exposed by the following combination: * alignas() is not allowed to weaken alignment requirements. * Clang enforces this according to standard, GCC does not. * JACK uses packed structs on x86 platforms, but not on others. Use multiple alignas() specifiers to let the compiler choose between minimum alignment (packed) and "natural" alignment (non-packed). The benefit of this approach is that it's both standard c++11 and compatible with older GCC, which does not propagate alignment requirements of substructures properly. Revert the GCC workarounds on powerpc, we have to test Clang.
@Ronald, @Piotr: Could you please test my patch on arm64 and powerpc? I still have to verify ABI compatibility between Clang and GCC, for JACK clients that need GCC to build. If all goes well I'll create a PR upstream. @Dimitry: Thanks for your help - I think multiple alignas() specifiers should have the same "relaxing" effect as the attribute, right?
(In reply to Florian Walpen from comment #20) With the patch in attachment #231525 [details] the port compiles ok for me on rpi4/aarch64/14-CURRENT with the base llvm.
jack with patch from comment 19 builds on powerpc64. I did not test on powerpc and powerpc64le, but since the error I encountered previously was the same, I assume it's also ok.
Update, the upstream PR based on my patch is here: https://github.com/jackaudio/jack2/pull/845 I did some experiments with GCC 10 and GCC 6 from ports. They seem to ignore weaker alignment requirements in our use cases. Therefore I don't expect this change to introduce any ABI incompatibility. On a side note, GCC has a whole slew of alignas() related bugs, gathered in this meta bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64236
My patch was tested and merged upstream, so we're good to go. Set maintainer approval accordingly. @Mikael: Is the patch fine for you like this? Now that it has been merged upstream we could also draw in the patch from github if you prefer. Me, I don't care. The commit would be https://github.com/jackaudio/jack2/commit/21b293dbc37d42446141a08922cdec0d2550c6a0 Thanks to everyone involved!
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=22c0c4f2a6e599c4530dc47025f9f7153262f381 commit 22c0c4f2a6e599c4530dc47025f9f7153262f381 Author: Mikael Urankar <mikael@FreeBSD.org> AuthorDate: 2022-02-09 14:49:45 +0000 Commit: Mikael Urankar <mikael@FreeBSD.org> CommitDate: 2022-02-09 16:19:35 +0000 audio/jack: Fix alignas() on non-packed architectures Unbreak the build with Clang on architectures where JACK uses non-packed data structures, like arm64 or powerpc. The alignment errors are exposed there because: * The non-packed data structures require 8 byte alignment. * alignas() is not allowed to weaken alignment requirements to 4 bytes. * Clang enforces this according to standard, GCC ignores it. Use an additional alignas() specifier to let the compiler choose between minimum alignment (packed) and "natural" alignment (non-packed). This is both standard c++11 and compatible with older GCC, which does not propagate alignment requirements of packed substructures properly. PR: 261508 Submitted by: Florian Walpen <dev@submerge.ch> audio/jack/Makefile | 3 +++ audio/jack/distinfo | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-)
(In reply to Piotr Kubaj from comment #22) Piotr, I'll let you test on ppc64 and remove the gcc deps if it's not necessary after 22c0c4f2a6e599c4530dc47025f9f7153262f381