Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

[David Holmes]

On 18/12/2018 6:02 pm, Baesken, Matthias wrote:

Hi David, thanks for  the update on your internal builds . Same is true for our 
internal builds .

Regarding C99  with -Xa set :



It's not at all clear to me that C99-isms will be allowed if -Xa is set.



The C99 features I tested are allowed when -Xa is set  (tested with SS12 update 
4) -


Thanks for the info. Seems okay for now then.

David


 -Xa  is  set, without  other compile flags  :

bash-4.1$ /compiler/SS12u4-Oct2017/SUNWspro/bin/cc bool.c -Xa -o bool
bash-4.1$ ./bool
b is true.
a: 1

-Xa is set  together with  the old flag forbidding C99 , this leads to a 
lot of compile errors :

bash-4.1$ /compiler/SS12u4-Oct2017/SUNWspro/bin/cc bool.c -xc99=%none  -Xa -o 
bool
"bool.c", line 5: undefined symbol: bool
"bool.c", line 5: syntax error before or at: b
"bool.c", line 6: undefined symbol: b
"bool.c", line 9: syntax error before or at: /
"bool.c", line 12: undefined symbol: a
cc: acomp failed for bool.c

The  example program  contains  bool , C++-style comments  and  declaration of  
a   after the if-statement.

bash-4.1$ more bool.c
#include 
#include 

int main() {
   bool b = true;
   if (b) {
 printf("b is true.\n");
   }
   // C++ style comments
   // decl.
   int a = 1;
   printf("a: %d \n", a);

   return 0;
}


Best regards, Matthias




-Original Message-
From: David Holmes 
Sent: Dienstag, 18. Dezember 2018 01:24
To: Baesken, Matthias ; 2d-
d...@openjdk.java.net; erik.joels...@oracle.com; 'build-
d...@openjdk.java.net' ; awt-
d...@openjdk.java.net; 'magnus.ihse.bur...@oracle.com'

Subject: Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on
Solaris

Our internal builds pass okay.

David

On 18/12/2018 8:02 am, David Holmes wrote:

Hi Matthias,

On 17/12/2018 11:12 pm, Baesken, Matthias wrote:


Hello,  please review

http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/

in my change just -xc99=%none  is removed, so we do not forbid c99
coding.

The -Xa compile flag is kept,  no special additional settings are
needed to compile png/awt .


It's not at all clear to me that C99-isms will be allowed if -Xa is set.

I don't think jdk-submit tests Solaris. I'm putting this through our
internal builds.

Thanks,
David

Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

[David Holmes]

On 18/12/2018 6:56 pm, Baesken, Matthias wrote:

Thanks David,  can I add you as a reviewer ?


Yes.


Unfortunately  the  jdk/jdk  Solaris sparc  results   are  currently  so broken 
   (with or without the change)  that it is hard to tell  what difference it 
really makes ...


This is a build flag change that relates to the source language used and 
the build is fine so I don't see there are any issues. My own tests had 
no new issues in our tiers 1 - 3.


Cheers,
David


Best regards, Matthias




-Original Message-
From: David Holmes 
Sent: Dienstag, 18. Dezember 2018 09:45
To: Baesken, Matthias ; 2d-
d...@openjdk.java.net; erik.joels...@oracle.com; 'build-
d...@openjdk.java.net' ; awt-
d...@openjdk.java.net; 'magnus.ihse.bur...@oracle.com'

Subject: Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on
Solaris

On 18/12/2018 6:02 pm, Baesken, Matthias wrote:

Hi David, thanks for  the update on your internal builds . Same is true for

our internal builds .


Regarding C99  with -Xa set :



It's not at all clear to me that C99-isms will be allowed if -Xa is set.



The C99 features I tested are allowed when -Xa is set  (tested with SS12

update 4) -

Thanks for the info. Seems okay for now then.

David


  -Xa  is  set, without  other compile flags  :

bash-4.1$ /compiler/SS12u4-Oct2017/SUNWspro/bin/cc bool.c -Xa -o bool
bash-4.1$ ./bool
b is true.
a: 1

 -Xa is set  together with  the old flag forbidding C99 , this leads to a 
lot of

compile errors :


bash-4.1$ /compiler/SS12u4-Oct2017/SUNWspro/bin/cc bool.c -

xc99=%none  -Xa -o bool

"bool.c", line 5: undefined symbol: bool
"bool.c", line 5: syntax error before or at: b
"bool.c", line 6: undefined symbol: b
"bool.c", line 9: syntax error before or at: /
"bool.c", line 12: undefined symbol: a
cc: acomp failed for bool.c

The  example program  contains  bool , C++-style comments  and

declaration of  a   after the if-statement.


bash-4.1$ more bool.c
#include 
#include 

int main() {
bool b = true;
if (b) {
  printf("b is true.\n");
}
// C++ style comments
// decl.
int a = 1;
printf("a: %d \n", a);

return 0;
}


Best regards, Matthias




-Original Message-
From: David Holmes 
Sent: Dienstag, 18. Dezember 2018 01:24
To: Baesken, Matthias ; 2d-
d...@openjdk.java.net; erik.joels...@oracle.com; 'build-
d...@openjdk.java.net' ; awt-
d...@openjdk.java.net; 'magnus.ihse.bur...@oracle.com'

Subject: Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on
Solaris

Our internal builds pass okay.

David

On 18/12/2018 8:02 am, David Holmes wrote:

Hi Matthias,

On 17/12/2018 11:12 pm, Baesken, Matthias wrote:


Hello,  please review

http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/

in my change just -xc99=%none  is removed, so we do not forbid c99
coding.

The -Xa compile flag is kept,  no special additional settings are
needed to compile png/awt .


It's not at all clear to me that C99-isms will be allowed if -Xa is set.

I don't think jdk-submit tests Solaris. I'm putting this through our
internal builds.

Thanks,
David

Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

[David Holmes]

Correction: jdk-submit does test Solaris.

David

On 18/12/2018 8:02 am, David Holmes wrote:

Hi Matthias,

On 17/12/2018 11:12 pm, Baesken, Matthias wrote:


Hello,  please review

http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/

in my change just -xc99=%none  is removed, so we do not forbid c99 
coding.


The -Xa compile flag is kept,  no special additional settings are 
needed to compile png/awt .


It's not at all clear to me that C99-isms will be allowed if -Xa is set.

I don't think jdk-submit tests Solaris. I'm putting this through our 
internal builds.


Thanks,
David



Thanks, Matthias



--

Message: 1
Date: Fri, 14 Dec 2018 15:39:26 +0100
From: Magnus Ihse Bursie 
To: Erik Joelsson , build-dev
, "awt-dev@openjdk.java.net"
, 2d-dev <2d-...@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on
Solaris
Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed



On 2018-12-14 12:49, Magnus Ihse Bursie wrote:


13 dec. 2018 kl. 19:07 skrev Erik Joelsson mailto:erik.joels...@oracle.com>>:



On 2018-12-13 02:11, Magnus Ihse Bursie wrote:



-D_XPG6

??

To be honest, I'm not completely sure about this. Without this
define, the build failed with the following error message:
Compiler or options invalid for pre-UNIX 03 X/Open applications and
pre-2001 POSIX applications

This was triggered by the following section in
/usr/include/sys/feature_tests.h:
/*
  * It is invalid to compile an XPG3, XPG4, XPG4v2, or XPG5 
application

  * using c99.  The same is true for POSIX.1-1990, POSIX.2-1992,
POSIX.1b,
  * and POSIX.1c applications. Likewise, it is invalid to compile an
XPG6
  * or a POSIX.1-2001 application with anything other than a c99 or
later
  * compiler.  Therefore, we force an error in both cases.
  */
#if defined(_STDC_C99) && (defined(__XOPEN_OR_POSIX) &&
!defined(_XPG6))
#error "Compiler or options invalid for pre-UNIX 03 X/Open
applications \
 and pre-2001 POSIX applications"
#elif !defined(_STDC_C99) && \
 (defined(__XOPEN_OR_POSIX) && defined(_XPG6))
#error "Compiler or options invalid; UNIX 03 and POSIX.1-2001
applications \
 require the use of c99"
#endif

The solution, as also hinted to by searching for other resolutions
to this error online, was to provide the _XPG6 system define. But
exactly how we end up in feature_tests.h with __XOPEN_OR_POSIX set,
without _XPG6 set, and only when compiling this library and not
others, I don't know. I also don't understand what the XPG standard
refers to, nor what versions 2-5 means or what version 6 has that
differs from them.

By setting this flag, I am telling solaris include headers that we
want to compile using the XPG standard version 6, instead of an
older one. It solves the problem. I am happy enough with this. Are 
you?



It looks like this comes from libpng. It has this in
src/java.desktop//share/native/libsplashscreen/libpng/pngpriv.h:

/* Feature Test Macros.  The following are defined here to ensure
that correctly
  * implemented libraries reveal the APIs libpng needs to build and
hide those
  * that are not needed and potentially damaging to the compilation.
  *
  * Feature Test Macros must be defined before any system header is
included (see
  * POSIX 1003.1 2.8.2 "POSIX Symbols."
  *
  * These macros only have an effect if the operating system supports
either
  * POSIX 1003.1 or C99, or both.  On other operating systems
(particularly
  * Windows/Visual Studio) there is no effect; the OS specific tests
below are
  * still required (as of 2011-05-02.)
  */
#ifndef _POSIX_SOURCE
# define _POSIX_SOURCE 1 /* Just the POSIX 1003.1 and C89 APIs */
#endif

This in turn triggers _XOPEN_OR_POSIX to be defined in
/usr/include/sys/feature_tests.h and so triggers the error.

What I'm not clear about is if libpng is trying to declare that it
should not be compiled with any newer standards, and so by doing
that, we risk introducing problems. Reading in the system header, it
seems the _XPG6 macro is internal and should not be used by the
application. It's derived from _XOPEN_SOURCE=600 or
_POSIX_C_SOURCE=200112L which is what applications should use.


Interesting. We should probably define one, or both of these. Perhaps
globally for all native files and compilers. It might have been the
case that the solstudio compiler set _POSIX_C_SOURCE for us before,
prior to setting -std=c99. The following stack overflow article claims
that this is at least the behavior of gcc/clang:

https://stackoverflow.com/questions/21867897/c89-and-posix-at-the-

same-time



So we might have had an implicit _POSIX_C_SOURCE that we now miss.
That would explain why this starts to fail. I'll see if I can confirm
this the next time I log i

Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

[David Holmes]

On 20/12/2018 1:31 am, Baesken, Matthias wrote:


Sounds like a simpler change, at least for now. Does it pass jdk-submit?



Hello Magnus ,   pushed it to  jdk/submit  , however I do not expect  much info 
from it because David  told me Solaris is not tested there


I was incorrect.


  (and the other platforms are not affected by my path).

However David tested it Oracle-internally   with success .


Yes.


Can I add you and David as reviewers ?


Yes from me.

Thanks,
David



Best regards, Matthias




-Original Message-
From: Magnus Ihse Bursie 
Sent: Montag, 17. Dezember 2018 16:11
To: Baesken, Matthias 
Cc: 2d-...@openjdk.java.net; erik.joels...@oracle.com; build-
d...@openjdk.java.net; awt-dev@openjdk.java.net
Subject: Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on
Solaris

Sounds like a simpler change, at least for now. Does it pass jdk-submit? Do
you intend to push to 12 or 13?

Looks good to me, as long as it doesn't break anything.

/Magnus


17 dec. 2018 kl. 14:12 skrev Baesken, Matthias

:



Hello,  please review

http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/

in my change just -xc99=%none  is removed, so we do not forbid c99

coding.


The -Xa compile flag is kept,  no special additional settings are needed to

compile png/awt .



Thanks, Matthias



--

Message: 1
Date: Fri, 14 Dec 2018 15:39:26 +0100
From: Magnus Ihse Bursie 
To: Erik Joelsson , build-dev
, "awt-dev@openjdk.java.net"
, 2d-dev <2d-...@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on
Solaris
Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed




On 2018-12-14 12:49, Magnus Ihse Bursie wrote:

13 dec. 2018 kl. 19:07 skrev Erik Joelsson mailto:erik.joels...@oracle.com>>:




On 2018-12-13 02:11, Magnus Ihse Bursie wrote:


-D_XPG6

??

To be honest, I'm not completely sure about this. Without this
define, the build failed with the following error message:
Compiler or options invalid for pre-UNIX 03 X/Open applications and
pre-2001 POSIX applications

This was triggered by the following section in
/usr/include/sys/feature_tests.h:
/*
* It is invalid to compile an XPG3, XPG4, XPG4v2, or XPG5 application
* using c99.  The same is true for POSIX.1-1990, POSIX.2-1992,
POSIX.1b,
* and POSIX.1c applications. Likewise, it is invalid to compile an
XPG6
* or a POSIX.1-2001 application with anything other than a c99 or
later
* compiler.  Therefore, we force an error in both cases.
*/
#if defined(_STDC_C99) && (defined(__XOPEN_OR_POSIX) &&
!defined(_XPG6))
#error "Compiler or options invalid for pre-UNIX 03 X/Open
applications \
and pre-2001 POSIX applications"
#elif !defined(_STDC_C99) && \
(defined(__XOPEN_OR_POSIX) && defined(_XPG6))
#error "Compiler or options invalid; UNIX 03 and POSIX.1-2001
applications \
require the use of c99"
#endif

The solution, as also hinted to by searching for other resolutions
to this error online, was to provide the _XPG6 system define. But
exactly how we end up in feature_tests.h with __XOPEN_OR_POSIX

set,

without _XPG6 set, and only when compiling this library and not
others, I don't know. I also don't understand what the XPG standard
refers to, nor what versions 2-5 means or what version 6 has that
differs from them.

By setting this flag, I am telling solaris include headers that we
want to compile using the XPG standard version 6, instead of an
older one. It solves the problem. I am happy enough with this. Are

you?

It looks like this comes from libpng. It has this in
src/java.desktop//share/native/libsplashscreen/libpng/pngpriv.h:

/* Feature Test Macros.  The following are defined here to ensure
that correctly
* implemented libraries reveal the APIs libpng needs to build and
hide those
* that are not needed and potentially damaging to the compilation.
*
* Feature Test Macros must be defined before any system header is
included (see
* POSIX 1003.1 2.8.2 "POSIX Symbols."
*
* These macros only have an effect if the operating system supports
either
* POSIX 1003.1 or C99, or both.  On other operating systems
(particularly
* Windows/Visual Studio) there is no effect; the OS specific tests
below are
* still required (as of 2011-05-02.)
*/
#ifndef _POSIX_SOURCE
# define _POSIX_SOURCE 1 /* Just the POSIX 1003.1 and C89 APIs */
#endif

This in turn triggers _XOPEN_OR_POSIX to be defined in
/usr/include/sys/feature_tests.h and so triggers the error.

What I'm not clear about is if libpng is trying to declare that it
should not be compiled with any newer standards, and so by doing
that, we risk introducing problems. Reading in the system header, it
seems the _XPG6 macro is internal and should not be used by the
application. It's derived from _XOPEN_SOURCE=600 or
_POSIX_C_SOURCE=200112L which is what applications should use.


Interesting. We should probably define one, 

Re: RFR(xs): 8221375: Windows 32bit build (VS2017) broken

[David Holmes]

Hi Thomas,

On 25/03/2019 5:01 pm, Thomas Stüfe wrote:

Hi David,

(added net-dev, awt-dev, security-dev since part of these fixes are in 
their territory)


May be better to split up the reviews, cross-posting that many groups 
gets very messy given most people won't be subscribed to them all - 
myself included.


On Mon, Mar 25, 2019 at 1:34 AM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Thomas,

A few queries, comments and concerns ...

On 25/03/2019 6:58 am, Thomas Stüfe wrote:
 > Hi all,
 >
 > After a long time I tried to build a Windows 32bit VM (VS2017)
and failed;

I'm somewhat surprised as I thought someone was actively doing Windows
32-bit builds out there, plus there are shared code changes that should
also have been caught by non-Windows 32-bit builds. :(


Not sure what others do. I did a 32bit windows build, slowdebug, warning 
enabled, and it failed with those 5+ issues.


 > multiple errors and warnings. Lets reverse the bitrot:
 >
 > cr:
 >

http://cr.openjdk.java.net/~stuefe/webrevs/8221375--windows-32bit-build-(vs2017)-broken-in-many-places/webrev.00/webrev/
 >
 > Issue: https://bugs.openjdk.java.net/browse/JDK-8221375
 >
 > Most of the fixes are trivial: Calling convention mismatches (awt
DTRACE
 > callbacks), printf specifiers etc.
 >
 > Had to supress a warning in os_windows_x86.cpp - I was surprised
by this
 > since this did not look 32bit specifc, do we disable warnings on
Windows
 > 64bit builds?

What version of VS2017? We use VS2017 15.9.6 and we don't disable
warnings.


I use VS2017 CE. Not sure which version spcifically, but my compiler is at

Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26431 for x86


I think that would equate to an older version - 15.7

MSVC++ 14.14 _MSC_VER == 1914 (Visual Studio 2017 version 15.7)

Any chance you can upgrade to latest version? (Especially in light of 
the apparent compiler bug you cite below.)


Thanks,
David
-


 > The error I had in vmStructs.cpp was a bit weird: compiler
complained about
 > an assignment of an enum value defined like this:
 >
 > hash_mask_in_place       = (address_word)hash_mask << hash_shift
 >
 > to an uint64_t variable, complaining about narrowing. I did not
find out
 > what his problem was. In the end, I decided to add an explicit
cast to
 > GENERATE_VM_LONG_CONSTANT_ENTRY(name) (see vmStructs.hpp).

Not at all sure that's the right fix. In markOop.hpp we see that value
gets special treatment on Windows-x64:


#ifndef _WIN64
           ,hash_mask               = right_n_bits(hash_bits),
           hash_mask_in_place       = (address_word)hash_mask <<
hash_shift
#endif
    };

    // Alignment of JavaThread pointers encoded in object header
required
by biased locking
    enum { biased_lock_alignment    = 2 << (epoch_shift + epoch_bits)
    };

#ifdef _WIN64
      // These values are too big for Win64
      const static uintptr_t hash_mask = right_n_bits(hash_bits);
      const static uintptr_t hash_mask_in_place  =
                              (address_word)hash_mask << hash_shift;
#endif

perhaps something special is needed for Windows-x86. I'm unclear how
the
values can be "too big" ??


I banged my head against this for an hour or so and I think this is a 
compiler bug.


What I get is:

warning C4838: conversion from '' to 'uint64_t' requires a narrowing 
conversion


(Note the empty "from" string.)

Here are my tries to provoke the error:

VMLongConstantEntry iii[] = {  { "hallo", 
markOopDesc::hash_mask_in_place }, {0,0}};  <<< this fails
VMLongConstantEntry iii = { "hallo", markOopDesc::hash_mask_in_place };  
  << but this succeeds

uint64_t iii = markOopDesc::hash_mask_in_place;   << this succeeds  too

I have no clue what this means. It is difficult to fix since the 
expression is hidden in such a macro pile. But I think either we go with 
my change or we disable the warning for win32 for the whole section.


 >
 > With this patch we can build with warnings enabled on 32bit and 64bit
 > windows.
 >
 > Since patch fixes both hotspot and JDK parts, I'm sending it to
hs-dev and
 > core-libs-dev.

Don't see anything that actually comes under core-libs-dev. Looks like
one net-dev, one awt-dev and one security-dev. Sorry.


Okay, I will add them.

Cheers,
David
-


Thanks for reviewing,

Thomas

 > Thanks, Thomas
 >

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[David Holmes]

Hi Gerard,

On 14/11/2019 6:05 am, Langer, Christoph wrote:

Hi Gerard,

generally it looks like a nice cleanup.

I've got a patch prepared though, which I was planning on posting tomorrow. It 
is about cleanup for the canonicalize function in libjava. I wanted to use 
jdk_util.h for the function prototype. I had not yet filed a bug but here is 
what I have:
http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/

So maybe you could refrain from removing jdk_util.h or maybe you can hold off 
submitting your change until my cleanup is reviewed?


I'd also suggest not deleting jdk_util.h. It seems very odd to me to 
have jdk_util_md.h with no shared jdk_util.h. If you keep jdk_util.h 
then you don't need to touch a number of the files.


Otherwise this looks like a good cleanup.

Thanks,
David
-


I'll create a bug and post an official review thread tomorrow...

Thanks
Christoph


-Original Message-
From: hotspot-dev  On Behalf Of
Mandy Chung
Sent: Mittwoch, 13. November 2019 20:30
To: gerard ziemski 
Cc: awt-dev@openjdk.java.net; hotspot-dev developers ; core-libs-...@openjdk.java.net
Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
JDK_GetVersionInfo0 and the supporting code"



On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and
related code, since we can get build versions directly from within the
VM itself:

I'm including core-libs and awt in this review because the proposed
fix touches their corresponding files.


bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6



This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago
in particular in HS express support that is no longer applicable.

One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
    // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[David Holmes]

On 15/11/2019 1:13 am, gerard ziemski wrote:

Thank you for the review.

I'm definitively going to wait for Christoph to check in his fix first.

I tried in fact to leave jdk_util.c/.h files in empty without removing 
them from the repo, and even though Mac/Linux were OK with that, 
Solaris/Windows were not.


The .c file can go. The .h file wouldn't be empty as it still has the 
other includes.


David



cheers

On 11/13/19 9:21 PM, David Holmes wrote:

Hi Gerard,

On 14/11/2019 6:05 am, Langer, Christoph wrote:

Hi Gerard,

generally it looks like a nice cleanup.

I've got a patch prepared though, which I was planning on posting 
tomorrow. It is about cleanup for the canonicalize function in 
libjava. I wanted to use jdk_util.h for the function prototype. I had 
not yet filed a bug but here is what I have:

http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/

So maybe you could refrain from removing jdk_util.h or maybe you can 
hold off submitting your change until my cleanup is reviewed?


I'd also suggest not deleting jdk_util.h. It seems very odd to me to 
have jdk_util_md.h with no shared jdk_util.h. If you keep jdk_util.h 
then you don't need to touch a number of the files.


Otherwise this looks like a good cleanup.

Thanks,
David
-


I'll create a bug and post an official review thread tomorrow...

Thanks
Christoph


-Original Message-
From: hotspot-dev  On Behalf Of
Mandy Chung
Sent: Mittwoch, 13. November 2019 20:30
To: gerard ziemski 
Cc: awt-dev@openjdk.java.net; hotspot-dev developers ; core-libs-...@openjdk.java.net
Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
JDK_GetVersionInfo0 and the supporting code"



On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and
related code, since we can get build versions directly from within the
VM itself:

I'm including core-libs and awt in this review because the proposed
fix touches their corresponding files.


bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6



This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago
in particular in HS express support that is no longer applicable.

One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
    // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

Re: [15] Review Request: 8235975 Update copyright year to match last edit in jdk repository for 2014/15/16/17/18

[David Holmes]

Hi Sergey,

I've looked at the hotspot files. Changes seem okay, though for the 
files with dual-copyrights it isn't clear whether the Oracle copyright 
or other copyright should be updated for any given change. But it seems 
to me that if anyone wants their specific copyright updated then it is 
up to them to make that change at the time.


Thanks for doing this tedious work.

David

On 23/12/2019 6:24 am, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 15.

Bug: https://bugs.openjdk.java.net/browse/JDK-8235975
Patch (2 Mb): http://cr.openjdk.java.net/~serb/8235975/webrev.02/open.patch
Fix: http://cr.openjdk.java.net/~serb/8235975/webrev.02

I have updated the source code copyrights by the "update_copyright_year.sh"
script for 2014/15/16/18/19 years, unfortunately, cannot run it for 2017
because of: "JDK-8187443: Forest Consolidation: Move files to unified 
layout"

which touched all files.

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[David Holmes]

On Thu, 10 Sep 2020 10:40:15 GMT, Alan Bateman  wrote:

>> @kevinrushforth thanks. Done.
>> 
>> Similar issues:
>> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014
>> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246
>> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237
>> 
>> could be joined somehow?
>
> The code in java.base was updated to use String::isEmpty in JDK 12 
> (JDK-8215281). There was follow-up in JDK 13 to do
> the same in the java.desktop module (JDK-8223237). Changing the remaining 
> usages make sense although I see that more
> more than half are in tests.  It would be good to hear from security-dev on 
> the changes to the Apache Santuario code
> (in java.xml.crypto module) in case it would be better to contribute those 
> upstream instead. Ditto for the Apache Xalan
> code (in the java.xml module) but it may be significantly forked already that 
> it doesn't matter.  I assume you can use
> JDK-8215014 rather than creating a new issue.

This should be broken up to deal with the files in different functional areas 
under different bugids and PRs. Otherwise
the cross-posting to so many lists is prohibitive. Files in different areas 
need to be reviewed by different teams.
Thank you.

-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[David Holmes]

On Thu, 10 Sep 2020 12:18:51 GMT, Kevin Rushforth  wrote:

>> This should be broken up to deal with the files in different functional 
>> areas under different bugids and PRs. Otherwise
>> the cross-posting to so many lists is prohibitive. Files in different areas 
>> need to be reviewed by different teams.
>> Thank you.
>
> @dholmes-ora raises a good point. Hopefully there is a balance point between 
> a dozen different bugs / pull requests
> each targeting one area and one bug / PR targeting a dozen separate areas. I 
> don't have a good general rule to suggest.
> Maybe @AlanBateman or @jddarcy can offer some advice?

