incubator-kato-spec mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bobrovsky, Konstantin S" <konstantin.s.bobrov...@intel.com>
Subject RE: Apache Kato release - comments please
Date Wed, 02 Dec 2009 05:49:29 GMT
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.

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

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

Mime
View raw message