RFR: 8153858: Clean up needed when obtaining the package name from a fully qualified class name
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Jun 6 22:46:44 UTC 2016
Hi,
http://cr.openjdk.java.net/~rprotacio/8153858.02/src/share/vm/oops/instanceKlass.cpp.udiff.html
The comment
+// Used to obtain the package name from a fully qualified class name.
+// It is the responsibility of the caller to establish ResourceMark.
+Symbol* InstanceKlass::package_from_name(const Symbol* name, TRAPS) {
Is confusing since you have a ResourceMark a few lines down.
http://cr.openjdk.java.net/~rprotacio/8153858.02/src/share/vm/classfile/systemDictionary.cpp.udiff.html
+ TempNewSymbol pkg_name = InstanceKlass::package_from_name(class_name,
CHECK_(nh));
Can you change the end to CHECK_NULL? Trying to get rid of these
CHECK_(nh).
http://cr.openjdk.java.net/~rprotacio/8153858.02/src/share/vm/classfile/classLoader.cpp.udiff.html
+ TempNewSymbol pkg_name = InstanceKlass::package_from_name(class_name,
THREAD);
The last parameter of this should be CHECK_NULL since the
SymbolTable::new_symbol() can throw OutOfMemoryError.
These are minor comments, so I don't need to see another webrev.
This is a nice consolidation of tedious character parsing code!
Thanks,
Coleen
On 6/6/16 4:19 PM, Rachel Protacio wrote:
> Also, I updated systemDictionary.cpp at line 1161 to use
> StringUtils::replace_no_expand() instead of stepping through the
> string. http://cr.openjdk.java.net/~rprotacio/8153858.02/
>
> Rachel
>
>
> On 6/6/2016 2:52 PM, Rachel Protacio wrote:
>> Hi David,
>>
>> I see what you're suggesting with the test, but since there is no
>> THREAD to use in the gtest, I can't for example write
>> TempNewSymbol sym = SymbolTable::new_symbol("", THREAD);
>> to then pass into InstanceKlass::package_from_name. On the other
>> hand, creating a new_symbol("", NULL) succeeds I think because it
>> looks up the symbol with the empty string name which happens to exist
>> in the table, but in reality it cannot create a new symbol on a
>> non-existent thread. So even though it would be good to test, I don't
>> think there's a way to do it. What do you think?
>>
>> Thanks,
>> Rachel
>>
>> On 6/5/2016 9:15 PM, David Holmes wrote:
>>> Hi Rachel,
>>>
>>> On 4/06/2016 6:20 AM, Rachel Protacio wrote:
>>>> Hi,
>>>>
>>>> Thanks for the comments. New webrev:
>>>> http://cr.openjdk.java.net/~rprotacio/8153858.01/
>>>
>>> All changes look okay to me.
>>>
>>> Regarding the test ... you stated that because gtest can't pass
>>> THREAD that the InstanceKlass functionality is just tested via JCK
>>> tests. But in fact you do have one test for
>>> InstanceKlass::package_from_name where you pass in NULL as THREAD.
>>> Given the code in package_from_name:
>>>
>>> 2187 Symbol* InstanceKlass::package_from_name(const Symbol* name,
>>> TRAPS) {
>>> 2188 if (name == NULL) {
>>> 2189 return NULL;
>>> 2190 } else {
>>> 2191 if (name->utf8_length() <= 0) {
>>> 2192 return NULL;
>>> 2193 }
>>> 2194 ResourceMark rm;
>>> 2195 const char* package_name =
>>> ClassLoader::package_from_name((const char*) name->as_C_string());
>>> 2196 if (package_name == NULL) {
>>> 2197 return NULL;
>>> 2198 }
>>> 2199 Symbol* pkg_name = SymbolTable::new_symbol(package_name,
>>> THREAD);
>>> 2200 return pkg_name;
>>> 2201 }
>>> 2202 }
>>>
>>> You are currently testing the return path at L#2189. In theory you
>>> can test all return paths until the NULL THREAD would cause a
>>> problem. So you could have a test to return at L#2192 and L#2197.
>>> But given the NULL returning calls to ClassLoader::package_from_name
>>> are already being independently tested, there is no need to
>>> duplicate them. So that just leaves adding a test for return at
>>> L#2192 - which I'm guessing using "" as the name would test.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Replies inline.
>>>>
>>>> On 6/3/2016 1:48 AM, David Holmes wrote:
>>>>> Hi Rachel,
>>>>>
>>>>> Overall looks good, but a few comments below.
>>>>>
>>>>> On 3/06/2016 3:12 AM, Rachel Protacio wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review this fix, which consolidates/refactors all the
>>>>>> instances
>>>>>> of code parsing fully qualified names and returning the package
>>>>>> name.
>>>>>> Includes gtests for the functions. The gtest framework does not
>>>>>> allow
>>>>>> for passing in a THREAD, so the InstranceKlass functionality is just
>>>>>> tested by jck tests.
>>>>>
>>>>> src/share/vm/classfile/classLoader.hpp
>>>>>
>>>>> Can you document what the bad_class_name boolean indicates please. It
>>>>> took me a while to figure out that
>>>>> InstanceKlass::is_same_class_package (but seemingly no other caller)
>>>>> needs to distinguish between "has no package" and "an error occurred
>>>>> trying to determine what the package is"
>>>>>
>>>> Added.
>>>>> ---
>>>>>
>>>>> src/share/vm/classfile/systemDictionary.cpp
>>>>>
>>>>> 1157 TempNewSymbol pkg_name =
>>>>> InstanceKlass::package_from_name(parsed_name, THREAD);
>>>>>
>>>>> Why are you not using a CHECK macro here?
>>>> My mistake. I've put in a CHECK_NULL.
>>>>>
>>>>> ---
>>>>>
>>>>> src/share/vm/oops/instanceKlass.cpp
>>>>>
>>>>> 2318 bool bad_class_name1 = false;
>>>>> 2324 bool bad_class_name2 = false;
>>>>>
>>>>> You only need a single local variable.
>>>>>
>>>>> 2336 // Check that package is identical
>>>>> 2337 return !strcmp(name1, name2);
>>>>>
>>>>> Please avoid implicit booleans: return strcmp(name1, name2) == 0;
>>>> Ok.
>>>>>
>>>>> ---
>>>>>
>>>>> test/native/runtime/test_instance_klass.cpp
>>>>>
>>>>> Should the naming convention here match the source file that holds
>>>>> the
>>>>> code being tested ie test_instanceKlass and test_classLoader?
>>>> Done.
>>>>>
>>>>> It seems that without being able to pass a Thread you can test a
>>>>> number of inputs, as long as you won't reach the
>>>>> SymbolTable::new_symbol call. That said the call to
>>>>> ClassLoader::package_from_name is already being tested separately, so
>>>>> it probably suffices to just add a test for the utf8_length() check.
>>>> Sorry, I'm not sure what you mean here. Can you elaborate?
>>>>>
>>>>>> Note that the change in method.hpp was a
>>>>>> compilation error uncovered by my gtest.
>>>>>
>>>>> ??? Your change would seem to me to be introducing a potential
>>>>> compilation error:
>>>>>
>>>>> return (sizeof(Method)/wordSize + is_native()) ? 2 : 0;
>>>>>
>>>>> this adds a boolean to an integer. And the result will always be 2!
>>>> Oops, you're right! I've moved the parentheses around the ternary
>>>> statement.
>>>>
>>>> Rachel
>>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>>
>>>>>> Passes hotspot gtests, jck lang and api/java_lang tests, JPRT,
>>>>>> and RBT
>>>>>> hotspot and non-colo tests.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153858
>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8153858.00/
>>>>>>
>>>>>> Thank you,
>>>>>> Rachel
>>>>
>>
>
More information about the hotspot-runtime-dev
mailing list