Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14 [v4]
> 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. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: correction for ppc - Changes: - all: https://git.openjdk.org/jdk/pull/11935/files - new: https://git.openjdk.org/jdk/pull/11935/files/98c7d3dd..076f8b54 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11935&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11935&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11935.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11935/head:pull/11935 PR: https://git.openjdk.org/jdk/pull/11935
Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14 [v3]
> 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. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: missed update for debug mode - Changes: - all: https://git.openjdk.org/jdk/pull/11935/files - new: https://git.openjdk.org/jdk/pull/11935/files/62a6be82..98c7d3dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11935&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11935&range=01-02 Stats: 14 lines in 6 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/11935.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11935/head:pull/11935 PR: https://git.openjdk.org/jdk/pull/11935
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 00:57:14 GMT, Archie L. Cobbs wrote:
>> 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.
>
>> 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.
I was curious how much of a difference this type filtering makes, so I built
the JDK with and without it.
The results were identical except for one case:
package java.lang.invoke;
...
public abstract sealed class CallSite permits ConstantCallSite,
MutableCallSite, VolatileCallSite {
...
CallSite(MethodType targetType, MethodHandle createTargetHook) throws
Throwable {
this(targetType); // need to initialize target to make CallSite.type()
work in createTargetHook
ConstantCallSite selfCCS = (ConstantCallSite) this;
MethodHandle boundTarget = (MethodHandle)
createTargetHook.invokeWithArguments(selfCCS);
setTargetNormal(boundTarget); // ConstantCallSite doesn't publish
CallSite.target
UNSAFE.storeStoreFence(); // barrier between target and isFrozen updates
}
When we do type filtering, `(ConstantCallSite) this` causes the 'this'
reference to be discarded so no leak is reported on the next line for
`invokeWithArguments(selfCCS)`. Just a side note, there is also a leak on the
next line because final method `setTargetNormal()` invokes
`MethodHandleNatives.setCallSiteTargetNormal(this, newTarget)`, so a leak does
get reported anyway even with type filtering.
When type filtering is disabled, we report a leak at
`invokeWithArguments(selfCCS)` - which is accurate.
So what did we learn?
First it looks like type filtering has very minimal effect. I think this is
because it requires some version of two casts in a row in and out of type
compatibility, and this is probably very rare.
But looking at the one data point we do have, the type filtering did in fact
cause a false negative.
And when building the entire JDK, it causes zero net new false positives.
So to me this is evidence that we should just remove the type filtering
altogether...
@vicente-romero-oracle your thoughts?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8298853: JvmtiVTMSTransitionDisabler should support disabling one virtual thread transitions [v5]
On Wed, 4 Jan 2023 07:45:48 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiThreadState.cpp line 482:
>>
>>> 480: _VTMS_transition_disable_for_all_count > 0) {
>>> 481: MonitorLocker ml(JvmtiVTMSTransition_lock,
>>> Mutex::_no_safepoint_check_flag);
>>> 482: ml.notify_all();
>>
>> Checking the counts outside the lock seems really racy. Maybe it is safe in
>> the original code but now we have two counters it is very hard to ascertain
>> this is correct. Overall I find it very hard to see exactly how these
>> counters get used.
>
> This notification code is just an optimization. All related waiting fragments
> are with timeouts.
> I agree this sync protocol is kind of complicated and also does not scale
> well. I'm still thinking on how to improve it.
I have a two week vacation starting from Monday.
Not sure, that I'll be able to see review comments this time.
-
PR: https://git.openjdk.org/jdk/pull/11690
Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14 [v2]
On Fri, 13 Jan 2023 23:27:36 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. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > more in src/hotspot Thank you for making these additional changes! I'm still seeing some issue building on linux-x64, for example: src/hotspot/share/opto/regalloc.hpp:130:17: note: candidate: 'virtual char* PhaseRegAlloc::dump_register(const Node*, char*, size_t) const' 130 | virtual char *dump_register( const Node *n, char *buf, size_t buf_size) const = 0; | ^ - PR: https://git.openjdk.org/jdk/pull/11935
Integrated: 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)`. This pull request has now been integrated. Changeset: f4e119d5 Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/f4e119d5fcdf592f59a7d029070eba3878e24a7c Stats: 19 lines in 1 file changed: 0 ins; 17 del; 2 mod 8300012: Remove unused JDI VirtualMachineImpl.removeObjectMirror(ObjectReferenceImpl object) method Reviewed-by: alanb, sspitsyn - PR: https://git.openjdk.org/jdk/pull/11957
Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14 [v2]
> 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. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: more in src/hotspot - Changes: - all: https://git.openjdk.org/jdk/pull/11935/files - new: https://git.openjdk.org/jdk/pull/11935/files/1f7706f9..62a6be82 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11935&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11935&range=00-01 Stats: 59 lines in 12 files changed: 2 ins; 2 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/11935.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11935/head:pull/11935 PR: https://git.openjdk.org/jdk/pull/11935
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v10]
> 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:
- Fix bug where field initializer warnings could be incorrectly suppressed.
- Consolidate all the unit tests that generate warnings into one.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11874/files
- new: https://git.openjdk.org/jdk/pull/11874/files/ae37ff7c..0b06dc32
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=09
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=08-09
Stats: 1454 lines in 35 files changed: 658 ins; 762 del; 34 mod
Patch: https://git.openjdk.org/jdk/pull
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 21:33:02 GMT, Archie L. Cobbs wrote:
>> Sounds good - thanks.
>
> Ah. I just realized that we need to do it your way because of the following
> bug:
>
> Suppose you have a constructor and a field with initializer that both leak,
> but you have `@SuppressWarnings("this-escape")` on the constructor.
>
> Then the leak for the field will never be reported because we never get to it.
>
> I'll fix.
yep, right in that case that leak wouldn't be reported
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 16:20:41 GMT, Archie L. Cobbs wrote:
>> I'm OK either way we can revisit this later either as part of this PR or in
>> a future one. I let it to your consideration
>
> Sounds good - thanks.
Ah. I just realized that we need to do it your way because of the following bug:
Suppose you have a constructor and a field with initializer that both leak, but
you have `@SuppressWarnings("this-escape")` on the constructor.
Then the leak for the field will never be reported because we never get to it.
I'll fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
On Fri, 13 Jan 2023 20:21:24 GMT, Vicente Romero wrote:
>> Archie L. Cobbs has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Use more idiomatic test for java.lang.Object.
>> - Revert 27cb30129; the error was actually fixed in edf3c3f51.
>> - Fix formatting issue with the "this-escape" --help-lint description.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 1088:
>
>> 1086: private void visitLooped(T tree, Consumer
>> visitor) {
>> 1087: visitScoped(tree, false, t -> {
>> 1088: while (true) {
>
> I have also been thinking if the loop analysis could go wild and execute a
> large, unbound number of times. But it seems from Archie's experiments that
> this probably won't occur in "normal" code and worst case scenario if that
> were to occur we can always limit the number of times we will process loops
> to a set number of times
The number of times around any single loop is bounded by the number of new
references that can possibly be created during the analysis of that loop.
That number is at most 2 * (1 + 1 + 1 + 1 + N) where N is the number of
parameters and/or variables declared in that scope (the 2 is for direct or
indirect, and the 1's are for each of the singleton reference types `ThisRef`,
`OuterRef`, `ExprRef`, and `ReturnRef`).
If you have nested scopes A, B, C each with Na, Nb, and Nc variables declared
therein (respectively), then the bound would be something like 2 * (1 + 1 + 1 +
1 + Na + (Na * Nb) + (Na * Nb * Nc)) worst case (waving hands here).
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
On Fri, 13 Jan 2023 20:16:25 GMT, Vicente Romero wrote:
>> Archie L. Cobbs has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Use more idiomatic test for java.lang.Object.
>> - Revert 27cb30129; the error was actually fixed in edf3c3f51.
>> - Fix formatting issue with the "this-escape" --help-lint description.
>
> test/langtools/tools/javac/warnings/ThisEscape/ThisEscapeBasic.java line 8:
>
>> 6: */
>> 7:
>> 8: public class ThisEscapeBasic {
>
> I wonder if it could be better to just fold most of these tests in particular
> the small ones and create just one, or a few tests, with several subclasses,
> that will reduce the overhead if in the future we change the code and we need
> to change the golden files.
I really don't have any informed opinion on this. I can see arguments both
ways... putting all you eggs in one basket can be seen alternately as foolhardy
or highly efficient :)
FWIW in the past I've had problems with trying to combine multiples tests that
generate errors into one file. The compiler seems to like to bail out early
when it sees errors. I guess that experience was working subconsciously.
Of course that doesn't apply here as these are all warnings. And a bunch of
separate files make the test run slower.
So I agree with you - I'll consolidate them.
-
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. Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8300032: DwarfParser resource leak
On Fri, 13 Jan 2023 07:45:59 GMT, Daniel Jeliński wrote: > 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. Ok. So the lambda needs to reference a field of the `this` in order to capture `this`. I guess that's why you see hacks like this: // Register a cleaning function to close the handle final long local_handle = handle;// local to prevent capture of this CleanerFactory.cleaner().register(this, () -> closeHandle(local_handle)); - PR: https://git.openjdk.org/jdk/pull/11965
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0 [v2]
On Fri, 13 Jan 2023 07:16:35 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. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > MacOS fix Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
On Fri, 13 Jan 2023 19:09:54 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 three
> additional commits since the last revision:
>
> - Use more idiomatic test for java.lang.Object.
> - Revert 27cb30129; the error was actually fixed in edf3c3f51.
> - Fix formatting issue with the "this-escape" --help-lint description.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 1088:
> 1086: private void visitLooped(T tree, Consumer
> visitor) {
> 1087: visitScoped(tree, false,
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
On Fri, 13 Jan 2023 19:09:54 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 three
> additional commits since the last revision:
>
> - Use more idiomatic test for java.lang.Object.
> - Revert 27cb30129; the error was actually fixed in edf3c3f51.
> - Fix formatting issue with the "this-escape" --help-lint description.
test/langtools/tools/javac/warnings/ThisEscape/ThisEscapeBasic.java line 8:
> 6: */
> 7:
> 8: public class ThisEscapeBasic {
I wonder if it could be better to just fold most of these tests in particu
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 17:49:05 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 685:
>>
>>> 683:
>>> 684: @Override
>>> 685: public void visitDoLoop(JCDoWhileLoop tree) {
>>
>> I was thinking, code can also loop using labels and `break` / `continue`,
>> not something we need to cover as part of this prototype but could be a
>> future TODO that we can document
>
> Hah - I didn't think of that. But actually I don't think we would miss
> anything (see if you agree).
>
> The code "executes" every loop, in a sort-of simulation, adding references
> until the set of references converges. Moreover, that reference set is
> "append only" while this is happening.
>
> Therefore, during actual execution, a break or continue may cause less code
> to be executed than in our simulation, but never more code than our
> simulation. So during actual execution it might be that fewer actual 'this'
> references are created, but never more.
>
> Therefore, this effect might cause false positives (which of course we
> already have with loops and code in general because we take all possible
> branches), but never false negatives.
yep I agree
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
> 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 three
additional commits since the last revision:
- Use more idiomatic test for java.lang.Object.
- Revert 27cb30129; the error was actually fixed in edf3c3f51.
- Fix formatting issue with the "this-escape" --help-lint description.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11874/files
- new: https://git.openjdk.org/jdk/pull/11874/files/edf3c3f5..ae37ff7c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=08
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=07-08
Stats: 12 lines in 3 files changed: 3 ins; 4 del; 5 mod
Patc
Re: RFR: 8300024: Replace use of JNI_COMMIT mode with mode 0 [v2]
On Fri, 13 Jan 2023 07:16:35 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. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > MacOS fix Marked as reviewed by sspitsyn (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11963
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v8]
On Fri, 13 Jan 2023 17:39:41 GMT, Justin King wrote: >> Remove abstraction that is a holdover from Solaris. Direct usages of >> `MmapArrayAllocator` have been switched to normal `malloc`. The >> justification is that none of the code paths are called from signal >> handlers, so using `mmap` directly does not make sense and is potentially >> slower than going through `malloc` which can potentially re-use memory >> without making any system calls. The remaining usages of `ArrayAllocator` >> and `MallocArrayAllocator` are equivalent. > > Justin King has updated the pull request incrementally with one additional > commit since the last revision: > > Apply patch from stefank@ > > Signed-off-by: Justin King Restored `mmap` for G1, though in the future it might make sense to switch to just using malloc. - PR: https://git.openjdk.org/jdk/pull/11931
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 17:35:08 GMT, Vicente Romero wrote:
>> 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 caps in comments.
>> - Clarify confusing comment.
>> - ... and 6 more: https://git.openjdk.org/jdk/compare/6e96a7d7...edf3c3f5
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 685:
>
>> 683:
>> 684: @Override
>> 685: public void visitDoLoop(JCDoWhileLoop tree) {
>
> I was thinking, code can also loop using labels and `break` / `continue`, not
> something we need to cover as part of this prototype but could be a future
> TODO that we can document
Hah - I didn't think of that. But actually I don't think we would miss anything
(see if you agree).
The code "executes" every loop, in a sort-of simulation, adding references
until the set of references converges. Moreover, that reference set is "append
only" while this is happening.
Therefore, during actual execution, a break or continue may cause less code to
be executed than in our simulation, but never more code than our simulation. So
during actual execution it might be that fewer actual 'this' references are
created, but never more.
Therefore, this effect might cause false positives (which of course we already
have with loops and code in general because we take all possible branches), but
never false negatives.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v9]
> Remove abstraction that is a holdover from Solaris. Direct usages of > `MmapArrayAllocator` have been switched to normal `malloc`. The justification > is that none of the code paths are called from signal handlers, so using > `mmap` directly does not make sense and is potentially slower than going > through `malloc` which can potentially re-use memory without making any > system calls. The remaining usages of `ArrayAllocator` and > `MallocArrayAllocator` are equivalent. Justin King has updated the pull request incrementally with two additional commits since the last revision: - Fix compilation error Signed-off-by: Justin King - Preserve mmap usage in gc/g1/g1ConcurrentMark Signed-off-by: Justin King - Changes: - all: https://git.openjdk.org/jdk/pull/11931/files - new: https://git.openjdk.org/jdk/pull/11931/files/4aba784e..4b5ce84b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11931&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11931&range=07-08 Stats: 33 lines in 2 files changed: 30 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11931.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11931/head:pull/11931 PR: https://git.openjdk.org/jdk/pull/11931
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v4]
On Wed, 11 Jan 2023 15:08:23 GMT, Stefan Karlsson wrote:
>>> I'm happy to see this flag getting removed. I'm less happy about seeing
>>> usages of the array allocators being replaced by macros. I'd rather see an
>>> effort towards getting rid of these macros. Could we limit this patch to
>>> only remove the ArrayAllocatorMallocLimit flag and ArrayAllocator class,
>>> and defer the discussion around what API to use for array allocations?
>>
>> `ArrayAllocator` with `ArrayAllocatorMallocLimit` removed is effectively
>> `MallocArrayAllocator`. Are you suggesting leaving `MallocArrayAllocator`
>> and `MmapArrayAllocator` thus update references to `ArrayAllocator` to be
>> `MallocArrayAllocator`?
>>
>> As far as APIs, the majority of the codebase uses the macros. IMO
>> consistency would be better and having two ways of doing things doesn't
>> help. But if you feel strongly about it, we can punt and just surgically
>> remove the bare minimum, assuming you clarify your expectation (see above).
>
>> ArrayAllocator with ArrayAllocatorMallocLimit removed is effectively
>> MallocArrayAllocator. Are you suggesting leaving MallocArrayAllocator and
>> MmapArrayAllocator thus update references to ArrayAllocator to be
>> MallocArrayAllocator?
>
> Yes, that was what I wanted.
>
>> As far as APIs, the majority of the codebase uses the macros. IMO
>> consistency would be better and having two ways of doing things doesn't
>> help. But if you feel strongly about it, we can punt and just surgically
>> remove the bare minimum, assuming you clarify your expectation (see above).
>
> I agree about the argument about consistency, so I retract my objection. We
> can deal with these macros as a separate RFE (if we ever get to it).
>
> I would like to retain the usage of mmapped memory for ZGC to minimize the
> risk of any surprises for us. ZGC code also tend to initialize as much as
> possible in the initialization list, so the extra memset that comes after
> initialization lists sticks out a bit. Could you create a private
> `ZGranuleMap::allocate_array` function that uses
> os::reserve_memory/commmit_memory and change the code to be:
>
> inline ZGranuleMap::ZGranuleMap(size_t max_offset) :
> _size(max_offset >> ZGranuleSizeShift),
> _map(allocate_array(_size)) {
>
>
> Or if you like, I can provide a patch on top of your branch to do that.
Applied your patch @stefank
-
PR: https://git.openjdk.org/jdk/pull/11931
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v8]
> Remove abstraction that is a holdover from Solaris. Direct usages of > `MmapArrayAllocator` have been switched to normal `malloc`. The justification > is that none of the code paths are called from signal handlers, so using > `mmap` directly does not make sense and is potentially slower than going > through `malloc` which can potentially re-use memory without making any > system calls. The remaining usages of `ArrayAllocator` and > `MallocArrayAllocator` are equivalent. Justin King has updated the pull request incrementally with one additional commit since the last revision: Apply patch from stefank@ Signed-off-by: Justin King - Changes: - all: https://git.openjdk.org/jdk/pull/11931/files - new: https://git.openjdk.org/jdk/pull/11931/files/5d30170a..4aba784e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11931&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11931&range=06-07 Stats: 10 lines in 1 file changed: 3 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/11931.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11931/head:pull/11931 PR: https://git.openjdk.org/jdk/pull/11931
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 04:04:36 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 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.
> -
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.
This looks great. Very nice work!
src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 306:
> 304: // set relativized locals
> 305: // this line can be changed into an assert when we have fixed the
> "frame padding problem"
> 306: *f.addr_at(frame::interpreter_frame_locals_offset) = (bottom - 1) -
> f.fp();
Thank you for this comment. When you file the CR for this problem, can you
point to this CR?
src/hotspot/cpu/ppc/continuationFreezeThaw_ppc.inline.hpp line 554:
> 552: inline void ThawBase::set_interpreter_frame_bottom(const frame& f,
> intptr_t* bottom) {
> 553: // set relativized locals
> 554: // this line can be changed into an assert when we have fixed the
> "frame padding problem"
Can you file an RFE (or is there already one) to describe this?
-
Marked as reviewed by coleenp (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11902
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 16:12:50 GMT, Vicente Romero wrote: >> Yes... I did it that way is so that it doesn't require any adaptation >> if/when JDK-8194743 ever gets implemented. And it keeps the code a little >> simpler in exchange for a little redundancy. >> >> I'm happy to fix this if you think it is necessary though. > > I'm OK either way we can revisit this later either as part of this PR or in a > future one. I let it to your consideration Sounds good - thanks. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 16:06:04 GMT, Maurizio Cimadamore wrote: >>> Something seems to be up with the lint description for this-escape - >>> compare this: >> >> Oops, will fix - thanks. > >> The decision was to go with drawing the "warning boundary" at the >> compilation unit. The reasoning is that (a) this aligns with the compiler's >> "knowledge boundary", i.e., we can know for sure from code inspection, and >> also (b) focuses the warning on the particularly pernicious aspect of these >> bugs, which is that they arise from the non-obvious interaction among two or >> more files > > Sorry for being picky - you mention this "compilation unit" boundary before, > but this is not really the reason here. Note that in my example B constructor > calls out to a static method that is "outside" the boundary. The reason as to > why my example is not flagged is simply that "escaping" is defined as > "escaping into a subclass method", not just "escaping from the constructor > (into some other compilation unit)". Oops, you're right, I answered the wrong question so to speak. The "must involve a subclass" requirement is another dimension on which a "boundary" was declared. This was also part of the original discussion. So yes the requirement is: "requires involvement of a subclass" **AND** "that subclass lives in a separate compilation unit". - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 15:14:05 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 516:
>>
>>> 514: Name name = TreeInfo.name(invoke.meth);
>>> 515: if (name == names._super) {
>>> 516: scanInitializers();
>>
>> it seems like the code scan initializers every time it finds a super()
>> invocation, I guess that this scanning could be done once per class
>
> Yes... I did it that way is so that it doesn't require any adaptation if/when
> JDK-8194743 ever gets implemented. And it keeps the code a little simpler in
> exchange for a little redundancy.
>
> I'm happy to fix this if you think it is necessary though.
I'm OK either way we can revisit this later either as part of this PR or in a
future one. I let it to your consideration
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 15:08:59 GMT, Archie L. Cobbs wrote: >>> I guess I was confused because, while subclasses are a particularly sneaky >>> case where uninitialized values can show up, the above leak seems >>> potentially dangerous as well... >> >> Yes - and this very question did come up in the discussions around this >> warning (see amber-dev). >> >> The decision was to go with drawing the "warning boundary" at the >> compilation unit. The reasoning is that (a) this aligns with the compiler's >> "knowledge boundary", i.e., we can know for sure from code inspection, and >> also (b) focuses the warning on the particularly pernicious aspect of these >> bugs, which is that they arise from the non-obvious interaction among two or >> more files - even when looking at any single one of those files, there >> doesn't seem to be any apparent problem. In other words, we decided "not to >> try to save any single source code from itself". >> >> But I think it's still an interesting question. Maybe experience will >> provide more guidance over time. > >> Something seems to be up with the lint description for this-escape - compare >> this: > > Oops, will fix - thanks. > The decision was to go with drawing the "warning boundary" at the compilation > unit. The reasoning is that (a) this aligns with the compiler's "knowledge > boundary", i.e., we can know for sure from code inspection, and also (b) > focuses the warning on the particularly pernicious aspect of these bugs, > which is that they arise from the non-obvious interaction among two or more > files Sorry for being picky - you mention this "compilation unit" boundary before, but this is not really the reason here. Note that in my example B constructor calls out to a static method that is "outside" the boundary. The reason as to why my example is not flagged is simply that "escaping" is defined as "escaping into a subclass method", not just "escaping from the constructor (into some other compilation unit)". - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 12:42:24 GMT, Vicente Romero wrote:
>> 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 caps in comments.
>> - Clarify confusing comment.
>> - ... and 6 more: https://git.openjdk.org/jdk/compare/6e96a7d7...edf3c3f5
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 516:
>
>> 514: Name name = TreeInfo.name(invoke.meth);
>> 515: if (name == names._super) {
>> 516: scanInitializers();
>
> it seems like the code scan initializers every time it finds a super()
> invocation, I guess that this scanning could be done once per class
Yes... I did it that way is so that it doesn't require any adaptation if/when
JDK-8194743 ever gets implemented. And it keeps the code a little simpler in
exchange for a little redundancy.
I'm happy to fix this if you think it is necessary though.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 533:
>
>> 531: if (sym != null &&
>> 532: sym.owner.kind == TYP &&
>> 533: ((ClassSymbol)sym.owner).fullname == names.java_lang_Object
>> &&
>
> nit: in general we use another idiom for this in the compiler:
>
> sym.owner.type.tsym == syms.objectType.tsym
>
> where `syms` is an instance of: `com.sun.tools.javac.code.Symtab`
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 Fri, 13 Jan 2023 15:08:43 GMT, Archie L. Cobbs wrote: >> Something seems to be up with the lint description for this-escape - compare >> this: >> >> >> serial Warn about Serializable classes that do not have a >> serialVersionUID field. >> Also warn about other suspect declarations in >> Serializable and Externalizable classes and interfaces. >> >> with this: >> >> >> this-escape Warn when a constructor invokes a method that could >> be overriden in a subclass; >> such a method would execute before the subclass constructor completes its >> initialization. >> >> >> Indentation seems to be missing, which causes readability issues in the >> `--help-lint` output. > >> I guess I was confused because, while subclasses are a particularly sneaky >> case where uninitialized values can show up, the above leak seems >> potentially dangerous as well... > > Yes - and this very question did come up in the discussions around this > warning (see amber-dev). > > The decision was to go with drawing the "warning boundary" at the compilation > unit. The reasoning is that (a) this aligns with the compiler's "knowledge > boundary", i.e., we can know for sure from code inspection, and also (b) > focuses the warning on the particularly pernicious aspect of these bugs, > which is that they arise from the non-obvious interaction among two or more > files - even when looking at any single one of those files, there doesn't > seem to be any apparent problem. In other words, we decided "not to try to > save any single source code from itself". > > But I think it's still an interesting question. Maybe experience will provide > more guidance over time. > Something seems to be up with the lint description for this-escape - compare > this: Oops, will fix - thanks. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 11:08:33 GMT, Maurizio Cimadamore wrote: >> Perhaps my confusion might come from the name `this-escape` of the lint >> warning - which seems overpromising in this respect. But I looked at the >> description of the lint warning using `javac --help-lint` and I got this: >> >> >> this-escape Warn when a constructor invokes a method that could >> be overriden in a subclass; >> >> >> Which indeed aligns well with what this PR is doing. So that's ok. > > Something seems to be up with the lint description for this-escape - compare > this: > > > serial Warn about Serializable classes that do not have a > serialVersionUID field. > Also warn about other suspect declarations in > Serializable and Externalizable classes and interfaces. > > with this: > > > this-escape Warn when a constructor invokes a method that could be > overriden in a subclass; > such a method would execute before the subclass constructor completes its > initialization. > > > Indentation seems to be missing, which causes readability issues in the > `--help-lint` output. > I guess I was confused because, while subclasses are a particularly sneaky > case where uninitialized values can show up, the above leak seems potentially > dangerous as well... Yes - and this very question did come up in the discussions around this warning (see amber-dev). The decision was to go with drawing the "warning boundary" at the compilation unit. The reasoning is that (a) this aligns with the compiler's "knowledge boundary", i.e., we can know for sure from code inspection, and also (b) focuses the warning on the particularly pernicious aspect of these bugs, which is that they arise from the non-obvious interaction among two or more files - even when looking at any single one of those files, there doesn't seem to be any apparent problem. In other words, we decided "not to try to save any single source code from itself". But I think it's still an interesting question. Maybe experience will provide more guidance over time. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 04:04:36 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 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.
> -
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 04:04:36 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 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.
> -
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 10:58:33 GMT, Maurizio Cimadamore
wrote:
>> 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.
>
> So, to clarify, in this case:
>
>
> import java.util.*;
>
> class B {
> final Object ref;
>
> private B(Object ref) {
> Foo.consume(this);
> this.ref = ref;
> }
> }
>
>
> Even though `this` leaks to a method clearly before it is fully initialized,
> we do not care because there can be no subclass involved observing this. I
> guess I was confused because, while subclasses are a particularly sneaky case
> where uninitialized values can show up, the above leak seems potentially
> dangerous as well - we're effectively leaking a class whose final field has
> not been initialized!
>
> That said, if that was discussed, and it was decided for the warning not to
> deal with this case, that's ok.
Perhaps my confusion might come from the name `this-escape` of the lint warning
- which seems overpromising in this respect. But I looked at the description of
the lint warning using `javac --help-lint` and I got this:
this-escape Warn when a constructor invokes a method that could be
overriden in a subclass;
Which indeed aligns well with what this PR is doing. So that's ok.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 11:05:51 GMT, Maurizio Cimadamore
wrote:
>> So, to clarify, in this case:
>>
>>
>> import java.util.*;
>>
>> class B {
>> final Object ref;
>>
>> private B(Object ref) {
>> Foo.consume(this);
>> this.ref = ref;
>> }
>> }
>>
>>
>> Even though `this` leaks to a method clearly before it is fully initialized,
>> we do not care because there can be no subclass involved observing this. I
>> guess I was confused because, while subclasses are a particularly sneaky
>> case where uninitialized values can show up, the above leak seems
>> potentially dangerous as well - we're effectively leaking a class whose
>> final field has not been initialized!
>>
>> That said, if that was discussed, and it was decided for the warning not to
>> deal with this case, that's ok.
>
> Perhaps my confusion might come from the name `this-escape` of the lint
> warning - which seems overpromising in this respect. But I looked at the
> description of the lint warning using `javac --help-lint` and I got this:
>
>
> this-escape Warn when a constructor invokes a method that could
> be overriden in a subclass;
>
>
> Which indeed aligns well with what this PR is doing. So that's ok.
Something seems to be up with the lint description for this-escape - compare
this:
serial Warn about Serializable classes that do not have a
serialVersionUID field.
Also warn about other suspect declarations in
Serializable and Externalizable classes and interfaces.
with this:
this-escape Warn when a constructor invokes a method that could be
overriden in a subclass;
such a method would execute before the subclass constructor completes its
initialization.
Indentation seems to be missing, which causes readability issues in the
`--help-lint` output.
-
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 21:04:09 GMT, Archie L. Cobbs wrote:
>> 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.
So, to clarify, in this case:
import java.util.*;
class B {
final Object ref;
private B(Object ref) {
Foo.consume(this);
this.ref = ref;
}
}
Even though `this` leaks to a method clearly before it is fully initialized, we
do not care because there can be no subclass involved observing this. I guess I
was confused because, while subclasses are a particularly sneaky case where
uninitialized values can show up, the above leak seems potentially dangerous as
well - we're effectively leaking a class whose final field has not been
initialized!
That said, if that was discussed, and it was decided for the warning not to
deal with this case, that's ok.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14
On Thu, 12 Jan 2023 07:25:07 GMT, Xue-Lei Andrew Fan wrote: >>> This PR does not address all the remaining sprintf:s in hotspot, and with >>> it now explicitly forbidden the build will fail: >>> >> >> This is a question to me as well. I noticed there are still some use of >> sprintf, but the building passed on MacOS and Linux. I was wondering if the >> following update really work (if the '...' parameter works for the >> forbidden?), or something else matters. >> >> >> FORBID_C_FUNCTION(int sprintf(char*, const char*, ...), "use os::snprintf"); >> >> >>> I count ~30 sprintf:s that need updating. >>> >>> I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - >>> are your builds including C2? If so, why are you not running into the issue >>> for those files? >> >> I'm new to hotspot. Do you know how could I enable C2? Thanks! > >> > I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - >> > are your builds including C2? If so, why are you not running into the >> > issue for those files? >> >> I'm new to hotspot. Do you know how could I enable C2? Thanks! > > Never mind, I got it from configuration help message (use > --with-jvm-features=compiler2). > @XueleiFan Could you, please, do not integrate until more cases with the same > problem are fixed? Sure. That's on my plan. I am trying to figure out how to get these files included in the building. My build passed on MacOS and Linux, even with C2 enabled. The update on test may be different from src update. I may divide the fix into two PRs, if I figure out how to build the missing uses of sprintf:s. Thank for for the feedback. I appreciated it. - PR: https://git.openjdk.org/jdk/pull/11935
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. More cases with the same issue were found. - Changes requested by sspitsyn (Reviewer). PR: https://git.openjdk.org/jdk/pull/11935
Re: RFR: 8299635: More test issues for deprecated sprintf in Xcode 14
On Thu, 12 Jan 2023 07:25:07 GMT, Xue-Lei Andrew Fan wrote: >>> This PR does not address all the remaining sprintf:s in hotspot, and with >>> it now explicitly forbidden the build will fail: >>> >> >> This is a question to me as well. I noticed there are still some use of >> sprintf, but the building passed on MacOS and Linux. I was wondering if the >> following update really work (if the '...' parameter works for the >> forbidden?), or something else matters. >> >> >> FORBID_C_FUNCTION(int sprintf(char*, const char*, ...), "use os::snprintf"); >> >> >>> I count ~30 sprintf:s that need updating. >>> >>> I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - >>> are your builds including C2? If so, why are you not running into the issue >>> for those files? >> >> I'm new to hotspot. Do you know how could I enable C2? Thanks! > >> > I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - >> > are your builds including C2? If so, why are you not running into the >> > issue for those files? >> >> I'm new to hotspot. Do you know how could I enable C2? Thanks! > > Never mind, I got it from configuration help message (use > --with-jvm-features=compiler2). @XueleiFan Could you, please, do not integrate until more cases with the same problem are fixed? - PR: https://git.openjdk.org/jdk/pull/11935
Integrated: 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. This pull request has now been integrated. Changeset: be8e6d05 Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/be8e6d05db2f623626506e64a2fb7caf755d5d06 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod 8299957: Enhance error logging in instrument coding with additional jplis_assert_msg Reviewed-by: sspitsyn - PR: https://git.openjdk.org/jdk/pull/11960
