Skip to content

Commit

Permalink
Fix codegen for modulo operation
Browse files Browse the repository at this point in the history
There was a regression with the modulo operation. It turns out the srem
instruction does a signed division under the hood. Somewhat obvious
after the fact but easy to miss.

Example of previously broken script:

    BEGIN
    {
      @start = 0;
    }

    interval:ms:1
    {
      if ((@start % 200) == 0) {
        ++@start;
      }
    }

Compile error:

    Error: Unsupport signed division for DAG: 0x11d46d8: i64 = sdiv
    0x11d4d58, Constant:i64<200>Please convert to unsigned div/mod.
    ...
  • Loading branch information
danobi committed Dec 20, 2019
1 parent e87897c commit fe0ed5a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/ast/codegen_llvm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,11 @@ void CodegenLLVM::visit(Binop &binop)
case bpftrace::Parser::token::MUL: expr_ = b_.CreateMul (lhs, rhs); break;
case bpftrace::Parser::token::DIV: expr_ = b_.CreateUDiv (lhs, rhs); break;
case bpftrace::Parser::token::MOD: {
expr_ = do_signed ? b_.CreateSRem(lhs, rhs) : b_.CreateURem(lhs, rhs);
// Always do an unsigned modulo operation here even if `do_signed`
// is true. bpf instruction set does not support signed divison.
// We already warn in the semantic analyser that signed modulo can
// lead to undefined behavior (because we will treat it as unsigned).
expr_ = b_.CreateURem(lhs, rhs);
break;
}
case bpftrace::Parser::token::BAND: expr_ = b_.CreateAnd (lhs, rhs); break;
Expand Down
3 changes: 2 additions & 1 deletion src/ast/semantic_analyser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,8 @@ void SemanticAnalyser::visit(Binop &binop)
//
// SDIV is not implemented for bpf. See Documentation/bpf/bpf_design_QA
// in kernel sources
if (binop.op == bpftrace::Parser::token::DIV) {
if (binop.op == bpftrace::Parser::token::DIV ||
binop.op == bpftrace::Parser::token::MOD) {
// Convert operands to unsigned if possible
if (lsign && left->is_literal && get_int_literal(left) >= 0)
left->type.is_signed = lsign = false;
Expand Down

0 comments on commit fe0ed5a

Please sign in to comment.