Bug 236194 - math/secp256k1: fails to build with clang 8 (blocks 3 ports)
Summary: math/secp256k1: fails to build with clang 8 (blocks 3 ports)
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: Alex Dupre
URL:
Keywords: needs-patch
Depends on:
Blocks: 236062
  Show dependency tree / graph
 
Reported: 2019-03-04 05:53 UTC by Jan Beich
Modified: 2019-03-20 23:35 UTC (History)
5 users (show)

See Also:
bugzilla: maintainer-feedback? (ale)
jbeich: maintainer-feedback? (kbowling)
jbeich: maintainer-feedback? (swills)
jbeich: maintainer-feedback? (milios)
jbeich: maintainer-feedback? (vd)

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer freebsd_triage 2019-03-04 05:53:12 UTC 
$ poudriere jail -cj clang8 -v projects/clang800-import -m svn+https
$ poudriere testport -j clang8 math/secp256k1
[...]
In file included from src/secp256k1.c:12:
In file included from ./src/scalar_impl.h:20:
./src/scalar_4x64_impl.h:379:19: error: value '4624529908474429119' out of range for constraint 'n'
    : "S"(l), "n"(SECP256K1_N_C_0), "n"(SECP256K1_N_C_1)
                  ^~~~~~~~~~~~~~~
./src/scalar_4x64_impl.h:17:25: note: expanded from macro 'SECP256K1_N_C_0'
#define SECP256K1_N_C_0 (~SECP256K1_N_0 + 1)
                        ^~~~~~~~~~~~~~~~~~~~
./src/scalar_4x64_impl.h:458:74: error: value '4624529908474429119' out of range for constraint 'n'
    : "g"(m0), "g"(m1), "g"(m2), "g"(m3), "g"(m4), "g"(m5), "g"(m6), "n"(SECP256K1_N_C_0), "n"(SECP256K1_N_C_1)
                                                                         ^~~~~~~~~~~~~~~
./src/scalar_4x64_impl.h:17:25: note: expanded from macro 'SECP256K1_N_C_0'
#define SECP256K1_N_C_0 (~SECP256K1_N_0 + 1)
                        ^~~~~~~~~~~~~~~~~~~~
./src/scalar_4x64_impl.h:504:64: error: value '4624529908474429119' out of range for constraint 'n'
    : "g"(p0), "g"(p1), "g"(p2), "g"(p3), "g"(p4), "D"(r), "n"(SECP256K1_N_C_0), "n"(SECP256K1_N_C_1)
                                                               ^~~~~~~~~~~~~~~
./src/scalar_4x64_impl.h:17:25: note: expanded from macro 'SECP256K1_N_C_0'
#define SECP256K1_N_C_0 (~SECP256K1_N_0 + 1)
                        ^~~~~~~~~~~~~~~~~~~~