14 cc'd mailing lists is unworkable. You need to be subscribed to lists to 
post, but even if you are a reply-all will
be delayed due to all of the mails being held up for moderator approval due to:

" Too many recipients to the message"

I have a longer email coming once it gets through the moderator approval delay. 
:(

-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[David Holmes]

On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee  wrote:

> Please review this small patch to enable the OSX build using Xcode 12.0.
> 
> Thanks,
> Paul

Changes requested by dholmes (Reviewer).

src/hotspot/share/runtime/sharedRuntime.cpp line 2851:

> 2849: if (buf != NULL) {
> 2850:   CodeBuffer buffer(buf);
> 2851:   short locs_buf[80];

This code is just weird. Why is the buf array not declared to be the desired 
type? If the compiler rejects double
because it isn't relocInfo* then why does it accept short? And if it accepts 
short will it accept int or long long or
int64_t? Using int64_t would be a clearer change. Though semantically this code 
is awful. :( Should it be intptr_t ?

-

PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[David Holmes]

On 10/09/2020 10:07 pm, Dmitriy Dumanskiy wrote:

On Thu, 10 Sep 2020 11:21:28 GMT, David Holmes  wrote:


The code in java.base was updated to use String::isEmpty in JDK 12 
(JDK-8215281). There was follow-up in JDK 13 to do
the same in the java.desktop module (JDK-8223237). Changing the remaining 
usages make sense although I see that more
more than half are in tests.  It would be good to hear from security-dev on the 
changes to the Apache Santuario code
(in java.xml.crypto module) in case it would be better to contribute those 
upstream instead. Ditto for the Apache Xalan
code (in the java.xml module) but it may be significantly forked already that 
it doesn't matter.  I assume you can use
JDK-8215014 rather than creating a new issue.


This should be broken up to deal with the files in different functional areas 
under different bugids and PRs. Otherwise
the cross-posting to so many lists is prohibitive. Files in different areas 
need to be reviewed by different teams.
Thank you.


I have in mind dozens of improvements all over the code like this one. I 
already did some further review and in most
cases, those tiny changes go trough all codebase. I can create PR for every 
separate module, but in general, it would
multiply x5 the number of PRs in total. If you think it's a better way to go, 
no problem, I can do that.


Find a reasonable middle ground. You have around 14 mailing lists cc'd 
here, for changes on 150 files. It is very unlikely anyone will review 
all 150, and files in different areas are, by convention, reviewed by 
Reviewers for those areas. Even if people only look at a subset of 
files, communicating that to you effectively so you can figure out when 
all 150 have been reviewed, is not very practical.


My initial breakdown would be:
- build
- desktop (awt/swing/2d)
- serviceability/jmx (the SA and JVMTI changes)
- security
- core-libs

Also note that while the original PR email went out to 14 lists, most 
people trying to reply-all won't be able to as they don't subscribe to 
all 14 lists, so the review threads will get very fragmented.


Cheers,
David
-


-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR: 8255043: Incorrectly styled copyright text

[David Holmes]

On Tue, 20 Oct 2020 08:17:27 GMT, Sergey Bylokhov  wrote:

> In some files, the copyright text is styled as a JavaDoc comment.
> Most of the affected files are tests, only one product file is affected:
> src/java.sql/share/classes/javax/sql/package-info.java
> 
> The chenge is trivial:
>  - /**
>  + /*
> * Copyright (c)

Seems trivially fine.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/759

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

[David Holmes]

On 16/03/2021 11:58 am, Sergey Bylokhov wrote:

On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen  wrote:


This patch ensure launcher won't crash JVM for the new static Methods from 
local/anonymous class on MacOS.

As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when 
the launcher trying to grab class name to be displayed as the Application name 
on the menu.

The fix is to not setting name, test shows that GUI java application shows 'bin' as the 
application name. It's possible for us to set the name to something more friendly, for 
example, "Java", but I am not sure that should be launcher's responsibility to 
choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d 
environment variable should be responsible to pick such name in case the environment 
variable is not set.


This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and 
the fix looks fine.


Both issues involve a problem trying to use the canonical name, but I'd 
consider both fixes deficient when an alternative name could be used. 
But this isn't my code so ...


David


-

PR: https://git.openjdk.java.net/jdk/pull/2999

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

[David Holmes]

On 16/03/2021 2:59 pm, David Holmes wrote:

On 16/03/2021 11:58 am, Sergey Bylokhov wrote:

On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen  wrote:

This patch ensure launcher won't crash JVM for the new static Methods 
from local/anonymous class on MacOS.


As @dholmes-ora pointed out in the analysis, this is a MacOS specific 
bug when the launcher trying to grab class name to be displayed as 
the Application name on the menu.


The fix is to not setting name, test shows that GUI java application 
shows 'bin' as the application name. It's possible for us to set the 
name to something more friendly, for example, "Java", but I am not 
sure that should be launcher's responsibility to choose such a 
default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d 
environment variable should be responsible to pick such name in case 
the environment variable is not set.


This bug is similar to 
https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine.


Both issues involve a problem trying to use the canonical name, but I'd 
consider both fixes deficient when an alternative name could be used. 


Except I overlooked that this is an anonymous class so no simple name 
either. I agree with Henry's later proposal - fix the crash simply then 
outlaw the usecase later.


Cheers,
David


But this isn't my code so ...

David


-

PR: https://git.openjdk.java.net/jdk/pull/2999

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

[David Holmes]

On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang  wrote:

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> With JEP 411 and the default value of `-Djava.security.manager` becoming 
> `disallow`, tests calling `System.setSecurityManager()`  need 
> `-Djava.security.manager=allow` when launched. This PR covers such changes 
> for tier1 to tier3 (except for the JCK tests).
> 
> To make it easier to focus your review on the tests in your area, this PR is 
> divided into multiple commits for different areas. Mostly the rule is the 
> same as how Skara adds labels, but there are several small tweaks:
> 
> 1. When a file is covered by multiple labels, only one is chosen. I make my 
> best to avoid putting too many tests into `core-libs`. If a file is covered 
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
> `rmi` commit.
> 4. One file not covered by any label -- 
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
> the `swing` commit.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> all files to minimize unnecessary merge conflict.
> 
> Please note that this PR can be integrated before the source changes for JEP 
> 411, as the possible values of this system property was already defined long 
> time ago in JDK 9.
> 
> Most of the change in this PR is a simple adding of 
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it 
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we 
> add the line.
> 
> There are several tests that launch another Java process that needs to call 
> the `System.setSecurityManager()` method, and the system property is added to 
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a 
> shell test).
> 
> 3 langtools tests are added into problem list due to 
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
> 
> 2 SQL tests are moved because they need different options on the `@run` line 
> but they are inside a directory that has a `TEST.properties`:
> 
> rename test/jdk/java/sql/{testng/test/sql/othervm => 
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>  ```
> 
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.

Changes to hotspot-* and serviceability look good.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4071

Re: [8u60] RFR: 8043340: [macosx] Fix hard-wired paths to JavaVM.framework

[David Holmes]

On 15/01/2015 6:23 AM, David DeHaven wrote:


Can someone from hotspot-dev please look at the hotspot changes?


Looks okay to me.

Thanks,
David H.


-DrD-


Hello,

This looks good to me. Thanks for the detailed table!

/Erik

On 2015-01-14 04:09, David DeHaven wrote:

The --with-xcode-path argument is optional, you should also be able to build with Xcode 4 
selected via "sudo xcode-select -switch /path/to/Xcode4.app". I leave MAS 
managed Xcode (currently 6) active as I'm constantly bouncing between projects and it's a 
hassle to have to remember to reset the active toolchain, so I thought it best to allow 
configure to be provided a path.

Ugh. I broke something along the way, this doesn't *quite* work.

xcrun complains with "xcrun: error: missing DEVELOPER_DIR path:"

I think I'm exporting an empty DEVELOPER_DIR. I shall fix and update.

TL;DR: Please review round 2:
http://cr.openjdk.java.net/~ddehaven/8043340/jdk8u/v1/

(I removed generated-configure.sh to reduce the review size, it will be 
re-generated prior to pushing)


I've tested the following configuration scenarios (output from a shell script I 
cobbled together..)

field values:
XC6 - Xcode 6 installed in /Applications/Xcode.app
XC4 - Xcode 4 installed in some other dir
(empty) - Argument not passed to configure

Result meanings:
DEV_DIR set - configure succeeded, DEVELOPER_DIR was properly exported in 
spec.gmk
DEV_DIR NOT set - configure succeeded, DEVELOPER_DIR was not needed (XC4 must 
be selected to achieve this)
Configure failed - Configure properly failed when it detected Xcode > 4

"Selected" Xcode means version reported by xcode-select -p


| Xcode selected | --with-xcode-path | DEVELOPER_DIR | result   |
-
| XC4|   |   | DEV_DIR NOT set  |
-
| XC4|   | XC4   | DEV_DIR set  |
-
| XC4|   | XC6   | Configure failed |
-
| XC4| XC4   |   | DEV_DIR set  |
-
| XC4| XC4   | XC4   | DEV_DIR set  |
-
| XC4| XC4   | XC6   | DEV_DIR set  |
-
| XC4| XC6   |   | Configure failed |
-
| XC4| XC6   | XC4   | Configure failed |
-
| XC4| XC6   | XC6   | Configure failed |
-
| XC6|   |   | Configure failed |
-
| XC6|   | XC4   | DEV_DIR set  |
-
| XC6|   | XC6   | Configure failed |
-
| XC6| XC4   |   | DEV_DIR set  |
-
| XC6| XC4   | XC4   | DEV_DIR set  |
-
| XC6| XC4   | XC6   | DEV_DIR set  |
-
| XC6| XC6   |   | Configure failed |
-
| XC6| XC6   | XC4   | Configure failed |
-
| XC6| XC6   | XC6   | Configure failed |
-

All the results are as expected. Please note that --with-xcode-path overrides 
DEVELOPER_DIR, since that could be set in the environment.

(yeah, I went a little OCD on this...)

-DrD-

Re: RfR JDK-8076552 nightly build break fix

[David Holmes]

Pete,

I think Erik's suggestion in the bug report is more appropriate. If this 
is only a source bundle issue then the build instructions for javadoc 
should either be Windows specific, or else check for source existence.


David

On 8/04/2015 3:51 PM, Pete Brunet wrote:

Please review/approve the following patch.

http://cr.openjdk.java.net/~ptbrunet/JDK-8076552/webrev.01/

The recent push for JDK-8076182 caused a build break, i.e. a problem for
the creation of the Javadoc in the environment used by the nightly
build.  This was because a newly opened package
com.sun.java.accessibility.util was mistakenly located in a windows
directory.  This patch moves the package's files from
jdk/src/windows/classes to jdk/src/share/classes and this should resolve
the build break for the jdk8u-dev nightly.

JPRT builds run OK on solaris, mac, and linux.  As of this writing the
Win jobs haven't started yet but the 64 bit build completed OK on my
local machine.

This patch also had to include the fix for JDK-8051297 "Remove
com.sun.java.accessibility.util.java.awt.ChoiceTranslator".  That file
is dead code and its existence in jdk/src/share/classes causes a
compilation failure, access of a non-existent enum, the reason the file
was planned to be removed.

Thanks, Pete

Re: RfR JDK-8076552 nightly build break fix

[David Holmes]

Hi Pete,

Sorry all this was happening in the wee hours for me :)

On 9/04/2015 3:55 AM, Pete Brunet wrote:

How's this?
http://cr.openjdk.java.net/~ptbrunet/JDK-8076552/webrev.03


Seems like a good temporary fix.

To answer your earlier question to my comment, the $60K question is 
whether this stuff is indeed all platforms or not. If it is then of 
course the files should be relocated and the issue will disappear.


Thanks,
David


On 4/8/15 12:47 PM, Mandy Chung wrote:

I agree with Phil's suggestion and file a bug to follow up the javadoc
build issue.

You can verify the result from make docs that there is no javadoc
generated for this package on windows build.

Mandy

On 4/8/2015 10:29 AM, Phil Race wrote:

Isn't it sufficient to comment out this one line ?

1215 ALL_OTHER_TARGETS += jaccessdocs

.. and add a comment as to why ?

-phil.


On 04/08/2015 10:25 AM, Pete Brunet wrote:

Here is an updated patch.
http://cr.openjdk.java.net/~ptbrunet/JDK-8076552/webrev.02/

It simply removes the com.sun.java.accessibility.util part of the
javadoc generation.

How to better deal with the javadoc generation can be left to later.

Please let me know if this patch meets with your approval.

I have started a local Win build and will start JPRT builds on Linux,
Windows, Solaris, and Mac shortly.

Thanks,
Pete

On 4/8/15 12:51 AM, Pete Brunet wrote:

Please review/approve the following patch.

http://cr.openjdk.java.net/~ptbrunet/JDK-8076552/webrev.01/

The recent push for JDK-8076182 caused a build break, i.e. a
problem for
the creation of the Javadoc in the environment used by the nightly
build.  This was because a newly opened package
com.sun.java.accessibility.util was mistakenly located in a windows
directory.  This patch moves the package's files from
jdk/src/windows/classes to jdk/src/share/classes and this should
resolve
the build break for the jdk8u-dev nightly.

JPRT builds run OK on solaris, mac, and linux.  As of this writing the
Win jobs haven't started yet but the 64 bit build completed OK on my
local machine.

This patch also had to include the fix for JDK-8051297 "Remove
com.sun.java.accessibility.util.java.awt.ChoiceTranslator". That file
is dead code and its existence in jdk/src/share/classes causes a
compilation failure, access of a non-existent enum, the reason the
file
was planned to be removed.

Thanks, Pete

Re: Change submitted with wrong bugId.

[David Holmes]

Including awt-dev.

David

On 12/06/2015 3:04 AM, Vladimir Kozlov wrote:

First, we should let people know about this. (I don't know client-lib
mailing list).

Fortunately 8075505 is AWT backport bug which was fixed already. 8075505
will not appear in changeset since we use main bug id in backport
changesets. So we don't need to blacklist it for jcheck.

I updated 8075506 to resolved state with linked changeset.
Please, add comment there about bug id.

Regards,
Vladimir

On 6/11/15 2:11 AM, Lindenmaier, Goetz wrote:

Hi Vladimir,

I just figured I have an open bug where I already have submitted the
corresponding

webrev: https://bugs.openjdk.java.net/browse/JDK-8075506

This is because the bugID in the change is wrong, and thus the wrong bug
was closed.

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/1c471be03faf

I can’t figure what went wrong, as the webrev is fine:

http://cr.openjdk.java.net/~goetz/webrevs/8075506-aixMem/webrev.00/

Probably I messed up the change when editing the reviewers.

Unfortunately, https://bugs.openjdk.java.net/browse/JDK-8075505

is a closed bug, so I can’t tell whether an important issue was lost.

What can I do about this?  Could you open 8075505 under a new bugID?

Should I make 8075506 a duplicate of 8075505 and close it?

Sorry for the trouble and best regards,

   Goetz.

Historical use of Thread.sleep(0) in the GUI?

[David Holmes]

Hi Folks,

I'm currently looking at some hotspot cleanup surrounding the 
ConvertSleepToYield VM flag. When this flag is true it replaces a call 
to Thread.sleep(0) with a platform yield call; otherwise the platform 
"sleep" function is called with sleep time of 1ms. (Note: no reasonable 
Java code should ever use Thread.sleep(0) :) )


This flag stems back to "pre-historic" days when hotspot was replacing 
the classic VM on Windows, and IIUC only a Solaris sparc implementation 
existed. The default for Windows (and all platforms subsequently ported 
to - including Solaris x86!) was to set the flag true; while for Solaris 
sparc it was set to false. The comment in the code is:


// When ConvertSleepToYield is on, this matches the classic VM 
implementation of

// JVM_Sleep. Critical for similar threading behaviour (Win32)
// It appears that in certain GUI contexts, it may be beneficial to do a 
short sleep

// for SOLARIS

Does anyone have any knowledge of what those GUI contexts may have been? 
(I suspect some old motif based environment.)


As I said, for Solaris x86 the flag value differs from sparc (presumably 
an oversight) for what should be an OS issue I would think. Can anyone 
think of a reason why the GUI on sparc may respond differently to x86 in 
the context of using a sleep(0)?


Finally, as I want to get rid of the flags altogether I first need to 
change the setting on sparc. Are there any specific GUI responsiveness 
tests that I might run to ensure this change has no averse impact?


Thanks,
David

RFR: 8140723: Remove source code conditionalized on JAVASE_EMBEDDED

[David Holmes]

The bug report for this is confidential but quite simply all of the 
little tweaks and knobs we added to the open build and source files to 
support the Java SE Embedded product no longer need to be there for JDK 
9. Many of them have already been removed via other changes but this 
cleans up the rest.


webrev: http://cr.openjdk.java.net/~dholmes/8140723/webrev.jdk/

The changes relate to:
- Java version information
- isEmbedded() check in test code Platform.java
- special AWT build settings for Embedded
- special Toolkit handling for Embedded

In make/lib/Awt2dLibraries.gmk I commented out a setting which was 
seemingly introduced only to support (old) embedded releases:


# decimal constant is unsigned only in ISO C90 (JAVASE_EMBEDDED)
BUILD_LIBAWT_XAWT_XToolkit.c_CFLAGS := -w

which I think pertained to:

#define DEF_AWT_MAX_POLL_TIMEOUT ((uint32_t)40)

Unless AWT folk indicate otherwise I will delete those two lines in 
final push.


Thanks,
David
-

Re: RFR: 8140723: Remove source code conditionalized on JAVASE_EMBEDDED

[David Holmes]

Thanks for the review Alan!

David

On 15/07/2016 6:52 AM, Alan Bateman wrote:



On 14/07/2016 05:25, David Holmes wrote:

The bug report for this is confidential but quite simply all of the
little tweaks and knobs we added to the open build and source files to
support the Java SE Embedded product no longer need to be there for
JDK 9. Many of them have already been removed via other changes but
this cleans up the rest.

webrev: http://cr.openjdk.java.net/~dholmes/8140723/webrev.jdk/

Looks okay to me, hopefully the line in
BUILD_LIBAWT_XAWT_XToolkit.c_CFLAGS can be removed.

-Alan

Re: RFR: 8140723: Remove source code conditionalized on JAVASE_EMBEDDED

[David Holmes]

Thanks for the review Paul!

David

On 14/07/2016 11:12 PM, Paul Sandoz wrote:



On 14 Jul 2016, at 06:25, David Holmes  wrote:

The bug report for this is confidential but quite simply all of the little 
tweaks and knobs we added to the open build and source files to support the 
Java SE Embedded product no longer need to be there for JDK 9. Many of them 
have already been removed via other changes but this cleans up the rest.

webrev: http://cr.openjdk.java.net/~dholmes/8140723/webrev.jdk/

The changes relate to:
- Java version information
- isEmbedded() check in test code Platform.java
- special AWT build settings for Embedded
- special Toolkit handling for Embedded

In make/lib/Awt2dLibraries.gmk I commented out a setting which was seemingly 
introduced only to support (old) embedded releases:

# decimal constant is unsigned only in ISO C90 (JAVASE_EMBEDDED)
BUILD_LIBAWT_XAWT_XToolkit.c_CFLAGS := -w

which I think pertained to:

#define DEF_AWT_MAX_POLL_TIMEOUT ((uint32_t)40)

Unless AWT folk indicate otherwise I will delete those two lines in final push.



+1

Paul.

Re: RFR: 8140723: Remove source code conditionalized on JAVASE_EMBEDDED

[David Holmes]

Can I please get someone from AWT to approve this.

Thanks,
David

On 14/07/2016 2:25 PM, David Holmes wrote:

The bug report for this is confidential but quite simply all of the
little tweaks and knobs we added to the open build and source files to
support the Java SE Embedded product no longer need to be there for JDK
9. Many of them have already been removed via other changes but this
cleans up the rest.

webrev: http://cr.openjdk.java.net/~dholmes/8140723/webrev.jdk/

The changes relate to:
- Java version information
- isEmbedded() check in test code Platform.java
- special AWT build settings for Embedded
- special Toolkit handling for Embedded

In make/lib/Awt2dLibraries.gmk I commented out a setting which was
seemingly introduced only to support (old) embedded releases:

# decimal constant is unsigned only in ISO C90 (JAVASE_EMBEDDED)
BUILD_LIBAWT_XAWT_XToolkit.c_CFLAGS := -w

which I think pertained to:

#define DEF_AWT_MAX_POLL_TIMEOUT ((uint32_t)40)

Unless AWT folk indicate otherwise I will delete those two lines in
final push.

Thanks,
David
-

Re: RFR: 8140723: Remove source code conditionalized on JAVASE_EMBEDDED

[David Holmes]

Thanks Alexandr!

David

On 19/07/2016 9:26 PM, Alexandr Scherbatiy wrote:

The fix looks good to me.

Thanks,
Alexandr.

On 7/16/2016 2:55 AM, David Holmes wrote:

Can I please get someone from AWT to approve this.

Thanks,
David

On 14/07/2016 2:25 PM, David Holmes wrote:

The bug report for this is confidential but quite simply all of the
little tweaks and knobs we added to the open build and source files to
support the Java SE Embedded product no longer need to be there for JDK
9. Many of them have already been removed via other changes but this
cleans up the rest.

webrev: http://cr.openjdk.java.net/~dholmes/8140723/webrev.jdk/

The changes relate to:
- Java version information
- isEmbedded() check in test code Platform.java
- special AWT build settings for Embedded
- special Toolkit handling for Embedded

In make/lib/Awt2dLibraries.gmk I commented out a setting which was
seemingly introduced only to support (old) embedded releases:

# decimal constant is unsigned only in ISO C90 (JAVASE_EMBEDDED)
BUILD_LIBAWT_XAWT_XToolkit.c_CFLAGS := -w

which I think pertained to:

#define DEF_AWT_MAX_POLL_TIMEOUT ((uint32_t)40)

Unless AWT folk indicate otherwise I will delete those two lines in
final push.

Thanks,
David
-

Re: Building openjdk8 on windows 7, tests fail for i18n problems

[David Holmes]

   On Windows DLL's in the JRE's BIN directory cannot be
  found by windows DLL loading as that directory is not
  on the Windows PATH.

  To avoid link error we have to load freetype
explicitly
  before we load fontmanager.

  Note that we do not need to do this for T2K because
  fontmanager.dll does not depend on t2k.dll.

  NB: consider moving freetype wrapper part to separate
  shared library in order to avoid dependency. */
   System.loadLibrary("freetype");
   }
   System.loadLibrary("fontmanager");

   return null;



So I guess it does not find awt, freetype or fontmanager libraries, so
it probably is the freetype library. Now the configure script asked for
the freetype lib to be installed and gave explicit instructions how to
rename teh library - but do I have to do something with that manually so
the runtime finds it then?

The freetype.dll is located inside the bin folder, as well as awt and
fontmanager:

/cygdrive/c/openjdk/jdk8u/build/windows-x86-normal-server-fastdebug/jdk/bin
$ ls
appletviewer.exe  jaas_nt.dll  java-rmi.exe
jjs.pdb keytool.exe   pack200.exe   sunmscapi.pdb
appletviewer.pdb  jaas_nt.pdb  java-rmi.pdb
jli.dll keytool.pdb   pack200.pdb   tnameserv.exe
attach.dlljabswitch.exejavaw.exe
jli.pdb kinit.exe policytool.exetnameserv.pdb
attach.pdbjabswitch.pdbjavaw.pdb
jmap.exekinit.pdb policytool.pdbunpack.dll
awt.dll   jar.exe  jawt.dll
jmap.pdbklist.exe rmic.exe  unpack.pdb
awt.pdb   jar.pdb  jawt.pdb
jpeg.dllklist.pdb rmic.pdb  unpack200.exe
dt_shmem.dll  jarsigner.exeJAWTAccessBridge.dll
jpeg.pdbktab.exe  rmid.exe  verify.dll
dt_shmem.pdb  jarsigner.pdbJAWTAccessBridge.pdb
jps.exe ktab.pdb  rmid.pdb  verify.pdb
dt_socket.dll java.dll JAWTAccessBridge-32.dll
jps.pdb lcms.dll  rmiregistry.exe   w2k_lsa_auth.dll
dt_socket.pdb java.exe JAWTAccessBridge-32.pdb
jrunscript.exe  lcms.pdb  rmiregistry.pdb   w2k_lsa_auth.pdb
extcheck.exe  java.pdb jcmd.exe
jrunscript.pdb  management.dllsawindbg.dll  WindowsAccessBridge.dll
extcheck.pdb  java_crw_demo.dlljcmd.pdb
jsadebugd.exe   management.pdbsawindbg.map  WindowsAccessBridge.pdb
fontmanager.dll   java_crw_demo.pdbjconsole.exe
jsadebugd.pdb   mlib_image.dllsawindbg.pdb
WindowsAccessBridge-32.dll
fontmanager.pdb   JavaAccessBridge.dll jconsole.pdb
jsdt.dll    mlib_image.pdbschemagen.exe
WindowsAccessBridge-32.pdb
freetype.dll ...

So maybe a problem with paths? or this .pdb file missing for
freetype.dll? not sure what that is.

It probably would be easier if FileNotFoundExceptions would actually
give out the filename somewhere instead of a bloated stacktrace, which
is something that really have been bothering me frequently since I wrote
my first applet in 1996...

the cc to jdk8u-dev has to wait, I still have not received the
subscription confirmation request.


On Tue, 20 Dec 2016, David Holmes wrote:


On 20/12/2016 9:34 AM, Peter Koellner wrote:

On Tue, 20 Dec 2016, David Holmes wrote:


On 20/12/2016 7:41 AM, Peter Koellner wrote:

How do I add jvm args to the make test java processes? localized
windows
7 home can't change the locale to en, but setting -Duser.country=US
-Duser-language=en should solve a number of i18n related test
failures...


It might work to run:

make test JAVA_VM_ARGS="-Duser.country=US -Duser-language=en"


Thanks. I'll do that after the next build tomorrow. Can I also increase
the timeouts for the tests somehow?


Try adding:

JTREG_TIMEOUT_OPTION=-timeoutFactor:4

but increase 4 to whatever you think suitable.

BTW I'm determining this by reading jdk/test/Makefile and seeing how
the jtreg tests get run.

David

Re: [RFR]: 8209115: adjust libsplashscreen linux ppc64le builds for easier libpng update - was : RE: RFR 8195615 : libsplashscreen linux ppc64le build error after libpng update

[David Holmes]

Hi Matthias,

On 13/08/2018 4:41 PM, Baesken, Matthias wrote:

Thank‘s !
Can I have a second review please ?


As the build team seem to be on vacation right now I can add that second 
Review. :)


Cheers,
David


Best regards, Matthias


From: Phil Race 
Sent: Freitag, 10. August 2018 20:12
To: Baesken, Matthias 
Cc: 'build-...@openjdk.java.net' ; 
awt-dev@openjdk.java.net
Subject: Re: [RFR]: 8209115: adjust libsplashscreen linux ppc64le builds for 
easier libpng update - was : RE: RFR 8195615 : libsplashscreen linux ppc64le 
build error after libpng update

+1 from me.

-phil.
On 08/10/2018 08:18 AM, Baesken, Matthias wrote:
Hello  , here is a new webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8209115.1/

I set PNG_POWERPC_VSX_OPTnow only on the  linux ppc  platforms .

Best regards, Matthias


From: Baesken, Matthias
Sent: Donnerstag, 9. August 2018 08:48
To: 'Philip Race' 
Cc: 'build-...@openjdk.java.net' 
; 
awt-dev@openjdk.java.net
Subject: RE: [RFR]: 8209115: adjust libsplashscreen linux ppc64le builds for 
easier libpng update - was : RE: RFR 8195615 : libsplashscreen linux ppc64le 
build error after libpng update


Ø  Seems like the ARM one has been harmless for an x64 build

   *   so we could for now assume the PPC one is too

Hi Phil ,
   I  will  guard  the define  for   the platform  where  we want to use  it -  
 I think this is better for the future than just setting it  on all platforms .

And btw.  it has to be

PNG_POWERPC_VSX_OPT   not  PNG_POWERPC_VSX

☹  - my mistake .

For some reason the current setting still builds on our ppc64 (le) test machine 
   in the current codebase , strange !

Best regards, Matthias


From: Philip Race mailto:philip.r...@oracle.com>>
Sent: Donnerstag, 9. August 2018 04:09
To: Baesken, Matthias 
mailto:matthias.baes...@sap.com>>
Cc: 'build-...@openjdk.java.net' 
mailto:build-...@openjdk.java.net>>; 
awt-dev@openjdk.java.net
Subject: Re: [RFR]: 8209115: adjust libsplashscreen linux ppc64le builds for 
easier libpng update - was : RE: RFR 8195615 : libsplashscreen linux ppc64le 
build error after libpng update


The only comment I have is about this

-  LIBSPLASHSCREEN_CFLAGS += -DSPLASHSCREEN -DPNG_NO_MMX_CODE 
-DPNG_ARM_NEON_OPT=0

+  LIBSPLASHSCREEN_CFLAGS += -DSPLASHSCREEN -DPNG_NO_MMX_CODE 
-DPNG_ARM_NEON_OPT=0 -DPNG_POWERPC_VSX=0



I suppose these platform defines are harmless and un-referenced unless building 
for

the specified platform. Seems like the ARM one has been harmless for an x64 
build

so we could for now assume the PPC one is too.

Or you could use jdk-submit to prove this :-)



