Re: RFR: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 21:26:09 GMT, Chris Plummer wrote: > This seems like a pretty serious flaw with using Lambdas and Cleaners that > probably should be brought up with the libs and language teams. They are aware of this issue; it's even mentioned in [this article](https://inside.java/2022/05/25/clean-cleaner/): > For example, if `chars` is a field, the lambda could refer to `this.chars` > inadvertently capturing `this`. [...] One way to ensure that this is not > captured is to create the lambda in a static method. Its scope does not have > this so it cannot accidentally be captured. As far as I could tell, the cleaners you pointed out use local variables only. As long as the lambda does not reference any object fields or instance methods, `this` is not captured. - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
On Thu, 12 Jan 2023 21:02:23 GMT, Chris Plummer wrote: > There are occurrences of JNI_COMMIT on macos in libawt and libsaproc. Is > there a reason you did not fix these also? My search for JNI_COMMIT filtered out all *.m files. Thanks for pointing that out. libsaproc is corrected now. I will fix AWT in a separate PR. - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0 [v2]
> Please review this patch that fixes a few memory leaks in JNI code. > > [The latest > documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) > of JNI functions makes an explicit note about the use of JNI_COMMIT: > >> If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of >> the elements in `array`, then a final call to >> *ReleaseArrayElements* passing a `mode` argument of "0" or >> `JNI_ABORT`, should be made to free the `elems` buffer > > No new regression test. I manually verified the Linux fix using ClhdsbPstack > test in root mode. Also, tier1-2 tests on mach5 continue to pass. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: MacOS fix - Changes: - all: https://git.openjdk.org/jdk/pull/11963/files - new: https://git.openjdk.org/jdk/pull/11963/files/cecacd13..bea9f536 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11963=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11963=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11963.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11963/head:pull/11963 PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:47:28 GMT, Maurizio Cimadamore wrote: > Ok - I thought false negative was the thing to absolutely avoid - and that > was the no. 1 concern. You're right. I think at the time I reasoned that it would be unusual enough for the type of an expression to start as an instanceof X, then change to not being an instanceof X, and then change back, that it was worth it to go ahead and filter these out. But admittedly that was based on intuition, not science. > I think a possible approach to keep both the filtering and the code more or > less similar to what you have, is to save the type of the expression in the > ExprRef. Then, I believe that, at the end of scan() you can just get whatever > type is there, and check that it's the correct one, if not drop it. That's a nice idea... thanks. I'll play around with it some. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:12:27 GMT, Archie L. Cobbs wrote:
>> Uhm. Turns out I probably did not understand the filter correctly, and now
>> I'm more dubious about what it actually does. Say you have this hierarchy:
>>
>>
>> interface A { }
>> class B {
>> B() {
>> A a = (A)this;
>> ...
>> }
>> }
>> class C extends B implements A { }
>> ```
>>
>> Pathological case, I know. But the filtering will end up dropping the
>> expression Ref on the floor, right? (because B and A are unrelated).
>
>> But the filtering will end up dropping the expression Ref on the floor,
>> right? (because B and A are unrelated).
>
> Ah, I see what you mean.
>
> Here's a more complete example:
>
> public class CastLeak {
>
> public CastLeak() {
> ((CastLeak)(Runnable)this).mightLeak();
> }
>
> public void mightLeak() {
> }
> }
>
> That would be a leak for any subclass that implements `Runnable`. Yet no
> warning is generated.
>
> So the filtering by expression type is going to potentially create false
> negatives. But it also eliminates a bunch of false positives. And the false
> negatives are probably all somewhat pathological like the example above.
>
> So I still think it's worth doing.
Ok - I thought false negative was the thing to absolutely avoid - and that was
the no. 1 concern. I think a possible approach to keep both the filtering and
the code more or less similar to what you have, is to save the type of the
expression in the ExprRef. Then, I believe that, at the end of scan() you can
just get whatever type is there, and check that it's the correct one, if not
drop it.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
> This PR adds a new lint warning category `this-escape`.
>
> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
> allow the JDK to continue to compile with `-Xlint:all`.
>
> A 'this' escape warning is generated for a constructor `A()` in a class `A`
> when the compiler detects that the following situation is _in theory
> possible:_
> * Some subclass `B extends A` exists, and `B` is defined in a separate source
> file (i.e., compilation unit)
> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
> * During the execution of `A()`, some non-static method of `B.foo()` could
> get invoked, perhaps indirectly
>
> In the above scenario, `B.foo()` would execute before `A()` has returned and
> before `B()` has performed any initialization. To the extent `B.foo()`
> accesses any fields of `B` - all of which are still uninitialized - it is
> likely to function incorrectly.
>
> Note, when determining if a 'this' escape is possible, the compiler makes no
> assumptions about code outside of the current compilation unit. It doesn't
> look outside of the current source file to see what might actually happen
> when a method is invoked. It does follow method and constructors within the
> current compilation unit, and applies a simplified
> union-of-all-possible-branches data flow analysis to see where 'this' could
> end up.
>
> From my review, virtually all of the warnings generated in the various JDK
> modules are valid warnings in the sense that a 'this' escape, as defined
> above, is really and truly possible. However, that doesn't imply that any
> bugs were found within the JDK - only that the possibility of a certain type
> of bug exists if certain superclass constructors are used by someone,
> somewhere, someday.
>
> For several "popular" classes, this PR also adds `@implNote`'s to the
> offending constructors so that subclass implementors are made aware of the
> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>
> More details and a couple of motivating examples are given in an included
> [doc
> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
> for some background.
>
> Ideally, over time the owners of the various modules would review their
> `@SuppressWarnings("this-escape")` annotations and determine which other
> constructors also warranted such an `@implNote`.
>
> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch of
> different JDK modules. My apologies for that. Adding these annotations was
> determined to be the more conservative approach, as compared to just
> excepting `this-escape` from various module builds globally.
>
> **Patch Navigation Guide**
>
> * Non-trivial compiler changes:
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>
> * Javadoc additions of `@implNote`:
>
> * `src/java.base/share/classes/java/io/PipedReader.java`
> * `src/java.base/share/classes/java/io/PipedWriter.java`
> * `src/java.base/share/classes/java/lang/Throwable.java`
> * `src/java.base/share/classes/java/util/ArrayDeque.java`
> * `src/java.base/share/classes/java/util/EnumMap.java`
> * `src/java.base/share/classes/java/util/HashSet.java`
> * `src/java.base/share/classes/java/util/Hashtable.java`
> * `src/java.base/share/classes/java/util/LinkedList.java`
> * `src/java.base/share/classes/java/util/TreeMap.java`
> * `src/java.base/share/classes/java/util/TreeSet.java`
>
> * New unit tests
> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>
> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
Archie L. Cobbs has updated the pull request incrementally with 16 additional
commits since the last revision:
- Fix bug where all but the last yeild statement were being ignored.
- Add method RefSet.mapInto() and use to refactor/clean up.
- Fix possible assertion failure when handling if statements.
- Use Symbol methods isSubClass() and isEnclosedBy() instead of homebrew stuff.
Suggested-by: mcimadamore
- Add comment regarding limitations of expresison type filtering.
- Add a few more DISABLED_WARNINGS to unbreak build.
- Clean up handling of switch expressions a bit.
- Remove unused method variant of analyzeTree().
- Avoid all
Re: RFR: 8300012: Remove unused JDI VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object) method [v2]
On Thu, 12 Jan 2023 16:44:44 GMT, Chris Plummer wrote: >> `VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object)` is not >> used. Furthermore it confuses the reader that runs across >> `removeObjectMirror()` calls, because what is actually being called is >> `removeObjectMirror(SoftObjectReference ref)`. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Make removeObjectMirror() private. Looks good and simple. :) Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11957
Re: RFR: JDK-8299957: Enhance error logging in instrument coding with additional jplis_assert_msg
On Thu, 12 Jan 2023 08:10:29 GMT, Matthias Baesken wrote: > There are a few places in the instrument coding where errors occur in our > jtreg test, but the already existing error logging method jplis_assert_msg / > jplis_assert is unfortunately missing so not much details are shown. This > could be enhanced. Looks good. Thank you for fixing it. Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11960
Re: RFR: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 12:08:51 GMT, Daniel Jeliński wrote: > Please review this fix for DwarfParser cleaner. > > The original code registered the cleaner using a lambda that captured a > reference to the parser object; as a result, the object was never GCed, and > the cleaner never ran. > > In this version I moved the lambda creation to a static method, so that it > can't capture a reference to the parser. > > Additionally, the new code uses a static cleaner; the cleaner object creation > and cleanup are heavy operations (every cleaner comes with its own thread), > and it's preferable to use a single shared cleaner where possible. > > I verified manually that the native `destroyDwarfContext` method is called > after this fix. No new automated test. Existing tier1-2 tests continue to > pass. This looks good modulo there can be more issues like that as Chris mentioned. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
On Thu, 12 Jan 2023 09:23:49 GMT, Daniel Jeliński wrote: > Please review this patch that fixes a few memory leaks in JNI code. > > [The latest > documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) > of JNI functions makes an explicit note about the use of JNI_COMMIT: > >> If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of >> the elements in `array`, then a final call to >> *ReleaseArrayElements* passing a `mode` argument of "0" or >> `JNI_ABORT`, should be made to free the `elems` buffer > > No new regression test. I manually verified the Linux fix using ClhdsbPstack > test in root mode. Also, tier1-2 tests on mach5 continue to pass. Marked as reviewed by sspitsyn (Reviewer). Looks good. Thank you for taking care about it. Serguei - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14
On Wed, 11 Jan 2023 06:26:18 GMT, Xue-Lei Andrew Fan wrote: > The sprintf is deprecated in Xcode 14 because of security concerns. The issue > was addressed in [JDK-8296812](https://bugs.openjdk.org/browse/JDK-8296812) > for hotspot impl, and > [JDK-8299378](https://bugs.openjdk.org/browse/JDK-8299378) for building, but > the test case was not covered. The failure was reported in [PR > 11793](https://github.com/openjdk/jdk/pull/11793#issuecomment-1371151565), > while running tier1 testing. > > This patch is trying to find the use of sprintf in test cases, and replace it > with snprintf accordingly. This looks good. Thank you for fixing it. Serguei There are more uses of sprintf in some serviceability folders: src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c: sprintf(fname, "/proc/%d/status", pid); src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c: sprintf(fname, "/proc/%d/maps", ph->pid); src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c: s = filename + sprintf (filename, "%s/.build-id/", debug_file_directory); src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c: s += sprintf (s, "%02x", *data++); src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:s += sprintf (s, "%02x", *data++); src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp: sprintf(errmsg, "%s (hr: 0x%08X)", str, hr); \ src/jdk.management/share/native/libmanagement_ext/management_ext.c: sprintf(errmsg, "errno: %d error: %s\n", errno, msg); src/java.management/share/native/libmanagement/VMManagementImpl.c: sprintf(buf, "%d.%d", major, minor); src/java.management/share/native/libmanagement/management.c:sprintf(errmsg, "errno: %d error: %s\n", errno, msg); src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c: sprintf(msg, "handshake failed - received >%s< - expected >%s<", b, hello); src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c: sprintf(buf, "%d", portNum); src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c: sprintf(ebuf, "ERROR: Peer not allowed to connect: %s\n", src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c:sprintf(buf, "winsock error %d", error); src/jdk.jdwp.agent/windows/native/libjdwp/linker_md.c:sprintf(holder, "%s.dll", fname); - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11935
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:28:12 GMT, Archie L. Cobbs wrote:
>> My point is about who puts ThisRef in the set to begin with. It seems to me
>> that ThisRef is put there at the start of a method analysis. After which,
>> there's several code points where we say "if there's a direct ThisRef in the
>> set, do this, otherwise, if there's an indirect ThisRef in the set, do
>> that". But the ThisRef (whether indirect or direct) seems set once and for
>> all (when we start the analysis, and then inside visitApply).
>>
>> There is also this bit in `visitReference`:
>>
>>
>> case SUPER:
>> if (this.refs.contains(ThisRef.direct()))
>> receiverRefs.add(ThisRef.direct());
>> if (this.refs.contains(ThisRef.indirect()))
>> receiverRefs.add(ThisRef.indirect());
>> break;
>>
>>
>> But this doesn't change what I'm saying - there seems to be a general
>> property when we execute this analysis which says whether the current
>> execution context has a direct `this` or not. This seems to me better
>> captured with a boolean, which is then fed back to all the other various
>> downstream ref creation.
>>
>> The main observation, at least for me, is that the code unifies everything
>> under refs, when in reality there are different aspects:
>>
>> * the set of variables that can point to this, whether directly or
>> indirectly (this is truly a set)
>> * whether the current context has a direct or indirect this (this seems more
>> a flag to me)
>> * whether the expression on top of stack is direct/indirect this reference
>> or not (again, 3 possible values here) - but granted, there `depth` here to
>> take into account, so you probably end up with a set (given that we don't
>> want to model a scope with its own set)
>>
>> When reading the code, seeing set expression like containment checks or
>> removals for things that doesn't seem to be truly sets (unless I'm missing
>> something) was genuinely confusing me.
>
> I get what you're saying - it seems silly to model what is essentially a
> fixed, boolean property with the membership of a singleton in a set field,
> rather than with a simple boolean field.
>
> There is a conceptual trade-off though... a lot of the code relates to
> converting `Ref`'s from one type to another. For example, as pointed out
> above, a method invocation might convert a `ExprRef` to a `ThisRef`, then to
> a `ReturnRef`'s, etc. Having these things all be considered part of the same
> family helps conceptually. The fact that a direct `ThisRef` is a singleton is
> just a coincidence in this way of looking at things.
>
> The benefit is the simplicity of being able to think of the data model as
> "just a set of references".
>
> For example, methods like `RefSet.replaceExprs()` become less elegant (or
> basically impossible) if there have to be special cases for each type of
> reference, whereas currently we can do clean stuff like this:
>
>
> @Override
> public void visitReturn(JCReturn tree) {
> scan(tree.expr);
> refs.replaceExprs(depth, ReturnRef::new);
> }
>
>
> But I'm also realizing now that several places can be cleaned up by taking
> this event further. E.g., we should replace this code:
>
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
>
> with something like this:
>
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
>
>
> I will go through and refactor to do that and clean things up a little more.
>
>> When reading the code, seeing set expression like containment checks or
>> removals for things that doesn't seem to be truly sets (unless I'm missing
>> something) was genuinely confusing me.
>
> Probably my fault for not providing better documentation of the overall "set
> of references" conceptual approach. FWIW I added a little bit more in
> f83a9cf0.
> ```java
> ```java
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
> ```
>
>
>
>
>
>
>
>
>
>
>
> with something like this:
> ```java
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
> ```
> ```
This sounds like a good idea, that idiom is quite widespread, so it should help
avoiding repetition.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:01:10 GMT, Maurizio Cimadamore
wrote:
>> The code you quoted has nothing specifically to do with method invocations.
>> This code is simply handing the evaluation of the expressions `this` and
>> `super`. For example, `this` could just be a parameter we're passing to
>> another method.
>>
>> When `this` or `super` expressions are evaluated, the thing left on top of
>> the stack is a direct/indirect reference (i.e., an `ExprRef`) exactly when
>> the there is a direct/indirect reference of type `ThisRef` in the current
>> reference set.
>>
>> In cases where `this` is then "invoked", e.g., `this()`, the `ExprRef` (top
>> of Java stack) becomes the method receiver, and when the method is "invoked"
>> it turns back into a `ThisRef` (see earlier question).
>>
>> Regarding the optimization you mention, in light of the above I'm not sure
>> it would still work.
>
> My point is about who puts ThisRef in the set to begin with. It seems to me
> that ThisRef is put there at the start of a method analysis. After which,
> there's several code points where we say "if there's a direct ThisRef in the
> set, do this, otherwise, if there's an indirect ThisRef in the set, do that".
> But the ThisRef (whether indirect or direct) seems set once and for all (when
> we start the analysis, and then inside visitApply).
>
> There is also this bit in `visitReference`:
>
>
> case SUPER:
> if (this.refs.contains(ThisRef.direct()))
> receiverRefs.add(ThisRef.direct());
> if (this.refs.contains(ThisRef.indirect()))
> receiverRefs.add(ThisRef.indirect());
> break;
>
>
> But this doesn't change what I'm saying - there seems to be a general
> property when we execute this analysis which says whether the current
> execution context has a direct `this` or not. This seems to me better
> captured with a boolean, which is then fed back to all the other various
> downstream ref creation.
>
> The main observation, at least for me, is that the code unifies everything
> under refs, when in reality there are different aspects:
>
> * the set of variables that can point to this, whether directly or indirectly
> (this is truly a set)
> * whether the current context has a direct or indirect this (this seems more
> a flag to me)
> * whether the expression on top of stack is direct/indirect this reference or
> not (again, 3 possible values here) - but granted, there `depth` here to take
> into account, so you probably end up with a set (given that we don't want to
> model a scope with its own set)
>
> When reading the code, seeing set expression like containment checks or
> removals for things that doesn't seem to be truly sets (unless I'm missing
> something) was genuinely confusing me.
I get what you're saying - it seems silly to model what is essentially a fixed,
boolean property with the membership of a singleton in a set field, rather than
with a simple boolean field.
There is a conceptual trade-off though... a lot of the code relates to
converting `Ref`'s from one type to another. For example, as pointed out above,
a method invocation might convert a `ExprRef` to a `ThisRef`, then to a
`ReturnRef`'s, etc. Having these things all be considered part of the same
family helps conceptually. The fact that a direct `ThisRef` is a singleton is
just a coincidence in this way of looking at things.
The benefit is the simplicity of being able to think of the data model as "just
a set of references".
For example, methods like `RefSet.replaceExprs()` become less elegant (or
basically impossible) if there have to be special cases for each type of
reference, whereas currently we can do clean stuff like this:
@Override
public void visitReturn(JCReturn tree) {
scan(tree.expr);
refs.replaceExprs(depth, ReturnRef::new);
}
But I'm also realizing now that several places can be cleaned up by taking this
event further. E.g., we should replace this code:
if (refs.contains(ThisRef.direct()))
outerRefs.add(OuterRef.direct());
if (refs.contains(ThisRef.indirect()))
outerRefs.add(OuterRef.indirect());
with something like this:
refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
I will go through and refactor to do that and clean things up a little more.
> When reading the code, seeing set expression like containment checks or
> removals for things that doesn't seem to be truly sets (unless I'm missing
> something) was genuinely confusing me.
Probably my fault for not providing better documentation of the overall "set of
references" conceptual approach. FWIW I added a little bit more in f83a9cf0.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 12:08:51 GMT, Daniel Jeliński wrote: > Please review this fix for DwarfParser cleaner. > > The original code registered the cleaner using a lambda that captured a > reference to the parser object; as a result, the object was never GCed, and > the cleaner never ran. > > In this version I moved the lambda creation to a static method, so that it > can't capture a reference to the parser. > > Additionally, the new code uses a static cleaner; the cleaner object creation > and cleanup are heavy operations (every cleaner comes with its own thread), > and it's preferable to use a single shared cleaner where possible. > > I verified manually that the native `destroyDwarfContext` method is called > after this fix. No new automated test. Existing tier1-2 tests continue to > pass. This seems like a pretty serious flaw with using Lambdas and Cleaners that probably should be brought up with the libs and language teams. I see other occurrences of this issue in these files (I may have missed some): src/java.base/share/classes/com/sun/crypto/provider/*Key.java (4 files) src/java.base/windows/classes/java/lang/ProcessImpl.java src/java.desktop/share/classes/javax/swing/border/TitledBorder.java src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:24:50 GMT, Archie L. Cobbs wrote:
>> This patch passes all tests:
>>
>>
>> diff --git
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>>
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> index 9d35c2fbc0a..e755c54d0c8 100644
>> ---
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> +++
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> @@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> }
>>
>> // If the expression type is incompatible with 'this', discard
>> it
>> -if (type != null && !this.isSubtype(this.targetClass.sym.type,
>> type))
>> +if (type != null && !this.targetClass.sym.isSubClass(type.tsym,
>> types))
>> this.refs.remove(ExprRef.direct(this.depth));
>> }
>> }
>> @@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> if (explicitOuterThis != null) {
>> this.scan(explicitOuterThis);
>> this.refs.removeExprs(this.depth, direct -> outerRefs.add(new
>> OuterRef(direct)));
>> -} else if (this.types.hasOuterClass(type, this.methodClass.type)) {
>> +} else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
>> if (this.refs.contains(ThisRef.direct()))
>> outerRefs.add(OuterRef.direct());
>> if (this.refs.contains(ThisRef.indirect()))
>> @@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> // Explicit outer 'this' reference?
>> final Type selectedType = this.types.erasure(tree.selected.type);
>> if (selectedType.hasTag(CLASS)) {
>> -final Type.ClassType selectedClassType =
>> (Type.ClassType)selectedType;
>> if (tree.name == this.names._this &&
>> -this.types.hasOuterClass(currentClassType,
>> selectedClassType)) {
>> +
>> currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
>> if (this.refs.contains(OuterRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>> if (this.refs.contains(OuterRef.indirect()))
>> @@ -895,9 +894,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> final MethodSymbol sym = (MethodSymbol)tree.sym;
>>
>> // Check for implicit 'this' reference
>> -final Type.ClassType currentClassType =
>> (Type.ClassType)this.methodClass.sym.type;
>> -final Type methodOwnerType = sym.owner.type;
>> -if (this.isSubtype(currentClassType, methodOwnerType)) {
>> +if (methodClass.sym.isSubClass(sym.owner, types)) {
>> if (this.refs.contains(ThisRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>> if (this.refs.contains(ThisRef.indirect()))
>> @@ -906,7 +903,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> }
>>
>> // Check for implicit outer 'this' reference
>> -if (this.types.hasOuterClass(currentClassType,
>> methodOwnerType)) {
>> +if (methodClass.sym.isEnclosedBy((ClassSymbol)sym.owner)) {
>> if (this.refs.contains(OuterRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>>
>>
>> Btw, I believe a similar trick can be used in
>> TreeInfo.isExplicitThisReference. If I'm correct, `hasOuterClass` should
>> probably be removed as it duplicates already existing functionality.
>>
>> Since I'm bringing this up, as TreeInfo.isExplicitThisReference is only used
>> by the new analyzer, it would be cleaner if it was defined there, at least
>> until it's clear it might be needed by some other client.
>
> Weird. I don't get that build failure.
>
> Neither does github... I have been relying on the github build "Actions"
> succeeding to determine if "the build works" and according to that it is
> building.
>
> I will add the `DISABLED_WARNINGS` in any case.
Thanks for the patch!
The semantics of `hasOuterClass()` returns false if A and B are the same class,
while `isEnclosedBy()` returns true if A and B are the same class.
However, I don't think it would actually matter in practice...
Regardless, I'll add the extra equality comparison and apply your patch and
also the suggestions to integrate `isExplicitThisReference()` and eliminate
`hasOuterClass()`.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:48:25 GMT, Maurizio Cimadamore wrote: >>> I guess what I'm thinking about: >> >> No leak is possible in that example. >> * `new Foo()` creates an instance of `Foo` (not a subclass of `Foo`) >> therefore `m()` is not overridden >> * Any subclass of `Foo` that may exist in the outside world cannot use the >> `Foo()` constructor that leaks because it's private > > but what if `m` is a static method in a separate compilation unit? Should it > be able to observe a partially initialized Foo? Caring about the proper initialization of any class in the _current_ compilation unit is an explicit non-goal. We only care about bugs where a superclass and subclass are in separate compilation units. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
On Thu, 12 Jan 2023 09:23:49 GMT, Daniel Jeliński wrote: > Please review this patch that fixes a few memory leaks in JNI code. > > [The latest > documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) > of JNI functions makes an explicit note about the use of JNI_COMMIT: > >> If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of >> the elements in `array`, then a final call to >> *ReleaseArrayElements* passing a `mode` argument of "0" or >> `JNI_ABORT`, should be made to free the `elems` buffer > > No new regression test. I manually verified the Linux fix using ClhdsbPstack > test in root mode. Also, tier1-2 tests on mach5 continue to pass. There are occurrences of JNI_COMMIT on macos in libawt and libsaproc. Is there a reason you did not fix these also? - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
On Thu, 12 Jan 2023 09:23:49 GMT, Daniel Jeliński wrote: > Please review this patch that fixes a few memory leaks in JNI code. > > [The latest > documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) > of JNI functions makes an explicit note about the use of JNI_COMMIT: > >> If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of >> the elements in `array`, then a final call to >> *ReleaseArrayElements* passing a `mode` argument of "0" or >> `JNI_ABORT`, should be made to free the `elems` buffer > > No new regression test. I manually verified the Linux fix using ClhdsbPstack > test in root mode. Also, tier1-2 tests on mach5 continue to pass. Marked as reviewed by amenkov (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:40:38 GMT, Maurizio Cimadamore
wrote:
>> This patch:
>>
>>
>> diff --git a/make/test/BuildMicrobenchmark.gmk
>> b/make/test/BuildMicrobenchmark.gmk
>> index 1c89328a388..7c3f0293edc 100644
>> --- a/make/test/BuildMicrobenchmark.gmk
>> +++ b/make/test/BuildMicrobenchmark.gmk
>> @@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
>> TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
>> SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
>> INCLUDE_FILES := indify/Indify.java, \
>> -DISABLED_WARNINGS := rawtypes serial options, \
>> +DISABLED_WARNINGS := this-escape rawtypes serial options, \
>> BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
>> JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
>> ))
>> @@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation,
>> BUILD_JDK_MICROBENCHMARK, \
>> TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
>> SMALL_JAVA := false, \
>> CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
>> -DISABLED_WARNINGS := processing rawtypes cast serial preview, \
>> +DISABLED_WARNINGS := this-escape processing rawtypes cast serial
>> preview, \
>> SRC := $(MICROBENCHMARK_SRC), \
>> BIN := $(MICROBENCHMARK_CLASSES), \
>> JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
>> diff --git
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>>
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> index 9d35c2fbc0a..4e2b1e558e7 100644
>> ---
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> +++
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> @@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> // Check for implicit 'this' reference
>> final Type.ClassType currentClassType =
>> (Type.ClassType)this.methodClass.sym.type;
>> final Type methodOwnerType = sym.owner.type;
>> +//if (currentClassType.tsym.isSubClass(sym.owner, types)) {
>> if (this.isSubtype(currentClassType, methodOwnerType)) {
>> if (this.refs.contains(ThisRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>> @@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>> }
>>
>> // Check for implicit outer 'this' reference
>> +// if
>> (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner)) {
>> if (this.types.hasOuterClass(currentClassType,
>> methodOwnerType)) {
>> if (this.refs.contains(OuterRef.direct()))
>> this.refs.add(ExprRef.direct(this.depth));
>>
>>
>> Fixes the build failure.
>
> This patch passes all tests:
>
>
> diff --git
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> index 9d35c2fbc0a..e755c54d0c8 100644
> ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> @@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> }
>
> // If the expression type is incompatible with 'this', discard it
> -if (type != null && !this.isSubtype(this.targetClass.sym.type,
> type))
> +if (type != null && !this.targetClass.sym.isSubClass(type.tsym,
> types))
> this.refs.remove(ExprRef.direct(this.depth));
> }
> }
> @@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> if (explicitOuterThis != null) {
> this.scan(explicitOuterThis);
> this.refs.removeExprs(this.depth, direct -> outerRefs.add(new
> OuterRef(direct)));
> -} else if (this.types.hasOuterClass(type, this.methodClass.type)) {
> +} else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
> if (this.refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (this.refs.contains(ThisRef.indirect()))
> @@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
> // Explicit outer 'this' reference?
> final Type selectedType = this.types.erasure(tree.selected.type);
> if (selectedType.hasTag(CLASS)) {
> -final Type.ClassType selectedClassType =
> (Type.ClassType)selectedType;
> if (tree.name == this.names._this &&
> -this.types.hasOuterClass(currentClassType,
> selectedClassType)) {
> +
> currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
> if (this.refs.contains(OuterRef.direct()))
> this.refs.add(ExprRef.direct(this.depth));
> if
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:40:36 GMT, Maurizio Cimadamore
wrote:
> But the filtering will end up dropping the expression Ref on the floor,
> right? (because B and A are unrelated).
Ah, I see what you mean.
Here's a more complete example:
public class CastLeak {
public CastLeak() {
((CastLeak)(Runnable)this).mightLeak();
}
public void mightLeak() {
}
}
That would be a leak for any subclass that implements `Runnable`. Yet no
warning is generated.
So the filtering by expression type is going to potentially create false
negatives. But it also eliminates a bunch of false positives. And the false
negatives are probably all somewhat pathological like the example above.
So I still think it's worth doing.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:33:48 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 875:
>>
>>> 873: // Reference to this?
>>> 874: if (tree.name == names._this || tree.name == names._super) {
>>> 875: if (this.refs.contains(ThisRef.direct()))
>>
>> This idiom occurs quite a lot. If I'm correct, this basically amounts at
>> asking as to whether the receiver of the method we're currently evaluating
>> is direct or not (which is an invariant, given a method body - e.g. for a
>> given method this "fact" should stay the same). If that's the case, perhaps
>> capturing this in a flag could be better - then you could have just have a
>> single method e.g. `XYZRef.create(boolean direct)`, and remove the branching
>> (here and elsewhere).
>
> The code you quoted has nothing specifically to do with method invocations.
> This code is simply handing the evaluation of the expressions `this` and
> `super`. For example, `this` could just be a parameter we're passing to
> another method.
>
> When `this` or `super` expressions are evaluated, the thing left on top of
> the stack is a direct/indirect reference (i.e., an `ExprRef`) exactly when
> the there is a direct/indirect reference of type `ThisRef` in the current
> reference set.
>
> In cases where `this` is then "invoked", e.g., `this()`, the `ExprRef` (top
> of Java stack) becomes the method receiver, and when the method is "invoked"
> it turns back into a `ThisRef` (see earlier question).
>
> Regarding the optimization you mention, in light of the above I'm not sure it
> would still work.
My point is about who puts ThisRef in the set to begin with. It seems to me
that ThisRef is put there at the start of a method analysis. After which,
there's several code points where we say "if there's a direct ThisRef in the
set, do this, otherwise, if there's an indirect ThisRef in the set, do that".
But the ThisRef (whether indirect or direct) seems set once and for all (when
we start the analysis, and then inside visitApply).
There is also this bit in `visitReference`:
case SUPER:
if (this.refs.contains(ThisRef.direct()))
receiverRefs.add(ThisRef.direct());
if (this.refs.contains(ThisRef.indirect()))
receiverRefs.add(ThisRef.indirect());
break;
But this doesn't change what I'm saying - there seems to be a general property
when we execute this analysis which says whether the current execution context
has a direct `this` or not. This seems to me better captured with a boolean,
which is then fed back to all the other various downstream ref creation.
The main observation, at least for me, is that the code unifies everything
under refs, when in reality there are different aspects:
* the set of variables that can point to this, whether directly or indirectly
(this is truly a set)
* whether the current context has a direct or indirect this (this seems more a
flag to me)
* whether the expression on top of stack is direct/indirect this reference or
not (again, 3 possible values here) - but granted, there `depth` here to take
into account, so you probably end up with a set (given that we don't want to
model a scope with its own set)
When reading the code, seeing set expression like containment checks or
removals for things that doesn't seem to be truly sets (unless I'm missing
something) was genuinely confusing me.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:29:22 GMT, Maurizio Cimadamore
wrote:
>> I put it there because of switch expressions and `yeild`... ?
>
> Well, yield can... yield a value - `case` doesn't. So I'm confused. Also
> because the variable is called `referenceExpressionNode` and `CASE` is not an
> expression. Can `CASE` leave anything on the stack? YIELD does, but CASE?
It's just an artifact of the way switch expressions are handled:
@Override
public void visitSwitchExpression(JCSwitchExpression tree) {
visitScoped(tree, true, t -> {
scan(t.selector);
refs.discardExprs(depth);
RefSet combinedRefs = new RefSet<>();
for (List cases = t.cases; cases.nonEmpty(); cases =
cases.tail) {
scan(cases.head);
combinedRefs.addAll(refs.removeExprs(depth));
}
refs.addAll(combinedRefs);
});
}
@Override
public void visitCase(JCCase tree) {
scan(tree.stats); // no need to scan labels
}
After scanning a switch expression case, the `yield`'ed value will be on the
top of the stack.
Then `visitCase` does NOT remove it, so that it can be picked up and removed by
`visitSwitchExpression()`.
But this can be cleaned up a little bit, by having `visitSwitchExpression()`
not delegate to `visitCase()` but rather iterate through the case statements
itself directly. Then we can remove the `CASE` as you suggest.
Also I realized we need to handle `yield`'ed values like `return` values, i.e.,
collect and remember them until the entire case is complete.
I'll fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:37:06 GMT, Archie L. Cobbs wrote:
>> I guess what I'm thinking about:
>>
>> class Foo {
>> private Foo() {
>> m(this);
>> }
>>
>> public void m() { ... } // overridable
>>
>> static Foo makeFoo() { return new Foo(); }
>> }
>
>> I guess what I'm thinking about:
>
> No leak is possible in that example.
> * `new Foo()` creates an instance of `Foo` (not a subclass of `Foo`)
> therefore `m()` is not overridden
> * Any subclass of `Foo` that may exist in the outside world cannot use the
> `Foo()` constructor that leaks because it's private
but what if `m` is a static method in a separate compilation unit? Should it be
able to observe a partially initialized Foo?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:18:38 GMT, Maurizio Cimadamore
wrote:
>> I can't seem to be able to run tests - I get failures in the build:
>>
>>
>> * For target support_test_micro_tools-classes__the.BUILD_INDIFY_batch:
>
> This patch:
>
>
> diff --git a/make/test/BuildMicrobenchmark.gmk
> b/make/test/BuildMicrobenchmark.gmk
> index 1c89328a388..7c3f0293edc 100644
> --- a/make/test/BuildMicrobenchmark.gmk
> +++ b/make/test/BuildMicrobenchmark.gmk
> @@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
> TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
> SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
> INCLUDE_FILES := indify/Indify.java, \
> -DISABLED_WARNINGS := rawtypes serial options, \
> +DISABLED_WARNINGS := this-escape rawtypes serial options, \
> BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
> JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
> ))
> @@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation,
> BUILD_JDK_MICROBENCHMARK, \
> TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
> SMALL_JAVA := false, \
> CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
> -DISABLED_WARNINGS := processing rawtypes cast serial preview, \
> +DISABLED_WARNINGS := this-escape processing rawtypes cast serial
> preview, \
> SRC := $(MICROBENCHMARK_SRC), \
> BIN := $(MICROBENCHMARK_CLASSES), \
> JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
> diff --git
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> index 9d35c2fbc0a..4e2b1e558e7 100644
> ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> @@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> // Check for implicit 'this' reference
> final Type.ClassType currentClassType =
> (Type.ClassType)this.methodClass.sym.type;
> final Type methodOwnerType = sym.owner.type;
> +//if (currentClassType.tsym.isSubClass(sym.owner, types)) {
> if (this.isSubtype(currentClassType, methodOwnerType)) {
> if (this.refs.contains(ThisRef.direct()))
> this.refs.add(ExprRef.direct(this.depth));
> @@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
> }
>
> // Check for implicit outer 'this' reference
> +// if
> (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner)) {
> if (this.types.hasOuterClass(currentClassType, methodOwnerType))
> {
> if (this.refs.contains(OuterRef.direct()))
> this.refs.add(ExprRef.direct(this.depth));
>
>
> Fixes the build failure.
This patch passes all tests:
diff --git
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
index 9d35c2fbc0a..e755c54d0c8 100644
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
@@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
}
// If the expression type is incompatible with 'this', discard it
-if (type != null && !this.isSubtype(this.targetClass.sym.type,
type))
+if (type != null && !this.targetClass.sym.isSubClass(type.tsym,
types))
this.refs.remove(ExprRef.direct(this.depth));
}
}
@@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
if (explicitOuterThis != null) {
this.scan(explicitOuterThis);
this.refs.removeExprs(this.depth, direct -> outerRefs.add(new
OuterRef(direct)));
-} else if (this.types.hasOuterClass(type, this.methodClass.type)) {
+} else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
if (this.refs.contains(ThisRef.direct()))
outerRefs.add(OuterRef.direct());
if (this.refs.contains(ThisRef.indirect()))
@@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
// Explicit outer 'this' reference?
final Type selectedType = this.types.erasure(tree.selected.type);
if (selectedType.hasTag(CLASS)) {
-final Type.ClassType selectedClassType =
(Type.ClassType)selectedType;
if (tree.name == this.names._this &&
-this.types.hasOuterClass(currentClassType, selectedClassType))
{
+
currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
if (this.refs.contains(OuterRef.direct()))
this.refs.add(ExprRef.direct(this.depth));
if
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 16:40:33 GMT, Maurizio Cimadamore wrote: > I guess what I'm thinking about: No leak is possible in that example. * `new Foo()` creates an instance of `Foo` (not a subclass of `Foo`) therefore `m()` is not overridden * Any subclass of `Foo` that may exist in the outside world cannot use the `Foo()` constructor that leaks because it's private - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 18:11:01 GMT, Maurizio Cimadamore
wrote:
>> Same comment as previous: I don't quite know what I'm doing and I'm loathe
>> to break what is already working. Do you have a suggested patch?
>
> I can't seem to be able to run tests - I get failures in the build:
>
>
> * For target support_test_micro_tools-classes__the.BUILD_INDIFY_batch:
This patch:
diff --git a/make/test/BuildMicrobenchmark.gmk
b/make/test/BuildMicrobenchmark.gmk
index 1c89328a388..7c3f0293edc 100644
--- a/make/test/BuildMicrobenchmark.gmk
+++ b/make/test/BuildMicrobenchmark.gmk
@@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
INCLUDE_FILES := indify/Indify.java, \
-DISABLED_WARNINGS := rawtypes serial options, \
+DISABLED_WARNINGS := this-escape rawtypes serial options, \
BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
))
@@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation, BUILD_JDK_MICROBENCHMARK,
\
TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
SMALL_JAVA := false, \
CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
-DISABLED_WARNINGS := processing rawtypes cast serial preview, \
+DISABLED_WARNINGS := this-escape processing rawtypes cast serial preview, \
SRC := $(MICROBENCHMARK_SRC), \
BIN := $(MICROBENCHMARK_CLASSES), \
JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
diff --git
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
index 9d35c2fbc0a..4e2b1e558e7 100644
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
@@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
// Check for implicit 'this' reference
final Type.ClassType currentClassType =
(Type.ClassType)this.methodClass.sym.type;
final Type methodOwnerType = sym.owner.type;
+//if (currentClassType.tsym.isSubClass(sym.owner, types)) {
if (this.isSubtype(currentClassType, methodOwnerType)) {
if (this.refs.contains(ThisRef.direct()))
this.refs.add(ExprRef.direct(this.depth));
@@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
}
// Check for implicit outer 'this' reference
+// if (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner))
{
if (this.types.hasOuterClass(currentClassType, methodOwnerType)) {
if (this.refs.contains(OuterRef.direct()))
this.refs.add(ExprRef.direct(this.depth));
Fixes the build failure.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:28:12 GMT, Maurizio Cimadamore wrote: > This might also be related with the fact that we deal with return values in > different ways than with e.g. values returned from a nested scope (where we > just pop, and then copy all pending expression to the outer depth). Yes, a method return value that represents a reference is an `ExprRef` just before the `return` is actually executed, but then it turns into a `ReturnRef`. A `ReturnRef` is really just a yellow sticky note that reminds us "Hey whenever you finish 'executing' this method, remember that there is a reference in its return value". > This might also be related with the fact that we deal with return values in > different ways than with e.g. values returned from a nested scope (where we > just pop, and then copy all pending expression to the outer depth). Exactly. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:48:37 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 909:
>>
>>> 907:
>>> 908: // Check for implicit outer 'this' reference
>>> 909: if (this.types.hasOuterClass(currentClassType,
>>> methodOwnerType)) {
>>
>> Similarly here - look for `Symbol.isEnclosedBy`
>
> Same comment as previous: I don't quite know what I'm doing and I'm loathe to
> break what is already working. Do you have a suggested patch?
I can't seem to be able to run tests - I get failures in the build:
* For target support_test_micro_tools-classes__the.BUILD_INDIFY_batch:
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:39:05 GMT, Vicente Romero wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 200:
>
>> 198: //
>> 199:
>> 200: public void analyzeTree(Env env) {
>
> nit: this method could be removed in favor of the overloaded version below
Thanks - will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 204:
>
>> 202: }
>> 203:
>> 204: public void analyzeTree(Env env, JCTree tree) {
>
> nit: `env` parameter doesn't seem to be used could be dropped I think
Thanks - will fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:26:27 GMT, Maurizio Cimadamore wrote: > Do we really need a set for this? There are surely other ways to model things. But I got myself really confused trying to build more complicated models. What I ended up with is this simple model that works: * There is a set of `Ref` subclasses that model the various types of 'this' references possible: `OuterRef`, `ExprRef`, etc. * There is a singleton `this.refs`, which just is a `Set`, containing the 'this' references that are alive at the current moment * As we "execute" code, all we need to do is update the `this.refs` based on what the current bit of code does E.g When a variable is assigned a value that has a reference, we add a `VarRef` for that variable. When we leave a scope, we remove any `Ref`'s that are no longer in scope. Before executing a method, we add `Ref`'s for the method receiver and parameter. When we return from a non-void method, we convert any `ReturnRef` into a `ExprRef`. Etc. THAT I can understand. I don't see converting the above into a bitmap or whatever as worth the additional complexity. We're not programming in perl here. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:17:32 GMT, Maurizio Cimadamore wrote: > There is a concept of push/popScope and then there's a separate concept of > call stack (which is just a list of diagnostic position up to the point). I > wonder if this could be better modeled by using a single class e.g. > Scope/Frame which has a diagnostic position, plus other useful things. I think that would be more confusing, because they're really two different animals. Scopes only have meaning within a single method. They simply serve to bracket the lifetimes of `VarRef` references, following Java curly brace scope. When you "invoke" a method, you put aside the current stack of scopes and start over with an empty scope stack. They don't bridge between methods. Call stacks are just a list of method+code position therein, and they exist outside of any single method. They have nothing to do with Java scopes defined by curly braces. > Perhaps it might even be helpful to have a ref set on each scope, so that you > don't have to attach a "depth" to each ref - the depth of the ref would be > determined by the "scope" in which it appears. That's another way to skin the same cat... in fact I had a design like that before but then realized it was a lot simpler to just carry around one `RefSet`. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 12:15:17 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 175:
>
>> 173: private DiagnosticPosition[] pendingWarning;
>> 174:
>> 175: // These fields are scoped to the CONSTRUCTOR OR INVOKED METHOD BEING
>> ANALYZED
>
> Watch out for "all caps"
Will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 900:
>
>> 898: final Type.ClassType currentClassType =
>> (Type.ClassType)this.methodClass.sym.type;
>> 899: final Type methodOwnerType = sym.owner.type;
>> 900: if (this.isSubtype(currentClassType, methodOwnerType)) {
>
> I believe what you need here is not subtyping but subclassing - see
> `Symbol.isSubclass`
Hmm, I tried using `Symbol.isSubclass()` but it caused test failures. Obviously
I don't quite know what I'm doing and I'm loathe to break what is already
working. Do you have a suggested patch?
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 909:
>
>> 907:
>> 908: // Check for implicit outer 'this' reference
>> 909: if (this.types.hasOuterClass(currentClassType,
>> methodOwnerType)) {
>
> Similarly here - look for `Symbol.isEnclosedBy`
Same comment as previous: I don't quite know what I'm doing and I'm loathe to
break what is already working. Do you have a suggested patch?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 17:13:55 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 411:
>>
>>> 409: final boolean referenceExpressionNode;
>>> 410: switch (tree.getTag()) {
>>> 411: case CASE:
>>
>> surprised to see `CASE` here - as that's not an expression
>
> I put it there because of switch expressions and `yeild`... ?
Well, yield can... yield a value - `case` doesn't. So I'm confused. Also
because the variable is called `referenceExpressionNode` and `CASE` is not an
expression. Can `CASE` leave anything on the stack? YIELD does, but CASE?
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 454:
>>
>>> 452:
>>> 453: // If the expression type is incompatible with 'this',
>>> discard it
>>> 454: if (type != null &&
>>> !this.isSubtype(this.targetClass.sym.type, type))
>>
>> Instead of adding the direct reference, and then having to check if the
>> reference needs to be removed, would it be possible not to add the reference
>> in the first place if the types mismatch?
>
> No because (for example) what if you cast?
>
> The thing you're casting might be compatible, but after the cast it might
> become incompatible.
Uhm. Turns out I probably did not understand the filter correctly, and now I'm
more dubious about what it actually does. Say you have this hierarchy:
interface A { }
class B {
B() {
A a = (A)this;
...
}
}
class C extends B implements A { }
```
Pathological case, I know. But the filtering will end up dropping the
expression Ref on the floor, right? (because B and A are unrelated).
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 200:
> 198: //
> 199:
> 200: public void analyzeTree(Env env) {
nit: this method could be removed in favor of the overloaded version below
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 11:09:35 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 444:
>
>> 442: if (referenceExpressionNode) {
>> 443:
>> 444: // We treat instance methods as having a "value" equal to
>> their instance
>
> The comment is slightly misleading - e.g. I'd suggest clarifying "having a
> "value" whose type is the same as that of the class in which the method is
> defined"
Agreed - will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 454:
>
>> 452:
>> 453: // If the expression type is incompatible with 'this',
>> discard it
>> 454: if (type != null &&
>> !this.isSubtype(this.targetClass.sym.type, type))
>
> Instead of adding the direct reference, and then having to check if the
> reference needs to be removed, would it be possible not to add the reference
> in the first place if the types mismatch?
No because (for example) what if you cast?
The thing you're casting might be compatible, but after the cast it might
become incompatible.
> This will then determine how to interpret this in the context of that method
> analysis - e.g. when we see a JCIdent for this, we create a direct/indirect
> ExprRef based on what the receiver kind was. Correct?
Yes, exactly.
When we "invoke" a method, we "preload" the set of current references that the
method is going to have when it starts. That "preload" step includes any
references from (a) the method receiver (non-static methods only) and (b)
method parameters.
To the extent the method receiver has a direct/indirect reference, that turns
into a direct/indirect `ThisRef` during method execution. You can see this
happen in the very next lines after what you quoted. But of course the method
invocation (the "apply") could have been applied to any arbitrary expression as
the receiver.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 817:
>
>> 815: // Methods - the "value" of a non-static method is a reference
>> to its instance
>> 816: final Symbol sym = tree.sym;
>> 817: if (sym.kind == MTH) {
>
> This is perhaps where filtering based on the declaring class could make sense
> (to avoid having to filter later) ? Perhaps this could also be centralized -
> e.g. whenever you create an ExprRef you also pass the type for it, and if the
> type matches that for the current class you create it and add to the list,
> otherwise you skip it.
Yes but see previous comment regarding casting.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 875:
>
>> 873: // Reference to this?
>> 874: if (tree.name == names._this || tree.name == names._super) {
>> 875: if (this.refs.contains(ThisRef.direct()))
>
> This idiom occurs quite a lot. If I'm correct, this basically amounts at
> asking as to whether the receiver of the method we're currently evaluating is
> direct or not (which is an invariant, given a method body - e.g. for a given
> method this "fact" should stay the same). If that's the case, perhaps
> capturing this in a flag could be better - then you could have just have a
> single method e.g. `XYZRef.create(boolean direct)`, and remove the branching
> (here and elsewhere).
The code you quoted has nothing specifically to do with method invocations.
This code is simply handing the evaluation of the expressions `this` and
`super`. For example, `this` could just be a parameter we're passing to another
method.
When `this` or `super` expressions are evaluated, the thing left on top of the
stack is a direct/indirect reference (i.e., an `ExprRef`) exactly when the
there is a direct/indirect reference of type `ThisRef` in the current reference
set.
In cases where `this` is then "invoked", e.g., `this()`, the `ExprRef` (top of
Java stack) becomes the method receiver, and when the method is "invoked" it
turns back into a `ThisRef` (see earlier question).
Regarding the optimization you mention, in light of the above I'm not sure it
would still work.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:56:53 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 411:
>
>> 409: final boolean referenceExpressionNode;
>> 410: switch (tree.getTag()) {
>> 411: case CASE:
>
> surprised to see `CASE` here - as that's not an expression
I put it there because of switch expressions and `yeild`... ?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:48:49 GMT, Maurizio Cimadamore wrote: >> Archie L. Cobbs has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Use the more appropriate Type comparison method Types.isSameType(). >> - Add some more comments to clarify how the analysis works. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java > line 227: > >> 225: final boolean privateOuterPrev = this.privateOuter; >> 226: final Lint lintPrev = this.lint; >> 227: this.lint = this.lint.augment(tree.sym); > > general stylistic comment - I see here and everywhere in this class > `this.xyz` - this is not the norm in the rest of the javac codebase, and it > would be better to replace it with just `xyz` OK. I will reluctantly remove... `` > Why on earth wouldn't you want to make it clear which one of N outer classes > a field comes from, and that it's a field, and not a variable declared > somewhere off screen?? > > IMHO omitting 'this' qualifiers is effectively accepting the grave offense of > code obfuscation in exchange for a tiny smidgen of brevity. > > It's definitely made it harder for me to read and understand the compiler, > with all the levels of nesting it has. > `` I readily admit I'm probably in the minority on this and anyway it's a bikeshed thing so there's no point in debating it. I will go with the flow :) though I feel a little sorry for the next person who has to read this code. > From a documentation perspective it might carry a bit of value Yes, that's the purpose - the `final` is for the human viewer, not the compiler. Just trying to be helpful to the next person. But you're right it's inconsistent so I'll remove. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v4]
On Wed, 11 Jan 2023 14:49:59 GMT, Thomas Stuefe wrote: >> Justin King has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Initialize memory to zero in zGranuleMap >> >> Signed-off-by: Justin King > > Curious, I always thought we do ArrayAllocator - using mmap for larger > allocations - to prevent memory retention for libc variants whose allocators > are "grabby", i.e. which don't promptly return memory to the OS on free(). > E.g. because they only use sbrk (Solaris, AIX), or are just cautious about > returning memory (glibc). > > Glibc's retention problem is only relevant for fine-grained allocations, so > for glibc this is probably fine. This leaves at least AIX as a potential > problem. @backwaterred, does the AIX libc malloc() still exclusively use the > data segment ? Does free'd memory still stick to the process? > > (While writing this, I remember that we at SAP even rewrote Arena allocation > to use mmap for AIX, because large compile arenas caused lasting RSS > increase, so it has definitely been a problem in the past) > > To follow up on @tstuefe comment - and the one that I tried to say in the > > bug was that we added this MmapArrayAllocate feature for some G1 marking > > bits that used so much memory that hit the Solaris _sbrk issue. Maybe > > @stefank and @tschatzl remember this issue. Maybe it's ok for AIX, then > > removing this code is a good change. Maybe the G1 usages need a mmap > > implementation though. > > The padding.inline.hpp usage seems to have one caller which is called once. > The other mmap usage in G1 we can convert to mmap using a similar approach to > zGranuleMap if that is preferred. That would then be equivalent behavior, it > looks like the G1 code uses the page allocation granularity anyway so maybe > keeping it mmap is the better way to go here anyway? My uninformed opinion (I'm not the G1 code owner) is that it would be fine to use explicit mmap. I'd love the complexity reduction this patch brings. - PR: https://git.openjdk.org/jdk/pull/11931
Re: RFR: 8300012: Remove unused JDI VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object) method [v2]
On Thu, 12 Jan 2023 16:44:44 GMT, Chris Plummer wrote: >> `VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object)` is not >> used. Furthermore it confuses the reader that runs across >> `removeObjectMirror()` calls, because what is actually being called is >> `removeObjectMirror(SoftObjectReference ref)`. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Make removeObjectMirror() private. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11957
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 16:20:12 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 218:
>>
>>> 216: new TreeScanner() {
>>> 217:
>>> 218: private Lint lint = ThisEscapeAnalyzer.this.lint;
>>
>> On a first look I'm not sure about the granularity of suppression here. I
>> believe that suppressing at the class level, or at the constructor level is
>> enough. Allowing to further annotate var declarations and non-constructor
>> methods, while doable, might actually be counter productive - in the sense
>> that the annotation does not occur in the constructor (where you'd want to
>> see it) but in some other method. I think the fact that a constructor is
>> escaping (willingly) should be a visible thing. Something to consider.
>
> Two things...
>
> We need to support annotations on field declarations because their
> initializers are effectively mini-constructors. But we don't need it on local
> variables, parameters, etc. - too confusing.
>
> For annotations on methods, yes it's debatable. It can be handy if you have
> e.g. an `init()` method that all of your constructors invoke. However, I
> agree it's also confusing, so I will remove.
>
> But we should keep the notion that if a constructor invokes `this()`, and the
> invoked constructor has annotations suppressed, then we skip over the
> constructor invocation.
>
> I will make these updates.
Yep - I guess there's a general theme of "where do we want the warnings to be
reported". My general feeling is that reporting a warning on the constructor
might be enough - but I see that you try to generate a warning in the exact
spot where the leakage happens - which I'm not sure if it's ok, or too clever.
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 270:
>>
>>> 268: final boolean analyzable =
>>> this.currentClassIsExternallyExtendable() &&
>>> 269: TreeInfo.isConstructor(tree) &&
>>> 270: !tree.sym.isPrivate() &&
>>
>> Why aren't private constructors analyzed? If we have a class with a private
>> constructor and public static factory invoking said constructor, and the
>> constructor makes `this` escape, isn't that an issue we should detect?
>
>> If we have a class with a private constructor and public static factory
>> invoking said constructor, and the constructor makes this escape, isn't that
>> an issue we should detect?
>
> A static factory method will not create a subclassed instance, so there's no
> 'this' escape problem.
>
> But I admit I completely missed factory methods as a potential thing to worry
> about.
>
> Is it possible for a leak to be missed due to the use of a factory method?
>
> Hmm. I can't immediately think of how, but if you can come up with an example
> please share.
I guess what I'm thinking about:
class Foo {
private Foo() {
m(this);
}
public void m() { ... } // overridable
static Foo makeFoo() { return new Foo(); }
}
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 294:
>>
>>> 292: !(this.currentClass.sym.isSealed() &&
>>> this.currentClass.permitting.isEmpty()) &&
>>> 293: !(this.currentClass.sym.owner.kind == MTH) &&
>>> 294: !this.privateOuter;
>>
>> Here, note that is the owner of the current class symbol is a method, that
>> covers anonymous classes too, which is a part of `privateOuter`. So the only
>> think we need to check here is whether "currentClass" is private, which is a
>> simple predicate. No need to carry `privateOuter` I believe
>
> Unless you explicitly declare a nested class `private`, it won't have the
> `ACC_PRIVATE` flag, even though it is "implicitly private" because it has a
> `private` enclosing class.
>
> Example:
>
> $ cat PrivateOuter.java
> public class PrivateOuter {
> private static class Inner1 {
> static class Inner2 {
> }
> }
> }
> $ javap -v PrivateOuter$Inner1$Inner2
> Classfile
> /Users/archie/proj/jdk/flex-test/classes/PrivateOuter$Inner1$Inner2.class
> Last modified Jan 12, 2023; size 408 bytes
> SHA-256 checksum
> 51ba6d39a5e66df2a078761d6424acbea7a8e32b8451f6ca7d2af49889673b2c
> Compiled from "PrivateOuter.java"
> class PrivateOuter$Inner1$Inner2
> minor version: 0
> major version: 63
> flags: (0x0020) ACC_SUPER
> this_class: #7 // PrivateOuter$Inner1$Inner2
> super_class: #2 // java/lang/Object
> ...
>
>
> So we have to keep track of this "implicit privateness" manually, which is
> what the `privateOuter` flag is for.
D'oh - missed the `|=` - so this keeps updating...
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8300012: Remove unused JDI VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object) method [v2]
> `VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object)` is not > used. Furthermore it confuses the reader that runs across > `removeObjectMirror()` calls, because what is actually being called is > `removeObjectMirror(SoftObjectReference ref)`. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Make removeObjectMirror() private. - Changes: - all: https://git.openjdk.org/jdk/pull/11957/files - new: https://git.openjdk.org/jdk/pull/11957/files/fa48e07c..dbd2f3d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11957=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11957=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11957.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11957/head:pull/11957 PR: https://git.openjdk.org/jdk/pull/11957
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:32:19 GMT, Maurizio Cimadamore wrote: > If we have a class with a private constructor and public static factory > invoking said constructor, and the constructor makes this escape, isn't that > an issue we should detect? A static factory method will not create a subclassed instance, so there's no 'this' escape problem. But I admit I completely missed factory methods as a potential thing to worry about. Is it possible for a leak to be missed due to the use of a factory method? Hmm. I can't immediately think of how, but if you can come up with an example please share. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:25:27 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 218:
>
>> 216: new TreeScanner() {
>> 217:
>> 218: private Lint lint = ThisEscapeAnalyzer.this.lint;
>
> On a first look I'm not sure about the granularity of suppression here. I
> believe that suppressing at the class level, or at the constructor level is
> enough. Allowing to further annotate var declarations and non-constructor
> methods, while doable, might actually be counter productive - in the sense
> that the annotation does not occur in the constructor (where you'd want to
> see it) but in some other method. I think the fact that a constructor is
> escaping (willingly) should be a visible thing. Something to consider.
Two things...
We need to support annotations on field declarations because their initializers
are effectively mini-constructors. But we don't need it on local variables,
parameters, etc. - too confusing.
For annotations on methods, yes it's debatable. It can be handy if you have
e.g. an `init()` method that all of your constructors invoke. However, I agree
it's also confusing, so I will remove.
But we should keep the notion that if a constructor invokes `this()`, and the
invoked constructor has annotations suppressed, then we skip over the
constructor invocation.
I will make these updates.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 304:
>
>> 302:
>> 303: // We are looking for analyzable constructors only
>> 304: final Symbol sym = entry.getKey();
>
> This seems unused. And, if so, perhaps we only need a `Set`, not
> a map.
Thanks, you're right, the map keys are not used here. I'll clean up the loop.
But we still need a map, not a set, because the map is used elsewhere.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 12:08:51 GMT, Daniel Jeliński wrote: > Please review this fix for DwarfParser cleaner. > > The original code registered the cleaner using a lambda that captured a > reference to the parser object; as a result, the object was never GCed, and > the cleaner never ran. > > In this version I moved the lambda creation to a static method, so that it > can't capture a reference to the parser. > > Additionally, the new code uses a static cleaner; the cleaner object creation > and cleanup are heavy operations (every cleaner comes with its own thread), > and it's preferable to use a single shared cleaner where possible. > > I verified manually that the native `destroyDwarfContext` method is called > after this fix. No new automated test. Existing tier1-2 tests continue to > pass. Though I am not a usual reviewer for this area I believe the logic of your change is good. Please obtain the review from a maintainer of the area before integrating. - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 10:18:27 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 294:
>
>> 292: !(this.currentClass.sym.isSealed() &&
>> this.currentClass.permitting.isEmpty()) &&
>> 293: !(this.currentClass.sym.owner.kind == MTH) &&
>> 294: !this.privateOuter;
>
> Here, note that is the owner of the current class symbol is a method, that
> covers anonymous classes too, which is a part of `privateOuter`. So the only
> think we need to check here is whether "currentClass" is private, which is a
> simple predicate. No need to carry `privateOuter` I believe
Unless you explicitly declare a nested class `private`, it won't have the
`ACC_PRIVATE` flag, even though it is "implicitly private" because it has a
`private` enclosing class.
Example:
$ cat PrivateOuter.java
public class PrivateOuter {
private static class Inner1 {
static class Inner2 {
}
}
}
$ javap -v PrivateOuter$Inner1$Inner2
Classfile
/Users/archie/proj/jdk/flex-test/classes/PrivateOuter$Inner1$Inner2.class
Last modified Jan 12, 2023; size 408 bytes
SHA-256 checksum
51ba6d39a5e66df2a078761d6424acbea7a8e32b8451f6ca7d2af49889673b2c
Compiled from "PrivateOuter.java"
class PrivateOuter$Inner1$Inner2
minor version: 0
major version: 63
flags: (0x0020) ACC_SUPER
this_class: #7 // PrivateOuter$Inner1$Inner2
super_class: #2 // java/lang/Object
...
So we have to keep track of this "implicit privateness" manually, which is what
the `privateOuter` flag is for.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Thu, 12 Jan 2023 15:10:19 GMT, Maurizio Cimadamore wrote: > Interesting example - I thought you might have been referring to a case where > the class being analyzed was itself an exception. Yes - although that example doesn't compile (oops!). Just replace `catch (RuntimeException e)` with `catch (ThrownThis e)` and it should. > Question - shouldn't we conclude that this leak when we see throw this ? E.g. > what if the constructor did not have a catch (or if the catch was of a > different type) ? A thorough analysis would evaluate whether the exception was caught or not. But you're right - since we're not doing a thorough analysis, we should immediately declare a leak anytime we see `throw x` where `x` is a possible direct or indirect reference. Of course, this scenario should be unlikely in normal code. I will add a check for that. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 13:01:44 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 334:
>
>> 332: // of the stack trace of B. For example, if constructor Foo(int
>> x) has a leak, and constructor
>> 333: // Foo() invokes this(0), then emitting a warning for Foo()
>> would be redundant.
>> 334: final BiPredicate
>> extendsAsPrefix = (warning1, warning2) -> {
>
> Another strategy would be to give a single warning per leaking constructor.
We already impose a limit of at most one warning per constructor.
But for this limit what is technically meant by "per constructor" is "At most
one warning per constructor, assuming that constructor is the one being
analyzed (i.e., the one invoked by the subclass and not via `this()`)."
So that limitation doesn't stop a constructor from generating the same warning
multiple times due to it being invoked indirectly by other constructors via
`this()`. Those duplicate warnings are what the code above eliminates.
So this code only generates one warning, and it's reported for the second
constructor:
public class DupWarn1 {
public DupWarn1() {
this(0);
}
public DupWarn1(int x) {
this.mightLeak();
}
public void mightLeak() {
}
}
DupWarn1.java:8: warning: [this-escape] possible 'this' escape before subclass
is fully initialized
this.mightLeak();
^
This is appropriate. The leak is really in the second constructor; the first
constructor has nothing to do with it. Reporting a leak for both constructors
would be redundant.
An interesting side question: Is it possible to come up with an example where
constructor A has a 'this' leak, and some other constructor B invokes `this()`
to delegate to A, but when B delegates to A the leak occurs differently, or not
at all?
Because of the "flood" analysis, and the fact that you can't pass 'this'
references to `this()` or `super()` (these are static contexts), I don't think
it's possible.
So in fact the deduplication will always apply whenever `this()` is involved.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Thu, 12 Jan 2023 14:59:12 GMT, Archie L. Cobbs wrote:
>>> * On the Java stack
>>> (a) The current 'this' instance
>>> (b) A method parameter
>>> (c) A local variable
>>> (d) A temporary value that is part of the current expression being
>>> evaluated
>>> (e) The return value from a method that just returned
>>> (f) A caught exception
>>>
>>> * In a field of some object...
>>> (a) A normal field
>>> (b) An outer 'this' instance
>>> (c) Other synthetic field (e.g., captured free variable)
>>>
>>> * As an element in a reference array
>>>
>>> * In native code as a native reference
>>
>> Thanks for the classification. This is helpful.
>>
>> I'm not sure what you mean by (1f). You mean `this` can be embedded in an
>> exception being thrown? Is that different from (2)?
>>
>> Also, it seems to me that (3) is a special case of (2) - in the sense that
>> you can imagine a reference array as a big object that has many fields as
>> there are elements - so the same reasoning applies.
>>
>> When looking at (2d) and (2e) the fact that Javac's AST is not in SSA form
>> means that, yes, we need to track _expressions_ not just variables (e.g. for
>> chains of methods calls, ternary operators and such).
>
>> I'm not sure what you mean by (1f). You mean this can be embedded in an
>> exception being thrown? Is that different from (2)?
>
> Yes, this would be a different case from any other that you'd have to handle
> in the code if you wanted to deal with it.
>
> An example of how this could happen would be:
>
> public class ThrownThis extends RuntimeException {
>
> public ThrownThis(Object obj) {
> try {
> this.validate(obj);
> } catch (RuntimeException e) {
> e.mightLeak(); // LEAK HERE
> }
> }
>
> private void validate(Object obj) {
> if (obj.hashCode() != 123)
> throw this;
> }
>
> public void mightLeak() {
> }
> }
>
> Of course, that's an absurd example and the likelihood that any random piece
> of actually code does that is negligible.
>
> Regardless, I did briefly consider including handling for thrown exceptions
> but quickly decided it couldn't possibly be worth the trouble.
>
> As a result, if you compile that example with `-Xlint:this-escape` you don't
> get a warning. No major loss!
>
>> Also, it seems to me that (3) is a special case of (2) - in the sense that
>> you can imagine a reference array as a big object that has many fields as
>> there are elements - so the same reasoning applies.
>
> Yes it's effectively the same thing, but fields and array elements are
> accessed by different mechanisms in Java, so from the point of the analyzer
> they have to be handled separately for that reason, which is why I broke them
> out.
> Yes, this would be a different case from any other that you'd have to handle
> in the code if you wanted to deal with it.
>
> An example of how this could happen would be:
>
> ```java
> public class ThrownThis extends RuntimeException {
>
> public ThrownThis(Object obj) {
> try {
> this.validate(obj);
> } catch (RuntimeException e) {
> e.mightLeak(); // LEAK HERE
> }
> }
>
> private void validate(Object obj) {
> if (obj.hashCode() != 123)
> throw this;
> }
>
> public void mightLeak() {
> }
> }
> ```
>
Interesting example - I thought you might have been referring to a case where
the class being analyzed was itself an exception.
Question - shouldn't we conclude that `this` leak when we see `throw this` ?
E.g. what if the constructor did not have a `catch` (or if the catch was of a
different type) ?
> Of course, that's an absurd example and the likelihood that any random piece
> of actually code does that is negligible.
>
> Regardless, I did briefly consider including handling for thrown exceptions
> but quickly decided it couldn't possibly be worth the trouble.
>
> As a result, if you compile that example with `-Xlint:this-escape` you don't
> get a warning. No major loss!
>
> > Also, it seems to me that (3) is a special case of (2) - in the sense that
> > you can imagine a reference array as a big object that has many fields as
> > there are elements - so the same reasoning applies.
>
> Yes it's effectively the same thing, but fields and array elements are
> accessed by different mechanisms in Java, so from the point of the analyzer
> they have to be handled separately for that reason, which is why I broke them
> out.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Thu, 12 Jan 2023 09:57:00 GMT, Maurizio Cimadamore
wrote:
> I'm not sure what you mean by (1f). You mean this can be embedded in an
> exception being thrown? Is that different from (2)?
Yes, this would be a different case from any other that you'd have to handle in
the code if you wanted to deal with it.
An example of how this could happen would be:
public class ThrownThis extends RuntimeException {
public ThrownThis(Object obj) {
try {
this.validate(obj);
} catch (RuntimeException e) {
e.mightLeak(); // LEAK HERE
}
}
private void validate(Object obj) {
if (obj.hashCode() != 123)
throw this;
}
public void mightLeak() {
}
}
Of course, that's an absurd example and the likelihood that any random piece of
actually code does that is negligible.
Regardless, I did briefly consider including handling for thrown exceptions but
quickly decided it couldn't possibly be worth the trouble.
As a result, if you compile that example with `-Xlint:this-escape` you don't
get a warning. No major loss!
> Also, it seems to me that (3) is a special case of (2) - in the sense that
> you can imagine a reference array as a big object that has many fields as
> there are elements - so the same reasoning applies.
Yes it's effectively the same thing, but fields and array elements are accessed
by different mechanisms in Java, so from the point of the analyzer they have to
be handled separately for that reason, which is why I broke them out.
-
PR: https://git.openjdk.org/jdk/pull/11874
RFR: 8300032: DwarfParser resource leak
Please review this fix for DwarfParser cleaner. The original code registered the cleaner using a lambda that captured a reference to the parser object; as a result, the object was never GCed, and the cleaner never ran. In this version I moved the lambda creation to a static method, so that it can't capture a reference to the parser. Additionally, the new code uses a static cleaner; the cleaner object creation and cleanup are heavy operations (every cleaner comes with its own thread), and it's preferable to use a single shared cleaner where possible. I verified manually that the native `destroyDwarfContext` method is called after this fix. No new automated test. Existing tier1-2 tests continue to pass. - Commit messages: - Fix dwarf cleaner Changes: https://git.openjdk.org/jdk/pull/11965/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11965=00 Issue: https://bugs.openjdk.org/browse/JDK-8300032 Stats: 8 lines in 1 file changed: 5 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11965.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11965/head:pull/11965 PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 334:
> 332: // of the stack trace of B. For example, if constructor Foo(int
> x) has a leak, and constructor
> 333: // Foo() invokes this(0), then emitting
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 163:
> 161: * invoked from the target constructor; if empty, we're still in
> the constructor.
> 162: */
> 163: private final ArrayDeque callStack = new
>
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Wed, 11 Jan 2023 03:30:03 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Use the more appropriate Type comparison method Types.isSameType().
> - Add some more comments to clarify how the analysis works.
FWIW, the make changes look good. I have nothing to say about the actual lint
code itself.
-
Marked as reviewed by ihse (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11874
RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0
Please review this patch that fixes a few memory leaks in JNI code. [The latest documentation](https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#releaseprimitivetypearrayelements-routines) of JNI functions makes an explicit note about the use of JNI_COMMIT: > If `JNI_COMMIT` is passed as the `mode` argument when `elems` is a copy of > the elements in `array`, then a final call to > *ReleaseArrayElements* passing a `mode` argument of "0" or > `JNI_ABORT`, should be made to free the `elems` buffer No new regression test. I manually verified the Linux fix using ClhdsbPstack test in root mode. Also, tier1-2 tests on mach5 continue to pass. - Commit messages: - Fix release mode Changes: https://git.openjdk.org/jdk/pull/11963/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11963=00 Issue: https://bugs.openjdk.org/browse/JDK-8300024 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11963.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11963/head:pull/11963 PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8299795: Relativize locals in interpreter frames [v2]
On Wed, 11 Jan 2023 09:22:03 GMT, Fredrik Bredberg wrote: >> Implementation of relativized locals in interpreter frames for x86. x64, >> arm, aarch64, ppc64le and riscv. >> Not relativized locals on zero and s390 but done some changes to cope with >> the changed generic code. >> Tested tier1-tier8 on supported platforms. The rest was sanity tested using >> Qemu, except s390, which was only tested by GitHub Actions. > > Fredrik Bredberg has updated the pull request incrementally with one > additional commit since the last revision: > > Updated some copyright dates. Also verified on linux-riscv64 HiFive Unmatched board running hotspot_loom & jdk_loom jtreg tests with extra VM options: -XX:+VerifyStack -XX:+VerifyContinuations. - PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Thu, 12 Jan 2023 02:14:10 GMT, Archie L. Cobbs wrote: >>> >>> D'oh, you're right. But if you made `returnMe()` static or private then the >>> argument would still hold. >>> >>> > And, if the method is static, same story - you are passing ref3 somewhere >>> > else, and ref3 potentially contains this. >>> >>> Not true... static methods are safe to "invoke" because they can't be >>> overridden. When the analyzer "invokes" the method, it will see that all it >>> does with its parameter is return it. >>> >> >> So, for static methods, it could go down two ways: either we don't even look >> at referenced method bodies, give up and just declare "sorry, escaping". Or, >> if we look into method bodies, and see that the relationship between inner >> and outer parameter is as simple, it's just like assignment again: >> >> ref1 -> ref2 -> ref3 -> x -> ref4 >> >>> But I don't really have an analysis of all the trade-offs. Really, at a >>> certain point I stumbled on what I thought was a fairly straightforward way >>> to achieve a good false negative rate and a very low false positive rate, >>> without a lot of work. And frankly at that point I stopped caring about the >>> question you're asking, although I agree it's certainly interesting from an >>> academic point of view (I'm a practical person). >> >> The reason I'm asking these questions is that I'm trying to understand which >> ingredients went into the analysis and why, in order to try and build a >> mental problem of what the problem that needs to be solved is, what are the >> constraints, etc. I'm also a practical person :-) and I often find it easier >> to understand a piece of code if I know some of the reasoning that went >> behind it, or what's the behavior supposed to be. >> >> I do not know, off-hands, whether there is a simpler solution - I was mostly >> probing for war stories of the kind "I tried X and I got Y", and I apologize >> if that came off the wrong way. > >> So, for static methods, it could go down two ways: either we don't even look >> at referenced method bodies, give up and just declare "sorry, escaping". Or, >> if we look into method bodies, and see that the relationship between inner >> and outer parameter is as simple, it's just like assignment again: > > Right - and just to reconfirm, it's only the non-overridable methods in the > same compilation unit that are "invoked" (i.e., analyzed & tracked). Any > method that (might) exist in another compilation unit is off limits, so we > have to assume the worst case and declare a leak if we are passing it any > 'this' references. > >> in order to try and build a mental problem of what the problem that needs >> to be solved is > > Gotcha. > > Let's be clear about what exactly we're worrying about. It's this situation: > You have a class `B extends A`, where `B` and `A` are in different > compilation units, and during the `super()` call that constructor `B()` makes > to its superclass constructor `A()`, some code in class `B` gets executed > (really, it's some field in `B` getting accessed that actually matters) prior > to `A()` returning. > > So the basic goal is to analyze `A` constructors and watch 'this' references > until if/when they go to someplace where we can no longer watch them. At that > point we have to assume that some code in `B` might get invoked and so we > generate a warning. > > OK, so where can a 'this' reference go? > > Well, here are all of the possible places a Java reference can exist: > 1. On the Java stack > (a) The current 'this' instance > (b) A method parameter > (c) A local variable > (d) A temporary value that is part of the current expression being > evaluated > (e) The return value from a method that just returned > (f) A caught exception > 1. In a field of some object... > (a) A normal field > (b) An outer 'this' instance > (c) Other synthetic field (e.g., captured free variable) > 1. As an element in a reference array > 1. In native code as a native reference > > Those are the only possibilities AFAIK. > > So one way to locate yourself on the spectrum from "simple" to "complex" is > to answer this question: Which of those are you going to try to keep track > of, and for each one, how hard are you going to try? > > The `this-escape` analysis being proposed tracks 1(a-e) and 2(b) pretty > closely (it tries hard), and it adds a very course tracking of 2(a-c), and 3 > using the notion of indirect references (it doesn't try very hard). We do not > track 1(f) or 4. > > So to think about the overall problem, imagine how you might or might not > address all of those cases. > * On the Java stack > (a) The current 'this' instance > (b) A method parameter > (c) A local variable > (d) A temporary value that is part of the current expression being evaluated > (e) The return value from a method that just returned > (f) A caught exception > > * In a field of
RFR: JDK-8299957: Enhance error logging in instrument coding with additional jplis_assert_msg
There are a few places in the instrument coding where errors occur in our jtreg test, but the already existing error logging method jplis_assert_msg / jplis_assert is unfortunately missing so not much details are shown. This could be enhanced. - Commit messages: - JDK-8299957 Changes: https://git.openjdk.org/jdk/pull/11960/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11960=00 Issue: https://bugs.openjdk.org/browse/JDK-8299957 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/11960.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11960/head:pull/11960 PR: https://git.openjdk.org/jdk/pull/11960
Re: RFR: 8300012: Remove unused JDI VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object) method
On Thu, 12 Jan 2023 02:20:04 GMT, Chris Plummer wrote:
> `VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object)` is not
> used. Furthermore it confuses the reader that runs across
> `removeObjectMirror()` calls, because what is actually being called is
> `removeObjectMirror(SoftObjectReference ref)`.
Marked as reviewed by alanb (Reviewer).
src/jdk.jdi/share/classes/com/sun/tools/jdi/VirtualMachineImpl.java line 1448:
> 1446: }
> 1447:
> 1448: synchronized void removeObjectMirror(SoftObjectReference ref) {
It might able help to make this method removeObjectMirror, this will make it
clear that the method can't be called from code in other source files in this
package.
-
PR: https://git.openjdk.org/jdk/pull/11957
