Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
Thanks Alan! On 20 mar 2014, at 19:15, Alan Bateman wrote: > On 20/03/2014 12:11, Staffan Larsen wrote: >> : >> Yes, that one was a bit messy. I cleaned it up and added some comments. Also >> remove the SCCS workarounds. >> >> http://cr.openjdk.java.net/~sla/8037825/webrev.02/ >> >> > It's looks okay and thanks for adding a comment. > > -Alan
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
On 20/03/2014 12:11, Staffan Larsen wrote: : Yes, that one was a bit messy. I cleaned it up and added some comments. Also remove the SCCS workarounds. http://cr.openjdk.java.net/~sla/8037825/webrev.02/ It's looks okay and thanks for adding a comment. -Alan
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
On 20 mar 2014, at 11:25, Alan Bateman wrote: > On 20/03/2014 09:13, Staffan Larsen wrote: >> >> Thanks! >> >> Still need someone to look at the actual code changes. >> >> > In webrev.01 then jdk/make/lib/ServiceabilityLibraries.gmk and > src/solaris/native/sun/management/MacosxOperatingSystem.c are showing up as > new files, I can't tell why that is. I had some trouble creating reviews for multiple repos now that "webrev -f” does not work. The review at http://cr.openjdk.java.net/~sla/8037825/webrev.01/jdk/ is the good one. > The code changes mostly look good to me. The only change that isn't very > clear (I had to stare at it for a few minutes) in log_messages.c where it's > now two calls to strftime. It might be useful to include a comment to explain > this, also maybe reduce the line length to make it easier for future > side-by-side reviews. Yes, that one was a bit messy. I cleaned it up and added some comments. Also remove the SCCS workarounds. http://cr.openjdk.java.net/~sla/8037825/webrev.02/ Thanks, /Staffan
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
On 20/03/2014 09:13, Staffan Larsen wrote: Thanks! Still need someone to look at the actual code changes. In webrev.01 then jdk/make/lib/ServiceabilityLibraries.gmk and src/solaris/native/sun/management/MacosxOperatingSystem.c are showing up as new files, I can't tell why that is. The code changes mostly look good to me. The only change that isn't very clear (I had to stare at it for a few minutes) in log_messages.c where it's now two calls to strftime. It might be useful to include a comment to explain this, also maybe reduce the line length to make it easier for future side-by-side reviews. -Alan.
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
Thanks! Still need someone to look at the actual code changes. /Staffan On 20 mar 2014, at 09:49, Magnus Ihse Bursie wrote: > On 2014-03-19 17:47, Erik Joelsson wrote: >> Build part looks good! > > Build part looks good to me to. > > /Magnus >
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
On 2014-03-19 17:47, Erik Joelsson wrote: Build part looks good! Build part looks good to me to. /Magnus
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
Build part looks good! Don't forget to push the closed generated configure. /Erik On 2014-03-19 15:20, Staffan Larsen wrote: Erik, Magnus, Thanks for the quick looks. Here is an updated version that adds the new variable to flags.m4 instead. http://cr.openjdk.java.net/~sla/8037825/webrev.01/root/ http://cr.openjdk.java.net/~sla/8037825/webrev.01/jdk/ Thanks, /Staffan On 19 mar 2014, at 14:03, Erik Joelsson wrote: Hello, Nice work! Removing warnings and enforcing them is definitely something we like. For the makefile change, ideally the new variable WARNINGS_ARE_ERRORS should be set in configure. I would suggest flags.m4, in FLAGS_SETUP_COMPILER_FLAGS_MISC. Also, instead of making the conditional on OS, it should be on the new variable TOOLCHAIN_TYPE. Perhaps the variable name should contain the word CFLAGS somewhere to make it more obvious what it applies to. /Erik On 2014-03-19 13:30, Staffan Larsen wrote: The serviceability libraries (as defined by jdk/make/lib/ServiceabilityLibraries.gmk) should be compiled with "warnings as errors". To enable this all current warnings need to be fixed. The background for this is that we recently had a bug that could have easily been avoided if we had paid attention to the warnings. webrev: http://cr.openjdk.java.net/~sla/8037825/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8037825 Thanks, /Staffan Here is a list of warnings that I have fixed: linux-i586 jdk/src/share/back/eventHandler.c: In function 'eventHandler_createPermanentInternal': jdk/src/share/back/eventHandler.c:1685: error: cast from pointer to integer of different size jdk/src/share/back/eventHandler.c: In function 'eventHandler_createInternalThreadOnly': jdk/src/share/back/eventHandler.c:1694: error: cast from pointer to integer of different size jdk/src/share/back/inStream.c: In function 'inStream_readLong': jdk/src/share/back/inStream.c:147: error: integer constant is too large for 'long' type macosx jdk/src/share/back/SDE.c:51:1: warning: "true" redefined jdk/src/share/back/SDE.c:52:1: warning: "false" redefined jdk/src/share/back/log_messages.c: In function ‘get_time_stamp’: jdk/src/share/back/log_messages.c:69: warning: format not a string literal, argument types not checked jdk/src/share/back/log_messages.c: In function ‘log_message_end’: jdk/src/share/back/log_messages.c:174: warning: cast from pointer to integer of different size jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c: In function 'Java_sun_management_OperatingSystemImpl_getProcessCpuLoad0': jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c:136: warning: implicit declaration of function 'JVM_ActiveProcessorCount' windows jdk/src/share/back/error_messages.c(328) : warning C4996: '_sleep': This function or variable has been superceded by newer library or operating system functionality. Consider using Sleep instead. See online help for details. jdk/src/windows/back/linker_md.c(62) : warning C4013: 'free' undefined; assuming extern returning int jdk/src/share/instrument/PathCharsValidator.c(49) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(62) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(179) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data solaris-x64 jdk/src/share/back/inStream.c, line 147: constant promoted to unsigned long long
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
On 19 mar 2014, at 17:47, Erik Joelsson wrote: > Build part looks good! Thanks. > > Don't forget to push the closed generated configure. I would have forgotten… /Staffan > > /Erik > > On 2014-03-19 15:20, Staffan Larsen wrote: >> Erik, Magnus, >> >> Thanks for the quick looks. Here is an updated version that adds the new >> variable to flags.m4 instead. >> >> http://cr.openjdk.java.net/~sla/8037825/webrev.01/root/ >> http://cr.openjdk.java.net/~sla/8037825/webrev.01/jdk/ >> >> Thanks, >> /Staffan >> >> On 19 mar 2014, at 14:03, Erik Joelsson wrote: >> >>> Hello, >>> >>> Nice work! Removing warnings and enforcing them is definitely something we >>> like. >>> >>> For the makefile change, ideally the new variable WARNINGS_ARE_ERRORS >>> should be set in configure. I would suggest flags.m4, in >>> FLAGS_SETUP_COMPILER_FLAGS_MISC. Also, instead of making the conditional on >>> OS, it should be on the new variable TOOLCHAIN_TYPE. Perhaps the variable >>> name should contain the word CFLAGS somewhere to make it more obvious what >>> it applies to. >>> >>> /Erik >>> >>> On 2014-03-19 13:30, Staffan Larsen wrote: The serviceability libraries (as defined by jdk/make/lib/ServiceabilityLibraries.gmk) should be compiled with "warnings as errors". To enable this all current warnings need to be fixed. The background for this is that we recently had a bug that could have easily been avoided if we had paid attention to the warnings. webrev: http://cr.openjdk.java.net/~sla/8037825/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8037825 Thanks, /Staffan Here is a list of warnings that I have fixed: linux-i586 jdk/src/share/back/eventHandler.c: In function 'eventHandler_createPermanentInternal': jdk/src/share/back/eventHandler.c:1685: error: cast from pointer to integer of different size jdk/src/share/back/eventHandler.c: In function 'eventHandler_createInternalThreadOnly': jdk/src/share/back/eventHandler.c:1694: error: cast from pointer to integer of different size jdk/src/share/back/inStream.c: In function 'inStream_readLong': jdk/src/share/back/inStream.c:147: error: integer constant is too large for 'long' type macosx jdk/src/share/back/SDE.c:51:1: warning: "true" redefined jdk/src/share/back/SDE.c:52:1: warning: "false" redefined jdk/src/share/back/log_messages.c: In function ‘get_time_stamp’: jdk/src/share/back/log_messages.c:69: warning: format not a string literal, argument types not checked jdk/src/share/back/log_messages.c: In function ‘log_message_end’: jdk/src/share/back/log_messages.c:174: warning: cast from pointer to integer of different size jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c: In function 'Java_sun_management_OperatingSystemImpl_getProcessCpuLoad0': jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c:136: warning: implicit declaration of function 'JVM_ActiveProcessorCount' windows jdk/src/share/back/error_messages.c(328) : warning C4996: '_sleep': This function or variable has been superceded by newer library or operating system functionality. Consider using Sleep instead. See online help for details. jdk/src/windows/back/linker_md.c(62) : warning C4013: 'free' undefined; assuming extern returning int jdk/src/share/instrument/PathCharsValidator.c(49) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(62) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(179) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data solaris-x64 jdk/src/share/back/inStream.c, line 147: constant promoted to unsigned long long >
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
Erik, Magnus, Thanks for the quick looks. Here is an updated version that adds the new variable to flags.m4 instead. http://cr.openjdk.java.net/~sla/8037825/webrev.01/root/ http://cr.openjdk.java.net/~sla/8037825/webrev.01/jdk/ Thanks, /Staffan On 19 mar 2014, at 14:03, Erik Joelsson wrote: > Hello, > > Nice work! Removing warnings and enforcing them is definitely something we > like. > > For the makefile change, ideally the new variable WARNINGS_ARE_ERRORS should > be set in configure. I would suggest flags.m4, in > FLAGS_SETUP_COMPILER_FLAGS_MISC. Also, instead of making the conditional on > OS, it should be on the new variable TOOLCHAIN_TYPE. Perhaps the variable > name should contain the word CFLAGS somewhere to make it more obvious what it > applies to. > > /Erik > > On 2014-03-19 13:30, Staffan Larsen wrote: >> The serviceability libraries (as defined by >> jdk/make/lib/ServiceabilityLibraries.gmk) should be compiled with "warnings >> as errors". To enable this all current warnings need to be fixed. >> >> The background for this is that we recently had a bug that could have easily >> been avoided if we had paid attention to the warnings. >> >> webrev: http://cr.openjdk.java.net/~sla/8037825/webrev.00/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8037825 >> >> Thanks, >> /Staffan >> >> >> Here is a list of warnings that I have fixed: >> >> linux-i586 >> >> jdk/src/share/back/eventHandler.c: In function >> 'eventHandler_createPermanentInternal': >> jdk/src/share/back/eventHandler.c:1685: error: cast from pointer to integer >> of different size >> jdk/src/share/back/eventHandler.c: In function >> 'eventHandler_createInternalThreadOnly': >> jdk/src/share/back/eventHandler.c:1694: error: cast from pointer to integer >> of different size >> >> jdk/src/share/back/inStream.c: In function 'inStream_readLong': >> jdk/src/share/back/inStream.c:147: error: integer constant is too large for >> 'long' type >> >> >> macosx >> >> jdk/src/share/back/SDE.c:51:1: warning: "true" redefined >> jdk/src/share/back/SDE.c:52:1: warning: "false" redefined >> >> jdk/src/share/back/log_messages.c: In function ‘get_time_stamp’: >> jdk/src/share/back/log_messages.c:69: warning: format not a string literal, >> argument types not checked >> jdk/src/share/back/log_messages.c: In function ‘log_message_end’: >> jdk/src/share/back/log_messages.c:174: warning: cast from pointer to integer >> of different size >> >> jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c: In function >> 'Java_sun_management_OperatingSystemImpl_getProcessCpuLoad0': >> jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c:136: warning: >> implicit declaration of function 'JVM_ActiveProcessorCount' >> >> >> windows >> >> jdk/src/share/back/error_messages.c(328) : warning C4996: '_sleep': This >> function or variable has been superceded by newer library or operating >> system functionality. Consider using Sleep instead. See online help for >> details. >> >> jdk/src/windows/back/linker_md.c(62) : warning C4013: 'free' undefined; >> assuming extern returning int >> >> jdk/src/share/instrument/PathCharsValidator.c(49) : warning C4267: >> 'initializing' : conversion from 'size_t' to 'int', possible loss of data >> jdk/src/share/instrument/PathCharsValidator.c(62) : warning C4267: >> 'initializing' : conversion from 'size_t' to 'int', possible loss of data >> jdk/src/share/instrument/PathCharsValidator.c(179) : warning C4267: '=' : >> conversion from 'size_t' to 'int', possible loss of data >> >> >> solaris-x64 >> >> jdk/src/share/back/inStream.c, line 147: constant promoted to unsigned long >> long >
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
Hello, Nice work! Removing warnings and enforcing them is definitely something we like. For the makefile change, ideally the new variable WARNINGS_ARE_ERRORS should be set in configure. I would suggest flags.m4, in FLAGS_SETUP_COMPILER_FLAGS_MISC. Also, instead of making the conditional on OS, it should be on the new variable TOOLCHAIN_TYPE. Perhaps the variable name should contain the word CFLAGS somewhere to make it more obvious what it applies to. /Erik On 2014-03-19 13:30, Staffan Larsen wrote: The serviceability libraries (as defined by jdk/make/lib/ServiceabilityLibraries.gmk) should be compiled with "warnings as errors". To enable this all current warnings need to be fixed. The background for this is that we recently had a bug that could have easily been avoided if we had paid attention to the warnings. webrev: http://cr.openjdk.java.net/~sla/8037825/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8037825 Thanks, /Staffan Here is a list of warnings that I have fixed: linux-i586 jdk/src/share/back/eventHandler.c: In function 'eventHandler_createPermanentInternal': jdk/src/share/back/eventHandler.c:1685: error: cast from pointer to integer of different size jdk/src/share/back/eventHandler.c: In function 'eventHandler_createInternalThreadOnly': jdk/src/share/back/eventHandler.c:1694: error: cast from pointer to integer of different size jdk/src/share/back/inStream.c: In function 'inStream_readLong': jdk/src/share/back/inStream.c:147: error: integer constant is too large for 'long' type macosx jdk/src/share/back/SDE.c:51:1: warning: "true" redefined jdk/src/share/back/SDE.c:52:1: warning: "false" redefined jdk/src/share/back/log_messages.c: In function ‘get_time_stamp’: jdk/src/share/back/log_messages.c:69: warning: format not a string literal, argument types not checked jdk/src/share/back/log_messages.c: In function ‘log_message_end’: jdk/src/share/back/log_messages.c:174: warning: cast from pointer to integer of different size jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c: In function 'Java_sun_management_OperatingSystemImpl_getProcessCpuLoad0': jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c:136: warning: implicit declaration of function 'JVM_ActiveProcessorCount' windows jdk/src/share/back/error_messages.c(328) : warning C4996: '_sleep': This function or variable has been superceded by newer library or operating system functionality. Consider using Sleep instead. See online help for details. jdk/src/windows/back/linker_md.c(62) : warning C4013: 'free' undefined; assuming extern returning int jdk/src/share/instrument/PathCharsValidator.c(49) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(62) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(179) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data solaris-x64 jdk/src/share/back/inStream.c, line 147: constant promoted to unsigned long long
Re: RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
On 2014-03-19 13:30, Staffan Larsen wrote: The serviceability libraries (as defined by jdk/make/lib/ServiceabilityLibraries.gmk) should be compiled with "warnings as errors". To enable this all current warnings need to be fixed. The background for this is that we recently had a bug that could have easily been avoided if we had paid attention to the warnings. webrev: http://cr.openjdk.java.net/~sla/8037825/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8037825 Hi Staffan, I have only looked at the build part of the fix. I have one request: please check for toolchain type instead of operating system, e.g. ifeq ($(TOOLCHAIN_TYPE), microsoft) WARNINGS_ARE_ERRORS := /WX else ifeq ($(TOOLCHAIN_TYPE), solstudio) WARNINGS_ARE_ERRORS := -errtags -errwarn=%all else ifeq ($(TOOLCHAIN_TYPE), gcc) WARNINGS_ARE_ERRORS := -Werror else ifeq ($(TOOLCHAIN_TYPE), clang) WARNINGS_ARE_ERRORS := -Werror endif (with the clang part only if you have tested on clang, that is). Apart from this, it looks good. Thank you for hunting down those warnings! Great job! /Magnus
RFR 8037825 Fix warnings and enable "warnings as errors" in serviceability native libraries
The serviceability libraries (as defined by jdk/make/lib/ServiceabilityLibraries.gmk) should be compiled with "warnings as errors". To enable this all current warnings need to be fixed. The background for this is that we recently had a bug that could have easily been avoided if we had paid attention to the warnings. webrev: http://cr.openjdk.java.net/~sla/8037825/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8037825 Thanks, /Staffan Here is a list of warnings that I have fixed: linux-i586 jdk/src/share/back/eventHandler.c: In function 'eventHandler_createPermanentInternal': jdk/src/share/back/eventHandler.c:1685: error: cast from pointer to integer of different size jdk/src/share/back/eventHandler.c: In function 'eventHandler_createInternalThreadOnly': jdk/src/share/back/eventHandler.c:1694: error: cast from pointer to integer of different size jdk/src/share/back/inStream.c: In function 'inStream_readLong': jdk/src/share/back/inStream.c:147: error: integer constant is too large for 'long' type macosx jdk/src/share/back/SDE.c:51:1: warning: "true" redefined jdk/src/share/back/SDE.c:52:1: warning: "false" redefined jdk/src/share/back/log_messages.c: In function ‘get_time_stamp’: jdk/src/share/back/log_messages.c:69: warning: format not a string literal, argument types not checked jdk/src/share/back/log_messages.c: In function ‘log_message_end’: jdk/src/share/back/log_messages.c:174: warning: cast from pointer to integer of different size jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c: In function 'Java_sun_management_OperatingSystemImpl_getProcessCpuLoad0': jdk/src/solaris/native/sun/management/MacosxOperatingSystem.c:136: warning: implicit declaration of function 'JVM_ActiveProcessorCount' windows jdk/src/share/back/error_messages.c(328) : warning C4996: '_sleep': This function or variable has been superceded by newer library or operating system functionality. Consider using Sleep instead. See online help for details. jdk/src/windows/back/linker_md.c(62) : warning C4013: 'free' undefined; assuming extern returning int jdk/src/share/instrument/PathCharsValidator.c(49) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(62) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data jdk/src/share/instrument/PathCharsValidator.c(179) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data solaris-x64 jdk/src/share/back/inStream.c, line 147: constant promoted to unsigned long long
