Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement while #1066

Merged
merged 5 commits into from Apr 21, 2020
Merged

Implement while #1066

merged 5 commits into from Apr 21, 2020

Conversation

fbs
Copy link
Contributor

@fbs fbs commented Jan 6, 2020

With the loop supported added in the 5.0 kernel we're now able to
implement C-style while and for loops. Allowing us to write code like:

i:s:1 {
        $i = 0;
        while ($i < 100) {
                if ( ($i/10) * 10 == $i ) {
                        printf("\n");
                }
                if ( ($i/6)*6 == $i ){
                        printf("XX ");
                        $i+=1;
                        continue;
                }
                printf("%2d ", $i);
                $i+=1;
                if ($i >= 50) {
                        break;
                }
        }
}

To generate:

Attaching 1 probe...

XX  1  2  3  4  5 XX  7  8  9
10 11 XX 13 14 15 16 17 XX 19
20 21 22 23 XX 25 26 27 28 29
XX 31 32 33 34 35 XX 37 38 39
40 41 XX 43 44 45 46 47 XX 49

Using only a fewinstructions:

203: perf_event  name 1  tag 4881cacfe5abad41  gpl
        loaded_at 2020-03-26T18:17:17+0000  uid 0
        xlated 448B  jited 340B  memlock 4096B  map_ids 85

Note that LLVM tries to unroll quite aggressively causing a loop the
following be fully unrolled:

i:s:1 { $i = 0; while ($i < 30) { @=$i; $i++ } }

@fbs fbs changed the title 872 loops Implement while Jan 6, 2020
@brendangregg
Copy link
Contributor

Impressive! Amazing to see the backwards gotos.

Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of handling the continues/breaks in the various other control flow statements, would we be able to keep track of which loop we're in (with stacks of their BasicBlocks) and then implement them by emitting branches in void CodegenLLVM::visit(Jump &jump)? It might help to isolate the complexity.

Just having bpftrace emit the IR without lots of other junk would be useful, but I think it'd be better to have it as an option under the -d flag (#131) for flexibility and to avoid growing the list of arguments. Then we can run bpftrace -d ir input.bt > filename to get it in a file if that's desired.

@ajor
Copy link
Member

ajor commented Jan 7, 2020

Oh I missed that you already were keeping a stack of BasicBlocks, still, the idea about emitting the branches in the Jump visitor instead might help. There's no harm in emitting more instructions at this stage than are actually needed - the optimiser will get rid of any that can't be reached later on - keeping things simple is more important.

This might also help with implementing break and continue for unroll loops too (it looks like breaks or a continue inside an if won't work right at the moment, or maybe I'm just tired...)

What's the point of reserving the return keyword?

@fbs
Copy link
Contributor Author

fbs commented Jan 7, 2020

Oh I missed that you already were keeping a stack of BasicBlocks, still, the idea about emitting the branches in the Jump visitor instead might help.

Now that I have a working base it should be easier to shuffle things around and improve the logic a bit. Moving the jumps will indeed help a bit.

There's no harm in emitting more instructions at this stage than are actually needed - the optimiser will get rid of any that can't be reached later on - keeping things simple is more important.

I've bumped into quite a few segfaults and hangs that seem to have ben caused empty labels and multiple sequential unconditional branches. E.g:
with this patch:

+  else_stmt = BasicBlock::Create(module_->getContext(), "else_stmt", parent);
   if_block.cond->accept(*this);
   Value *cond = expr_;

   if (if_block.else_stmts)
   {
-    else_stmt = BasicBlock::Create(module_->getContext(), "else_stmt", parent);
     b_.CreateCondBr(b_.CreateICmpNE(cond, b_.getInt64(0), "true_cond"), if_true, else_stmt);
   }

It crashes:

[ RUN      ] codegen.if_nested_printf
Segmentation fault (core dumped)

I'm not sure why it happens.

The current if still has some bugs, I will refactor it and try to improve readability.

What's the point of reserving the return keyword?

Not much, I initially wanted to implement is but might leave it for another PR.

but I think it'd be better to have it as an option under the -d flag (#131) for flexibility and to avoid growing the list of arguments.

Just having bpftrace emit the IR without lots of other junk would be useful, but I think it'd be better to have it as an option under the -d flag (#131) for flexibility and to avoid growing the list of arguments. Then we can run bpftrace -d ir input.bt > filename to get it in a file if that's desired.

I'll give that a go :)

@fbs
Copy link
Contributor Author

fbs commented Jan 7, 2020

@ajor I've pushed a new version which I think improved the readability of if by quite a bit, what do you think?

I've also added a few TODOs for warnings, I'm not sure how to handle those, (@brendangregg )

  • Should a break in a while be a warning? E.g. while(x) { break }. Might be good to warn the user as it is probably a typo/bug. Clang doesn't seem to warn about it though.
  • Should statements after a break trigger a warning? E.g. while(x) { if (y) { break; @=nsecs }}
  • Should we allow jumps and loops in an unroll?

@ajor
Copy link
Member

ajor commented Jan 7, 2020

I've bumped into quite a few segfaults and hangs that seem to have ben caused empty labels and multiple sequential unconditional branches

