Re: [OpenJDK 2D-Dev] RFR: 8266545: 8261169 broke Harfbuzz build with gcc 7 and 8
On Wed, 5 May 2021 07:54:20 GMT, Thomas Stuefe wrote: > Harfbuzz upgrade broke Linux x64 build on older gccs. For details see JBS > issue. > > I fixed the issue in the harfbuzz sources, but I am not sure of the policy > here. Do we modify the harfbuzz sources or leave them untouched? Advice is > welcome. > > The patch fixes linux x64 fastdebug and opt build for me. FWIW, current jdk master builds fine with gcc 9.3.0. It fails with gcc 6.3.0 to me. I am unaware of the policy of changing the Harfbuzz sources too. If we are changing the Harfbuzz sources, I prefer the multi-line form you did in Harfbuzz PR, though: https://github.com/harfbuzz/harfbuzz/pull/2973 -- it would also simplify the merge. But there is an alternative, disable the warning and then wait for Harfbuzz fix to drop: diff --git a/make/modules/java.desktop/lib/Awt2dLibraries.gmk b/make/modules/java.desktop/lib/Awt2dLibraries.gmk index ff5fa00c720..627aa51ebdf 100644 --- a/make/modules/java.desktop/lib/Awt2dLibraries.gmk +++ b/make/modules/java.desktop/lib/Awt2dLibraries.gmk @@ -465,7 +465,7 @@ else HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits missing-field-initializers strict-aliasing HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder delete-non-virtual-dtor strict-overflow \ -maybe-uninitialized class-memaccess unused-result +maybe-uninitialized class-memaccess unused-result extra HARFBUZZ_DISABLED_WARNINGS_clang := unused-value incompatible-pointer-types \ tautological-constant-out-of-range-compare int-to-pointer-cast \ undef missing-field-initializers range-loop-analysis \ - PR: https://git.openjdk.java.net/jdk/pull/3873
Re: [OpenJDK 2D-Dev] RFR: JDK-8263467: Incorrect double-checked locking in sun.java2d.CRenderer [v2]
On Fri, 12 Mar 2021 07:22:26 GMT, Aleksey Shipilev wrote:
>> SonarCloud reports multiple incorrect double-checked locking cases in
>> `sun.java2d.CRenderer`. For example:
>>
>>
>> Arc2D arcToShape;
>>
>> ...
>> if (arcToShape == null) {
>> synchronized (this) {
>> if (arcToShape == null) {
>> arcToShape = new Arc2D.Float();
>> }
>> }
>> }
>>
>>
>> `Arc2D` contains fields that are not `final`. `arcToShape` is not
>> `volatile`. This makes it an incorrect DCL.
>> This code is used by Mac OS, do it would probably blow up some time later on
>> M1.
>>
>> This is the candidate fix that preserves the semantics. I am doing this fix
>> blindly so far, without testing on real Mac OS. But maybe there is no need
>> to do any of this, because the setter methods overwrite the contents of all
>> these objects under their own sync. So, maybe we can just allocate those
>> objects without DCL-backed caching and pass them in?
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop DCL and useless synchronization completely
Not now, bot.
-
PR: https://git.openjdk.java.net/jdk/pull/2948
[OpenJDK 2D-Dev] Integrated: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
On Thu, 8 Apr 2021 13:00:49 GMT, Aleksey Shipilev wrote: > Noticed this when backporting JDK-8242557: there is a trivial copy-paste > error in exception message. See: > https://hg.openjdk.java.net/jdk/jdk/rev/645c71334acd#l1.58 This pull request has now been integrated. Changeset: 051c117b Author: Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/051c117b Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8264923: PNGImageWriter.write_zTXt throws Exception with a typo Reviewed-by: aivanov, jdv, azvegint, kizune - PR: https://git.openjdk.java.net/jdk/pull/3397
Re: [OpenJDK 2D-Dev] RFR: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
On Thu, 8 Apr 2021 16:55:49 GMT, Alexander Zuev wrote: >> Noticed this when backporting JDK-8242557: there is a trivial copy-paste >> error in exception message. See: >> https://hg.openjdk.java.net/jdk/jdk/rev/645c71334acd#l1.58 > > Marked as reviewed by kizune (Reviewer). Whoa, so many reviews, thanks! :) - PR: https://git.openjdk.java.net/jdk/pull/3397
[OpenJDK 2D-Dev] RFR: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
Noticed this when backporting JDK-8242557: there is a trivial copy-paste error in exception message. See: https://hg.openjdk.java.net/jdk/jdk/rev/645c71334acd#l1.58 - Commit messages: - 8264923: PNGImageWriter.write_zTXt throws Exception with a typo Changes: https://git.openjdk.java.net/jdk/pull/3397/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3397=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264923 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3397.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3397/head:pull/3397 PR: https://git.openjdk.java.net/jdk/pull/3397
Re: [OpenJDK 2D-Dev] RFR: 8264428: Cleanup usages of StringBuffer in java.desktop
On Thu, 1 Apr 2021 12:48:52 GMT, Andrey Turbanov wrote: >> Looks like this change goes beyond of simple replacement of StringBuffer >> with StringBuilder. Please update the description of the bug and PR >> description. > > I've updated PR description, but I don't have rights to update JIRA. I updated the bug synopsis in JIRA. - PR: https://git.openjdk.java.net/jdk/pull/3251
Re: [OpenJDK 2D-Dev] RFR: 8264428: Replace uses of StringBuffer with StringBuilder in java.desktop
On Mon, 29 Mar 2021 20:50:14 GMT, Andrey Turbanov wrote: > Found by IntelliJ IDEA inspection `Java | Java language level migration aids > | Java 5 | 'StringBuffer' may be 'StringBuilder'` > As suggested in > https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created > separate PR for module `java.desktop` > Similar cleanups in the past: > https://bugs.openjdk.java.net/browse/JDK-8041679 > https://bugs.openjdk.java.net/browse/JDK-8264029 Submitted: https://bugs.openjdk.java.net/browse/JDK-8264428, please rename PR to "8264428: Replace uses of StringBuffer with StringBuilder in java.desktop". - PR: https://git.openjdk.java.net/jdk/pull/3251
[OpenJDK 2D-Dev] Integrated: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
On Mon, 22 Mar 2021 15:55:16 GMT, Aleksey Shipilev wrote: > SonarCloud reports the problem in ComponentSampleModel.equals: > Correct one of the identical sub-expressions on both sides of operator "&&" > > ...near "this.numBands == that.numBands". It is checked twice. hashCode also > processes it twice. This pull request has now been integrated. Changeset: cb776edf Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/cb776edf Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice Reviewed-by: serb, azvegint - PR: https://git.openjdk.java.net/jdk/pull/3125
Re: [OpenJDK 2D-Dev] RFR: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
On Tue, 23 Mar 2021 22:17:56 GMT, Sergey Bylokhov wrote: >> Please do. I'll wait for a bit. > > I have found this bug, and the fix for it just implemented both methods at > once, so this is a typo. > https://bugs.openjdk.java.net/browse/JDK-4430788 Brilliant, thanks. I'll integrate now. - PR: https://git.openjdk.java.net/jdk/pull/3125
Re: [OpenJDK 2D-Dev] RFR: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
On Mon, 22 Mar 2021 18:01:49 GMT, Sergey Bylokhov wrote: >> I am a bit confused by this comment :) I suspect that `equals` got >> duplicated `numBands` first, and then whoever did the `hashcode` just hashed >> the same fields in the same order, duplicating it again. > > I hope so, and not in the opposite order. I'll try to check the history f > this file. Please do. I'll wait for a bit. - PR: https://git.openjdk.java.net/jdk/pull/3125
Re: [OpenJDK 2D-Dev] RFR: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
On Mon, 22 Mar 2021 17:44:03 GMT, Sergey Bylokhov wrote: >> SonarCloud reports the problem in ComponentSampleModel.equals: >> Correct one of the identical sub-expressions on both sides of operator "&&" >> >> ...near "this.numBands == that.numBands". It is checked twice. hashCode also >> processes it twice. > > src/java.desktop/share/classes/java/awt/image/ComponentSampleModel.java line > 1228: > >> 1226: } >> 1227: hash ^= numBands; >> 1228: hash <<= 8; > > Since this mistake was done in two places I think this is a typo here and not > an intentional thing. I am a bit confused by this comment :) I suspect that `equals` got duplicated `numBands` first, and then whoever did the `hashcode` just hashed the same fields in the same order, duplicating it again. - PR: https://git.openjdk.java.net/jdk/pull/3125
[OpenJDK 2D-Dev] RFR: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
SonarCloud reports the problem in ComponentSampleModel.equals: Correct one of the identical sub-expressions on both sides of operator "&&" ...near "this.numBands == that.numBands". It is checked twice. hashCode also processes it twice. - Commit messages: - Update copyright line - JDK-8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice Changes: https://git.openjdk.java.net/jdk/pull/3125/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3125=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263981 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3125.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3125/head:pull/3125 PR: https://git.openjdk.java.net/jdk/pull/3125
Re: [OpenJDK 2D-Dev] RFR: JDK-8263467: Incorrect double-checked locking in sun.java2d.CRenderer
On Fri, 12 Mar 2021 03:19:12 GMT, Sergey Bylokhov wrote:
>> SonarCloud reports multiple incorrect double-checked locking cases in
>> `sun.java2d.CRenderer`. For example:
>>
>> Arc2D arcToShape;
>>
>> ...
>> if (arcToShape == null) {
>> synchronized (this) {
>> if (arcToShape == null) {
>> arcToShape = new Arc2D.Float();
>> }
>> }
>> }
>>
>> `Arc2D` contains fields that are not `final`. `arcToShape` is not
>> `volatile`. This makes it an incorrect DCL.
>> This code is used by Mac OS, do it would probably blow up some time later on
>> M1.
>>
>> This is the candidate fix that preserves the semantics. I am doing this fix
>> blindly so far, without testing on real Mac OS. But maybe there is no need
>> to do any of this, because the setter methods overwrite the contents of all
>> these objects under their own sync. So, maybe we can just allocate those
>> objects without DCL-backed caching and pass them in?
>
> I guess "Incorrect double-checked" is a too strict description of this code,
> it just tried to eliminate the creation of one object and do not care about
> the content of that object, since it is immediately overridden.
>
> I think It will be more clear to just eliminate this DCL since overhead and
> code complexity seems too much to just cache five objects.
>
> Probably it was important in past(jdk6 and below) when this renderer was used
> for onscreen rendering. Currently, it is used for printing(if actually used
> at all).
Ok, I removed the "DCL" from that code and used the objects directly. AFAICS,
synchronization is redundant on those local objects. What would be a good test
for this change?
-
PR: https://git.openjdk.java.net/jdk/pull/2948
Re: [OpenJDK 2D-Dev] RFR: JDK-8263467: Incorrect double-checked locking in sun.java2d.CRenderer [v2]
> SonarCloud reports multiple incorrect double-checked locking cases in
> `sun.java2d.CRenderer`. For example:
>
> Arc2D arcToShape;
>
> ...
> if (arcToShape == null) {
> synchronized (this) {
> if (arcToShape == null) {
> arcToShape = new Arc2D.Float();
> }
> }
> }
>
> `Arc2D` contains fields that are not `final`. `arcToShape` is not `volatile`.
> This makes it an incorrect DCL.
> This code is used by Mac OS, do it would probably blow up some time later on
> M1.
>
> This is the candidate fix that preserves the semantics. I am doing this fix
> blindly so far, without testing on real Mac OS. But maybe there is no need to
> do any of this, because the setter methods overwrite the contents of all
> these objects under their own sync. So, maybe we can just allocate those
> objects without DCL-backed caching and pass them in?
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Drop DCL and useless synchronization completely
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2948/files
- new: https://git.openjdk.java.net/jdk/pull/2948/files/c81b9ea2..d912f559
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2948=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2948=00-01
Stats: 80 lines in 1 file changed: 0 ins; 70 del; 10 mod
Patch: https://git.openjdk.java.net/jdk/pull/2948.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2948/head:pull/2948
PR: https://git.openjdk.java.net/jdk/pull/2948
[OpenJDK 2D-Dev] RFR: JDK-8263467: Incorrect double-checked locking in sun.java2d.CRenderer
SonarCloud reports multiple incorrect double-checked locking cases in
`sun.java2d.CRenderer`. For example:
Arc2D arcToShape;
...
if (arcToShape == null) {
synchronized (this) {
if (arcToShape == null) {
arcToShape = new Arc2D.Float();
}
}
}
`Arc2D` contains fields that are not `final`. `arcToShape` is not `volatile`.
This makes it an incorrect DCL.
This code is used by Mac OS, do it would probably blow up some time later on M1.
This is the candidate fix that preserves the semantics. I am doing this fix
blindly so far, without testing on real Mac OS. But maybe there is no need to
do any of this, because the setter methods overwrite the contents of all these
objects under their own sync. So, maybe we can just allocate those objects
without DCL-backed caching and pass them in?
-
Commit messages:
- JDK-8263467: Incorrect double-checked locking in sun.java2d.CRenderer
Changes: https://git.openjdk.java.net/jdk/pull/2948/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2948=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8263467
Stats: 50 lines in 1 file changed: 15 ins; 0 del; 35 mod
Patch: https://git.openjdk.java.net/jdk/pull/2948.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2948/head:pull/2948
PR: https://git.openjdk.java.net/jdk/pull/2948
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory [v2]
On Wed, 27 Jan 2021 13:09:56 GMT, Matthias Baesken wrote:
>> The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2
>> times malloc ; however the memory is not always freed correctly in case of
>> errors.
>> See also the related sonar issue :
>> https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8260426
This looks fine to me, modulo stylistic nits
src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 1289:
> 1287: /* failure if any of mallocs failed */
> 1288: if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL) {
> 1289: if (gpdata->pointTypes != NULL) { free(gpdata->pointTypes);
> gpdata->pointTypes = NULL; }
You might want to add an extra space before `gpdata->pointTypes = NULL;` to
align the statements vertically. You call.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory [v2]
On Thu, 28 Jan 2021 07:53:44 GMT, Aleksey Shipilev wrote:
>> Matthias Baesken has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8260426
>
> This looks fine to me, modulo stylistic nits
Pull from master to get x86_32 GHA jobs fixed and running.
> src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 1289:
>
>> 1287: /* failure if any of mallocs failed */
>> 1288: if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL) {
>> 1289: if (gpdata->pointTypes != NULL) { free(gpdata->pointTypes);
>> gpdata->pointTypes = NULL; }
>
> You might want to add an extra space before `gpdata->pointTypes = NULL;` to
> align the statements vertically. You call.
Can we also style it consistently, like:
if (gpdata->pointTypes != NULL) {
free(gpdata->pointTypes);
gpdata->pointTypes = NULL;
}
if (gpdata->pointCoords != NULL) {
free(gpdata->pointCoords);
gpdata->pointCoords = NULL;
}
-
PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory
On Wed, 27 Jan 2021 11:59:29 GMT, Matthias Baesken wrote:
>> src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 1290:
>>
>>> 1288:
>>> 1289: /* failure if any of mallocs failed */
>>> 1290: if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL)
>>
>> I think it would be cleaner to free the remaining allocations on the failing
>> path:
>>
>> if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL) {
>>if (gpdata->pointTypes != NULL) free(gpdata->pointTypes);
>>if (gpdata->pointCoords != NULL) free(gpdata->pointCoords);
>>return 0;
>> } else {
>>return 1;
>> }
>
> Then we would free as well for the realloc code path
> } else {
> /* do we have enough space? */
> ...
> }
> Is this okay?
Yes, I think that is better: `realloc` can fail as well, and we don't want to
leak those?
-
PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory
On Wed, 27 Jan 2021 08:39:33 GMT, Matthias Baesken wrote:
> The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2
> times malloc ; however the memory is not always freed correctly in case of
> errors.
> See also the related sonar issue :
> https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG
Changes requested by shade (Reviewer).
src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 1290:
> 1288:
> 1289: /* failure if any of mallocs failed */
> 1290: if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL)
I think it would be cleaner to free the remaining allocations on the failing
path:
if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL) {
if (gpdata->pointTypes != NULL) free(gpdata->pointTypes);
if (gpdata->pointCoords != NULL) free(gpdata->pointCoords);
return 0;
} else {
return 1;
}
-
PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: 8196086: java/awt/image/DrawImage/IncorrectSourceOffset.java fails
On Sun, 22 Nov 2020 07:36:12 GMT, Sergey Bylokhov wrote: > These tests draw some specific pattern to the VolatileImage and to the > BufferedImage, and then compare pixels. > > The test uses the getSnapshot() method to get the pixels from VolatileImage, > and this method produces some interpolation "artifacts" if the fractional > scale is used in the system(like 125%). > > The solution is to use some predefined scale to pass the tests, to reproduce > initial bug scale=1 is enough. > One test fails because of the bug in the XRender pipeline. Thanks for the explanation. Looks fine to me then. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1372
Re: [OpenJDK 2D-Dev] RFR: 8196086: java/awt/image/DrawImage/IncorrectSourceOffset.java fails
On Sun, 22 Nov 2020 07:36:12 GMT, Sergey Bylokhov wrote: > These tests draw some specific pattern to the VolatileImage and to the > BufferedImage, and then compare pixels. > > The test uses the getSnapshot() method to get the pixels from VolatileImage, > and this method produces some interpolation "artifacts" if the fractional > scale is used in the system(like 125%). > > The solution is to use some predefined scale to pass the tests, to reproduce > initial bug scale=1 is enough. > One test fails because of the bug in the XRender pipeline. I have a drive-by question: why do some tests run with `-Dsun.java2d.uiScale=3`, and some are not? - PR: https://git.openjdk.java.net/jdk/pull/1372
Re: [OpenJDK 2D-Dev] RFR (XS) 8250605: Linux x86_32 builds fail after JDK-8249821
On 7/27/20 6:53 PM, Philip Race wrote: > OK .. although I hope to come back in JDK 16 and eliminate all disabled > warnings > from the now much smaller libfontmanager : > https://bugs.openjdk.java.net/browse/JDK-8074844 OK, pushed. -- Thanks, -Aleksey signature.asc Description: OpenPGP digital signature
[OpenJDK 2D-Dev] RFR (XS) 8250605: Linux x86_32 builds fail after JDK-8249821
Bug:
https://bugs.openjdk.java.net/browse/JDK-8250605
I believe this happens because int-to-pointer-cast is now enabled for
libfontmanager. Below is the
minimal fix, but I wonder if we should reinstate the disabled warning for clang
(in the same manner)
and msvc (which warning code that one is?). Or we can wait for follow-up bug
reports there...
Minimal fix:
https://cr.openjdk.java.net/~shade/8250605/webrev.01/
Testing: Linux {x86_64, x86_32} builds; jdk-submit (running)
--
Thanks,
-Aleksey
signature.asc
Description: OpenPGP digital signature
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
On 11/15/2018 05:06 PM, Volker Simonis wrote:
> can I please have a review for the following small change:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/
*) I tested it on platform without libxrandr-dev installed, and configure
reported the failure
appopriately.
*) Indent looks off at L2207 here:
2205 static char *get_output_screen_name(JNIEnv *env, int screen) {
2206 #ifdef _AIX
2207 return NULL;
2208 #else
2209 if (!awt_XRRGetScreenResources || !awt_XRRGetOutputInfo) {
Thanks,
-Aleksey
signature.asc
Description: OpenPGP digital signature
Re: [OpenJDK 2D-Dev] JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts
On 02/16/2016 09:23 AM, joe darcy wrote: > Please review the webrev > > http://cr.openjdk.java.net/~darcy/8149896.0/ +1, nice cleanup. -Aleksey (not a Reviewer) signature.asc Description: OpenPGP digital signature
