This is an archive of the discontinued LLVM Phabricator instance.

Differential D118320

[libunwind] build fix for GCC on PPC32
ClosedPublic

Authored by thesamesam on Jan 26 2022, 11:36 PM.

Details

Reviewers
MaskRay
mgorny
Group Reviewers
Restricted Project
Commits
rGcd20e579df07: [unwind] fix build with GCC on PPC32
Summary

Originally reported downstream in Gentoo here.

/var/tmp/portage/sys-libs/llvm-libunwind-13.0.0/work/libunwind/src/libunwind.cpp:77:3: error: #error Architecture not supported
   77 | # error Architecture not supported
      |   ^~~~~
[...]
/var/tmp/portage/sys-libs/llvm-libunwind-13.0.0/work/libunwind/src/libunwind.cpp: In function ‘int __unw_init_local(unw_cursor_t*, unw_context_t*)’:
/var/tmp/portage/sys-libs/llvm-libunwind-13.0.0/work/libunwind/src/libunwind.cpp:80:57: error: ‘REGISTER_KIND’ was not declared in this scope
   80 |   new (reinterpret_cast<UnwindCursor<LocalAddressSpace, REGISTER_KIND> *>(cursor))
      |                                                         ^~~~~~~~~~~~~
[...]

PPC is actually a supported architecture, but GCC (tested with 11.2.0) on powerpc32 seems to only define:
__PPC__, _ARCH_PPC, __PPC, __powerpc and not __ppc__.

This instead uses __powerpc__ which should be around on PPC32 and PPC64 (but we check it after PPC64, so it's fine).

Diff Detail

Event Timeline

thesamesam created this revision.Jan 26 2022, 11:36 PM
Herald added subscribers: steven.zhang, shchenz. · View Herald TranscriptJan 26 2022, 11:36 PM
thesamesam requested review of this revision.Jan 26 2022, 11:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2022, 11:36 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B145928: Diff 403512.Jan 26 2022, 11:36 PM
thesamesam updated this revision to Diff 403515.Jan 26 2022, 11:46 PM
Arfrever added a subscriber: Arfrever.Jan 27 2022, 12:20 AM
Arfrever added inline comments.
libunwind/include/__libunwind_config.h
58

It would probably suffice:

# elif defined(__powerpc__)

Similarly in other places.

libunwind/src/assembly.h
219
#if defined(__powerpc__) || defined(__powerpc64__)
MaskRay added a comment.Jan 27 2022, 12:48 AM

In the future, please include more context in the diff :)

https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface "To make reviews easier, please always include as much context as possible with your diff!"

libunwind/include/__libunwind_config.h
58

__powerpc__ sounds good.

Both ppc32 and ppc64 define __powerpc__ and __PPC__. Can you also check whether this works for powerpc32?

MaskRay retitled this revision from llvm-libunwind build fix for GCC on PPC32 to [libunwind] build fix for GCC on PPC32.EditedJan 27 2022, 12:49 AM

__PPC__, _ARCH_PPC, __PPC, __powerpc and not __ppc__.

WoW, this looks like a Clang problem...

In GCC, __ppc__ is defined for FreeBSD/Darwin/etc but not Linux. Clang makes it apply to other systems.

Harbormaster completed remote builds in B145929: Diff 403515.Jan 27 2022, 8:42 AM
thesamesam updated this revision to Diff 403752.Jan 27 2022, 12:17 PM
thesamesam edited the summary of this revision. (Show Details)
thesamesam marked 3 inline comments as done.
MaskRay accepted this revision.Jan 27 2022, 12:22 PM

Thanks!

This revision is now accepted and ready to land.Jan 27 2022, 12:22 PM
thesamesam updated this revision to Diff 403755.Jan 27 2022, 12:30 PM
Harbormaster completed remote builds in B146105: Diff 403755.Jan 27 2022, 12:31 PM
mgorny accepted this revision.Jan 27 2022, 1:13 PM
mgorny added a subscriber: mgorny.

LGTM. Thanks.

This revision was landed with ongoing or failed builds.Jan 27 2022, 2:05 PM
Closed by commit rGcd20e579df07: [unwind] fix build with GCC on PPC32 (authored by thesamesam, committed by mgorny). · Explain Why
This revision was automatically updated to reflect the committed changes.
mgorny added a commit: rGcd20e579df07: [unwind] fix build with GCC on PPC32.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 2:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
ldionne added a subscriber: ldionne.Jan 27 2022, 2:50 PM

@mgorny @MaskRay Please consider adding yourselves to the libunwind review groups if you aren't, and accepting as the group when a patch can be shipped. There would be a lot of value in having folks "own" libunwind more officially. Cheers and thanks for reviewing.

MaskRay added a comment.Jan 27 2022, 3:00 PM
In D118320#3277666, @ldionne wrote:

@mgorny @MaskRay Please consider adding yourselves to the libunwind review groups if you aren't, and accepting as the group when a patch can be shipped. There would be a lot of value in having folks "own" libunwind more officially. Cheers and thanks for reviewing.

Thanks. I am in #libunwind:) I have should added #libunwind when I saw this.

Arfrever mentioned this in D119068: [libunwind] Define _Unwind_Backtrace for powerpc, sparc.Feb 5 2022, 10:39 PM
MaskRay mentioned this in rG022011078054: [libunwind] Define _Unwind_Backtrace for powerpc, sparc.Feb 7 2022, 9:37 AM

