Request for review: JDK-8004728 Hotspot support for parameter reflection

[Eric McCorkle]

On 01/07/13 12:02, Karen Kinnear wrote:
> Eric,
> 
> Code looks good. Thank you for running the testlist. What platform did you run it on?

Linux (gentoo) amd64, 3.6.4 kernel version.

> 
> Also - I assume that the unit test will be checked in with the reflection library code
> change. 
> 

Yes, there is an end-to-end test done in the JDK side.  The JDK side
depends on symbols exported from the VM, so it cannot go in until this does.

> So the two remaining issues are:
> 1) how does this work with class redefinition?

All the data for MethodParameters is stored in constMethod.  When we
discussed in person, you said that this would be ok (I personally lack
in-depth knowledge of hotspot to be able to comment directly)

> 2) how does this work serviceability agent?
>   - see agent/src/share/classes/sun/jvm/hotspot/oops/ConstMethod.java for where to add fixes
> 
> Please  file additional bugs for those to fix for jdk8.
> 
> It is ok to  check this is in  prior to
> fixing those because of the need to get this initial hotspot set of changes in to
> be able to get the core library changes in which depend on these. In general it
> is important to get all of the dependent functionality working before checking any of it in.
> 

I will file bugs for this, as well as for the GenericTypeSignature field
added to constMethod recently (which I believe has the same issue)

> 
> Questions/comments:
> 
> 1. test/Makefile
> Is there a reason you changed the jtreg version? Did someone ask you to?
> To do this you want to rerun all the jtreg tests :-) - or you could skip this change
> 

That's a mistake.  I believe I had to change it to get something to run
on my machine.  I will revert it prior to push.

> 2. constMethod.cpp/constMethod.hpp
> 
> I realize that this is complex, and it would be good to file an rfe to make this less
> brittle and sync up with serviceability agent better.
> 

I agree.  I had some very subtle errors arise from this code.

> In the meantime, thank you for the improved comments and fix to the
> generic_signature_index 
> Thank you for the comments in ConstMethod::set_inlined_tables_length.
> Could you possibly add a comment that  serviceability agent knows about the actual ordering
> and needs to be changed if any fields are added here?
> 

That's a very good idea.  Done.

> 3. vmSynbols.hpp
> Please check spacing for line 89
> 

Done.

Thanks for your review and help, Karen!