Bug 223342 - lang/rust: unbreak cfg_target_feature for PORT_LLVM=on
Summary: lang/rust: unbreak cfg_target_feature for PORT_LLVM=on
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: FreeBSD Rust Team
URL:
Keywords: patch, regression
Depends on:
Blocks: 223300 223314
  Show dependency treegraph
 
Reported: 2017-10-31 15:03 UTC by Jan Beich
Modified: 2018-05-15 13:23 UTC (History)
8 users (show)

See Also:
dumbbell: maintainer-feedback+


Attachments
v0 (6.66 KB, patch)
2017-10-31 15:03 UTC, Jan Beich
jbeich: maintainer-approval? (rust)
jbeich: maintainer-approval? (brooks)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer 2017-10-31 15:03:57 UTC
Created attachment 187613 [details]
v0

Rust 1.21+ regressed cfg_target_feature[1] by taking advantage of API specific to bundled LLVM. We can either apply a simple patch to devel/llvm* ports or drop LLVM_PORT option. So far this only affects simd crate in www/firefox and textproc/ripgrep.

[1] https://github.com/rust-lang/rust/issues/29717

Before:

  $ rustc --print target-cpus
  Target CPU help is not supported by this LLVM version.

  $ rustc --print target-features
  Target features help is not supported by this LLVM version.

After:

  $ rustc --print target-cpus
  Available CPUs for this target:
      amdfam10       - Select the amdfam10 processor.
      athlon         - Select the athlon processor.
      athlon-4       - Select the athlon-4 processor.
      athlon-fx      - Select the athlon-fx processor.
  [...]

  $ rustc --print target-features
  Available features for this target:
      16bit-mode             - 16-bit mode (i8086).
      32bit-mode             - 32-bit mode (80386).
      3dnow                  - Enable 3DNow! instructions.
      3dnowa                 - Enable 3DNow! Athlon instructions.
      64bit                  - Support 64-bit instructions.
      64bit-mode             - 64-bit mode (x86_64).
  [...]
Comment 1 Jan Beich freebsd_committer 2017-10-31 16:22:34 UTC
Comment on attachment 187613 [details]
v0

devel/llvm50 patch here is due to Rust being close to upgrading bundled LLVM.
https://github.com/rust-lang/rust/issues/43370

devel/llvm-devel because it's not clear whether Rust plans to upstream the patch after cfg_target_feature is stabilized.
Comment 2 Jean-Sébastien Pédron freebsd_committer 2017-10-31 17:50:32 UTC
I would be in favor of dropping the LLVM_PORT option from lang/rust. We don't know how well tested this combination is. This time we hit a regression visible at compile time, but it could be worse. Unfortunately, I don't know how popular this option is.

Opinions?
Comment 3 John Hein 2017-11-02 22:26:28 UTC
I've been using the LLVM_PORT option for a while with no problems until now.  Saves a healthy chunk of time and space.

I tested with attachment 187613 [details] - firefox compiles now, and I've had no problems yet.  The patch is pretty simple (just adds some accessors) and seems like it could be acceptable upstream.  I wonder if the rust-lang folks have already submitted the change.

I have not analyzed what else is different between rust-llvm and normal llvm.  That would be good information.
Comment 4 sid 2017-11-03 04:37:04 UTC
I think the port llvm option should be dropped, while not overriding llvm version from ports in make.conf. It should make troubleshooting easier, and those who use or test llvm from ports would still be able to do so from make.conf.
Comment 5 Thomas Zander freebsd_committer 2017-11-04 07:51:49 UTC
(In reply to sid from comment #4)

I am also in favor of dropping the LLVM_PORT option. Building rust is compilated enough as it is, we gain very little from supporting build configuration which are not supported upstream.
Comment 6 Jean-Sébastien Pédron freebsd_committer 2017-11-26 13:08:41 UTC
I'm preparing a patch to update lang/rust to 1.22.1. Once this is committed, I'm going to remove the PORT_LLVM option. The user support headache can be greater than the benefit of this option.
Comment 7 commit-hook freebsd_committer 2017-11-27 18:16:35 UTC
A commit references this bug:

Author: dumbbell
Date: Mon Nov 27 18:16:15 UTC 2017
New revision: 454983
URL: https://svnweb.freebsd.org/changeset/ports/454983

Log:
  lang/rust: Remove the LLVM_PORT option

  FTR, this option allowed to used LLVM from ports instead of building the
  bundled copy.

  The problem is that this combination isn't really tested upstream. This
  led to regressions which are difficult to diagnose. For instance, in
  Rust 1.21.0, the bundled LLVM provided a new API to query the features
  supported by the target arch. The equivalent code inside Rust was
  removed to use that new API. Unfortunately, building Rust 1.21.0+ with a
  copy of LLVM not providing this API didn't failed but instead made that
  list of CPU features empty. This resulted in the following obscure build
  failure in Firefox:

      error[E0432]: unresolved import `x86::sse2`

  To avoid future pain for both end users and maintainers, we decided to
  remove that option. Yes, it will increase the (already long) time to
  build Rust, but it should save time wasted on debugging what is not
  really supported anyway.

  PR:		223342, 223300
  Reported by:	Many users

Changes:
  head/lang/rust/Makefile
  head/lang/rust/files/config.toml
Comment 8 Mikhail T. 2017-11-28 16:15:47 UTC
> this option allowed to used LLVM from ports instead of building the
> bundled copy.

This should never even have been an option -- it should always have been the only way (unless the LLVM-pieces from the base are sufficient). I'm not going to repeat the arguments included here:

https://www.freebsd.org/doc/en/books/porters-handbook/bundled-libs.html

they all apply -- using bits and pieces of other projects bundled with the software being ported is Just Wrong[TM].

The arguments like "we don't know" or "how well-tested is it" are nothing but FUD -- rust is not tested (by its developers) on FreeBSD as well, should we all switch to Linux?

Yes, the option adds difficulty reproducing reported problems -- so get rid of the option, but make the behavior _standard_.

A port is supposed to provide the BEST way of building a piece of software on FreeBSD. Not just "a way", but the best way...

> the bundled LLVM provided a new API to query the features supported

Then the new API should be added to the LLVM port(s), as jbeich@ was suggesting from the beginning.

The bad old days of building a special ("well tested") version of the C-compiler for the sake of one application (think lang/gcc-ooo) may be back...
Comment 9 sid 2017-11-29 02:37:49 UTC
The edit is good. It will help diagnose problems. The port should not override the compiler chosen from /etc/make.conf, if that's what someone meant when they said make it standard, because that restricts testing for that, and it will make ports inconvenient in the future.
Comment 10 Jan Beich freebsd_committer 2018-05-15 13:23:13 UTC
Should be fixed in Rust 1.26.0 but I haven't tried.