Re: oprofile and jitted code -- proposal 1

[John L. <le...@mo...>]

On Wed, Apr 27, 2005 at 05:45:52PM -0600, Scott T Jones wrote:

> * The op_file_is_anon routine is still being called from
> libutil++/file_manip.cpp.  Could you please tell us where we should make
> the changes necessary to remove this layering problem?  We are not
> sufficiently familiar with the structure of Oprofile to know the best way
> to do this.

This won't be necessary with the changes done in the branch.

I'm disappointed that my comments regarding the agent API have been
apparently ignored, indeed I consider it a necessary requirement before
I consider merging this onto the branch. To reiterate: dealing with
temporary files is not an acceptable interface for agents. Mainly
because this interface *is* going to change (currently, I'm planning on
the unix domain socket approach).

I'd really appreciate:

1) a patch with libagent/ as proposed, unless there are particular
   objections, in which case I'd love to hear them

2) a patch against the BRANCH_ANON_MAPPING CVS branch, which already
   includes the necessary daemon changes: in particular, you no longer
   need op_jit_vma_to_offset() called from the daemon: all you need is to
   generate the ELF file with the appropriate path, which is currently
   something like:

   /var/lib/oprofile/samples/current/{anon}/usr/jre/bin/java-<tgid>

   I understand that I've played "moving target" on you a little here,
   but all this helps bring the feature to what we both require.

   It should be at most a couple of hours' work, it's mostly moving
   code about.

Once we have this, then we'll have a baseline to start from. I don't yet
think this stuff is finished. Naturally, I'll be happy to include you on
my thoughts on where the design needs to go.

> * The User's Guide and HACKING file need to be updated.  However, I do not
> know where these files are.  I could not find them in the 0.8.2
> distribution.  Our user documentation will closely resemble the text at
> the beginning of this note.  Would that be sufficient?

HACKING is in CVS. The user guide is doc/oprofile.xml. The above would
be a start, but we should mention restrictions etc.

> * There are 42 occurrences of camelCase labels in op_jan.c, all of which
> were defined in jvmpi.h and are out of our control.  I can easily add
> these to the camel_ok array in check_style.py, but I did not know how I
> should include that in the patch.

I'll handle that.

> * We only support a version of Java which matches Oprofile in 32/64-bits.
> The common routines that we call in libop.a must be called from both the
> daemon and java.  To do otherwise would require building unique 32 and
> 64-bit version of libop.a.

We can revisit this.

> * The decision of how to define X_ARCH and X_MACH to call
> bfd_set_arch_mach is still being made in op_jdl_internal.h.  We do not
> know of a better way to do this.

Fine - the defines should probably go into bfd_support.h then, where we
hide all the BFD gunk.

> * How do I demand the existence of PATH_MAX in the headers?  Isn't that
> what I am doing with the #ifndef?

Just #include <limits.h>, that's all you need.

> * The developer intended the data marked as "JIT logger instance data" to
> be unique per process and does not think it should be static.

This makes no sense. ~Is there some particular reason that I'm not
actually speaking directly to the people responsible for the work?

> * basename(x) treats x as a char const *, so this is not a problem.

Whilst this may be currently true, it's obscenely ugly to be passing a
truly const string to a non-const interface.

regards
john