http://package18.nyi.freebsd.org/data/headamd64PR236062-default/2019-03-01_06h42m53s/logs/errors/secp256k1-0.1.20190204.log
Comment 2 Alex Dupre freebsd_committer freebsd_triage 2019-03-04 13:46:05 UTC 
I'm not an ASM guru, but why does clang complain to set a 63 bit integer into a 64 bit register? Is it becaue there is a cast to ULL?
Comment 3 Jan Beich freebsd_committer freebsd_triage 2019-03-04 13:55:04 UTC 
Dimitry, can you help with inline ASM and/or at least find Clang commit?
Comment 4 Dimitry Andric freebsd_committer freebsd_triage 2019-03-04 18:04:56 UTC 
(In reply to Jan Beich from comment #3)
> Dimitry, can you help with inline ASM and/or at least find Clang commit?

At first sight it looks like a similar fallout from upstream's https://reviews.llvm.org/rL349561 ("Emit ASM input in a constant context"), as reported in bug 236216.

That commit made the "n" constraint very strict; as in, it accepts *only* "int" expressions, not long long ones.

I'll need to dig a bit further...
Comment 5 Jan Beich freebsd_committer freebsd_triage 2019-03-04 23:31:15 UTC 
Reminder: Clang 8 was merged to 13.0-CURRENT in base r344779. Maintainers, expect pkg-fallout@ mail soon.
Comment 6 Dimitry Andric freebsd_committer freebsd_triage 2019-03-06 21:33:51 UTC 
(In reply to Dimitry Andric from comment #4)
> At first sight it looks like a similar fallout from upstream's
> https://reviews.llvm.org/rL349561 ("Emit ASM input in a constant context"),
> as reported in bug 236216.

Verified that it is indeed that upstream change that breaks it.  I've added this to the upstream bug report.
Comment 7 Alex Dupre freebsd_committer freebsd_triage 2019-03-07 09:47:18 UTC 
Replacing "n" with "i" fixes the compilation. What do you suggest to do?
Comment 8 commit-hook freebsd_committer freebsd_triage 2019-03-07 19:33:48 UTC 
A commit references this bug:

Author: dim
Date: Thu Mar  7 19:33:40 UTC 2019
New revision: 344896
URL: https://svnweb.freebsd.org/changeset/base/344896

Log:
  ?Pull in r354937 from upstream clang trunk (by J?rg Sonnenberger):

    Fix inline assembler constraint validation

    The current constraint logic is both too lax and too strict. It fails
    for input outside the [INT_MIN..INT_MAX] range, but it also
    implicitly accepts 0 as value when it should not. Adjust logic to
    handle both correctly.

    Differential Revision: https://reviews.llvm.org/D58649

  Pull in r355491 from upstream clang trunk (by Hans Wennborg):

    Inline asm constraints: allow ICE-like pointers for the "n"
    constraint (PR40890)

    Apparently GCC allows this, and there's code relying on it (see bug).

    The idea is to allow expression that would have been allowed if they
    were cast to int. So I based the code on how such a cast would be
    done (the CK_PointerToIntegral case in
    IntExprEvaluator::VisitCastExpr()).

    Differential Revision: https://reviews.llvm.org/D58821

  These should fix assertions and errors when using the inline assembly
  "n" constraint in certain ways.

  In case of devel/valgrind, a pointer was used as the input for the
  constraint, which lead to "Assertion failed: (isInt() && "Invalid
  accessor"), function getInt".

  In case of math/secp256k1, a very large integer value was used as input
  for the constraint, which lead to "error: value '4624529908474429119'
  out of range for constraint 'n'".

  PR:             236216, 236194
  MFC after:      1 month
  X-MFC-With:     r344779

Changes:
  head/contrib/llvm/tools/clang/include/clang/AST/APValue.h
  head/contrib/llvm/tools/clang/include/clang/Basic/TargetInfo.h
  head/contrib/llvm/tools/clang/lib/AST/APValue.cpp
  head/contrib/llvm/tools/clang/lib/AST/ExprConstant.cpp
  head/contrib/llvm/tools/clang/lib/CodeGen/CGStmt.cpp
  head/contrib/llvm/tools/clang/lib/Sema/SemaStmtAsm.cpp
Comment 9 Dimitry Andric freebsd_committer freebsd_triage 2019-03-07 19:40:25 UTC 
(In reply to Alex Dupre from comment #7)
> Replacing "n" with "i" fixes the compilation. What do you suggest to do?

After r344896 it should build now without changes.  Similar for the other *coin ports which seem to share the secp code.  If you could verify, that would be great.
Comment 10 Jan Beich freebsd_committer freebsd_triage 2019-03-20 23:35:34 UTC 
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/secp256k1-0.1.20190225.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/bitcoin-0.17.1_2.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/bitcoin-daemon-0.17.1_5.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/bitcoin-utils-0.17.1_2.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/litecoin-0.16.3_4.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/litecoin-daemon-0.16.3_4.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/litecoin-utils-0.16.3_4.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/namecoin-0.17.0_4,1.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/namecoin-daemon-0.17.0_4,1.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/namecoin-utils-0.17.0_4,1.log
http://www.ipv6proxy.net/go.php?u=http://beefy12.nyi.freebsd.org/data/head-amd64-default/p495845_s345206/logs/qtum-0.17.2.log