-phil.

On 8/8/18, 12:59 AM, Baesken, Matthias wrote:
Hello ,
please review this small adjustment for  the build of libsplashscreen ,  
especially the libpng parts .
Back then,  we had to fix the  build of  the libpng parts  in libsplashscreen  
on linux ppc64le   with  “8195615 : libsplashscreen linux ppc64le build error 
after libpng update”  .

However this introduced a small adjustment to pngpriv.h  that needs to be kept 
every time libpng  is updated   (happened recently when Phil updated the lib).
So I better want to remove the adjustment  to  pngpriv   from   8195615 ,  and 
instead change the build settings to fix the compilation  on linuxppc64 le .

Webrev/bug :

http://cr.openjdk.java.net/~mbaesken/webrevs/8209115.0/

https://bugs.openjdk.java.net/browse/JDK-8209115

Thanks ,
   Matthias



From: Philip Race 
Sent: Dienstag, 7. August 2018 17:15
To: Baesken, Matthias 

Cc: Doerr, Martin ; 
2d-...@openjdk.java.net; Simonis, Volker 
; Lindenmaier, Goetz 

Subject: Re: libsplashscreen compilation on ppc64 ( le) - was : RE: RFR 8195615 
: libsplashscreen linux ppc64le build error after libpng update

Works for me. Include build-dev on the review.
And splashscreen is considered an AWT feature so it should be awt-dev not 2d-dev
although you may want to reference back to this earlier exchange.

-phil.

On 8/7/18, 8:04 AM, Baesken, Matthias wrote:
Hello,  should  I  prepare a change  setting the  -DPNG_POWERPC_VSX=0   flag  
in the makefile  (see below) ?
Might  make future  libpng updates  more simple .

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 2. August 2018 17:28
To: 'Phil Race' ; Doerr, Martin 

Cc: Simonis, Volker ; Lindenmaier, 
Goetz 
Subject: RE: RFR 8195615 : libsplashscreen linux ppc64le

Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

[David Holmes]

Hi Matthias,

On 17/12/2018 11:12 pm, Baesken, Matthias wrote:


Hello,  please review

http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/

in my change just -xc99=%none  is removed, so we do not forbid c99 coding.

The -Xa compile flag is kept,  no special additional settings are needed to 
compile png/awt .


It's not at all clear to me that C99-isms will be allowed if -Xa is set.

I don't think jdk-submit tests Solaris. I'm putting this through our 
internal builds.


Thanks,
David



Thanks, Matthias



--

Message: 1
Date: Fri, 14 Dec 2018 15:39:26 +0100
From: Magnus Ihse Bursie 
To: Erik Joelsson , build-dev
, "awt-dev@openjdk.java.net"
, 2d-dev <2d-...@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on
Solaris
Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed



On 2018-12-14 12:49, Magnus Ihse Bursie wrote:


13 dec. 2018 kl. 19:07 skrev Erik Joelsson mailto:erik.joels...@oracle.com>>:



On 2018-12-13 02:11, Magnus Ihse Bursie wrote:



-D_XPG6

??

To be honest, I'm not completely sure about this. Without this
define, the build failed with the following error message:
Compiler or options invalid for pre-UNIX 03 X/Open applications and
pre-2001 POSIX applications

This was triggered by the following section in
/usr/include/sys/feature_tests.h:
/*
  * It is invalid to compile an XPG3, XPG4, XPG4v2, or XPG5 application
  * using c99.  The same is true for POSIX.1-1990, POSIX.2-1992,
POSIX.1b,
  * and POSIX.1c applications. Likewise, it is invalid to compile an
XPG6
  * or a POSIX.1-2001 application with anything other than a c99 or
later
  * compiler.  Therefore, we force an error in both cases.
  */
#if defined(_STDC_C99) && (defined(__XOPEN_OR_POSIX) &&
!defined(_XPG6))
#error "Compiler or options invalid for pre-UNIX 03 X/Open
applications \
 and pre-2001 POSIX applications"
#elif !defined(_STDC_C99) && \
 (defined(__XOPEN_OR_POSIX) && defined(_XPG6))
#error "Compiler or options invalid; UNIX 03 and POSIX.1-2001
applications \
 require the use of c99"
#endif

The solution, as also hinted to by searching for other resolutions
to this error online, was to provide the _XPG6 system define. But
exactly how we end up in feature_tests.h with __XOPEN_OR_POSIX set,
without _XPG6 set, and only when compiling this library and not
others, I don't know. I also don't understand what the XPG standard
refers to, nor what versions 2-5 means or what version 6 has that
differs from them.

By setting this flag, I am telling solaris include headers that we
want to compile using the XPG standard version 6, instead of an
older one. It solves the problem. I am happy enough with this. Are you?


It looks like this comes from libpng. It has this in
src/java.desktop//share/native/libsplashscreen/libpng/pngpriv.h:

/* Feature Test Macros.  The following are defined here to ensure
that correctly
  * implemented libraries reveal the APIs libpng needs to build and
hide those
  * that are not needed and potentially damaging to the compilation.
  *
  * Feature Test Macros must be defined before any system header is
included (see
  * POSIX 1003.1 2.8.2 "POSIX Symbols."
  *
  * These macros only have an effect if the operating system supports
either
  * POSIX 1003.1 or C99, or both.  On other operating systems
(particularly
  * Windows/Visual Studio) there is no effect; the OS specific tests
below are
  * still required (as of 2011-05-02.)
  */
#ifndef _POSIX_SOURCE
# define _POSIX_SOURCE 1 /* Just the POSIX 1003.1 and C89 APIs */
#endif

This in turn triggers _XOPEN_OR_POSIX to be defined in
/usr/include/sys/feature_tests.h and so triggers the error.

What I'm not clear about is if libpng is trying to declare that it
should not be compiled with any newer standards, and so by doing
that, we risk introducing problems. Reading in the system header, it
seems the _XPG6 macro is internal and should not be used by the
application. It's derived from _XOPEN_SOURCE=600 or
_POSIX_C_SOURCE=200112L which is what applications should use.


Interesting. We should probably define one, or both of these. Perhaps
globally for all native files and compilers. It might have been the
case that the solstudio compiler set _POSIX_C_SOURCE for us before,
prior to setting -std=c99. The following stack overflow article claims
that this is at least the behavior of gcc/clang:

https://stackoverflow.com/questions/21867897/c89-and-posix-at-the-

same-time



So we might have had an implicit _POSIX_C_SOURCE that we now miss.
That would explain why this starts to fail. I'll see if I can confirm
this the next time I log into a Solaris computer.

Of course it was not as simple. Setting:
ifeq ($(OPENJDK_TARGET_OS), solaris)
  LIBSPLASHSCREEN_CFLAGS += -D_POSIX_C_SOURCE=200112L -
D_XOPEN_SOURCE=600
endif

instead made us fail with:
ope

Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

[David Holmes]

Our internal builds pass okay.

David

On 18/12/2018 8:02 am, David Holmes wrote:

Hi Matthias,

On 17/12/2018 11:12 pm, Baesken, Matthias wrote:


Hello,  please review

http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/

in my change just -xc99=%none  is removed, so we do not forbid c99 
coding.


The -Xa compile flag is kept,  no special additional settings are 
needed to compile png/awt .


It's not at all clear to me that C99-isms will be allowed if -Xa is set.

I don't think jdk-submit tests Solaris. I'm putting this through our 
internal builds.


Thanks,
David



Thanks, Matthias



--

Message: 1
Date: Fri, 14 Dec 2018 15:39:26 +0100
From: Magnus Ihse Bursie 
To: Erik Joelsson , build-dev
, "awt-dev@openjdk.java.net"
, 2d-dev <2d-...@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on
Solaris
Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed



On 2018-12-14 12:49, Magnus Ihse Bursie wrote:


13 dec. 2018 kl. 19:07 skrev Erik Joelsson mailto:erik.joels...@oracle.com>>:



On 2018-12-13 02:11, Magnus Ihse Bursie wrote:



-D_XPG6

??

To be honest, I'm not completely sure about this. Without this
define, the build failed with the following error message:
Compiler or options invalid for pre-UNIX 03 X/Open applications and
pre-2001 POSIX applications

This was triggered by the following section in
/usr/include/sys/feature_tests.h:
/*
  * It is invalid to compile an XPG3, XPG4, XPG4v2, or XPG5 
application

  * using c99.  The same is true for POSIX.1-1990, POSIX.2-1992,
POSIX.1b,
  * and POSIX.1c applications. Likewise, it is invalid to compile an
XPG6
  * or a POSIX.1-2001 application with anything other than a c99 or
later
  * compiler.  Therefore, we force an error in both cases.
  */
#if defined(_STDC_C99) && (defined(__XOPEN_OR_POSIX) &&
!defined(_XPG6))
#error "Compiler or options invalid for pre-UNIX 03 X/Open
applications \
 and pre-2001 POSIX applications"
#elif !defined(_STDC_C99) && \
 (defined(__XOPEN_OR_POSIX) && defined(_XPG6))
#error "Compiler or options invalid; UNIX 03 and POSIX.1-2001
applications \
 require the use of c99"
#endif

The solution, as also hinted to by searching for other resolutions
to this error online, was to provide the _XPG6 system define. But
exactly how we end up in feature_tests.h with __XOPEN_OR_POSIX set,
without _XPG6 set, and only when compiling this library and not
others, I don't know. I also don't understand what the XPG standard
refers to, nor what versions 2-5 means or what version 6 has that
differs from them.

By setting this flag, I am telling solaris include headers that we
want to compile using the XPG standard version 6, instead of an
older one. It solves the problem. I am happy enough with this. Are 
you?



It looks like this comes from libpng. It has this in
src/java.desktop//share/native/libsplashscreen/libpng/pngpriv.h:

/* Feature Test Macros.  The following are defined here to ensure
that correctly
  * implemented libraries reveal the APIs libpng needs to build and
hide those
  * that are not needed and potentially damaging to the compilation.
  *
  * Feature Test Macros must be defined before any system header is
included (see
  * POSIX 1003.1 2.8.2 "POSIX Symbols."
  *
  * These macros only have an effect if the operating system supports
either
  * POSIX 1003.1 or C99, or both.  On other operating systems
(particularly
  * Windows/Visual Studio) there is no effect; the OS specific tests
below are
  * still required (as of 2011-05-02.)
  */
#ifndef _POSIX_SOURCE
# define _POSIX_SOURCE 1 /* Just the POSIX 1003.1 and C89 APIs */
#endif

This in turn triggers _XOPEN_OR_POSIX to be defined in
/usr/include/sys/feature_tests.h and so triggers the error.

What I'm not clear about is if libpng is trying to declare that it
should not be compiled with any newer standards, and so by doing
that, we risk introducing problems. Reading in the system header, it
seems the _XPG6 macro is internal and should not be used by the
application. It's derived from _XOPEN_SOURCE=600 or
_POSIX_C_SOURCE=200112L which is what applications should use.


Interesting. We should probably define one, or both of these. Perhaps
globally for all native files and compilers. It might have been the
case that the solstudio compiler set _POSIX_C_SOURCE for us before,
prior to setting -std=c99. The following stack overflow article claims
that this is at least the behavior of gcc/clang:

https://stackoverflow.com/questions/21867897/c89-and-posix-at-the-

same-time



So we might have had an implicit _POSIX_C_SOURCE that we now miss.
That would explain why this starts to fail. I'll see if I can confirm
this the next time I log i

Re: Review Request: 8004151: build-infra: Generating X11 wrapper offset file is not cross compilable (AWT folks look here!)

[David Holmes]

Hi Erik,

I need this pushed to jdk8/build yesterday! :) Seriously!

This is now a blocker for the Profiles integration work. I thought I was 
utilizing 8005855 as a workaround but it never got pushed - instead I 
still have my hack of deleting the X_LIBS and X_CFLAGS from the sizer 
build. I can't have that hack in the repo that will go to PIT this week 
for the Profiles work, but I need something to get cross-compilation 
working.


Thanks,
David

On 19/01/2013 12:05 AM, Erik Joelsson wrote:

Here is a new webrev with my sorts incorporated. This has been baking
for quite a while in build-infra now and seems to be working.

http://cr.openjdk.java.net/~erikj/8004151/webrev.jdk.04/

/Erik

On 2012-12-07 11:05, Erik Joelsson wrote:

(resending to full recepients list)

I just hit another issue. I tried using a jdk8 boot-jdk and the order
of the output was different in the generated sizes file compared to
the one in the repo. Sorting both resulted in a clean diff.

/Erik

On 2012-12-05 14:20, Fredrik Öhrström wrote:

2012-11-29 15:54, Fredrik Öhrström skrev:

2012-11-29 15:36, Erik Joelsson skrev:

I just submitted a patch to build-infra for the dual generation on
all platforms since it breaks comparisons between old and new
build. In general, we can't change behavior in new build without
also changing the old before the old is removed.

Removing sizes.64-solaris-i386 breaks the old build on solaris-x86,
so I have readded it to build-infra.


Thanks Erik! The new and updated webrev is here:

http://cr.openjdk.java.net/~ohrstrom/webrev-8004151-gensrcX11wrapper-v2/




Ok, new webrev incorporating a few more Erik changes. Any comments
from the AWT experts?
http://cr.openjdk.java.net/~ohrstrom/webrev-8004151-gensrcX11wrapper-v3/

//Fredrik

Re: [8] Review Request for 8009911 : [macosx] SWT app freeze when going full screen using Java 7 on Mac

[David Holmes]

Adding core-libs as this is actually a launcher change.

David

On 21/05/2013 4:36 PM, Petr Pchelko wrote:

Hello, AWT Team.

Please review the fix for the issue:
http://bugs.sun.com/view_bug.do?bug_id=8009911
The webrev is available at:
http://cr.openjdk.java.net/~pchelko/8009911/webrev.00/

The problem:
When launching java with -XstatrOnFirstThread we were using dispatch_sync to 
start a Java main on the main thread. This blocked the main dispatch queue, so 
all the subsequent calls to GCD were never invoked. GCD is used a bit in our 
code, but it is used inside Cocoa, for example for fullscreen.

The solution:
We now launch Java main using a performSelectorOnMaintThread, so we do not 
block GCD.

Notes:
1. This also fixes the following SWT bug: 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=36
2. Although I have added OBJC syntax into java_md_macosx.c file, I did not 
rename it to .m, because:
a. It is hard to update an old build system to compile with the new 
file name.
b. It is already compiled with -x objective-c, so I will compile
c. We would have lost all the hg history for this file.

With best regards. Petr.

Re: RFR: 8026404 - Logging in Applet can trigger ACE: access denied ("java.lang.RuntimePermission" "modifyThreadGroup")

[David Holmes]

On 15/10/2013 5:19 AM, Daniel Fuchs wrote:

Adding awt-dev...


The change looks okay to me but I would suggest sending to awt-dev so
that the folks that maintain this area know about it.

On the test then does initializing SunToolkit cause AWT to be
initialized? I just wonder if this test will run headless.

-Alan.



Hi,

Please find below a fix for:

8026404: Logging in Applet can trigger ACE:
  access denied ("java.lang.RuntimePermission"
  "modifyThreadGroup")

In some circumstances the call to JavaAWTAccess.getAppletContext()
will trigger a call to ecx.threadGroup.getParent() == null
The trouble is that this call will require a "modifyThreadGroup"
permission if it happens that ecx.threadGroup.getParent() is
the root thread group.

The fix for that is simple - the call to
 ecx.threadGroup.getParent() == null
needs to be done within a doPrivileged:


Doesn't the absence of the permission indicate that this can't be the 
mainAppContext? Just wondering if catching the security exception was an 
alternative - not that I see any issue with the doPrivileged in this case.





The reg test fails without the fix and passes with it.


