Bug 233664 - enable LLVM libunwind for armv7, armv6
Summary: enable LLVM libunwind for armv7, armv6
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Michal Meloun
URL:
Keywords: feature, needs-patch, needs-qa
Depends on:
Blocks: 233094
  Show dependency treegraph
 
Reported: 2018-11-30 13:57 UTC by Ed Maste
Modified: 2020-01-08 08:49 UTC (History)
3 users (show)

See Also:
koobs: mfc-stable12-
koobs: mfc-stable11-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Maste freebsd_committer 2018-11-30 13:57:25 UTC
arm currently uses Clang and LLD, but LLVM_LIBUNWIND is not enabled.

For armv7 there are a couple of minor conflicts with other unwind helpers, and we need to update the symbol version map.  The hack below gets it to build, but needs to be cleaned up.

arm and armv6 fail with:
/.../arm.arm/tmp/usr/lib/libcxxrt.so: undefined reference to `__gnu_unwind_frame@GCC_3.5'
/.../arm.arm/tmp/usr/lib/libcxxrt.so: undefined reference to `_Unwind_VRS_Set@GCC_3.5'
/.../arm.arm/tmp/usr/lib/libcxxrt.so: undefined reference to `_Unwind_VRS_Get@GCC_3.5'

armv7 hack patch:

diff --git a/contrib/compiler-rt/lib/builtins/gcc_personality_v0.c b/contrib/compiler-rt/lib/builtins/gcc_personality_v0.c
index 0bc765624564..034d323814c4 100644
--- a/contrib/compiler-rt/lib/builtins/gcc_personality_v0.c
+++ b/contrib/compiler-rt/lib/builtins/gcc_personality_v0.c
@@ -145,6 +145,7 @@ static uintptr_t readEncodedPointer(const uint8_t** data, uint8_t encoding)
 #if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) &&                 \
     !defined(__ARM_DWARF_EH__)
 #define USING_ARM_EHABI 1
+struct _Unwind_Exception;
 _Unwind_Reason_Code __gnu_unwind_frame(struct _Unwind_Exception *,
                                        struct _Unwind_Context *);
 #endif
diff --git a/contrib/compiler-rt/lib/builtins/unwind-ehabi-helpers.h b/contrib/compiler-rt/lib/builtins/unwind-ehabi-helpers.h
index ccb0765975a9..864dba716e9b 100644
--- a/contrib/compiler-rt/lib/builtins/unwind-ehabi-helpers.h
+++ b/contrib/compiler-rt/lib/builtins/unwind-ehabi-helpers.h
@@ -39,8 +39,6 @@
 #define _URC_OK       0
 #define _URC_FAILURE  9
 
-typedef uint32_t _Unwind_State;
-
 #if !defined(_US_UNWIND_FRAME_STARTING)
 #define _US_UNWIND_FRAME_STARTING ((_Unwind_State)1)
 #endif
diff --git a/lib/libgcc_s/Version.map b/lib/libgcc_s/Version.map
index 622732edb447..42a3b757d513 100644
--- a/lib/libgcc_s/Version.map
+++ b/lib/libgcc_s/Version.map
@@ -126,6 +126,12 @@ GCC_3.4.4 {
        __subvti3;
 } GCC_3.4.2;
 
+GCC_3.5 {
+       __aeabi_unwind_cpp_pr0;
+       __aeabi_unwind_cpp_pr1;
+       __aeabi_unwind_cpp_pr2;
+};
+
 GCC_4.0.0 {
        __divdc3;
        __divsc3;
Comment 1 Dimitry Andric freebsd_committer 2019-05-21 19:29:50 UTC
Ed, I'm OK with the version map changes, but I don't see where struct _Unwind_Exception comes from normally?  The continueUnwind function just below your change also uses a _Unwind_Exception pointer, and it seems to compile just fine on other platforms... 

I see _Unwind_Exception is declared in unwind-arm.h and unwind-itanium.h, but the former is not included in case of arm, for some reason?

Similar for _Unwind_State, this is declared in unwind-arm.h, so it clearly includes that file, if there's a conflict?
Comment 2 Ed Maste freebsd_committer 2019-07-29 17:20:42 UTC
(In reply to Dimitry Andric from comment #1)

The version map changes need a slight change, we want to include them only on arm. I think I need to:

- rename libgcc_s Version.map to Symbol.map, add a Symbol.arm.map
- In Makefile set SYMBOL_MAPS=Symbol.map, and +=Symbol.arm.map on arm only
Comment 3 Ed Maste freebsd_committer 2019-11-07 15:04:21 UTC
cem changed in r354348
Comment 4 Conrad Meyer freebsd_committer 2019-11-07 15:18:15 UTC
Ah, sorry for duplicating work.  I wasn't aware of this PR.

Yeah, there were a couple problems here.  The unwind-ehabi-helpers.h file just produced conflicts with existing definitions.  There are at least two different variants of unwind.h in our tree, and different portions gcc_personality_v0 build with different headers.

Like comment 2 suggests, I added the extra GCC 3.5 symbols to ARM only using Symbol.map.  And as the description suggests, we needed those 3 other symbols as well.  No one has complained yet, afaik, but I haven't checked my email yet this morning.

I think we can drop 'arm' from the Summary and resolve this, given it's on the chopping block for 2020.
Comment 5 Conrad Meyer freebsd_committer 2019-11-07 15:18:55 UTC
(In reply to Ed Maste from comment #3)
r354347 and r354348, fwiw.  The former is the bulk of the work.
Comment 6 Ed Maste freebsd_committer 2019-11-07 15:43:50 UTC
(In reply to Conrad Meyer from comment #4)
No worries, if you check out the dependency tree for PR 233094 you can see PRs for similar issues that you may or may not have an interest in looking at:

https://bugs.freebsd.org/bugzilla/showdependencytree.cgi?id=233094&hide_resolved=1
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2019-11-08 03:45:06 UTC
^Triage: Assign to committer that resolved (in base r354348). Assume no MFC (not referenced in commit message)
Comment 8 Ed Maste freebsd_committer 2019-11-08 16:15:17 UTC
> Assume no MFC

Indeed this would likely not be MFC'd
Comment 9 Conrad Meyer freebsd_committer 2019-12-12 04:51:43 UTC
Reopening at Michal's request.  (See base r355646.)
Comment 10 Conrad Meyer freebsd_committer 2019-12-12 04:55:31 UTC
Ed, do we need to add a note about dropping ARM6 and 7 for 13.0 in arch.7?
Comment 11 Warner Losh freebsd_committer 2019-12-12 10:35:51 UTC
(In reply to Conrad Meyer from comment #10)
absolutely not. armv6/7 will be in 13.0.
Comment 12 Conrad Meyer freebsd_committer 2019-12-12 16:48:11 UTC
(In reply to Warner Losh from comment #11)
If no one is willing to do the work, it doesn’t seem much better than keeping Sparc.
Comment 13 Warner Losh freebsd_committer 2019-12-12 18:05:11 UTC
(In reply to Conrad Meyer from comment #12)

Oh for FA*@$# sake. it's been one whole day. Let's not jump the gun here, shall we?
Comment 14 Conrad Meyer freebsd_committer 2019-12-12 18:37:18 UTC
Sure, I don't have a position on it.  It was genuinely a question.  The pro-ARM thought for getting it in arch.7 early is that it broadens awareness to maybe someone who has time and interest to take up the work before 13.0, and that timeframe is coming up quickly so the sooner we can find a motivated party, the better.  I don't feel strongly about it either way; my motivation around my prior work in this area was mostly just about having the libunwind API available on a broader set of FreeBSD archs.  llvm-libunwind (via libgcc_s) provides that API, while gcc4's libgcc does not.
Comment 15 commit-hook freebsd_committer 2019-12-16 14:09:21 UTC
A commit references this bug:

Author: mmel
Date: Mon Dec 16 14:08:50 UTC 2019
New revision: 355803
URL: https://svnweb.freebsd.org/changeset/base/355803

Log:
  Fix LLVM libunwnwind _Unwind_Backtrace symbol version for ARM.
  In original  GNU libgcc, _Unwind_Backtrace is published with GCC_3.3 version
  for all architectures but ARM. For ARM should be publishes with GCC_4.3.0
  version. This was originally omitted in r255095, fixed in r318024 and omitted
  aging in LLVM libunwind implementation in r354347.

  For ARM _Unwind_Backtrace should be published as default with GCC_4.3.0
  version , (because this is right original version) and again as
  normal(not-default) with GCC_3.3 version (to maintain ABI compatibility
  compiled/linked with wrong pre r318024 libgcc)

  PR:	233664

Changes:
  head/contrib/libunwind/src/UnwindLevel1-gcc-ext.c
  head/lib/libgcc_s/Makefile
  head/lib/libgcc_s/Symbol.map
  head/lib/libgcc_s/SymbolDefault.map
  head/lib/libgcc_s/arm/Symbol.map
Comment 16 Ed Maste freebsd_committer 2020-01-03 21:14:49 UTC
FYI LLVM libunwind is currently used for all FreeBSD CPU architectures except armv6, armv7, and sparc64.

Do we have a list of known issues affecting armv6/armv7?
Comment 17 Michal Meloun freebsd_committer 2020-01-06 13:51:34 UTC
Testing took me more time than I expected and also had much less free time than planned at Christmas. But the good news is that I can't reproduce any of the previously logged bugs with current sources.
So I switch back to the compiler-rt libunwind version asap.
Comment 18 Ed Maste freebsd_committer 2020-01-07 18:27:22 UTC
(In reply to Michal Meloun from comment #17)
Excellent, thank you!
Comment 19 Conrad Meyer freebsd_committer 2020-01-08 01:24:36 UTC
Next time, I'd appreciate it if you first confirmed there was actually any bug before telling me something is "unusable" and "broken." 🤦

In hindsight, the fallacious claim that the arm64 bits didn't compile (at a time when llvm-libunwind was compiling by default on arm64) is actually humorous, but we can do better.
Comment 20 commit-hook freebsd_committer 2020-01-08 07:26:27 UTC
A commit references this bug:

Author: mmel
Date: Wed Jan  8 07:25:37 UTC 2020
New revision: 356483
URL: https://svnweb.freebsd.org/changeset/base/356483

Log:
  Switch 32-bit arm back to LLVM libunwind.
  Actual LLVM libunwind passed all testing without issues, switch back to it.

  PR:	233664

Changes:
  head/share/mk/src.opts.mk
Comment 21 Michal Meloun freebsd_committer 2020-01-08 08:49:51 UTC
(In reply to Conrad Meyer from comment #19)
Well, the reality is that your commit broke ABI and many ports (rustc for example but not only)  for armv7 (and the following made it worse). So it was "unusable" and "broken". Using your words, next time I would appreciate you to test your commits properly.
From arm64 case, I'm afraid I mistaken the llvm implementation of libunwind with that from devel/libunwind. libunwind from this port will compiles itself, but many dependent ports will not. It exposes gcc-specific macros (a local variable in a named register) that is not used when compiling the library itself, but used by other parties. Did this looks like a humor for you?

Be honest, we are both people and we both have the right to make a mistake from time to time. (and the truth is, as my age grows, "from time to time" gets shorter and shorter).

I'm closing this bug.