Remove a redundant PTS updating

[Michael1015198808]

In ReflectiveActionModel's arrayNewInstance, the code first calls newReflectiveObj(context, invoke, arrayType) to create a reflective object for the array, and then calls solver.addVarPointsTo(context, result, csNewArray) to make the result points to this newly allocated object:

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/ReflectiveActionModel.java

Lines 258 to 259 in 21118b5

	CSObj csNewArray = newReflectiveObj(context, invoke, arrayType);
	solver.addVarPointsTo(context, result, csNewArray);

However, newReflectiveObj has already done the updation of the points-to set of result (Line 156), and it also performs a null-check on result to ensure that invocation does contain a left-value.

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/ReflectiveActionModel.java

Lines 149 to 159 in 21118b5

	private CSObj newReflectiveObj(Context context, Invoke invoke, ReferenceType type) {
	Obj newObj = heapModel.getMockObj(REF_OBJ_DESC,
	invoke, type, invoke.getContainer());
	// TODO: double-check if the heap context is proper
	CSObj csNewObj = csManager.getCSObj(context, newObj);
	Var result = invoke.getResult();
	if (result != null) {
	solver.addVarPointsTo(context, result, csNewObj);
	}
	return csNewObj;
	}

Therefore, I think

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/ReflectiveActionModel.java

Line 259 in 21118b5

solver.addVarPointsTo(context, result, csNewArray);

is redundant (because newReflectiveObj will add the points-to relation if result is not null) and dangerous (if the result is null due to some rare corner cases).

Is simply removing it safe? I am not sure if there is any special case that I ignore.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.19%. Comparing base (3f3b802) to head (17e6d18).
⚠️ Report is 21 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
+ Coverage     71.91%   75.19%   +3.28%     
- Complexity     4354     4584     +230     
============================================
  Files           479      480       +1     
  Lines         15856    15927      +71     
  Branches       2175     2181       +6     
============================================
+ Hits          11403    11977     +574     
+ Misses         3571     3078     -493     
+ Partials        882      872      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jjppp]

Just passing by, but I noticed that the assigned context for reflective objects is also suspicious. It bypasses the standard method selector.selectHeapContext() for selecting the context for reflective objects.

This makes reflective objects directly inherit the context of the method containing its allocation-site (limited by the parameter k in k-limiting context-sensitivity), instead of the object context assigned by a context selector (usually k - 1 for k-limiting sensitivity).

This might not be a huge concern for soundness, but it might introduce excessive reflective objects and therefore affect the efficiency of a pointer analysis.

[jjppp]

This redundancy should not be a concern since WorkList internally merges points-to sets for each pointer entry (i.e., adding multiple entries such as <p1, pts1> and <p1, pts2> results in a single entry <p1, pts1 + pts2>).

As for safety, the model of arraysNewInstance already checks if result is null:

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/ReflectiveActionModel.java

Lines 248 to 250 in 21118b5

	if (result == null) {
	return;
	}

[Michael1015198808]

Just passing by, but I noticed that the assigned context for reflective objects is also suspicious. It bypasses the standard method selector.selectHeapContext() for selecting the context for reflective objects.

This makes reflective objects directly inherit the context of the method containing its allocation-site (limited by the parameter k in k-limiting context-sensitivity), instead of the object context assigned by a context selector (usually k - 1 for k-limiting sensitivity).

This might not be a huge concern for soundness, but it might introduce excessive reflective objects and therefore affect the efficiency of a pointer analysis.

Thanks for pointing this out, I think it is an interesting issue, maybe this needs @zhangt2333 to make a further assessment.

This redundancy should not be a concern since WorkList internally merges points-to sets for each pointer entry (i.e., adding multiple entries such as <p1, pts1> and <p1, pts2> results in a single entry <p1, pts1 + pts2>).

As for safety, the model of arraysNewInstance already checks if result is null:

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/ReflectiveActionModel.java

Lines 248 to 250 in 21118b5

	if (result == null) {
	return;
	}

