Bug 260625
| Summary: | benchmarks/lzbench: Fix build on arm and riscv64, Remove BROKEN, Respect *FLAGS | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Robert Clausecker <fuz> | ||||
| Component: | Individual Port(s) | Assignee: | Alexey Dokuchaev <danfe> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Many People | CC: | danfe, grahamperrin, mikael | ||||
| Priority: | --- | Keywords: | needs-qa | ||||
| Version: | Latest | Flags: | danfe:
maintainer-feedback+
koobs: maintainer-feedback? koobs: merge-quarterly? |
||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Comment on attachment 230330 [details]
benchmarks/lzbench: fix build on arm and riscv64
maintainer timeout
(In reply to Robert Clausecker from comment #1) Sorry, I somehow missed this PR. Also, one should use feedback to minus (-) for maintainer timeouts. ;-) So, if I read the patch correctly, there are three problems that prevent the build on ARM and RISC-V: 1) missing (int64_t*) cast in pithy_Load64() for ARM 2) missing __riscv_xlen == 64 check for FREEARC_64BIT 3) RISC-V compiler does not understand -march=native The first two seem reasonable, but the last one bugs me: it is used in the port to detect whether the target CPU supports SSE4.1 instructions for the particular compressor; it is not actually used for the build. Why isn't this switch supported by our compiler on RISC-V? (In reply to Alexey Dokuchaev from comment #3) RISC-V provides no portable method to detect the CPU feature set, so like on most architectures, -march=native is not supported. And even if it would be: packages should not adapt themselves to the hardware on compilation. See §13.14.1 Porter's Handbook. (In reply to Robert Clausecker from comment #4) > RISC-V provides no portable method to detect the CPU feature set, so like > on most architectures, -march=native is not supported. Then it should rather be silently ignored instead of emitting an error. So far it looks like a compiler bug, not the port's one. :-) > And even if it would be: packages should not adapt themselves to the > hardware on compilation. As I've said, it is not used during the actual build (code generation). But perhaps we could move the detection elsewhere to avoid hitting the compiler bug. (In reply to Alexey Dokuchaev from comment #5) > Then it should rather be silently ignored instead of emitting an error. So far it looks like a compiler bug, not the port's one. :-) This is by design, not a compiler bug. As far as I know, i386 and amd64 are the only architectures that support -march=native. > As I've said, it is not used during the actual build (code generation). But perhaps we could move the detection elsewhere to avoid hitting the compiler bug. The patch already does so: it turns the detection into a port option (default on; processors without SSE4.1 are rare these days). (In reply to Robert Clausecker from comment #6) > As far as I know, i386 and amd64 are the only architectures that support > -march=native. Okay. But then again, I don't see how this breaks the build. If I replace "native" with an arbitrary unknown CPU target (so I could reproduce it on x86), the logic remains the same: SSE bits won't be built. The "error: unknown target CPU 'foobar'" build log pollution is harmless, fixing it does not IMHO warrant the proposed complication of the Makefile logic and introduction of the port option. P.S. Please wrap the text manually when quoting with '>' since Bugzilla is too stupid to do that automatically. (In reply to Alexey Dokuchaev from comment #7) > The "error: unknown target CPU 'foobar'" build log pollution is harmless, > fixing it does not IMHO warrant the proposed complication of the Makefile > logic and introduction of the port option. Ports are not supposed to detect CPU features at build time (which is what the `-march=native` part does). So patching this logic out and replacing it with a switch is a matter of bringing the port into compliance with the ports conventions in addition to fixing this warning. (In reply to Alexey Dokuchaev from comment #7) cc -march=native hello.c cc: error: the clang compiler does not support '-march=native' Exit 1 Any progress on this one? maintainer timeout ^Triage: Reset Assignee, timeout (3 months) Comment on attachment 230330 [details]
benchmarks/lzbench: fix build on arm and riscv64
Approved by: portmgr (blanket build fix, ports compliance)
MFH: 2022Q2
@Mikael Can you take this?
(In reply to Kubilay Kocak from comment #12) > Reset Assignee, timeout (3 months) I explicitly disagreed with some of the suggested changes, there's no timeout, it's still in progress. (In reply to Mikael Urankar from comment #9) > cc -march=native hello.c > cc: error: the clang compiler does not support '-march=native' > Exit 1 Errors inside gmake's $(shell ...) are not propagated up to the calling context, e.g. target recipe. (In reply to Alexey Dokuchaev from comment #15) I have explained my reasoning and why my changes are required to bring the port into compliance with the ports conventions (cf §13.14.4 Porter's Handbook). You have then decided not to respond to that for multiple months, despite repeated requests. Also, fact is that the port without my changes does not build at all on armv7 or riscv64. So a patch is needed anyway. You could have at any time just committed the parts needed for portability while keeping your compliance-violating -march=native tuning. So: what is your response? What is your justification for keeping behaviour that is not in compliance with the ports rules? To give a specific example for why this rule exists: Suppose a binary package is build on the ports cluster which does support SSE. Thus, the package is generated with SSE-enabled compressors. It is then attempted to use the package on a computer that does not support SSE. Boom! Package doesn't work. To avoid this problem, ports must not detect which CPU features to use based on the host they are compiled on. Because the host they will later run on might not support these features after all! (In reply to Alexey Dokuchaev from comment #14) Please set maintainer-approval to - on proposed patches not accepted so its clear, thanks! > I have explained my reasoning and why my changes are required to bring the > port into compliance with the ports conventions (cf §13.14.4 Porter's > Handbook). This reasoning is correct in general, but does not quite apply in this particular case, as I've explained earlier and will explain again later. > You have then decided not to respond to that for multiple months, despite > repeated requests. Of course I did respond, in comment #5 and comment #7. > Suppose a binary package is build on the ports cluster which does support > SSE. Thus, the package is generated with SSE-enabled compressors. Okay, so that would be a 64-bit x86 CPU. > It is then attempted to use the package on a computer that does not support > SSE. Boom! First of all, no, no boom. There are only three SSE-enabled tests out of many, and you'd have to specifically ask the benchmark binary to run them. Second, most if not all 64-bit x86 CPU today support SSE. If you have a real case that would demonstrate this breakage, I'd like to hear that, but at this point the rationale for introducing the option is too weak, despite what you might read in the PHB. > make the port obey CFLAGS This is a benchmark, and custom CFLAGS could alter the results, making them incomparable between different users, operating systems, etc. Perhaps I should add a note about it in the Makefile. So, again, while we usually try to respect CFLAGS, in this case we actually do *not* want to do that. :) > Also, fact is that the port without my changes does not build at all on > armv7 or riscv64. So a patch is needed anyway. The more I look into this, the less I understand proposed changes and how they are fixing the build, exactly. Maybe you can attach a build log, as we do not have armv7 or riscv64 machines in our cluster. We do have arm64 one though, but when I tried to build the port with CC='cc --target=arm7', the build failed, but not when building `pithy/pithy.cpp', it was `ucl/ucl_init.c': ucl/acc/acc_chk.ch:161:5: error: static_assert failed due to requirement 'sizeof(long) == (64 / 8)' "sizeof(long) == (64 / 8)" With CC='cc --target=riscv64', the error is different, but again, nothing to do with `tornado/Common.h': bzip2/bzlib_private.h:25:10: fatal error: 'stdlib.h' file not found #include <stdlib.h> > You could have at any time just committed the parts needed for portability I need to understand what I'm committing first, see the breakage, understand what's causing it, and determine the right fix. We're not there yet, sorry. > while keeping your compliance-violating -march=native tuning. Robert, again: there's *no* -march=native tuning happening here, -march=native is not used for the build, is not propagated to the CFLAGS, it only conveniently determines whether to build optional SSE tests when they make sense. While it might not seem quite right to you, it does not create real-world problem in this particular case (and yet if it does, I'd like to hear the real story with all the specifics and details). (In reply to Kubilay Kocak from comment #18) > Please set maintainer-approval to - on proposed patches not accepted so its > clear, thanks! I'm always confused with these feedback flags on PR vs patches. Let's hope I got it right now. (In reply to Alexey Dokuchaev from comment #19) > it does not create real-world problem in this particular case (and yet if it > does, I'd like to hear the real story with all the specifics and details). To elaborate a bit: as an example of the real-world breakage due to native CPU instructions tuning, see bug #253235 and bug #257520. (In reply to Alexey Dokuchaev from comment #19) > Of course I did respond, in comment #5 and comment #7. To which I responded (comment #6, comment #8, comment #10). You then decided to no longer respond and not to give any indication of whether you accept the patch or not. So I considered you to be in maintainer timeout. > Okay, so that would be a 64-bit x86 CPU. If it builds an i386 packages, it behaves like a 32 bit x86 CPU because an i386 jail is used to build these. Recall that x86 processors can run 32 bit programs just fine in 64 bit mode. Note also that SSE2 is indeed baseline for amd64, so indeed SSE2 support will always be built on an amd64 host. > First of all, no, no boom. There are only three SSE-enabled tests out of many, > and you'd have to specifically ask the benchmark binary to run them. Second, > most if not all 64-bit x86 CPU today support SSE. If you have a real case that > would demonstrate this breakage, I'd like to hear that, but at this point the > rationale for introducing the option is too weak, despite what you might read in > the PHB. Now this begs the question: why then should it matter if the host support SSE2 or not for building the SSE-enabled tests? If the package works fine on a non-SSE2 CPU (e.g. many embedded and industrial x86 processors) except for specific use cases, then surely patching out the detection to always build SSE2 on i386/amd64 would be the right way to go, once again meaning that the host detection must go. And if on the contrary the package does not work fine on non-SSE2 hosts when build with SSE2 support, then the detection code must go to and SSE2 must be disabled unconditionally on i386 (perhaps with a port option). So in both possible cases there's no saving for this auto-detection code. > This is a benchmark, and custom CFLAGS could alter the results, making them incomparable between different users, operating systems, etc. Perhaps I should add a note about it in the Makefile. So, again, while we usually try to respect CFLAGS, in this case we actually do *not* want to do that. :) The benchmarks are already incomparable in this regard as different operating systems use different compilers, different compiler versions, different default options for these compiler versions, different C standard libraries, ... So I'm not sure why violating ports policy will make this any better (but I do agree that it's a case worth of consideration). > The more I look into this, the less I understand proposed changes and how they are fixing the build, exactly. Maybe you can attach a build log, as we do not have armv7 or riscv64 machines in our cluster. We do have arm64 one though, but when I tried to build the port with CC='cc --target=arm7', the build failed, but not when building `pithy/pithy.cpp', it was `ucl/ucl_init.c': You do have at least armv7 machines in the cluster (perhaps these are arm64 machines running native armv7 jails; I don't know, I'm not a project member) and you get build failures every other day. Check the Ports Fallout site. Here is a recent one: https://lists.freebsd.org/archives/freebsd-pkg-fallout/2022-May/237130.html Note that arm7 is not a valid target. The correct name is "armv7". I'm not sure if this will do the trick either as the libc header files differ between architectures. Try building an armv7 Poudriere jail with QEMU. The problems reproduce quite well there. > Robert, again: there's *no* -march=native tuning happening here, -march=native is not used for the build, is not propagated to the CFLAGS, it only conveniently determines whether to build optional SSE tests when they make sense. And that exactly is a violation. > I need to understand what I'm committing first, see the breakage, understand what's causing it, and determine the right fix. We're not there yet, sorry. Fair enough. Please let me know if you need any additional details. I unfortunately do not have access to a RISC-V machine right now, but the problem reproduces well on QEMU (make a QEMU-based RISC-V jail with Poudriere and test the port there). The error I get with the current 2022Q2 state is: nxb-bin/usr/bin/c++ -Wno-unknown-pragmas -Wno-sign-compare -Wno-conversion -fomit-frame-pointer -fstrict-aliasing -ffast-math -O3 -DNDEBUG -I./. -I./zstd/lib -I./zstd/lib/common -I./brotli/include -I./xpack/common -I./libcsc -I./xz -I./xz/api -I./xz/check -I./xz/common -I./xz/lz -I./xz/lzma -I./xz/rangecoder -DHAVE_CONFIG_H -DFL2_SINGLETHREAD -DBENCH_REMOVE_LZSSE -std=c++11 _lzbench/compressors.cpp -c -o _lzbench/compressors.o In file included from tornado/tor_test.cpp:40: In file included from tornado/Tornado.cpp:17: tornado/MatchFinder.cpp:155:38: warning: cast to 'unsigned char *' from smaller integer type 'PtrVal' (aka 'unsigned int') [-Wint-to-pointer-cast] BYTE *toPtr (PtrVal n) {return (BYTE*) n;} ^~~~~~~~~ tornado/MatchFinder.cpp:156:38: error: cast from pointer to smaller type 'PtrVal' (aka 'unsigned int') loses information PtrVal fromPtr(BYTE *p) {return (PtrVal) p;} ^~~~~~~~~~ 1 warning and 1 error generated. (In reply to Robert Clausecker from comment #22) > SSE2 must be disabled unconditionally on i386 (perhaps with a port option) SSE bits are only built when target CPU is 64-bit x86 *and* supports SSE4.1; i386 (and all other) packages are built with -DBENCH_REMOVE_LZSSE as you can see in the build log: http://beefy4.nyi.freebsd.org/data/latest-per-pkg/lzbench/1.8.1/123i386-quarterly.log (sadly, it requires IPv6). We can't include SSE on i386 because its highest standard is SSE2; for amd64 it's safer as full SSE4.1 support arrived with AMD FX more than 10 years ago. > So in both possible cases there's no saving for this auto-detection code. It enables SSE correctly in 99% cases, with no patches, without facing users with an option, etc. It just works. You suggest making in toggable so it can be 100% correct *theoretically* while neglecting practical side of things. Introducing patches and options brings albeit small, yet noticeable maintenance burden and extra cognitive load on the user for little to none real gain. > The benchmarks are already incomparable in this regard as different > operating systems use different compilers, different compiler versions, > different default options for these compiler versions, different C standard > libraries, ... Right, and we should rather judge about (and benchmark) those things than worry about particular compiler options. > You do have at least armv7 machines in the cluster and you get build > failures every other day. Yes, I do receive the emails; while we have package build machines, we do not have 32-bit ARM development machines which I could log into: https://www.freebsd.org/internal/machines/ > Try building an armv7 Poudriere jail with QEMU. The problems reproduce > quite well there. I've managed to setup an ARM jail and with this `emulators/qemu-user-static' thingy could reproduce the breakage, thanks! A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=8bfcc7c9738a835ba65030236d816c222cb5e8fc commit 8bfcc7c9738a835ba65030236d816c222cb5e8fc Author: Alexey Dokuchaev <danfe@FreeBSD.org> AuthorDate: 2022-05-24 13:39:30 +0000 Commit: Alexey Dokuchaev <danfe@FreeBSD.org> CommitDate: 2022-05-24 13:39:30 +0000 benchmarks/lzbench: fix the port's build on ARM and RISC-V While here, mute the compiler when it is being called inside the GNU make's $(shell ...) context: these errors do not cause build failures (they are not propagated to the caller) but can confuse careless readers of the build log. PR: 260625 benchmarks/lzbench/Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) |
Created attachment 230330 [details] benchmarks/lzbench: fix build on arm and riscv64 - add a type cast clang requires - patch in a word size check for riscv64 - remove use of -march=native - add an SSE option to configure building the lzsse encoder instead of detecting that from the host - make the port obey CFLAGS - pet portlint Tested with Poudriere on i386 amd64 armv7 arm64 riscv64 FreeBSD 13. Portlint wants LICENSE to be defined but is otherwise happy.