bpf/analyze: replace BlockIterator.Previous() with Backtracker

This commit removes the concept of 'global backtracking' (backtracking through
chains of blocks with single predecessors) and refactors BlockIterator.Previous()
into its own object. This removes the hazard of accidentally backtracking using
the main iterator.

Since no loop/depth tracking or block rollovers need to be done, this makes the
implementation significantly more straightforward.

This makes starting a backtracking session for reachability analysis on a
program with many branches a lot faster. Compared to main before this PR:

goos: linux
goarch: amd64
pkg: github.com/cilium/cilium/pkg/bpf/analyze
cpu: AMD Ryzen 7 3700X 8-Core Processor
                 │   old.txt   │              new.txt               │
                 │   sec/op    │   sec/op     vs base               │
ComputeBlocks-16   654.6µ ± 1%   637.1µ ± 3%   -2.67% (p=0.009 n=6)
Reachability-16    29.16µ ± 2%   16.30µ ± 2%  -44.10% (p=0.002 n=6)
geomean            138.1µ        101.9µ       -26.24%

                 │   old.txt    │               new.txt                │
                 │     B/op     │     B/op      vs base                │
ComputeBlocks-16   372.5Ki ± 0%   372.5Ki ± 0%       ~ (p=0.935 n=6)
Reachability-16      352.0 ± 0%     352.0 ± 0%       ~ (p=1.000 n=6) ¹
geomean            11.32Ki        11.32Ki       +0.00%
¹ all samples are equal

                 │   old.txt   │               new.txt               │
                 │  allocs/op  │  allocs/op   vs base                │
ComputeBlocks-16   8.053k ± 0%   8.053k ± 0%       ~ (p=1.000 n=6) ¹
Reachability-16     3.000 ± 0%    3.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean             155.4         155.4       +0.00%
¹ all samples are equal

Signed-off-by: Timo Beckers <timo@isovalent.com>

Author: ti-mo

pkg/bpf/analyze/blocks.go

@@ -219,17 +219,16 @@ func (b *Block) iterateGlobal(blocks Blocks, insns asm.Instructions) *BlockItera
 	}
 }
 
-// predecessor returns the previous Block in the control flow if there is
-// exactly one predecessor, otherwise returns nil.
+// backtrack returns a Backtracker starting at the end of the block.
 //
