Hi Sergey, thanks for checking . @Christoph : copyright years updated in the cpp files . @Phil , I checked the indentation it looked indeed strange in the udiff however in the cpp file itself it looks ok to me .
Thanks for the reviews and best regards, Matthias > -----Original Message----- > From: Phil Race [mailto:philip.r...@oracle.com] > Sent: Dienstag, 5. Juni 2018 23:41 > To: Sergey Bylokhov <sergey.bylok...@oracle.com>; Baesken, Matthias > <matthias.baes...@sap.com>; Langer, Christoph > <christoph.lan...@sap.com>; Thomas Stüfe <thomas.stu...@gmail.com>; > 'build-dev@openjdk.java.net' <build-dev@openjdk.java.net>; awt- > d...@openjdk.java.net > Cc: 2d-dev <2d-...@openjdk.java.net> > Subject: Re: RFR: JDK-8204211: windows : handle potential C++ exception in > GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > > > In that case Matthias is good to go after fixing the indentation. > > -phil. > > On 06/05/2018 01:37 PM, Sergey Bylokhov wrote: > > I have checked the fix using mach5. > > > > On 05/06/2018 12:45, Phil Race wrote: > >> Oh .. can I please ask that you make sure that VS2017 is OK with the > >> re-enabled > >> warning ? I seriously doubt that it has anything new to add over > >> VS2013, but a jdk-submit > >> will tell you if it has .. > >> > >> VS2017 is now the default so a jdk-submit will use that. > >> > >> -phil. > >> > >> On 06/05/2018 12:43 PM, Phil Race wrote: > >>> This looks good to me except for what looks like in my browser like > >>> missing indentation in > >>> > http://cr.openjdk.java.net/~mbaesken/webrevs/8204211.1/src/java.deskto > p/windows/native/libawt/java2d/windows > >>> > >>> /GDIRenderer.cpp.udiff.html > >>> > >>> You can fix that with or without an updated webrev. > >>> > >>> Good to clear a class of warnings. > >>> > >>> -phil. > >>> > >>> On 06/05/2018 12:47 AM, Baesken, Matthias wrote: > >>>> Hi Christoph, thank's for the review . > >>>> Could I have a second one for example from the awt or build-dev > >>>> reviewers ? > >>>> > >>>> Best Regards, Matthias > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Langer, Christoph > >>>>> Sent: Montag, 4. Juni 2018 16:49 > >>>>> To: Baesken, Matthias <matthias.baes...@sap.com>; Thomas Stüfe > >>>>> <thomas.stu...@gmail.com>; 'build-dev@openjdk.java.net' <build- > >>>>> d...@openjdk.java.net>; awt-...@openjdk.java.net > >>>>> Cc: 2d-dev <2d-...@openjdk.java.net> > >>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ > >>>>> exception in > >>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using > >>>>> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > >>>>> > >>>>> Hi Matthias, > >>>>> > >>>>> looks good to me. > >>>>> > >>>>> Don't forget the Copyright years. > >>>>> > >>>>> Best regards > >>>>> Christoph > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Baesken, Matthias > >>>>>> Sent: Montag, 4. Juni 2018 16:20 > >>>>>> To: Thomas Stüfe <thomas.stu...@gmail.com>; 'build- > >>>>>> d...@openjdk.java.net' <build-dev@openjdk.java.net>; awt- > >>>>>> d...@openjdk.java.net > >>>>>> Cc: 2d-dev <2d-...@openjdk.java.net>; Langer, Christoph > >>>>>> <christoph.lan...@sap.com> > >>>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ > >>>>>> exception in > >>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using > >>>>>> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > >>>>>> > >>>>>> Hello, I prepared a second webrev. > >>>>>> > >>>>>> - use now const-reference in the catch-statements as suggested by > >>>>> Thomas > >>>>>> - reenabled the cl warning showing the exception issues in > >>>>>> make/lib/Awt2dLibraries.gmk > >>>>>> - found a second place in WPrinterJob.cpp with similar issues > >>>>>> after > >>>>>> reenabling the warnings > >>>>>> > >>>>>> Please review : > >>>>>> > >>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8204211.1/ > >>>>>> > >>>>>> (cc-ing build-dev because of the makefile change, and > >>>>>> src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp > >>>>>> because of the awt change ) > >>>>>> > >>>>>> > >>>>>> Thanks, Matthias > >>>>>> > >>>>>> > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: Baesken, Matthias > >>>>>>> Sent: Montag, 4. Juni 2018 09:24 > >>>>>>> To: 'Thomas Stüfe' <thomas.stu...@gmail.com> > >>>>>>> Cc: '2d-dev' <2d-...@openjdk.java.net>; Langer, Christoph > >>>>>>> <christoph.lan...@sap.com> > >>>>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ > >>>>>>> exception > >>>>> in > >>>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using > >>>>>>> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > >>>>>>> > >>>>>>> A small update - I found a second place in WPrinterJob.cpp > >>>>>>> where the > >>>>>>> exception handling is missing. After fixing both places I can > >>>>>>> reenable > >>>>>>> warning 4297 in > >>>>>>> Awt2dLibraries.gmk which has been disabled before , probably > >>>>> because > >>>>>> of > >>>>>>> the warnings generated when the exceptions where not handled . > >>>>>>> Should I update the change with the other file + makefile change ? > >>>>>>> > >>>>>>> Best regards, Matthias > >>>>>>> > >>>>>>> > >>>>>>>> hg diff > >>>>>>> diff -r 12fe57c319e1 make/lib/Awt2dLibraries.gmk > >>>>>>> --- a/make/lib/Awt2dLibraries.gmk Tue Apr 10 11:02:09 2018 > >>>>>>> +0800 > >>>>>>> +++ b/make/lib/Awt2dLibraries.gmk Mon Jun 04 09:18:03 2018 > >>>>>>> +0200 > >>>>>>> @@ -213,6 +213,7 @@ > >>>>>>> LIBAWT_CFLAGS += -fgcse-after-reload > >>>>>>> endif > >>>>>>> > >>>>>>> > >>>>>>> $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \ > >>>>>>> NAME := awt, \ > >>>>>>> SRC := $(LIBAWT_DIRS), \ > >>>>>>> @@ -224,7 +225,7 @@ > >>>>>>> format-nonliteral parentheses, \ > >>>>>>> DISABLED_WARNINGS_clang := logical-op-parentheses extern- > >>>>> initializer, > >>>>>> \ > >>>>>>> DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE, \ > >>>>>>> - DISABLED_WARNINGS_microsoft := 4297 4244 4267 4996, \ > >>>>>>> + DISABLED_WARNINGS_microsoft := 4244 4267 4996, \ > >>>>>>> ASFLAGS := $(LIBAWT_ASFLAGS), \ > >>>>>>> LDFLAGS := $(LDFLAGS_JDKLIB) $(call > >>>>>>> SET_SHARED_LIBRARY_ORIGIN), > >>>>> \ > >>>>>>> LDFLAGS_macosx := -L$(INSTALL_LIBRARIES_HERE), \ > >>>>>>> diff -r 12fe57c319e1 > >>>>>>> > >>>>> > src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp > >>>>>>> --- > >>>>>>> > >>>>> > a/src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.c > >>>>>>> pp Tue Apr 10 11:02:09 2018 +0800 > >>>>>>> +++ > >>>>>>> > >>>>> > b/src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.c > >>>>>>> pp Mon Jun 04 09:18:03 2018 +0200 > >>>>>>> @@ -85,7 +85,13 @@ > >>>>>>> *pNpoints = outpoints; > >>>>>>> } > >>>>>>> if (outpoints > POLYTEMPSIZE) { > >>>>>>> - pPoints = (POINT *) SAFE_SIZE_ARRAY_ALLOC(safe_Malloc, > >>>>>>> sizeof(POINT), outpoints); > >>>>>>> + try { > >>>>>>> + pPoints = (POINT *) SAFE_SIZE_ARRAY_ALLOC(safe_Malloc, > >>>>>>> sizeof(POINT), outpoints); > >>>>>>> + } catch (const std::bad_alloc&) { > >>>>>>> + return NULL; > >>>>>>> + } > >>>>>>> } > >>>>>>> BOOL isempty = fixend; > >>>>>>> for (int i = 0; i < npoints; i++) { > >>>>>>> diff -r 12fe57c319e1 > >>>>>>> > src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp > >>>>>>> --- > >>>>> > a/src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp > >>>>>>> Tue Apr 10 11:02:09 2018 +0800 > >>>>>>> +++ > >>>>>> > b/src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp > >>>>>>> Mon Jun 04 09:18:03 2018 +0200 > >>>>>>> @@ -873,7 +873,12 @@ > >>>>>>> int numSizes = ::DeviceCapabilities(printerName, > >>>>>>> printerPort, > >>>>>>> DC_PAPERS, NULL, NULL); > >>>>>>> if (numSizes > 0) { > >>>>>>> - LPTSTR papers = > >>>>>>> (LPTSTR)SAFE_SIZE_ARRAY_ALLOC(safe_Malloc, > >>>>>>> numSizes, sizeof(WORD)); > >>>>>>> + LPTSTR papers; > >>>>>>> + try { > >>>>>>> + papers = (LPTSTR)SAFE_SIZE_ARRAY_ALLOC(safe_Malloc, > >>>>>> numSizes, > >>>>>>> sizeof(WORD)); > >>>>>>> + } catch (const std::bad_alloc&) { > >>>>>>> + papers = NULL; > >>>>>>> + } > >>>>>>> if (papers != NULL && > >>>>>>> ::DeviceCapabilities(printerName, printerPort, > >>>>>>> DC_PAPERS, papers, NULL) != > >>>>>>> -1) { > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: Baesken, Matthias > >>>>>>>> Sent: Freitag, 1. Juni 2018 14:18 > >>>>>>>> To: 'Thomas Stüfe' <thomas.stu...@gmail.com> > >>>>>>>> Cc: 2d-dev <2d-...@openjdk.java.net>; Langer, Christoph > >>>>>>>> <christoph.lan...@sap.com> > >>>>>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ > >>>>> exception > >>>>>> in > >>>>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using > >>>>>>>> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > >>>>>>>> > >>>>>>>> Hi Thomas , using the const-reference sounds like a good > >>>>>>>> idea ( I just > >>>>>>>> copied from other locations in the source code where (almost?) > >>>>> always > >>>>>>>> std::bad_alloc& (non-const) is caught . > >>>>>>>> > >>>>>>>> For example : > >>>>>>>> > >>>>>>>> > >>>>>>>> alloc.h 170 } catch (std::bad_alloc&) { \ > >>>>>>>> 177 } catch (std::bad_alloc&) { \ > >>>>>>>> 200 } catch (std::bad_alloc&) { \ > >>>>>>>> 206 } catch (std::bad_alloc&) { \ > >>>>>>>> > >>>>>>>> awt_InputTextInfor.cpp 223 } catch (std::bad_alloc&) { > >>>>>>>> 247 } catch (std::bad_alloc&) { > >>>>>>>> 317 } catch (std::bad_alloc&) { > >>>>>>>> 372 } catch (std::bad_alloc&) { > >>>>>>>> 407 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> awt_DnDDT.cpp 203 } catch (std::bad_alloc&) { > >>>>>>>> 264 } catch (std::bad_alloc&) { > >>>>>>>> 305 } catch (std::bad_alloc&) { > >>>>>>>> 366 } catch (std::bad_alloc&) { > >>>>>>>> 582 } catch (std::bad_alloc&) { > >>>>>>>> 635 } catch (std::bad_alloc&) { > >>>>>>>> 653 } catch (std::bad_alloc&) { > >>>>>>>> 698 } catch (std::bad_alloc&) { > >>>>>>>> 739 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> awt_Desktop.cpp 148 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> WPrinterJob.cpp 166 } catch (std::bad_alloc&) { > >>>>>>>> 345 } catch (std::bad_alloc&) { > >>>>>>>> 425 } catch (std::bad_alloc&) { > >>>>>>>> 488 } catch (std::bad_alloc&) { > >>>>>>>> 631 } catch (std::bad_alloc&) { > >>>>>>>> 709 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> awt_ole.h 158 } catch (std::bad_alloc&) {\ > >>>>>>>> > >>>>>>>> awt_DesktopProperties.cpp 125 catch (std::bad_alloc&) { > >>>>>>>> 269 catch (std::bad_alloc&) { > >>>>>>>> 647 catch (std::bad_alloc&) { > >>>>>>>> 664 catch (std::bad_alloc&) { > >>>>>>>> 689 catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> awt_PrintDialog.cpp 225 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> awt_DataTransferer.cpp 310 } catch (std::bad_alloc&) { > >>>>>>>> 724 } catch (std::bad_alloc &) { > >>>>>>>> 792 } catch (std::bad_alloc &) { > >>>>>>>> > >>>>>>>> awt_MenuItem.cpp 328 } catch (std::bad_alloc&) { > >>>>>>>> 348 } catch (std::bad_alloc&) { > >>>>>>>> 524 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> ShellFolder2.cpp 1410 } catch (std::bad_alloc&) { > >>>>>>>> 1435 } catch (std::bad_alloc&) { > >>>>>>>> > >>>>>>>> ... > >>>>>>>> > >>>>>>>> Best regards, Matthias > >>>>>>>> > >>>>>>>> > >>>>>>>>> -----Original Message----- > >>>>>>>>> From: Thomas Stüfe [mailto:thomas.stu...@gmail.com] > >>>>>>>>> Sent: Freitag, 1. Juni 2018 12:02 > >>>>>>>>> To: Baesken, Matthias <matthias.baes...@sap.com> > >>>>>>>>> Cc: 2d-dev <2d-...@openjdk.java.net>; Langer, Christoph > >>>>>>>>> <christoph.lan...@sap.com> > >>>>>>>>> Subject: Re: RFR: JDK-8204211: windows : handle potential C++ > >>>>>> exception > >>>>>>> in > >>>>>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using > >>>>>>>>> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > >>>>>>>>> > >>>>>>>>> Hi Matthias, > >>>>>>>>> > >>>>>>>>> Please consider catching all exceptions, not just std::alloc: > >>>>>>>>> > >>>>>>>>> } catch (...) { return NULL; } > >>>>>>>>> > >>>>>>>>> and doing it at the exit extern "C" function, not somewhere > >>>>>>>>> internally. Regardless of which exceptions get thrown around > >>>>>>>>> below > >>>>>> you > >>>>>>>>> and by whom, you are safe that way. > >>>>>>>>> > >>>>>>>>> However, if you want to keep your patch as it is, please catch at > >>>>>>>>> least as const reference: > >>>>>>>>> > >>>>>>>>> } catch (const std::bad_alloc&) {} > >>>>>>>>> > >>>>>>>>> Fine otherwise. I do not need another webrev. > >>>>>>>>> > >>>>>>>>> Best Regards, Thomas > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Fri, Jun 1, 2018 at 10:39 AM, Baesken, Matthias > >>>>>>>>> <matthias.baes...@sap.com> wrote: > >>>>>>>>>> Hi Thomas , thanks for the feedback. > >>>>>>>>>> I created a bug and change for the excpetion handling in > >>>>>>>> GDIRenderer.cpp > >>>>>>>>> . > >>>>>>>>>> Please review . > >>>>>>>>>> > >>>>>>>>>> Thanks, Matthias > >>>>>>>>>> > >>>>>>>>>> Bug: > >>>>>>>>>> > >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204211 > >>>>>>>>>> > >>>>>>>>>> JDK-8204211: windows : handle potential C++ exception in > >>>>>> GDIRenderer > >>>>>>>>>> > >>>>>>>>>> Change : > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8204211/ > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> -----Original Message----- > >>>>>>>>>>> From: Thomas Stüfe [mailto:thomas.stu...@gmail.com] > >>>>>>>>>>> Sent: Mittwoch, 30. Mai 2018 17:37 > >>>>>>>>>>> To: Baesken, Matthias <matthias.baes...@sap.com> > >>>>>>>>>>> Cc: 2d-dev <2d-...@openjdk.java.net> > >>>>>>>>>>> Subject: Re: [OpenJDK 2D-Dev] java2d coding using > >>>>>>>>>>> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc > >>>>>>>>>>> > >>>>>>>>>>> Letting c++ exceptions escape from extern "C" functions is > >>>>>>>>>>> UB and > >>>>>>> may > >>>>>>>>>>> (and probably will) crash the process. This should be fixed. > >>>>> Approach > >>>>>>>>>>> taken by JDK-8039394 is fine (I would probably catch every > C++ > >>>>>>>>>>> exception with catch(...), not just bad_alloc, just to be > >>>>>>>>>>> safe). > >>>>>>>>>>> > >>>>>>>>>>> Best Regards, Thomas > >>>>>>>>>>> > >>>>>>>>>>> On Wed, May 30, 2018 at 5:08 PM, Baesken, Matthias > >>>>>>>>>>> <matthias.baes...@sap.com> wrote: > >>>>>>>>>>>> Hello , there is still some java2d coding where > >>>>>>>>> SAFE_SIZE_ARRAY_ALLOC / > >>>>>>>>>>>> safe_Malloc is used and the (potentially occurring) > >>>>>>>>>>>> exception > >>>>> is > >>>>>>> not > >>>>>>>>>>>> handled . > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> This leads to CL warnings (when enabled ) like > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> " function assumed not to throw an exception but does ; The > >>>>>>> function > >>>>>>>> is > >>>>>>>>>>>> extern "C" and /EHc was specified" > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Example : > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>> > java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> static POINT *TransformPoly() > >>>>>>>>>>>> > >>>>>>>>>>>> ….. > >>>>>>>>>>>> > >>>>>>>>>>>> if (outpoints > POLYTEMPSIZE) { > >>>>>>>>>>>> > >>>>>>>>>>>> pPoints = (POINT *) > >>>>>>>>>>>> SAFE_SIZE_ARRAY_ALLOC(safe_Malloc, > >>>>>>>>>>>> sizeof(POINT), outpoints); > >>>>>>>>>>>> > >>>>>>>>>>>> } > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Should we add exception handling here ? > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Similar fixes were done in the change 8039394: Compiler > >>>>> warnings > >>>>>>>>> about > >>>>>>>>>>> C++ > >>>>>>>>>>>> exceptions in windows printing code > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8039394 > >>>>>>>>>>>> > >>>>>>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/823387e2bf42 > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Best regards, Matthias > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>> > >> > > > >