Page MenuHomeFreeBSD
Differential D19237

Fix TARGET_TRIPLE assembly and retrieve OS version automatically
AbandonedPublic
Actions

Authored by alfredo on Feb 18 2019, 7:30 PM.
Tags
Referenced Files
Unknown Object (File)
Fri, Nov 15, 4:37 PM
Unknown Object (File)
Wed, Nov 13, 10:09 PM
Unknown Object (File)
Tue, Nov 12, 4:50 PM
Unknown Object (File)
Tue, Nov 12, 1:44 PM
Unknown Object (File)
Mon, Nov 11, 1:41 PM
Unknown Object (File)
Mon, Nov 11, 8:59 AM
Unknown Object (File)
Sun, Nov 10, 6:10 PM
Unknown Object (File)
Sun, Nov 10, 4:45 PM
View All 59 Files
Subscribers
bdrewery
emaste
imp
jhb
leonardo.bianconi_eldorado.org.br
luporl
markj
Contributor Reviews (src)

Details

Reviewers
emaste
bdragon
jhibbits
imp
arichardson
brooks
Summary

This removes the hardcoded FreeBSD OS version used in target triple.
Also fixes wrong TARGET_TRIPLE assembly where TARGET_ABI is written where "vendor" string is expected. It's expected to be in the form "<arch><sub>-<vendor>-<sys>-<abi>" [1], but it's currently calculated as "<arch><sub>-<abi>-<sys>"

As example, 'make buildworld TARGET=powerpc TARGET_ARCH=powerpc64 TARGET_ABI=elfv2' must end like: 'TARGET_TRIPLE=powerpc64-unknown-freebsd13.0-elfv2'

TARGET_TRIPLE is used as argument to clang's "-target" parameter specially when cross compiling.

*note <sub> is ignored in this patch as well

[1] https://clang.llvm.org/docs/CrossCompilation.html#target-triple

Test Plan

buildworld powerpc64-elfv2 (cross)
buildworld x86_64 (native)

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34745
Build 31801: arc lint + arc unit

Event Timeline

alfredo created this revision.Feb 18 2019, 7:30 PM
Herald added subscribers: Contributor Reviews (src), emaste, imp. · View Herald TranscriptFeb 18 2019, 7:30 PM
Harbormaster completed remote builds in B22558: Diff 54046.Feb 18 2019, 7:30 PM
alfredo retitled this revision from Fix TARGET_TRIPLE variable computation when specifying TARGET_ABI to Fix TARGET_TRIPLE variable assembly when specifying TARGET_ABI.Feb 18 2019, 7:41 PM
alfredo edited the summary of this revision. (Show Details)
alfredo edited the test plan for this revision. (Show Details)
alfredo added reviewers: emaste, bdragon, jhibbits.
alfredo added subscribers: linimon, luporl, leonardo.bianconi_eldorado.org.br.
bdragon accepted this revision.Feb 18 2019, 8:57 PM

Yeah, that's better.

Since I'm not an official member, make sure and get more approvals.

This will need verification on mips and arm I believe, to ensure nothing breaks.

This revision is now accepted and ready to land.Feb 18 2019, 8:57 PM
emaste added subscribers: jhb, markj.Feb 27 2019, 5:15 PM
imp added inline comments.Feb 27 2019, 5:32 PM
Makefile.inc1
136

not an objection, per-se, but an observation that these same transforms are used 4 times in an identical fasion. Any way to make that more common?

137

any way of getting this automatically?

jhb added inline comments.Feb 27 2019, 6:44 PM
Makefile.inc1
136

You could have TARGET_TRIPLE_ARCH and MACHINE_TRIPLE_ARCH helper variables to at least cut it in half.

137

Not really, it's the version we are building as opposed to the build host, etc. We already have to bump this one when creating a new branch, this just centralizes it. I think Makefile.libcompat also hardcodes freebsd13 and it would be nice if we could fix that to use OS_VERSION as part of this change.

jhibbits added a comment.Jun 27 2019, 9:15 PM

@alfredo.junior_eldorado.org.br think you can make the changes @jhb and @imp are suggesting? Looks fine to me overall.

