Unwind code in Android uses .code 32 in inline assembly #18605

rengolin · 2013-12-12T22:40:46Z


Bugzilla Link	18231
Resolution	FIXED
Resolved on	Feb 21, 2014 05:20
Version	trunk
OS	Linux
Blocks	#19300
CC	@TNorthover

Extended Description

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}

The text was updated successfully, but these errors were encountered:

rengolin · 2013-12-12T22:52:49Z

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.

llvmbot · 2013-12-12T22:55:18Z

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?

llvmbot · 2013-12-12T22:56:59Z

Also, doing that unwinder code via inline assembly is crazy. That should be a .s file.

rengolin · 2013-12-12T23:12:06Z

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:

Do we support different instruction sets for prologue/epilogue on the same function?
How do you control multiple conditional jumps over the inlined part?

Is this what you had in mind?

Any other solution?

rengolin · 2013-12-12T23:21:47Z

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.

llvmbot · 2013-12-12T23:33:54Z

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:

Do we support different instruction sets for prologue/epilogue on the
same function?

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.

rengolin · 2013-12-13T00:17:45Z

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.

berolinux · 2013-12-13T01:18:58Z

asm code generated by gcc 4.8

rengolin · 2013-12-13T10:11:14Z

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}

llvmbot · 2013-12-13T19:42:37Z

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.

rengolin · 2013-12-13T19:52:04Z

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...

rengolin · 2014-02-04T19:57:24Z

Should be fixed in r199818.

Bero, please check.

cheers,
--renato

rengolin · 2021-11-26T18:57:58Z

mentioned in issue #19300

rengolin mentioned this issue Feb 21, 2014

[Meta] ARM Integrated assembler support #19300

Open

llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 9, 2021

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unwind code in Android uses .code 32 in inline assembly #18605

Unwind code in Android uses .code 32 in inline assembly #18605

rengolin commented Dec 12, 2013

rengolin commented Dec 12, 2013

llvmbot commented Dec 12, 2013

llvmbot commented Dec 12, 2013

rengolin commented Dec 12, 2013

rengolin commented Dec 12, 2013

llvmbot commented Dec 12, 2013

Option 1: add .thumb/.arm directives

Option 2: Change the instruction set

rengolin commented Dec 13, 2013

berolinux commented Dec 13, 2013

rengolin commented Dec 13, 2013

llvmbot commented Dec 13, 2013

rengolin commented Dec 13, 2013

rengolin commented Feb 4, 2014

rengolin commented Nov 26, 2021

Unwind code in Android uses .code 32 in inline assembly #18605

Unwind code in Android uses .code 32 in inline assembly #18605

Comments

rengolin commented Dec 12, 2013

Extended Description

rengolin commented Dec 12, 2013

llvmbot commented Dec 12, 2013

llvmbot commented Dec 12, 2013

rengolin commented Dec 12, 2013

Option 1: add .thumb/.arm directives

Option 2: Change the instruction set

rengolin commented Dec 12, 2013

llvmbot commented Dec 12, 2013

Option 1: add .thumb/.arm directives

Option 2: Change the instruction set

rengolin commented Dec 13, 2013

berolinux commented Dec 13, 2013

rengolin commented Dec 13, 2013

llvmbot commented Dec 13, 2013

rengolin commented Dec 13, 2013

rengolin commented Feb 4, 2014

rengolin commented Nov 26, 2021