'Re: RFR: [8218152] javac fails and exits with no error if a bad annotation processor is on the class'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: RFR: [8218152] javac fails and exits with no error if a bad annotation processor is on the class
From:       Jan Lahoda <jan.lahoda () oracle ! com>
Date:       2019-02-13 15:08:11
Message-ID: 5C6432DB.7070208 () oracle ! com
[Download RAW message or body]

On 13.2.2019 10:58, Steve Groeger wrote:
> Hi Jan,
> 
> Thanks for the response.
> 
> With regards to your comments re:
> 
> > But I guess I'd personally rather kept
> > it simple. Would there be an issue with simply changing the "catch
> > (Throwable)" to also report the same error as "catch
> > (ServiceConfigurationError)"? I.e. something like:
> > > ---
> > > diff -r 4ef401769c36
> src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
> 
> > > ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
>  Fri Feb 08 14:06:35 2019 +0100
> > > +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
>  Mon Feb 11 13:33:50 2019 +0100
> > > @@ -437,6 +437,7 @@
> > > 
> log.error(Errors.ProcBadConfigFile(sce.getLocalizedMessage()));
> > > throw new Abort(sce);
> > > } catch (Throwable t) {
> > > +
> log.error(Errors.ProcBadConfigFile(t.getLocalizedMessage()));
> > > throw new Abort(t);
> > > }
> > > }
> > > @@ -453,6 +454,7 @@
> > > 
> log.error(Errors.ProcBadConfigFile(sce.getLocalizedMessage()));
> > > throw new Abort(sce);
> > > } catch (Throwable t) {
> > > +
> log.error(Errors.ProcBadConfigFile(t.getLocalizedMessage()));
> > > throw new Abort(t);
> > > }
> > > }
> > ---
> 
> I can do it the waty you sugegsted but I had done it this way because
> Jonathan Gibbons had stated:
> 
> > > There's a `catch (Throwable t)` in the same set of catch blocks that you
> > > did not update. Rather than add yet another message for that case, it
> > > might be better to throw FatalError, to provoke the standard "javac
> > > crash" output for the unknown exception.
> 
> Either way is OK with me. We just need to make a decision on which is
> the preferred way - then I can update the webrevs.

To me personally, FatalError feels a little bit harsh. But I agree we 
should try to avoid adding new error messages for this if possible. I'll 
leave it up to Jon.

(If we wanted to always print a stack trace for these problems, we could 
change to code to simply throw AnnotationProcessingError, which always 
prints the stacktrace - some tweaking to make the errors look good might 
be needed, though.)

Jan

