RFR 8191102: Incorrect include file use in classLoader.hpp

[harold seigel]

Hi David,

Thanks for the review.

See comments in-line.

Thanks! Harold


On 3/4/2018 6:09 PM, David Holmes wrote:
> Hi Harold,
>
> Seems okay. Thanks for tackling it - the fan out problem can easily 
> get out of control.
>
> On 3/03/2018 12:03 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-11 change to fix JDK-8191102.
>>
>> The fix does the refactoring described in the bug by moving methods 
>> in .hpp files that call OrderAccess::load_acquire() or 
>> OrderAccess::release_store() to .inline.hpp files.
>>
>> Note that if a method in an .hpp file called a method that was moved 
>> to an .inline.hpp file then it needed to be moved also. For example, 
>> method slot_at() was moved from constantPool.hpp to 
>> constantPool.inline.hpp because it calls OrderAccess::load_acquire(). 
>> So, method is_pseudo_string_at() had to also be moved because it 
>> calls slot_at().  And, pseudo_string_at() had to be moved because it 
>> calls is_pseudo_string_at(), etc.  A couple of methods were moved 
>> from constantPool.hpp to constantPool.cpp to reduce this fan out of 
>> changes.
>
> So basically once a function definition is in a .inline.hpp file, 
> every callsite for that function must itself either be in a 
> .inline.hpp file or a .cpp file. So no trivial inline function 
> definition can go into a .hpp file if it transitively uses any 
> function from a .inline.hpp file?
That is my understanding.  The Mac OS X compiler, in particular, would 
complain, otherwise.
>
> Are there any tools to help us check this? I don't think the compiler 
> by itself is any help here.
I don't know of any tools, but as mentioned above, the Mac OS X compiler 
was good at detecting this.
>
> Thanks,
> David
>
>> This change also contains a small unrelated cleanup for 
>> classLoaderData.inline.hpp.
>>
>> Open Webrev: 
>> http://cr.openjdk.java.net/~hseigel/bug_8191102/webrev/index.html
>>
>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8191102
>>
>> The change was tested with Mach5 tiers 1 and 2 tests and builds on 
>> all Mach5 platforms and tiers 3-5 tests on Linux-X64.
>>
>> Thanks, Harold