Page MenuHomeFreeBSD
Differential D20261

[PowerPC64] Use ld.bfd to build LIB32 and STAND - when using llvm
ClosedPublic
Actions

Authored by alfredo on May 14 2019, 5:18 PM.
Tags
Referenced Files
F103145664: D20261.id57598.diff
Thu, Nov 21, 2:30 PM
F103138805: D20261.id65957.diff
Thu, Nov 21, 12:43 PM
F103129911: D20261.id65957.diff
Thu, Nov 21, 9:46 AM
F103125977: D20261.id65957.diff
Thu, Nov 21, 8:33 AM
F103107801: D20261.id.diff
Thu, Nov 21, 2:37 AM
F103106584: D20261.id61158.diff
Thu, Nov 21, 2:11 AM
Unknown Object (File)
Wed, Nov 20, 8:11 AM
Unknown Object (File)
Wed, Nov 20, 6:14 AM
View All Files
Subscribers
bdrewery
emaste
imp
luporl

Details

Reviewers
jhibbits
bdragon
bdrewery
imp
luporl
Commits
rS356054: [PowerPC64] Use ld.bfd to build LIB32 and STAND - when using llvm
Summary

This patch is to support ongoing work for replacing "GCC/BFD" by "CLANG/LLD" on target PowerPC64 [1], by proposing a way to specify and/or locate a secondary ld.bfd linker.
This is necessary as LLD currently doesn't support PowerPC 32 bits, so we keep using BFD for the 32 bit stuff on PowePC64(LIB32 compatibility and STAND/slof/loader.)

  • creates LD_BFD variable pointing to ld.bfd
  • use LD_BFD as linker for LIB32/compat
  • Default behavior for other platforms aren't changed.

[1] https://wiki.freebsd.org/powerpc/llvm-elfv2

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25324
Build 23984: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
alfredo updated this revision to Diff 57598.May 20 2019, 7:10 PM

updates

