LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 20529 - [ARM] clang integrated assembler cannot compile libcxxabi/src/Unwind/UnwindRegistersRestore.S
Summary: [ARM] clang integrated assembler cannot compile libcxxabi/src/Unwind/UnwindRe...
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: ARM (show other bugs)
Version: trunk
Hardware: PC Linux
: P normal
Assignee: Renato Golin
URL:
Keywords:
Depends on: 20025
Blocks: 18926
  Show dependency tree
 
Reported: 2014-08-04 07:36 PDT by İsmail Dönmez
Modified: 2014-09-11 08:12 PDT (History)
8 users (show)

See Also:
Fixed By Commit(s):


Attachments
Remove the hash part from UnwindRegistersSave.S and UnwindRegistersRestore.S (1.87 KB, patch)
2014-08-19 04:54 PDT, İsmail Dönmez
Details

Note You need to log in before you can comment on or make changes to this bug.
Description İsmail Dönmez 2014-08-04 07:36:06 PDT
This is with clang r214699:

cd /home/ismail/libcxxabi/build/src/Unwind && /havana/dist/llvm/bin/clang  -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -fPIC -Werror=date-time -ffunction-sections -fdata-sections -I/home/ismail/libcxxabi/include -I/home/ismail/libcxx/include    -UNDEBUG -fPIC -o CMakeFiles/unwind.dir/UnwindRegistersRestore.S.o   -c /home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S
/home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S:350:24: error: unknown token in expression
  ldc p11, cr0, [r0], {#0x20} @ fldmiad r0, {d0-d15}
                       ^
/home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S:350:24: error: illegal expression
  ldc p11, cr0, [r0], {#0x20} @ fldmiad r0, {d0-d15}
                       ^
/home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S:361:24: error: unknown token in expression
  ldc p11, cr0, [r0], {#0x21} @ fldmiax r0, {d0-d15}
                       ^
/home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S:361:24: error: illegal expression
  ldc p11, cr0, [r0], {#0x21} @ fldmiax r0, {d0-d15}
                       ^
/home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S:372:25: error: unknown token in expression
  ldcl p11, cr0, [r0], {#0x20} @ vldm r0, {d16-d31}
                        ^
/home/ismail/libcxxabi/src/Unwind/UnwindRegistersRestore.S:372:25: error: illegal expression
  ldcl p11, cr0, [r0], {#0x20} @ vldm r0, {d16-d31}
                        ^
Comment 1 Renato Golin 2014-08-04 12:57:44 PDT
I found two problems:

First, the hash is not really appropriate for that argument (it's a number, not an immediate), even though GAS accepts it, the ARM ARM doesn't mention it and I think it shouldn't be there.

Both MC and GAS accept that syntax (because MC uses the generic parser to read the number, which doesn't understand ARM hash immediate syntax).

Fixing this is as easy as changing it in libc++abi.

Second, the MatchAndEmitInstruction doesn't like the stream of operands and needs fixing. I'm looking into it now.
Comment 2 Renato Golin 2014-08-04 15:48:55 PDT
So, these co-processor instructions were allowed (and somewhat encouraged) in v5/v6 processors due to the poor nature of VFP support in those days, but v7 and v8 explicitly recommend using vector instructions instead.

I haven't found an explicit prohibition in the ARM ARM (either v7 or v8) but Richard has stated some strong facts about it earlier, so I'm inclined to continue the trend and forbid cp10/cp11 on those architectures.

I'll allow them on v5/v6, but for v7/v8 libc++abi's unwind libraries will *have* to use vector loads and stores.
Comment 3 Renato Golin 2014-08-04 18:29:14 PDT
Ismail, Joerg,

The v5/v6 support is now enabled, but ARMv8 explicitly disallow and v7 strongly disallow it (Richard might know more about it).

The solutions are:

1. Remove the hash from the constant, since both GCC and LLVM accept it that way.

2. Use VLDs/VSTs on v7/v8 targets via an ifdef in the S file.

cheers,
--renato
Comment 4 İsmail Dönmez 2014-08-19 04:33:27 PDT
Hi Renato,

(In reply to comment #3)
> Ismail, Joerg,
> 
> The v5/v6 support is now enabled, but ARMv8 explicitly disallow and v7
> strongly disallow it (Richard might know more about it).
> 
> The solutions are:
> 
> 1. Remove the hash from the constant, since both GCC and LLVM accept it that
> way.

This seems to be the easiest way forward. Could you please do this for now? This would unblock me from building libcxxabi on ARM :) (currently I do an ugly -no-integrated-as hack)
Comment 5 Renato Golin 2014-08-19 04:39:44 PDT
Hi Ismail,

I meant remove the hash from the source code, not in the assembler, since that's the correct syntax, and both GNU and LLVM assemblers accept it that way.

cheers,
--renato
Comment 6 İsmail Dönmez 2014-08-19 04:42:43 PDT
(In reply to comment #5)
> Hi Ismail,
> 
> I meant remove the hash from the source code, not in the assembler, since
> that's the correct syntax, and both GNU and LLVM assemblers accept it that
> way.

I meant the same. We are talking about modifying UnwindRegistersRestore.S to remove the hash ({#0x20} part) right?
Comment 7 Renato Golin 2014-08-19 04:48:33 PDT
Yes. Sorry! :)
Comment 8 İsmail Dönmez 2014-08-19 04:54:32 PDT
Created attachment 12917 [details]
Remove the hash part from UnwindRegistersSave.S and UnwindRegistersRestore.S
Comment 9 İsmail Dönmez 2014-08-19 04:55:16 PDT
(In reply to comment #7)
> Yes. Sorry! :)

Ok then please the patch in comment 8
Comment 10 Renato Golin 2014-08-19 05:51:37 PDT
Ok, right, that's not what I meant. The change is:

-  ldc p11, cr0, [r0], {#0x21}  @ fldmiax r0, {d0-d15}
+  ldc p11, cr0, [r0], {0x21}  @ fldmiax r0, {d0-d15}

etc.

Please, create a patch on Phabricator towards libc++abi at:

http://reviews.llvm.org/differential/

I don't know who are the default reviewers for libc++abi, though.

cheers,
--renato
Comment 11 İsmail Dönmez 2014-09-03 04:03:10 PDT
(In reply to comment #10)
> Ok, right, that's not what I meant. The change is:
> 
> -  ldc p11, cr0, [r0], {#0x21}  @ fldmiax r0, {d0-d15}
> +  ldc p11, cr0, [r0], {0x21}  @ fldmiax r0, {d0-d15}
> 
> etc.

Sorry for the late reply but that doesn't work:

../projects/libcxxabi/src/Unwind/UnwindRegistersSave.S:321:7: error: invalid operand for instruction
  stc p11, cr0, [r0], {0x20} @ fstmiad r0, {d0-d15}
      ^
../projects/libcxxabi/src/Unwind/UnwindRegistersSave.S:332:7: error: invalid operand for instruction
  stc p11, cr0, [r0], {0x21} @ fstmiax r0, {d0-d15}
      ^
../projects/libcxxabi/src/Unwind/UnwindRegistersSave.S:350:8: error: invalid operand for instruction
  stcl p11, cr0, [r0], {0x20} @ vldm r0, {d16-d31}
       ^
Comment 12 Renato Golin 2014-09-03 04:48:32 PDT
Hi Ismail,

If you're running on v7/v8, than it shouldn't work. ARM has deprecated that a very long time ago due to the existence of proper vector instructions.

The change I proposed makes it work on v5/v6, while conditionalising the code to use proper vector instructions on v7/v8 will make it work on the rest.

So, removing the hash solves half of the problem, while re-implementing usinf VFP/NEON will solve the other half. We need both changes to make it work on all ARM cores. 

Makes sense?

cheers,
--renato
Comment 13 İsmail Dönmez 2014-09-03 04:49:54 PDT
(In reply to comment #12)
> Hi Ismail,
> 
> If you're running on v7/v8, than it shouldn't work. ARM has deprecated that
> a very long time ago due to the existence of proper vector instructions.
> 
> The change I proposed makes it work on v5/v6, while conditionalising the
> code to use proper vector instructions on v7/v8 will make it work on the
> rest.
> 
> So, removing the hash solves half of the problem, while re-implementing
> usinf VFP/NEON will solve the other half. We need both changes to make it
> work on all ARM cores. 
> 
> Makes sense?

It does indeed makes sense. But I never touched ARM assembly (barely read ARM ARM). So someone else will have to do this :/
Comment 14 Renato Golin 2014-09-03 04:56:30 PDT
Right, ok. It's about time I re-learn unwinding...
Comment 15 Renato Golin 2014-09-07 09:03:40 PDT
http://reviews.llvm.org/D5234
Comment 16 Renato Golin 2014-09-11 08:12:00 PDT
Fixed in r217585.