Hmm I guess LLVM is expecting well formed basic blocks, it's a bit crap that it just segfaults though. If we do need valid basic blocks though, would creating a new one at the end of visit(Jump) fix the segfaults and mean we don't need the dynamic_casts any more?

e.g.

void CodegenLLVM::visit(Jump &jump)
{
  if (jump.ident == "continue") {
    b_.CreateBr(std::get<0>(loops_.back()));
  }
  else if (jump.ident == "break") {
    b_.CreateBr(std::get<1>(loops_.back()));
  }
  else {
    throw new std::runtime_error("Unknown jump: " + jump.ident);
  }

  Function *parent = b_.GetInsertBlock()->getParent();
  BasicBlock *blah = BasicBlock::Create(module_->getContext(), "blah", parent);
  b_.SetInsertPoint(blah);
}

@ajor
Copy link
Member

ajor commented Jan 7, 2020

* Should we allow jumps and loops in an unroll?

Might as well allow jumps since they'll be forward only in an unrolled loop. I'm not sure why anyone would want a regular loop inside an unrolled one but don't seem the harm in allowing it through (I'm assuming it would just work unless we explicitly block it).

@fbs fbs force-pushed the 872_loops branch 3 times, most recently from ca2342b to 414aa7d Compare March 26, 2020 18:25
@fbs fbs marked this pull request as ready for review March 26, 2020 18:25
@fbs
Copy link
Contributor Author

fbs commented Mar 26, 2020

I need to add docs but other than that this is ready for review.

@fbs
Copy link
Contributor Author

fbs commented Apr 11, 2020

@olsajiri @danobi . I think this is pretty much done for a first version. All the basic parts seem to work. Testing itself is a bit tricky as it requires a new kernel but also because LLVM unrolls loops quite aggressively.

Some improvements we can make in a future version (ill create tickets at some point)

  • for loops
  • break/continue/return in unroll
  • warning if there is no variable/map in cond (e.g. while(arg){} is an endless loop)

I'll fix any codegen tests failures when everyone is happy with the codegen.

@fbs fbs added this to the 0.11.0 milestone Apr 11, 2020
@olsajiri
Copy link
Contributor

@olsajiri @danobi . I think this is pretty much done for a first version. All the basic parts seem to work. Testing itself is a bit tricky as it requires a new kernel but also because LLVM unrolls loops quite aggressively.

it's working nicely under fedora 31 with its 5.5 kernel

Some improvements we can make in a future version (ill create tickets at some point)

* for loops

* break/continue/return in unroll

* warning if there is no variable/map in cond (e.g. while(arg){} is an endless loop)

I guess kernel will stop such program from loading,
but right, we might want to do nice warning

it all looks good to me, let's have it in soon ;-)

@fbs
Copy link
Contributor Author

fbs commented Apr 14, 2020

Rebased onto master

Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some runtime tests too? You can gate it on loop support like I did here: https://github.com/iovisor/bpftrace/blob/master/tests/runtime/builtin#L193

src/parser.yy Outdated Show resolved Hide resolved
docs/reference_guide.md Outdated Show resolved Hide resolved
@fbs fbs force-pushed the 872_loops branch 2 times, most recently from ec3fde8 to e6b7a12 Compare April 20, 2020 16:54
fbs added 4 commits April 20, 2020 19:05
With loops the log buffer can get really big quickly, 10MB of log output
is not uncommon. Trying to allocate a 10M log buffer on the stack causes
issues so let's use the heap instead.
With the loop supported added in the 5.0 kernel we're now able to
implement C-style while and for loops. Allowing us to write code like:

```
i:s:1 {
	$i = 0;
	while ($i < 100) {
		if ( ($i/10) * 10 == $i ) {
			printf("\n");
		}
		if ( ($i/6)*6 == $i ){
			printf("XX ");
			$i+=1;
			continue;
		}
		printf("%2d ", $i);
		$i+=1;
		if ($i >= 50) {
			break;
		}
	}
}
```

To generate:

```
Attaching 1 probe...

XX  1  2  3  4  5 XX  7  8  9
10 11 XX 13 14 15 16 17 XX 19
20 21 22 23 XX 25 26 27 28 29
XX 31 32 33 34 35 XX 37 38 39
40 41 XX 43 44 45 46 47 XX 49
```

Using only a few instructions:

```
203: perf_event  name 1  tag 4881cacfe5abad41  gpl
	loaded_at 2020-03-26T18:17:17+0000  uid 0
	xlated 448B  jited 340B  memlock 4096B  map_ids 85
```

Note that LLVM tries to unroll quite aggressively causing a loop the
following be fully unrolled:

```
i:s:1 { $i = 0; while ($i < 30) { @=$i; $i++ } }
```
For some reason it seems to fail ~50% of the time in the CI. Let's
disable it for now.
@fbs
Copy link
Contributor Author

fbs commented Apr 20, 2020

Removed the flaky test and the CI seems to be happy now. I'll merge this tomorrow if there are no further comments :)

@fbs fbs merged commit 24c4282 into bpftrace:master Apr 21, 2020
@fbs fbs deleted the 872_loops branch March 6, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants