Bug 239743 - Mk/bsd.port.mk: don't switch to ld.bfd on powerpc64 with clang
Summary: Mk/bsd.port.mk: don't switch to ld.bfd on powerpc64 with clang
Status: New
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Port Management Team
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-09 11:54 UTC by Piotr Kubaj
Modified: 2019-08-10 15:24 UTC (History)
3 users (show)

See Also:


Attachments
patch (625 bytes, patch)
2019-08-09 11:54 UTC, Piotr Kubaj
no flags Details | Diff
patch (1023 bytes, patch)
2019-08-10 04:48 UTC, Jan Beich
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Kubaj freebsd_committer 2019-08-09 11:54:37 UTC
Created attachment 206395 [details]
patch

With powerpc64 soon switching to clang, it doesn't make sense to use base ld.bfd when LLD_UNSAFE is defined.
Comment 1 Antoine Brodin freebsd_committer 2019-08-09 14:59:22 UTC
(In reply to Piotr Kubaj from comment #0)
The LDFLAGS line doesn't need to be changed.
Also the mis-indentation make me think the patch is wrong.
Comment 2 Antoine Brodin freebsd_committer 2019-08-09 15:13:37 UTC
And finally,  I don't understand why powerpc64 needs a special treatment here.
Comment 3 Antoine Brodin freebsd_committer 2019-08-09 15:33:59 UTC
You probably want something like this?

%%%
 LDFLAGS+=      -fuse-ld=bfd
 BINARY_ALIAS+= ld=${LD}
 .  if !defined(USE_BINUTILS)
-.    if exists(/usr/bin/ld.bfd)
+.    if exists(/usr/bin/ld.bfd) && (${ARCH} != powerpc64 || ${OSVERSION} < 1300038)
 LD=    /usr/bin/ld.bfd
 CONFIGURE_ENV+=        LD=${LD}
 MAKE_ENV+=     LD=${LD}
%%%
Comment 4 Piotr Kubaj freebsd_committer 2019-08-09 17:44:39 UTC
(In reply to Antoine Brodin from comment #3)
I'll test this patch later and confirm whether it's ok.

Regarding why powerpc64 needs special treatment:
when it switches to clang, ld.bfd will still be installed. The reason is that lld currently doesn't support linking powerpc (32-bit) binaries. We will use lld for 64-bit binaries, however.

So even after the switch /usr/bin/ld.bfd will still be installed (until lld is able to link powerpc binaries).
Comment 5 Piotr Kubaj freebsd_committer 2019-08-09 18:45:29 UTC
(In reply to Piotr Kubaj from comment #4)
This is still not enough (my patch works). misc/e2fsprogs-libuuid fails to configure:
   1187 configure:3325: cc -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing -std=gnu99  -I/tmp/usr/ports/misc/e2fsprogs-libuuid/work/e2fsprogs-1.45.3/lib -I/usr/local/include
  -fstack-protector-strong -fuse-ld=bfd  conftest.c  >&5
   1188 /usr/bin/ld.bfd: Dwarf Error: found dwarf version '4', this reader only handles version 2 information.
   1189 /usr/lib/crt1.o: In function `_start':
   1190 crt1.c:(.text+0x2): undefined reference to `.TOC.'

The reason is that ld.bfd is used, which doesn't even know about elfv2. We need to either use lld (which works for this port on CURRENT but not on 12.0-RELEASE/amd64) or ld.bfd from ports. Since -fuse-ld allows for passing the whole patch to the linker, this is what we must do.
Comment 6 Piotr Kubaj freebsd_committer 2019-08-09 18:49:06 UTC
Relevant base review: https://reviews.freebsd.org/D20261
Comment 7 Piotr Kubaj freebsd_committer 2019-08-09 19:13:35 UTC
(In reply to Antoine Brodin from comment #3)
This patch is ok and much simpler:
Index: Mk/bsd.port.mk
===================================================================
--- Mk/bsd.port.mk      (revision 508433)
+++ Mk/bsd.port.mk      (working copy)
@@ -1820,10 +1820,10 @@
 .endif

 .if defined(LLD_UNSAFE) && ${/usr/bin/ld:L:tA} == /usr/bin/ld.lld
-LDFLAGS+=      -fuse-ld=bfd
+LDFLAGS+=      -fuse-ld=${LD}
 BINARY_ALIAS+= ld=${LD}
 .  if !defined(USE_BINUTILS)
-.    if exists(/usr/bin/ld.bfd)
+.    if exists(/usr/bin/ld.bfd) && (${ARCH} != powerpc64 || ${OSVERSION} < 1300038)
 LD=    /usr/bin/ld.bfd
 CONFIGURE_ENV+=        LD=${LD}
 MAKE_ENV+=     LD=${LD}
Comment 8 Antoine Brodin freebsd_committer 2019-08-09 19:59:45 UTC
(In reply to Piotr Kubaj from comment #7)
gcc doesn't understand this syntax
Comment 9 Piotr Kubaj freebsd_committer 2019-08-09 20:07:35 UTC
(In reply to Antoine Brodin from comment #8)
This is not about GCC, powerpc64 switches to Clang (both 32-bit and 64-bit). The problem is with LLD which can't link ppc32 binaries, that's why GNU ld needs to be used.

So 32-bit binaries will be compiled by Clang and linked by GNU ld.
Comment 10 Antoine Brodin freebsd_committer 2019-08-09 20:17:32 UTC
(In reply to Piotr Kubaj from comment #9)
Sure, but the ports tree is not just powerpc64 and some ports are compiled with gcc even on amd64 or i386

Does this patch work?

%%%
 LDFLAGS+=      -fuse-ld=bfd
 BINARY_ALIAS+= ld=${LD}
 .  if !defined(USE_BINUTILS)
-.    if exists(/usr/bin/ld.bfd)
+.    if exists(/usr/bin/ld.bfd) && (${ARCH} != powerpc64 || ${OSVERSION} < 1300038)
 LD=    /usr/bin/ld.bfd
 CONFIGURE_ENV+=        LD=${LD}
 MAKE_ENV+=     LD=${LD}
 .    else
+BINARY_ALIAS+= ld.bfd=${LOCALBASE}/bin/ld.bfd
 USE_BINUTILS=  yes
 .    endif
 .  endif
%%%

The first patch you proposed ( https://bugs.freebsd.org/bugzilla/attachment.cgi?id=206395 ) was breaking ports on amd64 / i386 / aarch64 btw.
Comment 11 Piotr Kubaj freebsd_committer 2019-08-09 20:59:27 UTC
(In reply to Antoine Brodin from comment #10)
No, it still fails with the same error:
   1187 configure:3325: cc -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing -std=gnu99  -I/tmp/usr/ports/misc/e2fsprogs-libuuid/work/e2fsprogs-1.45.3/lib -I/usr/local/include
  -fstack-protector-strong -fuse-ld=bfd  conftest.c  >&5
   1188 /usr/bin/ld.bfd: Dwarf Error: found dwarf version '4', this reader only handles version 2 information.
   1189 /usr/lib/crt1.o: In function `_start':
   1190 crt1.c:(.text+0x2): undefined reference to `.TOC.'
   1191 /usr/bin/ld.bfd: /usr/lib/crt1.o: unknown relocation type 252 for symbol .TOC.
Comment 12 Piotr Kubaj freebsd_committer 2019-08-09 21:14:32 UTC
Note that since powerpc64 currently still use gcc in head, this PR is just to agree on a proper patch. Once we have a proper patch, this PR should wait until the switch to clang.
Comment 13 Jan Beich freebsd_committer 2019-08-10 04:48:33 UTC
Created attachment 206411 [details]
patch

Antoine, does this work for you? It worked for pkubaj@. I'm not sure if my commit message/inline comment are correct, though.

- Only one extra conditional (easy to understand)
- Comment why there (easy to refactor)
- Drop OSVERSION to avoid redundancy with ld == lld conditional
- Add -B${LOCALBASE}/bin because USE_BINUTILS alone doesn't

(In reply to Piotr Kubaj from comment #4)
> Regarding why powerpc64 needs special treatment:
> when it switches to clang, ld.bfd will still be installed.
> The reason is that lld currently doesn't support linking powerpc (32-bit) binaries.
> We will use lld for 64-bit binaries, however.

Do you mean -m64 uses ELFv2 while -m32 uses ELFv1? If so either review D20261 or review D20383 needs to make Clang default to BFD linker when -m32 is passed e.g., via getDefaultLinker().
Comment 14 Antoine Brodin freebsd_committer 2019-08-10 07:02:56 UTC
(In reply to Jan Beich from comment #13)
If it works on powerpc64,  it's less intrusive for other archs.
Comment 15 Piotr Kubaj freebsd_committer 2019-08-10 15:24:25 UTC
(In reply to Jan Beich from comment #13)
There's no ELFv1 and ELFv2 for powerpc. powerpc has only one ABI and that doesn't change with the switch to Clang. ELFv2 is only defined for powerpc64.

Regarding using GNU ld by default in Clang, I passed your suggestion to bdragon@, who currently leads the effort of switching to Clang, he said he would look at it.

Since there's no OSVERSION check, I think this patch is ok to be committed now. OK?