A feature recently being introduced to avoid the madness of having .arm/.thumb (.code 32/.code 16) in inline assembly will break the Android build because the unwind code uses that extensively. Here's a snippet. As you can see, the function is Thumb (.code 16), the inline ASM changes it to ARM (.code 32), than the function continues and returns with an instruction that is only valid in Thumb mode (pop.w). .section .text._ZN13UnwindCurrent6UnwindEj,"ax",%progbits .globl _ZN13UnwindCurrent6UnwindEj .align 2 .type _ZN13UnwindCurrent6UnwindEj,%function .code 16 .thumb_func _ZN13UnwindCurrent6UnwindEj: .Lfunc_begin3: .loc 1 66 0 .loc 1 0 0 push.w {r11, lr} mov r2, r0 .Ltmp7: .loc 1 67 0 prologue_end add.w r0, r2, #8 .Ltmp8: @APP .align 2 bx pc nop .code 32 stmia r0, {r0-r15} orr r0, pc, #1 bx r0 @NO_APP .Ltmp9: .loc 1 72 0 mov r0, r2 movs r2, #1 .Ltmp10: bl _ZN13UnwindCurrent17UnwindFromContextEjb(PLT) .loc 1 73 0 pop.w {r11, pc}
This is the revision to completely disable .arm/.thumb in inline asm: http://llvm-reviews.chandlerc.com/D2255 While I think this is correct, and should be enforced, I don't think this is realistic at the moment, maybe even at long term. My opinion is that we should have a command-line flag that controls what to do in those cases, with the default of being an error. The obvious behaviour (and what I think GCC is doing) is to Always revert to the function's mode on exit IF .arm/.thumb was used, and make sure there is no code motion between the inline ASM block and the code directive. Anther, nonsense but plausible behaviour, would be to let them exist and do nothing. I believe that no one in their right (and even wrong) minds would want both behaviours to co-exist on the same compilation job. I also think that the first one makes more sense (because of conditional jumping), so if we have to pick one to support, I'd pick that one. Of course, for now, we have to know what GCC does, but I'm betting it's the first.
By "switch back" you just mean the encoding for the instructions (i.e., as-if a .thumb/.arm were present), not insert an instruction sequence to actually do the mode switch, right?
Also, doing that unwinder code via inline assembly is crazy. That should be a .s file.
Hi Jim, I think I misread your question, so, please bear with me while I explain what I thought... Problem: .thumb push.w cmp.w r0, #0 beq.w after /-- Inline --\ .arm add mul \-- Inline --/ after: pop.w In that case, the conditional jump could either let the .arm run, or not, making the code after .after unpredictable on how to encode. I'm not sure how the MC layer takes inline asm into the stream, but if it doesn't re-interpret them in time to chose the instruction set, we can't predict how this is going to change. As is, if r0 is not 0, this code fails. Option 1: add .thumb/.arm directives ------------------------------------ .thumb push.w cmp.w r0, #0 beq.w after /-- Inline --\ .arm add mul \-- Inline --/ after: .thumb pop.w There should be multiple .thumb directives after each label and one after the inlined last instruction, to make sure all cases are covered. This should be simple and require less changes to the assembler... Is this the one you didn't like? Option 2: Change the instruction set ------------------------------------ .thumb push.w cmp.w r0, #0 beq.w after /-- Inline --\ .arm add mul \-- Inline --/ after: pop This solution has a few problems: 1. Do we support different instruction sets for prologue/epilogue on the same function? 2. How do you control multiple conditional jumps over the inlined part? Is this what you had in mind? Any other solution?
> Also, doing that unwinder code via inline assembly is crazy. That should be a .s file. I agree, this is horrible. Before we start pursuing this, I've asked Bero to show me what GCC is producing. Another question is to know why this is there and if we can change to something more sane before actually implementing it in Clang/LLVM. This is more of a placeholder bug for something more concrete once we have more info.
(In reply to comment #4) > Hi Jim, I think I misread your question, so, please bear with me while I > explain what I thought... > > Problem: > > .thumb > push.w > cmp.w r0, #0 > beq.w after > /-- Inline --\ > .arm > add > mul > \-- Inline --/ > after: > pop.w > > In that case, the conditional jump could either let the .arm run, or not, > making the code after .after unpredictable on how to encode. I'm not sure > how the MC layer takes inline asm into the stream, but if it doesn't > re-interpret them in time to chose the instruction set, we can't predict how > this is going to change. > > As is, if r0 is not 0, this code fails. Right. This is malformed code and I don't think the compiler can realistically do anything about it. It's no different than any other incorrect source code bug in that regard. > > > Option 1: add .thumb/.arm directives > ------------------------------------ > > .thumb > push.w > cmp.w r0, #0 > beq.w after > /-- Inline --\ > .arm > add > mul > \-- Inline --/ > after: > .thumb > pop.w > > There should be multiple .thumb directives after each label and one after > the inlined last instruction, to make sure all cases are covered. > > This should be simple and require less changes to the assembler... > > Is this the one you didn't like? If we're going to do anything here, it should be to add a ".thumb" directive immediately following the inline assembler (as-if it were actually inside the inline asm block). For this simple case, that also has the benefit of being semantically correct. Oof. this actually raises an interesting point. The behaviour has to be the same when we're compiling -S vs. -c. -S doesn't parse the inline assembly at all. > Option 2: Change the instruction set > ------------------------------------ > > .thumb > push.w > cmp.w r0, #0 > beq.w after > /-- Inline --\ > .arm > add > mul > \-- Inline --/ > after: > pop > > This solution has a few problems: > > 1. Do we support different instruction sets for prologue/epilogue on the > same function? > > 2. How do you control multiple conditional jumps over the inlined part? This would require massive, massive surgery in the compiler. It would require supporting changing thumb vs. arm on basic block boundaries, effectively.
Ok, so I think we're on the same page... > The behaviour has to be the same when we're compiling -S vs. -c. -S doesn't parse the inline assembly at all. This is what I was afraid... Regardless of this issue, I think we should go ahead with Greg's patch. It's also entirely possible that the Android libraries have both ARM and Thumb versions of the inlined ASM and just picked the wrong one. Once we have the GCC asm we'll know more.
Created attachment 11718 [details] asm code generated by gcc 4.8
As I suspected, it emits a .code 16 right after the inline asm block. .size _ZN13UnwindCurrent17UnwindFromContextEjb, .-_ZN13UnwindCurrent17UnwindFromContextEjb .align 2 .global _ZN13UnwindCurrent6UnwindEj .code 16 .thumb_func .type _ZN13UnwindCurrent6UnwindEj, %function _ZN13UnwindCurrent6UnwindEj: push {r3, lr} mov r3, r0 add r0, r0, #8 #APP @ 67 "system/core/libbacktrace/UnwindCurrent.cpp" 1 .align 2 bx pc nop .code 32 stmia r0, {r0-r15} orr r0, pc, #1 bx r0 @ 0 "" 2 .code 16 mov r2, #1 mov r0, r3 bl _ZN13UnwindCurrent17UnwindFromContextEjb @ sp needed pop {r3, pc}
Ya know, we could just always emit a directive like that, whether there was anything in the asm block or not. If there was, it'll switch back. If there wasn't, it's effectively whitespace.
That's a good point. And it completely bypasses the -c vs. -S problem. And it could even be done on Greg's patch while it's still hot...
Should be fixed in r199818. Bero, please check. cheers, --renato