spotBugs and JDK-8194978: Javac produces dead code for try-with-resource
Jan Lahoda
jan.lahoda at oracle.com
Mon Jan 28 13:40:23 UTC 2019
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
>
More information about the compiler-dev
mailing list