Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Christian, Christian Thalinger wrote: On Fri, 2005-12-02 at 12:22 +0100, Nicolas Geoffray wrote: gnu/java/lang/instrument. Would you like InstrumentationImpl to be in vm/reference/java/lang/InsutrumentationImpl? This sounds weird. We could also make the callTransformers method public. I think it's very helpful for vm implementors that all vm interface stuff is _only_ located in vm/reference. Just my 2 cents... I couldn't agree more. What surprised me is not the vm/reference/* emplacement Mark suggested, but that VMInstrumentationImpl and Instrumentation had to go in the java.lang package, whereas I was more thinking to gnu.java.lang package. But Mark is right when he says it will be easier with security and package private method issues to have the files in the java.lang package. Cheers, Nicolas ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Nicolas, On Fri, 2005-12-02 at 17:52 +0100, Nicolas Geoffray wrote: > Last RFC, and I'm commiting this in > > 2005-12-02 Nicolas Geoffray <[EMAIL PROTECTED]> > > * java/lang/instrument: New directory. > * java/lang/instrument/ClassDefinition.java: > New file. > * java/lang/instrument/ClassFileTransformer.java: > New file. > * java/lang/instrument/IllegalClassFormatException.java: > New file. > * java/lang/instrument/Instrumentation.java: > New file. > * java/lang/instrument/UnmodifiableClassException.java: > New file. > * vm/reference/java/lang/InstrumentationImpl.java: > New file. > * vm/reference/java/lang/VMInstrumentationImpl.java: > New file. Looks very nice! Thanks. One change request: - Please put only VMInstrumentationImpl into vm/reference/java/lang and put InstrumentationImpl (which is shared and for which we don't expect people to change it per VM) into java/lang/ directly. Two nitpicks: - We check for null in InstrumentationImpl.getObjectSize() so the VMInstrumentationImpl.getObjectSize() documentation should say that. - There should be a space after if, while and synchronized according to our coding style guides. When you commit this can you please send a little message explaining the new instrumentation addition and how runtime hackers are supposed to provide support for it to the main classpath mailinglist? That way we hopefully get more feedback and make sure runtime hackers aren't completely surprised when this pops up in the next (generics) release. Thanks, Mark ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
On Fri, 2005-12-02 at 12:22 +0100, Nicolas Geoffray wrote: > gnu/java/lang/instrument. Would you like InstrumentationImpl > to be in vm/reference/java/lang/InsutrumentationImpl? This sounds > weird. We could also make the callTransformers method public. I think it's very helpful for vm implementors that all vm interface stuff is _only_ located in vm/reference. Just my 2 cents... TWISTI ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Mark Wielaard wrote: Yes, but it makes it easier to share code with ClassLoader and VMClassLoader if we do this. Then callTransformers doesn't have to be public, but can just be package private. Then we don't have to worry about whether or not someone can get insecure access to a IntrumentationImpl (although I admit that is not very likely). It just seems more natural to me to have all implementation classes in the same package because they seem very related. Allright then, VMInstrumentationImpl and InstrumentationImpl are in vm/reference/java/lang Last RFC, and I'm commiting this in 2005-12-02 Nicolas Geoffray <[EMAIL PROTECTED]> * java/lang/instrument: New directory. * java/lang/instrument/ClassDefinition.java: New file. * java/lang/instrument/ClassFileTransformer.java: New file. * java/lang/instrument/IllegalClassFormatException.java: New file. * java/lang/instrument/Instrumentation.java: New file. * java/lang/instrument/UnmodifiableClassException.java: New file. * vm/reference/java/lang/InstrumentationImpl.java: New file. * vm/reference/java/lang/VMInstrumentationImpl.java: New file. instrument.tar.gz Description: GNU Zip compressed data ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Nicolas, On Fri, 2005-12-02 at 12:22 +0100, Nicolas Geoffray wrote: > >Aha. Sorry I missed the dual nature of the > >ClassFileTransFormer.transform() method. I see you documented this in > >your original patch. In that case I really do think we should move these > >to vm/reference/java/lang and make this method package private. > > > Which method? callTransformers? It's a method from the > InstrumentationImpl class, therefore it's in > gnu/java/lang/instrument. Would you like InstrumentationImpl > to be in vm/reference/java/lang/InsutrumentationImpl? This sounds > weird. We could also make the callTransformers method public. Yes, but it makes it easier to share code with ClassLoader and VMClassLoader if we do this. Then callTransformers doesn't have to be public, but can just be package private. Then we don't have to worry about whether or not someone can get insecure access to a IntrumentationImpl (although I admit that is not very likely). It just seems more natural to me to have all implementation classes in the same package because they seem very related. > I'm working on it (the jnjvm can now execute with the generics branch, > yeah :)). OK for making suggestions, where should it be written? In the VM Integration Guide of course and in the NEWS file. And when we have a first implementation of all this checked in it might make sense to explicitly ask on the main classpath list to get the attention of those runtime implementors that don't follow the patches list that closely. Cheers, Mark ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Mark,
Mark Wielaard wrote:
My idea was even that a VM might only want to have one redefine
prepare/apply cycle running at the same time. Then it can easily cleanup
anything not used. But this might be optimizing for an unusual case. We
can probably assume that transformers will actually transform correctly.
You're deleting all shared code I came up with :) I switched
to your idea because it reduces the number of native methods.
Aha. Sorry I missed the dual nature of the
ClassFileTransFormer.transform() method. I see you documented this in
your original patch. In that case I really do think we should move these
to vm/reference/java/lang and make this method package private.
Which method? callTransformers? It's a method from the
InstrumentationImpl class, therefore it's in
gnu/java/lang/instrument. Would you like InstrumentationImpl
to be in vm/reference/java/lang/InsutrumentationImpl? This sounds
weird. We could also make the callTransformers method public.
Right, I see we need to break the VM interface for this :{
Probably best not to do that for the first release (0.20) but lets
suggest how we would like to change the interface for 0.21 so runtime
implementers can prepare and scream about it. On the other hand this is
the generics branch only, not very many VMs depend on it yet. But not
very urgent I guess before we have this working with at least one VM.
I'm working on it (the jnjvm can now execute with the generics branch,
yeah :)). OK for making suggestions, where should it be written?
Just make a new paragraph for Instrumentation. Then mention that it
currently only is on the generics branch since it uses some new language
features.
OK then.
Cheers,
Nicolas
___
Classpath-patches mailing list
Classpath-patches@gnu.org
http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Nicolas,
On Thu, 2005-12-01 at 17:51 +0100, Nicolas Geoffray wrote:
> >Also if something went wrong with the transformation
> >of any of them we might want to have a cancelRedefineClass() to let
> the
> >VM know we won't be applying any prepared classes.
> >
> I had in mind that the VM wouldn't need a cancel method. The
> prepareRedefineClass gives a vm internal representation
> of a class, and if redefinition is canceled this representation would
> be garbage collected and forgotten by the vm. But maybe some vm
> would like to be noticed. I added the method since it might be
> usefull.
My idea was even that a VM might only want to have one redefine
prepare/apply cycle running at the same time. Then it can easily cleanup
anything not used. But this might be optimizing for an unusual case. We
can probably assume that transformers will actually transform correctly.
> >Why does the private callTransformers() implementation take a
> >ProtectionDomain? Isn't that
> classBeingRedefined.getProtectionDomain()?
> >
> Yes, but the classBeingRedefined might be null, if the method is
> called
> by the vm from the native ClassLoader.defineClass method. Therefore,
> the vm gives as arguments the protectionDomain it has.
Aha. Sorry I missed the dual nature of the
ClassFileTransFormer.transform() method. I see you documented this in
your original patch. In that case I really do think we should move these
to vm/reference/java/lang and make this method package private.
> >Normally we like to provide some default/noop implementation for the
> VM
> >reference classes. Is it possible to provide the necessary hooks in
> >ClassLoader/VMClassLoader to call the transformers just before
> >defineClass() is called?
> >
> It's possible, but it could break all existing vms lying on the vm
> reference classes.
> The VMClassLoader would need an extra defineClass method and an extra
> field
> for an InstrumentationImpl object. The extra defineClass method would
> check if the
> InstrumentationImpl is null and if not, call the
> InstrumentationImpl.callTransformers
> method.
Right, I see we need to break the VM interface for this :{
Probably best not to do that for the first release (0.20) but lets
suggest how we would like to change the interface for 0.21 so runtime
implementers can prepare and scream about it. On the other hand this is
the generics branch only, not very many VMs depend on it yet. But not
very urgent I guess before we have this working with at least one VM.
> Yeah, documentation to write! :). I will create a documentation file
> for the java.lang.instrument package,
> and change the vm integration guide (should I create a new paragraph
> for generics?)
Just make a new paragraph for Instrumentation. Then mention that it
currently only is on the generics branch since it uses some new language
features.
Cheers,
Mark
___
Classpath-patches mailing list
Classpath-patches@gnu.org
http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Mark Mark Wielaard wrote: Hi Nicolas, On Mon, 2005-11-28 at 19:08 +0100, Nicolas Geoffray wrote: Here's two files, InstrumentationImpl and VMInstrumentationImpl. I put all native vm specific methods in VMInstrumentationImpl, but if you think it's not necessary, it's ok with me to have all these methods in InstrumentationImpl. This looks pretty good. Thanks! Some comments and questions: InstrumentationImpl should implement Instrumentation (then a couple of methods need to be made public). Oops, so obvious that I forgot it :) I believe the Vector of transformaers is handled correctly even when manipulated by multiple threads. But I like using an ArrayList and Iterator and do explicit synchronization (but in this case that is clearly just a personal preference, so feel free to ignore if you disagree that it is clearer.) OK for an ArrayList, it's just that I'm not really used to generics and Vector seemed easier How atomic do we want applyRedefineClasses() to be? Instead of looping through all classes to redefine and calling applyRedefineClass() on each one, it might be a good idea to have an applyRedefineClasses(Object[]) in VMInstrumentationImpl to redefine them all at the same time after preparing them all. You're right. It's better to apply them all. Also if something went wrong with the transformation of any of them we might want to have a cancelRedefineClass() to let the VM know we won't be applying any prepared classes. I had in mind that the VM wouldn't need a cancel method. The prepareRedefineClass gives a vm internal representation of a class, and if redefinition is canceled this representation would be garbage collected and forgotten by the vm. But maybe some vm would like to be noticed. I added the method since it might be usefull. Why does the private callTransformers() implementation take a ProtectionDomain? Isn't that classBeingRedefined.getProtectionDomain()? Yes, but the classBeingRedefined might be null, if the method is called by the vm from the native ClassLoader.defineClass method. Therefore, the vm gives as arguments the protectionDomain it has. About the calltransformers FIXME. What else can/should we do here? I have no idea for now : the description of Sun doesn't specify which behaviour sould be adopted. At first I wanted to skip the transformer that raises the exception. But then, it might be the transformer called before that returned an illegal class. And so on. We can know if the byte[] given is correct by forcing the vm to read it, but that would not be efficient. I chose to return the initial byte[], because I didn't want to make assumptions on how to treat transformers. I should do some tests with the sun implementation :) Normally we like to provide some default/noop implementation for the VM reference classes. Is it possible to provide the necessary hooks in ClassLoader/VMClassLoader to call the transformers just before defineClass() is called? It's possible, but it could break all existing vms lying on the vm reference classes. The VMClassLoader would need an extra defineClass method and an extra field for an InstrumentationImpl object. The extra defineClass method would check if the InstrumentationImpl is null and if not, call the InstrumentationImpl.callTransformers method. The extra field can only be set by the command parsing method, because it's created when an instrument agent is created. If we do this wouldn't it be slightly easier to have all these classes in vm/reference/java/lang to make package access easier? Probably. I have set private the callTransformers method, but it could be set protected. An example how this is all supposed to work together when a VM implements this VM interface would be nice. I could propose something with my vm (jnjvm). But I have to make sure the generics branch works with it :) Also to make clear whether the premain() method is an instance of static method, what command line extensions/flags are expected, etc. Yeah, documentation to write! :). I will create a documentation file for the java.lang.instrument package, and change the vm integration guide (should I create a new paragraph for generics?) Cheers, Nicolas ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Nicolas, On Mon, 2005-11-28 at 19:08 +0100, Nicolas Geoffray wrote: > Here's two files, InstrumentationImpl and VMInstrumentationImpl. > I put all native vm specific methods in VMInstrumentationImpl, but > if you think it's not necessary, it's ok with me to have all these methods > in InstrumentationImpl. This looks pretty good. Thanks! Some comments and questions: InstrumentationImpl should implement Instrumentation (then a couple of methods need to be made public). I believe the Vector of transformaers is handled correctly even when manipulated by multiple threads. But I like using an ArrayList and Iterator and do explicit synchronization (but in this case that is clearly just a personal preference, so feel free to ignore if you disagree that it is clearer.) How atomic do we want applyRedefineClasses() to be? Instead of looping through all classes to redefine and calling applyRedefineClass() on each one, it might be a good idea to have an applyRedefineClasses(Object[]) in VMInstrumentationImpl to redefine them all at the same time after preparing them all. Also if something went wrong with the transformation of any of them we might want to have a cancelRedefineClass() to let the VM know we won't be applying any prepared classes. Why does the private callTransformers() implementation take a ProtectionDomain? Isn't that classBeingRedefined.getProtectionDomain()? About the calltransformers FIXME. What else can/should we do here? Normally we like to provide some default/noop implementation for the VM reference classes. Is it possible to provide the necessary hooks in ClassLoader/VMClassLoader to call the transformers just before defineClass() is called? If we do this wouldn't it be slightly easier to have all these classes in vm/reference/java/lang to make package access easier? An example how this is all supposed to work together when a VM implements this VM interface would be nice. Also to make clear whether the premain() method is an instance of static method, what command line extensions/flags are expected, etc. Cheers, Mark ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Mark,
Mark Wielaard wrote:
Ideally yes. I was just thinking that if the interface between
InstrumentationImpl and VMInstrumentationImpl is really thin and there
is not much code that can be shared maybe we just have one class that
the VM should implement. I haven't thought very deep about the actual
implementation yet or how easy it is to write generic code that can be
easily shared.
Here's two files, InstrumentationImpl and VMInstrumentationImpl.
I put all native vm specific methods in VMInstrumentationImpl, but
if you think it's not necessary, it's ok with me to have all these methods
in InstrumentationImpl.
I'll change the vm integration guide when you'll judge these files can
come into cvs-generics.
Cheers,
Nicolas
/* InstrumentationImpl.java -- GNU implementation of
java.lang.instrument.Instrumentation
Copyright (C) 2005 Free Software Foundation, Inc.
This file is part of GNU Classpath.
GNU Classpath is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2, or (at your option)
any later version.
GNU Classpath is distributed in the hope that it will be useful, but
WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
You should have received a copy of the GNU General Public License
along with GNU Classpath; see the file COPYING. If not, write to the
Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA.
Linking this library statically or dynamically with other modules is
making a combined work based on this library. Thus, the terms and
conditions of the GNU General Public License cover the whole
combination.
As a special exception, the copyright holders of this library give you
permission to link this library with independent modules to produce an
executable, regardless of the license terms of these independent
modules, and to copy and distribute the resulting executable under
terms of your choice, provided that you also meet, for each linked
independent module, the terms and conditions of the license of that
module. An independent module is a module which is not derived from
or based on this library. If you modify this library, you may extend
this exception to your version of the library, but you are not
obligated to do so. If you do not wish to do so, delete this
exception statement from your version. */
package gnu.java.lang.instrument;
import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.ClassDefinition;
import java.lang.instrument.UnmodifiableClassException;
import java.lang.instrument.IllegalClassFormatException;
import java.security.ProtectionDomain;
import java.util.Vector;
import java.util.Enumeration;
/**
* An Instrumentation object has transformers that will
* be called each time a class is defined or redefined.
* The object is given to a premain function
* that is called before the main function.
*
* @author Nicolas Geoffray ([EMAIL PROTECTED])
* @since 1.5
*/
final class InstrumentationImpl
{
/* List of transformers */
private Vector transformers =
new Vector();
private InstrumentationImpl()
{
}
/**
* Adds a ClassFileTransformer object
* to the instrumentation. Each time a class is defined
* or redefined, the transform method of the
* transformer object is called.
*
* @param transformer the transformer to add
* @throws NullPointerException if transformer is null
*/
void addTransformer(ClassFileTransformer transformer)
{
if(transformer == null)
throw new NullPointerException();
transformers.add(transformer);
}
/**
* Removes the given transformer from the set of transformers
* this Instrumentation object has.
*
* @param transformer the transformer to remove
* @return true if the transformer was found and removed, false if
* the transformer was not found
* @throws NullPointerException if transformer is null
*/
boolean removeTransformer(ClassFileTransformer transformer)
{
if(transformer == null)
throw new NullPointerException();
return transformers.remove(transformer);
}
/**
* Returns if the current JVM supports class redefinition
*
* @return true if the current JVM supports class redefinition
*/
boolean isRedefineClassesSupported()
{
return VMInstrumentationImpl.isRedefineClassesSupported();
}
/**
* Redefine classes present in the definitions array, with
* the corresponding class files.
*
* @param definitions an array of classes to redefine
*
* @throws ClassNotFoundException if a class cannot be found
* @throws UnmodifiableClassException if a class cannot be modified
* @throws UnsupportedOperationException if the JVM does not support
* redefinition or the redefinition made unsu
Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package
Hi Nicolas, On Sat, 2005-11-26 at 19:11 +0100, Nicolas Geoffray wrote: > >Yes, that name is OK. I don't know how much we can actually share > >between runtimes though. The interface is pretty minimal already, so a > >split between VMInstrumentationImpl and InstrumentationImpl that calls > >the VM class seems overkill. > > > The InstrumentationImpl.getObjectSize is clearly a native for vm. > And we also need a native redefineClass(Class, byte[]). > > If we don't provide a VMInstrumentationImpl class, would > these method be native in InstrumentationImpl? (I thought > vm implementors only had to care about vm/reference/* files?) Ideally yes. I was just thinking that if the interface between InstrumentationImpl and VMInstrumentationImpl is really thin and there is not much code that can be shared maybe we just have one class that the VM should implement. I haven't thought very deep about the actual implementation yet or how easy it is to write generic code that can be easily shared. > Because it's the vm that parses arguments, it knowns if the > application does instrumentation. Therefore, if it supports > redefining classes, it can set a flag or change definition of > VMClassLoader.defineClass. The vm then creates an > InstrumentationImpl object and calls premain. > > The VMClassLoader.defineClass will be changed to call a > private InstrumentationImpl.defineClass method. This method will then call > each transformer. > > Does that sound correct to you? This way I don't modify > ClassLoader.defineClass. Yes, this makes perfect sense. Thanks, Mark ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