Test nit: while(rootTG  <- need a space after while

Thanks for keeping TEST.groups up to date!

David


best regards,

-- daniel

Re: RFR: 8025673: Disable X11 AWT toolkit

[David Holmes]

On 23/10/2013 2:10 PM, David DeHaven wrote:


Updated webrev:

http://cr.openjdk.java.net/~ddehaven/8025673/jdk.2/

Summary of changes since last:
- Added awt_headless to java_props_t (set to NULL by default which does not set 
the property)


Not sure about this part. We already force this property to be set in 
Embedded without needing any changes in the code you have modified and 
I'm not sure if your changes will break what we already do. Why do you 
need to do it?


I'm getting concerned about this change touching stuff outside of OSX.

David


- Replaced the toolkit selection code in java_props_macosx.[ch]. I could have 
just exposed isAquaSession but I wanted to preserve the AWT_TOOLKIT environment 
variable support (no idea if it's actually used or not...).
- Modified WrappedToolkitTest.sh to check that it's wrapping LWCToolkit in 
HeadlessToolkit and now all five awt/Toolkit/Headless jtreg tests pass.

No build system or hotspot changes since the last patch.

-DrD-

Re: RFR: 8025673: Disable X11 AWT toolkit

[David Holmes]

On 24/10/2013 1:11 AM, Artem Ananiev wrote:


On 10/23/2013 4:34 PM, Anthony Petrov wrote:

On 10/23/2013 08:52 AM, David Holmes wrote:

On 23/10/2013 2:10 PM, David DeHaven wrote:


Updated webrev:

http://cr.openjdk.java.net/~ddehaven/8025673/jdk.2/

Summary of changes since last:
- Added awt_headless to java_props_t (set to NULL by default which
does not set the property)


Not sure about this part. We already force this property to be set in
Embedded without needing any changes in the code you have modified and
I'm not sure if your changes will break what we already do. Why do you
need to do it?

I'm getting concerned about this change touching stuff outside of OSX.


I think I agree with David. E.g. I'm not sure I like changing
j/l/System.c for this fix.

Given the time constraints, perhaps going with HToolkit would be a good
enough solution for now?

Artem, what's your opinion?


I'm personally fine with the current version. The fix is now Mac OS X
specific, as sprops.awt_headless is only set within #ifdef MACOSX,


Are we looking at the same code? This change in System.c:

 209 if (sprops->awt_headless) {
 210 PUTPROP(props, "java.awt.headless", sprops->awt_headless);
 211 }

is not in any ifdef.

http://cr.openjdk.java.net/~ddehaven/8025673/jdk.2/src/share/native/java/lang/System.c.frames.html

David H.


 it

won't have any impact on other platforms. At the same time it enables
headless functionality on Mac OS X, while HToolkit doesn't.

Note that it can be improved further. As it stands now, the only purpose
of getPreferredToolkit() is to check if Aqua session is active. The
toolkit we use is always CToolkit, regardless of what this method
returns, so the method can be removed, and isInAquaSession() can be
called instead. Leaving this further cleanup up to David D., though.

Thanks,

Artem


--
best regards,
Anthony



David


- Replaced the toolkit selection code in java_props_macosx.[ch]. I
could have just exposed isAquaSession but I wanted to preserve the
AWT_TOOLKIT environment variable support (no idea if it's actually
used or not...).
- Modified WrappedToolkitTest.sh to check that it's wrapping
LWCToolkit in HeadlessToolkit and now all five awt/Toolkit/Headless
jtreg tests pass.

No build system or hotspot changes since the last patch.

-DrD-

Re: RFR: 8025673: Disable X11 AWT toolkit

[David Holmes]

David,

In src/share/native/java/lang/java_props.h the new field should really 
also be in ifdef MACOSX.


The change to System.c allays my concerns there.

You can also consider the hotspot changes Reviewed.

Thanks,
David H.

On 24/10/2013 1:52 PM, David DeHaven wrote:



Another option (I think would make everyone happy) would be to add a native 
method to LWCToolkit.{java,m} that implements isAquaSession and returns a 
boolean. Call this new method in the static initializer and use it's return 
value to set java.awt.headless before calling initIDs. This will still be done 
early enough that Toolkit.java will end up wrapping LWCToolkit in a 
HeadlessToolkit, since it first initializes the wrapped toolkit *then* checks 
to see if it needs to be wrapped. Then all that code in java_props* and 
System.c could be removed.


I like this idea. But it needs testing.


Cool, I'll poke at it when I get a chance.


Doesn't work in LWCToolkit, GraphicsEnvironment gets initialized first which 
messes things up.

It sort of works in CGraphicsEnvironment. The problem there is 
GraphicsEnvironment.isHeadless() is called at least once prior to 
CGraphicsEnvironment being loaded which then caches the value (false) so just 
changing the system property doesn't do anything. I added a protected method to 
change headless and it works, but I'm not entirely happy with that approach and 
I'm not convinced there won't be other side effects. For one thing, what is 
calling isHeadless before CGE gets loaded and will it cope with the mode 
changing? I don't think we have time to chase down all the issues that might 
manifest from that change, when we know for certain the setting the property in 
System.c works fine.

I think the safest bet at this point is to move all the java_props.awt_headless 
field related code into #ifdef blocks so they only exist on Mac OS X. That will 
address the cross-platform concern and fix this issue. I just thought it would 
be useful for other platforms (especially embedded) to be able to poll their 
environment and set themselves up accordingly...


Updated (hopefully final...) webrev:
http://cr.openjdk.java.net/~ddehaven/8025673/jdk.3/

Changes from last version:
http://cr.openjdk.java.net/~ddehaven/8025673/jdk.3/webrev.delta/index.html

(src/windows/native/java/lang/java_props_md.c only shows up because of the 
delta webrev, it gets reverted back to the way it was)


As a side note, SplashScreen calls isHeadless and dumps a nice ugly stack trace 
in a non-GUI session. It does this in the existing code so that's something 
that'll need to be addressed as a separate issue. Also, the message returned by 
GraphicsEnvironment.getHeadlessMessage() isn't correct for Mac, that should 
either be made more generic or let the platform impl class return that string 
(if it's even known or loaded at the time...).

-DrD-

Re: RFR: Allow using a system installed libpng

[David Holmes]

On 4/02/2014 7:51 AM, Magnus Ihse Bursie wrote:


On 2014-02-03 21:44, Omair Majid wrote:

Whoops. Updated webrevs:
root: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/02/
jdk: http://cr.openjdk.java.net/~omajid/webrevs/system-libpng/02-jdk/


Due to this, I'd like to ask you to please double check that
--with-libpng=system | bundled | invalid-value | , and no
flag at all, all work as expected.

I re-ran all these 4 options. system builds with system, bundled and
 build a bundled copy and invalid-value fails during configure.

Great, thank you. The fix looks good.


Looks good to me too from a build perspective. Have no idea whether 
there may be any issues actually trying to use the system version of 
libpng. Someone from AWT also needs to sign-off.


David


/Magnus

Re: RFR: JDK-8038340: Cleanup and fix sysroot and devkit handling on Linux and Solaris

[David Holmes]

sysroots and tool-chains and dev-kits! oh my!

Seriously, what does all this mean for someone who just wants to build 
the OpenJDK? I use dev-kit for cross-compilation builds but I have no 
idea what these different things are supposed to represent.


David

On 26/03/2014 8:32 PM, Erik Joelsson wrote:

Hello,

(including 2d-dev/awt-dev because I'm changing linker flags on desktop
libs.)

In preparation for upgrading compilers and build platforms, we would
like to get better separation between the two. At least on Linux and
Solaris this is certainly possible. Some work for this was already done
in Jdk 8 but it was never taken all the way through into actual usage.

In this bug, the configure parameters relating to this will be changed to:

--with-sysroot: Sets a sysroot directory that will be propagated to the
compiler (gcc --sysroot) if supported and which will be used by
configure to look for all dependencies like headers and libraries. (old
--with-sys-root will be kept for compatibility as an alias)

--with-toolchain-path: Prepends to the path when looking for compilers
and other target specific tools. Replaces --with-tools-dir, which will
also be kept for compatibility as an alias.

--with-extra-path: Prepends to the path when looking for everything.

--with-devkit: Points to the root of a devkit. A devkit may have a
"devkit.info" in its root detailing values for the above three
parameters. If not, the fallback behavior is what it used to do. (set
toolchain_path to devkit/bin and some options for subdirs for sysroot)


Along with these changes some more was needed and done:

* To get it to work properly on Solaris, the OPENWIN variables were
removed and replaced with X_LIBS and X_CFLAGS. Several unnecessary
runtime paths for awt* libs were removed and some were added where they
were actually needed instead. All the libs have been verified to still
find their dependencies with ldd.

* The devkit generation makefiles for linux (in root/make/devkit) were
updated to the proposed compiler versions and adding the devkit.info file.

* A minor fix in compare.sh that was missed in an earlier patch.

Bug: https://bugs.openjdk.java.net/browse/JDK-8038340
Webrevs:
http://cr.openjdk.java.net/~erikj/8038340/webrev.root.01/
http://cr.openjdk.java.net/~erikj/8038340/webrev.jdk.01/

/Erik

Re: RFR: JDK-8038340: Cleanup and fix sysroot and devkit handling on Linux and Solaris

[David Holmes]

On 26/03/2014 9:57 PM, Magnus Ihse Bursie wrote:


On 2014-03-26 12:51, David Holmes wrote:

sysroots and tool-chains and dev-kits! oh my!

Seriously, what does all this mean for someone who just wants to build
the OpenJDK? I use dev-kit for cross-compilation builds but I have no
idea what these different things are supposed to represent.


You can continue to use --with-devkit as you had. That was one of the
design goals for this change.

The theory is something like this:
* A "sysroot" is a stripped-down root directory of the *target*
platform, containing include files and libraries needed to compile and
link for a target (possibly cross-) platform.
* The toolchain path is one or more directories that contains binaries
that will run on the build platform, but generate code for the target
platform. While the separation is somewhat fuzzy, we use the toolchain
path for "toolchain" binaries, basically, those that compile, link or
otherwise interact with the target platform binaries.
* The extra path is one or more directories that contain any kind of
binaries, e.g. a proper version of zip or grep. If you do not deal with
cross-platform devkits, there is no real difference between extra path
and toolchain path.


So what then is a "dev-kit"? A combined sysroot+tool-chain?

Thanks,
David


And that is what is needed to encapsulate the tools needed to compile
OpenJDK.

/Magnus



David

On 26/03/2014 8:32 PM, Erik Joelsson wrote:

Hello,

(including 2d-dev/awt-dev because I'm changing linker flags on desktop
libs.)

In preparation for upgrading compilers and build platforms, we would
like to get better separation between the two. At least on Linux and
Solaris this is certainly possible. Some work for this was already done
in Jdk 8 but it was never taken all the way through into actual usage.

In this bug, the configure parameters relating to this will be
changed to:

--with-sysroot: Sets a sysroot directory that will be propagated to the
compiler (gcc --sysroot) if supported and which will be used by
configure to look for all dependencies like headers and libraries. (old
--with-sys-root will be kept for compatibility as an alias)

--with-toolchain-path: Prepends to the path when looking for compilers
and other target specific tools. Replaces --with-tools-dir, which will
also be kept for compatibility as an alias.

--with-extra-path: Prepends to the path when looking for everything.

--with-devkit: Points to the root of a devkit. A devkit may have a
"devkit.info" in its root detailing values for the above three
parameters. If not, the fallback behavior is what it used to do. (set
toolchain_path to devkit/bin and some options for subdirs for sysroot)


Along with these changes some more was needed and done:

* To get it to work properly on Solaris, the OPENWIN variables were
removed and replaced with X_LIBS and X_CFLAGS. Several unnecessary
runtime paths for awt* libs were removed and some were added where they
were actually needed instead. All the libs have been verified to still
find their dependencies with ldd.

* The devkit generation makefiles for linux (in root/make/devkit) were
updated to the proposed compiler versions and adding the devkit.info
file.

* A minor fix in compare.sh that was missed in an earlier patch.

Bug: https://bugs.openjdk.java.net/browse/JDK-8038340
Webrevs:
http://cr.openjdk.java.net/~erikj/8038340/webrev.root.01/
http://cr.openjdk.java.net/~erikj/8038340/webrev.jdk.01/

/Erik

Re: Review Request for 8056911: Remove internal API usage from ExtendedRobot class

[David Holmes]

The fix that was pushed for this does not compile:

/opt/jprt/T/P1/053347.jdeploy/s/jdk/src/java.desktop/share/classes/java/awt/Robot.java:565: 
error: unexpected type
if (AccessController.doPrivileged(OSInfo.getOSTypeAction()) 
= OSInfo.OSType.MACOSX) {

 ^
  required: variable
  found:value


Use of = instead of == or !=

David
--

Hi Sergey,

Thanks for review!
I've started a bug to track javadoc changes:
https://bugs.openjdk.java.net/browse/JDK-8062545

Thanks,
Dima

On 10/30/2014 03:33 PM, Sergey Bylokhov wrote:
> Hi, Dmitriy.
> The fix looks good.
>
> - dmitriy.ermashov at oracle.com:
>
>> My fault.
>>
>> Updated version:
>> http://cr.openjdk.java.net/~dermashov/8056911/webrev.05/
>>
>> Thanks,
>> Dima
>>

Re: VM hangs in VS2008 build

[David Holmes]

Hi Artem,

I would guess this is a deadlock with the process lock. But that first stack 
trace is very suspicious as you can't jump into the JVM from the OS critical 
section code (or shouldn't!). The attach-listener thread stack looks wrong 
as well - again jumping to VM code from OS code.


No ideas beyond those observations though - sorry.

David

On 13/07/2010 9:56 PM, Artem Ananiev wrote:

Hi, AWT & HotSpot teams,

I've just experienced a problem with a simple test - see the attached
file. The test shows a file dialog and then hangs when the dialog is
being disposed. I tried to get a stack trace... and failed, both with
jstack and ctrl+break from the console.

When I attached to the process with Visual Studio, I noticed several
suspicious threads:

1. One of the threads with AWT code waits for safe_Malloc() to return,
which in turn waits for JVM code in Monitor::set_owner_implementation():

ntdll.dll!_zwwaitforsingleobj...@12()
ntdll.dll!_zwwaitforsingleobj...@12()
jvm.dll!Monitor::set_owner_implementation(Thread * new_owner=0x)
Line 1307
ntdll.dll!_rtlentercriticalsect...@4()
ntdll.d...@rtlpallocateheap@24()
ntdll.dll!_rtlallocateh...@12()
msvcr90.dll!75293db8()
[Frames below may be incorrect and/or missing, no symbols loaded for
msvcr90.dll]
awt.dll!safe_Malloc(unsigned int size=6) Line 85
awt.dll!CreateLocaleObject(JNIEnv_ * env=0x04601d34, const char *
name=0x002a1e58) Line 539
awt.dll!Java_sun_awt_windows_WInputMethod_getNativeLocale(JNIEnv_ *
env=0x04601d34, _jclass * cls=0x065bf728) Line 299


2. Another thread is in os::free():

ntdll.dll!_zwwaitforsingleobj...@12()
ntdll.dll!_zwwaitforsingleobj...@12()
kernelbase.dll!_getprocaddr...@8()
ntdll.dll!_rtlentercriticalsect...@4()
ntdll.d...@rtlpfreeheap@16()
ntdll.dll!_rtlfreeh...@12()
kernel32.dll!_heapf...@12()
msvcr90.dll!75293c1b()
[Frames below may be incorrect and/or missing, no symbols loaded for
msvcr90.dll]
jvm.dll!os::free(void * memblock=0x01f64d50) Line 602
jvm.dll!ChunkPool::free_all_but(unsigned int n=5) Line 152
jvm.dll!ChunkPoolCleaner::task() Line 195
jvm.dll!PeriodicTask::real_time_tick(unsigned int delay_time=50) Line 60
jvm.dll!WatcherThread::run() Line 1086
jvm.dll!java_start(Thread * thread=0x01f5c800) Line 377


3. One more thread waiting for a memory-related operation - I suspect
it's the thread that should provide the stack trace:

ntdll.dll!_zwwaitforsingleobj...@12()
ntdll.dll!_zwwaitforsingleobj...@12()
jvm.dll!InterfaceSupport::serialize_memory(JavaThread *
thread=0x0148) Line 37
kernel32.dll!_waitforsingleobjecteximplementat...@12()
kernel32.dll!_waitforsingleobj...@8()
jvm.dll!Win32AttachListener::dequeue() Line 233
jvm.dll!AttachListener::dequeue() Line 353
jvm.dll!attach_listener_thread_entry(JavaThread * thread=0x01f4d800,
Thread * __the_thread__=0x01f4d800) Line 376
jvm.dll!JavaThread::thread_main_inner() Line 1402
jvm.dll!java_start(Thread * thread=0x01f4d800) Line 377


Any ideas about what's going? The hang only occurs if I build JDK and
HotSpot using VS2008 - exactly the same JDK7-b99 promoted build (which
is built with VS2003) works fine. If I change the file dialog with a
regular AWT modal dialog, the problem goes away as well.

Thanks,

Artem

[Fwd: Missing -L path for xawt]

[David Holmes]

Total silence on build-dev. Perhaps someone on awt-dev knows what to 
look for?


Please cc me as I'm not a member of awt-dev.

Thanks,
David

 Original Message 
Subject: Missing -L path for xawt
Date: Mon, 06 Dec 2010 20:34:12 +1000
From: David Holmes 
Organization: Oracle Corporation
To: build-dev 

I have a problem building for Solaris-i586. When building libjawt.so I
get missing symbol references that should be coming from libxawt.so.
However the -L path is for lib/i386 whereas xawt.so resides in
lib/i386/xawt, hence it isn't found.

I can't tell why this error is arising. Does anyone have any suggestions
as to what might be the source of the problem?

The other possibility is that the lib is meant to be build with "z
nodefs" rather than "z defs" - though again I'm not sure where this flag
might be being mis-set.

Thanks,
David Holmes

Re: [Fwd: Missing -L path for xawt]

[David Holmes]

Hi Anthony,

Anthony Petrov said the following on 12/07/10 20:29:
We've never seen this problem before. Are you building a clean repo, or 
are there any changes to the makefiles? Is that a full or incremental 
build?


It is a full build and there are some changes to the Makefiles. So far 
I've been unable to make any connection between the problem and my 
changes - hence the request for help in terms of what to look for.


Can someone confirm that:

a) "z defs" is an expected linker argument
b) .../lib/i386/xawt is, or is not, expected to be specific as a -L path

Thanks,
David



--
best regards,
Anthony

On 12/7/2010 3:06 AM, David Holmes wrote:

Total silence on build-dev. Perhaps someone on awt-dev knows what to
look for?

Please cc me as I'm not a member of awt-dev.

Thanks,
David

 Original Message 
Subject: Missing -L path for xawt
Date: Mon, 06 Dec 2010 20:34:12 +1000
From: David Holmes 
Organization: Oracle Corporation
To: build-dev 

I have a problem building for Solaris-i586. When building libjawt.so I
get missing symbol references that should be coming from libxawt.so.
However the -L path is for lib/i386 whereas xawt.so resides in
lib/i386/xawt, hence it isn't found.

I can't tell why this error is arising. Does anyone have any suggestions
as to what might be the source of the problem?

The other possibility is that the lib is meant to be build with "z
nodefs" rather than "z defs" - though again I'm not sure where this flag
might be being mis-set.

Thanks,
David Holmes

Re: [Fwd: Missing -L path for xawt]

[David Holmes]

Hi Artem,

Artem Ananiev said the following on 12/07/10 21:14:


On 12/7/2010 3:06 AM, David Holmes wrote:

Total silence on build-dev. Perhaps someone on awt-dev knows what to
look for?


Could you provide an exact build error you're observing, please?

As far as I know, we shouldn't link to libmawt.so (I assume you meant 
libmawt.so, not libxawt.so, right?) directly as there are multiple 
versions of libmawt (in i386/xawt, i386/headless, i386/motif in JDK6), 
and what libmawt.so should be used is determined in runtime. 
sun/xawt/Makefile doesn't contain -lxawt or -lmawt, which makes me 
believe my assumption is correct.


The error is in linking libjawt.so. There are references to undefined 
symbols - see below.


And yes I meant libmawt not libxawt - sorry about that, got confused 
between the library name and the directory it can be found in.


AFAICS the missing symbols are in libmawt, which is specified with 
-lmawt but I couldn't see where it would be found. However you've now 
told me it should be in headless (which is set by -L) - and there it is, 
but the symbols are not in there - so I think my problem is that 
headless/libmawt.so is not being built right, and that is something I 
can link to my Makefile changes!


Many thanks!

David
-


Rebuilding 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386/libjawt.so 
because of 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/.files_compiled 
mapfile-vers
/java/devtools/i586/SUNWspro/SS12u1/bin/cc  -g  -xO2 
-L/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO 
-xc99=%none -xCC -errshort=tags -Xa  -v -mt -xstrconst -W0,-noglobal 
-m32 -erroff=E_BAD_PRAGMA_PACK_VALUE -KPIC   -DDEBUG -DLOGGING -DDBINFO 
-D__solaris__  -Di586 -DcpuIntel -D_LITTLE_ENDIAN= -Di386 -DTRACING 
-DMACRO_MEMSYS_OPS -DBREAKPTS -I. 
-I/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/CClassHeaders 
-I../../../src/solaris/javavm/export -I../../../src/share/javavm/export 
-I../../../src/share/native/common -I../../../src/solaris/native/common 
-I../../../src/share/native/sun/awt 
-I../../../src/solaris/native/sun/awt -DJAVASE_EMBEDDED 
-I/usr/openwin/include -I../../../src/share/native/sun/awt/debug 
-I../../../src/share/native/sun/awt/image 
-I../../../src/share/native/sun/awt/image/cvutils 
-I../../../src/share/native/sun/awt/alphacomposite 
-I../../../src/share/native/sun/awt/medialib 
-I../../../src/solaris/native/sun/awt/medialib 
-I../../../src/share/native/sun/awt/../java2d/loops 
-I../../../src/share/native/sun/awt/../java2d/pipe 
-I../../../src/share/native/sun/awt/../java2d/opengl 
-I../../../src/solaris/native/sun/awt/../java2d/opengl 
-I../../../src/solaris/native/sun/awt/../java2d/x11 
-I../../../src/share/native/sun/awt/../dc/doe 
-I../../../src/share/native/sun/awt/../dc/path 
-I../../../src/solaris/native/sun/awt/../jdga   -Mmapfile-vers -z defs 
-L/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386 
-xildoff -R\$ORIGIN  -ztext   -G -o 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386/libjawt.so 

/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/jawt.o 

-L/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386 
-L/usr/openwin/lib 
-L/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386/headless 
-lmawt -L/usr/openwin/sfw/lib -lXrender -ljava 
-L/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386/client 
-ljvm -m32 -lc

Undefined   first referenced
 symbol in file
awt_FreeDrawingSurface 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/jawt.o
awt_Unlock 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/jawt.o
awt_GetComponent 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/jawt.o
awt_GetDrawingSurface 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/jawt.o
awt_Lock 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/tmp/sun/sun.awt/jawt/obj_gO/jawt.o
ld: fatal: Symbol referencing errors. No output written to 
/java/embedded/ws/ej2se-master/1.7.0/builds/b02/solaris-i586-ea/../solaris-i586-fastdebug/lib/i386/libjawt.so





Thanks,

Artem


Please cc me as I'm not a member of awt-dev.

Thanks,
David

 Original Message ---

Request for review: 7030063 AWT support for SE-Embedded integration

[David Holmes]

(build-dev cc'ed as there are some makefile changes (Hi Kelly! :) ) )

webrev:

http://cr.openjdk.java.net/~dholmes/7030063/webrev/

This CR integrates support for SE-Embedded into the AWT. The main 
changes are:


- forward-port from JDK6 the replacement of hard-wired X11 paths to use 
OPENWIN_HOME-based paths

- cross-compilation support
- Support for BUILD_HEADLESS_ONLY, which is a specialized build for 
environments where there are no X11 libraries available at build-time. 
X11 headers are still needed due to various dependencies in the native code.

- Customize the tray-icon timeout for slow devices
- Add new HToolkit for headless environments without any xawt support 
(used for SE-Embedded reduced-headless builds for which the VM 
automatically sets -Djava.awt.headless=true)
- Update logic for default toolkit selection on Solaris/Linux (AWT team 
request)
- SE-Embedded specific customizations (conditional on defining 
JAVASE_EMBEDDED in the build variables)
- New AWT event polling algorithm with changed defaults for SE-Embedded 
(SE behaviour remains the same)


Thanks,
David Holmes
Java SE VM Embedded Team

[Fwd: hg: jdk7/tl/jdk: 7030063: AWT support for SE-Embedded integration]

[David Holmes]

FYI - pushed via TL to keep all the SE-Embedded changes together.

David

 Original Message 
Subject: hg: jdk7/tl/jdk: 7030063: AWT support for SE-Embedded integration
Date: Fri, 25 Mar 2011 11:10:37 +
From: david.hol...@oracle.com
To: jdk7-chan...@openjdk.java.net, compiler-...@openjdk.java.net, 
 core-libs-...@openjdk.java.net, serviceability-...@openjdk.java.net, 
  security-...@openjdk.java.net, net-...@openjdk.java.net


Changeset: a2793622a8d8
Author:dholmes
Date:  2011-03-25 07:09 -0400
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a2793622a8d8

7030063: AWT support for SE-Embedded integration
Summary: AWT support for SE-Embedded
Reviewed-by: anthony, art, bobv, collins, alanb

! make/launchers/Makefile
! make/sun/Makefile
! make/sun/awt/mawt.gmk
! make/sun/jawt/Makefile
! make/sun/jpeg/Makefile
! make/sun/security/tools/Makefile
! make/sun/xawt/Makefile
! src/share/classes/java/awt/Toolkit.java
+ src/share/classes/sun/awt/HToolkit.java
! src/solaris/classes/sun/awt/X11/XToolkit.java
! src/solaris/classes/sun/awt/X11/XTrayIconPeer.java
! src/solaris/native/java/lang/java_props_md.c
! src/solaris/native/sun/awt/jawt.c
! src/solaris/native/sun/xawt/XToolkit.c

Urgent Request for review: 7035109 Regression: awt SplashScreen/test18.sh fails - missing mapfile entry

[David Holmes]

Very simple review - the mapfile was missing an entry for a new native 
method added in 7030063 and caused an UnsatisfiedLinkError


http://cr.openjdk.java.net/~dholmes/jdk7-clone/webrev-7035109/

Failing test now passes.

Due to the urgency this will get pushed directly into the TL PIT jdk 
repo so that it will be paired with 7030063 and present in b138.


Thanks,
David

Re: Urgent Request for review: 7035109 Regression: awt SplashScreen/test18.sh fails - missing mapfile entry

[David Holmes]

Phil Race said the following on 04/11/11 02:39:

I think that there can be times when something is extremely cross-area
and that makes it more of a pain if it can't easily be divided up.


The embedded changes were very cross-area and we would have had to put 
back through three different team repos, meaning we wouldn't actually 
get a full working copy until everything propagated through the master. 
Hence everything was pushed through TL/jdk (with approval from build and 
AWT folk)


But I completely agree that running the right tests is a vital part of 
making
sure there are no problems. I don't know if that would have happened any 
faster in this case.


Yes the testing was lacking - mea culpa. This was the last piece to push 
through and the AWT folk insisted that we re-work parts of it compared 
to how it was done in 6u23. The change that introduced the native method 
wasn't specifically tested.


But so is the review. Code changes that cross areas should also be 
posted for

review by the relevant teams. Some times that might save pain down
the line. So if you change awt code, send the review to the AWT team (etc).
I expect core libs would like to know if I decided to change something 
in java.util :-)


Just so there's no misunderstanding these changes were all approved by AWT.

All of the SE Embedded integration work was done in full cooperation 
with the TL, Build and AWT folk and I've immensely grateful for their 
assistance in getting this all pushed through.


This fix has been pushed into the PIT respin for b138.

Cheers,
David



-phil.


On 4/9/2011 8:08 AM, Kumar Srinivasan wrote:

Approved!. I agree with Alan if a change is to be made in
a component, it is best that it is pushed to that component's
forest/repo, where all the necessary/appropriate tests will be
performed on a nightly basis.

Kumar


- alan.bate...@oracle.com wrote:


From: alan.bate...@oracle.com
To: david.hol...@oracle.com
Cc: awt-dev@openjdk.java.net, core-libs-...@openjdk.java.net
Sent: Saturday, April 9, 2011 1:59:17 AM GMT -08:00 US/Canada Pacific
Subject: Re: Urgent Request for review: 7035109 Regression: awt 
SplashScreen/test18.sh fails - missing mapfile entry


David Holmes wrote:

Very simple review - the mapfile was missing an entry for a new

native

method added in 7030063 and caused an UnsatisfiedLinkError

http://cr.openjdk.java.net/~dholmes/jdk7-clone/webrev-7035109/

Failing test now passes.

Due to the urgency this will get pushed directly into the TL PIT jdk
repo so that it will be paired with 7030063 and present in b138.

There is always a bit of risk pushing AWT or other client area changes

to the TL forest as probably very few of us run those tests. The
update
the map file looks good to me.

-Alan.

Re: xawt/libmawt.so and headless/libmawt.so

[David Holmes]

Hi Alan,

Alan Bateman said the following on 04/20/11 01:46:
I'm currently looking at the location of our native libraries (the 
context is jigsaw, not jdk7) and I'm wondering about xawt/libmawt.so and 
headless/libmawt.so. Would I be correct to say that they aren't strictly 
required to be a sub-directory and if given unique names there wouldn't 
be an issue if they were located in the same directory as the other 
native libraries for the desktop area?


libmawt.so is known to the VM. We use it with Embedded to detect if we 
are currently running our headless JRE or not: if xawt/libmawt.so (or 
motif21/libxawt.so) isn't present then it is an Embedded headless JRE.


So any change here would have to be coordinated with a VM change.

Glad I was subscribing to this list :)

David

Re: xawt/libmawt.so and headless/libmawt.so

[David Holmes]

Alan Bateman said the following on 04/20/11 21:01:

David Holmes wrote:


libmawt.so is known to the VM. We use it with Embedded to detect if we 
are currently running our headless JRE or not: if xawt/libmawt.so (or 
motif21/libxawt.so) isn't present then it is an Embedded headless JRE.


So any change here would have to be coordinated with a VM change.

Glad I was subscribing to this list :)
I'm glad too. Do you think this could be done a different way? In 
java.awt.GraphicsEnvironment I see that isHeadless consults 
getHeadlessProperty to determine if we are headless and one of the 
things it checks is whether the DISPLAY variable is set. If this 
additionally checked that libxawt.so is available then would it avoid 
needing code in the VM?


It's a different definition of "headless". The JDK can have headful 
capabilities but operate in headless-mode - all the libs are still 
present. The SE-Embedded headless JRE has had all the headful libs 
stripped out. We use the absence of the libs to recognize that this is a 
headless-JRE and use that to set the java.awt.headless property so that 
the Java libs act in headless-mode. So the VM drives this.


