Page MenuHomeFreeBSD

[PowerPC64] Adds support for using ld.bfd on LIB32 and STAND, when default linker is ld.lld
Needs ReviewPublic

Authored by on May 14 2019, 5:18 PM.



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, however these can make use of LIB32LD variable, optionally.


Diff Detail

Lint OK
No Unit Test Coverage
Build Status
Buildable 26030
Build 24575: arc lint + arc unit

Event Timeline edited the test plan for this revision. (Show Details) edited the summary of this revision. (Show Details) 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

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 marked an inline comment as done.May 20 2019, 7:14 PM
  • 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.

why a separate file?


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

This revision now requires changes to proceed.Jul 12 2019, 4:01 PM

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.


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

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. 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. marked 3 inline comments as done.Jul 12 2019, 7:06 PM
imp added inline comments.Jul 14 2019, 1:23 AM

This will fail when compiled stand alone outside of buildworld.

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

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'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/, 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.

Replaced by _HOST_ARCH.


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/

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

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