8169867: Method::restore_unshareable_info does not invoke Method::link_method

Reviewed-by: jiangli, kvn, thartmann

hotspot/src/share/vm/oops/constMethod.hpp

@@ -205,7 +205,7 @@ private:
   // Adapter blob (i2c/c2i) for this Method*. Set once when method is linked.
   union {
     AdapterHandlerEntry* _adapter;
-    AdapterHandlerEntry** _adapter_trampoline;
+    AdapterHandlerEntry** _adapter_trampoline; // see comments around Method::link_method()
   };
 
   int               _constMethod_size;

hotspot/src/share/vm/oops/method.cpp

@@ -953,34 +953,103 @@ void Method::unlink_method() {
 }
 #endif
 
+/****************************************************************************
+// The following illustrates how the entries work for CDS shared Methods:
+//
+// Our goal is to delay writing into a shared Method until it's compiled.
+// Hence, we want to determine the initial values for _i2i_entry,
+// _from_interpreted_entry and _from_compiled_entry during CDS dump time.
+//
+// In this example, both Methods A and B have the _i2i_entry of "zero_locals".
+// They also have similar signatures so that they will share the same
+// AdapterHandlerEntry.
+//
+// _adapter_trampoline points to a fixed location in the RW section of
+// the CDS archive. This location initially contains a NULL pointer. When the
+// first of method A or B is linked, an AdapterHandlerEntry is allocated
+// dynamically, and its c2i/i2c entries are generated.
+//
+// _i2i_entry and _from_interpreted_entry initially points to the same
+// (fixed) location in the CODE section of the CDS archive. This contains
+// an unconditional branch to the actual entry for "zero_locals", which is
+// generated at run time and may be on an arbitrary address. Thus, the
+// unconditional branch is also generated at run time to jump to the correct
+// address.
+//
+// Similarly, _from_compiled_entry points to a fixed address in the CODE
+// section. This address has enough space for an unconditional branch
+// instruction, and is initially zero-filled. After the AdapterHandlerEntry is
+// initialized, and the address for the actual c2i_entry is known, we emit a
+// branch instruction here to branch to the actual c2i_entry.
+//
+// The effect of the extra branch on the i2i and c2i entries is negligible.
+//
+// The reason for putting _adapter_trampoline in RO is many shared Methods
+// share the same AdapterHandlerEntry, so we can save space in the RW section
+// by having the extra indirection.
+
+
+[Method A: RW]
+  _constMethod ----> [ConstMethod: RO]
+                       _adapter_trampoline -----------+
+                                                      |
+  _i2i_entry              (same value as method B)    |
+  _from_interpreted_entry (same value as method B)    |
+  _from_compiled_entry    (same value as method B)    |
+                                                      |
+                                                      |
+[Method B: RW]                               +--------+
+  _constMethod ----> [ConstMethod: RO]       |
+                       _adapter_trampoline --+--->(AdapterHandlerEntry* ptr: RW)-+
+                                                                                 |
+                                                 +-------------------------------+
+                                                 |
+                                                 +----> [AdapterHandlerEntry] (allocated at run time)
+                                                              _fingerprint
+                                                              _c2i_entry ---------------------------------+->[c2i entry..]
+ _i2i_entry  -------------+                                   _i2c_entry ---------------+-> [i2c entry..] |
+ _from_interpreted_entry  |                                   _c2i_unverified_entry     |                 |
+         |                |                                                             |                 |
+         |                |  (_cds_entry_table: CODE)                                   |                 |
+         |                +->[0]: jmp _entry_table[0] --> (i2i_entry_for "zero_locals") |                 |
+         |                |                               (allocated at run time)       |                 |
+         |                |  ...                           [asm code ...]               |                 |
+         +-[not compiled]-+  [n]: jmp _entry_table[n]                                   |                 |
+         |                                                                              |                 |
+         |                                                                              |                 |
+         +-[compiled]-------------------------------------------------------------------+                 |
+                                                                                                          |
+ _from_compiled_entry------------>  (_c2i_entry_trampoline: CODE)                                         |
+                                    [jmp c2i_entry] ------------------------------------------------------+
+
+***/
+
 // Called when the method_holder is getting linked. Setup entrypoints so the method
 // is ready to be called from interpreter, compiler, and vtables.
 void Method::link_method(const methodHandle& h_method, TRAPS) {
   // If the code cache is full, we may reenter this function for the
   // leftover methods that weren't linked.
   if (is_shared()) {
-    if (adapter() != NULL) return;
+    address entry = Interpreter::entry_for_cds_method(h_method);
-  } else {
+    assert(entry != NULL && entry == _i2i_entry,
-    if (_i2i_entry != NULL) return;
+           "should be correctly set during dump time");
-
+    if (adapter() != NULL) {
-    assert(adapter() == NULL, "init'd to NULL" );
+      return;
+    }
+    assert(entry == _from_interpreted_entry,
+           "should be correctly set during dump time");
+  } else if (_i2i_entry != NULL) {
+    return;
   }
   assert( _code == NULL, "nothing compiled yet" );
 
   // Setup interpreter entrypoint
   assert(this == h_method(), "wrong h_method()" );
-  address entry;
 
-  if (this->is_shared()) {
+  if (!is_shared()) {
-    entry = Interpreter::entry_for_cds_method(h_method);
+    assert(adapter() == NULL, "init'd to NULL");
-  } else {
+    address entry = Interpreter::entry_for_method(h_method);
-    entry = Interpreter::entry_for_method(h_method);
-  }
     assert(entry != NULL, "interpreter entry must be non-null");
-  if (is_shared()) {
-    assert(entry == _i2i_entry && entry == _from_interpreted_entry,
-           "should be correctly set during dump time");
-  } else {
     // Sets both _i2i_entry and _from_interpreted_entry
     set_interpreter_entry(entry);
   }
@@ -1024,7 +1093,7 @@ address Method::make_adapters(methodHandle mh, TRAPS) {
 
   if (mh->is_shared()) {
     assert(mh->adapter() == adapter, "must be");
-    assert(mh->_from_compiled_entry != NULL, "must be"); // FIXME, the instructions also not NULL
+    assert(mh->_from_compiled_entry != NULL, "must be");
   } else {
     mh->set_adapter_entry(adapter);
     mh->_from_compiled_entry = adapter->get_c2i_entry();
@@ -1034,9 +1103,9 @@ address Method::make_adapters(methodHandle mh, TRAPS) {
 
 void Method::restore_unshareable_info(TRAPS) {
   // Since restore_unshareable_info can be called more than once for a method, don't
-  // redo any work.   If this field is restored, there is nothing to do.
+  // redo any work.
-  if (_from_compiled_entry == NULL) {
+  if (adapter() == NULL) {
-    // restore method's vtable by calling a virtual function
+    // Restore Method's C++ vtable by calling a virtual function
     restore_vtable();
 
     methodHandle mh(THREAD, this);

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

@@ -2600,6 +2600,7 @@ AdapterHandlerEntry* AdapterHandlerLibrary::new_entry(AdapterFingerPrint* finger
 AdapterHandlerEntry* AdapterHandlerLibrary::get_adapter(const methodHandle& method) {
   AdapterHandlerEntry* entry = get_adapter0(method);
   if (method->is_shared()) {
+    // See comments around Method::link_method()
     MutexLocker mu(AdapterHandlerLibrary_lock);
     if (method->adapter() == NULL) {
       method->update_adapter_trampoline(entry);
@@ -2609,6 +2610,7 @@ AdapterHandlerEntry* AdapterHandlerLibrary::get_adapter(const methodHandle& meth
       CodeBuffer buffer(trampoline, (int)SharedRuntime::trampoline_size());
       MacroAssembler _masm(&buffer);
       SharedRuntime::generate_trampoline(&_masm, entry->get_c2i_entry());
+      assert(*(int*)trampoline != 0, "Instruction(s) for trampoline must not be encoded as zeros.");
 
       if (PrintInterpreter) {
         Disassembler::decode(buffer.insts_begin(), buffer.insts_end());

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

@@ -676,6 +676,9 @@ class AdapterHandlerEntry : public BasicHashtableEntry<mtCode> {
   void print_adapter_on(outputStream* st) const;
 };
 
+// This class is used only with DumpSharedSpaces==true. It holds extra information
+// that's used only during CDS dump time.
+// For details, see comments around Method::link_method()
 class CDSAdapterHandlerEntry: public AdapterHandlerEntry {
   address               _c2i_entry_trampoline;   // allocated from shared spaces "MC" region
   AdapterHandlerEntry** _adapter_trampoline;     // allocated from shared spaces "MD" region