promote scalar printf args to 64-bits

Due to BPF stack alignment restrictions, LLVM might change the size used to stored printf arguments in the stack, resulting in alignment issues when formatting and outputting those arguments. To avoid this issue, we promote every non-array argument to 64-bits, assuring the size used to store the argument is the same used to dereference it later when calling printf. Fixes: bpftrace#47
sumanthkorikkar · Sep 27, 2018 · df51b11 · df51b11
1 parent 75c41dd
commit df51b11
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 10 deletions.
diff --git a/src/ast/codegen_llvm.cpp b/src/ast/codegen_llvm.cpp
@@ -428,7 +428,7 @@ void CodegenLLVM::visit(Call &call)
       Expression &arg = *call.vargs->at(i);
       arg.accept(*this);
       Value *offset = b_.CreateGEP(printf_args, {b_.getInt32(0), b_.getInt32(i)});
-      if (arg.type.type == Type::string || arg.type.type == Type::usym)
+      if (arg.type.IsArray())
         b_.CreateMemCpy(offset, expr_, arg.type.size, 1);
       else
         b_.CreateStore(expr_, offset);
@@ -477,7 +477,7 @@ void CodegenLLVM::visit(Call &call)
       Expression &arg = *call.vargs->at(i);
       arg.accept(*this);
       Value *offset = b_.CreateGEP(system_args, {b_.getInt32(0), b_.getInt32(i)});
-      if (arg.type.type == Type::string || arg.type.type == Type::usym)
+      if (arg.type.IsArray())
         b_.CreateMemCpy(offset, expr_, arg.type.size, 1);
       else
         b_.CreateStore(expr_, offset);

diff --git a/src/ast/irbuilderbpf.cpp b/src/ast/irbuilderbpf.cpp
@@ -73,7 +73,7 @@ AllocaInst *IRBuilderBPF::CreateAllocaBPF(int bytes, const std::string &name)
 llvm::Type *IRBuilderBPF::GetType(const SizedType &stype)
 {
   llvm::Type *ty;
-  if (stype.type == Type::string || stype.type == Type::usym || (stype.type == Type::cast && !stype.is_pointer))
+  if (stype.IsArray())
   {
     ty = ArrayType::get(getInt8Ty(), stype.size);
   }

diff --git a/src/ast/semantic_analyser.cpp b/src/ast/semantic_analyser.cpp
@@ -262,7 +262,11 @@ void SemanticAnalyser::visit(Call &call)
         String &fmt = static_cast<String&>(fmt_arg);
         std::vector<Field> args;
         for (auto iter = call.vargs->begin()+1; iter != call.vargs->end(); iter++) {
-          args.push_back({ .type = (*iter)->type });
+          auto ty = (*iter)->type;
+          // Promote to 64-bit if it's not an array type
+          if (!ty.IsArray())
+            ty.size = 8;
+          args.push_back({ .type =  ty });
         }
         err_ << verify_format_string(fmt.str, args);
 

diff --git a/src/types.cpp b/src/types.cpp
@@ -23,6 +23,11 @@ bool SizedType::operator==(const SizedType &t) const
   return type == t.type && size == t.size;
 }
 
+bool SizedType::IsArray() const
+{
+  return type == Type::string || type == Type::usym || (type == Type::cast && !is_pointer);
+}
+
 std::string typestr(Type t)
 {
   switch (t)

diff --git a/src/types.h b/src/types.h
@@ -50,6 +50,8 @@ class SizedType
   bool is_pointer = false;
   size_t pointee_size;
 
+  bool IsArray() const;
+
   bool operator==(const SizedType &t) const;
 };
 

diff --git a/tests/codegen.cpp b/tests/codegen.cpp
@@ -42,6 +42,7 @@ TEST(codegen, printf_offsets)
   BPFtrace bpftrace;
   Driver driver;
 
+  // TODO (mmarchini): also test printf with a string argument
   ASSERT_EQ(driver.parse_str("struct Foo { char c; int i; } kprobe:f { $foo = (Foo*)0; printf(\"%c %u\\n\", $foo->c, $foo->i) }"), 0);
   ClangParser clang;
   clang.parse(driver.root_, bpftrace.structs_);
@@ -60,13 +61,15 @@ TEST(codegen, printf_offsets)
 
   EXPECT_EQ(args.size(), 2);
 
+  // NOTE (mmarchini) type.size is the original arg size, and it might be
+  // different from the actual size we use to store in memory
   EXPECT_EQ(args[0].type.type, Type::integer);
-  EXPECT_EQ(args[0].type.size, 1);
+  EXPECT_EQ(args[0].type.size, 8);
   EXPECT_EQ(args[0].offset, 8);
 
   EXPECT_EQ(args[1].type.type, Type::integer);
-  EXPECT_EQ(args[1].type.size, 4);
-  EXPECT_EQ(args[1].offset, 12);
+  EXPECT_EQ(args[1].type.size, 8);
+  EXPECT_EQ(args[1].offset, 16);
 }
 
 std::string header = R"HEAD(; ModuleID = 'bpftrace'
@@ -1694,7 +1697,7 @@ TEST(codegen, call_printf)
 {
   test("struct Foo { char c; long l; } kprobe:f { $foo = (Foo*)0; printf(\"%c %lu\\n\", $foo->c, $foo->l) }",
 
-R"EXPECTED(%printf_t = type { i64, i8, i64 }
+R"EXPECTED(%printf_t = type { i64, i64, i64 }
 
 ; Function Attrs: nounwind
 declare i64 @llvm.bpf.pseudo(i64, i64) #0
@@ -1716,7 +1719,7 @@ define i64 @"kprobe:f"(i8*) local_unnamed_addr section "s_kprobe:f_1" {
   %3 = load i8, i8* %Foo.c, align 1
   call void @llvm.lifetime.end.p0i8(i64 -1, i8* nonnull %Foo.c)
   %4 = getelementptr inbounds %printf_t, %printf_t* %printf_args, i64 0, i32 1
-  store i8 %3, i8* %4, align 8
+  store i8 %3, i64* %4, align 8
   %5 = bitcast i64* %Foo.l to i8*
   call void @llvm.lifetime.start.p0i8(i64 -1, i8* nonnull %5)
   %probe_read1 = call i64 inttoptr (i64 4 to i64 (i8*, i64, i8*)*)(i64* nonnull %Foo.l, i64 8, i64 8)
@@ -1726,7 +1729,7 @@ define i64 @"kprobe:f"(i8*) local_unnamed_addr section "s_kprobe:f_1" {
   store i64 %6, i64* %7, align 8
   %pseudo = call i64 @llvm.bpf.pseudo(i64 1, i64 1)
   %get_cpu_id = call i64 inttoptr (i64 8 to i64 ()*)()
-  %perf_event_output = call i64 inttoptr (i64 25 to i64 (i8*, i8*, i64, i8*, i64)*)(i8* %0, i64 %pseudo, i64 %get_cpu_id, %printf_t* nonnull %printf_args, i64 20)
+  %perf_event_output = call i64 inttoptr (i64 25 to i64 (i8*, i8*, i64, i8*, i64)*)(i8* %0, i64 %pseudo, i64 %get_cpu_id, %printf_t* nonnull %printf_args, i64 24)
   call void @llvm.lifetime.end.p0i8(i64 -1, i8* nonnull %1)
   ret i64 0
 }