Emit the low-level guard of a predicated run by leftbyte · Pull Request #295 · static-analysis-engineering/CodeHawk-Binary

leftbyte · 2026-09-16T19:36:32Z

For ARM predicated instructions, the high level AST from the lifting is represented as a branch on the instruction predicate. The serialization from the high level to low level representation flattened every block into one instruction sequence so a conditional store was rendered as though it always executes. This resulted in the low-level form of the guard being detached from the rest of the graph (i.e the exprid did not have a node.)

The following is an example three-line function whose guarded store gcc if-converts:

void set_channel(char *dst, int idx, char v, unsigned *nwrites) {
    if (idx >= 0)
        dst[idx] = v;          /* one predicated instruction */
    (*nwrites)++;
}

1074c:  cmp    r1, #0
10750:  strbge r2, [r0, r1]    <- executes only when the guard holds
10754:  ldr    r2, [r3]

Before, in the low-level AST, the store always happens and the provenance pointed at a guard that was not there:

#450  block
  #449  block
    #409  label
    #447  instrs
      #413  assign  ignored = (R1 minus 0)                 @ 0x1074c
      #424  assign  *((R0 plus (R1 lsl 0))) = R2           @ 0x10750   <- unconditional
      #432  assign  R2 = *((R3 plus 0))                    @ 0x10754
      #437  assign  R2 = (R2 plus 1)                       @ 0x10758
      #445  assign  *((R3 plus 0)) = R2                    @ 0x1075c
      #446  nop                                            @ 0x10760
    #448  return

expression-mapping: {'3': 2, '12': 4, '18': 15, '26': 21, '38': 28, '47': 39}
  hl exprid 3   -> ll exprid 2   |  #382 idx                 -> #412 (R1 minus 0)
  hl exprid 12  -> ll exprid 4   |  #358 v                   -> #423 R2
  hl exprid 18  -> ll exprid 15  |  #349 (idx ge 0)          -> NO NODE CARRIES EXPRID 15
  hl exprid 26  -> ll exprid 21  |  #395 *(nwrites)          -> #431 *((R3 plus 0))
  hl exprid 38  -> ll exprid 28  |  #404 (*(nwrites) plus 1) -> #436 (R2 plus 1)
  hl exprid 47  -> ll exprid 39  |  #373 (*(nwrites) plus 1) -> #444 R2

Five of the six entries resolve to a node. The sixth is the guard where the high-level exprid 18 is node #349, (idx ge 0) (the condition of the high-level if) that maps to low-level exprid 15, which did not have a node in the low-level AST. A consumer that resolves the low-level form of a guard finds nothing, which caused the mram-patcher to raise a KeyError.

After. The predicated run is emitted as a branch, and node #421 carries exprid 15:

#463  block
  #462  block
    #409  label
    #460  block
      #414  instrs
        #413  assign  ignored = (R1 minus 0)               @ 0x1074c
      #436  if (N eq V)  [cond node #421 exprid=15]  predicated=1   @ 0x10750
        #434  block
          #433  instrs
            #432  assign  *((R0 plus (R1 lsl 0))) = R2     @ 0x10750
        #435  block
      #459  instrs
        #444  assign  R2 = *((R3 plus 0))                  @ 0x10754
        ...
    #461  return

hl exprid 18  -> ll exprid 15  |  #349 (idx ge 0)          -> #421 (N eq V)

(N eq V) is the flag test the ge predicate reads and it is the same expression object the mapping already pointed at. assembly_ast now partitions a block that has control flow and emits each fragment, mirroring the structure the high-level side already uses (ast -> fragmented_ast -> ast_fragment). A predicated fragment becomes a branch so the node keeps its exprid.

sipma

It looks good to me. Thank you for fixing this.

Ricardo, could you please also have a look at it, and merge it if you think it is okay?

waskyo

First pass: I think I understand the reason for the change and (high-level) what you're doing to fix it, now I'm just trying to make sure the code details match up.

Apologies for the pain.

waskyo · 2026-09-18T14:34:32Z

+            self.basicblock.partition_control_flow()
+            return self.fragmented_assembly_ast(astree)
+
+        else:


consider removing the else since you do early return on the if. If you choose to keep it, then nuke the empty line.

Sure, done!

waskyo · 2026-09-18T14:36:39Z

        return self.hl_ast_cc_condition

+    def assembly_ast_cc_condition(
+            self, astree: "ASTInterface") -> Optional[AST.ASTExpr]:


style nit: looking at the code around, the astree param should be in its own line

...and actually, this style isn't consistent (but I'll change my code to fit the "each param on new line" style. ast_fragment, trampoline_block_instructions, and some other methods have all the params on the same line. I prefer your suggestion though, but won't change them all right now.

waskyo · 2026-09-18T14:37:51Z

            self.ast_cc_condition_prov(astree)
        return self.hl_ast_cc_condition

