RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

Hi JC,

thanks for looking at it and even paying attention to the changes;)

regarding your nits. a space before comma is straightforward and must be done for test code to be aligned w/ regular hotspot code style, static_case<> is more controversial, it's not used much by hotspot. I do recall some discussion around it 3-4 years ago, but I can't recall the outcome. anyhow, I'd prefer to clean it up later, as these editorial/style fixes can be done more gradually, and this patch kinda blocks your work on 8209547.

Thanks,
-- Igor


> On Aug 22, 2018, at 11:53 AM, JC Beyler <jcbeyler at google.com> wrote:
> 
> Hi Igor,
> 
> I looked over the changes and tried to pay attention to all the mechanical changes done. They look good to me as they are the step in the direction of getting it all in C++. I will be happy to start on 8209547 once this goes in. 
> 
> (As always, not an official reviewer)
> 
> Two nits:
> 
> - We could use static_cast?
> +        *new_bytes = (u1*) realloc(gen, *new_length);
> 
> - Add a space since we are doing the change (this is one example)?
> -    arrayOrig[i]=env->GetObjectArrayElement(orig,i); CE
> -    arrayClone[i]=env->GetObjectArrayElement(clone,i); CE
> +    arrayOrig[i]=(jarray) env->GetObjectArrayElement(orig,i); CE
> +    arrayClone[i]=(jarray) env->GetObjectArrayElement(clone,i); CE
> 
> On the other hand, we could just clean this up later once this big move to C++ happens, so I'd be happy to see us tackle it in a second/third step.
> 
> So looks good to me and thanks for doing it!
> Jc
> 
> On Tue, Aug 21, 2018 at 9:58 PM Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
> Hi David,
> 
> thanks for looking at the webrev and all your comments. my answers are inlined.
> 
> enjoy your vacation!
> 
> -- Igor 
> 
> > On Aug 21, 2018, at 9:28 PM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
> > 
> > Hi Igor,
> > 
> > On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
> >> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html <http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html> <http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.02/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.02/index.html>> is a new version of patch, which moves only vmTestbase tests.
> > 
> > This seems okay in principle. I didn't verify all the mechanical changes individually.
> > 
> > make/common/TestFilesCompilation.gmk
> > 
> > I suspect you can just find all .c and .cpp files and process them together, rather than duplicate all the logic. But I'll leave that to Magnus to comment on.
> I had to duplicate this foreach cycle to specify a correct TOOLCHAIN, w/o it linking fails on linux-slowdebug. 
> > 
> > make/test/JtregNativeHotspot.gmk
> > 
> > + BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
> > 
> > Just a note that defining these is obsolete once we use C11/C++11 - which I think is an intent for JDK 12 if possible.
> initially I wanted to use <cinttypes> instead of <inttype.h> + -D__STDC_FORMAT_MACROS, but solaris studio apparently doesn't have cinttypes header file.
> 
> hotspot makefiles also use -D__STDC_, so I hope both places will be fixed together.
> 
> > 
> > test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp
> > 
> >  #ifdef __cplusplus
> > ! #define JNI_ENV_ARG(x, y) y
> >  #define JNI_ENV_PTR(x) x
> >  #else
> >  #define JNI_ENV_ARG(x, y) x , y
> >  #define JNI_ENV_PTR(x) (*x)
> >  #endif
> > 
> > If this is now a C++ source file why do you a still have the code that allows for either C or C++? Is this stuff that will be cleaned up in the later RFE?
> yes, 8209547 will clean up JNI_ENV_ARG macros.
> 
> > 
> > ---
> > 
> > The switch to C++ means a lot of code now needs:
> > 
> > + #ifdef __cplusplus
> > + extern "C" {
> > + #endif
> 
> that's true, and this patch adds 'extern "C"' very conservatively (basically to everywhere). this, if needed, can be cleaned up later.
> 
> > 
> > I still have to wonder whether it better to leave these as C tests and only convert to C++ as and when needed ... seems like so much work.
> > 
> > Anyway I'm off on vacation after today so I'll leave it to others to take this up.
> > 
> > Thanks,
> > David
> > 
> >> Thanks,
> >> -- Igor
> >>> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com> <mailto:igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>>> wrote:
> >>> 
> >>>>> It has been discussed (not widely enough and I accept that) in 8209547 and the related email thread b/w JC(cc'ed) and myself.
> >>>>> as I said, I might went a way too far, so I'll revert changes in the non-vmTestbase tests and made appropriate changes in makefiles. what do you think?
> >>>> 
> >>>> I think I need to see what you mean exactly :)
> >>> sure, it will take some time for me to do that, hopefully will upload new webrevs tomorrow morning PT. but the basic idea is to leave files in test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, testlibrary as .c files, exactly as they were before, and restore corresponding filenames in make/test/JtregNativeHotspot.gmk.
> 
> 
> 
> -- 
> 
> Thanks,
> Jc