This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] add hexagon support
ClosedPublic

Authored by bcain on Apr 2 2020, 7:46 PM.

Details

Reviewers
sidneym
mstorsjo
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

bcain created this revision.Apr 2 2020, 7:46 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 2 2020, 7:46 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo added inline comments.
libunwind/src/AddressSpace.hpp
310 ↗(On Diff #254678)

I'm not familiar with this aspect of dwarf - but this seems to be a change that isn't within arch specific ifdefs, contrary to the rest of the patch. I'd like an explicit clarification of that aspect (in e.g. the commit message/description).

bcain marked an inline comment as done.Apr 6 2020, 2:06 PM
bcain added inline comments.
libunwind/src/AddressSpace.hpp
310 ↗(On Diff #254678)

I'm not familiar with this aspect of dwarf - but this seems to be a change that isn't within arch specific ifdefs, contrary to the rest of the patch. I'd like an explicit clarification of that aspect (in e.g. the commit message/description).

Yeah, that's fair. It may have been a bit premature for me to post this patch at all, so apologies for the distraction.

bcain updated this revision to Diff 256188.Apr 8 2020, 10:29 PM
bcain retitled this revision from [libunwind] --draft-- add hexagon support to [libunwind] add hexagon support.
  • Remove encoding == DW_EH_PE_omit guard from getEncodedP()
  • Remove hexagon changes from findFDE()
  • Fix hexagon unw_getcontext definition, declaration in UnwindRegistersSave.S
bcain added a comment.Apr 8 2020, 10:31 PM
  • Remove encoding == DW_EH_PE_omit guard from getEncodedP()
  • Remove hexagon changes from findFDE()
  • Fix hexagon unw_getcontext definition, declaration in UnwindRegistersSave.S

I did not apply changes from git-clang-format. When I did, it looked like the style no longer matched the neighboring code. I can happily add it in but it seems like we should consider localizing the specified style for libunwind or applying the llvm style to the entire content.

mstorsjo added inline comments.Apr 9 2020, 3:51 AM
libunwind/src/UnwindRegistersRestore.S
853

There's no other .size directives used for other ELF platforms as far as I can see, and you aren't adding one in UnwindRegistersSave.S - so this feels a bit inconsistent.

libunwind/src/UnwindRegistersSave.S
1109

What's the purpose and need of handling the weak alias differently here? Does the assembler use a different syntax for the weak things there, or do you apply weak to the symbol __unw_getcontext as well?

I think it might be better to move this difference into assembly.h to encapsulate the implementation details of how to set up a weak alias there (there's three different cases there already).

bcain added inline comments.Apr 9 2020, 7:19 AM
libunwind/src/UnwindRegistersRestore.S
853

You're right, I don't think it's necessary. I'll remove it.

libunwind/src/UnwindRegistersSave.S
1109

Does the assembler use a different syntax for the weak things there

Yes -- the assembler can't stack the directives with semicolons, AFAICT we need a newline.

I played with definitions of WEAK_ALIAS in assembly.h but couldn't get it right. I gave up too easily, though -- I'll fix it.

bcain updated this revision to Diff 256388.Apr 9 2020, 1:25 PM
  • Corrected WEAK_ALIAS definition for hexagon
  • Removed .size directive from UnwindRegistersRestore.S
bcain marked an inline comment as done.Apr 9 2020, 1:32 PM
bcain added inline comments.
libunwind/src/assembly.h
82 ↗(On Diff #256388)

Tsk, no -- this was hasty. This creates multiple weak symbols and doesn't do the aliasing. Sorry for the distraction.

mstorsjo added inline comments.Apr 9 2020, 3:14 PM
libunwind/src/UnwindRegistersSave.S
1109

If you use llvm as assembler, it might be possible to fix it to support some separator char though (if it's ok to require the latest version of llvm for building libunwind for hexagon).

bcain updated this revision to Diff 256443.Apr 9 2020, 4:33 PM

Fix WEAK_ALIAS() definition for hexagon

bcain added inline comments.Apr 9 2020, 4:43 PM
libunwind/src/UnwindRegistersSave.S
1109

If you use llvm as assembler, it might be possible to fix it to support some separator char though (if it's ok to require the latest version of llvm for building libunwind for hexagon).

I misunderstood the nature of the problem. It's not related to separators, semicolons work just fine.

It was the alias assignment statement, it's not supported by the assembler. So I found the .equiv directive and it looks like it satisfies the same use case.

After this patch, I checked that the resulting hexagon libunwind.a had the same type, visibility, binding for __unw_getcontext and unw_getcontext as the one of the x86 libunwind.a binaries from 10.0.0 hosted on releases.llvm.org.

mstorsjo accepted this revision.Apr 9 2020, 10:06 PM

Looks good to me now

libunwind/src/Registers.hpp
3556

This looks like a leftover

This revision is now accepted and ready to land.Apr 9 2020, 10:06 PM
bcain updated this revision to Diff 256529.Apr 10 2020, 2:11 AM

removed obsolete/commented-out getIP() implementation