David

Re: xawt/libmawt.so and headless/libmawt.so

[David Holmes]

Alan Bateman said the following on 04/20/11 23:10:

David Holmes wrote:

:
The only glitch I see with that is that the 
GraphicsEnvironment.getHeadlessProperty code is Java code while to 
check for the native lib we'd need native code.
The VM code is just stat'ing the library to check that it exists. It 
looks like we could do the equivalent in 
GraphicsEnvironment.getHeadlessProperty without needing any native code.


But the file to be stat'd is OS dependent is it not?

It's doable I guess, but is it worthwhile? What problem are we trying 
to solve?
In the modules build today then the JDK's native libraries still go into 
lib/ but this may change so that a module's native libs go into 
the module's tree. Any re-organization or changes to the layout of 
native libraries is going to expose dependencies and other issues but 
hopefully that we can't overcome.


Not sure how this use of sub-directories impacts that at all. Surely 
within the AWT module (assuming such a beast exists) it can organize its 
files how it wants?


Cheers,
David

Re: xawt/libmawt.so and headless/libmawt.so

[David Holmes]

Alan Bateman said the following on 04/20/11 21:55:

David Holmes wrote:


It's a different definition of "headless". The JDK can have headful 
capabilities but operate in headless-mode - all the libs are still 
present. The SE-Embedded headless JRE has had all the headful libs 
stripped out. We use the absence of the libs to recognize that this is 
a headless-JRE and use that to set the java.awt.headless property so 
that the Java libs act in headless-mode. So the VM drives this.
I guess I'm wondering if the absence check could be done in the AWT code 
rather than in the VM.


The only glitch I see with that is that the 
GraphicsEnvironment.getHeadlessProperty code is Java code while to check 
for the native lib we'd need native code.


It's doable I guess, but is it worthwhile? What problem are we trying to 
solve?


David

Re: 7044285: VM crashes in server app

[David Holmes]

This isn't a hotspot issue but an AWT (or rather java2d) issue. I've
cc'ed the AWT folk and bcc'd hotspot.

The incident report is mis-filed and I'll report that.

David Holmes

Yasumasa Suenaga said the following on 06/06/11 21:24:
> Hi,
> 
> Our customer's system was also crashed in the same case.
> I check core image, and I suspect overflow of "pDst" in 
> "Java_sun_java2d_loops_MaskFill_MaskFill()"
> 
> In order to fix this problem, I made a patch for typecasting "ptrdiff_t" in 
> PtrCoord macro.
> 
> Please merge this patch if you don't fix this problem yet.
> ("test.c" is not a patch. It is minimal sample of this overflow problem.)
> 
> 
> from hs_err log:
> 
> #
> # An unexpected error has been detected by Java Runtime Environment:
> #
> #  SIGSEGV (0xb) at pc=0x2aabcb644177, pid=27759, tid=1142659392
> #
> # Java VM: OpenJDK 64-Bit Server VM (1.6.0-b09 mixed mode linux-amd64)
> # Problematic frame:
> # C  [libawt.so+0x63177]  IntArgbSrcOverMaskFill+0x127
> #
> # If you would like to submit a bug report, please visit:
> #   http://icedtea.classpath.org/bugzilla
> # The crash happened outside the Java Virtual Machine in native code.
> # See problematic frame for where to report the bug.
> 
> :
> :
> 
> OS:Red Hat Enterprise Linux Server release 5.4 (Tikanga)
> 
> uname:Linux 2.6.18-164.el5 #1 SMP Tue Aug 18 15:51:48 EDT 2009 x86_64
> libc:glibc 2.5 NPTL 2.5
> rlimit: STACK 10240k, CORE infinity, NPROC infinity, NOFILE 65536, AS infinity
> load average:1.04 0.56 0.41
> 
> CPU:total 4 (1 cores per cpu, 1 threads per core) family 6 model 10 stepping 
> 5, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3
> 
> Memory: 4k page, physical 5830108k(39684k free), swap 4192956k(4065544k free)
> 
> vm_info: OpenJDK 64-Bit Server VM (1.6.0-b09) for linux-amd64 JRE 
> (1.6.0-b09), built on Aug  5 2009 11:16:51 by "mockbuild" with gcc 4.1.2 
> 20080704 (Red Hat 4.1.2-44)
> 
> time: Thu Jun  2 21:04:51 2011
> elapsed time: 517630 seconds
> 
> 
> from core image:
> 
> [root@RHEL5-4 T2011060009]# gdb java core.27759
> 
> :
> :
> 
> (gdb) f 7
> #7  0x2aabcb61cd3d in Java_sun_java2d_loops_MaskFill_MaskFill 
> (env=0x2aabcc36f598,
> self=, sg2d=0x441b70c8, sData=,
> comp=, x=50, y=26188, w=32, h=32, 
> maskArray=0x441b7120,
> maskoff=0, maskscan=32) at 
> ../../../src/share/native/sun/java2d/loops/MaskFill.c:85
> 85  ../../../src/share/native/sun/java2d/loops/MaskFill.c: No such file 
> or directory.
> in ../../../src/share/native/sun/java2d/loops/MaskFill.c
> (gdb) p pDst
> $1 = (void *) 0x2aaa8aaea6e0
> (gdb) p rasInfo
> $2 = {bounds = {x1 = 50, y1 = 26188, x2 = 82, y2 = 26220}, rasBase = 
> 0x2aab0a4fc718,
>   pixelBitOffset = 0, pixelStride = 4, scanStride = 82240, lutSize = 0, 
> lutBase = 0x0,
>   invColorTable = 0x0, redErrTable = 0x0, grnErrTable = 0x0, bluErrTable = 
> 0x0,
>   invGrayTable = 0x2aabb15d4d68, priv = {align = 0x3,
> data = "\003\000\000\000\000\000\000\000\030ヌO\nォ*", '\0'  times>, "@\000\000\000\000\000\000\000X\213P爼*\000\000\001", '\0'  times>}}
> 
> 
> "pDst" is calculated in "MaskFill.c" as following:
> 
>void *pDst = PtrCoord(rasInfo.rasBase,
>   rasInfo.bounds.x1, rasInfo.pixelStride,
>   rasInfo.bounds.y1, rasInfo.scanStride);
> 
> 
> "PtrCoord" is defined in "GraphicsPrimitiveMgr.h":
> 
> #define PtrAddBytes(p, b)   ((void *) (((intptr_t) (p)) + (b)))
> #define PtrCoord(p, x, xinc, y, yinc)   PtrAddBytes(p, (y)*(yinc) + 
> (x)*(xinc))
> 
> 
> In this case, "b" in PtrAddBytes macro is
> 
>   (rasInfo.bounds.y1 * rasInfo.scanStride) + (rasInfo.bounds.x1 * 
> rasInfo.pixelStride)
>= (26188 * 82240) + (50 * 4)
>= 2153701320 ( > INT_MAX ( 2147483647 (0x7fff) ))
> 
> "b" sets to be -2141265976. So, "pDst" set to be as following:
> 
> pDst = rasInfo.bounds.rasBase - 2141265976
>  = 0x2aaa8aaea6e0
> 
> 
> pDst should set to be 0x2aab8aaea6e0,
> however, it set to be 0x2aaa8aaea6e0.
> 
> 
> 
> Best regards,
> 
> Yasumasa
> 

Re: KDE Task bar is always on top of fullscreen Java applications

[David Holmes]

goues...@orange.fr said the following on 07/01/11 08:27:

I get this at the end of the compilation:
"/usr/bin/ld: cannot open output file libjvm.so: Too many levels of symbolic 
links"

I tried to recompile after cleaning all. What can I do to work around this 
problem?


Are you building hotspot? This is a quirk in the hotspot makefiles. If 
the link fails to create the libjvm you get a symbolic link that refers 
to itself. Subsequent build attempts don't try to rebuild libjvm as it 
seems to exist but when ld tries to access it you get the "too many 
levels of symbolic links" error.


A full clean should fix it, but then you need to see what the original 
error was caused by.


David Holmes
-


Message du 29/06/11 21:29
De : "Anthony Petrov" 
A : goues...@orange.fr

Copie à : awt-dev@openjdk.java.net
Objet : Re:  KDE Task bar is always on top of fullscreen Java applications

On 6/29/2011 5:59 PM, goues...@orange.fr wrote:

How can I detect whether a window is mapped? When XQueryTree returns zero, does 
it mean that the window is unmapped?
You could use XGetWindowAttributes() and examine the map_state field of 
the XWindowAttributes() structure.


Alternatively, you could pass an additional argument to the 
X11GD_SetFullscreenMode() because at AWT level we always know whether a 
window is mapped (see XBaseWindow.isMapped()).



PS. Your email client seems to replace the correct mailing list address 
"awt-dev@openjdk.java.net" with something strange: 
"awt-...@rea.oracle.com" when pressing Reply All. Could you please 
configure it properly?


--
best regards,
Anthony




I can create another patch, I will do this as soon as possible. I will ask to 
the KDE team that it is up to them not to promote only windows on top of the 
stack.


Message du 29/06/11 15:17
De : "Anthony Petrov" 
A : goues...@orange.fr

Copie à : awt-...@rea.oracle.com
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

Hi Julien,

So in your sample application you first set the window to the full 
screen mode, and only then you setVisible(true) it. In this case the 
EWMH spec states the following:


***
The Window Manager SHOULD honor _NET_WM_STATE whenever a withdrawn 
window requests to be mapped.

***

In other words, the X11GD_SetFullscreenMode() should actually check 
whether the window is currently mapped, and if so, do exactly what it 
currently does. However, if the window is currently unmapped, then we 
indeed have to use the XChangeProperty() call instead of the 
XSendEvent() one.


Please note that in either case we should not set the 
_NET_WM_STATE_ABOVE state. The _NET_WM_STATE_FULLSCREEN alone should 
work just fine. If it doesn't, then this is a problem with KDE.


Could you please try to create such a patch and test it on KDE4?

--
best regards,
Anthony

On 6/29/2011 3:49 PM, goues...@orange.fr wrote:

One guy of the KDE team answered that we have misunderstood the EWMH 
specification, that some window managers derivate from it and that using 
XChangeProperty there has some sense. What should I do now?



Message du 28/06/11 22:13
De : "Anthony Petrov" 
A : "Phil Race" 

, goues...@orange.fr

Copie à : awt-dev@openjdk.java.net
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

Phil: Ah, right! Haven't used to the new rules yet. Thanks for reminding 
us about that.


Julien: I have a few questions about your patch:

1. The xprop output that you've attached to the KDE bug report [1] 
indicates that the full screen window is maximized (i.e. using the 
emulated full screen mode rather than the exclusive one). In this case, 
the behavior is correct. But I assume you did try to do the same with 
the exclusive FS mode enabled, didn't you? Could you please provide an 
xprop output in that case, too?


2. The EWMH specification [2] states that "A Client wishing to change 
the state of a window MUST send a _NET_WM_STATE client message to the 
root window". However, your proposed patch calls XChangeProperty() which 
changes the property manually, and therefore violates the EWMH spec. I 
think that a subsequent XSendEvent() to the root window should be enough 
for our purposes.


3. The comments at the KDE bug report, as well as the EWMH spec (see the 
"Stacking order" section) suggest that a window with the 
_NET_WM_STATE_FULLSCREEN state should already be above any other windows 
(including the _NET_WM_STATE_ABOVE windows). Also, the specification 
states that the latter state should not be used by applications 
directly. Note that the function X11GD_SetFullscreenMode() which you're 
changing with your patch already sets the _NET_WM_STATE_FULLSCREEN state 
to the full screen window, and, according to the EWMH specification, 
that a

Re: KDE Task bar is always on top of fullscreen Java applications

[David Holmes]

goues...@orange.fr said the following on 07/02/11 06:11:

libstdc++ devel package is installed but libstdc.so is not found. How can I 
solve this problem?


Install the missing dev package for your distro.

David




Message du 01/07/11 14:34
De : "David Holmes" 
A : goues...@orange.fr

Copie à : "Anthony Petrov" , awt-dev@openjdk.java.net
Objet : Re:  KDE Task bar is always on top of fullscreen Java applications

goues...@orange.fr said the following on 07/01/11 08:27:

I get this at the end of the compilation:
"/usr/bin/ld: cannot open output file libjvm.so: Too many levels of symbolic 
links"

I tried to recompile after cleaning all. What can I do to work around this 
problem?
Are you building hotspot? This is a quirk in the hotspot makefiles. If 
the link fails to create the libjvm you get a symbolic link that refers 
to itself. Subsequent build attempts don't try to rebuild libjvm as it 
seems to exist but when ld tries to access it you get the "too many 
levels of symbolic links" error.


A full clean should fix it, but then you need to see what the original 
error was caused by.


David Holmes
-


Message du 29/06/11 21:29
De : "Anthony Petrov" 
A : goues...@orange.fr

Copie à : awt-dev@openjdk.java.net
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

On 6/29/2011 5:59 PM, goues...@orange.fr wrote:

How can I detect whether a window is mapped? When XQueryTree returns zero, does 
it mean that the window is unmapped?
You could use XGetWindowAttributes() and examine the map_state field of 
the XWindowAttributes() structure.


Alternatively, you could pass an additional argument to the 
X11GD_SetFullscreenMode() because at AWT level we always know whether a 
window is mapped (see XBaseWindow.isMapped()).



PS. Your email client seems to replace the correct mailing list address 
"awt-dev@openjdk.java.net" with something strange: 
"awt-...@rea.oracle.com" when pressing Reply All. Could you please 
configure it properly?


--
best regards,
Anthony




I can create another patch, I will do this as soon as possible. I will ask to 
the KDE team that it is up to them not to promote only windows on top of the 
stack.


Message du 29/06/11 15:17
De : "Anthony Petrov" 
A : goues...@orange.fr

Copie à : awt-...@rea.oracle.com
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

Hi Julien,

So in your sample application you first set the window to the full 
screen mode, and only then you setVisible(true) it. In this case the 
EWMH spec states the following:


***
The Window Manager SHOULD honor _NET_WM_STATE whenever a withdrawn 
window requests to be mapped.

***

In other words, the X11GD_SetFullscreenMode() should actually check 
whether the window is currently mapped, and if so, do exactly what it 
currently does. However, if the window is currently unmapped, then we 
indeed have to use the XChangeProperty() call instead of the 
XSendEvent() one.


Please note that in either case we should not set the 
_NET_WM_STATE_ABOVE state. The _NET_WM_STATE_FULLSCREEN alone should 
work just fine. If it doesn't, then this is a problem with KDE.


Could you please try to create such a patch and test it on KDE4?

--
best regards,
Anthony

On 6/29/2011 3:49 PM, goues...@orange.fr wrote:

One guy of the KDE team answered that we have misunderstood the EWMH 
specification, that some window managers derivate from it and that using 
XChangeProperty there has some sense. What should I do now?



Message du 28/06/11 22:13
De : "Anthony Petrov" 
A : "Phil Race" 

, goues...@orange.fr

Copie à : awt-dev@openjdk.java.net
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

Phil: Ah, right! Haven't used to the new rules yet. Thanks for reminding 
us about that.


Julien: I have a few questions about your patch:

1. The xprop output that you've attached to the KDE bug report [1] 
indicates that the full screen window is maximized (i.e. using the 
emulated full screen mode rather than the exclusive one). In this case, 
the behavior is correct. But I assume you did try to do the same with 
the exclusive FS mode enabled, didn't you? Could you please provide an 
xprop output in that case, too?


2. The EWMH specification [2] states that "A Client wishing to change 
the state of a window MUST send a _NET_WM_STATE client message to the 
root window". However, your proposed patch calls XChangeProperty() which 
changes the property manually, and therefore violates the EWMH spec. I 
think that a subsequent XSendEvent() to the root window should be enough 
for our purposes.


3. The comments at the KDE bug report, as well as the EWMH spec (see the 
"Stacking order" section) suggest that a window with th

Re: KDE Task bar is always on top of fullscreen Java applications

[David Holmes]

goues...@orange.fr said the following on 07/02/11 08:54:

The problem is that I don't find a package containing this file, I looked at 
rpmfind.net and in the official repository. I already have libstdc++.so. I 
installed all packages about GCC and standard C++.


Hmmm a google search doesn't reveal libstdc.so as being an real entity. 
Now I think about it it should just be libc - no "std".


Does the linker error specifically say libstdc.so? Can you show the link 
command that is being used.


David




Message du 01/07/11 23:24
De : "David Holmes" 
A : goues...@orange.fr

Copie à : "Anthony Petrov" , awt-dev@openjdk.java.net
Objet : Re:  KDE Task bar is always on top of fullscreen Java applications

goues...@orange.fr said the following on 07/02/11 06:11:

libstdc++ devel package is installed but libstdc.so is not found. How can I 
solve this problem?

Install the missing dev package for your distro.

David


Message du 01/07/11 14:34
De : "David Holmes" 
A : goues...@orange.fr

Copie à : "Anthony Petrov" , awt-dev@openjdk.java.net
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

goues...@orange.fr said the following on 07/01/11 08:27:

I get this at the end of the compilation:
"/usr/bin/ld: cannot open output file libjvm.so: Too many levels of symbolic 
links"

I tried to recompile after cleaning all. What can I do to work around this 
problem?
Are you building hotspot? This is a quirk in the hotspot makefiles. If 
the link fails to create the libjvm you get a symbolic link that refers 
to itself. Subsequent build attempts don't try to rebuild libjvm as it 
seems to exist but when ld tries to access it you get the "too many 
levels of symbolic links" error.


A full clean should fix it, but then you need to see what the original 
error was caused by.


David Holmes
-


Message du 29/06/11 21:29
De : "Anthony Petrov" 
A : goues...@orange.fr

Copie à : awt-dev@openjdk.java.net
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

On 6/29/2011 5:59 PM, goues...@orange.fr wrote:

How can I detect whether a window is mapped? When XQueryTree returns zero, does 
it mean that the window is unmapped?
You could use XGetWindowAttributes() and examine the map_state field of 
the XWindowAttributes() structure.


Alternatively, you could pass an additional argument to the 
X11GD_SetFullscreenMode() because at AWT level we always know whether a 
window is mapped (see XBaseWindow.isMapped()).



PS. Your email client seems to replace the correct mailing list address 
"awt-dev@openjdk.java.net" with something strange: 
"awt-...@rea.oracle.com" when pressing Reply All. Could you please 
configure it properly?


--
best regards,
Anthony




I can create another patch, I will do this as soon as possible. I will ask to 
the KDE team that it is up to them not to promote only windows on top of the 
stack.


Message du 29/06/11 15:17
De : "Anthony Petrov" 
A : goues...@orange.fr

Copie à : awt-...@rea.oracle.com
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

Hi Julien,

So in your sample application you first set the window to the full 
screen mode, and only then you setVisible(true) it. In this case the 
EWMH spec states the following:


***
The Window Manager SHOULD honor _NET_WM_STATE whenever a withdrawn 
window requests to be mapped.

***

In other words, the X11GD_SetFullscreenMode() should actually check 
whether the window is currently mapped, and if so, do exactly what it 
currently does. However, if the window is currently unmapped, then we 
indeed have to use the XChangeProperty() call instead of the 
XSendEvent() one.


Please note that in either case we should not set the 
_NET_WM_STATE_ABOVE state. The _NET_WM_STATE_FULLSCREEN alone should 
work just fine. If it doesn't, then this is a problem with KDE.


Could you please try to create such a patch and test it on KDE4?

--
best regards,
Anthony

On 6/29/2011 3:49 PM, goues...@orange.fr wrote:

One guy of the KDE team answered that we have misunderstood the EWMH 
specification, that some window managers derivate from it and that using 
XChangeProperty there has some sense. What should I do now?



Message du 28/06/11 22:13
De : "Anthony Petrov" 
A : "Phil Race" 

, goues...@orange.fr

Copie à : awt-dev@openjdk.java.net
Objet : Re: KDE Task bar is always on top of fullscreen Java applications

Phil: Ah, right! Haven't used to the new rules yet. Thanks for reminding 
us about that.


Julien: I have a few questions about your patch:

1. The xprop output that you've attached to the KDE bug report [1] 
indicates that the full screen window is maximized (i.e. using the 
emulated full screen mode rather than the e

Re: "libjawt.so" not loaded by Java 7

[David Holmes]

Kapta Ulo said the following on 08/02/11 19:45:

The Java 7 release seems to have broken the LWJGL project on Linux, it no longer works 
and complains that "libjawt.so" is not loaded. The project has been calling the 
following method: java.awt.Toolkit.getDefaultToolkit(); to ensure that this native file 
is loaded, this has worked fine for the last 6 or so years. There seems to have been some 
internal change which causes this to break. Any idea's on what it could be and how to fix 
this?
There is some more information on the LWJGL forum here 
http://lwjgl.org/forum/index.php/topic,4085.0.html
Some people have found a workaround using "sudo ldconfig 
/opt/java/jre/lib/amd64/" which they claim allows it to work as before, but doesn't 
really explain why its failing, any idea's or pointers would be appreciated.


I've cc'ed the AWT dev folk as they should know what has changed in 7.

David


Thanks
Kapta 		 	   		  

Re: numAppContexts in AppContext modified in not-thread-safe way.

[David Holmes]

On 19/08/2011 8:02 AM, Clemens Eisserer wrote:

Mario wrote:

It looks good to me, and I don't expect any issues after this patch.
The only thing is that you should check the settings for the editor, I
think you still use a mix of tabs and spaces :)


Hmm, I should really check those eclipse settings ^^


Also,

public final static AppContext getAppContext()

Doesn't seem to be thread safe by nature, I don't know if this is part
of the issue you found, so maybe you should consider to synchronize
this as well.


Could be I got this wrong ... but isn't the idea behind getAppContext() that
it can be called  from any thread, and get the AppContext(if there is any) back?


Right. The appContext table is a synchronized map keyed on the current 
ThreadGroup. The first thread created in a ThreadGroup for a given 
AppContext will create the AppContext before any other threads can be 
created, hence this is thread-safe.


The change to use AtomicInteger seems okay - the main concern would be class 
initialization problems, but presumably that doesn't happen. Part of the 
missing picture here is how AppContexts get created and dispose()'d as it 
may be that the way AppContext is used you can't actually get concurrent 
modification of numAppContexts. But there's no way to discern that from the 
AppContext code so it would be safer to use the AtomicInteger.


Cheers,
David Holmes



- Clemens

Re: numAppContexts in AppContext modified in not-thread-safe way.

[David Holmes]

Hi Clemens,

On 19/08/2011 10:34 AM, Clemens Eisserer wrote:

Hi David,

  Part of the missing picture here is how AppContexts get created and
dispose()'d as it may be that the way AppContext is used you can't
actually get concurrent modification of numAppContexts. But there's no
way to discern that from the AppContext code so it would be safer to use
the AtomicInteger.


I had a look how AppContexts are created, and at least the IcedTea plugin
calls SunToolkit.createNewAppContext() in a fresh thread without any
synchronization (as does caciocavallo-web).
I don't this with all the code the non-atomicity of operations on
numAppContexts is a real-world problem, but on the other hand - it wouldn't
hurt to have this replaced with correct code ;)


Agreed.

Though it is also interesting to note that the only use of numAppContexts is 
for the shortcut lookup of the main app context when numAppContexts==1. It's 
not clear to me why this case needs to be handled specially. I also can't 
tell what code would be calling getAppContext under those conditions.


Anyway, use of AtomicInteger is correct and should be "harmless".

Cheers,
David

Re: numAppContexts in AppContext modified in not-thread-safe way.

[David Holmes]

On 19/08/2011 6:40 PM, Clemens Eisserer wrote:

Found a mistake, corrected version at:
http://cr.openjdk.java.net/~ceisserer/7080700/webrev.02/
It ended up with numAppContexts=2 when class-initialization was done,
although there was only one context.


The original code is rather strange. The static field will be default 
initialized to zero, the AppContext constructor will increment it to 1, then 
the static block explicitly sets it to one.


It would be cleaner/clearer if the field were declared and initialized 
before the static block.



It's not clear to me why this case needs to be handled specially. I also
can't tell what code would be calling getAppContext under those conditions.

 From what I understand its a fast-path. For normal Swing Applications, no
AppContexts are ever created (except the "system" context when
AppContext.class is loaded -> numAppContexts=1), so the code avoids the
look-up. I may have misunderstood it of course ;)


That's an uninteresting case from a data-race perspective though because the 
counter is only ever written once.


However I now wonder about how often this gets called in a Swing app? If 
this is a frequent call then the overhead of the atomic might be 
significant. Perhaps the Swing folk should be evaluating this change as well?


Cheers,
David


Thanks, Clemens

Re: numAppContexts in AppContext modified in not-thread-safe way.

[David Holmes]

On 20/08/2011 8:12 AM, Clemens Eisserer wrote:

The original code is rather strange. The static field will be default
initialized to zero, the AppContext constructor will increment it to 1,
then the static block explicitly sets it to one.
It would be cleaner/clearer if the field were declared and initialized
before the static block.

Agreed. Please find the modified patch at:
http://cr.openjdk.java.net/~ceisserer/7080700/webrev.04/


Looks good. Just need someone from AWT to chime in.



That's an uninteresting case from a data-race perspective though because
the counter is only ever written once.

In the constructor the non-atomic increment can cause a missed update and
therefor cause numAppContexts to become 1 or even 0, which could trigger
that logic unintentionally.


My point was that in a Swing app the constructor is only ever called once 
and so there can not be a missed update. Anyway I was just musing :)




