RFR(M/T) : 8243935 : s.h.WhiteBoxPermission must be ClassFileInstall-ed together w/ s.h.WhiteBox

[Igor Ignatyev]

Hi David, Stefan,

there is no issue, this patch is just a spin-off of other patch I was working on. I noticed that some tests copy WhiteBoxPermission and other don't, so I decided to clean this up. I completely forgot about the hack we did in ClassFileInstaller (I guess partially b/c we still have tests which copy WhiteBoxPermission explicitly) so now I think it's better to just remove all explicit copying of WhiteBox$WhiteBoxPermission (which kinda echoes Stefan's patch). this of course touches more files, on the bright side this makes them all neater. so I run sed '/^\s*\*\s*sun.hotspot.WhiteBox\$WhiteBoxPermission/'d to remove all WhiteBoxPermission, and them removed some leftovers in 'containers/docker': 
- full webrev:
  http://cr.openjdk.java.net/~iignatyev//8243935/webrev.01/
> 656 lines changed: 0 ins; 612 del; 44 mod;

- incremental webrev:
  http://cr.openjdk.java.net/~iignatyev//8243935/webrev.0-1/
> 722 lines changed: 0 ins; 719 del; 3 mod; 

if we agree that this is the way to go, I'll repurpose 8243935 to remove WhiteBoxPermission (and obviously will retest the patch).

Thanks,
-- Igor 


> On Apr 28, 2020, at 2:49 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> On 28/04/2020 5:34 pm, Stefan Karlsson wrote:
>> Hi,
>> I have another idea. Can't we simply remove WhiteBoxPermission and incorporate that functionality in the WhiteBox class? We would then be able to remove all these WhiteBoxPermission lines.
> 
> Interesting though a little hackish. :)
> 
> But why is this an issue anyway we already hacked ClassfileInstaller to ensure it always copies the permission class!!!
> 
> 102     // Add commonly used inner classes that are often omitted by mistake. Currently
> 103     // we support only sun.hotspot.WhiteBox$WhiteBoxPermission. See JDK-8199290
> 104     private static String[] addInnerClasses(String[] classes, int startIdx) {
> 105         boolean seenWB = false;
> 106         boolean seenWBInner = false;
> 107         final String wb = "sun.hotspot.WhiteBox";
> 108         final String wbInner = "sun.hotspot.WhiteBox$WhiteBoxPermission";
> 
> David
> -----
> 
>> I've tested this and it seems to work:
>> https://cr.openjdk.java.net/~stefank/prototype/removeWhiteBoxPermission/webrev.01/ Summary of changes:    695 lines changed: 0 ins; 657 del; 38 mod; 60752 unchg
>> The interesting code:
>> https://cr.openjdk.java.net/~stefank/prototype/removeWhiteBoxPermission/webrev.01/test/lib/sun/hotspot/WhiteBox.java.udiff.html (There are some parts of the patch that was butchered by the sed-script - I'll fix this if we agree that this is a good thing to do)
>> If you think this is something we should do, I'll create an RFE, complete the patch, run more tests, and send it out for review.
>> Thanks,
>> StefanK
>> On 2020-04-28 08:08, David Holmes wrote:
>>> On 28/04/2020 3:37 pm, Igor Ignatyev wrote:
>>>> 
>>>> 
>>>>> On Apr 27, 2020, at 10:03 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>> 
>>>>> Hi Igor,
>>>>> 
>>>>> On 28/04/2020 12:32 pm, Igor Ignatyev wrote:
>>>>>> http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00/
>>>>>>> 338 lines changed: 107 ins; 0 del; 231 mod
>>>>>> Hi all,
>>>>>> could you please review the trivial yet tedious patch which adds 'sun.hotspot.WhiteBox$WhiteBoxPermission' to all 'ClassFileInstaller sun.hotspot.WhiteBox' actions?
>>>>>> the patch also
>>>>>>   - replaces '@run main ClassFileInstaller' w/ '@run driver ClassFileInstaller'
>>>>>>   - unifies the way ClassFileInstaller sun.hotspot.WhiteBox are aligned
>>>>> 
>>>>> That alignment change is unnecessary and makes it very hard to review this patch.
>>>>> 
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243935
>>>>>> webrevs:
>>>>>>   - http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00/ -- regular webrev
>>>>>>   - http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00.including_ws -- shows "whitespace only" changes
>>>>> 
>>>>> I'd like to see the patch that leaves out the whitespace-only changes - is that possible?
>>>> sure, I've used `hg diff -w` (--ignore-all-space    ignore white space when comparing lines) to remove all whitespace changes, here is webrev -- http://cr.openjdk.java.net/~iignatyev//8243935/webrev.00.w/
>>> 
>>> Thanks that was somewhat better. Change seems okay.
>>> 
>>> David
>>> 
>>>> -- Igor
>>>>> 
>>>>> Thanks,
>>>>> David
>>>>> 
>>>>>> Thanks,
>>>>>> -- Igor
>>>>