spotBugs and JDK-8194978: Javac produces dead code for try-with-resource
Enrico Olivelli
eolivelli at gmail.com
Mon Feb 4 22:22:31 UTC 2019
Jan, sorry for late reply.
I appreciate your explanation.
Best regards
Enrico
Il giorno lun 28 gen 2019, 14:40 Jan Lahoda <jan.lahoda at oracle.com> ha
scritto:
> Hi Enrico,
>
> One thing I'd like to point out is that I don't think that JDK-8194978
> added any new checks. There have been improvements to the
> try-with-resources desugaring over the past years, usually with the goal
> of making the bytecode shorter.
>
> So, for example, having:
> try (InputStream in = new FileInputStream(...)) {
> ...
> }
>
> AFAIK, originally, before "close()" was called on "in" there was a check
> if "in" is non-null, which happened every time. In case the resources is
> initialized with the new class instance expression, this check was
> removed, but in all other cases it was still there, as far as I known.
> (And I suspect Find/SpotBugs have heuristics to detect this pattern and
> ignore it.)
>
> The $closeResource method was also introduced to make the bytecode
> smaller, but was only used when there were enough try-with-resources
> constructs in the given file, as the method itself also had an space
> overhead. So some classfiles had the try-with-resources code without
> $closeResource.
>
> With JDK-8194978, the code generated for try-with-resources has been
> improved once again, to eliminate some of the branches in the "finally"
> block that cannot be used at the given places where finally is inlined.
> This made the $closeResource method unnecessary, so it is not produced
> anymore.
>
> Basically, here we are talking about eliding the null check on the
> resource if the resource must be non-null because a NullPointerException
> would already appear before reaching the point where the check is done.
> I don't think there are current plans to do that, sorry.
>
> If SpotBugs can detect the new pattern and ignore it (as I suspect it is
> doing with the older patterns), that would be helpful.
>
> Jan
>
> On 25.1.2019 21:44, Enrico Olivelli wrote:
> > Up
> >
> > Regards
> > Enrico
> >
> > Il 10 gen 2019 8:13 PM, "Enrico Olivelli" <eolivelli at gmail.com
> > <mailto:eolivelli at gmail.com>> ha scritto:
> >
> > Kindly pinging on this..
> >
> > Cheers
> > Enrico
> >
> > Il lun 7 gen 2019, 14:28 Enrico Olivelli <eolivelli at gmail.com
> > <mailto:eolivelli at gmail.com>> ha scritto:
> >
> > Two questions/ideas:
> > 1) Wouldn't it be possible for javac to annotate such code
> > blocks and
> > write into the class file that that code is "synthetic" ? I
> really
> > don't know where to store such flags, but maybe someone could
> > elaborate more this idea
> > 2) Why isn't javac adding the "$closeResource" method ?
> >
> > Enrico
> >
> > Il giorno ven 9 nov 2018 alle ore 23:01 Alex Buckley
> > <alex.buckley at oracle.com <mailto:alex.buckley at oracle.com>> ha
> > scritto:
> > >
> > > On 11/9/2018 12:31 PM, Vicente Romero wrote:
> > > > My evaluation on this issue is that javac is generating code
> > > > according to the spec, see JLS11 14.20.3.1 [1]. There is an
> > explicit
> > > > check for null on the resource before invoking the close()
> > method.
> > > > This is the null check javac is generating.
> > >
> > > I confirm that the explicit null check on the resource
> > variable is
> > > required, per the translation in
> > >
> >
> https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-14.20.3.1-140
> > <
> https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-14.20.3.1-140
> >
> > > -- it has been specified that way since JLS7.
> > >
> > > > Fix for JDK-8194978 went a bit further trying to eliminate
> > dead code
> > > > for some cases for which javac could statically determine
> > that the
> > > > resource would be different from null. In particular if the
> > resource
> > > > was initialized with a new class expression.
> > >
> > > I confirm it's acceptable to not perform the null check in
> > generated
> > > code if you can prove the resource variable is non-null.
> Frankly,
> > > though, there is so much that can occur in a "new class
> > expression" --
> > > `new Outer(a(b(c)))<>.new <String>Inner(d(e(f)))` -- that I
> > would be
> > > wary of trying to prove anything.
> > >
> > > > It could be argued that we could try to analyze the body
> > and if we
> > > > find out that a NPE must be thrown in the body of the try,
> > then the
> > > > null check on the resource would be deemed unnecessary. But
> > then we
> > > > can get to an implementation that will be out of sync with
> > the spec
> > > > plus it probably won't cover all cases.
> > >
> > > Right, let's not be too clever.
> > >
> > > Alex
> > >
> > > > Thanks,
> > > > Vicente
> > > >
> > > > [1]
> > > >
> >
> https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-14.20.3.1
> > <
> https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-14.20.3.1
> >
> > > >
> > > > On 11/9/18 10:22 AM, Ismael Juma wrote:
> > > >> Any comments on this? Many people are disabling spotBugs
> when
> > > >> compiling with Java 11 due to this issue:
> > > >>
> > > >> https://github.com/spotbugs/spotbugs/issues/756
> > <https://github.com/spotbugs/spotbugs/issues/756>
> > > >>
> > > >> Ismael
> > > >>
> > > >> On Thu, Sep 13, 2018 at 10:05 PM Ismael Juma
> > <mlists at juma.me.uk <mailto:mlists at juma.me.uk>
> > > >> <mailto:mlists at juma.me.uk <mailto:mlists at juma.me.uk>>>
> wrote:
> > > >>
> > > >> Hi all,
> > > >>
> > > >> JDK-8194978 introduced some changes to the bytecode
> > generated by
> > > >> javac for the try with resource construct. In the
> > following code,
> > > >> it seems to generate a null check on a reference after
> > invoking a
> > > >> method on it:
> > > >>
> > > >> public static void readFileAsString(String path)
> > throws
> > > >> IOException {
> > > >> try (FileChannel fc =
> > FileChannel.open(Paths.get(path))) {
> > > >> fc.size();
> > > >> }
> > > >>
> > > >> }
> > > >>
> > > >> In line 16 to 22 of the bytecode, it looks like we
> > check for null
> > > >> after calling a method on the fc reference:
> > > >>
> > > >> 16: aload_1
> > > >> 17: invokevirtual #6 // Method
> > > >> java/nio/channels/FileChannel.size:()J
> > > >> 20: pop2
> > > >> 21: aload_1
> > > >> 22: ifnull 52
> > > >> 25: aload_1
> > > >> 26: invokevirtual #7 // Method
> > > >> java/nio/channels/FileChannel.close:()V
> > > >>
> > > >> Is this intentional? I ask because this pattern
> triggers a
> > > >> spotBugs warning since it often implies a bug in
> > user's code:
> > > >>
> > > >> RCN | Nullcheck of fc at line 10 of value previously
> > dereferenced
> > > >> in TryTest.readFileAsString(String, Charset)
> > > >>
> > > >> Note that this works fine in Java versions older than
> > Java 11.
> > > >> Since this spotBugs warning is generally useful, it
> > would be handy
> > > >> if javac did not trigger it. Alternatively, if there's
> > a good way
> > > >> to detect the code that was generated by javac,
> > spotBugs could be
> > > >> updated to ignore it. For reference, this was
> > discussed in the
> > > >> spotBugs issue tracker:
> > > >>
> > > >> https://github.com/spotbugs/spotbugs/issues/756
> > <https://github.com/spotbugs/spotbugs/issues/756>
> > > >>
> > > >> And method bytecode in full:
> > > >>
> > > >> public static void
> > readFileAsString(java.lang.String) throws
> > > >> java.io.IOException;
> > > >> Code:
> > > >> 0: aload_0
> > > >> 1: iconst_0
> > > >> 2: anewarray #2 // class
> > java/lang/String
> > > >> 5: invokestatic #3 // Method
> > > >>
> >
> java/nio/file/Paths.get:(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path;
> > > >> 8: iconst_0
> > > >> 9: anewarray #4 // class
> > > >> java/nio/file/OpenOption
> > > >> 12: invokestatic #5 // Method
> > > >>
> >
> java/nio/channels/FileChannel.open:(Ljava/nio/file/Path;[Ljava/nio/file/OpenOption;)Ljava/nio/channels/FileChannel;
> > > >> 15: astore_1
> > > >> 16: aload_1
> > > >> 17: invokevirtual #6 // Method
> > > >> java/nio/channels/FileChannel.size:()J
> > > >> 20: pop2
> > > >> 21: aload_1
> > > >> 22: ifnull 52
> > > >> 25: aload_1
> > > >> 26: invokevirtual #7 // Method
> > > >> java/nio/channels/FileChannel.close:()V
> > > >> 29: goto 52
> > > >> 32: astore_2
> > > >> 33: aload_1
> > > >> 34: ifnull 50
> > > >> 37: aload_1
> > > >> 38: invokevirtual #7 // Method
> > > >> java/nio/channels/FileChannel.close:()V
> > > >> 41: goto 50
> > > >> 44: astore_3
> > > >> 45: aload_2
> > > >> 46: aload_3
> > > >> 47: invokevirtual #9 // Method
> > > >>
> java/lang/Throwable.addSuppressed:(Ljava/lang/Throwable;)V
> > > >> 50: aload_2
> > > >> 51: athrow
> > > >> 52: return
> > > >> Exception table:
> > > >> from to target type
> > > >> 16 21 32 Class java/lang/Throwable
> > > >> 37 41 44 Class java/lang/Throwable
> > > >>
> > > >> Thanks,
> > > >> Ismael
> > > >>
> > > >
> >
> > --
> >
> >
> > -- Enrico Olivelli
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190204/8fbde81c/attachment-0001.html>
More information about the compiler-dev
mailing list