Fully implement ++ and -- #486

ajor · 2019-03-19T23:52:40Z

Issue #412

ajor · 2019-03-19T23:53:57Z

Despite the title I've only done ++ for maps so far, but -- and variables will be very similar

ajor · 2019-03-22T22:45:11Z

I'm not very pleased with the output of the AST printer, but I'm not sure a better way to represent it.

y = ++x

=
 y
 ++ (pre)
  x

y = x++

=
 y
 ++ (post)
  x

It also makes error messages in the semantic analyser a bit uglier, e.g. $x = "blah"; $x++:
The ++ (post) operator can not be used on expressions of type 'string'

Maybe something like this, but suggestions are welcome:
y = ++x

=
 y
 ++
  x

y = x++

=
 y
 x
  ++

brendangregg · 2019-03-23T06:35:34Z

Second examples look more intuitive.

dalehamel · 2019-03-23T15:33:39Z

src/ast/ast.h

@@ -111,9 +111,11 @@ class Binop : public Expression {

 class Unop : public Expression {
 public:
-  Unop(int op, Expression *expr) : expr(expr), op(op) { }
+  Unop(int op, Expression *expr, bool is_post_op = false)


I like this approach of using the unary operator. I had considered something similar but couldn't quite get it figured out

dalehamel · 2019-03-23T15:34:34Z

src/ast/codegen_llvm.cpp

+          b_.CreateMapUpdateElem(map, key, newval);
+          b_.CreateLifetimeEnd(key);
+
+          if (unop.is_post_op)


Yes, this is exactly how I was thinking of implementing pre/post 👍

dalehamel · 2019-03-23T15:37:46Z

src/ast/codegen_llvm.cpp

+        }
+        break;
+      }
+      case bpftrace::Parser::token::MINUSMINUS:


We can probably reuse most of the ++ code, and just change:

b_.CreateStore(b_.CreateAdd(oldval, b_.getInt64(1)), newval);

To CreateSub. Not sure of the cleanest way to DRY this up but it shouldn't be too hard.

dalehamel · 2019-03-23T15:39:55Z

I agree the second printing example is more intuitive.

This approach looks good! It's a simpler version of what I had in mind, overloading the unop constructor is a good approach.

mmarchini · 2019-04-15T20:40:31Z

@ajor did you had a chance to work on this? I'm trying to fix #544 and I might have to the parser for that.

ajor · 2019-04-16T23:20:47Z

Not yet, but it shouldn't take me long to finish off. Are you saying there'll be conflicts? I don't mind fixing them up in this pull request if you're blocked.

mmarchini · 2019-04-17T00:11:07Z

Not yet, but it shouldn't take me long to finish off. Are you saying there'll be conflicts? I don't mind fixing them up in this pull request if you're blocked.

I was mistaken, this wasn't needed to fix #544. Also, at a glance it doesn't seem like this PR will conflict with recent changes.

mmarchini · 2019-04-17T00:12:22Z

I want to fix the two shift/reduce conflicts we have on our parser, but I'll wait until this land since it might involve rearranging some grammar rules.

mmarchini · 2019-05-24T00:59:05Z

@ajor were you able to work on this lately? Would be nice to include this in the next version (0.9.1).

ajor · 2019-05-24T19:04:50Z

Finished off my other two pull requests last weekend, so will get back to this one next

ajor · 2019-05-26T21:43:26Z

I've implemented it fully now. The ntop codegen tests had to be changed because they used ++ on a map without first initialising it. I think it's ok that this isn't allowed now, because .count() is what should be used there to get more accurate results (uses PER_CPU maps) and we shouldn't encourage bad practises like this by making them easier to type than the preferred way. If people really want to use ++ on a map to count things then they just have to initialise it to 0 first.

ajor · 2019-06-02T21:46:44Z

Read through #448 and now I'm thinking we should maybe keep ++ initialising to 0 for consistency. Would have to make it clear in the documentation that count() is better though

mmarchini · 2019-06-04T00:15:09Z

Or we can change #448 to not allow compound assignment of uninitialized variables and maps.