-// This is useful for walking the control flow backwards when there is no
-// branching, e.g. for finding the last instruction that wrote to a register
-// before it is read.
-func (b *Block) predecessor() *Block {
-	if len(b.predecessors) == 1 {
-		return b.predecessors[0]
+// After the next call to [Backtracker.Previous], the backtracker will point to
+// the last instruction in the block.
+func (b *Block) backtrack(insns asm.Instructions) *Backtracker {
+	return &Backtracker{
+		insns: insns,
+		stop:  b.start,
+		index: b.end,
 	}
-	return nil
 }
 
 func (b *Block) String() string {
@@ -285,12 +284,6 @@ func (b *Block) Dump(insns asm.Instructions) string {
 	return sb.String()
 }
 
-// maxDepth is the maximum depth of block traversal when backtracking to
-// predecessors. Used in favor of a visited set since it's much cheaper than
-// frequent map lookups. Typical depth while looking for map pointer loads is
-// 1-3 with some double-digit outliers.
-const maxDepth = 128
-
 // BlockIterator is an iterator over the instructions in a block or a list of
 // blocks.
 //
@@ -308,7 +301,6 @@ type BlockIterator struct {
 	block  *Block
 
 	insns asm.Instructions
-	depth uint8
 
 	ins   *asm.Instruction
 	index int
@@ -332,19 +324,6 @@ func (i *BlockIterator) Offset() asm.RawInstructionOffset {
 	return i.offset
 }
 
-func (i *BlockIterator) Clone() *BlockIterator {
-	return &BlockIterator{
-		i.blocks,
-		i.block,
-		i.insns,
-		i.depth,
-		i.ins,
-		i.index,
-		i.offset,
-		i.local,
-	}
-}
-
 // nextBlock pulls the next block by identifier, if it exists. Otherwise,
 // returns false.
 //
@@ -358,62 +337,15 @@ func (i *BlockIterator) nextBlock() bool {
 	if i.block.id+1 >= i.blocks.count() {
 		return false
 	}
-	next := i.blocks[i.block.id+1]
-
-	// Reset loop detection when moving forward since we're primarily concerned
-	// with infinite loops while backtracking continuously. Fallthroughs always
-	// point to the end of the program eventually.
-	i.depth = 0
 
-	i.block = next
+	i.block = i.blocks[i.block.id+1]
 	i.index = i.block.start
 	i.offset = i.block.raw
 	i.ins = &i.insns[i.index]
 
 	return true
 }
 
-// prevBlock pulls the previous block in the control flow if there is exactly
-// one predecessor, otherwise returns false.
-//
-// Sometimes, map pointers are loaded into a register in a previous block.
-// Backtracking into multiple predecessors is not useful since the contents of
-// the pointer register would be ambiguous if assigned from multiple
-// predecessors.
-//
-// Positions the iterator at the end of the previous block. Offset is set to 0
-// since backtracking requires summing up instruction sizes from the start of
-// the block.
-func (i *BlockIterator) prevBlock() bool {
-	if i.block == nil {
-		return false
-	}
-
-	prev := i.block.predecessor()
-	if prev == nil || prev == i.block {
-		return false
-	}
-
-	// Simple loop detection to avoid infinite loops when backtracking
-	// continuously. Depth gets reset when moving forward.
-	if i.depth >= maxDepth {
-		return false
-	}
-	i.depth++
-
-	i.block = prev
-	i.index = i.block.end
-
-	// Raw offset tracking disabled for backtracking since it requires summing
-	// up instruction sizes from the start of the block. Raw offsets are only
-	// used for dumping instructions in forward order.
-	i.offset = 0
-
-	i.ins = &i.insns[i.index]
-
-	return true
-}
-
 // Next advances the iterator to the next instruction in the block. If the end
 // of the block is reached, it will either stop (if iterating locally) or roll
 // over to the next block (if iterating globally).
@@ -445,43 +377,64 @@ func (i *BlockIterator) Next() bool {
 	return true
 }
 
-func (i *BlockIterator) Previous() bool {
-	if i.block == nil || i.index < i.block.start || i.index > i.block.end {
-		return false
+// Backtrack returns a Backtracker starting at the current instruction of the
+// BlockIterator.
+//
+// [Backtracker.Instruction] will return the same instruction as the current
+// instruction of the BlockIterator.
+//
+// [Backtracker.Previous] will return the instruction preceding the current one,
+// if any.
+func (i *BlockIterator) Backtrack() *Backtracker {
+	return &Backtracker{
+		insns: i.insns,
+		stop:  i.block.start,
+		index: i.index,
+		ins:   &i.insns[i.index],
 	}
+}
 
-	if i.ins == nil {
-		i.index = i.block.end
-		i.ins = &i.insns[i.index]
-
-		// Raw offset tracking disabled for backtracking since it requires summing
-		// up instruction sizes from the start of the block. Raw offsets are only
-		// used for dumping instructions in forward order.
-		i.offset = 0
+// Backtracker is an iterator that walks backwards through a Block's
+// instructions.
+//
+// This is useful for finding the last instruction that wrote to a register
+// before it is read, by following the control flow backwards.
+type Backtracker struct {
+	insns asm.Instructions
+	stop  int
 
-		return true
-	}
+	index int
+	ins   *asm.Instruction
+}
 
-	if i.index-1 < i.block.start {
-		if !i.local {
-			// Iterating globally, roll over to the previous block if it exists.
-			return i.prevBlock()
-		}
+// Instruction returns the current instruction.
+func (bt *Backtracker) Instruction() *asm.Instruction {
+	return bt.ins
+}
 
-		// Iterating locally, stop here.
+// Previous moves to the previous instruction within the block.
+// Returns false when reaching the start of the block.
+func (bt *Backtracker) Previous() bool {
+	if bt.index <= bt.stop {
 		return false
 	}
 
-	i.index--
-	i.ins = &i.insns[i.index]
+	// Only decrement if Previous was called before.
+	if bt.ins != nil {
+		bt.index--
+	}
 
-	// Prevent offset underflow.
-	raw := asm.RawInstructionOffset(i.ins.Size() / asm.InstructionSize)
-	i.offset = min(i.offset, i.offset-raw)
+	bt.ins = &bt.insns[bt.index]
 
 	return true
 }
 
+// Clone creates a copy of the Backtracker at its current position.
+func (bt *Backtracker) Clone() *Backtracker {
+	cpy := *bt
+	return &cpy
+}
+
 // getBlock retrieves the block associated with an instruction. It checks both
 // the leader and edge metadata to find the block. If neither is found, it
 // returns nil, indicating that the instruction forms neither the start nor end
@@ -523,13 +476,6 @@ func (bl Blocks) first() *Block {
 	return bl[0]
 }
 
-func (bl Blocks) last() *Block {
-	if len(bl) == 0 {
-		return nil
-	}
-	return bl[len(bl)-1]
-}
-
 func (bl Blocks) iterate(insns asm.Instructions) *BlockIterator {
 	if len(bl) == 0 {
 		return nil

pkg/bpf/analyze/blocks_test.go

@@ -54,65 +54,6 @@ func TestMakeBlocksSimple(t *testing.T) {
 	assert.Equal(t, b, b2)
 }
 
-func TestBlocksMultiplePredecessors(t *testing.T) {
-	// A program with multiple predecessors to the last block.
-	insns := asm.Instructions{
-		asm.Mov.Imm32(asm.R0, 1),
-		asm.JEq.Imm(asm.R0, 0, "target"),
-		asm.Mov.Imm32(asm.R1, 1),
-		asm.JEq.Imm(asm.R1, 0, "target"),
-		asm.Mov.Imm32(asm.R0, 0).WithSymbol("target"),
-		asm.Return(),
-	}
-
-	// Marshal instructions to fix up references.
-	require.NoError(t, insns.Marshal(io.Discard, binary.LittleEndian))
-
-	blocks, err := MakeBlocks(insns)
-	require.NoError(t, err)
-
-	assert.EqualValues(t, 3, blocks.count())
-
-	first := blocks.first()
-	assert.EqualValues(t, 0, first.id)
-	assert.Empty(t, first.predecessors)
-	assert.Equal(t, 0, first.start)
-	assert.Equal(t, 1, first.end)
-	assert.Equal(t, blocks[2], first.branch)
-	assert.Equal(t, blocks[1], first.fthrough)
-
-	second := blocks[1]
-	assert.EqualValues(t, 1, second.id)
-	assert.Len(t, second.predecessors, 1)
-	assert.Equal(t, first, second.predecessors[0])
-	assert.Equal(t, 2, second.start)
-	assert.Equal(t, 3, second.end)
-	assert.Equal(t, blocks[2], second.branch)
-	assert.Equal(t, blocks[2], second.fthrough)
-
-	last := blocks.last()
-	assert.EqualValues(t, 2, last.id)
-	assert.Len(t, last.predecessors, 2)
-	assert.Equal(t, first, last.predecessors[0])
-	assert.Equal(t, second, last.predecessors[1])
-	assert.Equal(t, 4, last.start)
-	assert.Equal(t, 5, last.end)
-	assert.Nil(t, last.branch)
-	assert.Nil(t, last.fthrough)
-
-	// Pull instructions from the last block and make sure it doesn't continue
-	// past the start of the block since it has multiple predecessors.
-	iter := last.iterateGlobal(blocks, insns)
-
-	require.True(t, iter.Previous())
-	assert.Equal(t, iter.Instruction(), &insns[5])
-
-	require.True(t, iter.Previous())
-	assert.Equal(t, iter.Instruction(), &insns[4])
-
-	require.False(t, iter.Previous())
-}
-
 func TestMakeBlocksManyBranches(t *testing.T) {
 	insns := branchingProg(t, 1000)
 
@@ -141,9 +82,6 @@ func TestBlocksIterateLocal(t *testing.T) {
 	assert.True(t, iter.Next())
 	assert.False(t, iter.Next())
 	assert.Equal(t, 0, iter.index)
-
-	// Iterator has nowhere to move. First instruction is already pulled.
-	assert.False(t, iter.Previous())
 }
 
 func TestBlocksIterateGlobal(t *testing.T) {
@@ -169,41 +107,6 @@ func TestBlocksIterateGlobal(t *testing.T) {
 	// We should have seen all instructions.
 	assert.Equal(t, 100, i)
 	assert.Equal(t, 99, iter.index)
-
-	i = 0
-	for ; iter.Previous(); i++ {
-		assert.EqualValues(t, iter.index, iter.ins.Constant)
-	}
-
-	// Iterator should be back at the start.
-	assert.Equal(t, 99, i)
-	assert.Equal(t, 0, iter.index)
-}
-
-func TestBlocksIterateGlobalLoop(t *testing.T) {
-	insns := asm.Instructions{
-		asm.Mov.Imm32(asm.R0, 1).WithSymbol("loop"),
-		asm.JEq.Imm(asm.R0, 0, "exit"),
-		asm.Mov.Imm32(asm.R0, 0),
-		asm.JEq.Imm(asm.R0, 1, "loop"),
-		asm.Return().WithSymbol("exit"),
-	}
-
-	// Marshal instructions to fix up references.
-	require.NoError(t, insns.Marshal(io.Discard, binary.LittleEndian))
-
-	bl, err := MakeBlocks(insns)
-	require.NoError(t, err)
-
-	assert.EqualValues(t, 3, bl.count())
-
-	iter := bl.first().iterateGlobal(bl, insns)
-	for iter.Previous() {
-	}
-
-	// Iterator should be back at the start and depth limit should be hit.
-	assert.Equal(t, 0, iter.index)
-	assert.Equal(t, uint8(maxDepth), iter.depth)
 }
 
 func TestBlocksIterateOffset(t *testing.T) {