Fix variable lexical scoping

This implements a scope stack to prevent blocks from accessing variables outside of their lexical scope. Example, which is no longer valid: ``` BEGIN { if (0) { $var = 1; } print($var) } ``` We need to do this for: - While - For - If (both the if and else blocks) - Unroll - Probe - Subprog Issue: #3017
bpftrace · Oct 25, 2024 · 8d8c06e · 8d8c06e
1 parent 673a6b9
commit 8d8c06e
Show file tree

Hide file tree

Showing 20 changed files with 549 additions and 261 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,9 @@ and this project adheres to
 - Remove multi-map `delete` functionality
   - [#3506](https://github.com/bpftrace/bpftrace/pull/3506)
   - [Migration guide](docs/migration_guide.md#multi-key-delete-removed)
+- Add lexical/block scoping for variables
+  - [#3367](https://github.com/bpftrace/bpftrace/pull/3367)
+  - [Migration guide](docs/migration_guide.md#added-block-scoping-for-scratch-variables)
 #### Added
 - Add `--dry-run` CLI option
   - [#3203](https://github.com/bpftrace/bpftrace/pull/3203)

diff --git a/docs/migration_guide.md b/docs/migration_guide.md
@@ -9,6 +9,78 @@ compatibility in some way. Each entry should contain:
 
 ## Versions 0.21.x (or earlier) to 0.22.x (or later)
 
+### Added block scoping for scratch variables
+
+https://github.com/bpftrace/bpftrace/pull/3367
+
+Previously, scratch variables were "probe" scoped meaning the following
+was valid syntax:
+```
+BEGIN {
+    if (0) {
+        $x = "hello";
+    }
+    print(($x));
+}
+
+// prints an empty line
+```
+However, the value of `$x` at the print statement was considered undefined
+behavior. Issue: https://github.com/bpftrace/bpftrace/issues/3017
+
+Now variables are "block" scoped and the the above will throw an error at the 
+print statement: "ERROR: Undefined or undeclared variable: $x".
+
+If you see this error you can do multiple things to resolve it.
+
+**Option 1: Initialize variable before use**
+```
+BEGIN {
+    $x = "";
+    if (0) {
+        $x = "hello";
+    }
+    print(($x));
+}
+```
+
+**Option 2: Declare variable before use**
+```
+BEGIN {
+    let $x;
+    // let $x = ""; is also valid
+    if (0) {
+        $x = "hello";
+    }
+    print(($x));
+}
+```
+Declaring is useful for variables that hold internal bpftrace types
+e.g. the type returned by the `macaddr` function.
+
+This is also not valid even though `$x` is set in both branches (`$x` still 
+needs to exist in the outer scope):
+```
+BEGIN {
+    if (0) {
+        $x = "hello";
+    } else {
+        $x = "bye";
+    }
+    print(($x));
+}
+```
+
+Additionally, scratch variable shadowing is not allowed e.g. this is not valid:
+```
+BEGIN {
+    let $x;
+    if (0) {
+        let $x = "hello"; // shadows $x in the parent scope
+    }
+}
+```
+
 ### multi-key `delete` removed
 
 https://github.com/bpftrace/bpftrace/pull/3506

diff --git a/man/adoc/bpftrace.adoc b/man/adoc/bpftrace.adoc
@@ -941,13 +941,27 @@ fentry:tcp_connect {
 
 bpftrace knows two types of variables, 'scratch' and 'map'.
 
-'scratch' variables are kept on the BPF stack and only exists during the execution of the action block and cannot be accessed outside of the program.
-Scratch variable names always start with a `$`, e.g. `$myvar`.
+'scratch' variables are kept on the BPF stack and their names always start
+with a `$`, e.g. `$myvar`.
+'scratch' variables cannot be accessed outside of their lexical block e.g.
+```
+$a = 1;
+if ($a == 1) {
+  $b = "hello"
+  $a = 2;
+}
+// $b is not accessible here
+```
 
 'scratch' variables can also declared before or during initialization with `let` e.g.
 ```
-let $x;
-let $y = 11;
+let $a = 1;
+let $b;
+if ($a == 1) {
+  $b = "hello"
+  $a = 2;
+}
+// $b IS accessible here and would be an empty string if the condition wasn't true
 ```
 
 If no assignment is specified variables will initialize to 0.

diff --git a/src/ast/ast.cpp b/src/ast/ast.cpp
@@ -43,6 +43,7 @@ MAKE_ACCEPT(Unroll)
 MAKE_ACCEPT(While)
 MAKE_ACCEPT(For)
 MAKE_ACCEPT(Config)
+MAKE_ACCEPT(Block)
 MAKE_ACCEPT(Jump)
 MAKE_ACCEPT(Probe)
 MAKE_ACCEPT(SubprogArg)
@@ -242,31 +243,33 @@ AttachPoint::AttachPoint(const std::string &raw_input, location loc)
 {
 }
 
-If::If(Expression *cond, StatementList &&stmts)
-    : cond(cond), stmts(std::move(stmts))
+Block::Block(StatementList &&stmts) : stmts(std::move(stmts))
 {
 }
 
-If::If(Expression *cond, StatementList &&stmts, StatementList &&else_stmts)
-    : cond(cond), stmts(std::move(stmts)), else_stmts(std::move(else_stmts))
+If::If(Expression *cond, StatementList &&stmts)
+    : cond(cond), if_block(Block(std::move(stmts))), else_block(Block({}))
 {
 }
 
-Unroll::Unroll(Expression *expr, StatementList &&stmts, location loc)
-    : Statement(loc), expr(expr), stmts(std::move(stmts))
+If::If(Expression *cond, StatementList &&stmts, StatementList &&else_stmts)
+    : cond(cond),
+      if_block(Block(std::move(stmts))),
+      else_block(Block(std::move(else_stmts)))
 {
 }
 
-Scope::Scope(StatementList &&stmts) : stmts(std::move(stmts))
+Unroll::Unroll(Expression *expr, StatementList &&stmts, location loc)
+    : Statement(loc), expr(expr), block(std::move(stmts))
 {
 }
 
 Probe::Probe(AttachPointList &&attach_points,
              Predicate *pred,
              StatementList &&stmts)
-    : Scope(std::move(stmts)),
-      attach_points(std::move(attach_points)),
-      pred(pred)
+    : attach_points(std::move(attach_points)),
+      pred(pred),
+      block(std::move(stmts))
 {
 }
 
@@ -284,9 +287,9 @@ Subprog::Subprog(std::string name,
                  SizedType return_type,
                  SubprogArgList &&args,
                  StatementList &&stmts)
-    : Scope(std::move(stmts)),
-      args(std::move(args)),
+    : args(std::move(args)),
       return_type(std::move(return_type)),
+      stmts(std::move(stmts)),
       name_(std::move(name))
 {
 }

diff --git a/src/ast/ast.h b/src/ast/ast.h
@@ -430,6 +430,18 @@ class AssignConfigVarStatement : public Statement {
   AssignConfigVarStatement(const AssignConfigVarStatement &other) = default;
 };
 
+class Block : public Statement {
+public:
+  DEFINE_ACCEPT
+
+  Block(StatementList &&stmts);
+
+  StatementList stmts;
+
+private:
+  Block(const Block &other) = default;
+};
+
 class If : public Statement {
 public:
   DEFINE_ACCEPT
@@ -438,8 +450,8 @@ class If : public Statement {
   If(Expression *cond, StatementList &&stmts, StatementList &&else_stmts);
 
   Expression *cond = nullptr;
-  StatementList stmts;
-  StatementList else_stmts;
+  Block if_block;
+  Block else_block;
 
 private:
   If(const If &other) = default;
@@ -453,7 +465,7 @@ class Unroll : public Statement {
 
   long int var = 0;
   Expression *expr = nullptr;
-  StatementList stmts;
+  Block block;
 
 private:
   Unroll(const Unroll &other) = default;
@@ -507,12 +519,12 @@ class While : public Statement {
   DEFINE_ACCEPT
 
   While(Expression *cond, StatementList &&stmts, location loc)
-      : Statement(loc), cond(cond), stmts(std::move(stmts))
+      : Statement(loc), cond(cond), block(std::move(stmts))
   {
   }
 
   Expression *cond = nullptr;
-  StatementList stmts;
+  Block block;
 
 private:
   While(const While &other) = default;
@@ -530,7 +542,6 @@ class For : public Statement {
   Variable *decl = nullptr;
   Expression *expr = nullptr;
   StatementList stmts;
-
   SizedType ctx_type;
 
 private:
@@ -551,14 +562,6 @@ class Config : public Statement {
   Config(const Config &other) = default;
 };
 
-class Scope : public Node {
-public:
-  Scope(StatementList &&stmts);
-  virtual ~Scope() = default;
-
-  StatementList stmts;
-};
-
 class AttachPoint : public Node {
 public:
   DEFINE_ACCEPT
@@ -606,7 +609,7 @@ class AttachPoint : public Node {
 };
 using AttachPointList = std::vector<AttachPoint *>;
 
-class Probe : public Scope {
+class Probe : public Node {
 public:
   DEFINE_ACCEPT
 
@@ -616,6 +619,7 @@ class Probe : public Scope {
 
   AttachPointList attach_points;
   Predicate *pred = nullptr;
+  Block block;
 
   std::string name() const;
   std::string args_typename() const;
@@ -649,7 +653,7 @@ class SubprogArg : public Node {
 };
 using SubprogArgList = std::vector<SubprogArg *>;
 
-class Subprog : public Scope {
+class Subprog : public Node {
 public:
   DEFINE_ACCEPT
 
@@ -660,6 +664,7 @@ class Subprog : public Scope {
 
   SubprogArgList args;
   SizedType return_type;
+  StatementList stmts;
 
   std::string name() const;
 

diff --git a/src/ast/passes/callback_visitor.h b/src/ast/passes/callback_visitor.h
@@ -35,14 +35,15 @@ class CallbackVisitor : public Visitor {
   void visit(AssignMapStatement &assignment) override;
   void visit(AssignVarStatement &assignment) override;
   void visit(AssignConfigVarStatement &assignment) override;
-  void visit(If &if_block) override;
+  void visit(If &if_node) override;
   void visit(Unroll &unroll) override;
   void visit(While &while_block) override;
   void visit(Jump &jump) override;
   void visit(Predicate &pred) override;
   void visit(AttachPoint &ap) override;
   void visit(Probe &probe) override;
   void visit(Config &config) override;
+  void visit(Block &block) override;
   void visit(Program &program) override;
 
 private: