Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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). |
libunwind/src/AddressSpace.hpp | ||
---|---|---|
310 ↗ | (On Diff #254678) |
Yeah, that's fair. It may have been a bit premature for me to post this patch at all, so apologies for the distraction. |
- 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.
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). |
libunwind/src/UnwindRegistersRestore.S | ||
---|---|---|
853 | You're right, I don't think it's necessary. I'll remove it. | |
libunwind/src/UnwindRegistersSave.S | ||
1109 |
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. |
- Corrected WEAK_ALIAS definition for hexagon
- Removed .size directive from UnwindRegistersRestore.S
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. |
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). |
libunwind/src/UnwindRegistersSave.S | ||
---|---|---|
1109 |
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. |
Looks like this has landed in https://github.com/llvm/llvm-project/commit/9107594f376e37e99c71881404c686b306f93ad2.
clang-format: please reformat the code