Page MenuHomePhabricator

[libunwind] [sparc] Add SPARCv9 support
ClosedPublic

Authored by koakuma on Jan 8 2022, 12:16 AM.

Details

Summary

Adds libunwind support for SPARCv9 (aka sparc64). This is a rebase of @kettenis' patch D32450, which I created (with his permission) because the original review has become inactive.
The changes are of a cosmetic nature to make it fit better with the new code style, and to reuse the existing SPARCv8 code, whenever possible.

Please let me know if I posted this on the wrong place. Also, the summary of the original review is reproduced below:

This adds unwinder support for 64-bit SPARC (aka SPARCv9). The implementation was done on OpenBSD/sparc64, so it takes StackGhost into account:

https://www.usenix.org/legacy/publications/library/proceedings/sec01/full_papers/frantzen/frantzen_html/index.html

Since StackGhost xor's return addresses with a random cookie before storing them on the stack, the unwinder has to do some extra work to recover those. This is done by introducing a new kRegisterInCFADecrypt "location" type that is used to implement the DW_CFA_GNU_window_save opcode. That implementation is SPARC-specific, but should work for 32-bit SPARC as well. DW_CFA_GNU_window_save is only ever generated on SPARC as far as I know.

Diff Detail

Event Timeline

koakuma created this revision.Jan 8 2022, 12:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2022, 12:16 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
koakuma requested review of this revision.Jan 8 2022, 12:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2022, 12:16 AM

I have a question about the code formatting suggestions. I worked on this following the original code's style (indentation, C-style casts, etc) but it looks like clang-format doesn't like it.
Should I revise it following clang-format's suggestion, or is there a more proper way to go with it?

brad added a subscriber: brad.Jan 10 2022, 7:50 AM

I have a question about the code formatting suggestions. I worked on this following the original code's style (indentation, C-style casts, etc) but it looks like clang-format doesn't like it.
Should I revise it following clang-format's suggestion, or is there a more proper way to go with it?

Just ignore it. The clang-format diagnostic is more useful for files conforming to the style or new files. For existing files violating it, it is sometimes better to leave it.

libunwind/src/libunwind.cpp
70

__sparc64__ or __sparcv9__?

Just ignore it. The clang-format diagnostic is more useful for files conforming to the style or new files. For existing files violating it, it is sometimes better to leave it.

I see, thanks.

libunwind/src/libunwind.cpp
70

__sparc64__ and __sparcv9__ are clang-only, it seems. GCC doesn't define any of it.

koakuma updated this revision to Diff 399660.Jan 13 2022, 6:28 AM

This fixes the wrong #endif placement in DwarfParser.hpp that causes AArch64 build to fail.

kettenis added inline comments.Jan 13 2022, 8:16 AM
libunwind/src/libunwind.cpp
70

Correct. IIRC defined(sparc) && defined(arch64) is what was used by Sun when their own compiler was still a thing and as far as I know all open source compilers that support 64-bit SPARC define those symbols.

kettenis added inline comments.Jan 13 2022, 8:36 AM
libunwind/src/libunwind.cpp
70

And to be clear, I meant

defined(__sparc__) && defined(__arch64__)

there.

Ping. Is there anything else that needs to be done?

Thanks for doing the work on this and @kettenis for working on it originally too. I'd love to see this landed.

I've tested this on sparc64 on Gentoo and it doesn't seem to build for me right now at commit bddc814b442ae9f30d62e2f881274d6255411225.

I've attached the build log:

The failure looks like:

