Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
Magnus: Looks good to me as well. Tim On 03/04/15 05:31, Erik Joelsson wrote: Hello, Really nice to finally see this patch getting done! Only one comment: flags.m4: In the grep expression, could you move the extra [] outside of the actual command line options to grep so that the command line could be copied to the shell for debugging in the future? Also, how hard would it be to instead do AC_COMPILE_IFELSE with a dummy warning to instead test for capability? /Erik On 2015-03-04 14:17, Magnus Ihse Bursie wrote: When building the native code in the jdk repo, a lot of warnings are generated. So many that it's hard to spot any new issues. While the ultimate goal must be to address and fix these warnings individually, this bug is about disabling these warnings where they occur, so that it is easier to spot new warnings, and that the existing warnings can be addressed one at a time. Disabling warnings instead of fixing them can be problematic. If you are too broad with disabling warnings, you might hide future problems. On the other hand, there have been a lot of warnings that indicate serious problems that has been hidden in plain sight for a very long time in the code base anyway. I have opted to disable warnings at the library level. Disabling warnings globally would be too broad. Disabling per file would have been too tedious. Many warnings also tend to repeat across multiple files in the same library, due to e.g. less well-suited programming style or design choice. A library also seems like a suitable chunk of work to address later on, in trying to get rid of the source of the warnings. This fix will not get rid of all warnings, but the bulk of them. It will make the remaining warnings stick out more. The few warnings that remain are either: * Not possible to disable. * Not resulting from native compilation (but from linking, or javadoc, etc). * Not possible to disable with this framework (this goes for builds on pre-4.4 gcc, which Oracle currently does as default on macosx), and libfontmanager on Solaris, which mixes C and C++ code. (While the solstudio C compiler does not fail on requests to disable C++ warnings, the converse is unfortunately not true). It did not seem worth the trouble to build a more extensible framework to handle this, compared to actually fixing these warnings. Note that the current JPRT build environment in Oracle uses a pre-4.4 gcc for macosx by default, so the default macosx build will still contain warnings. When building with --with-toolchain-type=clang, the clang warning disabling kicks in. (This will be the default in JPRT later on this year.) I intend to file individual bugs to handle these remaining warnings, so the net result will be a completely warning free build. I also intend to file individual bugs on all the libraries that has had warnings disabled. I expect the outcome of these bugs to be either: A) The code modified so it does not trigger warnings, and the warnings re-enabled, or B) The warnings (or a subset of them) kept disabled, but a comment added to the makefile describing why this is the proper course of action. Not all bugs might be worth fixing. For instance, the GCC warnings type-limits, sign-compare, pointer-to-int-cast, conversion-null, deprecated-declarations, clobbered, int-to-pointer-cast and type-limits are all more or less benign, and is possibly the result of a false positive. On the other hands, warnings such as format-nonliteral, unused-result, maybe-uninitialized, format, format-security, int-to-pointer-cast, reorder and delete-non-virtual-dtor are more likely to actually point to real issues. I recommend anyone finding these warnings on a library they care about to try and fix them as soon as possible. Here is a summary of the libraries and binaries that have gotten warnings disabled. I'm not sure about what group owns all these libraries; if I missed sending this mail to the correct list, please help me and forward it. * libunpack * libfdlibm * libverify * libjava * libzip * libjli/libjli_static * libj2gss * libsunec * libj2pkcs11 * libnet * libnio * libosxkrb5 * libosxapp * libosx * libapplescriptengine * libjsound * libjsoundalsa * libmlib_image * libawt * libawt_xawt * libawt_lwawt * liblcms * libjavajpeg * libawt_headless * libfontmanager * libsplashscreen * unpack200 * The JVMTI demos compiledMethodLoad and waiters Bug: https://bugs.openjdk.java.net/browse/JDK-8074096 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8074096-disable-native-warnings/webrev.01 /Magnus
Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
On 04/03/2015 15:03, Magnus Ihse Bursie wrote: I believe all warnings are important to check! Unfortunately, this has not been done for a lot of warnings for a lot of time. :( Right, although in the past we had some areas close to be cleared of warnings, it's just that we didn't keep up the effort and of course the compilers get more pedantic with each version. With this framework, it is simple to enable a single warning, recompile and see the effect. Hopefully this lowers the threshold far enough so that all warnings are given proper attention. The incremental build functionality will come in very handy. Just by simply removing a warning from the DISABLED_WARNINGS_toolchain on a library and running make again, only the files affected will be recompiled. Yes, it looks easy to maintain. I can easily paste in what warning categories are disabled for a specific library, yes. However, if you are asking me to build each library, individually, with warnings re-enabled, and pasting the output, I'd rather not. That would be a lot of work, to detangle the output of each individual library. I'm not asking that, it would be too much work. Maybe it's worth saving the logs somewhere so that you can point the bugs at it. It would also be useful for the bugs to point to your change sets (when they are pushed) so that it's obvious which make files were changed to silence the warnings. -Alan
Re: AWT Dev [9] Review Request for 8074092: Newly introduced unnecessary dependencies on internal API in client regtests
Hi Yuri, the fix looks good to me too. -- Thanks, Alexander. On 04.03.2015 16:28, Yuri Nesterenko wrote: Hi! Please review this fix for several regression tests. Webrev: http://cr.openjdk.java.net/~yan/8074092/webrev.00 CR: https://bugs.openjdk.java.net/browse/JDK-8074092 In bug8062561.java test an error message also changed. Verified on 3 platforms. Thanks, -yan
Re: AWT Dev [9] Review Request: 8074028 Remove API references to java.awt.peer
On 03/04/2015 04:35 PM, Sergey Bylokhov wrote: On 04.03.2015 15:51, Alan Bateman wrote: On 04/03/2015 12:37, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. There are a number of public API whichreference the unsupported java.awt.peer interfaces. public java.awt.Component.getPeer() @deprecated 1.1 public java.awt.Font.getPeer() @deprecated 1.1 public java.awt.MenuComponent.getPeer() @deprecated 1.1 I don't know this area so I don't feel qualified to do a thorough review of the changes. However I welcome the changes, this is long standing technical debt and important that we address this as part of the move to modules. I note that Font::getPeer is changed to private, hence something try to access it outside of the Font class will get an IllegalAccessError as opposed to NoSuchMethodError for the outright removal in the other cases. Did you consider renaming to something else, say private FontPeer getFontPeer so that it is consistent with the removal of the other two methods? Make sense, I will update the fix. I note that you might not have caught all tests but I would assume that any missed tests should fail. Perhaps there are other failures that are potentially hiding tests using getPeer? At least I fixed most(if not all) tests, which were passed before the fix and fails after Speaking of the tests, there are some more to fix (like test/java/awt/regtesthelpers/UtilInternal.java etc.) but not too many. We could do it separately. -yan -Alan
Re: AWT Dev Swing Dev [9] Review Request for 8074092: Newly introduced unnecessary dependencies on internal API in client regtests
Hi, @library ../../regtesthelpers @library ../../../../lib/testlibrary can be replaced with @library /javax/swing/regtesthelper @library /lib/testlibrary Library path is concatenated to the directory in which TEST.ROOT is located. Andrei Eremeev On 03/04/2015 04:28 PM, Yuri Nesterenko wrote: Hi! Please review this fix for several regression tests. Webrev: http://cr.openjdk.java.net/~yan/8074092/webrev.00 CR: https://bugs.openjdk.java.net/browse/JDK-8074092 In bug8062561.java test an error message also changed. Verified on 3 platforms. Thanks, -yan
Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
I like the overall approach. We can then attack the warnings lib by lib and/or platform by platform. I do want to mention that some libs like liblcms are 3rd party open source libraries and it may not always be possible to convince upstream to change their code. -phil. On 03/04/2015 08:30 AM, Alan Bateman wrote: On 04/03/2015 15:03, Magnus Ihse Bursie wrote: I believe all warnings are important to check! Unfortunately, this has not been done for a lot of warnings for a lot of time. :( Right, although in the past we had some areas close to be cleared of warnings, it's just that we didn't keep up the effort and of course the compilers get more pedantic with each version. With this framework, it is simple to enable a single warning, recompile and see the effect. Hopefully this lowers the threshold far enough so that all warnings are given proper attention. The incremental build functionality will come in very handy. Just by simply removing a warning from the DISABLED_WARNINGS_toolchain on a library and running make again, only the files affected will be recompiled. Yes, it looks easy to maintain. I can easily paste in what warning categories are disabled for a specific library, yes. However, if you are asking me to build each library, individually, with warnings re-enabled, and pasting the output, I'd rather not. That would be a lot of work, to detangle the output of each individual library. I'm not asking that, it would be too much work. Maybe it's worth saving the logs somewhere so that you can point the bugs at it. It would also be useful for the bugs to point to your change sets (when they are pushed) so that it's obvious which make files were changed to silence the warnings. -Alan
Re: AWT Dev [9] review request for 8072769: System tray icon title freezes java
On 04.03.2015 16:33, Semyon Sadetsky wrote: Sergey, OK. You can file a request to improve TrayIcon test coverage but it's an another story. Here we are discussing regression test for a specific bug. I cannot agree with the approach you've proposed. In your logic we need start to write regression tests for all methods in JDK classes those take String parameters by iterating all possible parameters lengths to discover potential bugs. Yes, if we know that there is a high probability to crash the method. Tests need to be written by certain rules. Scanning all possible values of input parameters is not the best rule in my opinion. Validation of one number is useless too. Its even does not check all corner cases , 0,1,max-1,max,max+1. so the range is better. --Semyon On 3/4/2015 4:03 PM, Sergey Bylokhov wrote: On 04.03.2015 15:08, Semyon Sadetsky wrote: I think it's more like bug regression test policy. Since I'm a novice in the project maybe you or somebody can advise me the right process. Lets see what we have here: The fix is connected to the windows platform native code only. It is a platform specific corner case. I would not say it contains magic numbers, those numbers a not magic for the native layer. But in the test we call native code from java where in truth string parameters maximum lengths are not specified. I think it make sense to run the test on all other platforms test iterating strings lengths up to 1000 or more just to ensure that there no similar issues there. We can do this once or have an option to switch this scenario on. The goal of the test is to catch all possibly related issues. So instead of numbers use ranges, cover all platforms if the test don't use platform's specific classes, cover all look and feels, automatic test is better than manual, make the test generic. But it has of course some restrictions like performance and stability. Moreover it seems that right now displayMessage/setToolTip are used in the manual tests only. But I'm not 100% sure that such test scenario should be included in the regular regression run because sending messages to the system tray is not very fast and the scan can take noticeable amount of time. In my opinion such issue seeking scenario is reasonable to run time to time or as a part of stress testing profile, but it does not make sense to run it during the regular regression because it tests nothing specific on platforms other than Windows and even in Windows there is only one specific lengths combination. It's not worth to do this potential issue seek it's time expensive but will not bring us a lot of value really. --Semyon On 3/4/2015 2:13 PM, Sergey Bylokhov wrote: Hi, Semyon. I suggest to remove stuff related to windows platform from the test. Also it will be good to test some reasonable range of data instead of magic constant, wrap setTooltip/displayMessage in a loop. Please add tray.remove(trayIcon); at the end of the test, otherwise it can hang, when will be run w/o jtreg. On 03.03.2015 15:32, Semyon Sadetsky wrote: accepted. http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote: Just few comments about the test: - The test sets WindowsLookAndFeel and seems fails on non Windows platforms. - There is the second SystemTray.isSupported() check on line 49. Does it depends on LF? - The copyright should be updated to 2015. Thanks, Alexandr. On 3/2/2015 4:01 PM, Semyon Sadetsky wrote: Hello, Test was added. Please review. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ bug: https://bugs.openjdk.java.net/browse/JDK-8072769 On 2/26/2015 10:46 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ On 2/24/2015 12:12 PM, Semyon Sadetsky wrote: Hello, AWT Dev [9] review request for 8061636: Fix for JDK-7079254 changes behavior of MouseListener, MouseMotionListener please review the fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769 System tray icon title freezes java This fix contains: fix corner case: buffer size == length of the string for two string parameters: 1. balloon title string 2. balloon text string -- Thanks, Semyon. -- Best regards, Sergey. -- Best regards, Sergey.
Re: AWT Dev Swing Dev [9] Review Request for 8074092: Newly introduced unnecessary dependencies on internal API in client regtests
Yes, Andrei, you are right. Starting from the next time (perhaps it will be a bulk test update for Jigsaw) we'll use this notation. Thank you! -yan On 03/04/2015 06:58 PM, andrei.eremeev wrote: Hi, @library ../../regtesthelpers @library ../../../../lib/testlibrary can be replaced with @library /javax/swing/regtesthelper @library /lib/testlibrary Library path is concatenated to the directory in which TEST.ROOT is located. Andrei Eremeev On 03/04/2015 04:28 PM, Yuri Nesterenko wrote: Hi! Please review this fix for several regression tests. Webrev: http://cr.openjdk.java.net/~yan/8074092/webrev.00 CR: https://bugs.openjdk.java.net/browse/JDK-8074092 In bug8062561.java test an error message also changed. Verified on 3 platforms. Thanks, -yan
Re: AWT Dev [9] review request for 8072769: System tray icon title freezes java
Hi, Semyon. I suggest to remove stuff related to windows platform from the test. Also it will be good to test some reasonable range of data instead of magic constant, wrap setTooltip/displayMessage in a loop. Please add tray.remove(trayIcon); at the end of the test, otherwise it can hang, when will be run w/o jtreg. On 03.03.2015 15:32, Semyon Sadetsky wrote: accepted. http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote: Just few comments about the test: - The test sets WindowsLookAndFeel and seems fails on non Windows platforms. - There is the second SystemTray.isSupported() check on line 49. Does it depends on LF? - The copyright should be updated to 2015. Thanks, Alexandr. On 3/2/2015 4:01 PM, Semyon Sadetsky wrote: Hello, Test was added. Please review. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ bug: https://bugs.openjdk.java.net/browse/JDK-8072769 On 2/26/2015 10:46 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ On 2/24/2015 12:12 PM, Semyon Sadetsky wrote: Hello, AWT Dev [9] review request for 8061636: Fix for JDK-7079254 changes behavior of MouseListener, MouseMotionListener please review the fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769 System tray icon title freezes java This fix contains: fix corner case: buffer size == length of the string for two string parameters: 1. balloon title string 2. balloon text string -- Thanks, Semyon. -- Best regards, Sergey.
Re: AWT Dev [9] review request for 8072769: System tray icon title freezes java
Thanks, Sergey! tray.remove at the end is accepted. Below is about your proposal to run reg test on all other platforms and scan parameters lengths: -- I think it's more like bug regression test policy. Since I'm a novice in the project maybe you or somebody can advise me the right process. Lets see what we have here: The fix is connected to the windows platform native code only. It is a platform specific corner case. I would not say it contains magic numbers, those numbers a not magic for the native layer. But in the test we call native code from java where in truth string parameters maximum lengths are not specified. I think it make sense to run the test on all other platforms test iterating strings lengths up to 1000 or more just to ensure that there no similar issues there. We can do this once or have an option to switch this scenario on. But I'm not 100% sure that such test scenario should be included in the regular regression run because sending messages to the system tray is not very fast and the scan can take noticeable amount of time. In my opinion such issue seeking scenario is reasonable to run time to time or as a part of stress testing profile, but it does not make sense to run it during the regular regression because it tests nothing specific on platforms other than Windows and even in Windows there is only one specific lengths combination. It's not worth to do this potential issue seek it's time expensive but will not bring us a lot of value really. --Semyon On 3/4/2015 2:13 PM, Sergey Bylokhov wrote: Hi, Semyon. I suggest to remove stuff related to windows platform from the test. Also it will be good to test some reasonable range of data instead of magic constant, wrap setTooltip/displayMessage in a loop. Please add tray.remove(trayIcon); at the end of the test, otherwise it can hang, when will be run w/o jtreg. On 03.03.2015 15:32, Semyon Sadetsky wrote: accepted. http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote: Just few comments about the test: - The test sets WindowsLookAndFeel and seems fails on non Windows platforms. - There is the second SystemTray.isSupported() check on line 49. Does it depends on LF? - The copyright should be updated to 2015. Thanks, Alexandr. On 3/2/2015 4:01 PM, Semyon Sadetsky wrote: Hello, Test was added. Please review. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ bug: https://bugs.openjdk.java.net/browse/JDK-8072769 On 2/26/2015 10:46 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ On 2/24/2015 12:12 PM, Semyon Sadetsky wrote: Hello, AWT Dev [9] review request for 8061636: Fix for JDK-7079254 changes behavior of MouseListener, MouseMotionListener please review the fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769 System tray icon title freezes java This fix contains: fix corner case: buffer size == length of the string for two string parameters: 1. balloon title string 2. balloon text string -- Thanks, Semyon.
Re: AWT Dev [9] Review request for 8056151: Switching to GTK LF on-the-fly leads to X Window System error RenderBadPicture
Hi,Alexander. The fix looks fine. On 02.03.2015 18:47, Alexander Zvegintsev wrote: Hello, please review the fix http://cr.openjdk.java.net/~azvegint/jdk/9/8056151/00/ for the issue https://bugs.openjdk.java.net/browse/JDK-8056151 There are several prerequisites for this crash: enabled XRender pipeline, visible window with non-opaque background with LF other than GTK. Switching to GTK LF leads to load_gtk() call. We crashing on gtk_init_check() call [0]. It invokes some XLib functions internally. Hence, wrap it with awtLock is enough to fix the crash. This fix also removes some unnecessary unboxing. [0] http://hg.openjdk.java.net/jdk9/client/jdk/file/3850097b0fe9/src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c#l925 -- Best regards, Sergey.
Re: AWT Dev [9] review request for 8072769: System tray icon title freezes java
On 04.03.2015 15:08, Semyon Sadetsky wrote: I think it's more like bug regression test policy. Since I'm a novice in the project maybe you or somebody can advise me the right process. Lets see what we have here: The fix is connected to the windows platform native code only. It is a platform specific corner case. I would not say it contains magic numbers, those numbers a not magic for the native layer. But in the test we call native code from java where in truth string parameters maximum lengths are not specified. I think it make sense to run the test on all other platforms test iterating strings lengths up to 1000 or more just to ensure that there no similar issues there. We can do this once or have an option to switch this scenario on. The goal of the test is to catch all possibly related issues. So instead of numbers use ranges, cover all platforms if the test don't use platform's specific classes, cover all look and feels, automatic test is better than manual, make the test generic. But it has of course some restrictions like performance and stability. Moreover it seems that right now displayMessage/setToolTip are used in the manual tests only. But I'm not 100% sure that such test scenario should be included in the regular regression run because sending messages to the system tray is not very fast and the scan can take noticeable amount of time. In my opinion such issue seeking scenario is reasonable to run time to time or as a part of stress testing profile, but it does not make sense to run it during the regular regression because it tests nothing specific on platforms other than Windows and even in Windows there is only one specific lengths combination. It's not worth to do this potential issue seek it's time expensive but will not bring us a lot of value really. --Semyon On 3/4/2015 2:13 PM, Sergey Bylokhov wrote: Hi, Semyon. I suggest to remove stuff related to windows platform from the test. Also it will be good to test some reasonable range of data instead of magic constant, wrap setTooltip/displayMessage in a loop. Please add tray.remove(trayIcon); at the end of the test, otherwise it can hang, when will be run w/o jtreg. On 03.03.2015 15:32, Semyon Sadetsky wrote: accepted. http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote: Just few comments about the test: - The test sets WindowsLookAndFeel and seems fails on non Windows platforms. - There is the second SystemTray.isSupported() check on line 49. Does it depends on LF? - The copyright should be updated to 2015. Thanks, Alexandr. On 3/2/2015 4:01 PM, Semyon Sadetsky wrote: Hello, Test was added. Please review. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ bug: https://bugs.openjdk.java.net/browse/JDK-8072769 On 2/26/2015 10:46 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ On 2/24/2015 12:12 PM, Semyon Sadetsky wrote: Hello, AWT Dev [9] review request for 8061636: Fix for JDK-7079254 changes behavior of MouseListener, MouseMotionListener please review the fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769 System tray icon title freezes java This fix contains: fix corner case: buffer size == length of the string for two string parameters: 1. balloon title string 2. balloon text string -- Thanks, Semyon. -- Best regards, Sergey.
Re: AWT Dev [9] review request for 8066436: Minimize can cause window to disappear on osx
Hi, Nakul. Thanks for the fix. A few notes: - Please remove the bugid information from the comment since we have mercurial history for such notes. - I suppose that the comment can be shorter, like this: We need an up to date size of the peer, so we flush the native events to be sure that there are no setBounds requests in the queue. - Can you provide a test case for this bug, even if you cannot reproduce it on your local system, you can write a test, which will fail on the users system. On 02.03.2015 22:18, Nakul Natu wrote: Hello, Please review the fix for JDK 9. CPlatformWindow set bounds using setBounds() which calls native function nativeNSSetWindowBounds() - calls the OSX function. After that ox callbacks to java using deliverMoveresizeEvent() where the peer or window associated with native window is notified of change of bounds. In maximize() we need peer’s bound which are saved as normalBounds to use them to unmaximize. The problem 1. Customer creates window where the size width height is (1, 1) 2. setSize call is made (600,900) which starts the setBounds process described earlier 3. setExtendedState(MAXIMIZED) is called which calls maximize(). But the callback for deliverMoveresizeEvent() has not yet processed and the the bounds we get are earlier (1,1). 4. Window is maximized. 5. Now when we try to unmaximize or iconify the window the size is set to (1,1) and we can’t see the window. Even if we are using the bounds which are available (as the event processing is asynchronous) this can cause lot of problems as the window just disappears. Adding LWCToolkit.flushNativeSelectors(); just before saving the bounds can get us the correct bounds which we are expecting. Bug : https://bugs.openjdk.java.net/browse/JDK-8066436 Webrev: http://cr.openjdk.java.net/~van/8066436/webrev.00/ Regards, Nakul -- Best regards, Sergey.
AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
When building the native code in the jdk repo, a lot of warnings are generated. So many that it's hard to spot any new issues. While the ultimate goal must be to address and fix these warnings individually, this bug is about disabling these warnings where they occur, so that it is easier to spot new warnings, and that the existing warnings can be addressed one at a time. Disabling warnings instead of fixing them can be problematic. If you are too broad with disabling warnings, you might hide future problems. On the other hand, there have been a lot of warnings that indicate serious problems that has been hidden in plain sight for a very long time in the code base anyway. I have opted to disable warnings at the library level. Disabling warnings globally would be too broad. Disabling per file would have been too tedious. Many warnings also tend to repeat across multiple files in the same library, due to e.g. less well-suited programming style or design choice. A library also seems like a suitable chunk of work to address later on, in trying to get rid of the source of the warnings. This fix will not get rid of all warnings, but the bulk of them. It will make the remaining warnings stick out more. The few warnings that remain are either: * Not possible to disable. * Not resulting from native compilation (but from linking, or javadoc, etc). * Not possible to disable with this framework (this goes for builds on pre-4.4 gcc, which Oracle currently does as default on macosx), and libfontmanager on Solaris, which mixes C and C++ code. (While the solstudio C compiler does not fail on requests to disable C++ warnings, the converse is unfortunately not true). It did not seem worth the trouble to build a more extensible framework to handle this, compared to actually fixing these warnings. Note that the current JPRT build environment in Oracle uses a pre-4.4 gcc for macosx by default, so the default macosx build will still contain warnings. When building with --with-toolchain-type=clang, the clang warning disabling kicks in. (This will be the default in JPRT later on this year.) I intend to file individual bugs to handle these remaining warnings, so the net result will be a completely warning free build. I also intend to file individual bugs on all the libraries that has had warnings disabled. I expect the outcome of these bugs to be either: A) The code modified so it does not trigger warnings, and the warnings re-enabled, or B) The warnings (or a subset of them) kept disabled, but a comment added to the makefile describing why this is the proper course of action. Not all bugs might be worth fixing. For instance, the GCC warnings type-limits, sign-compare, pointer-to-int-cast, conversion-null, deprecated-declarations, clobbered, int-to-pointer-cast and type-limits are all more or less benign, and is possibly the result of a false positive. On the other hands, warnings such as format-nonliteral, unused-result, maybe-uninitialized, format, format-security, int-to-pointer-cast, reorder and delete-non-virtual-dtor are more likely to actually point to real issues. I recommend anyone finding these warnings on a library they care about to try and fix them as soon as possible. Here is a summary of the libraries and binaries that have gotten warnings disabled. I'm not sure about what group owns all these libraries; if I missed sending this mail to the correct list, please help me and forward it. * libunpack * libfdlibm * libverify * libjava * libzip * libjli/libjli_static * libj2gss * libsunec * libj2pkcs11 * libnet * libnio * libosxkrb5 * libosxapp * libosx * libapplescriptengine * libjsound * libjsoundalsa * libmlib_image * libawt * libawt_xawt * libawt_lwawt * liblcms * libjavajpeg * libawt_headless * libfontmanager * libsplashscreen * unpack200 * The JVMTI demos compiledMethodLoad and waiters Bug: https://bugs.openjdk.java.net/browse/JDK-8074096 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8074096-disable-native-warnings/webrev.01 /Magnus
Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
Hello, Really nice to finally see this patch getting done! Only one comment: flags.m4: In the grep expression, could you move the extra [] outside of the actual command line options to grep so that the command line could be copied to the shell for debugging in the future? Also, how hard would it be to instead do AC_COMPILE_IFELSE with a dummy warning to instead test for capability? /Erik On 2015-03-04 14:17, Magnus Ihse Bursie wrote: When building the native code in the jdk repo, a lot of warnings are generated. So many that it's hard to spot any new issues. While the ultimate goal must be to address and fix these warnings individually, this bug is about disabling these warnings where they occur, so that it is easier to spot new warnings, and that the existing warnings can be addressed one at a time. Disabling warnings instead of fixing them can be problematic. If you are too broad with disabling warnings, you might hide future problems. On the other hand, there have been a lot of warnings that indicate serious problems that has been hidden in plain sight for a very long time in the code base anyway. I have opted to disable warnings at the library level. Disabling warnings globally would be too broad. Disabling per file would have been too tedious. Many warnings also tend to repeat across multiple files in the same library, due to e.g. less well-suited programming style or design choice. A library also seems like a suitable chunk of work to address later on, in trying to get rid of the source of the warnings. This fix will not get rid of all warnings, but the bulk of them. It will make the remaining warnings stick out more. The few warnings that remain are either: * Not possible to disable. * Not resulting from native compilation (but from linking, or javadoc, etc). * Not possible to disable with this framework (this goes for builds on pre-4.4 gcc, which Oracle currently does as default on macosx), and libfontmanager on Solaris, which mixes C and C++ code. (While the solstudio C compiler does not fail on requests to disable C++ warnings, the converse is unfortunately not true). It did not seem worth the trouble to build a more extensible framework to handle this, compared to actually fixing these warnings. Note that the current JPRT build environment in Oracle uses a pre-4.4 gcc for macosx by default, so the default macosx build will still contain warnings. When building with --with-toolchain-type=clang, the clang warning disabling kicks in. (This will be the default in JPRT later on this year.) I intend to file individual bugs to handle these remaining warnings, so the net result will be a completely warning free build. I also intend to file individual bugs on all the libraries that has had warnings disabled. I expect the outcome of these bugs to be either: A) The code modified so it does not trigger warnings, and the warnings re-enabled, or B) The warnings (or a subset of them) kept disabled, but a comment added to the makefile describing why this is the proper course of action. Not all bugs might be worth fixing. For instance, the GCC warnings type-limits, sign-compare, pointer-to-int-cast, conversion-null, deprecated-declarations, clobbered, int-to-pointer-cast and type-limits are all more or less benign, and is possibly the result of a false positive. On the other hands, warnings such as format-nonliteral, unused-result, maybe-uninitialized, format, format-security, int-to-pointer-cast, reorder and delete-non-virtual-dtor are more likely to actually point to real issues. I recommend anyone finding these warnings on a library they care about to try and fix them as soon as possible. Here is a summary of the libraries and binaries that have gotten warnings disabled. I'm not sure about what group owns all these libraries; if I missed sending this mail to the correct list, please help me and forward it. * libunpack * libfdlibm * libverify * libjava * libzip * libjli/libjli_static * libj2gss * libsunec * libj2pkcs11 * libnet * libnio * libosxkrb5 * libosxapp * libosx * libapplescriptengine * libjsound * libjsoundalsa * libmlib_image * libawt * libawt_xawt * libawt_lwawt * liblcms * libjavajpeg * libawt_headless * libfontmanager * libsplashscreen * unpack200 * The JVMTI demos compiledMethodLoad and waiters Bug: https://bugs.openjdk.java.net/browse/JDK-8074096 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8074096-disable-native-warnings/webrev.01 /Magnus
Re: AWT Dev [9] review request for 8072769: System tray icon title freezes java
Sergey, OK. You can file a request to improve TrayIcon test coverage but it's an another story. Here we are discussing regression test for a specific bug. I cannot agree with the approach you've proposed. In your logic we need start to write regression tests for all methods in JDK classes those take String parameters by iterating all possible parameters lengths to discover potential bugs. Tests need to be written by certain rules. Scanning all possible values of input parameters is not the best rule in my opinion. --Semyon On 3/4/2015 4:03 PM, Sergey Bylokhov wrote: On 04.03.2015 15:08, Semyon Sadetsky wrote: I think it's more like bug regression test policy. Since I'm a novice in the project maybe you or somebody can advise me the right process. Lets see what we have here: The fix is connected to the windows platform native code only. It is a platform specific corner case. I would not say it contains magic numbers, those numbers a not magic for the native layer. But in the test we call native code from java where in truth string parameters maximum lengths are not specified. I think it make sense to run the test on all other platforms test iterating strings lengths up to 1000 or more just to ensure that there no similar issues there. We can do this once or have an option to switch this scenario on. The goal of the test is to catch all possibly related issues. So instead of numbers use ranges, cover all platforms if the test don't use platform's specific classes, cover all look and feels, automatic test is better than manual, make the test generic. But it has of course some restrictions like performance and stability. Moreover it seems that right now displayMessage/setToolTip are used in the manual tests only. But I'm not 100% sure that such test scenario should be included in the regular regression run because sending messages to the system tray is not very fast and the scan can take noticeable amount of time. In my opinion such issue seeking scenario is reasonable to run time to time or as a part of stress testing profile, but it does not make sense to run it during the regular regression because it tests nothing specific on platforms other than Windows and even in Windows there is only one specific lengths combination. It's not worth to do this potential issue seek it's time expensive but will not bring us a lot of value really. --Semyon On 3/4/2015 2:13 PM, Sergey Bylokhov wrote: Hi, Semyon. I suggest to remove stuff related to windows platform from the test. Also it will be good to test some reasonable range of data instead of magic constant, wrap setTooltip/displayMessage in a loop. Please add tray.remove(trayIcon); at the end of the test, otherwise it can hang, when will be run w/o jtreg. On 03.03.2015 15:32, Semyon Sadetsky wrote: accepted. http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote: Just few comments about the test: - The test sets WindowsLookAndFeel and seems fails on non Windows platforms. - There is the second SystemTray.isSupported() check on line 49. Does it depends on LF? - The copyright should be updated to 2015. Thanks, Alexandr. On 3/2/2015 4:01 PM, Semyon Sadetsky wrote: Hello, Test was added. Please review. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ bug: https://bugs.openjdk.java.net/browse/JDK-8072769 On 2/26/2015 10:46 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ On 2/24/2015 12:12 PM, Semyon Sadetsky wrote: Hello, AWT Dev [9] review request for 8061636: Fix for JDK-7079254 changes behavior of MouseListener, MouseMotionListener please review the fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769 System tray icon title freezes java This fix contains: fix corner case: buffer size == length of the string for two string parameters: 1. balloon title string 2. balloon text string -- Thanks, Semyon. -- Best regards, Sergey.
Re: AWT Dev [9] Review Request: 8074028 Remove API references to java.awt.peer
On 04/03/2015 12:37, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. There are a number of public API whichreference the unsupported java.awt.peer interfaces. public java.awt.Component.getPeer() @deprecated 1.1 public java.awt.Font.getPeer() @deprecated 1.1 public java.awt.MenuComponent.getPeer() @deprecated 1.1 I don't know this area so I don't feel qualified to do a thorough review of the changes. However I welcome the changes, this is long standing technical debt and important that we address this as part of the move to modules. I note that Font::getPeer is changed to private, hence something try to access it outside of the Font class will get an IllegalAccessError as opposed to NoSuchMethodError for the outright removal in the other cases. Did you consider renaming to something else, say private FontPeer getFontPeer so that it is consistent with the removal of the other two methods? I note that you might not have caught all tests but I would assume that any missed tests should fail. Perhaps there are other failures that are potentially hiding tests using getPeer? -Alan
Re: AWT Dev [9] Review Request for 8074092: Newly introduced unnecessary dependencies on internal API in client regtests
Hi, Yuri. The fix looks good. On 04.03.2015 16:28, Yuri Nesterenko wrote: Hi! Please review this fix for several regression tests. Webrev: http://cr.openjdk.java.net/~yan/8074092/webrev.00 CR: https://bugs.openjdk.java.net/browse/JDK-8074092 In bug8062561.java test an error message also changed. Verified on 3 platforms. Thanks, -yan -- Best regards, Sergey.
Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
On 04/03/2015 13:17, Magnus Ihse Bursie wrote: : I intend to file individual bugs to handle these remaining warnings, so the net result will be a completely warning free build. I also intend to file individual bugs on all the libraries that has had warnings disabled. I expect the outcome of these bugs to be either: A) The code modified so it does not trigger warnings, and the warnings re-enabled, or B) The warnings (or a subset of them) kept disabled, but a comment added to the makefile describing why this is the proper course of action. Not all bugs might be worth fixing. For instance, the GCC warnings type-limits, sign-compare, pointer-to-int-cast, conversion-null, deprecated-declarations, clobbered, int-to-pointer-cast and type-limits are all more or less benign, and is possibly the result of a false positive. Right, although for some of these it is important to double check. Do you plan to paste in the warnings into the bugs that you will create? That wold be useful as warnings are a moving target. -Alan
AWT Dev [9] Review Request: 8074028 Remove API references to java.awt.peer
Hello.
Please review the fix for jdk 9.
There are a number of public API whichreference the unsupported
java.awt.peer interfaces.
public java.awt.Component.getPeer() @deprecated 1.1
public java.awt.Font.getPeer() @deprecated 1.1
public java.awt.MenuComponent.getPeer() @deprecated 1.1
There is a decision to remove this methods as described:
http://mail.openjdk.java.net/pipermail/awt-dev/2015-February/008924.html
Most important changes:
http://cr.openjdk.java.net/~serb/8074028/webrev.05/src/java.desktop/share/classes/java/awt/Component.java.sdiff.html
-/**
- * @deprecated As of JDK version 1.1,
- * programs should not directly manipulate peers;
- * replaced by codeboolean isDisplayable()/code.
- * @return the peer for this component
- */
-@Deprecated
-public ComponentPeer getPeer() {
-return peer;
-}
Component.getPeer was removed and its usage was replaced by:
- dirrect access to the field if possible
- (getPeer() !=/== null) is replaced by the isDisplayable()
- All other places now use an AWTAccessor.
http://cr.openjdk.java.net/~serb/8074028/webrev.05/src/java.desktop/share/classes/java/awt/MenuComponent.java.sdiff.html
-/**
- * @deprecated As of JDK version 1.1,
- * programs should not directly manipulate peers.
- * @return the peer for this component
- */
-@Deprecated
-public MenuComponentPeer getPeer() {
-return peer;
-}
MenuComponentPeer.getPeer was removed, now we use an AWTAccessor instead.
http://cr.openjdk.java.net/~serb/8074028/webrev.05/src/java.desktop/share/classes/java/awt/Font.java.sdiff.html
Font.getPeer() access was changed to the private, because it is not
simple getter, and it is used in native code. On the java side now we
use an AWTAccessor.
Also I tried to fix the open tests, but not sure that I cover all of
them. I'll fix other tests in the separate CR.
Bug: https://bugs.openjdk.java.net/browse/JDK-8074028
Webrev can be found at: http://cr.openjdk.java.net/~serb/8074028/webrev.05
--
Best regards, Sergey.
AWT Dev [9] Review Request for 8074092: Newly introduced unnecessary dependencies on internal API in client regtests
Hi! Please review this fix for several regression tests. Webrev: http://cr.openjdk.java.net/~yan/8074092/webrev.00 CR: https://bugs.openjdk.java.net/browse/JDK-8074092 In bug8062561.java test an error message also changed. Verified on 3 platforms. Thanks, -yan
Re: AWT Dev [9] Review Request: 8074028 Remove API references to java.awt.peer
On 04.03.2015 15:51, Alan Bateman wrote: On 04/03/2015 12:37, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. There are a number of public API whichreference the unsupported java.awt.peer interfaces. public java.awt.Component.getPeer() @deprecated 1.1 public java.awt.Font.getPeer() @deprecated 1.1 public java.awt.MenuComponent.getPeer() @deprecated 1.1 I don't know this area so I don't feel qualified to do a thorough review of the changes. However I welcome the changes, this is long standing technical debt and important that we address this as part of the move to modules. I note that Font::getPeer is changed to private, hence something try to access it outside of the Font class will get an IllegalAccessError as opposed to NoSuchMethodError for the outright removal in the other cases. Did you consider renaming to something else, say private FontPeer getFontPeer so that it is consistent with the removal of the other two methods? Make sense, I will update the fix. I note that you might not have caught all tests but I would assume that any missed tests should fail. Perhaps there are other failures that are potentially hiding tests using getPeer? At least I fixed most(if not all) tests, which were passed before the fix and fails after -Alan -- Best regards, Sergey.
Re: AWT Dev [9] review request for 8072769: System tray icon title freezes java
the updated webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.04/ On 3/4/2015 4:33 PM, Semyon Sadetsky wrote: Sergey, OK. You can file a request to improve TrayIcon test coverage but it's an another story. Here we are discussing regression test for a specific bug. I cannot agree with the approach you've proposed. In your logic we need start to write regression tests for all methods in JDK classes those take String parameters by iterating all possible parameters lengths to discover potential bugs. Tests need to be written by certain rules. Scanning all possible values of input parameters is not the best rule in my opinion. --Semyon On 3/4/2015 4:03 PM, Sergey Bylokhov wrote: On 04.03.2015 15:08, Semyon Sadetsky wrote: I think it's more like bug regression test policy. Since I'm a novice in the project maybe you or somebody can advise me the right process. Lets see what we have here: The fix is connected to the windows platform native code only. It is a platform specific corner case. I would not say it contains magic numbers, those numbers a not magic for the native layer. But in the test we call native code from java where in truth string parameters maximum lengths are not specified. I think it make sense to run the test on all other platforms test iterating strings lengths up to 1000 or more just to ensure that there no similar issues there. We can do this once or have an option to switch this scenario on. The goal of the test is to catch all possibly related issues. So instead of numbers use ranges, cover all platforms if the test don't use platform's specific classes, cover all look and feels, automatic test is better than manual, make the test generic. But it has of course some restrictions like performance and stability. Moreover it seems that right now displayMessage/setToolTip are used in the manual tests only. But I'm not 100% sure that such test scenario should be included in the regular regression run because sending messages to the system tray is not very fast and the scan can take noticeable amount of time. In my opinion such issue seeking scenario is reasonable to run time to time or as a part of stress testing profile, but it does not make sense to run it during the regular regression because it tests nothing specific on platforms other than Windows and even in Windows there is only one specific lengths combination. It's not worth to do this potential issue seek it's time expensive but will not bring us a lot of value really. --Semyon On 3/4/2015 2:13 PM, Sergey Bylokhov wrote: Hi, Semyon. I suggest to remove stuff related to windows platform from the test. Also it will be good to test some reasonable range of data instead of magic constant, wrap setTooltip/displayMessage in a loop. Please add tray.remove(trayIcon); at the end of the test, otherwise it can hang, when will be run w/o jtreg. On 03.03.2015 15:32, Semyon Sadetsky wrote: accepted. http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote: Just few comments about the test: - The test sets WindowsLookAndFeel and seems fails on non Windows platforms. - There is the second SystemTray.isSupported() check on line 49. Does it depends on LF? - The copyright should be updated to 2015. Thanks, Alexandr. On 3/2/2015 4:01 PM, Semyon Sadetsky wrote: Hello, Test was added. Please review. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ bug: https://bugs.openjdk.java.net/browse/JDK-8072769 On 2/26/2015 10:46 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ On 2/24/2015 12:12 PM, Semyon Sadetsky wrote: Hello, AWT Dev [9] review request for 8061636: Fix for JDK-7079254 changes behavior of MouseListener, MouseMotionListener please review the fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769 System tray icon title freezes java This fix contains: fix corner case: buffer size == length of the string for two string parameters: 1. balloon title string 2. balloon text string -- Thanks, Semyon. -- Best regards, Sergey.
Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis
On 2015-03-04 14:48, Alan Bateman wrote: On 04/03/2015 13:17, Magnus Ihse Bursie wrote: : I intend to file individual bugs to handle these remaining warnings, so the net result will be a completely warning free build. I also intend to file individual bugs on all the libraries that has had warnings disabled. I expect the outcome of these bugs to be either: A) The code modified so it does not trigger warnings, and the warnings re-enabled, or B) The warnings (or a subset of them) kept disabled, but a comment added to the makefile describing why this is the proper course of action. Not all bugs might be worth fixing. For instance, the GCC warnings type-limits, sign-compare, pointer-to-int-cast, conversion-null, deprecated-declarations, clobbered, int-to-pointer-cast and type-limits are all more or less benign, and is possibly the result of a false positive. Right, although for some of these it is important to double check. I believe all warnings are important to check! Unfortunately, this has not been done for a lot of warnings for a lot of time. :( With this framework, it is simple to enable a single warning, recompile and see the effect. Hopefully this lowers the threshold far enough so that all warnings are given proper attention. The incremental build functionality will come in very handy. Just by simply removing a warning from the DISABLED_WARNINGS_toolchain on a library and running make again, only the files affected will be recompiled. Do you plan to paste in the warnings into the bugs that you will create? That wold be useful as warnings are a moving target. I can easily paste in what warning categories are disabled for a specific library, yes. However, if you are asking me to build each library, individually, with warnings re-enabled, and pasting the output, I'd rather not. That would be a lot of work, to detangle the output of each individual library. /Magnus
