Code review for SA changes to allow for new processor architectures (7154641)

[Staffan Larsen]

Thanks for fixing my comments and providing good reasons for your design. I'm ok with these changes now. 

What the future of SA is remains to be seen, but it's not going to be replaced very soon.

Thanks,
/Staffan

On 22 aug 2012, at 18:13, BILL PITTORE <bill.pittore at oracle.com> wrote:

> Updated the webrev at http://cr.openjdk.java.net/~bpittore/7154641/webrev.01/
> 
> On 8/22/2012 4:20 AM, Staffan Larsen wrote:
>> I like the idea of using reflection. How much work would it be to make the change for the existing platforms as well? I don't really like that there are two different code paths. Also, you only made the change for Linux.
> We only made changes to get some embedded platforms supported. Right now that's only Linux. Someone else asked a similar question about the code paths. We (embedded) didn't want to make too many changes to already running/working code for x86, amd64 and sparc on Linux, BSD, Windows, and solaris. It was also our understanding that your team is in the process of evaluating where to go with SA so it seemed best to just make the minimal changes. If using reflection works into your plans going forward someone (your team?) could implement it for all os/cpu combos.
>> Some other comments:
>> 
>> LinuxCDebugger.java - This change would return null on non-supported cpus instead of throwing an exception with an error message. The error message is more user-friendly.
> I added a comment here. The exception for unknown cpu would be thrown by LinuxThreadContextFactory.java. I changed the message in that file as per below.
>> 
>> LinuxDebuggerLocal.c and libproc.h - I don't understand why these changes were made. Probably came from some other change?
> In the case of a new cpu, there would be a platform specific version of
> 
> Java_sun_jvm_hotspot_debugger_linux_LinuxDebuggerLocal_getThreadIntegerRegisterSet0
> 
> that would be called by the Java code to get the processor registers. Likewise, the functions throw_new_debugger_exception() and get_proc_handle() are exported to the platform specific code.
>> 
>> LinuxThreadContextFactory.java, RemoteDebuggerClient.java, HotSpotAgent.java, HTMLGenerator.java - include the name of the CPU or machine type that wasn't found in the exception message
> Fixed.
>> 
>> VM.java and vmStructs.cpp - Looks like an unrelated change.
> Actually needed for stack walking on PPC.
>> saproc.make:94 - weird indentation
> Fixed.
> 
> bill
>> 
>> Thanks,
>> /Staffan
>> 
>> 
>> On 21 aug 2012, at 23:47, BILL PITTORE<bill.pittore at oracle.com>  wrote:
>> 
>>> These changes allow for the (easier) addition of new processor types to SA other than the standard x86, amd64 and sparc. By using reflection, it is not necessary to instantiate the new class directly in the existing code. Rather the class name is derived from the cpu/os name and is loaded and the constructor called. Note that the existing cpus (x86, amd64, and sparc) code was not modified. Only newly added cpus would go through the reflection code path.
>>> 
>>> http://cr.openjdk.java.net/~bpittore/7154641/webrev.00/
>>> 
>>> thanks,
>>> bill
>>> 
>