RFR - Changes to address CCC 8014135: Support for statically linked agents

[serguei.spitsyn at oracle.com]

On 8/5/13 3:13 PM, Coleen Phillimore wrote:
>
> Hi Bill,  I have some high level comments and other comments. This 
> wasn't as easy to review as Bob promised me!
>
> http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.02/src/share/classes/com/sun/tools/attach/VirtualMachine.java.udiff.html 
>
>
> paramter  (typo - two instances)
>
>
>       * @throws  AgentInitializationException
> -     *          If the <code>Agent_OnAttach</code> function returns 
> an error
> +     *          If the <code>Agent_OnAttach[_L]</code> function 
> returns an error.
> +     *          an error.
>       *
>
> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/os/posix/vm/os_posix.cpp.frames.html 
>
>
> nameLen, prefixLen, suffixLen - the JVM coding convention (unlike 
> Java) is to have variable names lower case and separated by a _ (not 
> camelcase).  Same for all the new code here buildAgentFunctionName.
>
> Can you put a function above this function to say what it does? So 
> once it's code reviewed, we can quickly know what it does without 
> having to study this again - ie.  Is name something like libxxx.so and 
> you're trying to extract lib and .so from it? There's no verification 
> of this at lines 286 and 287 but the code is handling the case of 
> other sorts of unexpected input (ie line 283).   Why don't you strip 
> the lib and the .so if !is_absolute_path?

The !is_absolute_path case is to cover the agentlib start-up option that
requires the agent library name in a platform-independent form which
does not have to be stripped:
http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#starting


Thanks,
Serguei

>
>  291   agentEntryName = NEW_C_HEAP_ARRAY(char, len, mtThread);
>  292   if (agentEntryName == NULL) {
>  293     return NULL;
>  294   }
>
> If NEW_C_HEAP_ARRAY fails it terminates the JVM.   I think you might 
> want this function instead NEW_C_HEAP_ARRAY_RETURN_NULL if there's a 
> reasonable recovery possible.
>
> os_windows.cpp
>
> Same comments as above.  Also:
>
> 5432       strncat(agentEntryName, name, nameLen);
> 5433       strcat(agentEntryName, p);
> 5434       //agentEntryName == _Agent_OnLoad_libName at 12
> 5435     } else {
>
> You could say why 12 but shouldn't it be the length of the resulting 
> symbol instead and not 12?
>
> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/share/vm/runtime/os.cpp.frames.html 
>
>
> findAgentFunction - same comment about the coding conventions. 
> functions and variables are lower case with underscores.  Class names 
> are CamelCase.  It would be convenient if the jvm code used the java 
> coding convention but the rest of it doesn't so this new code is 
> inconsistent.   This is hard to follow as it is!
>
> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/share/vm/runtime/thread.cpp.frames.html 
>
>
> 3826   size_t num_symbol_entries = sizeof(on_unload_symbols) / 
> sizeof(char*);
>
> Why not use ARRAY_SIZE macro?
>
>
> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/share/vm/prims/jvmtiExport.cpp.frames.html 
>
>
> 2208   // Check for builtin agent. If not then if the path is absolute 
> we attempt
> 2209   // to load the library. Otherwise we try to load it from the 
> standard
> 2210   // dll directory.
>
> I think you are missing the word "found" in this sentence.   You are 
> using builtin agent to mean agent defined in a statically linked 
> library, I believe.  Can you say that instead?  Builtin means that the 
> jvm knows about it and implements it but in this case it doesn't.
>
> Maybe find_statically_linked_agent() would be a better name for this 
> function?
>
> Is the is_valid() function really mean has_been_loaded()?
>
> 2236   if (agentLib->valid()) {
>
> Did you run the Internal NSK vm.quick.testlist tests on this to verify 
> that nothing breaks?  There are a lot of tests with different agents 
> and combinations in that suite and it frequently holds some surprises 
> in this area so best to run it first.   Let me know if you need help.
>
> Coleen