Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,57 @@

/**
* Conservative, flow-insensitive analysis for values and functions which may
* depend on {@code GetLocalPlayer()}.
* depend on client-local native values such as {@code GetLocalPlayer()} or
* camera state.
*
* Optimizers use this analysis as a barrier. False positives only cost an
* optimization; false negatives could move synchronized work into a
* client-local control-flow region.
*/
public final class LocalPlayerContextAnalyzer {

/**
* Native return values which may differ between clients during the same
* synchronized execution without requiring user code to mutate local state.
* Event responses are synchronized, while handles and UI/audio/visual state
* made local by user code remain the user's responsibility.
*/
private static final Set<String> CLIENT_LOCAL_VALUE_SOURCES = Set.of(
Comment thread
Frotty marked this conversation as resolved.
// Player identity and values explicitly documented as asynchronous.
"GetLocalPlayer",
"GetLocationZ",

// Camera state belongs to the local client's camera.
"GetCameraMargin",
"GetCameraBoundMinX",
"GetCameraBoundMinY",
"GetCameraBoundMaxX",
"GetCameraBoundMaxY",
"GetCameraField",
"GetCameraTargetPositionX",
"GetCameraTargetPositionY",
"GetCameraTargetPositionZ",
"GetCameraTargetPositionLoc",
"GetCameraEyePositionX",
"GetCameraEyePositionY",
"GetCameraEyePositionZ",
"GetCameraEyePositionLoc",

// Localized data may vary with the client's language.
"GetLocalizedString",
"GetLocalizedHotkey",
"GetObjectName",
Comment on lines +56 to +59

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Seed SkinManagerGetLocalPath as a locality source

When clients use different UI skins, SkinManagerGetLocalPath (declared in common.j:3443) can return different asset paths without map code mutating local state. Because it is omitted here, a condition derived from that path remains untainted and BranchMerger may hoist synchronized work across client-local control flow; include this native and add a behavioral regression covering the branch-merger case.

AGENTS.md reference: AGENTS.md:L63-L63

Useful? React with 👍 / 👎.


// Reforged client-local world and client state.
"BlzGetLocalUnitZ",
Comment thread
Frotty marked this conversation as resolved.
"BlzGetUnitZ",
"BlzGetLocalClientWidth",
"BlzGetLocalClientHeight",
"BlzIsLocalClientActive",
"BlzGetMouseFocusUnit",
Comment thread
Frotty marked this conversation as resolved.
"BlzGetLocale"
);

private final Set<ImVar> localPlayerDependentVars =
Collections.newSetFromMap(new IdentityHashMap<>());
private final Set<ImFunction> localPlayerDependentReturns =
Expand Down Expand Up @@ -75,7 +118,7 @@ public boolean isLocalPlayerDependent(Element element) {
}
if (element instanceof ImFunctionCall) {
ImFunctionCall call = (ImFunctionCall) element;
if (isGetLocalPlayer(call.getFunc())
if (isClientLocalValueSource(call.getFunc())
|| localPlayerDependentReturns.contains(call.getFunc())) {
return true;
}
Expand All @@ -98,12 +141,12 @@ public boolean isLocalPlayerDependent(Element element) {

public boolean functionUsesLocalPlayer(ImFunction function) {
return function != null
&& (isGetLocalPlayer(function) || functionsUsingLocalPlayer.contains(function));
&& (isClientLocalValueSource(function) || functionsUsingLocalPlayer.contains(function));
}

public boolean functionInliningIsLocalPlayerSensitive(ImFunction function) {
return function != null
&& (isGetLocalPlayer(function)
&& (isClientLocalValueSource(function)
|| functionsDirectlyUsingLocalPlayer.contains(function)
|| localPlayerDependentReturns.contains(function));
}
Expand All @@ -113,15 +156,15 @@ public boolean isLocalPlayerDependent(ImVar variable) {
}

public boolean isLocalPlayerSource(ImFunction function) {
return isGetLocalPlayer(function);
return isClientLocalValueSource(function);
}

private void analyze(ImProg prog) {
sourceFacts.add(unknownDispatchSource);
for (ImFunction function : ImHelper.calculateFunctionsOfProg(prog)) {
returnFact(function);
useFact(function);
if (isGetLocalPlayer(function)) {
if (isClientLocalValueSource(function)) {
addLocalPlayerSource(function);
} else if (!function.isNative()) {
indexElement(function.getBody(), function, entryControlFact(function));
Expand Down Expand Up @@ -298,7 +341,7 @@ private void indexFunctionCall(ImFunctionCall call, ImFunction owner, Object con
if (!called.isNative()) {
addEnclosingControlDependency(controlContext, entryControlFact(called));
}
if (isGetLocalPlayer(called)) {
if (isClientLocalValueSource(called)) {
functionsDirectlyUsingLocalPlayer.add(owner);
addLocalPlayerSource(called);
}
Expand Down Expand Up @@ -508,9 +551,9 @@ private Fact(FactKind kind, Object subject) {
}
}

private static boolean isGetLocalPlayer(ImFunction function) {
private static boolean isClientLocalValueSource(ImFunction function) {
return function != null
&& function.isNative()
&& "GetLocalPlayer".equals(function.getName());
&& CLIENT_LOCAL_VALUE_SOURCES.contains(function.getName());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2158,6 +2158,142 @@ public void functionUsingGetLocalPlayerMustNotBeInlined() throws Exception {
"transitive GetLocalPlayer wrappers must remain explicit calls");
}

@Test
public void branchMergerMustNotHoistAcrossClientLocalConditions() throws Exception {
test().lines(
"type unit extends handle",
"package test",
"@extern native GetCameraTargetPositionX() returns real",
"@extern native BlzGetUnitZ(unit whichUnit) returns real",
"@extern native BlzIsLocalClientActive() returns boolean",
"native getUnit() returns unit",
"native print(integer i)",
"integer cameraResult = 0",
"integer unitResult = 0",
"integer activeClientResult = 0",
"init",
" real cameraX = GetCameraTargetPositionX()",
" if cameraX > 0.",
" cameraResult = 41",
" else",
" cameraResult = 41",
" real unitZ = BlzGetUnitZ(getUnit())",
" if unitZ > 0.",
" unitResult = 43",
" else",
" unitResult = 43",
" boolean activeClient = BlzIsLocalClientActive()",
" if activeClient",
" activeClientResult = 53",
" else",
" activeClientResult = 53",
" print(cameraResult)",
" print(unitResult)",
" print(activeClientResult)"
);

String optimized = Files.toString(
new File("test-output/OptimizerTests_branchMergerMustNotHoistAcrossClientLocalConditions_opt.j"),
Charsets.UTF_8);
assertTrue(countOccurrences(optimized, "test_cameraResult = 41") >= 2,
"statements must not be hoisted across a client-local camera condition");
assertTrue(countOccurrences(optimized, "test_unitResult = 43") >= 2,
"statements must not be hoisted across a client-local unit Z condition");
assertTrue(countOccurrences(optimized, "test_activeClientResult = 53") >= 2,
"statements must not be hoisted across local-client activity state");
}

@Test
public void clientLocalNativeValuesAreLocalitySources() {
java.util.Set<String> localValueSources = new java.util.LinkedHashSet<>(java.util.Arrays.asList(
"GetLocalPlayer",
"GetLocationZ",
"GetCameraMargin",
"GetCameraBoundMinX",
"GetCameraBoundMinY",
"GetCameraBoundMaxX",
"GetCameraBoundMaxY",
"GetCameraField",
"GetCameraTargetPositionX",
"GetCameraTargetPositionY",
"GetCameraTargetPositionZ",
"GetCameraTargetPositionLoc",
"GetCameraEyePositionX",
"GetCameraEyePositionY",
"GetCameraEyePositionZ",
"GetCameraEyePositionLoc",
"GetLocalizedString",
"GetLocalizedHotkey",
"GetObjectName",
"BlzGetLocalUnitZ",
"BlzGetUnitZ",
"BlzGetLocalClientWidth",
"BlzGetLocalClientHeight",
"BlzIsLocalClientActive",
"BlzGetMouseFocusUnit",
"BlzGetLocale"
));
java.util.Set<String> intentionallyExcludedSources = new java.util.LinkedHashSet<>(java.util.Arrays.asList(
"BlzGetTriggerPlayerMouseX",
"BlzGetTriggerPlayerKey",
"BlzGetTriggerFrameValue",
"BlzFrameIsVisible",
"BlzGetLocalSpecialEffectX",
"AddLightning",
"MoveLightning",
"LoadEffectHandle",
"LoadLightningHandle",
"LoadFrameHandle",
"GetSoundIsPlaying",
"BlzIsSelectionEnabled"
));
Element trace = Ast.NoExpr();
ImFunctions functions = JassIm.ImFunctions();
java.util.Map<String, ImFunction> functionsByName = new java.util.LinkedHashMap<>();
for (String name : localValueSources) {
ImFunction nativeFunction = nativeIntFunction(trace, name);
functions.add(nativeFunction);
functionsByName.put(name, nativeFunction);
}
for (String name : intentionallyExcludedSources) {
ImFunction nativeFunction = nativeIntFunction(trace, name);
functions.add(nativeFunction);
functionsByName.put(name, nativeFunction);
}
ImProg prog = JassIm.ImProg(
trace,
JassIm.ImVars(),
functions,
JassIm.ImMethods(),
JassIm.ImClasses(),
JassIm.ImTypeClassFuncs(),
new java.util.HashMap<>()
);
LocalPlayerContextAnalyzer analyzer = new LocalPlayerContextAnalyzer(prog);

for (String name : localValueSources) {
assertTrue(analyzer.isLocalPlayerSource(functionsByName.get(name)),
name + " must be treated as a client-local value source");
}
for (String name : intentionallyExcludedSources) {
assertFalse(analyzer.isLocalPlayerSource(functionsByName.get(name)),
name + " is synchronized event data or user-managed local state");
}
}

private static ImFunction nativeIntFunction(Element trace, String name) {
return JassIm.ImFunction(
trace,
name,
JassIm.ImTypeVars(),
JassIm.ImVars(),
TypesHelper.imInt(),
JassIm.ImVars(),
JassIm.ImStmts(),
Collections.singletonList(FunctionFlagEnum.IS_NATIVE)
);
}

@Test(timeOut = 10_000)
public void deeplyNestedIndependentCallsDoNotCauseExponentialLocalPlayerAnalysis() {
String nestedCall = "Player(0)";
Expand Down
Loading