Bug 277902 - Mk/Uses/cargo.mk: WITH_LTO no longer applies
Summary: Mk/Uses/cargo.mk: WITH_LTO no longer applies
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Gleb Popov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-23 07:35 UTC by Daniel Engberg
Modified: 2024-03-23 20:13 UTC (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Engberg freebsd_committer freebsd_triage 2024-03-23 07:35:22 UTC
WITH_LTO no longer gets applied when being called from Mk/Uses/cargo.mk, it does however work if you define it in a port.

Example:

make -V CARGO_ENV
CARGO_HOME=/usr/ports/audio/ebur128/work/cargo-home  CARGO_BUILD_JOBS=6  CARGO_BUILD_TARGET=x86_64-unknown-freebsd  CARGO_TARGET_DIR=/usr/ports/audio/ebur128/work/target  CARGO_TARGET_X86_64_UNKNOWN_FREEBSD_LINKER="cc"  RUSTC=/usr/local/bin/rustc  RUSTDOC=/usr/local/bin/rustdoc  RUSTFLAGS="-C target-cpu=tigerlake -C link-arg=-fstack-protector-strong" RUST_BACKTRACE=1

Adding WITH_LTO=yes to port Makefile:

CARGO_HOME=/usr/ports/audio/ebur128/work/cargo-home  CARGO_BUILD_JOBS=6  CARGO_BUILD_TARGET=x86_64-unknown-freebsd  CARGO_TARGET_DIR=/usr/ports/audio/ebur128/work/target  CARGO_TARGET_X86_64_UNKNOWN_FREEBSD_LINKER="cc"  RUSTC=/usr/local/bin/rustc  RUSTDOC=/usr/local/bin/rustdoc  RUSTFLAGS="-C target-cpu=tigerlake -C link-arg=-fstack-protector-strong" RUST_BACKTRACE=1 CARGO_PROFILE_RELEASE_LTO="true"  CARGO_PROFILE_RELEASE_PANIC="abort"  CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1

Commit d697653cffd19ddaf3cdee2589a830c72ab39777 broke this functionality
Comment 1 Benjamin Takacs 2024-03-23 07:49:41 UTC
WITH_* variables are to be set by the user, should Mk/Uses/cargo.mk really set it?

From Mk/bsd.port.mk: "Note: the distinction between the USE_* and WANT_* variables, and the WITH_* and WITHOUT_* variables, are that the former are restricted to usage inside the ports framework, and the latter are reserved for user-settable options."

According to that the right solution would be creating a WANT_LTO variable and do LTO if requested by the user or if the port wants it and the user didn't explicitly stopped it (WITHOUT_LTO{,_PORTS}).

It works from the port itself, as Mk/bsd.port.mk can't know if the user or the port set it.
Comment 2 Gleb Popov freebsd_committer freebsd_triage 2024-03-23 10:18:01 UTC
Yes, this happens because Uses are .include'd earlier than Features.

What I think the problem of Mk/Uses/cargo.mk is that it appends LTO flags based on USES file inclusion. I don't quite see why we're doing that.

I'd rather change Mk/Uses/cargo.mk to unconditionally set special LTO-related knobs for each language/buildsystem and then consume them in the appropriate USES code.

I'm happy to do the change if the proposed solution looks good.
Comment 3 Daniel Engberg freebsd_committer freebsd_triage 2024-03-23 12:47:06 UTC
Hi,

I'm not sure what you're suggesting as a solution, the current implementation's intention was to keep everything at one place rather than splitting bits into multiple files. This worked fine before mentioned referenced commit.

Best regards,
Daniel
Comment 4 Gleb Popov freebsd_committer freebsd_triage 2024-03-23 12:51:12 UTC
What I'm suggesting is

diff --git a/Mk/Features/lto.mk b/Mk/Features/lto.mk
index 3fef5a223e9c..3fbff00d5f5e 100644
--- a/Mk/Features/lto.mk
+++ b/Mk/Features/lto.mk
@@ -10,17 +10,17 @@ LTO_Include_MAINTAINER=     pkubaj@FreeBSD.org
 .  if !defined(LTO_UNSAFE) || defined(LTO_DISABLE_CHECK)
 .    if "${ARCH}" == "riscv64" && !defined(LTO_DISABLE_CHECK)
        DEV_WARNING+=   "LTO is currently broken on riscv64, to override set LTO_DISABLE_CHECK=yes"
-.    elif defined(_INCLUDE_USES_CARGO_MK)
-   CARGO_ENV+= CARGO_PROFILE_RELEASE_LTO="true" \
+.    else
+   CARGO_LTO_ENV=      CARGO_PROFILE_RELEASE_LTO="true" \
                CARGO_PROFILE_RELEASE_PANIC="abort" \
                CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1
-.    elif defined(_INCLUDE_USES_MESON_MK)
-   MESON_ARGS+=        -Db_lto=true
-.    elif defined(_INCLUDE_USES_CABAL_MK)
+
+   MESON_LTO_ARGS=     -Db_lto=true
+
    CABAL_LTO_ARGS=     --ghc-options=-split-sections \
                        --gcc-options="-fdata-sections -ffunction-sections" \
                        --ld-options=-Wl,--gc-sections,--build-id,--icf=all
-.    else
+
 # Overridable as a user may want to use -flto
    LTO_FLAGS?= -flto=thin
    CFLAGS+=    ${LTO_FLAGS}


and then use CARGO_LTO_ENV in Uses/cargo.mk
Comment 5 Daniel Engberg freebsd_committer freebsd_triage 2024-03-23 13:25:53 UTC
Problem is that currently Mk/Features/lto.mk isn't included/loaded at all?
Comment 6 Gleb Popov freebsd_committer freebsd_triage 2024-03-23 13:39:54 UTC
(In reply to Daniel Engberg from comment #5)
This is not a core problem, but a consequence of defining WITH_LTO inside cargo.mk

But you actually put me on the right track with this. Let me think about this a bit more.
Comment 7 Gleb Popov freebsd_committer freebsd_triage 2024-03-23 15:05:16 UTC
Proposed fix's below. It now adhere to WITHOUT_LTO_PORTS too.

diff --git a/Mk/Uses/cargo.mk b/Mk/Uses/cargo.mk
index 5d423d81661e..e3b733d35821 100644
--- a/Mk/Uses/cargo.mk
+++ b/Mk/Uses/cargo.mk
@@ -139,8 +139,11 @@ CARGO_ENV+= \
 CARGO_ENV+=    RUST_BACKTRACE=1
 .  endif
 
+.  if !defined(_WITHOUT_LTO) && (!defined(WITHOUT_LTO_PORTS) || ${WITHOUT_LTO_PORTS:N${PKGORIGIN}})
 _CARGO_MSG=    "===>   Additional optimization to port applied"
-WITH_LTO=      yes
+_WITH_LTO=     yes
+.undef _WITHOUT_LTO
+.  endif
 
 # Adjust -C target-cpu if -march/-mcpu is set by bsd.cpu.mk
 .  if ${ARCH} == amd64 || ${ARCH} == i386
Comment 8 commit-hook freebsd_committer freebsd_triage 2024-03-23 19:37:38 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=3c26c5acf2ce0ac62aa0ec1b946080867f122563

commit 3c26c5acf2ce0ac62aa0ec1b946080867f122563
Author:     Gleb Popov <arrowd@FreeBSD.org>
AuthorDate: 2024-03-23 15:13:31 +0000
Commit:     Gleb Popov <arrowd@FreeBSD.org>
CommitDate: 2024-03-23 19:35:55 +0000

    Uses/cargo.mk: Fix enabling LTO

    PR:             277902

 Mk/Uses/cargo.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Comment 9 Gleb Popov freebsd_committer freebsd_triage 2024-03-23 19:38:08 UTC
I've gone ahead and pushed the fix. Daniel, please check if it works for you and sorry for the breakage!
Comment 10 Daniel Engberg freebsd_committer freebsd_triage 2024-03-23 20:13:33 UTC
Seems to work on my end too, thanks for the quick fix and sorry for the late reply.