However I now wonder about how often this gets called in a Swing app? If
this is a frequent call then the overhead of the atomic might be
significant. Perhaps the Swing folk should be evaluating this change as
well?

Did some benchmarks,  and at least on x86 AtomicInteger.get() performs
identical to a volatile field read.


And of course I knew that because the intrinsics that underlie the atomics 
are the same as those for volatiles. :) Thanks for verifying.


Cheers,
David



Thanks, Clemens

Re: Endless loop in EventDispatchThread - proposed solution

[David Holmes]

Hi Clemens,

On 21/08/2011 9:25 PM, Clemens Eisserer wrote:

On our caciocavallo-web demo server I sometimes experience endless spinning
in an EventDispatchThread (in run()) which should have been shut down long
ago by AppContext.dispose() - causing our server to consume 100% cpu.

I tried to anylze the situation using a remote debugger, this is what I've
found:

The thread is interrupted, therefor no events are dispatched anymore by
pumpEventsForFilter:

   while (doDispatch && cond.evaluate()) {
 if (isInterrupted() || !pumpOneEventForFilters(id)) {
 doDispatch = false;
 }
 }


however, detaching the thread from the EventQueue fails, because there are
still events left for dispatching,
which in turn won't be dispatched, because the thread is interrupted.

1.) A fix would be to check for isInterruped also in the thread's run loop:

 public void run() {
 while (true) {
 try {
 pumpEvents(new Conditional() {
 public boolean evaluate() {
 return true;
 }
 });
 } finally {
 EventQueue eq = getEventQueue();
 if (eq.detachDispatchThread(this) || threadDeathCaught
|| isInterrupted()) {
 break;
 }
 }
 }
 }



What do you think about the proposed solutions?


It seems to me that when the app context is being shutdown in this manner 
that part of that should be to purge/clear the event queue. To me that would 
be a more appropriate fix for this non-terminating situation.




2.) A big mystery for me was, why the thread hasn't been killed by
Thread.stop(), called by AppContext.dispose() - and why the
"threadDeathCaught"-variable was still false (as this would have caused the
thread to exit anyway).
The reason was, ThreadDeath is swollowed by the IllegalMonitorStateException
caused by Thread.stop():

Basically this is what is going on, when waiting for new events:

  pushPopLock.lock();
 try {
   cond.await(); //Throws ThreadDeath
 }finally {
pushPopLock.unlock(); //Throws IllegalMonitorStateException,
"swollows" ThreadDeath
 }

So ThreadDeath is never catched.

This bug has been introduced when converting to java.util.concurrent, as
java monitors don't throw an exception in this situation.


Right. The ThreadDeath gets seen while trying to acquire the Lock to return 
from await() and so the lock is not reacquired and so the subsequent 
unlock() throws IMSE. There's an open bug on this (6627356), against 
Condition.await, but like anything to do with async exceptions there are no 
easy solutions. I'd prefer if AppContext could stop using a method that's 
been deprecated for a decade (and which I'd love to physically remove in 
Java 8).


BTW the interrupt() should really be causing the await() to complete, but 
there must be contention on the Lock causing the stop() to be issued before 
the lock is acquired. Maybe that contention could be removed to allow the 
interrupt() to do all the work?



My proposal for this bug would be to do away with the
threadDeathCaught-variable completly, as the isInterrupted()-check proposed
in 1.) will stop the thread anyway.


Seems to me that there should be a simple variable that the run() method can 
check which gets set when the thread is expected to terminate. That way it 
doesn't matter whether it gets hit by the interrupt() or the stop(), it will 
terminate based on the flag.


But any changes to this code need deep scrutiny - I know many eyes have 
looked at it over the years. And there's no doubt a fair amount of 
historical baggage as well.


David


Thanks, Clemens

Re: Endless loop in EventDispatchThread - proposed solution

[David Holmes]

On 22/08/2011 8:17 PM, Clemens Eisserer wrote:

Thanks again for taking a look at my comments and for your patience :)


Threads are my core interest :)



It seems to me that when the app context is being shutdown in this
manner that part of that should be to purge/clear the event queue. To me
that would be a more appropriate fix for this non-terminating situation.

Could be, I have to admit I am not an EventQueue expert :/


Right. The ThreadDeath gets seen while trying to acquire the Lock to
return from await() and so the lock is not reacquired and so the
subsequent unlock() throws IMSE. There's an open bug on this (6627356),
against Condition.await, but like anything to do with async exceptions
there are no easy solutions. I'd prefer if AppContext could stop using a
method that's been deprecated for a decade (and which I'd love to
physically remove in Java 8).


BTW the interrupt() should really be causing the await() to complete,
but there must be contention on the Lock causing the stop() to be issued
before the lock is acquired. Maybe that contention could be removed to
allow the interrupt() to do all the work?


There is at least one case when Thread.stop is required to halt the EDT:
If there are events added to the queue, after await() threw the
InterruptedException. In this case the EDT won't terminate and tries to
dispatch the remaining events, which it does until the queue is empty, and
then again it waits in await().
(the isInterrupted()-checks are a bit meaningless, because after an
InterruptedException is thrown in getNextEvent, isInterrupted() returns
false again)
There is no second Thread.interrupt(), so ThreadDeath is thrown when
Thread.stop() is called. (which currently leads to spinning, as
inInterrupted() returns true when Thread.stop has been called and
threadDeathCaught remains false forever)


Ah I see. So this would again be solved by purging the event queue. I'm no 
expert on the event system per se so it may be that events can continue to 
be generated after the decision to dispose of the app context has been made 
- otherwise I would expect no further events and so clearing the queue would 
allow the interrupt (plus variable setting) to act as the termination 
indicator for the EDT.


David


I experienced this issue only on the highly loaded (and swapping, but not
OOM) server, that would explain the timing issues.



My proposal for this bug would be to do away with the
threadDeathCaught-variable completly, as the isInterrupted()-check proposed
in 1.) will stop the thread anyway.


Seems to me that there should be a simple variable that the run() method
can check which gets set when the thread is expected to terminate. That
way it doesn't matter whether it gets hit by the interrupt() or the
stop(), it will terminate based on the flag.


Jep, I agree. Additionally, when pending events are dispatched during
shut-down, the queue should not block waiting for new events.
That async exceptions could even be consumed by bad-behaving user-code (e.g.
some Thread.sleep wrapped in a catch-block), a variable seems to be a much
cleaner solution.


But any changes to this code need deep scrutiny - I know many eyes have
looked at it over the years. And there's no doubt a fair amount of
historical baggage as well.

I'll prepare two patches:
- A simple one, which just restores pre-java7 behaviour, working arround
that IllegalMonitorStateException
- A more in-deptch change, trying not to rely on AsynExeptions for thread
termination.


Thanks again, Clemens

Re: Endless loop in EventDispatchThread - proposed solution

[David Holmes]

On 25/08/2011 2:20 AM, Clemens Eisserer wrote:

Hi again,

Please find the latest version of the "full" patch at:
http://cr.openjdk.java.net/~ceisserer/7081670/webrev_full.02/
Because the return value of pumpOneEventForFilters(int id) redundant as it
always equals shutdown, I now just set shutdown and return nothing.


You changed from Lock to ReentrantLock so that you could use 
isHeldByCurrentThread(), but that locks in (pardon the pun) the kind of Lock 
that AppContext must use.


 504 pushPopLock.lock();
 505 try {
 506 AWTEvent event = getNextEventPrivate();
 507 if (event != null) {
 508 return event;
 509 }
 510 
AWTAutoShutdown.getInstance().notifyThreadFree(dispatchThread);

 511 pushPopCond.await();
 512 } finally {
 513 if(pushPopLock.isHeldByCurrentThread()) {
 514pushPopLock.unlock();
 515 }
 516 }

You should add a comment explaining why the check for isHeldByCurrentThread 
is needed - and that if things are done right at a higher-level we should 
never need the stop() to break out of await().


Ditto for line 570

Overall I'm still concerned that there is an issue in the overall design 
that permits events to be queued even after a "shutdown" has been logically 
initiated. With this patch those events won't get processed and not knowing 
what they are I can't say whether this will be a problem or not. It is a 
concern that the current code in detachDispatchThread says:


1045/*
1046 * Don't detach the thread if any events are pending. Not
1047 * sure if it's a possible scenario, though.
1048 *
1049 * Fix for 4648733. Check both the associated java event
1050 * queue and the PostEventQueue.
1051 */

as it seems to indicate that the exact conditions for detachment are 
unclear. Based on reading 4648733 I'm assuming that we have to keep the 
event queue receiving events so that the  shutdown event can be posted (as 
part of AWT auto-shutdown), and that then allows other events in. The 
question remains as to whether those events should be processed even when 
shutdown has been initiated.


David
-



Thanks, Clemens

Re: Endless loop in EventDispatchThread - proposed solution

[David Holmes]

On 26/08/2011 2:39 AM, Clemens Eisserer wrote:

Hi David,

Thanks for your feedback.


You changed from Lock to ReentrantLock so that you could use
isHeldByCurrentThread(), but that locks in (pardon the pun) the kind of
Lock that AppContext must use.
You should add a comment explaining why the check for
isHeldByCurrentThread is needed - and that if things are done right at a
higher-level we should never need the stop() to break out of await().


I will add a comment about that, I also took the lock-in to ReentrantLock as
a trade-of as I couldn't think of another lock-implementation that would be
more suitable for the job of the pushpopLock.
I think things are done the right-way at a higher level (as with the "full"
patch, the code should *never* catch ThreadDeath execpt some user-code in an
InvocationEvent does strange things - which we can't control).
Also the "normal" way of termination doesn't depend on catching ThreadDeath
or InterruptedException, as shutdown is set synchronously to true by the
Thread calling AppContext.dispose().

What bothers me is that code known to be good style and is recommended
everywhere, simply hides an InterruptedException and causes an
IllegalMontorState-Exception to be thrown - actually the
isHeldByCurrentThread is just there to avoid ugly IllegalMonitorException


Yes I understand why you added the check. I'm not sure what you mean about 
hiding the InterruptedException though. As I understood the scenario the 
interrupt() breaks the thread out of await() by throwing IE, which is then 
caught at a higher-level (not sure exactly where), but because of further 
pending events the IE does not cause the EDT to detach/terminate but instead 
it continues to process events eventually calling await() again. Then the 
stop() gets issued which causes the await() to abort by throwing 
ThreadDeath, without reclaiming the Lock, and so the unlock in the finally 
clause throws IllegalMonitorStateException.



Stack-Traces printed out - as its done when using ThreadPools with Applets.
Java's monitor design took care of that. As far as I can see, a tryUnlock()
method would solve those problems.


But it would be the wrong solution. You should (almost) never be unlocking 
a Lock that you didn't lock first. The problem here is asynchronous 
exceptions - pure and simple. But I think it is time to revisit this issue 
in AbstractQueuedSynchronizer.



Overall I'm still concerned that there is an issue in the overall design
that permits events to be queued even after a "shutdown" has been
logically initiated. With this patch those events won't get processed
and not knowing what they are I can't say whether this will be a problem
or not. It is a concern that the current code in detachDispatchThread says:
as it seems to indicate that the exact conditions for detachment are
unclear. Based on reading 4648733 I'm assuming that we have to keep the
event queue receiving events so that the  shutdown event can be posted
(as part of AWT auto-shutdown), and that then allows other events in.
The question remains as to whether those events should be processed even
when shutdown has been initiated.


I am no AWT expert, but from how I interpret the old code, as soon as
interrupt() has been called, it was not intended to dispatch further events
(I don't think the isInterrupted() call was really ment that way).
However, I wonder why AppContext.stopEventDispatchThreads() is never used in
AppContext.dispose(), as it seems to provide a cleaner way for shutdown?


I'm no AWT expert either, it just concerns me when there is an introduced 
change in behaviour without a full understanding of the implications. We 
really need some input from an AWT event processing guru.


Cheers,
David


Thanks, Clemens

Re: Endless loop in EventDispatchThread - proposed solution

[David Holmes]

On 26/08/2011 8:58 AM, Clemens Eisserer wrote:

Hi,

Yes I understand why you added the check. I'm not sure what you mean
about hiding the InterruptedException though. As I understood the
scenario the interrupt() breaks the thread out of await() by throwing
IE, which is then caught at a higher-level (not sure exactly where), but
because of further pending events the IE does not cause the EDT to
detach/terminate but instead it continues to process events eventually
calling await() again. Then the stop() gets issued which causes the
await() to abort by throwing ThreadDeath, without reclaiming the Lock,
and so the unlock in the finally clause throws IllegalMonitorStateException.


Sorry, misunderstanding on my side. I thought when interrupting the thread,
re-claiming the lock would also fail, but it doesn't. Your analysis is
totally correct. So the "full" patch could probably do without the checks,
as it should not run into this condition (even when a ThreadDeath is covered
by IllegalMonitorException, shutodown=true -> no more calls of await()).
However, it doesn't hurt a lot on the other side. What do you think?


As long as it is clearly documented as to why the check is there

David


I'm no AWT expert either, it just concerns me when there is an
introduced change in behaviour without a full understanding of the
implications. We really need some input from an AWT event processing guru.

Absolutly.

Thanks again, Clemens

Re: Request for review - change two include header files according to POSIX.1-2008

[David Holmes]

On 12/10/2011 5:23 PM, Charles Lee wrote:

On 10/12/2011 03:12 PM, David Holmes wrote:

On 12/10/2011 4:26 PM, Charles Lee wrote:

sys/unistd.h, sys/fcntl.h are not supported in AIX. And according to the
POSIX.1-2008 (http://pubs.opengroup.org/onlinepubs/9699919799/), I have
changed them to unistd.h and fcntl.h. The patch has been tested on both
linux and aix.

I also change the header file in solaris, though I do not have a solaris
machine on the hand. Hope it will not break the build :-)


unistd.h and fcntl.h are already used extensively on Solaris


The patch and webrev has been attached.


Looks like they were stripped of the email. (And of the second attempt.)

By grepping I assume the splashscreen_config.h file is one place this
was fixed for unistd.h. It is interesting to note that in some cases
at least (eg splashscreen_sys.c) the C file includes  anyway.


Yes. Only splashscreen_config.h has 


Ok. I've cc'ed awt-dev as this is awt code.



We'll need to verify the changes for  in the
genUnixConstants code. There tends to be a reason (often historical
and possibly no longer applicable) for using the sys variants (though
sometimes it was just that the non-sys version didn't exist at some
point in time).



By grep, only genUnixConstants.c and genSolarisConstants.c use
 in the jdk repository. And more than 20 files use 


Yep. And the fcntl manpage on Solaris says to #include  and 
 - so I think we're okay there. I suspect Solaris 8 may be a 
different story but that's only an issue for JDK6.


David


Cheers,
David Holmes


Trying to attach a plain diff file again and again


Patch inlined for AWT-dev folk to see.

diff --git src/solaris/native/sun/awt/splashscreen/splashscreen_config.h 
src/solaris/native/sun/awt/splashscreen/splashscreen_config.h

index bb03165..e312c2b 100644
--- src/solaris/native/sun/awt/splashscreen/splashscreen_config.h
+++ src/solaris/native/sun/awt/splashscreen/splashscreen_config.h
@@ -32,7 +32,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git src/solaris/native/sun/nio/fs/genSolarisConstants.c 
src/solaris/native/sun/nio/fs/genSolarisConstants.c

index df46398..346bfbb 100644
--- src/solaris/native/sun/nio/fs/genSolarisConstants.c
+++ src/solaris/native/sun/nio/fs/genSolarisConstants.c
@@ -27,7 +27,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 

 /**
diff --git src/solaris/native/sun/nio/fs/genUnixConstants.c 
src/solaris/native/sun/nio/fs/genUnixConstants.c

index ea48d4d..56984a7 100644
--- src/solaris/native/sun/nio/fs/genUnixConstants.c
+++ src/solaris/native/sun/nio/fs/genUnixConstants.c
@@ -26,7 +26,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 

 /**

Re: Code Review 7110002: Rename xawt/libmawt.so and headless/libmawt.so so they can be colocated with libawt

[David Holmes]

Chris,

I just discovered that there is an additional change needed for 
JAVASE_EMBEDDED. On the JDK side we also have a check to try and 
determine if this is a headless or headful JRE, because a true 
headless-JRE requires the special HToolkit. That code checks for the 
existence of lib//xawt to make that decision and so presently all 
JRE's appear to headless JREs :(


Here's a fix:

diff -r 830d2e46023a src/solaris/native/java/lang/java_props_md.c
--- a/src/solaris/native/java/lang/java_props_md.c
+++ b/src/solaris/native/java/lang/java_props_md.c
@@ -325,8 +325,8 @@
 realpath((char *)dlinfo.dli_fname, buf);
 len = strlen(buf);
 p = strrchr(buf, '/');
-/* Default AWT Toolkit on Linux and Solaris is XAWT. */
-strncpy(p, "/xawt/", MAXPATHLEN-len-1);
+/* Default AWT Toolkit on Linux and Solaris is libawt_xawt. */
+strncpy(p, "/libawt_xawt.so", MAXPATHLEN-len-1);
 /* Check if it exists */
 if (stat(buf, &statbuf) == -1 && errno == ENOENT) {
 /* No - this is a reduced-headless-jre so use special HToolkit */

Sorry I didn't think of this earlier.

Thanks,
David

On 9/11/2011 10:28 PM, Chris Hegarty wrote:

Hi,

CR 7110002 proposes to rename the unix version of the awt toolkit
libraries to allow them reside in the same directory as libawt.
xawt/libmawt.so -> libawt_xawt.so
headless/libmawt.so -> libawt_headless.so

The proposed new library names are prefixed with 'libawt' since they are
subcomponents of libawt and this will make them more easily recognizably
as such. But this is not necessarily a requirement, just that they are
unique and can be colocated with libawt.

This CR is part of the modularity effort. A future requirement of the
jigsaw prototype is to treat native JDK implementation libraries as it
would any user module installed in a module library. That is, native
libraries should reside within the lib directory of the installed
module. In the module library then AWT's libraries might be located in
somewhere like modules/sun.desktop/8.0/lib.

See discussion on the awt-dev mailing list for further context:
http://mail.openjdk.java.net/pipermail/awt-dev/2011-April/001666.html

Webrev:
http://cr.openjdk.java.net/~chegar/7110002/webrev.00/

Thanks,
-Chris

Re: Code Review 7110002: Rename xawt/libmawt.so and headless/libmawt.so so they can be colocated with libawt

[David Holmes]

PS. I was also going to query about the continued existence of other 
Motif related code - even libmawt is still referenced in the comments of 
a couple of Motif specific functions. Is that now "dead" code?


David

On 11/11/2011 3:00 PM, David Holmes wrote:

Chris,

I just discovered that there is an additional change needed for
JAVASE_EMBEDDED. On the JDK side we also have a check to try and
determine if this is a headless or headful JRE, because a true
headless-JRE requires the special HToolkit. That code checks for the
existence of lib//xawt to make that decision and so presently all
JRE's appear to headless JREs :(

Here's a fix:

diff -r 830d2e46023a src/solaris/native/java/lang/java_props_md.c
--- a/src/solaris/native/java/lang/java_props_md.c
+++ b/src/solaris/native/java/lang/java_props_md.c
@@ -325,8 +325,8 @@
realpath((char *)dlinfo.dli_fname, buf);
len = strlen(buf);
p = strrchr(buf, '/');
- /* Default AWT Toolkit on Linux and Solaris is XAWT. */
- strncpy(p, "/xawt/", MAXPATHLEN-len-1);
+ /* Default AWT Toolkit on Linux and Solaris is libawt_xawt. */
+ strncpy(p, "/libawt_xawt.so", MAXPATHLEN-len-1);
/* Check if it exists */
if (stat(buf, &statbuf) == -1 && errno == ENOENT) {
/* No - this is a reduced-headless-jre so use special HToolkit */

Sorry I didn't think of this earlier.

Thanks,
David

On 9/11/2011 10:28 PM, Chris Hegarty wrote:

Hi,

CR 7110002 proposes to rename the unix version of the awt toolkit
libraries to allow them reside in the same directory as libawt.
xawt/libmawt.so -> libawt_xawt.so
headless/libmawt.so -> libawt_headless.so

The proposed new library names are prefixed with 'libawt' since they are
subcomponents of libawt and this will make them more easily recognizably
as such. But this is not necessarily a requirement, just that they are
unique and can be colocated with libawt.

This CR is part of the modularity effort. A future requirement of the
jigsaw prototype is to treat native JDK implementation libraries as it
would any user module installed in a module library. That is, native
libraries should reside within the lib directory of the installed
module. In the module library then AWT's libraries might be located in
somewhere like modules/sun.desktop/8.0/lib.

See discussion on the awt-dev mailing list for further context:
http://mail.openjdk.java.net/pipermail/awt-dev/2011-April/001666.html

Webrev:
http://cr.openjdk.java.net/~chegar/7110002/webrev.00/

Thanks,
-Chris

Re: hang when doing a fastdebug build

[David Holmes]

The AWT eventqueue thread has called System.exit and is waiting for a 
shutdown hook thread to complete. I'm guessing that thread is Thread-0, 
which is currently out in native code executing WToolkit.shutdown. So 
from the Java stacks there is no way to tell what that thread is doing 
(ie whether it is making progress).


David

On 21/02/2012 4:22 AM, Pete Brunet wrote:

There is something about a patch I am working on that results in a hang
when exiting an app with a fastdebug build but not with a product
build.  The results of ctrl+break follow.  Could someone look at this
and give me a hint as to what the deadlock is?  Thanks, Pete

Full thread dump OpenJDK Client VM (23.0-b11-fastdebug mixed mode):

"Thread-0" daemon prio=6 tid=0x0863ac00 nid=0x10c8 runnable [0x07d5f000]
java.lang.Thread.State: RUNNABLE
JavaThread state: _thread_in_native
Thread: 0x0863ac00  [0x10c8] State: _at_safepoint _has_called_back 0
_at_poll_safepoint 0
JavaThread state: _thread_in_native
 at sun.awt.windows.WToolkit.shutdown(Native Method)
 at sun.awt.windows.WToolkit.access$200(WToolkit.java:67)
 at sun.awt.windows.WToolkit$2$1.run(WToolkit.java:275)
 at java.lang.Thread.run(Thread.java:722)

"SwingWorker-pool-1-thread-1" daemon prio=6 tid=0x088c3800 nid=0x166c
waiting on condition [0x09d2f000]
java.lang.Thread.State: WAITING (parking)
JavaThread state: _thread_blocked
Thread: 0x088c3800  [0x166c] State: _at_safepoint _has_called_back 0
_at_poll_safepoint 0
JavaThread state: _thread_blocked
 at sun.misc.Unsafe.park(Native Method)
 - parking to wait for<0x15ffc1e0>  (a
java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
 at java.util.concurrent.locks.LockSupport.park(LockSupport.java:186)
 at
java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2043)
 at
java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:442)
 at
java.util.concurrent.ThreadPoolExecutor.getTask(ThreadPoolExecutor.java:1045)
 at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1103)
 at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
 at java.lang.Thread.run(Thread.java:722)

"DestroyJavaVM" prio=6 tid=0x00ec9000 nid=0xfa0 waiting on condition
[0x]
java.lang.Thread.State: RUNNABLE
JavaThread state: _thread_blocked
Thread: 0x00ec9000  [0xfa0] State: _at_safepoint _has_called_back 0
_at_poll_safepoint 0
JavaThread state: _thread_blocked

"AWT-EventQueue-0" prio=6 tid=0x0581f800 nid=0x125c in Object.wait()
[0x084de000]
java.lang.Thread.State: WAITING (on object monitor)
JavaThread state: _thread_blocked
Thread: 0x0581f800  [0x125c] State: _at_safepoint _has_called_back 0
_at_poll_safepoint 0
JavaThread state: _thread_blocked
 at java.lang.Object.wait(Native Method)
 - waiting on<0x155c77e0>  (a java.lang.Thread)
 at java.lang.Thread.join(Thread.java:1258)
 - locked<0x155c77e0>  (a java.lang.Thread)
 at java.lang.Thread.join(Thread.java:1332)
 at
java.lang.ApplicationShutdownHooks.runHooks(ApplicationShutdownHooks.java:106)
 at
java.lang.ApplicationShutdownHooks$1.run(ApplicationShutdownHooks.java:46)
 at java.lang.Shutdown.runHooks(Shutdown.java:123)
 at java.lang.Shutdown.sequence(Shutdown.java:167)
 at java.lang.Shutdown.exit(Shutdown.java:212)
 - locked<0x156be6b8>  (a java.lang.Class for java.lang.Shutdown)
 at java.lang.Runtime.exit(Runtime.java:107)
 at java.lang.System.exit(System.java:961)
 at javax.swing.JFrame.processWindowEvent(JFrame.java:312)
 at java.awt.Window.processEvent(Window.java:2003)
 at java.awt.Component.dispatchEventImpl(Component.java:4866)
 at java.awt.Container.dispatchEventImpl(Container.java:2287)
 at java.awt.Window.dispatchEventImpl(Window.java:2713)
 at java.awt.Component.dispatchEvent(Component.java:4691)
 at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:703)
 at java.awt.EventQueue.access$000(EventQueue.java:102)
 at java.awt.EventQueue$3.run(EventQueue.java:662)
 at java.awt.EventQueue$3.run(EventQueue.java:660)
 at java.security.AccessController.doPrivileged(Native Method)
 at
java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:75)
 at
