Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
On 2012-11-22 19:04, David Holmes wrote: On 22/11/2012 2:17 PM, David Holmes wrote: I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) I humbly withdraw that comment. It is required to be an "integer type" which includes both signed and unsigned and various sizes. I certainly prefer to use only int values. YMMV. I'm happy to keep it as an 'int'. Thanks for verifying though! :) /Mikael David -
Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
On 22/11/2012 2:17 PM, David Holmes wrote: I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) I humbly withdraw that comment. It is required to be an "integer type" which includes both signed and unsigned and various sizes. I certainly prefer to use only int values. YMMV. David -
Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
On 2012-11-22 16:35, David Holmes wrote: Hi Mikael, I prefer this as an internal test too. Though I don't understand why execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - this is an enhancement to the VM interface not the JNI interface isn't it? (Separate issue of course). I think we'll find that when we get a few more tests in there it's time to look at breaking it out into a separate file completely or do it in a completely different way, but that's for another day. I do hope it's soon though. BTW the weird indentation makes more sense viewing frames rather than diffs :) Right :) Looks good to go. Thanks! Eagerly awaiting a second review! /Mikael David On 23/11/2012 10:16 AM, Mikael Vidstedt wrote: I gave it some more thought and decided to try what it would look like having the test be part of the ExecuteInternalVMTests family instead. I like this one better personally, but comments are appreciated! http://cr.openjdk.java.net/~mikael/8003879/webrev.02 Cheers, Mikael On 2012-11-22 14:25, Mikael Vidstedt wrote: New webrev available here: http://cr.openjdk.java.net/~mikael/8003879/webrev.01 Cheers, Mikael On 2012-11-22 14:19, Mikael Vidstedt wrote: On 2012-11-21 20:17, David Holmes wrote: Hi Mikael, On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: Please review the below change. The change for 7045397 introduced a couple of duplicate entries in the vmStructs::localHotSpotVMTypes array. This shows up when using the jmap tool in a rather ugly way: Other than an indentation problem (which I don't think you introduced) this fixup seems fine. For some reason that's the standard for declaring types in that long list - aligning on the first letter of the type name. I followed suit. In addition to removing the two duplicated entries I also added a simple, naive runtime test to walk through and make sure no type is repeated. The VMStructs::init only called in debug_only so there's no startup overhead in product, but it may be better to turn the test into a unit test and only running it as part of ExecuteInternalVMTests. Feedback appreciated! I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) Will fix. - I can't clearly see how the localHotSpotVMTypes array is declared or filled but I assume there is guaranteed to be a sentinel entry at the end so that we don't index past the end? Yeah. I'm not sure you want to know. It took me a few minutes to figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro which generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed to call the macro and actually generate the marker, meaning only the "implementation" of VM_TYPES_OS_CPU actually has the call to last_entry(). Not sure why the end marker isn't just explicitly added at the end instead, but I think I'll queue that up for later. All in all, as far as I can tell there is one and only one end marker and it is generated as part of the expansion of VM_TYPES_OS_CPU. - assert(0, ...) should be assert(false, ...) (as per style guide [1] ;-) ) Will fix. That all said I'm not sure this test belongs there, but I don't feel strongly either way. Me neither :) Cheers, Mikael Cheers, David [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide http://cr.openjdk.java.net/~mikael/8003879/webrev.00/ Cheers, Mikael
Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
Hi Mikael, I prefer this as an internal test too. Though I don't understand why execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - this is an enhancement to the VM interface not the JNI interface isn't it? (Separate issue of course). BTW the weird indentation makes more sense viewing frames rather than diffs :) Looks good to go. David On 23/11/2012 10:16 AM, Mikael Vidstedt wrote: I gave it some more thought and decided to try what it would look like having the test be part of the ExecuteInternalVMTests family instead. I like this one better personally, but comments are appreciated! http://cr.openjdk.java.net/~mikael/8003879/webrev.02 Cheers, Mikael On 2012-11-22 14:25, Mikael Vidstedt wrote: New webrev available here: http://cr.openjdk.java.net/~mikael/8003879/webrev.01 Cheers, Mikael On 2012-11-22 14:19, Mikael Vidstedt wrote: On 2012-11-21 20:17, David Holmes wrote: Hi Mikael, On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: Please review the below change. The change for 7045397 introduced a couple of duplicate entries in the vmStructs::localHotSpotVMTypes array. This shows up when using the jmap tool in a rather ugly way: Other than an indentation problem (which I don't think you introduced) this fixup seems fine. For some reason that's the standard for declaring types in that long list - aligning on the first letter of the type name. I followed suit. In addition to removing the two duplicated entries I also added a simple, naive runtime test to walk through and make sure no type is repeated. The VMStructs::init only called in debug_only so there's no startup overhead in product, but it may be better to turn the test into a unit test and only running it as part of ExecuteInternalVMTests. Feedback appreciated! I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) Will fix. - I can't clearly see how the localHotSpotVMTypes array is declared or filled but I assume there is guaranteed to be a sentinel entry at the end so that we don't index past the end? Yeah. I'm not sure you want to know. It took me a few minutes to figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro which generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed to call the macro and actually generate the marker, meaning only the "implementation" of VM_TYPES_OS_CPU actually has the call to last_entry(). Not sure why the end marker isn't just explicitly added at the end instead, but I think I'll queue that up for later. All in all, as far as I can tell there is one and only one end marker and it is generated as part of the expansion of VM_TYPES_OS_CPU. - assert(0, ...) should be assert(false, ...) (as per style guide [1] ;-) ) Will fix. That all said I'm not sure this test belongs there, but I don't feel strongly either way. Me neither :) Cheers, Mikael Cheers, David [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide http://cr.openjdk.java.net/~mikael/8003879/webrev.00/ Cheers, Mikael
Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
I gave it some more thought and decided to try what it would look like having the test be part of the ExecuteInternalVMTests family instead. I like this one better personally, but comments are appreciated! http://cr.openjdk.java.net/~mikael/8003879/webrev.02 Cheers, Mikael On 2012-11-22 14:25, Mikael Vidstedt wrote: New webrev available here: http://cr.openjdk.java.net/~mikael/8003879/webrev.01 Cheers, Mikael On 2012-11-22 14:19, Mikael Vidstedt wrote: On 2012-11-21 20:17, David Holmes wrote: Hi Mikael, On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: Please review the below change. The change for 7045397 introduced a couple of duplicate entries in the vmStructs::localHotSpotVMTypes array. This shows up when using the jmap tool in a rather ugly way: Other than an indentation problem (which I don't think you introduced) this fixup seems fine. For some reason that's the standard for declaring types in that long list - aligning on the first letter of the type name. I followed suit. In addition to removing the two duplicated entries I also added a simple, naive runtime test to walk through and make sure no type is repeated. The VMStructs::init only called in debug_only so there's no startup overhead in product, but it may be better to turn the test into a unit test and only running it as part of ExecuteInternalVMTests. Feedback appreciated! I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) Will fix. - I can't clearly see how the localHotSpotVMTypes array is declared or filled but I assume there is guaranteed to be a sentinel entry at the end so that we don't index past the end? Yeah. I'm not sure you want to know. It took me a few minutes to figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro which generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed to call the macro and actually generate the marker, meaning only the "implementation" of VM_TYPES_OS_CPU actually has the call to last_entry(). Not sure why the end marker isn't just explicitly added at the end instead, but I think I'll queue that up for later. All in all, as far as I can tell there is one and only one end marker and it is generated as part of the expansion of VM_TYPES_OS_CPU. - assert(0, ...) should be assert(false, ...) (as per style guide [1] ;-) ) Will fix. That all said I'm not sure this test belongs there, but I don't feel strongly either way. Me neither :) Cheers, Mikael Cheers, David [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide http://cr.openjdk.java.net/~mikael/8003879/webrev.00/ Cheers, Mikael
Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
New webrev available here: http://cr.openjdk.java.net/~mikael/8003879/webrev.01 Cheers, Mikael On 2012-11-22 14:19, Mikael Vidstedt wrote: On 2012-11-21 20:17, David Holmes wrote: Hi Mikael, On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: Please review the below change. The change for 7045397 introduced a couple of duplicate entries in the vmStructs::localHotSpotVMTypes array. This shows up when using the jmap tool in a rather ugly way: Other than an indentation problem (which I don't think you introduced) this fixup seems fine. For some reason that's the standard for declaring types in that long list - aligning on the first letter of the type name. I followed suit. In addition to removing the two duplicated entries I also added a simple, naive runtime test to walk through and make sure no type is repeated. The VMStructs::init only called in debug_only so there's no startup overhead in product, but it may be better to turn the test into a unit test and only running it as part of ExecuteInternalVMTests. Feedback appreciated! I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) Will fix. - I can't clearly see how the localHotSpotVMTypes array is declared or filled but I assume there is guaranteed to be a sentinel entry at the end so that we don't index past the end? Yeah. I'm not sure you want to know. It took me a few minutes to figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro which generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed to call the macro and actually generate the marker, meaning only the "implementation" of VM_TYPES_OS_CPU actually has the call to last_entry(). Not sure why the end marker isn't just explicitly added at the end instead, but I think I'll queue that up for later. All in all, as far as I can tell there is one and only one end marker and it is generated as part of the expansion of VM_TYPES_OS_CPU. - assert(0, ...) should be assert(false, ...) (as per style guide [1] ;-) ) Will fix. That all said I'm not sure this test belongs there, but I don't feel strongly either way. Me neither :) Cheers, Mikael Cheers, David [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide http://cr.openjdk.java.net/~mikael/8003879/webrev.00/ Cheers, Mikael
Re: RFR (XS): 8003879: Duplicate definitions in vmStructs
On 2012-11-21 20:17, David Holmes wrote: Hi Mikael, On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: Please review the below change. The change for 7045397 introduced a couple of duplicate entries in the vmStructs::localHotSpotVMTypes array. This shows up when using the jmap tool in a rather ugly way: Other than an indentation problem (which I don't think you introduced) this fixup seems fine. For some reason that's the standard for declaring types in that long list - aligning on the first letter of the type name. I followed suit. In addition to removing the two duplicated entries I also added a simple, naive runtime test to walk through and make sure no type is repeated. The VMStructs::init only called in debug_only so there's no startup overhead in product, but it may be better to turn the test into a unit test and only running it as part of ExecuteInternalVMTests. Feedback appreciated! I have a few comments on this part: - Array indices should be int's not size_t (ie signed not unsigned) Will fix. - I can't clearly see how the localHotSpotVMTypes array is declared or filled but I assume there is guaranteed to be a sentinel entry at the end so that we don't index past the end? Yeah. I'm not sure you want to know. It took me a few minutes to figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro which generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed to call the macro and actually generate the marker, meaning only the "implementation" of VM_TYPES_OS_CPU actually has the call to last_entry(). Not sure why the end marker isn't just explicitly added at the end instead, but I think I'll queue that up for later. All in all, as far as I can tell there is one and only one end marker and it is generated as part of the expansion of VM_TYPES_OS_CPU. - assert(0, ...) should be assert(false, ...) (as per style guide [1] ;-) ) Will fix. That all said I'm not sure this test belongs there, but I don't feel strongly either way. Me neither :) Cheers, Mikael Cheers, David [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide http://cr.openjdk.java.net/~mikael/8003879/webrev.00/ Cheers, Mikael
Re: Review request for fix for 7200297
Bill, Looks good for me. -Dmitry On 2012-11-21 23:31, BILL PITTORE wrote: > Hi Dmitry, > > On 11/21/2012 4:56 AM, Dmitry Samersoff wrote: >> Bill, >> >> >> Few nits: >> >> 1. >> >> dll_build_name is exactly the same for all locations could we place it >> to some common place? > It looked somewhat intentional to have the agents and the hprof code be > self-contained. I looked at using a common file but the makefile changes > and source tree changes seemed a bit much. Hprof code is "unsupported > demo" code and jdwp is supported agent so I went with the current scheme > of having the code be self-contained. >> Also file_exists is not necessary and could be >> replaced with just ::access(buffer,R_OK) (Windows has it as well) > I'll go with the access suggestion above, less code. >> but if you prefer to keep it: >> >> strlen(filename)==0 could be a *filename == 0 >> >> >> 2. We don't need else after return in all *_md.c files >> e.g. linker_md.c:122 > Semantically, you're correct. I think in terms of code readability I > like the 'else' since it makes it clear to the reader that there are two > different cases. C compiler will 'do the right thing'. > Updated the webrev at > http://cr.openjdk.java.net/~bpittore/7200297/webrev.03/ > > thanks, > bill >> >> Otherwise looks good. >> >> -Dmitry >> >> On 2012-11-20 01:10, BILL PITTORE wrote: >>> Have gotten reviews from Serguei Spitsyn for the changes, made some >>> improvements and need an official reviewer to check it out. >>> >>> http://cr.openjdk.java.net/~bpittore/7200297/webrev.02 >>> >>> thanks, >>> bill >>> >>> >>> >>> On 11/14/2012 5:27 PM, bill.pitt...@oracle.com wrote: This bug has to do with the jdwp and hprof agents not parsing the sun.boot.library.path (dll_dir) correctly since the fix for 6819213 went in some years ago. This bug popped up while working on a particular platform that does not have the ability to set LD_LIBRARY_PATH before running the VM. As documented in the bug, on most platforms even if the sun.boot.library.path consists of multiple paths and the jdwp or hprof code fails to load a dependent lib, the system falls back to using LD_LIBRARY_PATH so the failure is masked. On some other platforms, this failover doesn't exist so we get an error when trying to load jdwp and hprof dependent libs. Some notes on a couple of files. * debugInit.c, hprof_init.c*: Rearranged the init order so that the jvmti pointer gets initialized before attempting to load the npt shared library. *linker_md.c, hprof_md.c* Much of the platform code in hprof and jdwp is duplicated and this change is no different. Based on the code in hotspot os_solaris/windows.cpp it parses the boot library path and attempts to find the library. * error_messages.c* Fixed a bug in the error message code that blindly dereferenced the npt pointer even if it wasn't set because of an error in loading. Webrev: http://cr.openjdk.java.net/~bpittore/7200297/webrev.00/ Ran the ute nsk/jdwp nsk/jvmti nsk/hprof tests, the JCK jdwp/jvmti tests and some command line testing on windows and linux. thanks, bill >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
