This is an archive of the discontinued LLVM Phabricator instance.

ARM Unwind syntax in libcxxabi
ClosedPublic

Authored by rengolin on Sep 7 2014, 7:11 AM.

Details

Summary

This patch fixes the bad argument that GAS accepted but the IAS didn't,
ie. {#0x20}, moving it to {0x20} which both accept. It also makes the
ARMv7+ save/restore correct by using VFP instructions rather than old
co-processor ones.

Fixes PR20529.

Diff Detail

Event Timeline

rengolin updated this revision to Diff 13373.Sep 7 2014, 7:11 AM
rengolin retitled this revision from to ARM Unwind syntax in libcxxabi.
rengolin updated this object.
rengolin edited the test plan for this revision. (Show Details)
rengolin added reviewers: joerg, rafael, compnerd.
rengolin set the repository for this revision to rL LLVM.
rengolin added subscribers: Unknown Object (MLST), ismail, richard.barton.arm.
rengolin updated this object.Sep 7 2014, 7:11 AM
rengolin edited the test plan for this revision. (Show Details)
compnerd added inline comments.Sep 7 2014, 11:41 AM
src/Unwind/UnwindRegistersRestore.S
329

Wow. Did this even work previously? This feels like it could be split out into a separate patch to first address use of the branch on ARM v4.

368

Why the comment here but not previously? Im not sure it really adds much. However, I would say that the comment indicating that fldmiax is equivalent to the ldc is more useful and it may be nice to restore those.

rengolin added inline comments.Sep 7 2014, 11:46 AM
src/Unwind/UnwindRegistersRestore.S
329

No, it didn't. :)

I thought about a separate patch, but it was so small that I didn't bother. I can just commit that one and discuss the rest of the patch.

368

I'll keep the old comment.

The new comment helps understand why I have done all three alternatives as vldmia, and that's because they're all the same on v7+, but didn't use to be up to v6. The main difference is the 0x21 instead of 0x20, which on v7+ they're all the same.

rengolin updated this revision to Diff 13376.Sep 7 2014, 11:56 AM

Leaving old comments intact, leaving the other fix out.

This is the commit responsible for changing them to the co-processor mnemonic in
the first place:
https://github.com/awong-dev/ndk/commit/e8a9a29190cf21b3506c797b175ff550a2d0eff8

Antoine's commit comment here is, I think still relevant:

"In some configurations we don't compile the library with -mfpu=vfp* but we
still want to provide on-demand unwinding support for VFP, so also use the
stc/ldc coprocessor mnemonics to generate the f{st,ld}mia{d,x} instructions."

Why does this change when we go to __ARM_ARCH >= 7 ?

Jon

Jon,

As explained in http://llvm.org/PR20529, ARM deprecated the use of ldc/stc for VFP and NEON operations in v7, so we have to use the VFP/NEON variants. Besides, it's only really necessry on archs that don't have VFP anyway.

cheers,
--renato

compnerd accepted this revision.Sep 10 2014, 8:30 PM
compnerd edited edge metadata.
This revision is now accepted and ready to land.Sep 10 2014, 8:30 PM
rengolin closed this revision.Sep 11 2014, 6:06 AM

Committed in r217585. Thanks!