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

[David Daney <ddaney at avtrex dot com>]

Tom Tromey wrote:
> > > > > > "David" == David Daney <ddaney@avtrex.com> writes:
>
> Sorry for the delay on this.
>
> David> This patch adds the ability to dump the contents of the runtime heap
> David> to a file from a java program.
>
> 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.

There are several ad hoc things done in the analyzer.  My feeling was 
that if issues arise, we can fix them on a case by case basis.

> 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.

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.

I forget exactly why gnu.classpath.SystemProperties is not visible to 
user code, but its documentation states that it is not.  Who am I to argue?

I will add an explicit check to see if the caller has permission to call 
 things in GCInfo.


> Please comment on these.
>
>
> 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.


The gc-analyze program is target independent.  The same build of it can 
analyze dumps from x86_64-linux and mipsel-linux.  It should work with 
big-endian dumps as well, but I have not tested it.

> David> +  // search all symbol tables
> David> +  // return -1 if none found
> David> +  long getAddress(SymbolLookup lookup, String symbol) throws IOException
> David> +  {
> David> +    //    for (Iterator it=map.entrySet().iterator(); it.hasNext(); )
> David> +    //      {
>
> Why is this commented out?

I think it is related to the un-exec capability that I stripped out of 
this version of the analyzer.  I was going to submit that later.  I will 
clean it up.

> David> +        if (s.charAt(0) == '#')
> David> +          continue;
> David> +        if (s == null)
> David> +          break;
>
> These tests are in the wrong order.

Yes they are.  Case in point as to why code review is a good idea.


> David> +  static void usage()
> David> +  {
>
> Someday I suppose we should convert the tools in gcj to use the
> classpath getopt code.
>
> David> +              if (name.compareTo("gnu.gcj.runtime.MethodRef") == 0)
>
> What is this?


Ha! Good catch.  Leftover from the 3.4.3 based version.  Interesting 
classes with gnu.gcj.RawData don't have their references traversed. 
This was a special case we added to find some long forgotten problem.

The class no longer exists in 4.3 and the code was not 32/64 bit safe 
anyhow.  I will remove it.

> David> +  public static String KindToName(int kind)
>
> Non-java name... method names should start with a lower case letter.


OK.  I missed that one.

> 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.

I didn't want to pollute the global namespace of libgcj.  We know what 
is in gnu.gcj.util so there will be no collisions there.

I will generate a new patch and retest.

David Daney.