[9 + 8u] RFR - 8066612: Add a test that will call getDeclaredFields() on all classes and try to set them accessible.
Daniel Fuchs
daniel.fuchs at oracle.com
Sat Dec 6 10:44:47 UTC 2014
Hi Peter,
On 12/6/14 11:16 AM, Peter Levart wrote:
>
> On 12/05/2014 09:06 PM, Daniel Fuchs wrote:
>> Hi David, all,
>>
>> @David: You're right David.
>> The loader parameter is never used - I have removed it.
>>
>> @all: I have received a comment from Alan that it would be better to use
>> the new jrt:/ FileSystem directly, rather than using private APIs.
>> One of the consequence is that the test now loads all the
>> classes in the runtime image (not just the ones in the BCL),
>> and therefore I have removed the toggle that allowed to test
>> the boot classes only.
>>
>> http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.02/
>>
>> best regards,
>>
>> -- daniel
>
> Hi Daniel,
>
> Would it be possible to extract a minimal API for streaming over
> classes and put that into the testlib so that only this part differs
> between JDK8 and JDK9 and the guts of the test is identical for both
> JDK8 and JDK9 ?
>
> This would ease backport efforts when the test(s) are later "beefed
> up" with other functionality. The API could later be extended with
> other functionality, but just for illustration what might be needed,
> here's what Martin Buchholz started as a test for finding possible
> data races in reflection and I just re-packed using Streams. From
> first glance it seems the tests need similar common functionality:
>
> http://cr.openjdk.java.net/~plevart/misc/SunReflectDataRaces/
>
> Regards, Peter
The test is already built like that:
The part that finds the class names is encapsulated in an object which
implements Iterable<String>.
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.02/ ->
ClassNameJrtStreamBuilder
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk8-00/ ->
ClassNameStreamBuilder
http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk7.00/ ->
ClassNameListBuilder
The bulk of the test should be identical (well, almost identical since I
only
updated 9 after the reviews, and since 9 is the only one that needs the
System ClassLoader)... I suppose we could extract these 3 classes and define
a library class with something like:
public static class ClassNameFinder {
public static Iterable<String> findAllClassNames() {
jdk9 impl: return new ClassNameJrtStreamBuilder();
jdk8 impl: return new ClassNameStreamBuilder();
jdk7 impl: return new ClassNameListBuilder();
}
}
As it stands - this corresponds to the
static Iterable<String> listAllClassNames()
factory method in each version of the test.
best regards,
-- daniel
>
>>
>> On 05/12/14 00:36, David Holmes wrote:
>>> Hi Daniel,
>>>
>>> I still find your use of the classloader very confusing. You talk about
>>> the defining loader but you never check the defining loader against
>>> anything. In
>>>
>>> 146 static void checkFor(Class<?> c, ClassLoader loader) {
>>>
>>> the loader variable is never used. And if loader is simply the name of
>>> the loader passed to forName, then it may not be the defining loader
>>> anyway - so the whole use of this loader variable seems superfluous at
>>> best, and confusing/misleading at worst. And you can use the simple
>>> forName(name) variant rather than passing a loader.
>>>
>>> David
>>>
>>> On 5/12/2014 3:13 AM, Daniel Fuchs wrote:
>>>> On 04/12/14 14:02, Seán Coffey wrote:
>>>>> Thanks for driving efforts in this area Daniel. I think it's a very
>>>>> useful test and is bound to help test code coverage. If it's
>>>>> currently
>>>>> passing on all JPRT platforms, it's a good measure.
>>>>
>>>> It seems to :-)
>>>>
>>>>> Eventually I think we can bulk up the tests that can be run on the
>>>>> Iterable returned from your class search.
>>>>> At moment you just test Field.setAccessible.
>>>>
>>>> Yes. If we change it later to test more, we will probably need to
>>>> change its name. I was already in lack of inspiration:
>>>> FieldSetAccessibleTest is not really a great name - but hopefully
>>>> it can do for now.
>>>>
>>>>> In the future, I don't see any harm in adding all simple Field method
>>>>> calls so that any corner cases in custom classes like the original
>>>>> issue
>>>>> are caught. e.g Field.getDeclaredAnnotations(), Field.getModifiers(),
>>>>> Field.isEnumConstant() etc., etc. Some methods won't be much value
>>>>> add
>>>>> but they're not a cost either.
>>>>
>>>> Agreed.
>>>>
>>>>> Same argument for running through all Class methods, e.g
>>>>> Class.getDeclaredClasses(), Class.getDeclaredMethods(). As a
>>>>> result this
>>>>> test might eventually become more general in test goal and might live
>>>>> better one level up in "test/java/lang/Class/" - it can be moved when
>>>>> the time comes.
>>>>
>>>> Agreed as well :-)
>>>>
>>>> Here is a new revision of the webrev for 9 in which I have
>>>> incorporated David's comment:
>>>>
>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.01/
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>>>
>>>>> regards,
>>>>> Sean.
>>>>>
>>>>> On 04/12/2014 12:25, Daniel Fuchs wrote:
>>>>>> On 04/12/14 13:06, David Holmes wrote:
>>>>>>> Hi Daniel,
>>>>>>>
>>>>>>> On 4/12/2014 9:38 PM, Daniel Fuchs wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> In fact I could use 'null' on JDK 9 as well.
>>>>>>>> My first version of the JDK 9 test was parsing over all the
>>>>>>>> .jimage
>>>>>>>> files under <jdk>/lib/modules - which explain why I needed to
>>>>>>>> use the System class loader.
>>>>>>>>
>>>>>>>> Then I switched to only parsing the bootmodules.jimage - because
>>>>>>>> I noticed that the results where more coherent with what I had
>>>>>>>> observed on 8 & 7 - but I kept using the System class loader.
>>>>>>>>
>>>>>>>> I am not sure whether we want the test for 9 should iterate over
>>>>>>>> the three .jimage - or continue to test only the boot .jimage.
>>>>>>>>
>>>>>>>> I have updated the JDK 9 test (refreshed the webrev in place)
>>>>>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.00/
>>>>>>>> and added support for possibly running the test in the two modes
>>>>>>>> (I added a 'test.boot.only' system property, true by default)
>>>>>>>> as well as additional traces to print the loaded classes by
>>>>>>>> defining loader at the end (test.list.classes, false by default).
>>>>>>>
>>>>>>> A couple of initial comments:
>>>>>>>
>>>>>>> 104 static ClassLoader getClassLoaderFor(String
>>>>>>> classFileName) {
>>>>>>> 105 if (restrictToBoot) return null; // only bootmodules
>>>>>>> 106 return ClassLoaders.systemClassLoader;
>>>>>>> 107 }
>>>>>>>
>>>>>>> I'm not clear the intent here. If it is to return a loader for
>>>>>>> which
>>>>>>> loadClass could be invoked then you can always just return the
>>>>>>> system
>>>>>>> loader - or just Class.forName. If it is meant to the return the
>>>>>>> expected defining loader then it isn't doing that as the extensions
>>>>>>> loader is not allowed for.
>>>>>>
>>>>>> The intent is to return the class loader that will be passed to
>>>>>> Class.forName( ). I've been fiddling & experimenting with this
>>>>>> test over 3 different platforms while trying to minimize the
>>>>>> differences - so that was my attempt at having a good place to
>>>>>> experiment with different strategy for loading classes.
>>>>>>
>>>>>>> Similarly for:
>>>>>>>
>>>>>>> 128 static ClassLoader getFor(String classFileName) {
>>>>>>> 129 return systemClassLoader;
>>>>>>> 130 }
>>>>>>
>>>>>> Oh - that's my mistake. In fact ClassLoader getClassLoaderFor(..)
>>>>>> was supposed to simply return ClassLoaders.getFor(...);
>>>>>> and all the code should be in ClassLoaders.getFor - my bad.
>>>>>>
>>>>>>> Minor nit - In:
>>>>>>>
>>>>>>> 135 System.err.println("Unexpected loader for
>>>>>>> "+c+":
>>>>>>> "+c.getClassLoader());
>>>>>>>
>>>>>>> c.getClassLoader() can be replaced by cl. Also put spaces around
>>>>>>> the +
>>>>>>> operator.
>>>>>>
>>>>>> Good catch.
>>>>>>
>>>>>> Thanks a lot David! Have a good night! (that's quite late - isn't
>>>>>> it?)
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>> (signing off for the night)
>>>>>>>
>>>>>>>> Thanks for your question, it triggered me into looking deeper
>>>>>>>> into what was happening :-)
>>>>>>>>
>>>>>>>> best regards,
>>>>>>>>
>>>>>>>> -- daniel
>>>>>>>>
>>>>>>>> On 04/12/14 10:05, Daniel Fuchs wrote:
>>>>>>>>>>> The differences between 8 & 9 are limited to:
>>>>>>>>>>>
>>>>>>>>>>> - ClassLoader:
>>>>>>>>>>> - on 8 we use 'null' (BCL)
>>>>>>>>>>> - on 9 we use the system class loader.
>>>>>>>>>>
>>>>>>>>>> I haven't seen anything in JEP 220, regarding modules, that
>>>>>>>>>> indicates
>>>>>>>>>> that classes currently loaded by the boot-loader will now be
>>>>>>>>>> loaded
>>>>>>>>>> by the system classloader ???
>>>>>>>>>
>>>>>>>>> In [1] towards the end:
>>>>>>>>>
>>>>>>>>> [1] http://openjdk.java.net/jeps/220
>>>>>>>>>
>>>>>>>>> "The defining class loader of the types in some existing packages
>>>>>>>>> will change. Existing code that makes assumptions about the
>>>>>>>>> class
>>>>>>>>> loaders of these types might not work correctly."
>>>>>>>>> (then there is a list of specific changes).
>>>>>>>>>
>>>>>>>>> This test looks up all class names in the image files and attempt
>>>>>>>>> to load the corresponding class. But as indicated in [1]
>>>>>>>>> some of these classes are now in the Boot CL, some in the
>>>>>>>>> Extension CL, and some in the Application CL.
>>>>>>>>>
>>>>>>>>> So the test uses the System CL to load each class - which ensures
>>>>>>>>> that the loading will be delegated to the appropriate
>>>>>>>>> ClassLoader.
>>>>>>>>>
>>>>>>>>> best regards,
>>>>>>>>>
>>>>>>>>> -- daniel
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
More information about the jdk8u-dev
mailing list