interp: mark pointers in aggregate call operands as external - #5586
Open
rdon-key wants to merge 1 commit into
Open
interp: mark pointers in aggregate call operands as external#5586rdon-key wants to merge 1 commit into
rdon-key wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While investigating the difference in interp behavior between
sort.Stringsandsort.Slicereported 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.Sliceis used from a package initializer.Reproducer
The following program reproduces the issue:
The correct result is:
Before this fix, TinyGo produces:
The
sort.Slicecall itself remains at runtime, but interp continues evaluating the following loop using the pre-sort contents ofsorted, and therefore generateslensfrom stale values.Cause
In
runAtRuntime, only call operands whose LLVM type was directly a pointer were passed tomarkExternalStore:However, aggregate values such as interfaces and structs may contain pointers internally.
markExternalalready knows how to recursively inspect structs and arrays, but thePointerTypeKindcheck prevented that logic from being used for aggregate call operands.sort.Slicehits this case because its arguments include an aggregate value containing a pointer.Fix
Pass all call operands to
markExternalStoreand letmarkExternaldetermine whether they contain pointers: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
1is incorrectly constant-folded:After the fix, the external call is correctly treated as potentially modifying
@main.value, so the load and store remain at runtime: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:
After the fix:
For the original
sort.Slicecase 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:
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.