Revision Contents

PathSize
libunwind/
include/
2 lines
src/
2 lines
2 lines
2 lines
2 lines
2 lines

Diff 403515

libunwind/include/__libunwind_config.h

Context not available.
# define _LIBUNWIND_CONTEXT_SIZE 167 # define _LIBUNWIND_CONTEXT_SIZE 167
# define _LIBUNWIND_CURSOR_SIZE 179 # define _LIBUNWIND_CURSOR_SIZE 179
# define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_PPC64 # define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_PPC64
# elif defined(__ppc__) # elif defined(__ppc__) || defined(__PPC__)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-# elif defined(__ppc__) || defined(__PPC__)
+#elif defined(__ppc__) || defined(__PPC__)
Lint: Pre-merge checks: clang-format: please reformat the code ``` -# elif defined(__ppc__) || defined(__PPC__) +#elif…
ArfreverUnsubmitted
Done
ReplyInline Actions

It would probably suffice:

# elif defined(__powerpc__)

Similarly in other places.

Arfrever: It would probably suffice: ``` # elif defined(__powerpc__) ``` Similarly in other places.
MaskRayUnsubmitted
Done
ReplyInline Actions

__powerpc__ sounds good.

Both ppc32 and ppc64 define __powerpc__ and __PPC__. Can you also check whether this works for powerpc32?

MaskRay: `__powerpc__` sounds good. Both ppc32 and ppc64 define `__powerpc__` and `__PPC__`. Can you…
# define _LIBUNWIND_TARGET_PPC 1 # define _LIBUNWIND_TARGET_PPC 1
# define _LIBUNWIND_CONTEXT_SIZE 117 # define _LIBUNWIND_CONTEXT_SIZE 117
# define _LIBUNWIND_CURSOR_SIZE 124 # define _LIBUNWIND_CURSOR_SIZE 124
Context not available.

libunwind/src/UnwindRegistersRestore.S

Context not available.
PPC64_LR(3) PPC64_LR(3)
bctr bctr
#elif defined(__ppc__) #elif defined(__ppc__) || defined(__PPC__)
DEFINE_LIBUNWIND_FUNCTION(_ZN9libunwind13Registers_ppc6jumptoEv) DEFINE_LIBUNWIND_FUNCTION(_ZN9libunwind13Registers_ppc6jumptoEv)
// //
Context not available.

libunwind/src/UnwindRegistersSave.S

Context not available.
blr blr
#elif defined(__ppc__) #elif defined(__ppc__) || defined(__PPC__)
// //
// extern int unw_getcontext(unw_context_t* thread_state) // extern int unw_getcontext(unw_context_t* thread_state)
Context not available.

libunwind/src/assembly.h

Context not available.
#endif #endif
#endif /* __arm__ */ #endif /* __arm__ */
#if defined(__ppc__) || defined(__powerpc64__) #if defined(__ppc__) || defined(__PPC__) || defined(__powerpc64__)
ArfreverUnsubmitted
Done
ReplyInline Actions
#if defined(__powerpc__) || defined(__powerpc64__)
Arfrever: ``` #if defined(__powerpc__) || defined(__powerpc64__) ```
#define PPC_LEFT_SHIFT(index) << (index) #define PPC_LEFT_SHIFT(index) << (index)
#endif #endif
Context not available.

libunwind/src/config.h

Context not available.
#define _LIBUNWIND_BUILD_SJLJ_APIS #define _LIBUNWIND_BUILD_SJLJ_APIS
#endif #endif
#if defined(__i386__) || defined(__x86_64__) || defined(__ppc__) || defined(__ppc64__) || defined(__powerpc64__) #if defined(__i386__) || defined(__x86_64__) || defined(__ppc__) || defined(__PPC__) || defined(__ppc64__) || defined(__powerpc64__)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-#if defined(__i386__) || defined(__x86_64__) || defined(__ppc__) || defined(__PPC__) || defined(__ppc64__) || defined(__powerpc64__)
+#if defined(__i386__) || defined(__x86_64__) || defined(__ppc__) ||            \
+    defined(__PPC__) || defined(__ppc64__) || defined(__powerpc64__)
Lint: Pre-merge checks: clang-format: please reformat the code ``` -#if defined(__i386__) || defined(__x86_64__) ||…
#define _LIBUNWIND_SUPPORT_FRAME_APIS #define _LIBUNWIND_SUPPORT_FRAME_APIS
#endif #endif
Context not available.

libunwind/src/libunwind.cpp

Context not available.
# define REGISTER_KIND Registers_x86_64 # define REGISTER_KIND Registers_x86_64
#elif defined(__powerpc64__) #elif defined(__powerpc64__)
# define REGISTER_KIND Registers_ppc64 # define REGISTER_KIND Registers_ppc64
#elif defined(__ppc__) #elif defined(__ppc__) || defined(__PPC__)
# define REGISTER_KIND Registers_ppc # define REGISTER_KIND Registers_ppc
#elif defined(__aarch64__) #elif defined(__aarch64__)
# define REGISTER_KIND Registers_arm64 # define REGISTER_KIND Registers_arm64
Context not available.