RFR [M] : 8087333, Optionally Pre-Generate the HotSpot Template Interpreter

[David Holmes]

Hi Bertrand,

I can't comment on all the details of this. Mostly it's a question of 
determining if the existing code is being modified based on the default 
definition of the CodeCacheExtensions class.

On 13/06/2015 3:30 AM, Bertrand Delsart wrote:
> Hi all,
>
> This webrev is related to a closed JEP. Its main objective is to
> support the template interpreter in highly secure contexts where
> dynamically generated code is not allowed.
>
> This will be pushed through hs-rt.
>
> https://bugs.openjdk.java.net/browse/JDK-8087333
>
> http://cr.openjdk.java.net/~bdelsart/8087333/webrev.00/webrev/

agent/src/share/classes/sun/jvm/hotspot/code/CodeCache.java
agent/src/share/classes/sun/jvm/hotspot/memory/CodeHeap.java
make/linux/makefiles/buildtree.make

Seems ok.

---

make/linux/makefiles/rules.make

$(QUIETLY) $(AS.S) $(DEPFLAGS) $(CPPFLAGS) $(CXXFLAGS) $(CFLAGS) -o $@ 
$< $(COMPILE_DONE)

CPPFLAGS no longer exists. If you specify CXXFLAGS and CFLAGS you will 
get a number of things specified twice - don't know if that is an issue, 
but it looks very odd to be passing C/C++ compiler flags to an assembler 
especially when:

  AS.S            = $(AS) $(ASFLAGS)

so ASFLAGS have already been applied. I'm thinking "CPPFLAGS" was really 
intended to be c-preprocessor flags, which are now folded in to the 
CXXFLAGS and CFLAGS.

---

make/linux/makefiles/vm.make

Seems ok.

---

src/share/vm/asm/codeBuffer.hpp

Are there any concurrency issues with making _name mutable? (volatile?, 
release-store?)

---

src/share/vm/c1/c1_Runtime1.cpp

If you initialize here:

  187   OopMapSet* oop_maps;
  188   int frame_size;
  189   bool must_gc_arguments;

