Bug 239870

Summary: audio/flac: fix build on powerpc64
Product: Ports & Packages Reporter: Piotr Kubaj <pkubaj>
Component: Individual Port(s)Assignee: Christian Weisgerber <naddy>
Status: Closed FIXED    
Severity: Affects Only Me CC: linimon, naddy
Priority: --- Flags: naddy: maintainer-feedback+
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch
none
v2 pkubaj: maintainer-approval? (naddy)

Description Piotr Kubaj freebsd_committer 2019-08-15 00:22:18 UTC
Created attachment 206567 [details]
patch

FreeBSD doesn't support getauxval(), it has elf_aux_info() instead. Use this to fix build on powerpc64.

Since flac seems to add VSX by default on POWER9, add a non-default option to enable VSX.
Comment 1 Christian Weisgerber freebsd_committer 2019-08-15 16:57:22 UTC
I agree with the libFLAC/cpu.c diff.

I don't understand the intended purpose of the VSX option. In libFLAC/stream_encoder.c, the POWER9 and POWER8 routines are only called if cpuinfo.ppc.arch_3_00 and cpuinfo.ppc.arch_2_07, respectively, are true. So the VSX code is compiled on all powerpc64 machines, but only called at runtime on those that support the VSX extensions.
Comment 2 Piotr Kubaj freebsd_committer 2019-08-15 17:58:48 UTC
(In reply to Christian Weisgerber from comment #1)
Ah, you're right about VSX.

BTW, the patch for elf_aux_info is being upstreamed now (in a slightly better version).
Comment 3 Christian Weisgerber freebsd_committer 2019-08-15 21:10:31 UTC
(In reply to Piotr Kubaj from comment #2)
I approve the cpu.c change if you want to go ahead and unbreak the build.
Comment 4 Piotr Kubaj freebsd_committer 2019-08-16 15:04:16 UTC
Ah, I forgot, -mvsx is necessary:
lpc_intrin_vsx.c: In function 'FLAC__lpc_compute_autocorrelation_intrin_power8_vsx_lag_16':
lpc_intrin_vsx.c:94:7: warning: implicit declaration of function 'vec_vsx_ld'; did you mean 'vec_vslh'? [-Wimplicit-function-declaration]
   94 |  d0 = vec_vsx_ld(0, base);
      |       ^~~~~~~~~~
      |       vec_vslh
Comment 5 Christian Weisgerber freebsd_committer 2019-08-16 21:33:35 UTC
(In reply to Piotr Kubaj from comment #4)
Well, if -mvsx is required, an option doesn't make sense either since the build would fail with it off.

So should -mvsx be added on powerpc64? Or does this cause the compiler to produce VSX code outside of the intrinsics that would then fail to run on some powerpc64 machines?

I can simply disable assembly optimizations on powerpc64. It is far more important for the port to build (as a dependency) than to be tuned for speed. Also, I just tried it without asm on amd64 and encoding is plenty fast.
Comment 6 Piotr Kubaj freebsd_committer 2019-08-18 02:59:57 UTC
(In reply to Christian Weisgerber from comment #5)
The problem is that vec_vsx_ld isn't visible without -mvsx. vec_vsx_ld is needed, unless we disable VSX altogether (--disable-vsx). We could pass -mvsx by default, but I'm not sure whether VSX would be autodisabled on POWER older than POWER8 so that flac would work.

Basically, we have 3 options:
a) add -mvsx for everyone on powerpc64,
b) disable VSX for everyone on powerpc64,
c) make VSX optional via option.
Comment 7 Piotr Kubaj freebsd_committer 2019-08-18 03:02:38 UTC
(In reply to Piotr Kubaj from comment #6)
And I'd rather not disable unconditionally that may improve performance on hardware that supports it. Users get new hardware in order to use new features, not to disable them.
Comment 8 Christian Weisgerber freebsd_committer 2019-08-18 20:15:19 UTC
(In reply to Piotr Kubaj from comment #7)
Just disable it for the time being, please.
Comment 9 Piotr Kubaj freebsd_committer 2019-08-19 19:59:40 UTC
Created attachment 206708 [details]
v2

Is this ok? The src/libFLAC/cpu.c is still needed (compiling without it fails because of no getauxval).
Comment 10 Christian Weisgerber freebsd_committer 2019-08-19 21:34:17 UTC
Yes, this is ok.
I have also sent some feedback to the flac-dev mailing list.
Comment 11 commit-hook freebsd_committer 2019-08-28 19:27:31 UTC
A commit references this bug:

Author: pkubaj
Date: Wed Aug 28 19:26:57 UTC 2019
New revision: 510095
URL: https://svnweb.freebsd.org/changeset/ports/510095

Log:
  audio/flac: fix build on powerpc64

  FreeBSD doesn't support getauxval(), it has elf_aux_info() instead. Use this to fix build on powerpc64.
  Disable VSX to not use VSX-related functions.

  PR:		239870
  Approved by:	linimon (mentor), naddy (maintainer)
  Differential Revision:	https://reviews.freebsd.org/D21284

Changes:
  head/audio/flac/Makefile
  head/audio/flac/files/patch-src_libFLAC_cpu.c
Comment 12 commit-hook freebsd_committer 2019-08-28 20:22:37 UTC
A commit references this bug:

Author: naddy
Date: Wed Aug 28 20:21:45 UTC 2019
New revision: 510099
URL: https://svnweb.freebsd.org/changeset/ports/510099

Log:
  Use proper autoconf test for getauxval() and elf_aux_info().
  Based on work by Jeremie Courreges-Anglas for OpenBSD.

  PR:		239870

Changes:
  head/audio/flac/Makefile
  head/audio/flac/files/patch-configure.ac
  head/audio/flac/files/patch-src_libFLAC_cpu.c