+    def assembly_ast_cc_condition(


#til q: what does the cc stand for in the function name?

cc is "condition code". The ARM instruction's conditional-execution suffix and the NZCV flags it tests.

waskyo · 2026-09-18T14:38:23Z


-    def fragmented_ast(self, astree: "ASTInterface") -> AST.ASTStmt:
+    def assembly_ast_fragment(
+            self, astree: "ASTInterface", frag: "BasicBlockFragment") -> AST.ASTStmt:


nit: follow coding style of the existing code. This comment applies to some of the other functions you added (don't want to spam on every instance)

Fixed here and I reviewed my modifications to check the lines I changed.

waskyo · 2026-09-18T15:03:05Z

+            instritems: List[ASTInterfaceInstruction],
+            ll: bool = False) -> AST.ASTStmt:
        instrs: List[AST.ASTInstruction] = []
        for i in instritems:


I think this is replacing the original code from def assembly_ast but that one looked at the instructions in sorted order:

for (a, i) in sorted(self.instructions.items(), key=lambda p: p[0]):

Does that difference matter?

The ordering is actually moved out into the callers, so the ordering has not changed.

waskyo · 2026-09-18T15:05:33Z

-    def fragmented_ast(self, astree: "ASTInterface") -> AST.ASTStmt:
+    def assembly_ast_fragment(
+            self, astree: "ASTInterface", frag: "BasicBlockFragment") -> AST.ASTStmt:
+        if frag.is_predicated:


A comment here explaining the magic would be pretty useful for future us.

All of these functions don't have docstrings - I propose that I add that as a separate PR.

waskyo · 2026-09-18T15:07:11Z

            return self.linear_ast(astree, instrs)

-    def fragmented_ast(self, astree: "ASTInterface") -> AST.ASTStmt:
+    def assembly_ast_fragment(


This is not something from your changes, but trying to keep track of the distinction between ast_fragment and fragmented_ast is making my head spin 😵‍💫

If you're feeling magnanimous, some comments would be amaze 🎅

Sure, but how about in a separate PR?

waskyo · 2026-09-18T15:12:05Z

+            cinstr = theninstrs[0]
+            brcond = cinstr.assembly_ast_cc_condition(astree)
+            if brcond is None:
+                chklogger.logger.warning(


rando: Use logger.error. Henny and I have been talking about using error always since we don't have a formal distinction between warning and error.

Done in this and the other log.warning message.

waskyo · 2026-09-18T15:13:34Z

+            thenstmt = self.linear_assembly_block_ast(astree, theninstrs)
+            elsestmt = self.linear_assembly_block_ast(astree, elseinstrs)
+            spans = [(i.iaddr, i.bytestring) for i in theninstrs + elseinstrs]
+            cinstr = theninstrs[0]


rando: what does cinstr stand for?

cinstr stands for condition instructions.

waskyo · 2026-09-18T15:15:56Z

+                brcond, thenstmt, elsestmt, cinstr.iaddr, predicated=instrcount)
+            astree.add_stmt_span(ifstmt.locationid, spans)
+            return ifstmt
+        else:


nit: possibly nicer organization for this code since this path is simple:

if not frag.is_predicated: instrs = [self.get_instruction(i.iaddr) for i in frag.linear] return self.linear_assembly_ast(astree, instrs) <predicated handling code from above, no need for else> ... return ifstmt

assembly_ast flattened every block into an instruction sequence, so an if-converted predicated run was rendered as unconditional assigns and the low-level form of its guard was attached to nothing. Since serialization only indexes nodes reachable from the ast start nodes, that expression never reached the exported AST, leaving provenance's expression-mapping pointing at an id no node carries. Partition a block that has control flow and emit each predicated fragment as a branch on the flag condition, mirroring what ast_fragment already does for the high-level AST. assembly_ast_cc_condition returns the same ll_ast_cc_condition object that ast_cc_condition_prov mapped the high-level condition onto, so the emitted node keeps its exprid and the mapping resolves. A fragment with no low-level predicate falls back to the flat sequence with a warning. Also makes the low-level AST stop claiming that a conditional instruction always executes.

The low-level block emitters added with the predicated guard were copies of their high-level counterparts so refactor to give each high-level emitter an ll selector.

leftbyte marked this pull request as ready for review September 17, 2026 03:05

sipma approved these changes Sep 17, 2026

View reviewed changes

sipma requested a review from waskyo September 17, 2026 20:01

waskyo reviewed Sep 18, 2026

View reviewed changes

Dan Phung added 2 commits September 18, 2026 16:49

refactor ast emitters

b254851

The low-level block emitters added with the predicated guard were copies of their high-level counterparts so refactor to give each high-level emitter an ll selector.

leftbyte force-pushed the dphung/link-dangling-entry branch from 5dac642 to b254851 Compare September 18, 2026 23:52

Dan Phung added 2 commits September 18, 2026 16:57

change log.warning to log.error

1da8772

refactor frag.is_predicated block for early return and less nesting

aeb2634

Conversation

leftbyte commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipma left a comment

Choose a reason for hiding this comment

Uh oh!

waskyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leftbyte commented Sep 16, 2026 •

edited

Loading