then you don't need:

  231   } else {
  232     /* ignored values */
  233     oop_maps = NULL;
  234     frame_size = 0;
  235     must_gc_arguments = false;

---

src/share/vm/code/codeBlob.cpp
src/share/vm/code/codeBlob.hpp

Seems okay

src/share/vm/code/codeCache.cpp

Does _heaps->length() >= 1 imply SegmentedCodeCache==true ?

src/share/vm/code/codeCache.hpp

Seems ok.

src/share/vm/code/stubs.cpp

Can't comment.

src/share/vm/code/stubs.hpp
src/share/vm/interpreter/interpreter.cpp

Seems ok

src/share/vm/interpreter/interpreter.hpp

Suggest "// possible extension" rather than "closed extension"

src/share/vm/interpreter/interpreterRuntime.cpp

Can't comment

src/share/vm/interpreter/interpreterRuntime.hpp

Seems ok.

src/share/vm/interpreter/templateInterpreter.cpp

In:

240 void TemplateInterpreterGenerator::generate_all() {
  241   // Loop, in case we need several variants of the interpreter entries
  242   do {
  243     if (!CodeCacheExtensions::skip_code_generation()) {

Can skip_code_generation return different results for different iterations?

---

src/share/vm/interpreter/templateInterpreter.hpp

Seems ok.

src/share/vm/memory/heap.hpp

Seems ok - though odd to be adding virtual to such seemingly basic 
functions. The indent needs fixing up in a couple of places (inline tabs 
by the look of it).

---

src/share/vm/memory/virtualspace.cpp

The expression:

  CodeCacheExtensions::support_dynamic_code() ? true : false

is just:

  CodeCacheExtensions::support_dynamic_code()

--

src/share/vm/precompiled/precompiled.hpp

Seems okay - but of course begs the question whether this has been 
checked with PCH disabled?

src/share/vm/prims/methodHandles.cpp
src/share/vm/runtime/arguments.cpp

Seems okay.

src/share/vm/runtime/arguments.hpp

I'm getting doubtful about the numerous, and varied "friend" 
declarations. Why should CodeCacheExtensions need anything but the 
public API of Arguments ?

---
src/share/vm/runtime/init.cpp

The complete_step mechanics seem somewhat intrusive. This would probably 
look better as a generic initialization/life-cycle hook mechanism, 
rather than being CodeCacheExtensions specific. Though a future RFE for 
that would be okay.

---

src/share/vm/runtime/sharedRuntime.cpp

This change looks to me like something better handled in the callers 
that always expect to find a handler, rather than just returning 
something non-NULL.

---

src/share/vm/runtime/stubCodeGenerator.cpp
src/share/vm/runtime/stubCodeGenerator.hpp
src/share/vm/runtime/stubRoutines.cpp
src/share/vm/runtime/thread.cpp
src/share/vm/runtime/vmStructs.cpp
src/share/vm/runtime/vm_operations.cpp
src/share/vm/runtime/vm_version.cpp
src/share/vm/utilities/globalDefinitions.hpp
src/share/vm/code/codeCacheExtensions.hpp

Seems ok.

---

src/share/vm/code/codeCacheExtensions_ext.hpp

39 // All the methods defined here are placeholders for proprietary 
extensions.

proprietary -> optional | potential | possible

---

src/share/vm/runtime/vmStructs_ext.hpp

Seems ok.

Thanks,

David
-----



> This is a forward port to 9 of a feature that was in our embedded
> workspace. That workspace was used to build both our regular embedded
> binaries and pregenerated-enabled ones. As such, the 8 changes were
> already mature and, even more important, did not lead to regressions on
> platforms not using them. Similarly, no regression has been noticed on
> the 9 forward and the changes pass JPRT (including the openJDK builds
> and tests).
>
> The above webrev may not apply as is to jdk9/hs-rt since it comes on top
> of other CRs concurrently being reviewed (the JEP was split into smaller
> easier to review parts). The corresponding changesets are:
> http://cr.openjdk.java.net/~bdelsart/8079473/
>   "allow demangling to be optional in dll_address_to_function_name"
>   (approved, not yet pushed)
> http://cr.openjdk.java.net/~bdelsart/8081406/
>   "cleanup and minor extensions of the debugging facilities in CodeStrings"
>   (waiting for reviewers)
> http://cr.openjdk.java.net/~bdelsart/8087133/
>   "Improve sharing of native wrappers in SignatureHandlerLibrary"
>   (waiting for reviewers)
> http://cr.openjdk.java.net/~bdelsart/8030076
>   "remove unused runtime related code"
>   (waiting for reviewers)
>
> Brief overview of the shared code changes for this webrev:
>
> - Minimum changes in agent code to support CodeHeap subclasses,
>    with different visitors, necessary for the pregenerated heap
>    Latest version is generic, allowing to define different CodeHeap
>    layouts for future extensions
>
>    Validated with "search codebase" CLHSDB agent command, which
>    correctly parses both the pregenerated and the regular heap.
>
> - Build extension for the hotspot subtree, to support closed
>    extensions
>    * an extra generated 'extensions' directory
>    * ability to compile the *.S files, forcing the preprocessor to
>      be used before the assembler (used to convert pregenerated code
>      to *.o files)
>    * extra *.o files added into libjvm
>    * ability to extend the content of vm.def
>
> - Source code support for a new CodeCacheExtensions API, which
>    allows to load a pregenerated template interpreter (+all
>    stubs required for the interpreter to work). The same API
>    is also used to pregenerate this code.
>
>    See hotspot/src/share/vm/code/codeCacheExtensions_ext.hpp
>    for a definition of that API. The default implementation in
>    open performs nothing and inlining should prevent overheads.
>
>    These are HotSpot internals only, not accessible to the
>    end-user Java application.
>
> Details of the shared source code changes, mostly additional calls
> allowing our closed extension to better control what happens during
> bootstrap:
>
> - support for more than one CodeHeap (independently of
>    -XX:+SegmentedCodeCache), including CodeHeap subclasses
>    that have a different layout
>
> - ability to rename a CodeBuffer (allowing to pass the name to
>    SignatureHandlerLibrary::set_handler)
>
> - ability to save/load all stubs generated during bootstrap,
>    using CodeCacheExtensions::handle_<...> methods
>
> - ability to avoid wasting away CodeHeap space (by optimizing
>    the buffer sizes, avoiding to generate useless code, detecting which
>    interpreter entries are impacted by JVMTI, ...)
>
> - ability to install a list of pregenerated fast native
>    signature handlers.
>
> - ability, similarly to zero, to generate two interpreter loops,
>    one highly optimized and one which supports JVMTI. The interface
>    is in fact more flexible, allowing to have more than two such loops
>
> - ability to prevent the CodeHeap from being executable
>
> - a few complete_step calls to allow the extension mechanism to
>    perform additional actions. This could be extended to plug-in
>    other extensions during the bootstrap.
>
> Regards,
>
> Bertrand.
>
>