On 01/12/16 02:30, David Holmes wrote:
> Hi Gary,
>
> I mainly looked at the hotspot files, but not everything in detail.
> Some issues will change when this sync's up with mainline again.
>
> Overall a lot of things making me cringe.
>
> Cheers,
> David
>
> ---
>
> hotspot/make/Makefile
>
> Isn't a JIT-less VM a ZERO build? Otherwise if we really want CORE
> then we should make CORE work.
I'll leave the zero versus core vm question to Bob.
If I recall correctly neither was being maintained
and tested on a regular basis, and the zero only required
minimal updates to get up and running on ios platform.
>
> ---
>
> hotspot/make/bsd/makefiles/dtrace.make
>
> Please make sure the comments are updated to reflect the code eg:
>
> 27 # We build libjvm_dtrace/libjvm_db/dtrace for COMPILER1 and
> COMPILER2
> 28 # but not for CORE configuration.
> 29
> 30 ifneq ("${TYPE}", "CORE")
> 31 ifneq ("${TYPE}", "MINIMAL1")
> 32 ifneq ($(OPENJDK_TARGET_OS), ios)
>
> line 28 needs updating. I'd also like to see indentation of
> conditional code if possible.
We held off on indentation changes, since the hotspot makefiles
had not been updated for the new build guidelines.
>
> 329 else # IPHONE build
>
> There is no "if" that references IPHONE build - is this really the iOS
> part?
>
> ---
>
> hotspot/make/bsd/makefiles/minimal1.make
> hotspot/make/linux/makefiles/minimal1.make
>
> This seems to be a new definition of the minimal VM which now includes
> JVM TI ?? Not sure the minimal VM should mean different things on
> different platforms. Also unclear if you can really enable JVM TI with
> all the other "minimal" components disabled - it was never
> intended/allowed-for an arbitrary subset of INCLUDE_XXX flags to be
> turned on/off.
I'll leave this for Bob to address.
We initially were supporting minimal and client vm for ios,
but found our users only needing the jvmti features from
the client vm.
>
> ---
>
> hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
> hotspot/src/cpu/x86/vm/c1_Runtime1_x86.cpp
> etc
>
> Without more context the change of casts from intptr_t to int32_t, or
> added cast of int32_t seem wrong - they should be 64-bit on a 64-bit
> platform. If this is all actually 32-bit only code then a lot of other
> changes can/should be made regarding the LP64 macros. Regardless if
> the type of NULL_WORD is causing a problem then its type should be
> fixed, not added casts everywhere it is used.
I'll leave this to Bertrand to address.
I'll leave the remaining android impl questions to Bob.
Our android developer is no longer with us, so we will have
some work to do before these initial code changes are
acceptable for mainstream integration.
>
> ---
>
> hotspot/src/os/linux/vm/jvm_linux.cpp
>
> Shouldn't this:
>
> + #ifdef __ANDROID__
> + // Android 'h' files don't have a define for SIGCLD
> + // I therefore took the following define & comment from Linux's
> + // /usr/include/bits/signum.h
> + #define SIGCLD SIGCHLD /* Same as SIGCHLD (System V). */
> + #endif
> "CLD", SIGCLD, /* Same as SIGCHLD (System V). */
> "CHLD", SIGCHLD, /* Child status has changed
> (POSIX). */
>
> just be:
>
> + #ifdef SIGCLD
> "CLD", SIGCLD, /* Same as SIGCHLD (System V). */
> +#endif
> "CHLD", SIGCHLD, /* Child status has changed
> (POSIX). */
>
> ---
>
> hotspot/src/os/linux/vm/os_linux.cpp (and others)
>
> Is the need for all the _ANDROID_ conditionals due to this not being
> Linux or not having glibc? We actually want to get rid of the dlsym
> dynamic lookup in mainline because we no longer support Linux versions
> without the desired functions!
>
> Overall the conditionals here make this code really ugly/messy. :(
>
> ---
>
> hotspot/src/share/vm/prims/jvm.cpp
>
> Really hate the android conditionals in this shared code!
>
> ---
>
> hotspot/src/share/vm/prims/jvmtiExport.cpp
>
> Not at all clear to me that the code conditionalized belongs to
> SERVICES! Is this just a hack to exclude attach-on-demand?
> ---
>
> hotspot/src/share/vm/runtime/java.cpp
>
> Would want to look at ways to make this cleaner.
>
> ---
>
> hotspot/src/share/vm/runtime/orderAccess.inline.hpp
>
> These changes are ugly and should not be necessary - needs further
> examination. The whole point of intptr_t is that it will the same as
> int on 32-bit (ILP32), and same as a pointer on 64-bit (LP64). Is
> Android/iOS defining a different programming model?
>
>
>
> On 12/12/2015 1:15 AM, Gary Adams wrote:
>> Here's the initial upload of changes that provides support for the ios
>> and android ports
>> for the mobile/dev repos. While there have been some preliminary reviews
>> of the code,
>> there is still more work required before we will look for more thorough
>> reviews
>> and an integration to mobile/jdk9 repos.
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8145132
>> Webrev: http://cr.openjdk.java.net/~gadams/8145132/webrev.00/
>>
>>
>> Here's a simple configure script to generate a ios-x86_64 build for use
>> with the iphone simulator. (uses homebrew 64 bit freetype from
>> pkgconfig)
>>
>> export JAVA_HOME=`/usr/libexec/java_home -v 1.8`
>> export PATH=$JAVA_HOME/bin:~/homebrew/bin:$PATH
>>
>> bash ../../configure \
>> --openjdk-target=x86_64-macos-ios \
>> --with-boot-jdk=$JAVA_HOME \
>> --disable-warnings-as-errors \
>> --disable-headful \
>> --enable-static-build=yes \
>> --with-jvm-variants=minimal1
>>
>>
>> Also, tested with i586-macos-ios target for 32 bit
>> with a locally built --with-freetype 2.6.2.