Bug 225922 - security/libargon2: optimised code gives illegal instruction fault on older/less capable machines
Summary: security/libargon2: optimised code gives illegal instruction fault on older/l...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: amd64 Any
: --- Affects Only Me
Assignee: Thomas Zander
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-15 16:39 UTC by Arthur Chance
Modified: 2018-11-18 12:16 UTC (History)
3 users (show)

See Also:
hsw: maintainer-feedback+
riggs: merge-quarterly+


Attachments
diff of original Makefile and working one (554 bytes, text/plain)
2018-02-15 16:39 UTC, Arthur Chance
no flags Details
patch to build without AVX by default (957 bytes, patch)
2018-02-21 05:36 UTC, Christopher Hall
hsw: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Chance 2018-02-15 16:39:05 UTC
The Gnu makefile for argon2 by default compiles for the build machine architecture. Unfortunately I use poudriere to build a local package repository on my fastest machine and install packages on all other machines on my network. My build machine has AVX-512 instructions, the machine I want to run the code on doesn't and crashes with an illegal instruction when it hits a vxorps opcode.

The ports Makefile has no option to control whether or not optimised code is used, that depends only on the actual hardware used for the build. It needs an option to disable use of the optimised code. I've got a quick hack to the Makefile that works, but don't know if it's of suitable quality for ports. The diff is attached. I reused the standard SIMD option as the control.
Comment 1 Arthur Chance 2018-02-15 16:39:53 UTC
Created attachment 190652 [details]
diff of original Makefile and working one
Comment 2 Christopher Hall 2018-02-21 03:14:18 UTC
Sorry for delay in replying, Chinese New Year holiday here.

How about changing the option to AVX as I found a couple of ports using that symbol. I will make an alternative patch and test locally that using the core2 is works on my application, then I will attach the patch, later today.
Comment 3 Christopher Hall 2018-02-21 05:36:33 UTC
Created attachment 190850 [details]
patch to build without AVX by default
Comment 4 Christopher Hall 2018-02-21 05:37:59 UTC
I added an updated patch, would you look to see if this works for you.
Comment 5 Arthur Chance 2018-02-21 14:49:55 UTC
Hope you had a good Chinese New Year, and thanks for the patch. The delay wasn't a problem, I went down with flu a couple of hours after submitting the bug report and I'm only just back online.

The patch works fine here, the code runs perfectly on my non-AVX machines as well as the build box.

The only other thing that occurs to me is should there also be an option to enable/disable use of threading? The "lanes" argument to argon2 is useless without it, but I have the impression library ports generally avoid forcing use of pthreads unless explicitly enabled? I could be wrong here, I'm speaking as a user rather than a porter.
Comment 6 Christopher Hall 2018-02-22 01:31:20 UTC
(In reply to Arthur Chance from comment #5)

Not sure about threads option, I will have to test to see, probably best to have separate issue for this and just get this build issue sorted first.
Comment 7 Christopher Hall 2018-02-22 03:58:31 UTC
(In reply to Christopher Hall from comment #6)

just checked by default pthreads are linked (/lib/libthr is shown as dependency)
Comment 8 Arthur Chance 2018-02-22 10:33:12 UTC
(In reply to Christopher Hall from comment #6)

Sure, if you want to close off this bug and take time to think over the threading issue that's fine by me. The illegal instruction crash is gone, that's the main thing.
Comment 9 Christopher Hall 2018-02-23 02:26:21 UTC
Now we just need a committer to take a look and apply the patch.
Comment 10 Thomas Zander freebsd_committer freebsd_triage 2018-09-18 15:03:55 UTC
I'll commit and MFH a minimal patch first (w/o options) to resolve the problem for the pkg builds, then we can develop the AVX option.
Comment 11 commit-hook freebsd_committer freebsd_triage 2018-09-18 15:09:37 UTC
A commit references this bug:

Author: riggs
Date: Tue Sep 18 15:08:53 UTC 2018
New revision: 480029
URL: https://svnweb.freebsd.org/changeset/ports/480029

Log:
  Fix runtime error: remove -march=native as optimisation target

  Details:
  The upstream Makefile contains -march=native as optimisation which leads
  to unconditional use of AVX instructions if built on a machine that has
  AVX support. Subsequently this causes SIGILL on processors without AVX,
  including latest-generation Atom descendants.

  PR:		225922
  Reported by:	arthur@qeng-ho.org
  Approved by:	hsw@bitmark.com (maintainer, implicit)
  MFH:		2018Q3

Changes:
  head/security/libargon2/Makefile
Comment 12 commit-hook freebsd_committer freebsd_triage 2018-09-18 15:10:43 UTC
A commit references this bug:

Author: riggs
Date: Tue Sep 18 15:10:05 UTC 2018
New revision: 480031
URL: https://svnweb.freebsd.org/changeset/ports/480031

Log:
  MFH: r480029

  Fix runtime error: remove -march=native as optimisation target

  Details:
  The upstream Makefile contains -march=native as optimisation which leads
  to unconditional use of AVX instructions if built on a machine that has
  AVX support. Subsequently this causes SIGILL on processors without AVX,
  including latest-generation Atom descendants.

  PR:		225922
  Reported by:	arthur@qeng-ho.org
  Approved by:	hsw@bitmark.com (maintainer, implicit)

  Approved by:	ports-secteam (riggs)

Changes:
_U  branches/2018Q3/
  branches/2018Q3/security/libargon2/Makefile
Comment 13 Thomas Zander freebsd_committer freebsd_triage 2018-09-18 16:58:59 UTC
The problem with this AVX option is that -march=native does not mean "include avx" but "use every bloody instruction available on the CPU you currently run on". It does not refer to AVX, but to any extension that might be available at compile time. Thus
1) We can't call it AVX, but rather something like "CPU specific optimisations"
2) The config dialog must make it inescapably clear that activating this option WILL break the code on many other CPUs.

To be honest, I would question whether this port needs this optimisation. On amd64 (in all likelihood our largest user base), -march=native includes SSE2 stuff. Are we really expecting a massive performance gain from AVX, especially since there is no use of intrinsics or asm to express algorithms better.

Thoughts?
Comment 14 Thomas Zander freebsd_committer freebsd_triage 2018-09-18 19:55:17 UTC
(In reply to Thomas Zander from comment #13)

Sorry, I meant -march=generic
Comment 15 Thomas Zander freebsd_committer freebsd_triage 2018-11-18 12:16:46 UTC
Closing this for now since the current state does not cause a problem.
We can revisit this anytime if someone wants to have the AVX instructions in this port.