> 
> > Looking at the tests, one thing to note is that binary files are
> > generally not allowed in the repository. I wonder if a test that would
> > run javac programmaticaly wouldn't be easier to do
> 
> I will take yopur advice on the tests and see if I can do this
> programtically rather than havng the binary files in the repository.
> 
> Will post again in the mailing list when I have updated webrevs that are
> suitable for being reviewed.
> 
> Thanks
> Steve Groeger
> IBM Runtime Technologies
> Hursley, Winchester
> Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
> Fax (44) 1962 816800
> Lotus Notes: Steve Groeger/UK/IBM
> Internet: groeges@uk.ibm.com
> 
> 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
> 
> 
> 
> From: Jan Lahoda <jan.lahoda@oracle.com>
> To: Steve Groeger <GROEGES@uk.ibm.com>, compiler-dev@openjdk.java.net
> Date: 11/02/2019 12:45
> Subject: Re: RFR: [8218152] javac fails and exits with no error if a bad
> annotation processor is on the classpath
> ------------------------------------------------------------------------
> 
> 
> 
> Hi Steve,
> 
> I apologize for a late reply.
> 
> One thing to note is that according to the ServiceLoader spec, the
> LinkageErrors shouldn't be thrown out of the Iterator. That is my
> reading at least. Please see:
> https://bugs.openjdk.java.net/browse/JDK-8196182
> 
> Given there are some issues associated with that, I don't see a problem
> with workarounding that in javac. But I guess I'd personally rather kept
> it simple. Would there be an issue with simply changing the "catch
> (Throwable)" to also report the same error as "catch
> (ServiceConfigurationError)"? I.e. something like:
> ---
> > diff -r 4ef401769c36
> src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
> 
> > ---
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
>  Fri Feb 08 14:06:35 2019 +0100
> > +++
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
>  Mon Feb 11 13:33:50 2019 +0100
> > @@ -437,6 +437,7 @@
> > 
> log.error(Errors.ProcBadConfigFile(sce.getLocalizedMessage()));
> > throw new Abort(sce);
> > } catch (Throwable t) {
> > +
> log.error(Errors.ProcBadConfigFile(t.getLocalizedMessage()));
> > throw new Abort(t);
> > }
> > }
> > @@ -453,6 +454,7 @@
> > 
> log.error(Errors.ProcBadConfigFile(sce.getLocalizedMessage()));
> > throw new Abort(sce);
> > } catch (Throwable t) {
> > +
> log.error(Errors.ProcBadConfigFile(t.getLocalizedMessage()));
> > throw new Abort(t);
> > }
> > }
> ---
> 
> Looking at the tests, one thing to note is that binary files are
> generally not allowed in the repository. I wonder if a test that would
> run javac programmaticaly wouldn't be easier to do, see e.g.:
> test/langtools/tools/javac/processing/rounds/GetElementsAnnotatedWithOnMissing.java
> 
> This should allow arbitrary changes to the classfiles, so that binaries
> don't need to be in the repository.
> 
> Thanks,
> Jan
> 
> On 11.2.2019 12:30, Steve Groeger wrote:
> > Hi all,
> > 
> > Is there anyone out there that would be able to review the webrevs
> > mentioned in the earlier post.
> > Jonathan Gibbons has agreed to sponsor these changes but we need 2
> > reviewers. Anyone?
> > 
> > Thanks
> > Steve Groeger
> > IBM Runtime Technologies
> > Hursley, Winchester
> > Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
> > Fax (44) 1962 816800
> > Lotus Notes: Steve Groeger/UK/IBM
> > Internet: groeges@uk.ibm.com
> > 
> > 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
> > 
> > 
> > 
> > From: Steve Groeger <GROEGES@uk.ibm.com>
> > To: compiler-dev@openjdk.java.net
> > Date: 01/02/2019 12:47
> > Subject: RFR: [8218152] javac fails and exits with no error if a bad
> > annotation processor is on the classpath
> > Sent by: "compiler-dev" <compiler-dev-bounces@openjdk.java.net>
> > ------------------------------------------------------------------------
> > 
> > 
> > 
> > Hi all,
> > 
> > I have made some changes to the code for issue [1] for both jdk8u and
> > jdk (jdk13)
> > The latest webrevs have a few extra changes suggested by  Jonathan
> > Gibbons mentioned in an earlier mailing [2]
> > The webrevs are for jdk8u [3] and for jdk [4].
> > Please could someone review these changes and let me know if there are
> > any issues.
> > 
> > [1]
> _https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_J \
> DK-2D8218152-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=78GW2OHz7nNTH2dBkTx7-TKh2QCt3JD3 \
> zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=GXmb6NEDC3BhXyEw8uaNod2uN_hYX-9HpHkoiGhEujY&e=
> 
> > [2]
> > 
> _https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openjdk.java.net_pipermai \
> l_compiler-2Ddev_2019-2DJanuary_012920.html-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=7 \
> 8GW2OHz7nNTH2dBkTx7-TKh2QCt3JD3zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=AhWE854aM6al5na1Eo9sxF4IoWBZC3jfvLHRo2a2ETo&e=
> 
> > [3]
> _https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Esgroeger \
> _8218152_jdk8u_webrev.01_-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=78GW2OHz7nNTH2dBkTx \
> 7-TKh2QCt3JD3zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=OM__1ZIlBCUXiHNLfltEv9c_6a8k8nspR0KnKjRT8II&e=
> 
> > [4]
> _https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Esgroeger \
> _8218152_jdk_webrev.01_-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=78GW2OHz7nNTH2dBkTx7- \
> TKh2QCt3JD3zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=HrpL5tvveJrGX0ABN9uGRn0bZV1nsiNxzuLOAVem3Z0&e=
> 
> > 
> > Thanks
> > Steve Groeger
> > IBM Runtime Technologies
> > Hursley, Winchester
> > Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
> > Fax (44) 1962 816800
> > Lotus Notes: Steve Groeger/UK/IBM
> > Internet: groeges@uk.ibm.com
> > 
> > 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
> > 
> > 
> > 
> > From: Jonathan Gibbons <jonathan.gibbons@oracle.com>
> > To: Steve Groeger <GROEGES@uk.ibm.com>
> > Cc: compiler-dev@openjdk.java.net
> > Date: 31/01/2019 16:56
> > Subject: Re: [NEW_BUG] javac fails and exits with no error if a bad
> > annotation processor is on the classpath
> > ------------------------------------------------------------------------
> > 
> > 
> > 
> > Steve,
> > 
> > When you post the new webrevs, can I suggest that you change the Subject
> > line of this thread to something like
> > 
> > RFR: 8218152 [javac] fails and exits with no error if a bad annotation
> > processor provided
> > 
> > That will help get attention from reviewers, and help any subsequent
> > bug-archaeology.
> > 
> > I can sponsor the changes for you once they have been reviewed.
> > 
> > -- Jon
> > 
> > On 1/31/19 8:49 AM, Steve Groeger wrote:
> > Hi Jonathan,
> > 
> > Thanks for responding.
> > 
> > I have created a couple of webrevs for jdk8u [1] and jdk [2] which also
> > include some JTReg tests.
> > I would be grateful if you, or someone else could review these just to
> > ensure I am doing the right thing.
> > I will update the code with the additional 'throw FatalError' that you
> > mentioned and will then create some new webrevs
> > 
> > If you are able to sponsor these changes for me I would be very grateful.
> > 
> > PS. As you saw I have created a JBS issue for this here [3]
> > 
> > [1]
> _https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Esgroeger \
> _8218152_jdk8u_webrev.00_-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=78GW2OHz7nNTH2dBkTx \
> 7-TKh2QCt3JD3zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=JoEgW12V0tuoZvuG9GDF7g5YmXhjGolWyibAYlifWME&e=
> 
> > [2]
> _https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Esgroeger \
> _8218152_jdk_webrev.00_-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=78GW2OHz7nNTH2dBkTx7- \
> TKh2QCt3JD3zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=NeM6uUrEq-vcqXQ5uHonCor_ChwsY20notpASJS4is8&e=
> 
> > 
> [3_https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse \
> _JDK-2D8218152-5F&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=78GW2OHz7nNTH2dBkTx7-TKh2QCt3J \
> D3zukzeUO8RpA&m=3eyXaufRP0Xc78gxZMprIOVTveYQ-jSaXZglJ3ZxTMo&s=GXmb6NEDC3BhXyEw8uaNod2uN_hYX-9HpHkoiGhEujY&e=
> 
> > 
> > Thanks
> > Steve Groeger
> > IBM Runtime Technologies
> > Hursley, Winchester
> > Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
> > Fax (44) 1962 816800
> > Lotus Notes: Steve Groeger/UK/IBM
> > Internet: _groeges@uk.ibm.com_ <mailto:groeges@uk.ibm.com>
> > 
> > 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
> > 
> > 
> > 
> > From: Jonathan Gibbons _<jonathan.gibbons@oracle.com>_
> > <mailto:jonathan.gibbons@oracle.com>
> > To: Steve Groeger _<GROEGES@uk.ibm.com>_ <mailto:GROEGES@uk.ibm.com>,
> > _compiler-dev@openjdk.java.net_ <mailto:compiler-dev@openjdk.java.net>
> > Date: 31/01/2019 16:33
> > Subject: Re: [NEW_BUG] javac fails and exits with no error if a bad
> > annotation processor is on the classpath
> > 
> > ------------------------------------------------------------------------
> > 
> > 
> > 
> > Steve,
> > 
> > Your fix is generally good: Abort should only be used after a suitable
> > message has been reported.
> > 
> > There's a `catch (Throwable t)` in the same set of catch blocks that you
> > did not update. Rather than add yet another message for that case, it
> > might be better to throw FatalError, to provoke the standard "javac
> > crash" output for the unknown exception.
> > 
> > You should provide a test case ... i.e. as a jtreg test.
> > 
> > -- Jon
> > 
> > On 1/29/19 3:18 AM, Steve Groeger wrote:
> > Hi all,
> > 
> > I have identified a potential issue where javac fails and exits with no
> > error if a bad annotation processor is on the classpath.
> > 
> > Background: an Annotation Processor class can be packaged in a jar file
> > and is identified by a special text file inside the jar named:
> > META-INF/services/javax.annotation.processing.Processor. This text file
> > has only one line, specifying the class name of the annotation process
> > class to run. When javac runs, it checks all jars on the classpath and
> > if it finds the special text file in any jar file, then it attempts to
> > run the class listed there.
> > 
> > The issue here is that when the annotation processor class can't be
> > executed, javac catches the exception and exits without reporting on the
> > reason for the exit. Examples of reasons why the annotation processor
> > can't be executed include:
> > 
> > * the annotation processor class is compiled for a higher version of
> > Java than the javac is running, giving UnsupportedClassVersionError.
> > * the annotation processor class file is corrupt so can't be loaded.
> > 
> > 
> > In either of the above cases javac will swallow the error and exit
> > without telling the user why it failed to compile the java source code
> > as expected.
> > 
> > Testcase
> > put HelloWorld.java in current directory, edit <path_to_jar> and run:
> > javac -cp .:<path_to_jar>/bad.annotation.processor.jar
> > HelloWorld.java
> > 
> > Instead of producing HelloWorld.class as expected, javac exits silently.
> > 
> > Here, bad.annotation.processor.jar is a file that I created to contain:
> > META-INF/services/javax.annotation.processing.Processor <<-- text
> > file to list bad class
> > bad/notaclass.class <<-- dummy file, can't be loaded as a java class
> > 
> > Does anyone know whether this is a bug and that this should throw an
> > error in these cases?
> > If so, I will raise a bug for this.
> > 
> > I have looked at the code and seen where javac Aborts the processing,
> > and if I make the following changes the javac detects the issue and
> > reports an error before Aborting.
> > 
> > diff -r 5178e4b58b17
> > 
> src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
> 
> > ---
> > 
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
> 
> > Mon Jan 28 09:56:00 2019 +0100
> > +++
> > 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
> 
> > Tue Jan 29 11:14:57 2019 +0000
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All rights
> > reserved.
> > + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All rights
> > reserved.
> > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > *
> > * This code is free software; you can redistribute it and/or modify it
> > @@ -436,6 +436,12 @@
> > } catch(ServiceConfigurationError sce) {
> > 
> > log.error(Errors.ProcBadConfigFile(sce.getLocalizedMessage()));
> > throw new Abort(sce);
> > +            } catch (UnsupportedClassVersionError ucve) {
> > +
> > log.error(Errors.ProcCantLoadClass(ucve.getLocalizedMessage()));
> > +                throw new Abort(ucve);
> > +            } catch (ClassFormatError cfe) {
> > +
> > log.error(Errors.ProcCantLoadClass(cfe.getLocalizedMessage()));
> > +                throw new Abort(cfe);
> > } catch (Throwable t) {
> > throw new Abort(t);
> > }
> > 
> > and to add a new message there is this change
> > 
> > diff -r 5178e4b58b17
> > 
> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> > ---
> > 
> a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> > Mon Jan 28 09:56:00 2019 +0100
> > +++
> > 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> > Tue Jan 29 11:16:03 2019 +0000
> > @@ -1,5 +1,5 @@
> > #
> > -# Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights
> > reserved.
> > +# Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights
> > reserved.
> > # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > #
> > # This code is free software; you can redistribute it and/or modify it
> > @@ -1051,6 +1051,10 @@
> > compiler.err.proc.cant.find.class=\
> > Could not find class file for ''{0}''.
> > 
> > +# 0: string
> > +compiler.err.proc.cant.load.class=\
> > +    Could not load processor class file due to ''{0}''.
> > +>  > # Print a client-generated error message; assumed to be localized, no
> > translation required
> > # 0: string
> > compiler.err.proc.messager=\
> > 
> > 
> > If this is deemed a bug and the change seems OK, please could someone
> > sponsor this for me and I will create a proper webrev for the change for
> > a full review.
> > 
> > Thanks
> > Steve Groeger
> > IBM Runtime Technologies
> > Hursley, Winchester
> > Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
> > Fax (44) 1962 816800
> > Lotus Notes: Steve Groeger/UK/IBM
> > Internet: _groeges@uk.ibm.com_ <mailto:groeges@uk.ibm.com>
> > 
> > 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
> > 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
> > 
> > 
> > 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
> > 
> > 
> > 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
> > 
> > 
> > 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
> 
> 
> 
> 
> 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


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic