Page MenuHomeFreeBSD

lang/ghc: add armv6, armv7 and aarch64 to the list of supported architectures
ClosedPublic

Authored by mikael on Jun 6 2018, 10:25 AM.
Tags
None
Referenced Files
F84148938: D15674.diff
Mon, May 20, 1:04 AM
F84143668: D15674.id47389.diff
Sun, May 19, 11:35 PM
Unknown Object (File)
Sat, May 18, 4:54 AM
Unknown Object (File)
Thu, May 16, 1:11 AM
Unknown Object (File)
Tue, May 7, 9:59 PM
Unknown Object (File)
Tue, May 7, 6:14 PM
Unknown Object (File)
Sat, May 4, 6:31 PM
Unknown Object (File)
Sun, Apr 28, 5:23 AM

Details

Summary

lang/ghc: add armv6, armv7 and aarch64 to the list of supported architectures.

Corresponding PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=196899

Test Plan

build and test on armv6, armv7 and aarch64 (real hw)
qemu-user-static has too many bugs to compile lang/ghc

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lang/ghc/Makefile
182 ↗(On Diff #43355)

Why exclude ARM arches here? IIRC, This fiddling is done for ino64 change, which affects all arches.

217 ↗(On Diff #43355)

The code here seems to be contradicting with the comment. You are replacing LOCALBASE with ${PREFIX} instead of setting linker. Can you explain this block?

235 ↗(On Diff #43355)

This also relates to ino64. Doesn't it work for ARM?

lang/ghc/files/extra-patch-aclocal.m4
1 ↗(On Diff #43355)

Do you plan to upstream these changes to GHC?

lang/ghc/Makefile
182 ↗(On Diff #43355)

My bootstraps binaries are based on FreeBSD 12, they already contains the ino64 change and don't need the 'wrap' hack. It's only needed when the bootstrap compiler is pre ino64 (FreeBSD 10.3 for example).

217 ↗(On Diff #43355)

I want to replace LD_NO_GOLD=ld with LD_NO_GOLD=/usr/local/bin/ld.
I can do @${REINPLACE_CMD} -e 's|LD_NO_GOLD=ld|LD_NO_GOLD=${PREFIX}/bin/ld|' if you prefer (and update aclocal.m4)

235 ↗(On Diff #43355)

I can build the 'wrap' stuff but it's not needed for arm since the bootstraps binaries is post ino64.
I can remove the 'arm' condition if you think the 'if' block is overloaded.

lang/ghc/files/extra-patch-aclocal.m4
1 ↗(On Diff #43355)

I will try to.

lang/ghc/Makefile
217 ↗(On Diff #43355)

What's LD_NO_GOLD? The linker to be used is set via env var LD. See line 120 of this Makefile.

235 ↗(On Diff #43355)

It can stay, I just making sure there is no hidden problem somewhere.

lang/ghc/files/extra-patch-aclocal.m4
1 ↗(On Diff #43355)

Please do this for the patches you added and share the link to review on GHC's Phabricator with me.

lang/ghc/files/patch-configure.ac
18 ↗(On Diff #43355)

What's the difference between AC_CHECK_TARGET_TOOL and AC_CHECK_TOOL? Do you plan to upstream this too?

35 ↗(On Diff #43355)

Judging from configure.ac, these can be set with LLC and OPT env vars.

lang/ghc/files/patch-llvm-targets
7 ↗(On Diff #43355)

Is this required? How this file is used?

lang/ghc/Makefile
217 ↗(On Diff #43355)

LD_NO_GOLD is just used to speed up some linking phase (ld.gold is slower than 'ld' and 'ld' can be used in some parts of the build without breaking ghc-stage2). Also, our base ld is too old and doesn't work (don't have the error log) so binutils:ld must be used.
USE_GCC overrides LD with /usr/local/bin/ld (line 120 has no effect for arm)

lang/ghc/files/patch-configure.ac
18 ↗(On Diff #43355)

ghc considers armv6 and armv7 as a single arch (called arm) and the configure script thinks that we are cross compiling and searches for cross compile tool. Build and target look like this: build : armv7-unknown-freebsd ; target : arm-unknown-freebsd
I will do the same on our side (ie build: arm-unknown-freebsd).
Thanks for pointing this out.

35 ↗(On Diff #43355)

Didn't know that, many thanks!

lang/ghc/files/patch-llvm-targets
7 ↗(On Diff #43355)

I don't know the gory details but it's required when the llvm backend is used (probably to generate code for the correct triple)
I haven't patched utils/llvm-targets/gen-data-layout.sh locally but I'll submit a patch upstream.

Address comments from arrowd

lang/ghc/Makefile
217 ↗(On Diff #43355)

If LD_NO_GOLD just speeds the build up, we can move it out of .if ${ARCH} == aarch64 || ${ARCH} == armv6 || ${ARCH} == armv7?

lang/ghc/Makefile
217 ↗(On Diff #43355)

It'll add an extra build dependency for amd64/i386: binutils and I'm not sure it's worth it on those arches.

lang/ghc/Makefile
217 ↗(On Diff #43355)

I don't quite get it. If we already aren't using gold, then setting LD_NO_GOLD should be a no-op, no?

lang/ghc/Makefile
217 ↗(On Diff #43355)

My comment about LD_NO_GOLD is completely wrong, I have mixed up something in my head....

From rules/build-package-way.mk:
NB. LD_NO_GOLD above: see #14328 (symptoms: #14675,#14291). At least
some versions of ld.gold appear to have a bug that causes the
generated GHCi library to have some bogus relocations. Performance
isn't critical here, so we fall back to the ordinary ld

Hi! Thanks for this work, I'd love to see this merged soon.

GHCi segfaults at launch on aarch64 (not a blocker — compiled mode working is good enough for now — but would be very very nice to have ghci working too):

* thread #1, name = 'ghc', stop reason = signal SIGSEGV
  * frame #0: 0x000000004859a1a4 libHSrts_thr-ghc8.4.3.so`freeStablePtrUnsafe + 28
    frame #1: 0x000000004859a1c4 libHSrts_thr-ghc8.4.3.so`freeStablePtr + 20
    frame #2: 0x0000000048591e90 libHSrts_thr-ghc8.4.3.so`freeHaskellFunctionPtr + 16
    frame #3: 0x0000000045d0fe44 libHSterminfo-0.4.1.1-ghc8.4.3.so
    frame #4: 0x00000000485b6ad0 libHSrts_thr-ghc8.4.3.so

Also, please fix bsd.cabal.mk — with the patch as-is, shared libraries in cabal based ports don't get packaged! — it looks for x86_64-… shared libs. CABAL_ARCH should probably just be ${ARCH:S/amd64/x86_64/} instead of the x86-only hard-code that is there.

In D15674#354015, @greg_unrelenting.technology wrote:

GHCi segfaults at launch on aarch64 (not a blocker — compiled mode working is good enough for now — but would be very very nice to have ghci working too):

It works here with ghc-8.4.2 (with gcc6, DYNAMIC and PROFILE options are "off")

root@tegra-x1: ~ ghci
GHCi, version 8.4.2: http://www.haskell.org/ghc/  :? for help
Prelude> putStrLn "Hello World"
Hello World
Prelude> :load Main
[1 of 1] Compiling Main             ( Main.hs, interpreted )
Ok, one module loaded.
*Main> fac 17
355687428096000
*Main>

Also, please fix bsd.cabal.mk — with the patch as-is, shared libraries in cabal based ports don't get packaged! — it looks for x86_64-… shared libs. CABAL_ARCH should probably just be ${ARCH:S/amd64/x86_64/} instead of the x86-only hard-code that is there.

I'm aware of the problem (see pr 196899 comment 5) but I'm not able to fix it :(

gcc6, DYNAMIC and PROFILE options are "off")

hm, I have gcc7 and these options ON.

How to fix the arch —

CABAL_ARCH should probably just be ${ARCH:S/amd64/x86_64/}

I meant this:

--- i/lang/ghc/bsd.cabal.mk
+++ w/lang/ghc/bsd.cabal.mk
@@ -54,10 +54,7 @@ GHC_LIB_DOCSDIR_REL= share/doc/ghc-${GHC_VERSION}/html/libraries
 
 CABAL_LIBDIR=          ${PREFIX}/lib/cabal/ghc-${GHC_VERSION}
 CABAL_LIBSUBDIR=       ${PACKAGE}
-CABAL_ARCH=            x86_64
-.if ("${ARCH}" == "i386")
-CABAL_ARCH=            i386
-.endif
+CABAL_ARCH=            ${ARCH:S/amd64/x86_64/}
 CABAL_ARCHSUBDIR=      ${CABAL_ARCH}-freebsd-ghc-${GHC_VERSION}
 CABAL_LIBDIR_REL=      ${CABAL_LIBDIR:S,^${PREFIX}/,,}

tegra-x1

hmm :) please tell me more!

(also additional replacements might be required for armv6/7 — I don't know how what ghc calls them, but e.g. if they're both just 'arm' you'd have something like ${ARCH:S/amd64/x86_64/:S/armv6/arm/:S/armv7/arm})

Rework CABAL_ARCH based on Greg suggestion.

And now I'm seeing a segfault on armv7 due to the use of the wrong linker :/
Give me a few days to troubleshot the issue.

lang/ghc/Makefile
112 ↗(On Diff #46649)

This message gives your opinion and a bad one about qemu. Please make it more neutral.

218 ↗(On Diff #46649)

This is probably wrong, it should be using LOCALBASE and not PREFIX.

182 ↗(On Diff #43355)

add a comment saying so.

Disable dynamic linking on arm

mikael added inline comments.
lang/ghc/Makefile
112 ↗(On Diff #46649)

It's not my opinion it's the sad reality.

lang/ghc/files/patch-configure
3 ↗(On Diff #47189)

This patch seems to be a duplicate of https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=228706 no?

lang/ghc/files/patch-configure
3 ↗(On Diff #47189)

No, pr 228706 adds the correct suffix for arm* target. This patch adds the correct compiler flags to build ghc on arm*

With last raised points fixed this looks OK. Does it apply cleanly to HEAD?

lang/ghc/Makefile
104 ↗(On Diff #47189)

${LOCALBASE}/bin/ part shouldn't be needed, just like for opt50.

lang/ghc/Makefile
112 ↗(On Diff #46649)

It does not change the fact that this message is insulting for people who work on qemu.

lang/ghc/Makefile
112 ↗(On Diff #46649)

I work on qemu-user-static and I don't find this message insulting. Feel free to propose something else if you don't like it.

With last raised points fixed this looks OK. Does it apply cleanly to HEAD?

It applies cleanly on ports @477958

Bad news. When building GHC with this patch applied on 12.0-CURRENT amd64, the ghc-iserv-prof executable gets broken.

To reproduce run make -C /usr/ports/lang/ghc and then /usr/ports/lang/ghc/work/stage/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof.

Backtrace doesn't ring a bell:

(gdb) run
Starting program: /wrkdirs/usr/ports/lang/ghc/work/stage/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof 

Program received signal SIGSEGV, Segmentation fault.
digest_dynamic1 (obj=<optimized out>, early=0, dyn_rpath=<optimized out>, dyn_soname=<optimized out>, dyn_runpath=<optimized out>) at /usr/src/libexec/rtld-elf/rtld.c:1099
1099                    obj->chain_zero_gnu = obj->buckets_gnu + obj->nbuckets_gnu -
(gdb) bt
#0  digest_dynamic1 (obj=<optimized out>, early=0, dyn_rpath=<optimized out>, dyn_soname=<optimized out>, dyn_runpath=<optimized out>) at /usr/src/libexec/rtld-elf/rtld.c:1099
#1  0x0000000801d3d4f6 in digest_dynamic (early=0, obj=<optimized out>) at /usr/src/libexec/rtld-elf/rtld.c:1348
#2  _rtld (sp=<optimized out>, exit_proc=0x7fffffffeaa0, objp=0x7fffffffeaa8) at /usr/src/libexec/rtld-elf/rtld.c:604
#3  0x0000000801d3c019 in .rtld_start () at /usr/src/libexec/rtld-elf/amd64/rtld_start.S:39
#4  0x0000000000000000 in ?? ()

Bad news. When building GHC with this patch applied on 12.0-CURRENT amd64, the ghc-iserv-prof executable gets broken.

To reproduce run make -C /usr/ports/lang/ghc and then /usr/ports/lang/ghc/work/stage/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof.

It also happens without my patch.
The package is also broken:

fetch http://pkg.freebsd.org/FreeBSD:12:amd64/latest/All/ghc-8.4.3_1.txz
mkdir /tmp/ghc
tar xf ghc-8.4.3_1.txz -C /tmp/ghc
/tmp/ghc/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof 
Segmentation fault (core dumped)
Exit 139

FWIW it works fine with gcc7:

/usr/ports/lang/ghc/work/stage/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof
ghc-iserv-prof: usage: iserv <write-fd> <read-fd> [-v]
In D15674#360189, @mikael.urankar_gmail.com wrote:

Bad news. When building GHC with this patch applied on 12.0-CURRENT amd64, the ghc-iserv-prof executable gets broken.

To reproduce run make -C /usr/ports/lang/ghc and then /usr/ports/lang/ghc/work/stage/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof.

It also happens without my patch.
The package is also broken:

fetch http://pkg.freebsd.org/FreeBSD:12:amd64/latest/All/ghc-8.4.3_1.txz
mkdir /tmp/ghc
tar xf ghc-8.4.3_1.txz -C /tmp/ghc
/tmp/ghc/usr/local/lib/ghc-8.4.3/bin/ghc-iserv-prof 
Segmentation fault (core dumped)
Exit 139

Hum, right. Then it shouldn't block this diff. But it is a strange bug, because stage-qa fails sometimes due to this broken executable, and sometimes it passes.

Very well, if you've verified that with your patch everything's working on ARM, I'm OK with this. I will reword IGNORE message for QEMU_EMULATING case and commit this after getting @tcberner or @AMDmi3 approval.

lang/ghc/Makefile
62 ↗(On Diff #47229)

^missing tab aber =

100 ↗(On Diff #47229)

^ could you indent the nested ifs - that makes it a tiny bit more readable.

.if ...
.  if ...

.  endif
.endif

Add missing tab after OPTIONS_EXCLUDE_aarch64=
Indent the nested 'if'

Very good -- thanks for working on this :)

@arrowd once you have the bootstrap tarball uploaded it looks good to me (obviously, I did not verify the arm-build :)) -- remember to change the comment.

This revision is now accepted and ready to land.Aug 28 2018, 6:11 PM
lang/ghc/distinfo
13 ↗(On Diff #47389)

This SIZE line refer ghc-8.4.1, while others are 8.4.2.

This revision now requires review to proceed.Aug 29 2018, 9:55 AM

Thanks for this! Have the autoconf patches made it upstream, out of curiosity?

lang/ghc/Makefile
112 ↗(On Diff #47436)

maybe add "for this architecture"?

Thanks for this! Have the autoconf patches made it upstream, out of curiosity?

yes, see https://github.com/ghc/ghc/commit/297879a78cc6ca4c27afb0cc863c8796b60da6e1

lang/ghc/Makefile
112 ↗(On Diff #47436)

It's already in the 'if ${ARCH}' block, do you think it's needed?

lang/ghc/Makefile
112 ↗(On Diff #47436)

For someone reading the Makefile no; I just assumed someone might see arm64 build log output without noticing that it wasn't amd64. But I suspect it's rather unlikely.

Accepting again to make it closable.

This revision is now accepted and ready to land.Aug 31 2018, 8:47 AM
This revision was automatically updated to reflect the committed changes.