This is an archive of the discontinued LLVM Phabricator instance.

[Sparc] Add Sparc V8 support
ClosedPublic

Authored by dcederman on Dec 17 2018, 5:41 AM.

Details

Summary

Adds the register class implementation for Sparc.
Adds support for DW_CFA_GNU_window_save.
Adds save and restore context functionality.

On Sparc the return address is the address of the call instruction, so an offset needs to be added when returning to skip the call instruction and its delay slot. If the function returns a struct it is also necessary to skip one extra instruction.

Diff Detail

Event Timeline

dcederman created this revision.Dec 17 2018, 5:41 AM

In general, this looks reasonable. No being a sparc expert, I don't know if it's correct, but it fits in with the existing libunwind structure.

src/DwarfParser.hpp
687

Should this be wrapped in #ifdef _LIBUNWIND_TARGET_SPARC ?

dcederman added inline comments.Dec 17 2018, 7:17 AM
src/DwarfParser.hpp
687

Yes, it probably should. I will fix that.

dcederman updated this revision to Diff 178460.Dec 17 2018, 7:19 AM

Only include DW_CFA_GNU_window_save support when compiling for Sparc.

mstorsjo added inline comments.Dec 17 2018, 9:56 AM
src/UnwindLevel1.c
495 ↗(On Diff #178460)

How widely is this builtin available, e.g. on older versions of clang or in gcc?

dcederman marked an inline comment as done.Dec 18 2018, 1:14 AM
dcederman added inline comments.
src/UnwindLevel1.c
495 ↗(On Diff #178460)

Looks like it has been available since 1997 for GCC and 2009 for Clang. But looking into it I think it would be better to follow the same approach as aarch64 and modify the return address in stepWithDwarf instead, without using the builtin. That would also allow for better handling of functions that returns structs.

dcederman updated this revision to Diff 178621.Dec 18 2018, 1:21 AM
dcederman edited the summary of this revision. (Show Details)

This update does not use __builtin_extract_return_addr in _Unwind_GetIP. Instead it modifies the return address in stepWithDwarf, similar to aarch64. It also checks if the instruction at the return address is an unimp instruction. If it is, that means that the function is returning a struct and that we should skip one extra instruction.

I have been using this Sparc version of libunwind when running the libc++ testsuite and have not had any problems related to libunwind. Does anyone have any additional comments or is it ok to commit?

compnerd accepted this revision.Dec 27 2018, 12:07 PM
compnerd added inline comments.
src/DwarfParser.hpp
701

Could you put this before the execution of the op please?

src/Registers.hpp
3339

I guess as long as we don't care about async exceptions where the %o7 is being twiddled around for a call or being used as a temporary, this should be okay (IIRC, %o7 needs to be saved before call on SPARC).

3374

Spurious empty line.

3389

Spurious empty line

src/UnwindRegistersRestore.S
1023

Indentation is off

src/UnwindRegistersSave.S
974

Indentation is off

This revision is now accepted and ready to land.Dec 27 2018, 12:07 PM

If Saleem is happy with this, then I'm good too.

dcederman marked an inline comment as done.Jan 8 2019, 12:17 AM

Thanks for the review!

src/DwarfParser.hpp
701

Sure. But for the other ops the trace printout is placed after the op, so this would break that pattern?

src/UnwindRegistersRestore.S
1023

The extra space is a convention used when writing Sparc assembly to indicate that the instruction is in the delay slot of the previous instruction.

dcederman updated this revision to Diff 180624.Jan 8 2019, 12:19 AM

Removed empty lines and moved the trace message to before the operation.

This revision was automatically updated to reflect the committed changes.
mgorny added a subscriber: mgorny.Jan 9 2019, 7:29 AM

This commit is causing regression when building with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON:

FAILED: src/CMakeFiles/unwind_objects.dir/libunwind.cpp.o
/usr/bin/c++  -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I../include -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -w -fdiagnostics-color -ffunction-sections -fdata-sections -std=c++11 -fPIC   -Werror=return-type -W -Wall -Wchar-subscripts -Wconversion -Wmissing-braces -Wno-unused-function -Wshadow -Wsign-compare -Wsign-conversion -Wstrict-aliasing=2 -Wstrict-overflow=4 -Wunused-parameter -Wunused-variable -Wwrite-strings -Wundef -Wno-error -pedantic -D_DEBUG -fno-exceptions -funwind-tables  -fstrict-aliasing -fno-rtti -MD -MT src/CMakeFiles/unwind_objects.dir/libunwind.cpp.o -MF src/CMakeFiles/unwind_objects.dir/libunwind.cpp.o.d -o src/CMakeFiles/unwind_objects.dir/libunwind.cpp.o -c ../src/libunwind.cpp
In file included from ../src/EHHeaderParser.hpp:18:0,
                 from ../src/AddressSpace.hpp:43,
                 from ../src/libunwind.cpp:28:
../src/DwarfParser.hpp: In instantiation of 'static bool libunwind::CFI_Parser<A>::parseInstructions(A&, libunwind::CFI_Parser<A>::pint_t, libunwind::CFI_Parser<A>::pint_t, const libunwind::CFI_Parser<A>::CIE_Info&, libunwind::CFI_Parser<A>::pint_t, libunwind::CFI_Parser<A>::PrologInfoStackEntry*&, libunwind::CFI_Parser<A>::PrologInfo*) [with A = libunwind::LocalAddressSpace; libunwind::CFI_Parser<A>::pint_t = long unsigned int]':
../src/DwarfParser.hpp:363:27:   required from 'static bool libunwind::CFI_Parser<A>::parseFDEInstructions(A&, const libunwind::CFI_Parser<A>::FDE_Info&, const libunwind::CFI_Parser<A>::CIE_Info&, libunwind::CFI_Parser<A>::pint_t, libunwind::CFI_Parser<A>::PrologInfo*) [with A = libunwind::LocalAddressSpace; libunwind::CFI_Parser<A>::pint_t = long unsigned int]'
../src/UnwindCursor.hpp:1874:46:   required from 'void libunwind::UnwindCursor<A, R>::setInfoBasedOnIPRegister(bool) [with A = libunwind::LocalAddressSpace; R = libunwind::Registers_x86_64]'
../src/libunwind.cpp:407:1:   required from here
../src/DwarfParser.hpp:689:5: error: duplicate case value
     case DW_CFA_GNU_window_save:
     ^~~~
../src/DwarfParser.hpp:682:5: note: previously used here
     case DW_CFA_AARCH64_negate_ra_state:
     ^~~~
ninja: build stopped: subcommand failed.

This commit is causing regression when building with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON:

I think the patch should be reverted until this can be fixed -- as DW_CFA_GNU_window_save and DW_CFA_AARCH64_negate_ra_state are the same constant, the two cases will need to be merged, and disambiguated by the target architecture.

jgorbe added a subscriber: jgorbe.Jan 9 2019, 3:43 PM
jgorbe added a comment.Jan 9 2019, 5:13 PM

I reverted this patch on r350787

mgorny added a comment.Jan 9 2019, 7:36 PM

Thank you.

dcederman updated this revision to Diff 181033.Jan 10 2019, 5:13 AM

For me native support is enough, so I removed the Sparc define from the !_LIBUNWIND_IS_NATIVE_ONLY section. That should make it compile with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON.

I did not see an easy way to get information about the correct architecture in parseInstructions(). If someone has a suggestion on how to best provide this information (extend the Register class with a getArch() function?) I could try it out in a subsequent patch.

For me native support is enough, so I removed the Sparc define from the !_LIBUNWIND_IS_NATIVE_ONLY section. That should make it compile with -DLIBUNWIND_ENABLE_CROSS_UNWINDING=ON.

I did not see an easy way to get information about the correct architecture in parseInstructions(). If someone has a suggestion on how to best provide this information (extend the Register class with a getArch() function?) I could try it out in a subsequent patch.

I think an enum with a value for each Registers_* type, and a function in each such type to return its enum-value. Then pass that enum value as an arg to parseFDEInstructions.

dcederman updated this revision to Diff 181260.Jan 11 2019, 6:34 AM

Added functionality to be able to differ between DW_CFA_AARCH64_negate_ra_state and DW_CFA_GNU_window_save.

mgorny added inline comments.Jan 11 2019, 6:39 AM
src/DwarfParser.hpp
682

Could you add a static_assert that verifies that the constants indeed have the same value?

dcederman updated this revision to Diff 181262.Jan 11 2019, 6:56 AM

Added assert to verify that DW_CFA_AARCH64_negate_ra_state and DW_CFA_GNU_window_save have the same value.

dcederman reopened this revision.Jan 11 2019, 6:56 AM
This revision is now accepted and ready to land.Jan 11 2019, 6:56 AM
jyknight accepted this revision.Jan 11 2019, 7:20 AM
jyknight added inline comments.
src/DwarfInstructions.hpp
209

I think this architecture conditional ought to be outside of the entire if block checking UNW_ARM64_RA_SIGN_STATE. LGTM other than this.

mgorny added inline comments.Jan 11 2019, 8:10 AM
src/DwarfParser.hpp
684
/var/tmp/portage/sys-libs/llvm-libunwind-9999/work/llvm-libunwind-9999/src/DwarfParser.hpp:686:75: error: macro "static_assert" requires 2 argument
s, but only 1 given                                                                                                                                
     static_assert(DW_CFA_AARCH64_negate_ra_state == DW_CFA_GNU_window_save);                                                                      
                                                                           ^
dcederman updated this revision to Diff 181281.Jan 11 2019, 8:15 AM

Added message to static_assert and moved the AArch64 check one step up.

Thanks, builds fine now.

It seems that warnings are disabled by default for libunwind? Or I did something wrong when running cmake. Enabling warnings got me list of things I will have to fix before committing.

Thanks for all comments and have a nice weekend everyone!

dcederman updated this revision to Diff 181504.Jan 14 2019, 2:08 AM

Fixed warnings related to sign and unused parameters.

This revision was automatically updated to reflect the committed changes.