From fe785e107d0f05bf8529d1460e017cbf5de184ec Mon Sep 17 00:00:00 2001 From: brendandouglas Date: Tue, 12 Jun 2018 09:39:38 -0700 Subject: Skylark debugger: Fix NPE listing frames. Environment$Continuation#caller is most definitely nullable in practice. I suspect it's a bug in skylark itself, but for now, just properly mark it nullable and handle it in the debugger. Stack trace from NPE: Caused by: java.lang.NullPointerException at com.google.devtools.build.lib.syntax.Environment.listFrames(Environment.java:1197) at com.google.devtools.build.lib.syntax.Environment.listFrames(Environment.java:81) at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.listFrames(ThreadHandler.java:236) at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.getThreadProto(ThreadHandler.java:345) at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.pauseCurrentThread(ThreadHandler.java:289) at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.pauseIfNecessary(ThreadHandler.java:203) at com.google.devtools.build.lib.skylarkdebug.server.SkylarkDebugServer.pauseIfNecessary(SkylarkDebugServer.java:158) at com.google.devtools.build.lib.skylarkdebug.server.SkylarkDebugServer$DebugAwareEval.exec(SkylarkDebugServer.java:262) at com.google.devtools.build.lib.syntax.UserDefinedFunction.call(UserDefinedFunction.java:91) at com.google.devtools.build.lib.syntax.BaseFunction.callWithArgArray(BaseFunction.java:462) at com.google.devtools.build.lib.syntax.BaseFunction.call(BaseFunction.java:440) at com.google.devtools.build.lib.analysis.skylark.SkylarkRuleConfiguredTargetUtil.lambda$buildRule$1(SkylarkRuleConfiguredTargetUtil.java:105) at com.google.devtools.build.lib.skylarkdebug.server.SkylarkDebugServer.runWithDebugging(SkylarkDebugServer.java:142) at com.google.devtools.build.lib.syntax.DebugServerUtils.runWithDebuggingIfEnabled(DebugServerUtils.java:70) at com.google.devtools.build.lib.analysis.skylark.SkylarkRuleConfiguredTargetUtil.buildRule(SkylarkRuleConfiguredTargetUtil.java:100) PiperOrigin-RevId: 200229036 --- .../lib/skylarkdebug/proto/skylark_debugging.proto | 3 ++- .../build/lib/skylarkdebug/server/DebugEventHelper.java | 13 ++++++++----- .../devtools/build/lib/syntax/BuiltinCallable.java | 3 +-- .../devtools/build/lib/syntax/BuiltinFunction.java | 3 +-- .../google/devtools/build/lib/syntax/Environment.java | 17 +++++++++++------ 5 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto index f81185f20a..3d74f951af 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto +++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto @@ -213,7 +213,8 @@ message Frame { // The scopes that contain value bindings accessible in this frame. repeated Scope scope = 2; - // The source location where the frame is currently paused. + // The source location where the frame is currently paused. May not be set in + // some situations. Location location = 3; } diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java index 0254484062..a389569bc2 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java @@ -159,11 +159,14 @@ final class DebugEventHelper { } static SkylarkDebuggingProtos.Frame getFrameProto(DebugFrame frame) { - return SkylarkDebuggingProtos.Frame.newBuilder() - .setFunctionName(frame.functionName()) - .setLocation(getLocationProto(frame.location())) - .addAllScope(getScopes(frame)) - .build(); + SkylarkDebuggingProtos.Frame.Builder builder = + SkylarkDebuggingProtos.Frame.newBuilder() + .setFunctionName(frame.functionName()) + .addAllScope(getScopes(frame)); + if (frame.location() != null) { + builder.setLocation(getLocationProto(frame.location())); + } + return builder.build(); } private static ImmutableList getScopes(DebugFrame frame) { diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java index 27bf364385..9a0a56b592 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java @@ -93,8 +93,7 @@ public class BuiltinCallable extends BaseFunction { @Override @Nullable - public Object call(Object[] args, - FuncallExpression ast, Environment env) + public Object call(Object[] args, @Nullable FuncallExpression ast, Environment env) throws EvalException, InterruptedException { Preconditions.checkNotNull(env); diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java index 03d24bd0a6..469df1e3e7 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java @@ -138,8 +138,7 @@ public class BuiltinFunction extends BaseFunction { @Override @Nullable - public Object call(Object[] args, - FuncallExpression ast, Environment env) + public Object call(Object[] args, @Nullable FuncallExpression ast, Environment env) throws EvalException, InterruptedException { Preconditions.checkNotNull(env); diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java index 6f84188b4b..5b22906d71 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java @@ -463,7 +463,7 @@ public final class Environment implements Freezable, Debuggable { final BaseFunction function; /** The {@link FuncallExpression} to which this Continuation will return. */ - final FuncallExpression caller; + @Nullable final FuncallExpression caller; /** The next Continuation after this Continuation. */ @Nullable final Continuation continuation; @@ -478,12 +478,12 @@ public final class Environment implements Freezable, Debuggable { @Nullable final LinkedHashSet knownGlobalVariables; Continuation( - Continuation continuation, + @Nullable Continuation continuation, BaseFunction function, - FuncallExpression caller, + @Nullable FuncallExpression caller, LexicalFrame lexicalFrame, GlobalFrame globalFrame, - LinkedHashSet knownGlobalVariables) { + @Nullable LinkedHashSet knownGlobalVariables) { this.continuation = continuation; this.function = function; this.caller = caller; @@ -719,13 +719,17 @@ public final class Environment implements Freezable, Debuggable { /** * Enters a scope by saving state to a new Continuation + * * @param function the function whose scope to enter * @param lexical the lexical frame to use * @param caller the source AST node for the caller * @param globals the global Frame that this function closes over from its definition Environment */ void enterScope( - BaseFunction function, LexicalFrame lexical, FuncallExpression caller, GlobalFrame globals) { + BaseFunction function, + LexicalFrame lexical, + @Nullable FuncallExpression caller, + GlobalFrame globals) { continuation = new Continuation( continuation, function, caller, lexicalFrame, globalFrame, knownGlobalVariables); @@ -1194,7 +1198,8 @@ public final class Environment implements Freezable, Debuggable { .build()); currentFrame = currentContinuation.lexicalFrame; - currentLocation = currentContinuation.caller.getLocation(); + currentLocation = + currentContinuation.caller != null ? currentContinuation.caller.getLocation() : null; currentContinuation = currentContinuation.continuation; } -- cgit v1.2.3