incubator-kato-spec mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steve Poole <spoole...@googlemail.com>
Subject Re: Apache Kato release - comments please
Date Wed, 02 Dec 2009 14:59:04 GMT
On Wed, Dec 2, 2009 at 5:49 AM, Bobrovsky, Konstantin S <
konstantin.s.bobrovsky@intel.com> wrote:

> Steve,
>
> My comments follow.
>
> Thanks,
> Konst
>
> Intel Novosibirsk
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
>
>
> >> === interface JavaField ===
> >> 1)
> >> During VM crash debugging it is sometimes important to know the offset
> of
> >a
> >> particular field from object start. So, something like
> >>        /** @return for instance fields, offset of the field from
> object's
> >> beginning in
> >> * the memory; -1 if unknown or for static fields */
> >> public int getOffset()
> >>
> >> This makes sense too -  though I think it would be better to  have
> >something like this instead :
> >
> >public ImagePointer getOffset(JavaObject )
> >
> >Gives you more flexibility in the implementation and returns something
> >you're going to need anyway.   The ImagePointer can then be used to look
> at
> >the contents of the object directly.
>
> I'm not quite comfortable with ImagePointer representing an offset of a
> JavaField. 'Offset' is a number of bytes between object start and field
> reference location within the object, whereas ImpagePointer is an absolute
> address within the image. Also, 'offset' is independent entity from the
> process image concept, and can be used without image API. On the other hand,
> unlike Hotspot JVM, the offset of given field can be different in objects of
> different types (subclasses of the class where the field is declared), so I
> like the addition of the JavaObject parameter.
>
> Understood, so  it becomes
 public int getOffset(JavaObject )

>> === interface JavaLocation ===
> ...
> >>        /** @return bytecode index corresponding to position in the code
> >> represented by
> >> * this JavaLocation instance */
> >>        public int getBytecodeIndex()
> >>
> >> Agreed -  perhaps its should be called  getProgramCounter()  or
> something
> >similar?
>
> Sounds good to me.
>
> >> Konst - I'm not sure I really understand the benefit of this item over
> >what
> >already exists.  Could you start a separate thread and provide some more
> >info about sort of usecase you had in mind?
>
> OK, I'll do that.
>
> >> === interface JavaStackFrame ===
> ...
> >> So it would be good to distinguish compiled frames from interpreted ones
> >> (IMO, this is implementable for any JVM)
> >>        public boolean isCompiled();
> >>        public boolean isInterpreted();
> >> In case of interpreted frames, more information might be available.
> >>
> >> Thats a good idea. Do we need both?   What about levels of compilation.
> >You suggested moving getCompilationLevel() to here so can't we use that
> >with
> >perhaps 0 = interpreted?
>
> I suggested to move getCompilationLevel() to a new CompiledMethodCode (or
> something like it), as JavaStackFrame does not have a 'compilation level' -
> it is the code which does. To be strict, 'isCompiled/isInterpreted' is not
> quite accurate too, something like
> 'belongsToCompiledMethod/belongsToInterpretedMethod' better reflects the
> sense, but is pretty awkward.
>
> I agree that JavaMethod is both static and dynamic.  We do need to find
some sort of separation.   Not sure how yet though :-)

