Skip to content

interp: mark pointers in aggregate call operands as external - #5586

Open
rdon-key wants to merge 1 commit into
tinygo-org:devfrom
rdon-key:fix/interp-aggregate-external-store
Open

interp: mark pointers in aggregate call operands as external#5586
rdon-key wants to merge 1 commit into
tinygo-org:devfrom
rdon-key:fix/interp-aggregate-external-store

Conversation

@rdon-key

Copy link
Copy Markdown
Contributor

While investigating the difference in interp behavior between sort.Strings and sort.Slice reported in #5583, I found a case where interp can generate incorrect compile-time values.

When a function call that must remain at runtime receives an aggregate value, such as a struct containing a pointer, the memory referenced by that pointer was not always marked as externally modified.

As a result, interp could continue treating memory that may be modified by the runtime call as compile-time known, and partially evaluate subsequent code using stale values.

I confirmed that this can produce incorrect static data when sort.Slice is used from a package initializer.

Reproducer

The following program reproduces the issue:

package main

import (
	"sort"
	"time"
)

var words = []string{"bb", "a"}
var lens = build(words)

func build(w []string) []int {
	sorted := append(make([]string, 0, len(w)), w...)

	sort.Slice(sorted, func(i, j int) bool {
		return sorted[i] < sorted[j]
	})

	result := make([]int, 2)
	for i, s := range sorted {
		result[i] = len(s)
	}
	return result
}

func main() {
	time.Sleep(2 * time.Second)

	println("a62 PR reproducer")
	println("lens[0]:", lens[0])
	println("lens[1]:", lens[1])
	println("expected: 1 2")
	println("done")
}

The correct result is:

a62 PR reproducer
lens[0]: 1
lens[1]: 2
expected: 1 2
done

Before this fix, TinyGo produces:

a62 PR reproducer
lens[0]: 2
lens[1]: 1
expected: 1 2
done

The sort.Slice call itself remains at runtime, but interp continues evaluating the following loop using the pre-sort contents of sorted, and therefore generates lens from stale values.

Cause

In runAtRuntime, only call operands whose LLVM type was directly a pointer were passed to markExternalStore:

for _, op := range operands {
	if op.Type().TypeKind() == llvm.PointerTypeKind {
		err := mem.markExternalStore(op)
		if err != nil {
			return r.errorAt(inst, err)
		}
	}
}

However, aggregate values such as interfaces and structs may contain pointers internally.

markExternal already knows how to recursively inspect structs and arrays, but the PointerTypeKind check prevented that logic from being used for aggregate call operands.

sort.Slice hits this case because its arguments include an aggregate value containing a pointer.

Fix

Pass all call operands to markExternalStore and let markExternal determine whether they contain pointers:

for _, op := range operands {
	err := mem.markExternalStore(op)
	if err != nil {
		return r.errorAt(inst, err)
	}
}

Scalar values are ignored by markExternal.

For aggregate values such as structs and arrays, pointers contained inside them are recursively processed, allowing memory that may be modified by the runtime call to be marked correctly as externally modified.

Regression test

This PR adds an LLVM IR regression test that passes a struct containing a pointer to an external call and then reads the pointed-to value afterward.

Before the fix, the memory is not marked as externally modified, so the stale value 1 is incorrectly constant-folded:

@main.value = global i32 1
@main.result = local_unnamed_addr global i32 1

define void @runtime.initAll() unnamed_addr {
entry:
  call void @externalAggregate({ ptr } { ptr @main.value })
  ret void
}

After the fix, the external call is correctly treated as potentially modifying @main.value, so the load and store remain at runtime:

@main.value = global i32 1
@main.result = local_unnamed_addr global i32 0

define void @runtime.initAll() unnamed_addr {
entry:
  call void @externalAggregate({ ptr } { ptr @main.value })
  %value = load i32, ptr @main.value, align 4
  store i32 %value, ptr @main.result, align 4
  ret void
}

I verified that the regression test fails without the fix and passes with the fix applied.

RP2040 verification

I tested the reproducer above on an RP2040 using -target=pico.

Before the fix:

a62 PR reproducer
lens[0]: 2
lens[1]: 1
expected: 1 2
done

After the fix:

a62 PR reproducer
lens[0]: 1
lens[1]: 2
expected: 1 2
done

For the original sort.Slice case from #5583, the fix also causes interp to safely fall back to runtime evaluation instead of partially evaluating subsequent code using stale values.

Tests

The interp test suite passes:

make test GOTESTPKGS=./interp

ok github.com/tinygo-org/tinygo/interp

Relation to #5583

This issue was discovered while investigating #5583, but this PR alone does not fully resolve it. The original issue of why sort.Strings cannot be fully evaluated by interp still needs investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant