Page MenuHomeFreeBSD

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

Authored by alfredo on May 14 2019, 5:18 PM.
Referenced Files
F105871017: D20261.id64096.diff
Sat, Dec 21, 9:21 PM
Unknown Object (File)
Wed, Dec 18, 3:58 AM
Unknown Object (File)
Wed, Dec 18, 1:41 AM
Unknown Object (File)
Tue, Dec 17, 11:33 PM
Unknown Object (File)
Sun, Dec 15, 11:29 AM
Unknown Object (File)
Fri, Dec 13, 9:18 AM
Unknown Object (File)
Mon, Nov 25, 6:48 PM
Unknown Object (File)
Mon, Nov 25, 5:33 PM

Details

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 28201
Build 26326: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Replaced LINKER_TYPE by X_LINKER_TYPE.
  • Added (") around X_LINKER_TYPE to fix "make clean"

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

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.

imp requested changes to this revision.Jul 12 2019, 4:01 PM
imp added a subscriber: imp.
imp added inline comments.
Makefile.libcompat
14 ↗(On Diff #57790)

why a separate file?

stand/defs.mk
104

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

This revision now requires changes to proceed.Jul 12 2019, 4:01 PM
Makefile.libcompat
14 ↗(On Diff #57790)

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
104

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

Makefile.libcompat
14 ↗(On Diff #57790)

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 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.

stand/defs.mk
104

This will fail when compiled stand alone outside of buildworld.

Makefile.libcompat
19 ↗(On Diff #59710)

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

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.

  • 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
luporl added inline comments.
Makefile.libcompat
19 ↗(On Diff #59710)

Replaced by _HOST_ARCH.

stand/defs.mk
104

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?

alfredo edited reviewers, added: luporl; removed: alfredo.

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

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.

stand/defs.mk
104

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 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

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

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

Needs rework for recent libcompat changes.

lld ppc32 support is coming along

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

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).

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

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 edited the summary of this revision. (Show Details)

updated patch for current HEAD

share/mk/bsd.compat.mk
66

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 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

alfredo requested review of this revision.
alfredo marked an inline comment as done.
stand/defs.mk
104

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).

share/mk/bsd.compat.mk
56

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

64

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

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

67

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

Simplified the code inspired by @imp inputs.

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

share/mk/bsd.compat.mk
56

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

share/mk/bsd.compat.mk
51

Shouldn't this just be

.if ${COMPAT_COMPILER_TYPE} == "gcc"

share/mk/bsd.cpu.mk
418

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 marked an inline comment as done.

add more comments

share/mk/bsd.cpu.mk
418

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

share/mk/bsd.cpu.mk
418

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.

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