FAILED: src/CMakeFiles/unwind_shared.dir/UnwindRegistersSave.S.o
/usr/bin/sparc64-unknown-linux-gnu-gcc -Dunwind_shared_EXPORTS -I/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/include  -O2 -mcpu=ultrasparc -pipe -fPIC -Werror=return-type -W -Wall -W
char-subscripts -Wconversion -Wmismatched-tags -Wmissing-braces -Wno-unused-function -Wshadow -Wsign-compare -Wsign-conversion -Wstrict-aliasing=2 -Wstrict-overflow=4 -Wunused-parameter -Wunused-variable -W
write-strings -Wundef -Wno-suggest-override -Wno-error -pedantic -funwind-tables -nostdinc++ -DNDEBUG -fno-rtti -MD -MT src/CMakeFiles/unwind_shared.dir/UnwindRegistersSave.S.o -MF src/CMakeFiles/unwind_sha
red.dir/UnwindRegistersSave.S.o.d -o src/CMakeFiles/unwind_shared.dir/UnwindRegistersSave.S.o -c /var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1010:43: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
 1010 | DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
      |                                           ^
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1010:43: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1010:43: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1010:43: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1167:46: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
 1167 |   WEAK_ALIAS(__unw_getcontext, unw_getcontext)
      |                                              ^
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1167:46: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1167:46: warning: invoking macro GLUE2 argument 1: empty macro arguments are undefined in ISO C90 [-Wpedantic]
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S: Assembler messages:
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1012: Error: detected global register use not covered by .register pseudo-op
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1012: Error: detected global register use not covered by .register pseudo-op
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1012: Error: detected global register use not covered by .register pseudo-op
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1013: Error: detected global register use not covered by .register pseudo-op
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1013: Error: detected global register use not covered by .register pseudo-op
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindRegistersSave.S:1013: Error: detected global register use not covered by .register pseudo-op

(I did try on 13.0.0 too but that was just out of interest and it applied with a bunch of fuzz and ended up getting a dodgy double-case error).

Let me know if I can give any other information.

koakuma updated this revision to Diff 403973.Jan 28 2022, 5:03 AM
koakuma set the repository for this revision to rG LLVM Github Monorepo.

Add .register directive for %g2, %g3, %g6, and %g7 as required by the assembly language.