Harbormaster completed remote builds in B24336: Diff 57598.May 20 2019, 7:10 PM
alfredo marked an inline comment as done.May 20 2019, 7:14 PM
  • Replaced LINKER_TYPE by X_LINKER_TYPE.
  • Added (") around X_LINKER_TYPE to fix "make clean"
alfredo added a child revision: D20378: [PowerPC64] LLVM as default toolchain for PowerPC and PowerPC64.May 23 2019, 2:15 PM
alfredo added a child revision: D20383: [PowerPC64] starting from FreeBSD 13.0, default 64-bit ELF ABI is V2 (ELFv2).May 23 2019, 4:20 PM
alfredo updated this revision to Diff 57790.May 23 2019, 6:51 PM

as pointed by bdrewery, X_LINKER_TYPE is not defined everywhere. Reverted it back to LINKER_TYPE.

Harbormaster completed remote builds in B24446: Diff 57790.May 23 2019, 6:51 PM
jhibbits added a comment.Jun 27 2019, 9:13 PM

@bdrewery any comments on this?

bdragon added a comment.Jul 1 2019, 8:29 PM

Not 100% sure, but I think you should skip the logic if either _NO_INCLUDE_COMPILERMK or _NO_INCLUDE_LINKERMK is defined. That appears to be what controls the variables not being defined.

alfredo mentioned this in D20378: [PowerPC64] LLVM as default toolchain for PowerPC and PowerPC64.Jul 1 2019, 8:46 PM
imp requested changes to this revision.Jul 12 2019, 4:01 PM
imp added a subscriber: imp.
imp added inline comments.
Makefile.libcompat
14

why a separate file?

stand/defs.mk
98

how does LIB32LD get defined? That's 100% opaque to me

This revision now requires changes to proceed.Jul 12 2019, 4:01 PM
alfredo added inline comments.Jul 12 2019, 4:22 PM
Makefile.libcompat
14

The alternative linker for the 32bit compat is a "hack" while LLD doesn't fully support powerpc32. So I did it in a different file to avoid mixing a "hack" with regular logic.

stand/defs.mk
98

It's defined by in Makefile.libcompat.inc1 and passed to 'stand' by Makefile.inc1 through CROSSENV variable.

bdrewery added inline comments.Jul 12 2019, 5:00 PM
Makefile.libcompat
14

I don't think we need a separate file for this. Especially the name is confusing if it is powerpc-specific. We have plenty of hacks already in both Makefile.inc1 and Makefile.libcompat that are arch-specific.

alfredo updated this revision to Diff 59710.Jul 12 2019, 7:05 PM
alfredo edited the test plan for this revision. (Show Details)

This is the updated patch addressing reviewer's comments.

However, I won't be able to do full tests in the next two weeks. I invite @jhibbits, @luporl and other to do more tests with this patch in case you want commit it soon.

Harbormaster completed remote builds in B25324: Diff 59710.Jul 12 2019, 7:05 PM
alfredo marked 3 inline comments as done.Jul 12 2019, 7:06 PM
imp added inline comments.Jul 14 2019, 1:23 AM
stand/defs.mk
98

This will fail when compiled stand alone outside of buildworld.

imp added inline comments.Jul 14 2019, 1:29 AM
Makefile.libcompat
19

Where is HOST_ARCH defined? Grep has no hits and make -V HOST_ARCH produces an empty string.

luporl commandeered this revision.Jul 19 2019, 4:01 PM
luporl added a reviewer: alfredo.

I've tested @alfredo.junior_eldorado.org.br's patch and I've found a few issues, that I plan to fix in the next diff, as well as address @imp's comments.

luporl updated this revision to Diff 59931.Jul 19 2019, 4:09 PM
  • Fix cross build detection
  • Refactored LIB32LD detection code
  • Avoid using short form of -fuse-ld, that avoids issues with native builds and picking up a newer ld in PATH
  • Using COMPILER_TYPE instead of LINKER_TYPE in stand/defs.mk, as an installed /usr/local/bin/ld pointing to ld.bfd can confuse LINKER_TYPE detection in this case.
  • Aborting if LIB32LD is not defined when building stand. This will tipically happen when trying to build stand alone and without performing a 'make buildenv' first
Harbormaster completed remote builds in B25424: Diff 59931.Jul 19 2019, 4:10 PM
luporl marked an inline comment as done.Jul 19 2019, 4:19 PM
luporl added inline comments.
Makefile.libcompat
19

Replaced by _HOST_ARCH.

stand/defs.mk
98

LIB32LD will be passed to stand either through buildworld or by first performing a 'make buildenv' before changing to stand and trying to build it.

But when trying to build stand without calling buildenv first, LIB32LD will be undefined.
In this case I chose to report an error, informing that LIB32LD must be set when stand is built with clang on powerpc64.

Is that ok? If not, what would be the alternative? Put the LIB32LD detection code in a separate make file that could be included by stand/defs.mk?

luporl marked an inline comment as done.Jul 19 2019, 4:23 PM
alfredo commandeered this revision.Aug 5 2019, 4:42 PM
alfredo edited reviewers, added: luporl; removed: alfredo.

I'm back, and I'll validate latest changes in different scenarios.

alfredo planned changes to this revision.Aug 5 2019, 4:45 PM

There's a problem with "make release" complaining about LIB32LD variable being not set on latest revision of this patch. I'm working on that.

imp added inline comments.Aug 6 2019, 5:39 PM
stand/defs.mk
98

No. I'd say that's not OK. It's almost never OK to have .error statements based on things not being defined. there's too many use cases to know you covered them all.

alfredo updated this revision to Diff 61142.Aug 22 2019, 11:26 PM
alfredo edited the test plan for this revision. (Show Details)

Patch refactored. It now addresses the following cases:

  • native powerpc64 build
  • native stand powerpc64 build from src/stand
  • cross build from amd64
  • cross build from amd64 using toolchain file
Harbormaster completed remote builds in B26025: Diff 61142.Aug 22 2019, 11:26 PM
alfredo updated this revision to Diff 61158.Aug 23 2019, 2:18 PM

cleanup/reorganize includes and fix missing WORLDTMP variable in a use case

Harbormaster completed remote builds in B26030: Diff 61158.Aug 23 2019, 2:18 PM
bdragon added a comment.Oct 12 2019, 8:45 PM

Needs reroll due to recent changes in Makefile.libcompat. (Although lld ppc32 support is coming along which will render this moot anyway)

bdragon added a comment.Oct 16 2019, 1:58 AM

Needs rework for recent libcompat changes.

emaste added a comment.Nov 8 2019, 7:13 PM

lld ppc32 support is coming along

Is the intent then to switch to lld for both 32- and 64-bit powerpc?

jhibbits added a comment.Nov 8 2019, 7:30 PM

That's the goal, yes. There are still bugs with powerpc, that we hope to iron out beforehand. However, if consensus says, we can punt on that for a little bit, but that would also delay a lot of other things I want to get in once we have lld (like ifuncs for pmap, etc).

emaste added a comment.Nov 8 2019, 8:05 PM

I would definitely get this in even if we have to leave ld.bfd for 32-bit powerpc initially.

imp added a comment.Nov 8 2019, 10:53 PM
In D20261#487283, @emaste wrote:

I would definitely get this in even if we have to leave ld.bfd for 32-bit powerpc initially.

After brooks' work, this needs to be rebased.

alfredo updated this revision to Diff 64096.Nov 9 2019, 1:41 AM
alfredo edited the summary of this revision. (Show Details)

updated patch for current HEAD

Harbormaster completed remote builds in B27408: Diff 64096.Nov 9 2019, 1:41 AM
imp added inline comments.Nov 9 2019, 6:49 PM
share/mk/bsd.compat.mk
59 ↗(On Diff #64096)

Please don't use TARGET_ARCH. It's incorrect here since this is included from both Makefile.inc1 and from individual makefiles.

Instead, please follow the (new) convention of the rest of the file and use COMPAT_ARCH since it does the proper dance.

alfredo updated this revision to Diff 64194.Nov 11 2019, 6:38 PM
alfredo retitled this revision from [PowerPC64] Adds support for using ld.bfd on LIB32 and STAND, when default linker is ld.lld to [PowerPC64] Use ld.bfd to build LIB32 and STAND - when using llvm.

fix rtld 32 bit build when cross building

Harbormaster completed remote builds in B27453: Diff 64194.Nov 11 2019, 6:38 PM
alfredo planned changes to this revision.Nov 11 2019, 6:38 PM
alfredo requested review of this revision.
alfredo marked an inline comment as done.
alfredo added a comment.Dec 2 2019, 1:10 PM

Any updates on this?

alfredo added a project: PowerPC.Dec 3 2019, 10:08 PM
imp added inline comments.Dec 13 2019, 5:47 PM
stand/defs.mk
98

LIB32LD still isn't defined in a file that gets included in the build.
Defining it all in bsd.compat.mk seems fundamentally wrong to me. that's not really what that's for.
If you want to use those things in the rest of the build, you'd need to define it bsd.cpu.mk (which bsd.compat.mk has access to).

imp added inline comments.Dec 13 2019, 5:56 PM
share/mk/bsd.compat.mk
64 ↗(On Diff #64194)

this hard wires the *HOSTS* current /usr/bin/ld.bfd. That's not right. It should be the one in the sysroot, even for native builds.

66 ↗(On Diff #64194)

we know COMPAT_ARCH is defined here, and that it necessarily equals powerpc64.

67 ↗(On Diff #64194)

we know 100% that COMPAT_ARCH is powerpc64 here, maybe you have a nesting issue?

94 ↗(On Diff #64194)

I'm having trouble seeing when this would be true given the full paths.
And full paths, btw, aren't a documented option for -fuse-ld in the gcc docs I found online

alfredo updated this revision to Diff 65641.Dec 14 2019, 12:22 AM

Simplified the code inspired by @imp inputs.

Please confirm if the approach looks correct. I still have to test on different scenarios

Harbormaster completed remote builds in B28145: Diff 65641.Dec 14 2019, 12:22 AM
alfredo marked 5 inline comments as done.Dec 14 2019, 12:22 AM
alfredo added inline comments.Dec 16 2019, 12:48 PM
share/mk/bsd.compat.mk
94 ↗(On Diff #64194)

Here -fuse-ld will be used when *not* using GCC. Clang implemented full paths after https://reviews.llvm.org/D17952.

alfredo edited the summary of this revision. (Show Details)Dec 16 2019, 7:16 PM
imp added inline comments.Dec 17 2019, 4:03 PM
share/mk/bsd.compat.mk
51 ↗(On Diff #65641)

Shouldn't this just be

.if ${COMPAT_COMPILER_TYPE} == "gcc"

share/mk/bsd.cpu.mk
418 ↗(On Diff #65641)

Right now CROSS_BINUTILS_PREFIX is defined only in Makefile.inc1, which I don't think exports this variable to the dependent make processes... I'm also unsure how this would work in a standalone build on a powerpc platform...
It's likely OK enough since most people don't do builds on native powerpc platforms outside of a buildworld, but that's supposed to work and may not w/o setting this variable by hand...

alfredo updated this revision to Diff 65756.Dec 17 2019, 8:46 PM
alfredo marked an inline comment as done.

add more comments

Harbormaster completed remote builds in B28201: Diff 65756.Dec 17 2019, 8:46 PM
alfredo added inline comments.Dec 17 2019, 8:47 PM
share/mk/bsd.cpu.mk
418 ↗(On Diff #65641)

Case "LD_BFD=${LOCALBASE}/bin/${CROSS_BINUTILS_PREFIX}-ld.bfd" seems to cover the native build. The correct linker path being passed to clang in my tests.

Regarding CROSS_BINUTILS_PREFIX, it's defined in the standard toolchain file (i.e: pkg install "powerpc64-xtoolchain-gcc-0.4_1" on amd64). That's the case I would like to cover

alfredo added inline comments.Dec 17 2019, 8:50 PM
share/mk/bsd.cpu.mk
418 ↗(On Diff #65641)

I'm not sure "in tree" build is covered here. But it can be handled after flagday, or it may not be needed in the near future, since ld.lld full support for PowerPC32 bit is almost complete and this LD_BFD workaround is temporary.

imp accepted this revision.Dec 24 2019, 6:08 AM

OK. I may cleanup the cross-threading I insisted on a comment for later, but this is good enough for now.

This revision is now accepted and ready to land.Dec 24 2019, 6:08 AM
Closed by commit rS356054: [PowerPC64] Use ld.bfd to build LIB32 and STAND - when using llvm (authored by jhibbits). · Explain WhyDec 24 2019, 4:03 PM
This revision was automatically updated to reflect the committed changes.
jhibbits added a commit: rS356054: [PowerPC64] Use ld.bfd to build LIB32 and STAND - when using llvm.

Revision Contents
Changeset List

PathSize
1 line
46 lines
stand/
4 lines

Diff 59710

View Options

Makefile.inc1

Loading...
View Options

Makefile.libcompat

Loading...
View Options

stand/defs.mk

Loading...