java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:86)
 at java.awt.EventQueue$4.run(EventQueue.java:676)
 at java.awt.EventQueue$4.run(EventQueue.java:674)
 at java.security.AccessController.doPrivileged(Native Method)
 at
java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:75)
 at java.awt.EventQueue.dispatchEvent(EventQueue.java:673)
 at
java.awt.Eve

RFR: 7171653 32-bit cross-compile on 64-bit build host generates 64-bit data for awt/X11 leading to crash

[David Holmes]

This is a tweak to the cross-compilation rules in sun/xawt/Makefile to 
force correct compilation of the sizers utility.


http://cr.openjdk.java.net/~dholmes/7171653/webrev/

webrev is against tl/jdk but I can use a different repo if you prefer.

Now that we use CFLAGS_32/64 in the cross compilation case we no longer 
need different cc commands, other than the actual choice of compiler.


Longer term 7153072 will make this fix obsolete but for now we need this 
for our new build servers, and I wanted to make sure this only affected 
cross-compilation.


Same change will need to go to the 7u train as well.

Thanks,
David

Re: RFR: 7171653 32-bit cross-compile on 64-bit build host generates 64-bit data for awt/X11 leading to crash

[David Holmes]

I have a review from AWT (thanks Anthony), can I have one from build please.

Any preference as to where to push this?

Thanks,
David

On 25/05/2012 4:20 PM, David Holmes wrote:

This is a tweak to the cross-compilation rules in sun/xawt/Makefile to
force correct compilation of the sizers utility.

http://cr.openjdk.java.net/~dholmes/7171653/webrev/

webrev is against tl/jdk but I can use a different repo if you prefer.

Now that we use CFLAGS_32/64 in the cross compilation case we no longer
need different cc commands, other than the actual choice of compiler.

Longer term 7153072 will make this fix obsolete but for now we need this
for our new build servers, and I wanted to make sure this only affected
cross-compilation.

Same change will need to go to the 7u train as well.

Thanks,
David

Re: [7u6] Review request for 7181027: [macosx] Unable to use headless mode

[David Holmes]

On 11/07/2012 11:46 PM, Leonid Romanov wrote:

Hi,
Please review a fix for 7181027: [macosx] Unable to use headless mode. The problem here 
is that for headless mode  "java.awt.graphicsenv" system property should be 
CGraphicsEnvironment because the way GraphicsEnvironment.createGE() method works: it 
first instantiates GraphicsEnvironment instance and then wraps it with 
HeadlessGraphicsEnvironment if in headless mode. This means twe can't use 
java.awt.graphicsenv property to determine whether we are in headless mode or not. So, 
I've replaced it with a toolkit check: if it's HToolkit, then we are in headless.


sun.awt.HToolkit was introduced for use by SE-Embedded's reduced 
headless JRE. How did the OSX port end up using it ???


Headless handling on OSX should be like regular headless on Linux, Solaris.

David


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181027
Webrev: http://cr.openjdk.java.net/~leonidr/7181027/webrev.00/

Thanks,
Leonid.

Re: [7u6] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[David Holmes]

Hi Oleg,

"deadlock" always grabs my attention :)

On 12/07/2012 2:08 AM, Oleg Pekhovskiy wrote:

Thank you for the review, Anthony,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/7u6/7177040.2

I followed your suggestions there.
I also added 'volatile' for isFlushing because it could be accessed from
different threads.


I was about to mention that :)

I don't have the complete picture here but the actions of flush() no 
longer "ensure the flush is completed before a new event can be posted 
to this queue". The assurance may still be there in the way things are 
called, but it not ensured by the flush() method.


It also seems that with this fix the interrupted EDT will not detach due 
to the flush being in progress. The EDT will resume the pumpEvents loop 
in its run() method. If the interrupt state of the EDT has not been 
maintained then the fact it was interrupted (and should detach?) is now 
lost. If it is maintained then presumably we will try to 
detachDispatchThread again, in which case the EDT will enter a busy 
polling loop trying, but failing, to detach, until the flush() has 
actually completed.


Cheers,
David



Thanks,
Oleg

7/11/2012 7:35 PM, Anthony Petrov wrote:

Hi Oleg,

1. I suggest to rename isPending to isFlushing, as it reflects the
current state of the PostEventQueue more precisely.

2. I suggest to use the try{}finally{} pattern to set/reset the new
flag in the flush() method to ensure the flag is reset even if the
while() loop throws an exception.

Otherwise the fix looks good.

--
best regards,
Anthony

On 7/11/2012 7:24 PM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7177040

Webrev:
http://cr.openjdk.java.net/~bagiras/7u6/7177040.1

The idea of the fix is that there are two concurrent threads that try
to get two synchronization objects EventQueue.pushPopLock and
PostEventQueue itself.
For NetBeans these threads are (EDT & WarmUp) or (EDT & TimerQueue).
Problem happens when EDT is interrupted and goes to
EventQueue.detachDispatchThread() where EventQueue.pushPopLock is
owned and
EDT is waiting to own PostEventQueue when calling
SunToolkit.isPostEventQueueEmpty() -> PostEventQueue.noEvents().
At the same time another thread calls EventQueue.postEvent() that
calls EventQueue.flushPendingEvents() -> PostEventQueue.flush() where
PostEventQueue is owned
and another thread is waiting to own EventQueue.pushPopLock when
calling EventQueue.postEvent() -> EventQueue.postEventPrivate().

To avoid potential deadlock I moved synchronization out of
postEvent()'s cycle in PostEventQueue.flush(),
but to be clear about the existence of Events that are not posted to
EventQueue yet I added PostEventQueue.isPending flag that is true
until the end of the cycle.

There are only two classes that utilize PostEventQueue.flush()
method: SunToolkit.flushPendingEvents() and
SunToolkitSubclass.flushPendingEvents().
They are both synchronized by static SunToolkit.flushLock. That
eliminates the situation when we have overlapped calls of
PostEventQueue.flush().

Thanks,
Oleg

Re: [7u6] Review request for 7181027: [macosx] Unable to use headless mode

[David Holmes]

On 12/07/2012 6:58 PM, Leonid Romanov wrote:

Perhaps Anthony might be able to answer your question: he has tinkered a lot 
with headless related stuff.
David, are you implying that in the current form my fix is no-go?


Not my place to do that. I'm just wondering how the HToolkit got 
involved with OSX, and why OSX headless is handled differently to 
Solaris/linux (which don't use HToolkit).


David


On 12.07.2012, at 5:15, David Holmes wrote:


On 11/07/2012 11:46 PM, Leonid Romanov wrote:

Hi,
Please review a fix for 7181027: [macosx] Unable to use headless mode. The problem here 
is that for headless mode  "java.awt.graphicsenv" system property should be 
CGraphicsEnvironment because the way GraphicsEnvironment.createGE() method works: it 
first instantiates GraphicsEnvironment instance and then wraps it with 
HeadlessGraphicsEnvironment if in headless mode. This means twe can't use 
java.awt.graphicsenv property to determine whether we are in headless mode or not. So, 
I've replaced it with a toolkit check: if it's HToolkit, then we are in headless.


sun.awt.HToolkit was introduced for use by SE-Embedded's reduced headless JRE. 
How did the OSX port end up using it ???

Headless handling on OSX should be like regular headless on Linux, Solaris.

David


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181027
Webrev: http://cr.openjdk.java.net/~leonidr/7181027/webrev.00/

Thanks,
Leonid.

Re: [7u6] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[David Holmes]

On 12/07/2012 10:18 PM, Anthony Petrov wrote:

On 07/12/12 05:41, David Holmes wrote:





It also seems that with this fix the interrupted EDT will not detach due
to the flush being in progress. The EDT will resume the pumpEvents loop
in its run() method. If the interrupt state of the EDT has not been
maintained then the fact it was interrupted (and should detach?) is now
lost. If it is maintained then presumably we will try to
detachDispatchThread again, in which case the EDT will enter a busy
polling loop trying, but failing, to detach, until the flush() has
actually completed.


When interrupt() is called on EDT, the shutdown flag is set, which is
subsequently passed as the forceDetach parameter to
detachDispatchingThread(). If the parameter is true, the detaching
happens unconditionally. So this shouldn't be an issue.


I don't quite follow this part. The logic is:

 if (!forceDetach && (peekEvent() != null) ||
  !SunToolkit.isPostEventQueueEmpty()) {
 return false;
 }

If forceDetach is true then we won't execute peekEvent() and 
isPostEventQueueEmpty(), which means we would not have hit the original 
deadlock.


Oleg's response seems to indicate that we will indeed keep looping but 
because of the values of various flags we won't do anything "useful". 
But that is the kind of "busy wait" that I was referring to.


Cheers,
David




--
best regards,
Anthony




Cheers,
David



Thanks,
Oleg

7/11/2012 7:35 PM, Anthony Petrov wrote:

Hi Oleg,

1. I suggest to rename isPending to isFlushing, as it reflects the
current state of the PostEventQueue more precisely.

2. I suggest to use the try{}finally{} pattern to set/reset the new
flag in the flush() method to ensure the flag is reset even if the
while() loop throws an exception.

Otherwise the fix looks good.

--
best regards,
Anthony

On 7/11/2012 7:24 PM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7177040

Webrev:
http://cr.openjdk.java.net/~bagiras/7u6/7177040.1

The idea of the fix is that there are two concurrent threads that try
to get two synchronization objects EventQueue.pushPopLock and
PostEventQueue itself.
For NetBeans these threads are (EDT & WarmUp) or (EDT & TimerQueue).
Problem happens when EDT is interrupted and goes to
EventQueue.detachDispatchThread() where EventQueue.pushPopLock is
owned and
EDT is waiting to own PostEventQueue when calling
SunToolkit.isPostEventQueueEmpty() -> PostEventQueue.noEvents().
At the same time another thread calls EventQueue.postEvent() that
calls EventQueue.flushPendingEvents() -> PostEventQueue.flush() where
PostEventQueue is owned
and another thread is waiting to own EventQueue.pushPopLock when
calling EventQueue.postEvent() -> EventQueue.postEventPrivate().

To avoid potential deadlock I moved synchronization out of
postEvent()'s cycle in PostEventQueue.flush(),
but to be clear about the existence of Events that are not posted to
EventQueue yet I added PostEventQueue.isPending flag that is true
until the end of the cycle.

There are only two classes that utilize PostEventQueue.flush()
method: SunToolkit.flushPendingEvents() and
SunToolkitSubclass.flushPendingEvents().
They are both synchronized by static SunToolkit.flushLock. That
eliminates the situation when we have overlapped calls of
PostEventQueue.flush().

Thanks,
Oleg

Re: [7u6] Review request for 7181027: [macosx] Unable to use headless mode

[David Holmes]

On 12/07/2012 10:33 PM, Anthony Petrov wrote:

The logic is all in src/solaris/native/java/lang/java_props_macosx.c.
The getPreferredToolkit() returns the HToolkit constant when the
headless mode is needed on the Mac. And the GetJavaProperties() will
then choose the sun.awt.HToolkit as the toolkit. Interesting.


Interesting indeed. But my concerns remain. HToolkit was not intended 
for general use. OSX seems to be handling headless mode in a completely 
different way to Solaris/linux.


Of course it may be that on OSX you run into the same conditions when 
headless that required the introduction of HToolkit. But somebody should 
have made a very conscious decision to do that.



But it all seems to work fine in headless mode on the Mac, right?


You mean apart from this bug? ;-) I see a few bugs involving headless on 
OSX.


Cheers,
David


Leonid, did you run headless regression tests with your fix, btw?

--
best regards,
Anthony

On 07/12/12 12:58, Leonid Romanov wrote:

Perhaps Anthony might be able to answer your question: he has tinkered
a lot with headless related stuff.
David, are you implying that in the current form my fix is no-go?

On 12.07.2012, at 5:15, David Holmes wrote:


On 11/07/2012 11:46 PM, Leonid Romanov wrote:

Hi,
Please review a fix for 7181027: [macosx] Unable to use headless
mode. The problem here is that for headless mode
"java.awt.graphicsenv" system property should be
CGraphicsEnvironment because the way GraphicsEnvironment.createGE()
method works: it first instantiates GraphicsEnvironment instance and
then wraps it with HeadlessGraphicsEnvironment if in headless mode.
This means twe can't use java.awt.graphicsenv property to determine
whether we are in headless mode or not. So, I've replaced it with a
toolkit check: if it's HToolkit, then we are in headless.


sun.awt.HToolkit was introduced for use by SE-Embedded's reduced
headless JRE. How did the OSX port end up using it ???

Headless handling on OSX should be like regular headless on Linux,
Solaris.

David


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181027
Webrev: http://cr.openjdk.java.net/~leonidr/7181027/webrev.00/

Thanks,
Leonid.

Re: [7u6] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[David Holmes]

On 13/07/2012 8:13 AM, Oleg Pekhovskiy wrote:

Hi David,

the condition you mentioned:

if (!forceDetach && (peekEvent() != null) ||
!SunToolkit.isPostEventQueueEmpty()) {
return false;
}

could lead to understanding problem (braces are the best way to clarify).

&& has higher priority than ||, thus we could rewrite that condition
like this:

( !forceDetach && (peekEvent() != null) ) ||
!SunToolkit.isPostEventQueueEmpty()


Sorry - need to clean my glasses. I was seeing !f && ( X || Y)

:(


That means SunToolkit.isPostEventQueueEmpty() is called when:
forceDetach == true
OR
peekEvent() == null
OR
both above.

Anthony has pushed the fix that changes (and simplifies) that place:
http://cr.openjdk.java.net/~anthony/7u6-16-missingAWTThread-7162144.0/
So maybe we should discuss the new code - the new behavior.


Yes I saw that patch too. All clear now.

Thanks,
David



Thanks,
Oleg

7/13/2012 1:10 AM, David Holmes wrote:

On 12/07/2012 10:18 PM, Anthony Petrov wrote:

On 07/12/12 05:41, David Holmes wrote:





It also seems that with this fix the interrupted EDT will not detach
due
to the flush being in progress. The EDT will resume the pumpEvents loop
in its run() method. If the interrupt state of the EDT has not been
maintained then the fact it was interrupted (and should detach?) is now
lost. If it is maintained then presumably we will try to
detachDispatchThread again, in which case the EDT will enter a busy
polling loop trying, but failing, to detach, until the flush() has
actually completed.


When interrupt() is called on EDT, the shutdown flag is set, which is
subsequently passed as the forceDetach parameter to
detachDispatchingThread(). If the parameter is true, the detaching
happens unconditionally. So this shouldn't be an issue.


I don't quite follow this part. The logic is:

if (!forceDetach && (peekEvent() != null) ||
!SunToolkit.isPostEventQueueEmpty()) {
return false;
}

If forceDetach is true then we won't execute peekEvent() and
isPostEventQueueEmpty(), which means we would not have hit the
original deadlock.

Oleg's response seems to indicate that we will indeed keep looping but
because of the values of various flags we won't do anything "useful".
But that is the kind of "busy wait" that I was referring to.

Cheers,
David




--
best regards,
Anthony




Cheers,
David



Thanks,
Oleg

7/11/2012 7:35 PM, Anthony Petrov wrote:

Hi Oleg,

1. I suggest to rename isPending to isFlushing, as it reflects the
current state of the PostEventQueue more precisely.

2. I suggest to use the try{}finally{} pattern to set/reset the new
flag in the flush() method to ensure the flag is reset even if the
while() loop throws an exception.

Otherwise the fix looks good.

--
best regards,
Anthony

On 7/11/2012 7:24 PM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7177040

Webrev:
http://cr.openjdk.java.net/~bagiras/7u6/7177040.1

The idea of the fix is that there are two concurrent threads that
try
to get two synchronization objects EventQueue.pushPopLock and
PostEventQueue itself.
For NetBeans these threads are (EDT & WarmUp) or (EDT & TimerQueue).
Problem happens when EDT is interrupted and goes to
EventQueue.detachDispatchThread() where EventQueue.pushPopLock is
owned and
EDT is waiting to own PostEventQueue when calling
SunToolkit.isPostEventQueueEmpty() -> PostEventQueue.noEvents().
At the same time another thread calls EventQueue.postEvent() that
calls EventQueue.flushPendingEvents() -> PostEventQueue.flush()
where
PostEventQueue is owned
and another thread is waiting to own EventQueue.pushPopLock when
calling EventQueue.postEvent() -> EventQueue.postEventPrivate().

To avoid potential deadlock I moved synchronization out of
postEvent()'s cycle in PostEventQueue.flush(),
but to be clear about the existence of Events that are not posted to
EventQueue yet I added PostEventQueue.isPending flag that is true
until the end of the cycle.

There are only two classes that utilize PostEventQueue.flush()
method: SunToolkit.flushPendingEvents() and
SunToolkitSubclass.flushPendingEvents().
They are both synchronized by static SunToolkit.flushLock. That
eliminates the situation when we have overlapped calls of
PostEventQueue.flush().

Thanks,
Oleg

Re: [7u6] Review request for 7181027: [macosx] Unable to use headless mode

[David Holmes]

On 13/07/2012 10:58 PM, Anthony Petrov wrote:

I dug into the code history a little. The following Mike's changeset is
"to blame" for using HToolkit in headless mode on the Mac:

http://hg.openjdk.java.net/macosx-port/macosx-port/jdk/rev/67591b2326bf

I've looked through the LWCToolkit.m which implements native methods
specific to the real, headful Mac toolkit, and actually all of the code
seems to be related to headful behavior only.

Note that in the headless mode the awt_LoadLibrary.c will still load the
correct lwawt dynamic native library, so all the necessary steps to
initialize the app from Mac OS X perspective will be performed anyway,
and all the native methods required by CFontManager and other C* classes
will be available also.

So basically I don't really see a problem with using the HToolkit class
for headless mode on the Mac. Note that Toolkit.getDefaultToolkit() will
wrap the real toolkit instance into a HeadlessToolkit class anyway, so
code that relies on instanceof checks against a toolkit instance should
not be affected by this choice in any way.

David, do you see any specific issues with using HToolkit on a desktop
(Mac) system?


No. I'm just wary of it. I'm curious what would have been done if this 
HToolkit class had not existed?


David
-


--
best regards,
Anthony

On 7/13/2012 1:26 AM, David Holmes wrote:

On 12/07/2012 10:33 PM, Anthony Petrov wrote:

The logic is all in src/solaris/native/java/lang/java_props_macosx.c.
The getPreferredToolkit() returns the HToolkit constant when the
headless mode is needed on the Mac. And the GetJavaProperties() will
then choose the sun.awt.HToolkit as the toolkit. Interesting.


Interesting indeed. But my concerns remain. HToolkit was not intended
for general use. OSX seems to be handling headless mode in a
completely different way to Solaris/linux.

Of course it may be that on OSX you run into the same conditions when
headless that required the introduction of HToolkit. But somebody
should have made a very conscious decision to do that.


But it all seems to work fine in headless mode on the Mac, right?


You mean apart from this bug? ;-) I see a few bugs involving headless
on OSX.

Cheers,
David


Leonid, did you run headless regression tests with your fix, btw?

--
best regards,
Anthony

On 07/12/12 12:58, Leonid Romanov wrote:

Perhaps Anthony might be able to answer your question: he has tinkered
a lot with headless related stuff.
David, are you implying that in the current form my fix is no-go?

On 12.07.2012, at 5:15, David Holmes wrote:


On 11/07/2012 11:46 PM, Leonid Romanov wrote:

Hi,
Please review a fix for 7181027: [macosx] Unable to use headless
mode. The problem here is that for headless mode
"java.awt.graphicsenv" system property should be
CGraphicsEnvironment because the way GraphicsEnvironment.createGE()
method works: it first instantiates GraphicsEnvironment instance and
then wraps it with HeadlessGraphicsEnvironment if in headless mode.
This means twe can't use java.awt.graphicsenv property to determine
whether we are in headless mode or not. So, I've replaced it with a
toolkit check: if it's HToolkit, then we are in headless.


sun.awt.HToolkit was introduced for use by SE-Embedded's reduced
headless JRE. How did the OSX port end up using it ???

Headless handling on OSX should be like regular headless on Linux,
Solaris.

David


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181027
Webrev: http://cr.openjdk.java.net/~leonidr/7181027/webrev.00/

Thanks,
Leonid.

Re: [7u6] Review request for 7181027: [macosx] Unable to use headless mode

[David Holmes]

On 14/07/2012 12:23 AM, Mike Swingler wrote:

On Jul 13, 2012, at 6:22 AM, Anthony Petrov  wrote:

On 7/13/2012 5:09 PM, David Holmes wrote:

On 13/07/2012 10:58 PM, Anthony Petrov wrote:


I dug into the code history a little. The following Mike's changeset is
"to blame" for using HToolkit in headless mode on the Mac:

http://hg.openjdk.java.net/macosx-port/macosx-port/jdk/rev/67591b2326bf

I've looked through the LWCToolkit.m which implements native methods
specific to the real, headful Mac toolkit, and actually all of the code
seems to be related to headful behavior only.

Note that in the headless mode the awt_LoadLibrary.c will still load the
correct lwawt dynamic native library, so all the necessary steps to
initialize the app from Mac OS X perspective will be performed anyway,
and all the native methods required by CFontManager and other C* classes
will be available also.

So basically I don't really see a problem with using the HToolkit class
for headless mode on the Mac. Note that Toolkit.getDefaultToolkit() will
wrap the real toolkit instance into a HeadlessToolkit class anyway, so
code that relies on instanceof checks against a toolkit instance should
not be affected by this choice in any way.

David, do you see any specific issues with using HToolkit on a desktop
(Mac) system?


No. I'm just wary of it. I'm curious what would have been done if this HToolkit 
class had not existed?


Either it would have been introduced, or the LWCToolkit/LWToolkit classes would have been 
made "more headless-aware," so to speak. I think Mike could shed some light on 
his decision.


I was looking for a way to ensure that once the choice to be headless was made, there would be no 
way to undo it. We've had problems in Java SE 6 and prior where the old CToolkit was "mostly 
headless", but could still try to make contact to the window server for somethings (edge cases 
in fonts, IIRC). This would "mostly work" for a while under an SSH session, but would die 
some hours later after Mach bootstrap session expired, and lead to diagnosing some fairly 
frustrating bugs.

With HToolkit, the boot comes down immediately about what you can and cannot do 
- and the implementation looked simple enough, I didn't see much risk. It was 
there, it worked, I went with it.


Anyway, to conclude the reviewing thread, given that we don't (currently) see 
any problems with using HToolkit on the Mac, and the fact that it's already 
been used in headless mode on the Mac for a while, I'm fine with the fix 
proposed by Leonid.


I personally don't see why HToolkit couldn't be used unilaterally for headless 
mode on all platforms. Wouldn't any breakage show an improper layering 
violation that should not have been allowed to occur in the first place?


It was introduced for platforms with absolutely zero "graphics" related 
capabilities - not even libraries installed let alone hardware. The 
pre-existing "headless" toolkits still required some of this support 
for, as I recall, printing/font related things - which must still be 
supported even in headless mode. As long as this is passing the full set 
of TCK/JCK tests then it is usable.


David
-


Regards,
Mike Swingler
Apple Inc.

Re: [7u8] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[David Holmes]

This fix still makes me uncomfortable because flush() is not thread-safe 
any more. If two threads can flush() then the first can clear the 
isFlushing state (in the finally block) that has been set by the second 
(in the synchronized block).


David

On 24/07/2012 9:42 PM, Peter Levart wrote:

Hi Oleg,

This is just cosmetics, but:

SunToolkit:
public synchronized boolean noEvents() {
 return queueHead == null && !isFlushing;
 }

... a thread calling noEvents could see occasional "spikes" of false
return even though there is no flushing being performed (when other
thread is calling flush on an empty PostEventQueue).

