RFR: JDK-8230105: -XDfind=diamond crashes
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Wed Aug 28 20:00:09 UTC 2019
I agree with Vicente, maybe replacing NON_SPECULATIVE with NORMAL (or
FULL) :-)
Maurizio
On 28/08/2019 20:03, Vicente Romero wrote:
> Hi Jan,
>
> I like the patch, I think it is very sensible. I just have a cosmetic
> comment. I think that rebranding speculative attribution to a concept
> based on its side-effects could bring confusion in the future as the
> concept of speculative attribution is very deep in the understanding
> we have of the attribution process. So what I'm proposing is just a
> change in the name to the three states you defined, just a proposal:
>
> enum AttributionMode {
> NON_SPECULATIVE(false),
> ANALYZER(true)
> SPECULATIVE(true);
>
> AttributionMode(boolean isSpeculative) {
> this.isSpeculative = isSpeculative;
> }
>
> boolean isSpeculative() {
> return isSpeculative;
> }
>
> final isSpeculative;
> }
>
> we can work on the names but my proposal is to put the
> speculative-ness of the attribution foremost and the side-effect-ness
> in second place. What do you think?
>
> Vicente
>
> On 8/26/19 6:50 AM, Jan Lahoda wrote:
>> Hi,
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8230105
>> Proposed webrev: http://cr.openjdk.java.net/~jlahoda/8230105/webrev.00/
>>
>> Consider this code:
>> ---
>> public class AnalyzerNotQuiteSpeculative {
>> private void test() {
>> Subclass1 c1 = null;
>> Subclass2 c2 = null;
>> Base b = null;
>>
>> t(new C<Base>(c1).set(c2));
>> t(new C<Base>(b).set(c2));
>> }
>>
>> public static class Base {}
>> public static class Subclass1 extends Base {}
>> public static class Subclass2 extends Base {}
>> public class C<T extends Base> {
>> public C(T t) {}
>> public C<T> set(T t) { return this; }
>> }
>> <T extends Base> void t(C<? extends Base> l) {}
>> }
>> ---
>>
>> When run with -XDfind=diamond, this crashes with:
>> ---
>>
>> at
>> jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:162)
>> at
>> jdk.compiler/com.sun.tools.javac.comp.Analyzer.doAnalysis(Analyzer.java:581)
>> ...
>> ---
>>
>> The embedded (thrown away) exception is:
>> ---
>> java.lang.NullPointerException
>> at
>> jdk.compiler/com.sun.tools.javac.comp.Analyzer$DiamondInitializer.process(Analyzer.java:261)
>> at
>> jdk.compiler/com.sun.tools.javac.comp.Analyzer$DiamondInitializer.process(Analyzer.java:225)
>> at
>> jdk.compiler/com.sun.tools.javac.comp.Analyzer.doAnalysis(Analyzer.java:579)
>> at
>> jdk.compiler/com.sun.tools.javac.comp.Analyzer$2.flush(Analyzer.java:552)
>> at
>> jdk.compiler/com.sun.tools.javac.comp.Analyzer.flush(Analyzer.java:594)
>> at
>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1412)
>> at
>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1380)
>> at
>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:972)
>> at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:318)
>> at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
>> at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:57)
>> at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:43)
>> ---
>>
>> The reason for this exception seems to be that when the Analyzer is
>> speculatively analyzing "t(new C<Base>(c1).set(c2));",
>> ArgumentAttr.setResult will set a deferred type to "new
>> C<Base>(c1).set(c2)" (because isSpeculative is true), which in turn
>> will prevent the tree from being actually attributed. So when the
>> analyzer asks for type of "new C<Base>(c1)", it gets null.
>>
>> It ultimate problem appears to be that analyzers want to analyze a
>> piece of AST "as if" it was part of the source code, but without
>> having permanent side effects. But, currently, there is only the
>> AttrContext.isSpeculative boolean, which means both "no permanent
>> sideeffects" and "the tree is a scratch tree, that can be modified in
>> any way", and the latter is not quite the case when doing spec
>> attribution for analyzers.
>>
>> The proposed solution is to make the isSpeculative flag 3 tristate
>> (by using enum and renaming the field), which should allow us to
>> differentiate between "actual AST", "AST in analyzer" and "actual
>> speculative AST".
>>
>> I also created a patch that runs analyzers on all source code which
>> building and testing JDK, and fixed a few smallish trouble it found.
>> The patch to run the analyzers is not part of the proposed patch, but
>> can be seen here:
>> http://cr.openjdk.java.net/~jlahoda/8230105/webrev.runall/
>>
>> How does this look?
>>
>> Thanks,
>> Jan
>
More information about the compiler-dev
mailing list