8317376: Minor improvements to the 'this' escape analyzer

[archiecobbs]

Please review several fixes and improvements to the this-escape lint warning analyzer.

The goal here is to apply some relatively simple logical fixes that improve the precision and accuracy of the analyzer, and capture the remaining low-hanging fruit so we can consider the analyzer relatively complete with respect to what's feasible with its current design.

Although the changes are small from a logical point of view, they generate a fairly large patch due to impact of refactoring (sorry!). Most of the patch derives from the first two changes listed below.

The changes are summarized here:

1. Generalize how we categorize references

The Ref class hierarchy models the various ways in which, at any point during the execution of a constructor or some other method/constructor that it invokes, there can be live references to the original object under construction lying around. We then look for places where one of these Ref's might be passed to a subclass method. In other words, the analyzer keeps track of these references and watches what happens to them as the code executes so it can catch them trying to "escape".

Previously the Ref categories were:

• ThisRef - The current instance of the (non-static) method or constructor being analyzed
• OuterRef - The current outer instance of the (non-static) method or constructor being analyzed
• VarRef - A local variable or method parameter currently in scope
• ExprRef - An object reference sitting on top of the Java execution stack
• YieldRef - The current switch expression's yield value(s)
• ReturnRef - The current method's return value(s)

For each of those types, we further classified the "indirection" of the reference, i.e., whether the reference was direct (from the thing itself) or indirect (from something the thing referenced).

The problem with that hierarchy is that we could only track outer instance references that happened to be associated with the current instance. So we might know that this had an outer instance reference, but if we said var x = this we wouldn't know that x had an outer instance reference.

In other words, we should be treating "via an outer instance" as just another flavor of indirection along with "direct" and "indirect".

As a result, with this patch the OuterRef class goes away and a new Indirection enum has been created, with values DIRECT, INDIRECT, and OUTER.

2. Track the types of all references

By keeping track of the actual type of each reference (as opposed to just looking at its compile-time type), we can make more precise calculations. We weren't doing this before.

For example consider this class:

public class Leaker {

    public Leaker() {
        Runnable r = new Runnable() {
            public void run() {
                Leaker.this.mightLeak();
            }
        };
        r.run();   // there is a leak here
    }

    protected void mightLeak() {
        // a subclass could override this method
    }
}

Previously we would not have detected a 'this' escape because we wouldn't have known that variable r is a Leaker$1, which allows us to deduce that r.run() is actually Leaker$1.run() (which we can analyze) instead of just Runnable.run() (which we can't).

Because of this change, some Ref types that were previously singletons are no longer singletons. For example, now there can be more than one ReturnRef in scope at any time, representing different possible types for the method's return value.

3. Handling of non-public outer instances

To eliminate some false positives, we no longer warn if a leak happens solely due to a "non-public" outer instance.

For example, consider this code:

public class Test {

    public Test() {
        System.out.println(new Inner());
    }   

    private class Inner {
        Object getMyOuter() {
            return Test.this;
        }
    }
}

The outer Test instance associated with objects of type Test.Inner is considered "non-public" because it's not directly accessible to the outside world (because Test.Inner is a private class). So while it's true that instances of Test.Inner retain a reference to their enclosing instance, an unrelated method like System.out.println() wouldn't know how to access them. The only way a Test.Inner could leak its outer instance to such a method is if it did so (perhaps indirectly) through some entrée that the method "knows about". Of course, that's possible, but absent any further analysis (which could be added in the future), the cost/reward trade-off doesn't seem worth generating a warning here.

This is an area for future research. In other words: when a Ref is passed to unknown code, when is it worth complaining about a leak, and when should we just be silent? Previously we always complained, but now that we have more information about references (namely, their types) we can be a little more discriminating.

In any case, this decision is encapsulated in a new method triggersUnknownInvokeLeak().

4. Fix tracking of new instances

Previously we were not tracking references from a constructor's "return value", i.e., the new instance. We fix this by pretending that constructors end with return this.

5. Fix tracking of enhanced for() loops

Previously we were not tracking the implicit invocations of iterator(), hasNext(), and next() associated with enhanced for() loops. This patch adds that tracking.

6. Fix leaks via return values from lambdas and method references

Leaks inside lambdas and methods referred-to by method reference were being detected properly, but leaks from the return values of those things were not.

So for example, this code would escape notice:

import java.util.function.Supplier;
public class Test {

    public Test() {
        ((Supplier<Test>)this::self).get().mightLeak();
    }   

