[9 + 8u] RFR - 8066612: Add a test that will call getDeclaredFields() on all classes and try to set them accessible.

[David Holmes]

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