alfredo updated this revision to Diff 59285.Jul 1 2019, 8:29 PM

I updated the patch to retrieve FreeBSD version automatically, however I'm not sure on how to better address 'lib/clang/llvm.build.mk'. The variables created on 'Makefile.inc1' are not visible there.

This revision now requires review to proceed.Jul 1 2019, 8:29 PM
Herald added a subscriber: bdrewery. · View Herald TranscriptJul 1 2019, 8:29 PM
Harbormaster completed remote builds in B25165: Diff 59285.Jul 1 2019, 8:29 PM
alfredo marked 4 inline comments as done.Jul 1 2019, 8:29 PM

TARGET=powerpc TARGET_ARCH=powerpc64 produces:

clang -target powerpc64-unknown-freebsd13.0

TARGET=powerpc TARGET_ARCH=powerpc64 TARGET_ABI=elfv2 produces:

clang -target powerpc64-unknown-freebsd13.0-elfv2
alfredo updated this revision to Diff 59315.Jul 2 2019, 3:43 PM

remove a level of identation

Harbormaster completed remote builds in B25180: Diff 59315.Jul 2 2019, 3:43 PM
alfredo added a reviewer: imp.Aug 22 2019, 1:25 PM
bdragon added inline comments.Sep 7 2019, 10:39 PM
share/mk/sys.mk
352 ↗(On Diff #59315)

This grep doesn't seem to actually work for some reason. It isn't picking up SRCTOP...

bdragon added inline comments.Sep 10 2019, 1:44 PM
share/mk/sys.mk
352 ↗(On Diff #59315)

of course now I'm having trouble reproducing this. I distinctly remember seeing lots of "grep: /sys/sys/param.h not found" but I'm not seeing it anymore.

imp added a comment.Sep 16 2019, 7:15 PM

So I think this belongs in share/mk/src.sys.env.mk, but I'm not sure.

share/mk/sys.mk
352 ↗(On Diff #59315)

One $ is what you want since SRCTOP is a makefile variable.
However, I'm 100% sure this is the wrong place for this.
Also, just use newvers.sh to get this info.
And I think you want is something like

.if !defined(OS_VERSION)
OS_VERSION!=${SRCTOP}/sys/conf/newvers.sh -V REVISION
.export OS_VERSION
.endif
alfredo updated this revision to Diff 62559.Sep 25 2019, 6:11 PM

This is a reworked version. I found that FreeBSD release was already being retrieved in share/mk/local.meta.sys.mk using variable FREEBSD_REVISION. Then I moved it to share/mk/src.sys.env.mk to make variable readable everywhere.

Harbormaster completed remote builds in B26694: Diff 62559.Sep 25 2019, 6:11 PM
alfredo updated this revision to Diff 62586.Sep 26 2019, 1:28 PM

removed debugging temporary code

Harbormaster completed remote builds in B26712: Diff 62586.Sep 26 2019, 1:28 PM
bdragon added a comment.Oct 16 2019, 1:03 AM

Needs reroll after the massive libcompat changes settle down.

alfredo retitled this revision from Fix TARGET_TRIPLE variable assembly when specifying TARGET_ABI to Fix TARGET_TRIPLE assembly and retrieve OS version automatically.Nov 9 2019, 1:45 AM
alfredo planned changes to this revision.Nov 18 2019, 6:26 PM

Need to reroll

share/mk/sys.mk
352 ↗(On Diff #59315)

Strange, it works for me.
Does "make -v OS_VERSION" give "13.0" for you?

I appended the following code

BLA!=echo '$${SRCTOP}/sys/sys/param.h'
.info BLA SRCTOP is ${BLA}

and it gives me:

# make
make: "/root/src/freebsd/share/mk/sys.mk" line 358: BLA SRCTOP is /root/src/freebsd/sys/sys/param.h

Explicit target required.  Likely "buildworld" is wanted.  See build(7).

Also, changing /sys/sys/param.h doesn't change OS_VERSION value. If I change /root/src/freebsd//sys/sys/param.h it does change OS_VERSION

I tried to reproduce using the shells "csh, bash and sh" and result is ok to me.
What's the build command you're using ?

alfredo added a project: PowerPC.Dec 3 2019, 10:04 PM
linimon removed a subscriber: linimon.Jan 5 2020, 10:30 PM
alfredo updated this revision to Diff 67117.Jan 21 2020, 8:23 PM
alfredo edited the summary of this revision. (Show Details)

Updated patch to latest HEAD (thanks to @bdragon)

Harbormaster completed remote builds in B28827: Diff 67117.Jan 21 2020, 8:23 PM
alfredo updated this revision to Diff 79435.Nov 11 2020, 5:37 PM

Updated patch to latest HEAD

Harbormaster completed remote builds in B34745: Diff 79435.Nov 11 2020, 5:38 PM
bdrewery added inline comments.Nov 11 2020, 11:41 PM
share/mk/src.sys.env.mk
26–29

I think the export will handle my concern but can you compare the time for make -j1 print-dir with and without this change? Maybe run it an extra time first to prime the caches.

alfredo marked an inline comment as done.Nov 17 2020, 1:33 PM
alfredo added inline comments.
share/mk/src.sys.env.mk
26–29

Hi @bdrewery, I found no noticeable difference here:

without patch:

1m18.201s
1m19.308s
1m18.782s
1m18.452s

with patch:

1m18.515s
1m18.386s
1m18.265s
1m18.228s

alfredo marked an inline comment as done.Nov 25 2020, 9:09 PM
alfredo added reviewers: arichardson, brooks.Nov 25 2020, 9:19 PM
brooks added a comment.Nov 25 2020, 11:24 PM

This generally looks fine.

The new MACHINE_ABI use will create conflicts with CheriBSD, but we can pick another name if needed. In CheriBSD we've renamed MACHINE_ABI to _MACHINE_ABI in Makefile.inc1 since it appears completely unused and MACHINE_ABI is now a list of ABI features similar to MACHINE_CPU.

arichardson added inline comments.Nov 26 2020, 9:52 AM
Makefile.inc1
139

Internally llvm::Triple uses "environment" rather than "abi" because the last component can also contain suffixes like -elf to indicate the object format.

https://github.com/llvm/llvm-project/blob/3b6481eae2597f656b9f5bb6a5eb5438eb8cb294/llvm/include/llvm/ADT/Triple.h#L386

If we were to use {TARGET,MACHINE}_TRIPLE_ENVIRONMENT (or {TARGET,MACHINE}_TRIPLE_ABI) instead, that would avoid conflicts with CheriBSD?

alfredo added inline comments.Nov 26 2020, 7:15 PM
Makefile.inc1
139

The problem with {TARGET,MACHINE}_ABI is that its use appears to be incorrect. Actually users wanted to use {TARGET,MACHINE}_VENDOR instead, when following "<arch><sub>-<vendor>-<sys>-<abi>" format. (note we don't have {TARGET,MACHINE}_SUB, we append it to {TARGET,MACHINE}_ARCH value)

I like your proposal of having {TARGET,MACHINE}_TRIPLE_ENVIRONMENT to customize compiler target triple (like {TARGET,MACHINE}_TRIPLE the overrides everything)
In this case I would drop {TARGET,MACHINE}_ABI and print a message explaining it should use {TARGET,MACHINE}_VENDOR to fill <vendor> field of target triple

alfredo mentioned this in D34429: retrieve OS release automatically to build compiler target triple.Mar 3 2022, 8:39 PM
alfredo planned changes to this revision.Mar 3 2022, 9:26 PM

Suspending this since it requires changes to continue.
The OS revision part is now being discussed in D34429.

alfredo abandoned this revision.Oct 6 2022, 6:04 PM

This was superseded by https://reviews.freebsd.org/D36420

Revision Contents
Changeset List

PathSize
35 lines
lib/
clang/
11 lines
share/
mk/
6 lines
4 lines
5 lines

Diff 79435

View Options

Makefile.inc1

Loading...
View Options

lib/clang/llvm.build.mk

Loading...
View Options

share/mk/bsd.compat.mk

Loading...
View Options

share/mk/local.meta.sys.mk

Loading...
View Options

share/mk/src.sys.env.mk

Loading...