Implement while

[fbs]

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++ } }

[brendangregg]

Impressive! Amazing to see the backwards gotos.

[ajor]

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]

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]

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]

@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]

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]

* 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]

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

[fbs]

@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.

[olsajiri]

@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]

Rebased onto master

[danobi]

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

[fbs]

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