mmarchini · 2019-06-05T18:59:12Z

tl;dr: changing how compound assignments work for uninitialized variables doesn't make sense, so we should change this to make it coherent with compound assignments.

diff --git a/src/ast/semantic_analyser.cpp b/src/ast/semantic_analyser.cpp
index aa58df0..ddbfe40 100644
--- a/src/ast/semantic_analyser.cpp
+++ b/src/ast/semantic_analyser.cpp
@@ -855,7 +855,6 @@ void SemanticAnalyser::visit(ExprStatement &expr)
 
 void SemanticAnalyser::visit(AssignMapStatement &assignment)
 {
-  assignment.map->accept(*this);
   assignment.expr->accept(*this);
 
   std::string map_ident = assignment.map->ident;
@@ -877,15 +876,19 @@ void SemanticAnalyser::visit(AssignMapStatement &assignment)
     }
   }
   else {
-    // This map hasn't been seen before
-    map_val_.insert({map_ident, assignment.expr->type});
-    if (map_val_[map_ident].type == Type::integer) {
-      // Store all integer values as 64-bit in maps, so that there will
-      // be space for any integer to be assigned to the map later
-      map_val_[map_ident].size = 8;
+    if (is_final_pass()) {
+      // This map hasn't been seen before
+      map_val_.insert({map_ident, assignment.expr->type});
+      if (map_val_[map_ident].type == Type::integer) {
+        // Store all integer values as 64-bit in maps, so that there will
+        // be space for any integer to be assigned to the map later
+        map_val_[map_ident].size = 8;
+      }
     }
   }
 
+  assignment.map->accept(*this);
+
   if (assignment.expr->type.type == Type::cast) {
     std::string cast_type = assignment.expr->type.cast_type;
     std::string curr_cast_type = map_val_[map_ident].cast_type;

This seems enough to force initialization of maps on compound assignment. But it changes the behavior of maps. For example, the program below is valid today:

# ./src/bpftrace -e 'kprobe:f { @x = @y; @y = 2; }'
Attaching 1 probe...

But it would fail with the patch above:

# ./src/bpftrace -e 'kprobe:f { @x = @y; @y = 2; }'
Undefined map: @y

At the same time, the program below kinda makes sense, but would be invalid with the patch above:

# ./src/bpftrace -e 'kprobe:f { @x = @y; } kprobe:do_nanosleep { @y = 2; }'
Undefined map: @y

But if we change the probe order it works:

# ./src/bpftrace -e 'kprobe:do_nanosleep { @y = 2; } kprobe:f { @x = @y; }'
Undefined map: @y

Which doesn't make sense since we can't guarantee the execution order of two different probes. Either both programs should fail or be valid. If both programs fail, we won't be able to use maps across different probes, so we should keep the behavior as it is today.

As for compound assignments, @a += 1 should behave the same as @a = @a + 1. Having a specific check to see if @a is defined somewhere else would be too cumbersome, so we should probably keep things as is here as well.

As for increment, users would expect @a++ to behave the same way as @a += 1, so we should change it to accept uninitialized maps as well.

Variables are a different case, and no changes should be needed in either PR.

I also think we might want to better explain maps behavior in our reference guide. Today we only have examples there, which makes it look like both maps and variables behave the same, with the only difference being their scope (https://github.com/iovisor/bpftrace/blob/master/docs/reference_guide.md#2---basic-variables).

mmarchini · 2019-06-05T20:03:34Z

An alternative implementation would be to turn assignments into expressions, and allow for pre/post assignments. Then we could implement using Assing*Expression insead of unop, and would guarantee the same behavior between normal assignments, compound assignments and increment/decrement. This would also allow for C-style syntax like below:

if (($a = [expression]) == [other expression]) {
   ...
}

Not saying that the syntax above is a good idea or that this would be a better implementation, just sharing an alternative that would comply with compound assignment behavior.

ajor · 2019-06-11T21:50:37Z

Assignments as expressions might be something we should do, but I'll leave it out of this pull request and just get this one working some other way.

EDIT: Changed my mind after thinking about what I'd have to do to implement this :P
I'll look into assignment expressions

ajor · 2019-06-12T22:07:34Z

Done - please review again!

I'd misunderstood what you meant about assignment expressions when I commented yesterday - I've just done it without them now

mmarchini

LGTM. Suggested changes on tests are not critical and we can add it later.

mmarchini · 2019-06-12T22:24:06Z

tests/codegen/map_increment_decrement.cpp

+
+TEST(codegen, map_increment_decrement)
+{
+  test("BEGIN { @x = 10; @x++; ++@x; @x--; --@x; }",


Could also test undefined map here:

Suggested change

test("BEGIN { @x = 10; @x++; ++@x; @x--; --@x; }",

test("BEGIN { @x = 10; @x++; ++@x; @x--; --@x; @y++ }",

mmarchini · 2019-06-12T22:25:56Z

tests/runtime/basic

@@ -92,3 +92,13 @@ NAME defines work
 RUN bpftrace -e "$(echo '#define _UNDERSCORE 314'; echo 'BEGIN { printf("%d\\n", _UNDERSCORE); exit(); }')"
 EXPECT 314
 TIMEOUT 1
+
+NAME increment/decrement map
+RUN bpftrace -e 'BEGIN { @x = 10; printf("%d", @x++); printf(" %d", ++@x); printf(" %d", @x--); printf(" %d\n", --@x); delete(@x); exit(); }'


Same here:

Suggested change

RUN bpftrace -e 'BEGIN { @x = 10; printf("%d", @x++); printf(" %d", ++@x); printf(" %d", @x--); printf(" %d\n", --@x); delete(@x); exit(); }'

RUN bpftrace -e 'BEGIN { @x = 10; printf("%d", @x++); printf(" %d", ++@x); printf(" %d", @x--); printf(" %d\n", --@x); printf(" %d", @y++); delete(@x); delete(@y); exit(); }'

ajor · 2019-06-12T22:33:33Z

Unrelated tests are failing probably due to me doing a dodgy rebase - I'll fix them too before merging

mmarchini · 2019-06-17T16:34:25Z

This should fix the last failing test:

diff --git a/tests/codegen/unroll.cpp b/tests/codegen/unroll.cpp
index 2fcfa87..6b8b27d 100644
--- a/tests/codegen/unroll.cpp
+++ b/tests/codegen/unroll.cpp
@@ -6,7 +6,7 @@ namespace codegen {
 
 TEST(codegen, unroll)
 {
-  test("BEGIN { @i = 0; unroll(5) { @i++ } }",
+  test("BEGIN { @i = 0; unroll(5) { @i += 1 } }",
 
 R"EXPECTED(; Function Attrs: nounwind
 declare i64 @llvm.bpf.pseudo(i64, i64) #0

ajor · 2019-06-17T18:44:48Z

Ahh that makes more sense! Thanks I'll get that updated

ajor force-pushed the prepost-incdec-expr branch from d0a7c2c to 82f5419 Compare March 19, 2019 23:54

dalehamel reviewed Mar 23, 2019

View reviewed changes

mmarchini modified the milestone: 0.9.1 May 24, 2019

ajor force-pushed the prepost-incdec-expr branch from 82f5419 to 1d2ad53 Compare May 26, 2019 21:34

ajor marked this pull request as ready for review May 26, 2019 21:44

ajor force-pushed the prepost-incdec-expr branch from 1d2ad53 to 47bf014 Compare May 27, 2019 01:04

mmarchini approved these changes May 31, 2019

View reviewed changes

This was referenced Jun 5, 2019

Help reviewing remaining PRs for 0.9.1 #725

Closed

anonymous map incorrectly gets special treatment #640

Closed

Improve documentation on maps wrt declaration #731

Open

ajor added the do-not-merge Changes are not ready to be merged into master yet label Jun 11, 2019

ajor force-pushed the prepost-incdec-expr branch from 47bf014 to ee77b3f Compare June 12, 2019 22:05

ajor force-pushed the prepost-incdec-expr branch from ee77b3f to 733d59d Compare June 12, 2019 22:09

ajor removed the do-not-merge Changes are not ready to be merged into master yet label Jun 12, 2019

ajor force-pushed the prepost-incdec-expr branch from 733d59d to 89981fb Compare June 12, 2019 22:24

mmarchini approved these changes Jun 12, 2019

View reviewed changes

ajor force-pushed the prepost-incdec-expr branch from 89981fb to 9b11afc Compare June 23, 2019 22:03

ajor added 2 commits June 23, 2019 23:16

Fully implement ++ and --

8d2bbc5

Make ++ and -- work on previously undefined maps

9b11afc

ajor merged commit ba3c1b0 into master Jun 23, 2019

ajor deleted the prepost-incdec-expr branch June 23, 2019 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully implement ++ and -- #486

Fully implement ++ and -- #486

ajor commented Mar 19, 2019

ajor commented Mar 19, 2019

ajor commented Mar 22, 2019

brendangregg commented Mar 23, 2019

dalehamel Mar 23, 2019

dalehamel Mar 23, 2019

dalehamel Mar 23, 2019

dalehamel commented Mar 23, 2019

mmarchini commented Apr 15, 2019

ajor commented Apr 16, 2019

mmarchini commented Apr 17, 2019

mmarchini commented Apr 17, 2019

mmarchini commented May 24, 2019

ajor commented May 24, 2019

ajor commented May 26, 2019 •

edited

Loading

ajor commented Jun 2, 2019

mmarchini commented Jun 4, 2019

mmarchini commented Jun 5, 2019

mmarchini commented Jun 5, 2019

ajor commented Jun 11, 2019 •

edited

Loading

ajor commented Jun 12, 2019

mmarchini left a comment

mmarchini Jun 12, 2019

mmarchini Jun 12, 2019

ajor commented Jun 12, 2019

mmarchini commented Jun 17, 2019

ajor commented Jun 17, 2019

	test("BEGIN { @x = 10; @x++; ++@x; @x--; --@x; }",
	test("BEGIN { @x = 10; @x++; ++@x; @x--; --@x; @y++ }",

	RUN bpftrace -e 'BEGIN { @x = 10; printf("%d", @x++); printf(" %d", ++@x); printf(" %d", @x--); printf(" %d\n", --@x); delete(@x); exit(); }'
	RUN bpftrace -e 'BEGIN { @x = 10; printf("%d", @x++); printf(" %d", ++@x); printf(" %d", @x--); printf(" %d\n", --@x); printf(" %d", @y++); delete(@x); delete(@y); exit(); }'

Fully implement ++ and -- #486

Fully implement ++ and -- #486

Conversation

ajor commented Mar 19, 2019

ajor commented Mar 19, 2019

ajor commented Mar 22, 2019

brendangregg commented Mar 23, 2019

dalehamel Mar 23, 2019

Choose a reason for hiding this comment

dalehamel Mar 23, 2019

Choose a reason for hiding this comment

dalehamel Mar 23, 2019

Choose a reason for hiding this comment

dalehamel commented Mar 23, 2019

mmarchini commented Apr 15, 2019

ajor commented Apr 16, 2019

mmarchini commented Apr 17, 2019

mmarchini commented Apr 17, 2019

mmarchini commented May 24, 2019

ajor commented May 24, 2019

ajor commented May 26, 2019 • edited Loading

ajor commented Jun 2, 2019

mmarchini commented Jun 4, 2019

mmarchini commented Jun 5, 2019

mmarchini commented Jun 5, 2019

ajor commented Jun 11, 2019 • edited Loading

ajor commented Jun 12, 2019

mmarchini left a comment

Choose a reason for hiding this comment

mmarchini Jun 12, 2019

Choose a reason for hiding this comment

mmarchini Jun 12, 2019

Choose a reason for hiding this comment

ajor commented Jun 12, 2019

mmarchini commented Jun 17, 2019

ajor commented Jun 17, 2019

ajor commented May 26, 2019 •

edited

Loading

ajor commented Jun 11, 2019 •

edited

Loading