Review request: Zero assembler port of HotSpot

[Tom Rodriguez]

Sorry this took so long.

Some generic comments first.  The normal hotspot style is for the { to  
being always be on the same line instead of having a break in  
between.  Many of the new files put a line break in between.

I don't really like the introduction of the CORE_BUILD define.  It  
seems orthogonal to the normal way of selecting builds.  Also zero is  
a core-like build but it's not really core so reusing core for the  
build directory doesn't seem right.  What's going to happen when shark  
is added?  I guess I would expect the build directories to look like  
linux_i486_zero instead of linux_zero_core which is what I think you'd  
currently get.

Is there some reason you can't allow ARCH_DATA_MODEL to be specified  
directly instead of adding ZERO_BITSPERWORD?  And couldn't the same be  
done with ZERO_ENDIANNESS being replaced with ENDIANNESS?  I'd prefer  
to generalize the makefiles instead of adding exceptions for zero.   
Maybe ZERO_LIBARCH could be handled similarly, though in at least one  
case we're inconsistent in our use of x86 vs. i386.

Can you rename method_entry_t to something like MethodEntryFunc?

I'm surprised by the amount of stubs you have.  Presumably these are  
only because of virtuals for types you don't use?  Wouldn't it be  
better to ifndef ZERO the files that aren't needed?  I'm assuming it's  
not a huge number of files, something like assembler, stubRoutines,  
stubGenerator and maybe some others?  Maybe that gets into a rathole?

There's something unsatisfying about the structure of these changes  
that I can't put my finger on.  Maybe it's that zero files end up  
containing both architecture independent and architecture dependent  
stuff. I know that's a pretty useless comment and I'm not sure what to  
do about it.   Maybe you have some thoughts on this?  Anyway, unless I  
can come up with something concrete doesn't worry about this.

Also I only looked at the hotspot pieces so someone familiar with the  
other parts will have to review those before they can be committed.

hotspot/make/Makefile

Don't reuse C2_BASE_DIR:

CORE_BASE_DIR=$(OUTPUTDIR)/$(VM_PLATFORM)_core

hotspot/make/linux/makefiles/buildtree.make

ZERO_ARCH_DATA_MODEL instead of ZERO_BITSPERWORD ?

hotspot/make/linux/makefiles/vm.make

remove the extra def of STATIC_CXX

hotspot/src/share/vm/runtime/icache.cpp

Why are you ifdefing this code instead of providing an empty  
implementation?  As an aside, I'm not sure it's safe to not provide an  
implementation of this at all.  Even the installation of newly  
generated code requires an icache flush, otherwise you might just see  
garbage instead of the code you actually generated.  Is shark handling  
this differently?

hotspot/src/share/vm/runtime/jniHandles.hpp

What's the purpose of promoting clear() to protected in  
JNIHandleBlock?  There are no subclasses of JNIHandle that I can see.   
It seems like only the friend declaration is needed and I don't think  
you need to ifdef it.  It seems reasonable to allow CppInterpreter  
access to that.

hotspot/src/share/vm/runtime/signature.hpp

I'm ok with these change as is but it's unfortunate that there are  
ifdefs here at all.  Any ideas how to restructure this to eliminate  
them completely?

hotspot/src/share/vm/utilities/vmError.cpp

The direct inclusion of stackPrinter_zero.hpp isn't very nice and the  
scattered ifdef to support it's use are somewhat ugly.  Is there some  
reason the stack printing for zero can't be integrated better into the  
existing printing logic?

src/cpu/zero/vm/assembler_zero.cpp

Why do some functions like code_fill_byte have implementations?   
Shouldn't they be Unimplemented()?

src/cpu/zero/vm/assembler_zero.hpp

Can you move the include block to the beginning of the file.

src/cpu/zero/vm/bytecodeInterpreter_zero.hpp

what's the purpose of the LOTS_OF_REGS define?  There's no reference  
to it anywhere else.

hotspot/src/cpu/zero/vm/frame_zero.hpp

Shouldn't is_deoptimizer_frame be is_deoptimized_frame?

hotspot/src/cpu/zero/vm/frame_zero.inline.hpp

I think you should be calling find_blob_unsafe instead of find_blob as  
the other frame code does.

tom

On Aug 17, 2009, at 7:32 AM, Gary Benson wrote:

> Hi all,
>
> Zero is an interpreter-only port of HotSpot that uses no assembler and
> can trivially be built on any Linux system.  The following webrev adds
> Zero support to OpenJDK:
>
>  http://cr.openjdk.java.net/~gbenson/zero-07/
>
> There are a number of environment variables that need setting before
> running make, but if you are using jdk/make/jdk_generic_profile.sh
> to set up your environment then you only need to set two, and a build
> can be as simple as:
>
>  export CORE_BUILD=true
>  export ZERO_BUILD=true
>  . jdk/make/jdk_generic_profile.sh
>  gmake sanity && gmake
>
> The two mandatory environment variables do the following:
>
>  CORE_BUILD
>    Setting CORE_BUILD to "true" will result in an interpreter-only
>    HotSpot being built, with neither the client nor the server
>    compilers.  If CORE_BUILD is unset, or set to any other value
>    than "true", then the compiler(s) will be built as normal.
>
>  ZERO_BUILD
>    Setting ZERO_BUILD to "true" will cause the Zero interpreter to
>    be used.  If ZERO_BUILD is unset, or set to any other value than
>    "true", the standard, platform-specific interpreter will be used.
>
> The reason for having two separate flags is to allow for Zero builds
> with JIT compilers such as Shark (not included in this webrev).  To
> build HotSpot with Zero and Shark you would set ZERO_BUILD to "true"
> but leave CORE_BUILD unset.
>
> Of the other environment variables the following are required when
> ZERO_BUILD is set to "true".  These are set by
> jdk/make/jdk_generic_profile.sh based on the result of uname -m:
>
>  ZERO_LIBARCH
>    This is the name of the architecture-specific subdirectory under
>    $JAVA_HOME/jre/lib.  Typically this will be the same as the output
>    of uname -m, although there are some exceptions: "amd64" instead
>    of "x86_64", for example, and "i386" instead of "i686".
>
>  ZERO_ARCHDEF
>    The value of ZERO_ARCHDEF will be passed to the C++ compiler using
>    -D${ZERO_ARCHDEF} to allow conditionalized platform-specific code.
>    This is typically set to ZERO_LIBARCH converted to uppercase but,
>    again, there are exceptions.  "i386" becomes "IA32", to match what
>    HotSpot already does, and on platforms with both 32- and 64-bit
>    variants ZERO_ARCHDEF corresponds to the 32-bit version, so both
>    ppc and ppc64 have ZERO_ARCHDEF set to "PPC".
>
>  ZERO_ENDIANNESS
>    This is set to "little" or "big".
>
>  ZERO_BITSPERWORD
>    This is set to "32" or "64".
>
>  ZERO_ARCHFLAG
>    This is a flag that will be passed to the C++ compiler and to the
>    linker to instruct them to generate code for the particular
>    platform.  This is required for platforms with both 32- and 64-bit
>    variants where the compiler needs to be told which variant to
>    build for.  ZERO_ARCHFLAG will typically be set to "-m32" or
>    "-m64", except on 31-bit zSeries where it will be set to "-m31".
>
> Zero uses one external library, libffi, for JNI method invocation.
> The following two variables are used to tell the compiler and linker
> how to find libffi.  These can be set by the user, but if left unset
> then jdk/make/jdk_generic_profile.sh will attempt to set them using
> pkg-config or, failing that, by supplying defaults pointing to the
> standard locations:
>
>  LIBFFI_CFLAGS
>    Flags to be passed to the C++ compiler to build against libffi.
>    If unset (and pkg-config is not installed, or if libffi is not
>    built with pkg-config) then this defaults to "".
>
>  LIBFFI_LIBS
>    Flags to be passed to the linker to link against libffi.  If
>    unset (and pkg-config is not installed, or if libffi is not
>    built with pkg-config) then this defaults to "-lffi".
>
> Cheers,
> Gary
>
> -- 
> http://gbenson.net/