Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch Instruction in Conditional Block (IT{TTT}) leads to incorrect Graph Dependencies #3976

Closed
pr0me opened this issue May 10, 2022 · 4 comments
Assignees
Labels
Arch: ARM/Thumb Issues with the ARM/Thumb architecture plugin Component: Architecture Issue needs changes to an architecture plugin State: Duplicate Issue is a duplicate of another issue

Comments

@pr0me
Copy link

pr0me commented May 10, 2022

If a branch instruction is made conditional by a previous IT instruction, the disassembler still treats the branch as unconditional.
This leads to disconnected graphs/functions:
Selection_082

Instead, the branch should be treated as conditional, connecting the following basic block as a fall-through edge.
This bug not only leads to disconnected basic block graphs but also to missed instructions:
Selection_083

Example binary (the behavior is observable in the Reset_Handler)

@pr0me
Copy link
Author

pr0me commented May 11, 2022

nvm, turns out they are shipping illegal assembly.

Quoting the manual:
"The instructions (including branches) in the IT block, except the BKPT instruction, must specify the condition in the {cond} part of their syntax."

[EDIT: This only holds true for ARM instructions, as many (most) thumb instructions cannot encode the condition themselves and rely on IT]

@pr0me pr0me closed this as completed May 11, 2022
@pr0me
Copy link
Author

pr0me commented May 12, 2022

Actually, although this is spec-conforming, a fall-through edge should still be added.

Many disassemblers, including Ghidra and Capstone, disassemble the instruction as BLT when provided with the full context.
More importantly, In reality the CPU indeed executes the instruction conditionally. Not showing the edge could be misleading.

@pr0me pr0me reopened this May 12, 2022
@lwerdna lwerdna self-assigned this May 12, 2022
@lwerdna
Copy link
Contributor

lwerdna commented May 12, 2022

...must specify the condition in the {cond} part of their syntax.

I think this is merely a consistency check between what the author intends with the it instruction and the conditional execution of the instructions that follow and are members of the IT block.

I don't think there are fields in the encoding of thumb instructions (except branches) for condition codes, which is why the it instruction exists. When a disassembler produces condition suffixes on an instruction, like "lt" on the nop and b from objdump here:

   10078:	429a      	cmp	r2, r3
   1007a:	bfbe      	ittt	lt
   1007c:	f3af 8000 	noplt.w
   10080:	f3af 8000 	noplt.w
   10084:	e7f8      	blt.n	10078 <.interesting>

It's due to objdump being aware of ittt lt at 0x1007a, not because any fields in their encodings store "lt".

This all points to you being right: not only should there be an edge between this block and its immediate successor (fall-through execution when the blt is not taken), but we should think about propagating the condition suffix to the disassembly of the instructions in the IT block.

Qemu usermode (qemu-arm) behavior verifies supports this conclusion. If I make r2 less than r3, the blt is taken and it loops. If r2 is made to be greater than r3, the blt is not taken and it falls through to the next block.

@plafosse plafosse transferred this issue from Vector35/arch-armv7 Mar 1, 2023
@fuzyll fuzyll added Arch: ARM/Thumb Issues with the ARM/Thumb architecture plugin State: Duplicate Issue is a duplicate of another issue Component: Architecture Issue needs changes to an architecture plugin labels May 22, 2023
@fuzyll
Copy link
Contributor

fuzyll commented May 22, 2023

Closing this as a duplicate of #1720.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: ARM/Thumb Issues with the ARM/Thumb architecture plugin Component: Architecture Issue needs changes to an architecture plugin State: Duplicate Issue is a duplicate of another issue
Projects
None yet
Development

No branches or pull requests

3 participants