>> 2)
> >> This API defines heap roots returned by getHeapRoots queries (e.g.
> >> JavaStackFrame.getHeapRoots) to be JavaReferences. Sometimes it is very
> >> helpful to know where the heap root actually resides - the corresponding
> >CPU
> >> register and/or memory address (in a memory stack frame or register
> stack
> >> frame), and this info is usually available in Java runtimes. Something
> >like
> >>
> >>    /** @return a CPU register name where the reference's target is
> >located
> >> or
> >>     * null if such information is not available */
> >>    public ImageRegister getRegister();
> >>
> >> I understand why you want this  - is it only restricted to references?
> >Are there other places where knowing which register was used is important?
>
> Yes, there are. What comes to mind is:
> - calling convention (in a broad sense) for compiled and interpreted
> managed methods. I.e. where input/output parameters or return value reside
> (which register or stack location) + (the "broad" part - which is entirely
> VM-specific) where additional information is kept, such as current thread
> pointer, method object pointer, stack pointer, etc.
> - calling convention for native methods and VM functions
>
> In some cases knowing these details helps crash analysis.
>
> There are several other places I can think of, but they are very VM
> specific and should probably not be a part of the API.
>
> BTW, "calling convention" is a part of so-called "Application Binary
> Interface" which is unique to a given platform (CPU+OS combination). There
> are other parts in a usual ABI which might be worth abstracting in the Kato
> API
>        - various alignments (code entries, values of various data types,
> method's stack frame). Misalignments are usually a good sign of an error. To
> develop this little further, a JavaRuntime created from a process image dump
> could have a "verify" method which would check all known "invariants" -
> alignments, argument types, range-checks for objects (must be in a heap),
> code pieces (must be in some "code cache"), etc.
>        - ...
>
> >
> >   /** @return  a location within this image where the reference's target
> >is
> >>     * located or null if such information is not available */
> >>    public ImagePointer.getImageLocation();
> >>
> >> Agree with this idea  -  I would probably go with something a little
> more
> >generic though :
> >
> >     public ImagePointer getAddress();
> >
>
> Yes, sounds good.
>
> >-----Original Message-----
> >From: Steve Poole [mailto:spoole167@googlemail.com]
> >Sent: Monday, November 30, 2009 5:32 PM
> >To: kato-spec@incubator.apache.org
> >Subject: Re: Apache Kato release - comments please
> >
> >Konst - by replies to you comments below.
> >
> >
> >On Wed, Nov 25, 2009 at 1:55 PM, Bobrovsky, Konstantin S <
> >konstantin.s.bobrovsky@intel.com> wrote:
> >
> >> Hi Stuart, all
> >>
> >> As we are approaching the end of the public review, I once again
> reviewed
> >> the specification, the results are below. These are some my older
> >comments
> >> which have not yet been anyhow resolved plus few new. I could volunteer
> >to
> >> develop some of the suggestions further and prepare a complete source
> >> updates for the parts which I suggest to change. BTW, I also entered
> >these
> >> comments into the JCP comments page for this JSR:
> >> http://wiki.jcp.org/boards/index.php?t=2852
> >>
> >> Thanks,
> >> Konst
> >>
> >> Intel Novosibirsk
> >> Closed Joint Stock Company Intel A/O
> >> Registered legal address: Krylatsky Hills Business Park,
> >> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> >> Russian Federation
> >>
> >> === interface ImageSymbol ===
> >> 1)
> >> Add 'public int getSize()' method which would return a number of bytes
> >> occupied by the symbol in the image.
> >>
> >> That does make sense -  perhaps we should also consider adding in a
> >getType()  method ?
> >
> >=== interface ImageStackFrame ===
> >> 1)
> >> API also needs to support 'split' stack frames: e.g. on Itanium every
> >frame
> >> consists of a traditinal memory stack frame and additionally, a register
> >> stack frame where Register Stack Engine (RSE) spills current register
> >frame
> >> during a call (and restores from upon return). The easiest way probably
> >is
> >> to mention in the spec something like "implementations can extend this
> >> interface to provide data specific to a particular platform, such as
> >> register stack frame location on Itanium (r), or frame size").
> >>
> >> Thanks - you've given as an easy way out :-)
> >
> >
> >> === interface JavaField ===
> >> 1)
> >> During VM crash debugging it is sometimes important to know the offset
> of
> >a
> >> particular field from object start. So, something like
> >>        /** @return for instance fields, offset of the field from
> object's
> >> beginning in
> >> * the memory; -1 if unknown or for static fields */
> >> public int getOffset()
> >>
> >> This makes sense too -  though I think it would be better to  have
> >something like this instead :
> >
> >public ImagePointer getOffset(JavaObject )
> >
> >Gives you more flexibility in the implementation and returns something
> >you're going to need anyway.   The ImagePointer can then be used to look
> at
> >the contents of the object directly.
> >
> >
> >> === interface JavaLocation ===
> >> 1)
> >> The spec says that it "Represents a point of execution within a Java
> >> method", however it does not provide access to bytecode index of
> >currently
> >> executed Java instruction, which is the only available (sensible)
> current
> >> Java code position description for interpreted frames, and an invaluable
> >> complement for compiled method's current code address. So, I would
> >suggest
> >> to also have
> >>        /** @return bytecode index corresponding to position in the code
> >> represented by
> >> * this JavaLocation instance */
> >>        public int getBytecodeIndex()
> >>
> >> Agreed -  perhaps its should be called  getProgramCounter()  or
> something
> >similar?
> >
> >2)
> >> getCompilationLevel() suits more to the piece of code which encloses
> this
> >> JavaLocation, and does not seem to fit logically well here
> >>
> >> Agreed -  see comments later about this.
> >
> >
> >> 3)
> >> JavaMethod getMethod() does not provide easy way to identify piece of
> >code
> >> this location belongs to. Another method
> >>        /** @return a description of a logical chunk of code this
> location
> >> belongs to */
> >>        public CodeStub getContainingCodeStub()
> >> would achieve this. Additionally, I think that a "piece of generated
> >code"
> >> is important concept within any JVM to dispense a specific interface for
> >it
> >> - CodeStub.
> >>
> >>  interface CodeStub {
> >>    String getName();
> >>    ImageSection getCode();
> >>  }
> >> CodeStub which a compiled method would extend from it:
> >>
> >> CompiledMethodCode extends CodeStub {
> >>        JavaMethod getMethod();
> >>        int getCompilationLevel();
> >>        /** @return (for OSR compilations - bytecode index of the entry
> >> point into
> >>  * this method) */
> >>        int getEntryPointBytecodeIndex();
> >>        // plus maybe GC maps (all implementaions) and other
> >> implementation-specific info
> >> }
> >>
> >> Konst - I'm not sure I really understand the benefit of this item over
> >what
> >already exists.  Could you start a separate thread and provide some more
> >info about sort of usecase you had in mind?
> >
> >
> >> === interface JavaMethod ===
> >> 1)
> >>        /** Get the set of ImageSections containing the compiled code of
> >> this method. */
> >> List getCompiledSections()
> >>
> >>
> >> I would suggest to change to
> >>        /** Get the set of CompiledMethodCode objects containing the
> >> compiled code of
> >>  *  this method. */
> >> List getCompiledSections()
> >>
> >> Understood -  this is tied up with the item above so we can resolve them
> >> together.
> >>
> >
> >
> >> === interface JavaStackFrame ===
> >> 1)
> >> Usually, Java stack frames contain information about Java monitors held
> >in
> >> this frame. This is necessary because Java spec requires "forced"
> release
> >of
> >> all monitors held by a method in case it "completes abruptly". So
> >something
> >> like
> >>        /** @return list of JavaObject instances representing monitors
> >held
> >> by
> >>  * this method activation */
> >> List getHeldMonitors()
> >> would be really useful.
> >>
> >> Agreed - this would be very useful.
> >
> >
> >> 2)
> >> Important case of JavaStackFrame is interpreted frames - in this case
> >> current position in the machine code is not that important (this
> position
> >is
> >> not unique to a method) as current position in the bytecode being
> >executed.
> >> So it would be good to distinguish compiled frames from interpreted ones
> >> (IMO, this is implementable for any JVM)
> >>        public boolean isCompiled();
> >>        public boolean isInterpreted();
> >> In case of interpreted frames, more information might be available.
> >>
> >> Thats a good idea. Do we need both?   What about levels of compilation.
> >You suggested moving getCompilationLevel() to here so can't we use that
> >with
> >perhaps 0 = interpreted?
> >
> >
> >
> >> 3)
> >> It would be of great help to an engineer investigating a crash in
> >compiled
> >> code or some Java-level runtime error to know the JVM virtual state (in
> >JVMS
> >> sense) for all Java stack frames, i.e.:
> >> -       held monitors
> >> -       current bytecode index
> >> -       values of all local variables
> >> -       content of the expression stack
> >>
> >> and some JVMs can provide such info even for compiled frames. I think it
> >is
> >> desirable to add something like (removing getHeldMonitors):
> >>
> >>        public JVMState getCurrentJVMState();
> >>
> >>
> >> === interface JavaReference ===
> >> 1)
> >> HEAP_ROOT_*, REFERENCE_* and other constants are omitted from the PDF
> >> document
> >>
> >> ok - thanks for spotting that.
> >
> >
> >> 2)
> >> This API defines heap roots returned by getHeapRoots queries (e.g.
> >> JavaStackFrame.getHeapRoots) to be JavaReferences. Sometimes it is very
> >> helpful to know where the heap root actually resides - the corresponding
> >CPU
> >> register and/or memory address (in a memory stack frame or register
> stack
> >> frame), and this info is usually available in Java runtimes. Something
> >like
> >>
> >>    /** @return a CPU register name where the reference's target is
> >located
> >> or
> >>     * null if such information is not available */
> >>    public ImageRegister getRegister();
> >>
> >> I understand why you want this  - is it only restricted to references?
> >Are there other places where knowing which register was used is important?
> >
> >   /** @return  a location within this image where the reference's target
> >is
> >>     * located or null if such information is not available */
> >>    public ImagePointer.getImageLocation();
> >>
> >> Agree with this idea  -  I would probably go with something a little
> more
> >generic though :
> >
> >     public ImagePointer getAddress();
> >
> >
> >
> >> would be helpful.
> >>
> >> === interface JavaRuntime ===
> >>
> >> 1) It should be possible in the API to iterate trough generated code
> >> sequences other than managed methods - so-called 'stubs'. Each stub is
> >> similar to a method: it has a name, (possibly, multiple) code sections.
> >This
> >> interface (JavaRuntime) seems an appropriate place for this API:
> >>        /** @return  a list of CodeStub objects representing code
> >sequences
> >>  * generated internally by the JVM */
> >>        public List getJVMCodeStubs();
> >>
> >> (2) Not all methods within a Java runtime are necessarily compiled. Many
> >> may remain interpreted, others might not even have a bytecode. So I
> would
> >> suggest to replace 'getCompiledMethods' with
> >>        /** @return a list of JavaMethod objects representing managed
> >> methods known
> >>  * to the system. Each JavaMethod may be either interpreted or compiled
> >or
> >>   native. Method's code for compiled methods can be quired via @see
> >>  #JavaMethod.getCompiledSections() */
> >> public List getJavaMethods();
> >>
> >> === Common Register Names ===
> >> 1)
> >> 'Long' or 'Integer' is too vague for specifying register type. It is
> >better
> >> to specify the width of the register - 256 (future Intel architectures)
> /
> >> 128 (SSE registers) / 80 (FP registers in ia32, Intel64, IA64)/ 64 / 32
> >bit
> >> - plus an additional characteristic such as 'general purpose/floating
> >> point/application/flags/...'
> >>
> >> Understood
> >
> >
> >> 2)
> >> The table title "Table A.2. IA64 Register Names" is wrong, it should
> read
> >> either "Intel64 Register Names" or "AMD64 Register Names", as "IA64" is
> >> Itanium architecture.
> >>
> >> Understood
> >
> >> 3)
> >> For the complete list of recommended register names and their
> >> specifications for ia32 and Intel64 architectures I suggest add a link
> to
> >>
> >> Intel(r) 64 and IA-32 Architectures Software Developer's Manual
> >(September
> >> 2009), Volume 1:Basic Architecture, Sections "3.4 BASIC PROGRAM
> EXECUTION
> >> REGISTERS" and "3.5 INSTRUCTION POINTER"
> >> http://www.intel.com/Assets/PDF/manual/253665.pdf
> >>
> >> Agreed
> >
> >> 4)
> >> I suggest to add a new section "IA64 Register Names" with a link to the
> >> official Itanium(r) 2 specification for the recommended IA64 register
> >names
> >> and their specifications, which is
> >>
> >> Intel(r) Itanium(r) Architecture Software Developer's Manual (rev 2.2),
> >> Volume 1: Application Architecture, Section 3.1 "Application Register
> >State"
> >> http://download.intel.com/design/Itanium/manuals/24531705.pdf
> >>
> >> Agreed
> >>
> >
> >
> >> >-----Original Message-----
> >> >From: Stuart Monteith [mailto:stukato@stoo.me.uk]
> >> >Sent: Friday, November 20, 2009 11:29 PM
> >> >To: kato-dev@incubator.apache.org
> >> >Subject: Apache Kato release - comments please
> >> >
> >> >Hello all,
> >> >     Steve and I have been working on getting the codebase ready for a
> >> >release. This is a non-trivial activity with lots of things to get
> >right.
> >> >
> >> >Given the state of the project as it is just now, we can't claim to be
> >> >doing a 1.0 release, instead I'm following the Milestone convention.
> >> >
> >> >I'm not proposing that what I've assembled is submitted as a release
> >> >candidate just yet, as there is still work to do on it.
> >> >
> >> >If you look here, you'll find the files that would make up a release:
> >> >
> >> http://people.apache.org/~monteith/kato/apache-kato-M1-incubating-<http://people.apache.org/%7Emonteith/kato/apache-kato-M1-incubating->
> >RC1/<http://people.apache.org/%7Emonteith/kato/apache-kato-M1-incubating-
> >RC1/>
> >> >
> >> >I've also created a tag:
> >> >
> >> >https://svn.apache.org/repos/asf/incubator/kato/tags/apache-kato-M1-
> >> >incubating-RC1/
> >> >
> >> >
> >> >Obvious things that are wrong:
> >> >     apache-kato-M1-incubating-tomcat.demos - packages are undocumented
> >> >and missing some files.
> >> >     The documentation - it needs more work, but I'd be interested in
> >> >some cursory feedback.
> >> >     The CJVMTI agent needs work under Linux - it's missing some
> >> >information.
> >> >     The TCK is still under development.
> >> >     Some filtering needs to be applied to the rat reports.
> >> >     There are some spurious files, like readme.html.
> >> >
> >> >I have found this a very useful exercise, as this is the first that
> >> >we've packaged the projects contents and seen what it actually
> includes,
> >> >so this should give everyone a good opportunity to have a look and see.
> >> >
> >> >
> >> >Regards,
> >> >     Stuart
> >> >
> >> >--
> >> >Stuart Monteith
> >> >http://blog.stoo.me.uk/
> >>
> >>
> >
> >
> >--
> >Steve
>



-- 
Steve

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message