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

[Daniel Fuchs]

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

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