Re: [Patch] Java: Add heap dump and analyze support.

[David Daney <ddaney@avtrex.com>]

Boehm, Hans wrote:
> I think the only partial excuse for the ugly way this is handled in the
> GC is that it tries to avoid stdio, in order to avoid accidentally
> calling malloc.  This probably doesn't matter for gcj, but it's
> important if malloc is actually an alias for GC_malloc, which it
> sometimes is.
> 
> Nonetheless, this should probably be parameterized with respect to file
> descriptors (probably not FILE *) on Posix systems, and the approximate
> equivalent on other systems.
> 
> GC7 handles this slightly better, but not a lot.
> 

I would propose that we commit the patch much as it is.

If the GC could add an entry point to allow the information to be 
printed to an arbitrary FILE* or file descriptor, then we would adjust 
the heap dumper when that is imported into GCC.

Does that sound reasonable?

David Daney

> Hans
> 
>> -----Original Message-----
>> From: java-patches-owner@gcc.gnu.org 
>> [mailto:java-patches-owner@gcc.gnu.org] On Behalf Of David Daney
>> Sent: Monday, January 29, 2007 3:58 PM
>> To: tromey@redhat.com
>> Cc: Java Patch List; gcc-patches@gcc.gnu.org; Johannes P. Schmidt
>> Subject: Re: [Patch] Java: Add heap dump and analyze support.
>>
>> New version of the patch:
>>
>> Tom Tromey wrote:
>>>>>>>> "David" == David Daney <ddaney@avtrex.com> writes:
>>>>>>>>             
>>> Nice.
>>>
>>> I read through this.  I don't see many issues -- a few formatting 
>>> things (on irc you said these are fixed).
>>>
>>> I'm a little concerned about the ad hoc approach to demangling, but 
>>> not enough to want to change it.
>>>
>>> I'm also worried about the GC private bits copied into the native 
>>> code.  My main concern is that if the GC changes, we won't notice.
>>>   
>> As I said before: I look at this quite a bit.  The GC has 
>> code that prints the needed information to stdout.  
>> Unfortunately we need it in a file.  I was thinking about 
>> modifying the GC so that you could supply a file descriptor, 
>> but that code has #ifdefs for every kind of OS known to man, 
>> and I was afraid I would break some obscure untestable configuration.
>>> My final high-level worry is about security.  Right now it 
>> is possible 
>>> for any program to call the code that dumps the memory map. 
>>  I suppose 
>>> we can just sweep this under the blanket we're putting 
>> 'gnu.classpath'
>>> stuff under.
>>>
>>>   
>> Fixed.
>>> I have a few more minor comments.
>>>
>>> David> +@item -p @var{tool-prefix}
>>> David> +Prefix added to the names of the @command{nm} and 
>> @command{readelf} commands.
>>> I'd prefer to compile in the target, but I suppose this tool isn't 
>>> built in non-native builds, so that wouldn't help.  Darn.
>>>
>>> David> +  // search all symbol tables
>>> David> +  // return -1 if none found
>>> David> +  long getAddress(SymbolLookup lookup, String 
>> symbol) throws 
>>> David> + IOException  {
>>> David> +    //    for (Iterator 
>> it=map.entrySet().iterator(); it.hasNext(); )
>>> David> +    //      {
>>>
>>> Why is this commented out?
>>>   
>> Unused code.  It has been removed.
>>> David> +        if (s.charAt(0) == '#')
>>> David> +          continue;
>>> David> +        if (s == null)
>>> David> +          break;
>>>
>>> These tests are in the wrong order.
>>>   
>> Fixed.
>>> David> +  static void usage()
>>> David> +  {
>>>
>>> Someday I suppose we should convert the tools in gcj to use the 
>>> classpath getopt code.
>>>   
>> Done.
>>> David> +              if 
>> (name.compareTo("gnu.gcj.runtime.MethodRef") 
>>> David> + == 0)
>>>
>>> What is this?
>>>   
>> Bogus code left over from 3.4.3 version.  It has been removed.
>>> David> +  public static String KindToName(int kind)
>>>
>>> Non-java name... method names should start with a lower case letter.
>>>   
>> Fixed.
>>> David> +namespace gnu
>>> David> +{
>>> David> +  namespace gcj
>>> David> +  {
>>> David> +    namespace util
>>> David> +    {
>>> David> +      class GC_enumerator
>>>
>>> This appears to be local to a single .cc file.  Do we need 
>> to put it 
>>> into a namespace like this?  I've always thought of these 
>> namespaces 
>>> as reserved for 'extern "Java"' code.
>>>   
>> My misunderstanding of namespaces.  Changed to anonymous 
>> namespace as suggested by Joel Dice and Chris Lattner.
>>
>> Tested on x86_64-pc-linux-gnu (FC6) with no regressions.  
>> Currently testing on i686-pc-linux-gnu.
>>
>> OK to commit if it passes?
>> 2007-01-29  David Daney  <ddaney@avtrex.com>
>> gcc/java:
>>     * Make-lang.in (JAVA_MANFILES): Added doc/gc-analyze.1.
>>     (java.maintainer-clean):Added gc-analyze.1.
>>     (.INTERMEDIATE): Added gc-analyze.pod.
>>     (gc-analyze.pod): New rule.
>>     (java.install-man): Install gc-analyze.1
>>     * gcj.texi: Added new section for the gc-analyze program.
>>
>> libjava:
>> 2007-01-29  Johannes Schmidt  <jschmidt@avtrex.com>
>>     David Daney  <ddaney@avtrex.com>
>>
>>     * configure.ac: Added check for /proc/self/maps.
>>     * Makefile.am (bin_PROGRAMS): Added gc-analyze.
>>     (AM_GCJFLAGS): Set classpath to $(srcdir)/classpath/tools/classes.
>>     (gcjh.stamp): Same.
>>     (gc_analyze_SOURCES): New.
>>     (gc_analyze_LDFLAGS): New.
>>     (gc_analyze_LINK): New.
>>     (gc_analyze_LDADD): New.
>>     (gc_analyze_DEPENDENCIES): New.
>>     (nat_source_files): Added gnu/gcj/util/natGCInfo.cc.
>>     * Makefile.in: Regenerated.
>>     * configure: Regenerated.
>>     * include/config.h.in: Regenerated.
>>     * sources.am: Regenerated.
>>     * scripts/makemake.tcl: Don't include gc-analyze classes 
>> in libgcj.
>>     * gnu/classpath/tools/getopt/Parser.h: New.
>>     * gnu/classpath/tools/getopt/FileArgumentCallback.h: New.
>>     * gnu/classpath/tools/getopt/Parser$1.h: New.
>>     * gnu/classpath/tools/getopt/Parser$2.h: New.
>>     * gnu/classpath/tools/getopt/Parser$3.h: New.
>>     * gnu/classpath/tools/getopt/OptionGroup.h: New.
>>     * gnu/classpath/tools/getopt/OptionException.h: New.
>>     * gnu/classpath/tools/getopt/Messages.h: New.
>>     * gnu/classpath/tools/getopt/Option.h: New.
>>     * 
>> gnu/gcj/tools/gc_analyze/MemoryAnalyze$SubstringComparator.h: New.
>>     * gnu/gcj/tools/gc_analyze/SymbolLookup.java: New.
>>     * gnu/gcj/tools/gc_analyze/BytePtr.h: New.
>>     * gnu/gcj/tools/gc_analyze/ItemList.h: New.
>>     * gnu/gcj/tools/gc_analyze/ToolPrefix.h: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryAnalyze.h: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryAnalyze$1.h: New
>>     * gnu/gcj/tools/gc_analyze/MemoryAnalyze$2.h: New
>>     * gnu/gcj/tools/gc_analyze/MemoryAnalyze$3.h: New
>>     * gnu/gcj/tools/gc_analyze/BlockMap$SizeKind.h: New.
>>     * gnu/gcj/tools/gc_analyze/ObjectMap.java: New.
>>     * gnu/gcj/tools/gc_analyze/SymbolLookup.h: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryMap.java: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryAnalyze$1$Info.h: New.
>>     * gnu/gcj/tools/gc_analyze/ObjectMap.h: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryMap.h: New.
>>     * gnu/gcj/tools/gc_analyze/SymbolTable.java: New.
>>     * gnu/gcj/tools/gc_analyze/SymbolTable.h: New.
>>     * gnu/gcj/tools/gc_analyze/ObjectMap$ObjectItem.h: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryMap$RangeComparator.h: New.
>>     * gnu/gcj/tools/gc_analyze/BlockMap$PtrMarks.h: New.
>>     * gnu/gcj/tools/gc_analyze/BlockMap.java: New.
>>     * gnu/gcj/tools/gc_analyze/BytePtr.java: New.
>>     * gnu/gcj/tools/gc_analyze/ItemList.java: New.
>>     * gnu/gcj/tools/gc_analyze/ToolPrefix.java: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryAnalyze.java: New.
>>     * gnu/gcj/tools/gc_analyze/MemoryMap$Range.h: New.
>>     * gnu/gcj/tools/gc_analyze/BlockMap.h: New.
>>     * gnu/gcj/util/GCInfo.java: New.
>>     * gnu/gcj/util/GCInfo.h: New.
>>     * gnu/gcj/util/natGCInfo.cc: New.
>>     * gnu/gcj/util/UtilPermission.java: New.
>>     * gnu/gcj/util/UtilPermission.h: New.
>>     * classpath/lib/Makefile.am (gcj_tools_classpath): New variable.
>>     (compile_classpath): Add $(gcj_tools_classpath) to classpath.
>>     * classpath/lib/Makefile.in: Regenerated.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/SymbolTable.class: New.
>>     * 
>> classpath/lib/gnu/gcj/tools/gc_analyze/ObjectMap$ObjectItem.class: 
>> New.
>>     *
>> classpath/lib/gnu/gcj/tools/gc_analyze/MemoryMap$RangeComparat
>> or.class: New.
>>     * 
>> classpath/lib/gnu/gcj/tools/gc_analyze/BlockMap$PtrMarks.class: New.
>>     * 
>> classpath/lib/gnu/gcj/tools/gc_analyze/MemoryMap$Range.class: New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/BlockMap.class: New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/BytePtr.class: New.
>>     *
>> classpath/lib/gnu/gcj/tools/gc_analyze/MemoryAnalyze$Substring
>> Comparator.class: 
>> New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/ItemList.class: New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/ToolPrefix.class: New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/MemoryAnalyze.class: New.
>>     * 
>> classpath/lib/gnu/gcj/tools/gc_analyze/BlockMap$SizeKind.class: New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/SymbolLookup.class: New.
>>     * 
>> classpath/lib/gnu/gcj/tools/gc_analyze/MemoryAnalyze$1$Info.class: 
>> New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/ObjectMap.class: New.
>>     * classpath/lib/gnu/gcj/tools/gc_analyze/MemoryMap.class: New.
>>     * classpath/lib/gnu/gcj/util/GCInfo.class: New.
>>     * classpath/lib/gnu/gcj/util/UtilPermission.class: New.
>>
>> As before machine generated files are omitted from the diff.
>>
>>