    public final Test self() {
        return this;
    }

    protected void mightLeak() {
        // a subclass could override this method
    }
}

This patch fixes this omission.

7. Some minor refactoring to make the code clearer

• Some methods were renamed
• Some bits of code were extracted into separate methods
• Added Ref methods toDirect(), toIndirect(), toOuter(), and fromOuter()

8. Remove some unnecessary suppression

Several *.gmk build files were suppressing this-escape warnings unnecessarily. These have been updated.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8317376: Minor improvements to the 'this' escape analyzer (Enhancement - P4)

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16208/head:pull/16208
$ git checkout pull/16208

Update a local copy of the PR:
$ git checkout pull/16208
$ git pull https://git.openjdk.org/jdk.git pull/16208/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16208

View PR using the GUI difftool:
$ git pr show -t 16208

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16208.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back acobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@archiecobbs The following labels will be automatically applied to this pull request:

• build
• compiler
• net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[openjdk]

@archiecobbs this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8317376
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[mlbridge]

Webrevs

• 04: Full - Incremental (304a92b6)
• 03: Full (e09e4229)
• 02: Full (1de2bb41)
• 01: Full (6023bfe5)
• 00: Full (e92c530a)

[dfuch]

Hummm... I know a few (two) places in java.net.http where we deliberately let this escape. One place is where we set up the double final link between an HttpClientImpl instance and its facade (set at construction time where the facade has a final field pointing at its implementation and the implementation has a final WeakRefeference pointing at its facade, all set up within their constructors). Another place is with the SSLflowDelegate where connect, a protected method implemented on the SSLTube.SSLTubeFlowDelegate subclass is called from within the super class constructor. I assume these were the reason why this-escape analysis had been disabled on java.net.http, and I expect the reason why the analysis can be reenabled by default is because of point 3 above?

[archiecobbs]

I assume these were the reason why this-escape analysis had been disabled on java.net.http, and I expect the reason why the analysis can be reenabled by default is because of point 3 above?

No, the goal here is simply to remove unnecessary build flags. All Java code is compiled with -Werror so if the build still succeeds with the build flag removed then (in theory at least) the flag is no longer needed.

[erikj79]

The build changes look ok.

[bridgekeeper]

@archiecobbs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[archiecobbs]

/pingbot

[openjdk]

@archiecobbs Unknown command pingbot - for a list of valid commands use /help.

[bridgekeeper]

@archiecobbs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[openjdk]

@archiecobbs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8317376: Minor improvements to the 'this' escape analyzer

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 161 new commits pushed to the master branch:

• 9fd7802: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
• 192ec38: 8329595: spurious variable "might not have been initialized" on static final field
• 03e8417: 8329948: Remove string template feature
• ff3e76f: 8330053: JFR: Use LocalDateTime instead ZonedDateTime
• 811aadd: 8324683: Unify AttachListener code for Posix platforms
• 5841cb3: 8330107: Separate out "awt" libraries from Awt2dLibraries.gmk
• 89129e3: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
• 9445047: 8330163: C2: improve CMoveNode::Value() when condition is always true or false
• d2f9a1e: Merge
• 33d7127: 8322122: Enhance generation of addresses
• ... and 151 more: https://git.openjdk.org/jdk/compare/4a11db8b606f2b10f48f0b45335b661fe3095fc4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@erikj79, @vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[vicente-romero-oracle]

looks sensible to me

[archiecobbs]

/integrate

[openjdk]

@archiecobbs
Your change (at version 304a92b) is now ready to be sponsored by a Committer.

[vicente-romero-oracle]

/sponsor

[openjdk]

Going to push as commit 0646284.
Since your change was applied there have been 163 commits pushed to the master branch:

• 4895a15: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object
• fd331ff: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
• 9fd7802: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
• 192ec38: 8329595: spurious variable "might not have been initialized" on static final field
• 03e8417: 8329948: Remove string template feature
• ff3e76f: 8330053: JFR: Use LocalDateTime instead ZonedDateTime
• 811aadd: 8324683: Unify AttachListener code for Posix platforms
• 5841cb3: 8330107: Separate out "awt" libraries from Awt2dLibraries.gmk
• 89129e3: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
• 9445047: 8330163: C2: improve CMoveNode::Value() when condition is always true or false
• ... and 153 more: https://git.openjdk.org/jdk/compare/4a11db8b606f2b10f48f0b45335b661fe3095fc4...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vicente-romero-oracle @archiecobbs Pushed as commit 0646284.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.