For this part, the 2 calls to solver.addVarPointsTo add exactly the same points-to relation (both the CS var and object are the same). Therefore, removing one will definitely improve the performance (though it may not be a giant boost.)

[zhangt2333]

Thank you for @Michael1015198808's PR.
Thank you for @jjppp's quick response and I completely agree.

I reviewed the three usages of newReflectiveObj, and the other two require using the return value of newReflectiveObj.

I'm not sure if line 259 is meant to maintain consistency with this usage pattern (needs further confirmation from @silverbullettt). If this line is removed, the left-hand side assignment on line 258 could also be deleted.

Regarding the heap selection mentioned by @jjppp, I'm not the author of the reflection analysis related code, but I noticed there's a comment at

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/ReflectiveActionModel.java

Line 152 in 21118b5

// TODO: double-check if the heap context is proper

, so I suspect this design might be intentional.

[silverbullettt]

I agree with @Michael1015198808 that L259 is indeed redundant and can be removed. Although this redundancy should not cause performance or correctness issues, it would still be good to eliminate it. As @zhangt2333 mentioned, if we remove it, csNewArray becomes a dead variable and can also be removed.

As for the heap context issue raised by @jjppp , when ContextSelector handles a non-NewObj object (such as an object created by reflection), it will choose an empty context:

Tai-e/src/main/java/pascal/taie/analysis/pta/core/cs/selector/AbstractContextSelector.java

Lines 42 to 50 in 21118b5

	public Context selectHeapContext(CSMethod method, Obj obj) {
	// Uses different strategies to select heap contexts
	// for different kinds of objects.
	if (obj instanceof NewObj) {
	return selectNewObjContext(method, (NewObj) obj);
	} else {
	return getEmptyContext();
	}
	}

Selecting k-1 as the heap context should be a detail within the ContextSelector, and I don’t want to expose that. Therefore, for precision considerations in reflection, a method context is directly chosen here. Since the proportion of objects created by reflection is not large, it should not cause noticeable performance issues. However, I am not sure which strategy for heap context is best for reflection—it needs analysis of different programs using different context sensitivities, including advanced techniques (like selective context sensitivity, etc.). Therefore, I left the TODO at L152 (mentioned by @zhangt2333). This issue, while not major, certainly deserves further exploration.

Additionally, to ensure correctness before merging this change, perhaps @Michael1015198808 could run pointer analyses on some reflection-intensive programs and then compare whether the results are exactly the same after deleting L259?

[Michael1015198808]

Additionally, to ensure correctness before merging this change, perhaps @Michael1015198808 could run pointer analyses on some reflection-intensive programs and then compare whether the results are exactly the same after deleting L259?

Sure! But I am only an API guy 😢 and not very familiar with different benchmark features. Therefore, I wrote scripts to test on ALL benchmarks provided by BenchmarkRunner. More specifically, my scripts run Tai-e with/without this line and test whether their results are the same. (For simplicity, I only compare the statistics that includes var points-to).
For safeness consideration, I created 2 branches, test-redundant-ref-op-0.5.2 and test-redundant-ref-op-359eb634, based on v0.5.2-SNAPSHOT and 359eb63 (the latest commit on branch master when I started tests). For every version, I tested ci, 2-obj, 2-cfa and 2-type with ALL benchmarks. My result shows that on v0.5.2-SNAPSHOT, removing this line does not affect the analysis result. (On 359eb63, the script is still running)

For an easier reproductivity, I provide 2 scripts, test.sh and test2.sh. The first one runs ALL benchmarks with ALL analyses variants, the second one checks whether their results (with/without the line) are the same.

Update at July 25th

I've finished testing on the latest commit 359eb63 and no difference was detected. But I have to say that I simply run the BenchmarkRunner and did not pay much attention to runtime errors caused by insufficient memory or other things, current tests only focus on those tests that can be successfully executed.

[silverbullettt]

Thank @Michael1015198808! Once csNewArray is removed, this PR will be ready for merging.

[Michael1015198808]

Okay! I removed the unused assignment in the latest commit.