Hi Mandy,
Apologies for the delay.
Could you review this cdiff as a proposal for the jtreg test?
Made sense to modify the existing test set for MethodHandle rather than
add a new one.
------------------------------------------------
test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java
*** 830,844 ****
--- 830,860 ----
public void testUnreflectSetter() throws Throwable {
CodeCacheOverflowProcessor.runMHTest(this::testUnreflectSetter0);
}
public void testUnreflectSetter0() throws Throwable {
+ //First we test the newer "static final field" behaviour.
+ testUnreflectSetterStaticFinalField();
+ //Now we test the more proven functionality.
if (CAN_SKIP_WORKING) return;
startTest("unreflectSetter");
testSetter(TEST_UNREFLECT);
}
+ static final int staticFinalField = 0;
+ public void testUnreflectSetterStaticFinalField() throws Throwable {
+ try {
+ Lookup l = MethodHandles.lookup();
+ Field f =
MethodHandlesGeneralTest.class.getDeclaredField("staticFinalField");
+ f.setAccessible(true);
+ MethodHandle s = l.unreflectSetter(f);
+ throw new
RuntimeException("MethodHandle.Lookup.unreflectSetter(Field) failed to
throw IAE when Field is static and final.");
+ } catch (IllegalAccessException e) {
+ // IllegalAccessException expected.
+ }
+ }
+
@Test
public void testFindSetter() throws Throwable {
CodeCacheOverflowProcessor.runMHTest(this::testFindSetter0);
}
------------------------------------------------
Best Regards
Adam Farley
IBM Runtimes
Mandy Chung <mandy.ch...@oracle.com> wrote on 31/01/2019 18:58:25:
> From: Mandy Chung <mandy.ch...@oracle.com>
> To: Adam Farley8 <adam.far...@uk.ibm.com>
> Cc: core-libs-dev <core-libs-dev@openjdk.java.net>
> Date: 31/01/2019 18:58
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails
> to throw IllegalAccessException for final fields
>
>
>
> On 1/31/19 4:18 AM, Adam Farley8 wrote:
> > Hi Mandy,
> >
> > I've made the changes to the webrev and uploaded the corrected
version.
> >
> > I've also uploaded the zips in the csr and bug.
> >
> > As for the test, I have mentioned that the test is attached to the
bug.
> > I attached the same test to the CSR.
>
> I did look at that but it's not a jtreg regression test. I assume
> you planned to convert it to jtreg test. Anyway, the test attached
> in the bug report is the reproducer of the bug.
>
> It would help the reviewer to include the regression test in
> the webrev along with the fix unless a test is not necessary.
> The OpenJDK developer guide gives a pretty good overview on the
> process [1] and please do ask if you have any question.
>
> https://urldefense.proofpoint.com/v2/url?
> u=http-3A__openjdk.java.net_guide_changePlanning.html-23bug&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=uZYDsYv1Pe21YaYk_fuEwKg7Cc5a9sdsiGMPjDqgT5k&s=mvs_zaoLuTOUkmdByobfKcpvovlTEiHtNcz-
> UMc3d70&e=
>
> > Are you saying that the test supplied is not a suitable test?
>
> TestStaticFinal.java is a standalone test. It needs to convert
> to a jtreg regression test.
>
> The regression should throw an exception if Lookup::unreflectSetter
> returns MH on a static final field with and without accessible flag
> set. However TestStaticFinal.java also exits with 0. The regression
> test should pass with your bug and fail without the fix (throwing
> an exception).
>
> This test verifies unreflectSetter on a static final field. I
> suggest it also includes a case with instance final field with
> and without accessible flag set to show the exception for
> unreflectSetter what case allows setting final fields.
>
> Mandy
> [1] https://urldefense.proofpoint.com/v2/url?
> u=http-3A__openjdk.java.net_guide_changePlanning.html-23bug&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=uZYDsYv1Pe21YaYk_fuEwKg7Cc5a9sdsiGMPjDqgT5k&s=mvs_zaoLuTOUkmdByobfKcpvovlTEiHtNcz-
> UMc3d70&e=
> P.S. should be updated to refer to CSR that replaced CCC.
>
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
