RFR[S] 8005165 Platform-independent C++ vtables for CDS

[Ioi Lam]

On 3/5/17 7:17 AM, coleen.phillimore at oracle.com wrote:
>
> Ioi,  Some comments inline (where no comments, insert "ok") :)
>
> On 3/2/17 10:37 PM, Ioi Lam wrote:
>> Hi Coleen,
>>
>> Thanks for the comments. I have updated the webrev. See in-line for 
>> responses.
>>
>> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v03/
>>
>>
>> On 3/2/17 8:48 AM, coleen.phillimore at oracle.com wrote:
>>>
>>> Ioi
>>> I like the concept of this a lot but have some stylistic comments to 
>>> help people reading this code later.
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/src/share/vm/memory/metaspaceShared.cpp.udiff.html
>>>
>>> s/vtab/vtable/g and s/Vtab/Vtable/ please.  It doesn't save many 
>>> characters, especially in CppVtableInfo/Testers
>>>
>> Done.
>>> +    // Start at slot 1, because slot 0 may be RTTI (on Solaris/Sparc)
>>> +    int i;
>>> +    for (i=1; ; i++) {
>>> Since you're using 'i' later, can you rename it to something 
>>> descriptive.  Or have another variable "vtable_length" to use 
>>> later.   This looks like an old style for loop.
>>>
>> Done
>>> Can the functions for CppVtableInfo be declared outside of the class 
>>> declaration?  They don't need to be inline and then the debug code 
>>> for testing the vtable size can be not in the middle of the class 
>>> declaration.   Then you can move the Tester classes to inside the 
>>> same #ifndef PRODUCT block.
>>>
>>> Can you put #endif // PRODUCT when the ifdef covers several lines of 
>>> code?
>>>
>> Done
>>> vtab_of could be more descriptive, like cpp_vtable_for().
>>>
>> I changed to vtable_of(). Because the class name is already 
>> CppVtableCloner, repeating the word "cpp" seems repetitive to me.
>>
>>> Was PrintSharedSpaces was never converted to UL?
>>>
>> Right. I've filed https://bugs.openjdk.java.net/browse/JDK-8176132 
>> (-XX:+PrintSharedSpaces should be converted to use Unified Logging.)
>>> +    int n = MAX_VTABLE_SIZE;
>>>
>>> Can you propagate MAX_VTABLE_SIZE to the places where it's used.  n 
>>> isn't descriptive.  This starts out with max_vtable_size and then 
>>> changes the size.  Reusing 'n' makes this really hard to follow.  
>>> Not having a comment that we only allocate enough slots for the 
>>> vtable makes it hard too.
>>>
>>> +    // allocate CppVtableInfo in the MD section
>>> +    _info = (CppVtabInfo*)md_top;
>>> +    _info->set_vtab_size(n);      // initially set to max_vtable_size
>>> +
>>> +    // allocate temporary local instance of the metadata type T
>>> +    T tmp;
>>> +    intptr_t* srcvtab = vtab_of(tmp);
>>> +    intptr_t* dstvtab = _info->vtab();
>>> +
>> Fixed.
>>> Something like that for comments.  dstvtab is the destination_vtable 
>>> in the MD section. 
>>
>> I've dropped the md_ prefix from the functions that deal with the 
>> vtables, since they shouldn't care whether it's the "MD" section or 
>> not. Now it looks like this:
>>
>> // Allocate and initialize the C++ vtables, starting from top, but do 
>> not go past end.
>> intptr_t* MetaspaceShared::allocate_cpp_vtable_clones(intptr_t* top, 
>> intptr_t* end) {
>>   assert(DumpSharedSpaces, "dump-time only");
>>   // Layout (each slot is a intptr_t):
>>   //   [number of slots in the first vtable = n1]
>>   //   [ <n1> slots for the first vtable]
>>   //   [number of slots in the first second = n2]
>>   //   [ <n2> slots for the second vtable]
>>   //   ...
>>   // The order of the vtables is the same as the 
>> CPP_VTAB_PATCH_TYPES_DO macro.
>>   CPP_VTABLE_PATCH_TYPES_DO(ALLOC_CPP_VTABLE_CLONE);
>>   return top;
>> }
>>
>>> +    for (int i=0; i<n; i++) {
>>> +      const intptr_t bad = intptr_t(0xdeadbeef);
>>> +      intptr_t num = SafeFetchN(&srcvtab[i], bad);
>>> +      if (num == bad
>>> +          // || i > 120 /* uncomment this line to test */
>>> +          ) {
>>> +        _info->set_vtab_size(i-1);
>>> +        break;
>>> +      }
>>> +      dstvtab[i] = num;
>>> +    }
>>>
>>> I dont understand this code.   You get deadbeef for a bad value if 
>>> SafeFetchN gets a fault but why would it get a fault at the end of 
>>> the metadata's vtable?   Couldn't it just run onto the next vtable?  
>>> I think your original way of counting vtable entries might be better 
>>> (sorry I didn't have time to study that thread).
>>>
>> I've modified the comments to this. Does it make sense to you?
>>
>>     // It is not always safe to call memcpy(), because srcvtable 
>> might be shorter than
>>     // MAX_VTABLE_SIZE, and the C++ linker might have placed the 
>> vtable at the very
>>     // end of the last page of libjvm.so. Crossing over to the next 
>> page might
>>     // cause a page fault.
>>
>> My fear is the JVM would suddenly start crashing because the order of 
>> .o files have changed on the linker's command line, or if you enable 
>> some special linker optimization flags. It's better safe than sorry.
>
> This wasn't exactly what I was not understanding.   I didn't see that 
> you are copying 120 entries from the old vtable and junk memory beyond 
> the old vtable, unless you get a segv, in which case you copy less.   
> I don't think you should copy random memory into the vtable in the 
> archive.  This doesn't seem secure, even with the segv protection.
>
> Since we already have assumptions about C++ vtable layout in the code 
> and it's mostly specified by various ABIs, and you have the assert 
> code, I think I would prefer that you copy only the vtable entries 
> into the archive.   I guess Thomas Stuefe had a different opinion.  
> I've read the original thread.  Two points:
>
> If new C++ compiler implementations add a discontigous vtable, both 
> the SafeFetchN and subclass additional virtual function at end 
> implementation will fail.  I don't think C++ implementations would do 
> this and a contiguous vtable as first in the instance has been 
> standard for years.   If our metadata adds multiple inheritance, the 
> same issue would be a problem for both implementations, as well as for 
> the implementation we have before Ioi's fix.
>
> Ioi's subclass adding virtual function method would work for any 
> esoteric C++ implementations in my memory, except the vptr for the old 
> DECC++ compiler was after the nonstatic data members (which would fail 
> with all of our implementations).
>
> Since the code is there anyway for debug purposes, we're not saving 
> code by implementing SafeFetchN.  The SafeFetchN implementation isn't 
> obvious at all what it's doing, and requires better comments, 
> especially if you don't know already what SafeFetchN does.  It looks 
> really cryptic.  The poisoned values also bothered me in that they 
> overload other poisoned values in other parts of the jvm.
>

I can go with either implementation, although I like the original one 
that doesn't use SafeFetch.
> Ioi, could you make all methods of CppVtableCloner out of line?
>
Is it for debugging purposes? I am using a recent version of gdb and I 
have no problems setting break points or stepping into the code. I can 
move the bigger methods out, but for clarity I think it's better to 
leave the small methods inside the class declaration.

Thanks
- Ioi

> The other changes look good, although I might have more requests for 
> comments.
>
> Thanks,
> Coleen
>
>>> Would be nice to have comments here too!!
>>>
>>> +  intptr_t* start = md_top;
>>>
>>> This doesn't do anything (?)
>>
>> Fixed. This was left over code.
>>>
>>> +   MetaspaceShared::zero_cpp_vtable_clones_for_writing();
>>>
>>> Why not zero the destination vtable in allocate?   Or does patching 
>>> the vtable pointers call virtual functions?  You could prevent that 
>>> so you don't need this code.
>>>
>> I added this comment:
>>
>>   // During patching, some virtual methods may be called, so at this 
>> point
>>   // the vtables must contain valid methods (as filled in by 
>> CppVtableCloner::allocate).
>>   MetaspaceShared::patch_cpp_vtable_pointers();
>>
>>   // The vtable clones contain addresses of the current process.
>>   // We don't want to write these addresses into the archive.
>> MetaspaceShared::zero_cpp_vtable_clones_for_writing();
>>
>>> +  // Restore the vtable in case we invoke any virtual methods.
>>> +  MetaspaceShared::clone_cpp_vtables((intptr_t*)vtbl_list);
>>> Can this be restore_cpp_vtables since that's what it's doing.   The 
>>> first is after the dump and the second call is at UseSharedSpaces.   
>>> A couple of comments in this clone_cpp_vtables --> 
>>> restore_cpp_vtables would be nice. eg:
>>>
>> I prefer to use the word clone. Otherwise when you just say "vtable" 
>> it's not clear whether you're talking about the original one (made by 
>> the c++ linker), or the cloned one in the CDS archive.
>>> +  static intptr_t* clone_vtable(const char* name, intptr_t* p) {
>>> +    T tmp;   // Allocate temporary dummy metadata object to get vtable initialized
>>> +    CppVtabInfo* info = (CppVtabInfo*)p;
>>> +    int n = info->vtab_size();
>>> +    intptr_t* srcvtab = vtab_of(tmp);
>>> +    intptr_t* dstvtab = info->vtab();
>>> +
>>> +    // We already checked (and, if necessary, adjusted n) when the vtables were allocated, so we are
>>> +    // safe to do memcpy.
>>> +    if (PrintSharedSpaces) {
>>> +      tty->print_cr("%s copying %d vtable entries", name, n);
>>> +    }
>>> +    memcpy(dstvtab, srcvtab, sizeof(intptr_t) * n);
>>> +    return dstvtab + n;
>>> +  }
>>>
>> Done. I changed the wording
>>
>>     T tmp; // Allocate temporary dummy metadata object to get to the 
>> original vtable.
>>
>> As we are not really "initializing a vtable" here.
>>
>>> Same with 'patch'.   It'd be so much faster and easier to read this 
>>> code with more comments please.
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/src/share/vm/oops/constantPool.hpp.udiff.html
>>>
>>> Why are these testers here?
>>>
>>
>> I updated the comment:
>>
>>   // Used by CDS. These classes need to access the private 
>> ConstantPool() constructor.
>>   template <class T> friend class CppVtableTesterA;
>>   template <class T> friend class CppVtableTesterB;
>>   template <class T> friend class CppVtableCloner;
>>
>>
>> Thanks
>> - Ioi
>>
>>>>
>>>>>> On 3/1/17 3:25 AM, Ioi Lam wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8005165
>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/  
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> This is the official review (follow up of the "Determining the size of C++ vtables" thread onhotspot-dev at openjdk.java.net).
>>>>>>
>>>>>> The new code has the same assumption as the existing code in JDK 10: for a C++ object that contains virtual methods (e.g., ConstantPool), we assume the first intptr_t slot of the object is a _vptr, which points to a vtable, which consists of no more than 150 intptr_t's.
>>>>>>
>>>>>> ConstantPool*p -->[ _vptr    ] -------> [ vtable slot 0 ]
>>>>>>                    [ field #0 ]          [ vtable slot 1 ]
>>>>>>                    [ field #1 ]          [ vtable slot 2 ]
>>>>>>                    [ field #2 ]          [ vtable slot 3 ]
>>>>>>                    [ ....     ]          [ vtable slot 4]
>>>>>>                                          [ vtable slot 5 ]
>>>>>>                                          [ ...           ]
>>>>>>
>>>>>> + In the existing code, we were pointing the vtable slots to
>>>>>>   code that's generated by HotSpot.
>>>>>>
>>>>>> + In the new code, we copy the vtable slots from an existing
>>>>>>   vtable (generated by the C++ linker).
>>>>>>
>>>>>> Per Thomas Stüfe's advice, I don't try to determine the size of the vtable (as that would add one more compiler requirement where new virtual methods added by a subclass must be placed at a higher offset in the vtable).
>>>>>>
>>>>>> Instead, I have added code in non-product builds to ensure that the vtables are no longer than 150 entries. You can run with "-XX:+PrintSharedSpaces -Xshare:dump" to print out the actual size of the vtables for your particular platform:
>>>>>>
>>>>>>   ConstantPool has 12 virtual methods
>>>>>>   InstanceKlass has 113 virtual methods
>>>>>>   InstanceClassLoaderKlass has 113 virtual methods
>>>>>>   InstanceMirrorKlass has 113 virtual methods
>>>>>>   InstanceRefKlass has 113 virtual methods
>>>>>>   Method has 12 virtual methods
>>>>>>   ObjArrayKlass has 114 virtual methods
>>>>>>   TypeArrayKlass has 114 virtual methods
>>>>>>
>>>>>> As mentioned in the code comments, if you have an esoteric C++ compiler, the verify_sufficient_size() function will probably fail, but hopefully that would give you some hints for porting this code.
>>>>>>
>>>>>> To avoid accidentally touching an unmapped page, the code uses SafeFetchN for copying the vtable contents, and would shrink the vtable to less than 150 entries if necessary. I can't test this for real, but I've added some code to simulate an error:
>>>>>>
>>>>>>     for (int i=0; i<n; i++) {
>>>>>>       const intptr_t bad = intptr_t(0xdeadbeef);
>>>>>>       intptr_t num = SafeFetchN(&srcvtab[i], bad);
>>>>>>       if (num == bad
>>>>>>           // || i > 120 /* uncomment this line to test */
>>>>>>           ) {
>>>>>>         _info->set_vtab_size(i-1);
>>>>>>         break;
>>>>>>       }
>>>>>>       dstvtab[i] = num;
>>>>>>     }
>>>>>>
>>>>>> Results:
>>>>>>
>>>>>> + Removed 850 lines of CPU-dependent code
>>>>>>
>>>>>> + CDS image is about 50K smaller
>>>>>>
>>>>>> + Previously Metadata objects must live in the read-write section in the CDS
>>>>>>   archive, because their _vptr was updated at run time. Now _vptr is no longer
>>>>>>   updated, so ConstantPool can be moved to the read-only section (see JDK-8171392).
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>
>>
>