Improved flush method would look like this:

 public void flush() {
 EventQueueItem tempQueue;
 synchronized (this) {
 tempQueue = queueHead;
 queueHead = queueTail = null;
 isFlushing =*/(tempQueue != null)/*;

 }
 try {
 while (tempQueue != null) {
 eventQueue.postEvent(tempQueue.event);
 tempQueue = tempQueue.next;
 }
 }
 finally {
 isFlushing = false;
 }
 }


Regards, Peter

2012/7/23 Oleg Pekhovskiy mailto:oleg.pekhovs...@oracle.com>>

Hi!

Please review this back-port being already pushed to jdk8 but
deferred for 7u6.

Bug:
http://bugs.sun.com/view_bug.__do?bug_id=7177040


Webrev:
http://cr.openjdk.java.net/~__bagiras/7u8/7177040.1


Review thread for 7u6:
http://mail.openjdk.java.net/__pipermail/awt-dev/2012-July/__003106.html


Reviewers 7u6 & 8:
Anthony Petrov, Anton Tarasov

Thanks,
Oleg

Re: [7u8] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[David Holmes]

On 26/07/2012 11:59 PM, Oleg Pekhovskiy wrote:

it's thread-safe indirectly. SunToolkit.flushPendingEvents() &
SunToolkitSubclass.flushPendingEvents() are the only methods that call
PostEventQueue.flush().
They both have flushLock synchronization object that guarantees
synchronized call of PostEventQueue.flush().


Yes but it is that undocumented indirection that concerns me. If someone 
decides to remove those outer locks they may not realize there is a 
lurking race here.


David


Thanks,
Oleg

7/26/2012 4:08 PM, David Holmes wrote:

This fix still makes me uncomfortable because flush() is not
thread-safe any more. If two threads can flush() then the first can
clear the isFlushing state (in the finally block) that has been set by
the second (in the synchronized block).

David

On 24/07/2012 9:42 PM, Peter Levart wrote:

Hi Oleg,

This is just cosmetics, but:

SunToolkit:
public synchronized boolean noEvents() {
return queueHead == null && !isFlushing;
}

... a thread calling noEvents could see occasional "spikes" of false
return even though there is no flushing being performed (when other
thread is calling flush on an empty PostEventQueue).

Improved flush method would look like this:

public void flush() {
EventQueueItem tempQueue;
synchronized (this) {
tempQueue = queueHead;
queueHead = queueTail = null;
isFlushing =*/(tempQueue != null)/*;

}
try {
while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
}
finally {
isFlushing = false;
}
}


Regards, Peter

2012/7/23 Oleg Pekhovskiy mailto:oleg.pekhovs...@oracle.com>>

Hi!

Please review this back-port being already pushed to jdk8 but
deferred for 7u6.

Bug:
http://bugs.sun.com/view_bug.__do?bug_id=7177040
<http://bugs.sun.com/view_bug.do?bug_id=7177040>

Webrev:
http://cr.openjdk.java.net/~__bagiras/7u8/7177040.1
<http://cr.openjdk.java.net/~bagiras/7u8/7177040.1>

Review thread for 7u6:
http://mail.openjdk.java.net/__pipermail/awt-dev/2012-July/__003106.html
<http://mail.openjdk.java.net/pipermail/awt-dev/2012-July/003106.html>

Reviewers 7u6 & 8:
Anthony Petrov, Anton Tarasov

Thanks,
Oleg

Re: [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[David Holmes]

Hi Anthony,

I took a look at the last incarnation of this so let me see if I can 
follow the new scheme.


On 29/08/2012 9:08 PM, Anthony Petrov wrote:

Hi Oleg,

I'm still concerned about the following:

detachDispatchThread()
{
flush();
lock();
// possibly detach
unlock();
}

at EventQueue.java. What if an even get posted to the queue after the
flush() returns but before we even acquired the lock? We may still end
up with a situation when we detach the thread while in fact there are
some pending events present, which actually contradicts the current
logic of the detach() method. I see that you say "Minimize discard
possibility" in the comment instead of "Prevent ...", but I feel
uncomfortable with this actually.


If a new event is posted before the lock() then within the locked region 
won't peekEvent() see it and so avoid the detach?



What exactly prevents us from adding some synchronization to ensure that
the detaching only happens when there's really no pending events?

SunToolkit.java:

2120 Boolean b = isThreadLocalFlushing.get();
2121 if (b != null && b) {
2122 return;
2123 }
2124 2125 isThreadLocalFlushing.set(true);
2126 try {


How does access to the isThreadLocalFlushing synchronized? What happens
if the flush() is being invoked from two different threads for the same
post event queue? Why do we have two "isFlushing" flags? Can we collapse
them into one? Why is the isFlushing set/reset in two disjunct
synchronized(){} blocks?


isThreadLocalFlushing is a ThreadLocal so no synchronization is needed. 
I presume it is used to prevent re-entrant/recursive calls to flush() 
when calling postEvent.


The isFlushing variable is the global flag to coordinate flushing across 
multiple threads. It has to be set and cleared in synchronized blocks, 
but the synchronization lock has to be dropped when calling postEvent to 
avoid deadlocks. So a thread acquires the lock and checks if flushing is 
in progress, and if so it waits. Else/then it updates isFlushing to 
indicate if that thread is doing flushing and releases the lock. It then 
does any flushing needed, reacquires the lock, sets isFlushing to false 
and notifies any other threads who may be waiting.



Overall, I find the current synchronization scheme in flush() very,
*very* (and I mean it) confusing. Can we simplify it somehow?


This seems like a reasonable protocol to coordinate multiple flushers 
whilst dropping the synchronization lock when posting events. The actual 
coordination might be simpler to understand if expressed using a 
Semaphore - but I think the semantics would be the same.


David


--
best regards,
Anthony

On 8/28/2012 6:33 PM, Oleg Pekhovskiy wrote:

Hi Artem, Anthony,

thank you for your proposals!

We with Artem also had off-line discussion,
so as a result I prepared improved version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.3/

What was done:
1. EventQueue.detachDispatchThread(): moved
SunToolkit.flushPnedingEvents() above the comments and added a
separate comment to it.
2. Moved SunToolkitSubclass.flushPendingEvents(AppContext) method to
SunToolkit. Deleted SunToolkitSubclass.
3. Moved isFlushingPendingEvents to PostEventQueue with the new name -
isThreadLocalFlushing and made it ThreadLocal.
4. Left PostEventQueue.flush() unsynchronized and created
wait()-notifyAll() synchronization mechanism to avoid blocking of
PostEventQueue.postEvent().

Looking forward to your comments!

Thanks,
Oleg

20.08.2012 20:20, Artem Ananiev wrote:

Hi, Oleg,

here are a few comments:

1. What is the reason of keeping "isFlushingPendingEvents" in
SunToolkit, given that PEQ.flush() is synchronized (and therefore
serialized) anyway?

2. flushPendingEvents(AppContext) may be moved directly to
SunToolkit, so we don't need a separate sun-class for that.

3. EQ.java:1035-1040 - this comment is obsolete and must be replaced
by another one.

Thanks,

Artem

On 8/17/2012 4:49 PM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7186109

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7186109.1/

The following changes were made:
1. Removed flushLock from SunToolkit.flushPendingEvent()
2. Returned method PostEventQueue.flush() as 'synchronized' back
3. Added call of SunToolkit.flushPendingEvents() to
EventQueue.detachDispatchThread(),
right before pushPopLock.lock()
4. Removed !SunToolkit.isPostEventQueueEmpty() check from
EventQueue.detachDispatchThread()
5. Removed SunToolkit.isPostEventQueueEmpty() &
PostEventQueue.noEvents();

Thanks,
Oleg

Re: [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[David Holmes]

> Ah, I see. Thanks for the insight. It now looks much clearer. I think
> that the final isThreadLocalFlushing.set(false); must be in the
> finally{} block, though.

Right!

Also there is a problem with the InterruptedException handling. Let 
thread A set isFlushing and be busy flushing. Then let Thread B call 
wait() but be interrupted. Thread B will enter the finally block grab 
the lock and set isFlushing to false, even though Thread A is actively 
flushing! We don't want the finally block to execute if 
InterruptedException is caught.


David

On 29/08/2012 10:02 PM, Anthony Petrov wrote:

Hi David,

On 8/29/2012 3:45 PM, David Holmes wrote:

I took a look at the last incarnation of this so let me see if I can
follow the new scheme.

On 29/08/2012 9:08 PM, Anthony Petrov wrote:

Hi Oleg,

I'm still concerned about the following:

detachDispatchThread()
{
flush();
lock();
// possibly detach
unlock();
}

at EventQueue.java. What if an even get posted to the queue after the
flush() returns but before we even acquired the lock? We may still end
up with a situation when we detach the thread while in fact there are
some pending events present, which actually contradicts the current
logic of the detach() method. I see that you say "Minimize discard
possibility" in the comment instead of "Prevent ...", but I feel
uncomfortable with this actually.


If a new event is posted before the lock() then within the locked
region won't peekEvent() see it and so avoid the detach?


peekEvent() checks the event queue only, while the posted event may be
stuck in the PostEventQueue. The flushPendingEvents() actually posts the
events from the PostEventQueue to the EventQueue.



What exactly prevents us from adding some synchronization to ensure that
the detaching only happens when there's really no pending events?

SunToolkit.java:

2120 Boolean b = isThreadLocalFlushing.get();
2121 if (b != null && b) {
2122 return;
2123 }
2124 2125 isThreadLocalFlushing.set(true);
2126 try {


How does access to the isThreadLocalFlushing synchronized? What happens
if the flush() is being invoked from two different threads for the same
post event queue? Why do we have two "isFlushing" flags? Can we collapse
them into one? Why is the isFlushing set/reset in two disjunct
synchronized(){} blocks?


isThreadLocalFlushing is a ThreadLocal so no synchronization is
needed. I presume it is used to prevent re-entrant/recursive calls to
flush() when calling postEvent.

The isFlushing variable is the global flag to coordinate flushing
across multiple threads. It has to be set and cleared in synchronized
blocks, but the synchronization lock has to be dropped when calling
postEvent to avoid deadlocks. So a thread acquires the lock and checks
if flushing is in progress, and if so it waits. Else/then it updates
isFlushing to indicate if that thread is doing flushing and releases
the lock. It then does any flushing needed, reacquires the lock, sets
isFlushing to false and notifies any other threads who may be waiting.


Overall, I find the current synchronization scheme in flush() very,
*very* (and I mean it) confusing. Can we simplify it somehow?


This seems like a reasonable protocol to coordinate multiple flushers
whilst dropping the synchronization lock when posting events. The
actual coordination might be simpler to understand if expressed using
a Semaphore - but I think the semantics would be the same.


Ah, I see. Thanks for the insight. It now looks much clearer. I think
that the final isThreadLocalFlushing.set(false); must be in the
finally{} block, though.

--
best regards,
Anthony




David


--
best regards,
Anthony

On 8/28/2012 6:33 PM, Oleg Pekhovskiy wrote:

Hi Artem, Anthony,

thank you for your proposals!

We with Artem also had off-line discussion,
so as a result I prepared improved version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.3/

What was done:
1. EventQueue.detachDispatchThread(): moved
SunToolkit.flushPnedingEvents() above the comments and added a
separate comment to it.
2. Moved SunToolkitSubclass.flushPendingEvents(AppContext) method to
SunToolkit. Deleted SunToolkitSubclass.
3. Moved isFlushingPendingEvents to PostEventQueue with the new name -
isThreadLocalFlushing and made it ThreadLocal.
4. Left PostEventQueue.flush() unsynchronized and created
wait()-notifyAll() synchronization mechanism to avoid blocking of
PostEventQueue.postEvent().

Looking forward to your comments!

Thanks,
Oleg

20.08.2012 20:20, Artem Ananiev wrote:

Hi, Oleg,

here are a few comments:

1. What is the reason of keeping "isFlushingPendingEvents" in
SunToolkit, given that PEQ.flush() is synchronized (and therefore
serialized) anyway?

2. flushPendingEvents(AppContext) may be moved directly to
SunToolkit, so we don't need a separate sun-class for that.

3. EQ.java:1035-1040 - this comment is obsolete and must be replaced
by another one.

Thanks,

Arte

Re: [7u10] Review request for 7188708 - REGRESSION: closed/java/awt/EventQueue/PostEventOrderingTest.java fails

[David Holmes]

Hi Oleg,

It seems to me that the original code has the

 isFlushingPendingEvents = false;

in the wrong place: it should only ever be cleared by an invocation that 
set it. So a simple reorganization of the code would achieve that:


 553 public static void flushPendingEvents()  {
 554 flushLock.lock();
 555 try {
 556 // Don't call flushPendingEvents() recursively
 557 if (!isFlushingPendingEvents) {
 558 isFlushingPendingEvents = true;
+ try {
559 AppContext appContext = AppContext.getAppContext();
560 PostEventQueue postEventQueue =
561 
(PostEventQueue)appContext.get(POST_EVENT_QUEUE_KEY);

562 if (postEventQueue != null) {
563 postEventQueue.flush();
564 }
+} finally {
+   isFlushingPendingEvents = false;
+}
 565 }
 566 } finally {
-567
 568 flushLock.unlock();
 569 }
 570 }

David
-

On 7/09/2012 11:57 AM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7188708

Webrev:
http://cr.openjdk.java.net/~bagiras/7u10/7188708.1/

The reason is that isFlushingPendingEvents in
SunToolkit.flushPendingEvents() is reset
after the first recursive call of flushPendingEvents(). Thus, if there
are several pending events in PostEventQueue,
recursion would be rejected only for the first event, allowing nested
call of PostEventQueue.flush() after.

To resolve the problem I added the counter "flushNestingLevel" instead
of the flag "isFlushingPendingEvents".
It's increased each time entering SunToolkit.flushPendingEvents() and
decreased on exit.
So, PostEventQueue.flush() method is called only when we enter
SunToolkit.flushPendingEvents() for the first time
within one thread.

That fix was prepared ONLY for 7u10.
For JDK 8 the fix for "CR7186109 - Simplify lock machinery for
PostEventQueue & EventQueue" should cover this case.

Thanks,
Oleg

Re: [7u10] Review request for 7188708 - REGRESSION: closed/java/awt/EventQueue/PostEventOrderingTest.java fails

[David Holmes]

That looks okay to me - it correctly prevents recursive calls.

David

On 7/09/2012 10:39 PM, Oleg Pekhovskiy wrote:

Hi David,

I thought about that variant first, then something stopped me.
Anyway, it resolves the regression.

Please review the changes here:
http://cr.openjdk.java.net/~bagiras/7u10/7188708.2/

Thanks,
Oleg

9/7/2012 3:36 PM, David Holmes wrote:

Hi Oleg,

It seems to me that the original code has the

isFlushingPendingEvents = false;

in the wrong place: it should only ever be cleared by an invocation
that set it. So a simple reorganization of the code would achieve that:

553 public static void flushPendingEvents() {
554 flushLock.lock();
555 try {
556 // Don't call flushPendingEvents() recursively
557 if (!isFlushingPendingEvents) {
558 isFlushingPendingEvents = true;
+ try {
559 AppContext appContext = AppContext.getAppContext();
560 PostEventQueue postEventQueue =
561 (PostEventQueue)appContext.get(POST_EVENT_QUEUE_KEY);
562 if (postEventQueue != null) {
563 postEventQueue.flush();
564 }
+ } finally {
+ isFlushingPendingEvents = false;
+ }
565 }
566 } finally {
-567
568 flushLock.unlock();
569 }
570 }

David
-

On 7/09/2012 11:57 AM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7188708

Webrev:
http://cr.openjdk.java.net/~bagiras/7u10/7188708.1/

The reason is that isFlushingPendingEvents in
SunToolkit.flushPendingEvents() is reset
after the first recursive call of flushPendingEvents(). Thus, if there
are several pending events in PostEventQueue,
recursion would be rejected only for the first event, allowing nested
call of PostEventQueue.flush() after.

To resolve the problem I added the counter "flushNestingLevel" instead
of the flag "isFlushingPendingEvents".
It's increased each time entering SunToolkit.flushPendingEvents() and
decreased on exit.
So, PostEventQueue.flush() method is called only when we enter
SunToolkit.flushPendingEvents() for the first time
within one thread.

That fix was prepared ONLY for 7u10.
For JDK 8 the fix for "CR7186109 - Simplify lock machinery for
PostEventQueue & EventQueue" should cover this case.

Thanks,
Oleg

Re: [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[David Holmes]

Hi Oleg,

I can't really comment on the higher-level logic/protocol here as I 
can't track which thread does what, where and when.


In EventQueue.java this comment is no longer valid:

  * Fix for 4648733. Check both the associated java event
  * queue and the PostEventQueue.
  */
! if (!forceDetach && (peekEvent() != null)) {

as the PostEventQueue is not checked.

---

The flush() logic can be simplified somewhat as you can check for 
recursion outside the sync block** and outside any try/finally:


   public void flush() {

Thread newThread = Thread.currentThread();
// Avoid method recursion
if (newThread == flushThread) {
return;
}

try {
EventQueueItem tempQueue;
synchronized (this) {
// Wait for other threads' flushing
while (flushThread != null) {
wait();
}
// Skip everything if queue is empty
if (queueHead == null) {
notifyAll();
return;
}
// Remember flushing thread
flushThread = newThread;

tempQueue = queueHead;
queueHead = queueTail = null;
}

while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
}
catch (InterruptedException e) {
// Couldn't allow exception go up, so at least recover the flag
newThread.interrupt();
}
finally {
synchronized (this) {
// Forget flushing thread, inform other pending threads
if (newThread == flushThread) {
flushThread = null;
notifyAll();
}
}
}
}

** This is safe because a thread only ever writes its own value to 
flushThread so even if it reads a stale value that value will either be 
null or some other thread - either way it is not the current thread so 
it proceeds with the main logic.


I also think you can move the try/finally to around the inner code that 
does the event processing, and leave just the try/catch at the outer-level


   public void flush() {

Thread newThread = Thread.currentThread();
// Avoid method recursion
if (newThread == flushThread) {
return;
}

try {
EventQueueItem tempQueue;
synchronized (this) {
// Wait for other threads' flushing
while (flushThread != null) {
wait();
}
// Skip everything if queue is empty
if (queueHead == null) {
notifyAll();
return;
}
// Remember flushing thread
flushThread = newThread;

tempQueue = queueHead;
queueHead = queueTail = null;
}
try {
while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
} finally {
// only the flushing thread can get here
synchronized (this) {
// Forget flushing thread, inform other pending threads
flushThread = null;
notifyAll();
}
}

}
catch (InterruptedException e) {
// Couldn't allow exception go up, so at least recover the flag
newThread.interrupt();
}
}

I'm also not convinced the notifyAll() is needed when you "Skip 
everything". It's harmless from a functional perspective, but 
unnecessary I think.


David

On 8/09/2012 10:09 AM, Oleg Pekhovskiy wrote:

Artem, Anthony, David,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.4/

What's new in comparison with the previous version:

1. Removed "isThreadLocalFlushing" and "isFlashing", created
"flushThread" instead
(stores the thread that currently performs flushing).
2. Implemented both recursion and multi-thread block on "flushThread" only.
3. Added Thread.interrupt() to the catch block as outside we would like
to know what happened in PostEventQueue.flush().
We are not able to rethrow the exception because we couldn't change the
signature (by adding "throwns")
for all related methods (e.g. EventQueue.postEvent()).

The fix successfully passed
"closed/java/awt/EventQueue/PostEventOrderingTest.java" test.

IMHO, the code became clearer.

Looking forward to your comments!

Thanks,
Oleg

8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:

There are also other 2 methods that will require 'throws
InterruptedException' or try-catch:
1. EventQueue.postEvent()
2. EventQueue.removeSourceEvents()

Thanks,
Oleg

Re: [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[David Holmes]

On 11/09/2012 1:17 AM, Artem Ananiev wrote:


On 9/10/2012 6:50 PM, Anthony Petrov wrote:

On 09/10/12 15:27, Artem Ananiev wrote:

** This is safe because a thread only ever writes its own value to
flushThread so even if it reads a stale value that value will either be
null or some other thread - either way it is not the current thread so
it proceeds with the main logic.


The "flushThread" field is not volatile, so we can't check its value
outside of synchronized blocks.


In this particular case you can do that, and in the above quote David
explains why.


Got it. I didn't even think about writing the code that survive stale
values, and therefore missed David's comment. Anyway, I don't think such
an anti-pattern as reading non-volatile field value outside of
synchronized block is acceptable. And I'm pretty sure static analyzers
like FindBugs will find this violation.


Yes they might, which is shame because this is one of a few patterns for 
data-races that are perfectly safe and valid. But if you are that 
concerned then make it volatile - I don't think it will affect 
performance in this context and I think the overall code simplification 
is worth it.


Cheers,
David



Thanks,

Artem


In other words: you only want to check whether the flushThread refers to
the current thread or not. If it's been actually set by the current
thread, then this thread must see the correct value w/o any
synchronization needed. Otherwise, (if it's null or set by another
thread,) your code will see a value that doesn't refer to the current
thread, and this is exactly what you wanted to check.

So I agree with David, this test needs no synchronization.

--
best regards,
Anthony

Re: [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[David Holmes]

Hi Oleg,

I have no further comments.

Thanks,
David

On 13/09/2012 4:56 AM, Oleg Pekhovskiy wrote:

Hi!

please look at the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.5/

What's new:
1. Moved "finally" block below "while-cycle".
2. Removed redundant notifyAll().
2. Added PostEventOrderingTest.java to the tests.
3. Removed "Fix for 4648733..." comment.

I left recursion check inside synchronized block to avoid making
flushThread "volatile"
or confusing people with unsynchronized usage of non-volatile variable,
in spite of the fact
it works properly.

Thanks,
Oleg

9/10/2012 3:27 PM, Artem Ananiev wrote:

Hi, David,

see my comments inline.

On 9/8/2012 4:44 AM, David Holmes wrote:

Hi Oleg,

I can't really comment on the higher-level logic/protocol here as I
can't track which thread does what, where and when.

In EventQueue.java this comment is no longer valid:

* Fix for 4648733. Check both the associated java event
* queue and the PostEventQueue.
*/
! if (!forceDetach && (peekEvent() != null)) {

as the PostEventQueue is not checked.


Agree.


---

The flush() logic can be simplified somewhat as you can check for
recursion outside the sync block** and outside any try/finally:

public void flush() {

Thread newThread = Thread.currentThread();
// Avoid method recursion
if (newThread == flushThread) {
return;
}

try {
EventQueueItem tempQueue;
synchronized (this) {
// Wait for other threads' flushing
while (flushThread != null) {
wait();
}
// Skip everything if queue is empty
if (queueHead == null) {
notifyAll();
return;
}
// Remember flushing thread
flushThread = newThread;

tempQueue = queueHead;
queueHead = queueTail = null;
}

while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
}
catch (InterruptedException e) {
// Couldn't allow exception go up, so at least recover the
flag
newThread.interrupt();
}
finally {
synchronized (this) {
// Forget flushing thread, inform other pending threads
if (newThread == flushThread) {
flushThread = null;
notifyAll();
}
}
}
}

** This is safe because a thread only ever writes its own value to
flushThread so even if it reads a stale value that value will either be
null or some other thread - either way it is not the current thread so
it proceeds with the main logic.


The "flushThread" field is not volatile, so we can't check its value
outside of synchronized blocks.


I also think you can move the try/finally to around the inner code that
does the event processing, and leave just the try/catch at the
outer-level

public void flush() {

Thread newThread = Thread.currentThread();
// Avoid method recursion
if (newThread == flushThread) {
return;
}

try {
EventQueueItem tempQueue;
synchronized (this) {
// Wait for other threads' flushing
while (flushThread != null) {
wait();
}
// Skip everything if queue is empty
if (queueHead == null) {
notifyAll();
return;
}
// Remember flushing thread
flushThread = newThread;

tempQueue = queueHead;
queueHead = queueTail = null;
}
try {
while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
} finally {
// only the flushing thread can get here
synchronized (this) {
// Forget flushing thread, inform other pending
threads
flushThread = null;
notifyAll();
}
}

}
catch (InterruptedException e) {
// Couldn't allow exception go up, so at least recover the
flag
newThread.interrupt();
}
}

I'm also not convinced the notifyAll() is needed when you "Skip
everything". It's harmless from a functional perspective, but
unnecessary I think.


Agree, it looks redundant.

Thanks,

Artem


David

On 8/09/2012 10:09 AM, Oleg Pekhovskiy wrote:

Artem, Anthony, David,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.4/

What's new in comparison with the previous version:

1. Removed "isThreadLocalFlushing" and "isFlashing", created
"flushThread" instead
(stores the thread that currently performs flushing).
2. Implemented both recursion and multi-thread block on "flushThread"
only.
3. Added Thread.interrupt() to the catch block as outside we would like
to know what happened in PostEventQueue.flush().
We are not able to rethrow the exception because we couldn't change the
signature (by adding "throwns")
for all related methods (e.g. EventQueue.postEvent()).

The fix successfully passed
"closed/java/awt/EventQueue/PostEventOrderingTest.java" test.

IMHO, the code became clearer.

Looking forward to your comments!

Thanks,
Oleg

8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:

There are also other 2 methods that will require 'throws
InterruptedException' or try-catch:
1. EventQueue.postEvent()
2. EventQueue.removeSourceEvents()

Thanks,
Oleg

8/30/2012 9:01 PM, Oleg Pekhovskiy wrote:

Hi,

I got another idea preparing the next 

Re: Review Request: 8004151: build-infra: Generating X11 wrapper offset file is not cross compilable (AWT folks look here!)

[David Holmes]

Minor typo in the gmk file

 32 # These are the once used.

once -> ones

Thanks,
David

On 19/01/2013 12:05 AM, Erik Joelsson wrote:

Here is a new webrev with my sorts incorporated. This has been baking
for quite a while in build-infra now and seems to be working.

http://cr.openjdk.java.net/~erikj/8004151/webrev.jdk.04/

/Erik

On 2012-12-07 11:05, Erik Joelsson wrote:

(resending to full recepients list)

I just hit another issue. I tried using a jdk8 boot-jdk and the order
of the output was different in the generated sizes file compared to
the one in the repo. Sorting both resulted in a clean diff.

/Erik

On 2012-12-05 14:20, Fredrik Öhrström wrote:

2012-11-29 15:54, Fredrik Öhrström skrev:

2012-11-29 15:36, Erik Joelsson skrev:

I just submitted a patch to build-infra for the dual generation on
all platforms since it breaks comparisons between old and new
build. In general, we can't change behavior in new build without
also changing the old before the old is removed.

Removing sizes.64-solaris-i386 breaks the old build on solaris-x86,
so I have readded it to build-infra.


Thanks Erik! The new and updated webrev is here:

http://cr.openjdk.java.net/~ohrstrom/webrev-8004151-gensrcX11wrapper-v2/




Ok, new webrev incorporating a few more Erik changes. Any comments
from the AWT experts?
http://cr.openjdk.java.net/~ohrstrom/webrev-8004151-gensrcX11wrapper-v3/

//Fredrik