Ref: SPARC Assembly Language Reference Manual, section 3.3
(https://docs.oracle.com/cd/E37838_01/html/E61063/pseudoops-1.html)

Thanks for doing the work on this and @kettenis for working on it originally too. I'd love to see this landed.

I've tested this on sparc64 on Gentoo and it doesn't seem to build for me right now at commit bddc814b442ae9f30d62e2f881274d6255411225.

Seems like this is because I forgot to add the .register directives before saving %g2, %g3, %g6, and %g7, and GCC doesn't like it.
I've added the directives, can you please check if it builds there too? Thanks a lot!

thesamesam added a comment.EditedJan 28 2022, 8:46 PM

Thanks for doing the work on this and @kettenis for working on it originally too. I'd love to see this landed.

I've tested this on sparc64 on Gentoo and it doesn't seem to build for me right now at commit bddc814b442ae9f30d62e2f881274d6255411225.

Seems like this is because I forgot to add the .register directives before saving %g2, %g3, %g6, and %g7, and GCC doesn't like it.
I've added the directives, can you please check if it builds there too? Thanks a lot!

Wow, thanks for the quick reply!

I think this is progress, only two errors:

/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/UnwindCursor.hpp:1927:6:   required from here
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/DwarfParser.hpp:774:9: error: duplicate case value
  774 |         case REGISTERS_SPARC:
      |         ^~~~
/var/tmp/portage/sys-libs/llvm-libunwind-14.0.0.9999/work/libunwind/src/DwarfParser.hpp:756:9: note: previously used here
  756 |         case REGISTERS_SPARC:
      |         ^~~~

I've attached the full log again:

.

Apparently I have both _LIBUNWIND_TARGET_SPARC and _LIBUNWIND_TARGET_SPARC64 set?

Apparently I have both _LIBUNWIND_TARGET_SPARC and _LIBUNWIND_TARGET_SPARC64 set?

This happens when libunwind is configured with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON passed to cmake.
Then https://github.com/llvm/llvm-project/blob/8faf2a0638d3e8d9411d55c3e5dfb7b452f35ca2/libunwind/CMakeLists.txt#L325-L328 does NOT enable _LIBUNWIND_IS_NATIVE_ONLY.
Then #else // !_LIBUNWIND_IS_NATIVE_ONLY block in libunwind/include/__libunwind_config.h is enabled and it contains (with changes from this patchset):

# define _LIBUNWIND_TARGET_SPARC 1
# define _LIBUNWIND_TARGET_SPARC64 1
koakuma updated this revision to Diff 404435.Jan 31 2022, 12:42 AM

Distinguish REGISTERS_SPARC from REGISTERS_SPARC64 to make building with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON possible.

thesamesam accepted this revision.Jan 31 2022, 12:28 PM

Thank you for your work and responsiveness!

-- Testing: 11 tests, 11 workers --
UNSUPPORTED: llvm-libunwind-shared.cfg.in :: floatregister.pass.cpp (1 of 11)
UNSUPPORTED: llvm-libunwind-shared.cfg.in :: signal_unwind.pass.cpp (2 of 11)
UNSUPPORTED: llvm-libunwind-shared.cfg.in :: remember_state_leak.pass.sh.s (3 of 11)
UNSUPPORTED: llvm-libunwind-shared.cfg.in :: unwind_leaffunction.pass.cpp (4 of 11)
PASS: llvm-libunwind-shared.cfg.in :: alignment.compile.pass.cpp (5 of 11)
PASS: llvm-libunwind-shared.cfg.in :: frameheadercache_test.pass.cpp (6 of 11)
PASS: llvm-libunwind-shared.cfg.in :: libunwind_02.pass.cpp (7 of 11)
PASS: llvm-libunwind-shared.cfg.in :: unw_getcontext.pass.cpp (8 of 11)
PASS: llvm-libunwind-shared.cfg.in :: signal_frame.pass.cpp (9 of 11)
PASS: llvm-libunwind-shared.cfg.in :: libunwind_01.pass.cpp (10 of 11)
PASS: llvm-libunwind-shared.cfg.in :: forceunwind.pass.cpp (11 of 11)

Testing Time: 0.35s
  Unsupported: 4
  Passed     : 7
>>> Completed testing sys-libs/llvm-libunwind-14.0.0.9999
MaskRay added a comment.EditedJan 31 2022, 3:12 PM

Apparently I have both _LIBUNWIND_TARGET_SPARC and _LIBUNWIND_TARGET_SPARC64 set?

This happens when libunwind is configured with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON passed to cmake.
Then https://github.com/llvm/llvm-project/blob/8faf2a0638d3e8d9411d55c3e5dfb7b452f35ca2/libunwind/CMakeLists.txt#L325-L328 does NOT enable _LIBUNWIND_IS_NATIVE_ONLY.
Then #else // !_LIBUNWIND_IS_NATIVE_ONLY block in libunwind/include/__libunwind_config.h is enabled and it contains (with changes from this patchset):

# define _LIBUNWIND_TARGET_SPARC 1
# define _LIBUNWIND_TARGET_SPARC64 1

Would it be clearer to have _LIBUNWIND_TARGET_SPARC32 and _LIBUNWIND_TARGET_SPARC64 to indicate 32-bit and 64-bit variants?

You can define _LIBUNWIND_TARGET_SPARC to mean either variant.

libunwind/src/DwarfInstructions.hpp
283
libunwind/src/DwarfParser.hpp
74
MaskRay added inline comments.Jan 31 2022, 3:17 PM
libunwind/src/DwarfInstructions.hpp
93

// Sparc-specific.

libunwind/src/Registers.hpp
91

// The Sparc-specific kRegisterInCFADecrypt uses getCookie. We define getWCookie to a dummy value for other targets.

3623
3624
3637

Remove

3654
3668
koakuma updated this revision to Diff 404779.Jan 31 2022, 6:45 PM

Some formatting changes as per @MaskRay and clang-format suggestions.

Would it be possible to simplify defined(_LIBUNWIND_TARGET_SPARC) || defined(_LIBUNWIND_TARGET_SPARC64) with defined(_LIBUNWIND_TARGET_SPARC) and let _LIBUNWIND_TARGET_SPARC mean either the 32-bit or 64-bit variant.

libunwind/src/DwarfInstructions.hpp
94

Perhaps guard the getWCookie() usage with the sparc macro here so that other targets don't have to define getWCookie. The comment can then be moved here.

Then you may remove // sparc64 specific.

libunwind/src/Registers.hpp
3636

Delete the ctor. _wcookie has been initialized.

Would it be clearer to have _LIBUNWIND_TARGET_SPARC32 and _LIBUNWIND_TARGET_SPARC64 to indicate 32-bit and 64-bit variants?

You can define _LIBUNWIND_TARGET_SPARC to mean either variant.

From what I see in other parts of LLVM, SPARC is also generally taken to mean the 32-bit variant of the architecture. Would it be okay to change it here?
In any case, though, I feel like it's better to be done in another changeset if it's really needed to be changed.

libunwind/src/Registers.hpp
3668

This one's also doing an early return in addition to setting the value, so I don't think I can remove the braces here.

koakuma added inline comments.Jan 31 2022, 8:39 PM
libunwind/src/DwarfInstructions.hpp
94

From my understanding, it wouldn't work when building with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON.
In that case, the sparc64 target (which has getWCookie) and other targets (which doesn't have it) will be built together, and the build will fail because this line tries to call getWCookie on other targets' objects.

koakuma updated this revision to Diff 404875.Feb 1 2022, 5:04 AM

Use default constructor for Registers_sparc64.

MaskRay accepted this revision.Feb 1 2022, 5:07 PM

LGTM.

This revision is now accepted and ready to land.Feb 1 2022, 5:07 PM
Arfrever added inline comments.Feb 1 2022, 9:45 PM
libunwind/src/Registers.hpp
3612

Formatting of this getSP() method is not aligned with formatting of other methods in this class.

3633

This line should be sizeof(_wcookie));

koakuma added inline comments.Feb 2 2022, 7:27 AM
libunwind/src/Registers.hpp
3612

I think getSP() until getWCookie() has been aligned properly.

Or do you mean that I should align those with getArch() and the methods above it? Currently I'm just following the formatting of other register classes where getSP() onwards have different alignment than previous methods.

koakuma updated this revision to Diff 405266.Feb 2 2022, 7:28 AM

Follow @Arfrever & clang-format's suggestions.

Arfrever added inline comments.Feb 2 2022, 6:45 PM
libunwind/src/Registers.hpp
3612

OK. The long "Lint: Pre-merge checks" comment made it so that I did not see that this line is aligned with lines below.
I withdraw this one suggestion.

Arfrever accepted this revision.Feb 2 2022, 6:50 PM
koakuma added a comment.EditedFeb 3 2022, 6:52 AM

Um... I don't have commit access, so can someone please help committing it for me? My email is koachan@protonmail.com.
Thanks a lot for the help.

EDIT: Oh, and since this patch was originally made by @kettenis, I think he should be credited too?
In any case, thanks a lot, everyone (especially Mr. @kettenis for originally writing this patchset).

Um... I don't have commit access, so can someone please help committing it for me? My email is koachan@protonmail.com.
Thanks a lot for the help.

EDIT: Oh, and since this patch was originally made by @kettenis, I think he should be credited too?
In any case, thanks a lot, everyone (especially Mr. @kettenis for originally writing this patchset).

Co-authored-by: Mark Kettenis

git commit --amend --author='Koakuma <koachan@protonmail.com>'

Will this be fine?

Co-authored-by: Mark Kettenis

git commit --amend --author='Koakuma <koachan@protonmail.com>'

Will this be fine?

Yeah, that should be okay, I think.

I still think there should be a better way handling registers.getWCookie(). Can getWCookie be defined only for Sparc?

MaskRay updated this revision to Diff 406207.Feb 5 2022, 12:59 PM
  • clang-format
  • use sfinae to avoid defining getWCookie
  • fix sign conversion warnings
MaskRay updated this revision to Diff 406208.Feb 5 2022, 1:06 PM
  • properly format lines
  • remove unneeded cast for memcpy
This revision was landed with ongoing or failed builds.Feb 5 2022, 1:08 PM
This revision was automatically updated to reflect the committed changes.