Page MenuHomePhabricator

[libunwind] Add support for ARMv7-M architecture which uses the Thumb 2 ISA (unified syntax)
ClosedPublic

Authored by jeremfg on Apr 8 2019, 12:03 PM.

Details

Summary

I'm a first time contributor to LLVM...

While attempting to compile the assembly files ( .S) for a Cortex-M4 device using GCC's arm-none-eabi, I encountered issue which this patch fixes.

Diff Detail

Repository
rL LLVM

Event Timeline

jeremfg created this revision.Apr 8 2019, 12:03 PM

The change looks sensible to me, but should we maybe even skip the #if altogether? If the files uses unified syntax and can't be parsed in thumb mode otherwise, there's maybe no need for conditionals at all?

jeremfg added a comment.EditedApr 8 2019, 7:59 PM

The change looks sensible to me, but should we maybe even skip the #if altogether? If the files uses unified syntax and can't be parsed in thumb mode otherwise, there's maybe no need for conditionals at all?

I'm no expert but I just read that UAL (Unified Assembler Language), while being backward compatible with the old ARM syntax, is not compatible with the previous Thumb syntax.
http://downloads.ti.com/docs/esd/SPNU118/unified-assembly-language-syntax-support-spnu1184444.html

But again, perhaps it doesn't matter in the specific case that concerns us here? I guess it depends on whether the code in the #if !defined(__ARM_ARCH_ISA_ARM) && __ARM_ARCH_ISA_THUMB == 1 condition that follows contains only valid UAL syntax as well. Which I am not qualified to confirm or not.

If somebody can confirm that my understanding is correct, I could try and see if GCC throws an error during compilation for the ARMv6-M architecture, but I don't trust myself enough to rely on my own tests alone and my weak understanding to ensure I won't break support for current Thumb-1 only users. Hopefully somebody else can pitch in to confirm (or infirm) my understanding.

By the way, are there other architecture besides ARMv6-M that doesn't support ARM and report that __ARM_ARCH_ISA_THUMB == 1 ?

If that's all UAL-compatible instructions, then yes I think my #if could be removed and only the .syntax unified should be kept. At least, that's how I understand it. I could be wrong...
Perhaps some assemblers simply refuses the .sytanx unified directive altogether when targeting legacy Thumb-1 only architectures, making this all a moot discussion?

The change looks sensible to me, but should we maybe even skip the #if altogether? If the files uses unified syntax and can't be parsed in thumb mode otherwise, there's maybe no need for conditionals at all?

I'm no expert but I just read that UAL (Unified Assembler Language), while being backward compatible with the old ARM syntax, is not compatible with the previous Thumb syntax.
http://downloads.ti.com/docs/esd/SPNU118/unified-assembly-language-syntax-support-spnu1184444.html

But again, perhaps it doesn't matter in the specific case that concerns us here? I guess it depends on whether the code in the #if !defined(__ARM_ARCH_ISA_ARM) && __ARM_ARCH_ISA_THUMB == 1 condition that follows contains only valid UAL syntax as well. Which I am not qualified to confirm or not.

Oh, sorry, I was sloppy and didn't actually check the rest of the file to see the existing conditional alternative codepaths with !defined(__ARM_ARCH_ISA_ARM) && __ARM_ARCH_ISA_THUMB == 1.

Given that, the suggested form of this patch is indeed correct - sorry for the noise.

I can commit the patch for you, but I'd need approval by either of @EricWF, @ldionne or @mclow.lists first.

Given that, the suggested form of this patch is indeed correct - sorry for the noise.

I can commit the patch for you, but I'd need approval by either of @EricWF, @ldionne or @mclow.lists first.

Ping @EricWF @ldionne @mclow.lists

Given that, the suggested form of this patch is indeed correct - sorry for the noise.

I can commit the patch for you, but I'd need approval by either of @EricWF, @ldionne or @mclow.lists first.

Ping @EricWF @ldionne @mclow.lists

Ping @EricWF @ldionne @mclow.lists

ldionne accepted this revision.Apr 17 2019, 1:40 PM

I'm relaying approval to @mstorsjo, who seems okay with the change.

This revision is now accepted and ready to land.Apr 17 2019, 1:40 PM

I have no problem with this.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 11:33 PM