Bug 239870 - audio/flac: fix build on powerpc64
Summary: audio/flac: fix build on powerpc64
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Christian Weisgerber
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-15 00:22 UTC by Piotr Kubaj
Modified: 2019-08-28 20:22 UTC (History)
2 users (show)

See Also:
naddy: maintainer-feedback+


Attachments
patch (1.65 KB, patch)
2019-08-15 00:22 UTC, Piotr Kubaj
no flags Details | Diff
v2 (1.64 KB, patch)
2019-08-19 19:59 UTC, Piotr Kubaj
pkubaj: maintainer-approval? (naddy)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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