Page MenuHomeFreeBSD

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

Authored by alfredo.junior_eldorado.org.br on May 14 2019, 5:18 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.)

  • On native powerpc64 build, it adds parameter "-fuse-ld=bfd" to clang, so it locates default ld.bfd install.
  • When cross compiling, it makes variable CROSS_BINUTILS_PREFIX mandatory, so it can locate the toolchain for the target platform, normally installed by pkg (as example, when CROSS_BINUTILS_PREFIX=powerpc64-unknown-freebsd12.0 is set, it looks for powerpc64-unknown-freebsd12.0-ld.bfd.
  • In any case, variable LIB32LD can be set to manually specify the absolute path for an arbitrary linker and override the options above
  • Default behavior for other platforms aren't changed.

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

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 27453
Build 25691: arc lint + arc unit

Event Timeline

alfredo.junior_eldorado.org.br edited the test plan for this revision. (Show Details)
alfredo.junior_eldorado.org.br edited the summary of this revision. (Show Details)
alfredo.junior_eldorado.org.br planned changes to this revision.May 17 2019, 11:49 AM

The case "cross compiling without using CROSS_TOOLCHAIN file" isn't currently supported by this change, I'm working on that.

improve patch to cover x86->powerpc64 cross compiling without external toolchain scenario

  • [PowerPC64] Adds support for using ld.bfd on LIB32 and STAND, when default linker is ld.lld
  • improve suport for using ld.bfd on LIB32, covering x86->powerpc64 cross compiling without external toolchain
bdragon added inline comments.May 20 2019, 4:42 PM
stand/defs.mk
102

I'm seeing build failures at cleandir stage. I guess LINKER_TYPE isn't being defined at that point. So ${LINKER_TYPE} would need to be wrapped in quotes, or for LINKER_TYPE to be defined at all stages.

jhibbits added a subscriber: bdrewery.

@bdrewery knows the Makefiles much better than probably anybody else on the project.

  • [PowerPC64] Adds support for using ld.bfd on LIB32 and STAND, when default linker is ld.lld
  • improve suport for using ld.bfd on LIB32, covering x86->powerpc64 cross compiling without external toolchain
  • fix an issue with make clean
alfredo.junior_eldorado.org.br 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"

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

@bdrewery any comments on this?

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
103

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
103

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 ↗(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.junior_eldorado.org.br 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.

alfredo.junior_eldorado.org.br 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
103

This will fail when compiled stand alone outside of buildworld.

imp added inline comments.Jul 14 2019, 1:29 AM
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 commandeered this revision.Jul 19 2019, 4:01 PM

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
luporl marked an inline comment as done.Jul 19 2019, 4:19 PM
luporl added inline comments.
Makefile.libcompat
19 ↗(On Diff #59710)

Replaced by _HOST_ARCH.

stand/defs.mk
103

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

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

alfredo.junior_eldorado.org.br 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
103

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.junior_eldorado.org.br 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.

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

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

emaste added a comment.Fri, Nov 8, 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.Fri, Nov 8, 10:53 PM

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

updated patch for current HEAD

imp added inline comments.Sat, Nov 9, 6:49 PM
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.junior_eldorado.org.br 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.junior_eldorado.org.br planned changes to this revision.Mon, Nov 11, 6:38 PM
alfredo.junior_eldorado.org.br requested review of this revision.
alfredo.junior_eldorado.org.br marked an inline comment as done.