[tests] add missing tests to codegen.cpp #574

mmarchini · 2019-04-26T20:15:42Z

Three codegen tests were not included in codegen.cpp, thus they were not running. Ideally we should generate codegen.cpp using cmake, but for now this increases our coverage a little bit.

ajor · 2019-04-27T11:40:38Z

Good spot! Yeah, auto-generating that file would make sense.

Just got a couple of failing tests in here.

ajor · 2019-05-01T22:58:00Z

Just to be clear, this looks good as-is, as long as the failing tests get fixed

mmarchini · 2019-05-31T00:31:10Z

Made a few changes to mock the tracepoint parser. Haven't tested on other LLVM versions, only 6. If tests fail on other LLVM versions I'll update them tomorrow.

mmarchini · 2019-05-31T00:33:13Z

tests/codegen/common.h

@@ -32,7 +53,20 @@ static void test(
  ASSERT_EQ(driver.parse_str(input), 0);

  ClangParser clang;
-  clang.parse(driver.root_, bpftrace);
+  // TODO (mmarchini): don't use kernel flags on tests


Couldn't get the args_multiple_tracepoints tests to work without kernel headers. We can probably rewrite those tests to use tracepoints that don't rely on kernel types, but this can be done in the future, it's better to have those tests running first.

Ok, tests won't work on CI with this. I'll have to try some alternatives.

Is this stuff just to get the umode_t type in? Could just remove the mode entry from the tracepoint format definitions you've embedded in the test to save on a lot of extra complexity here - there's no need for them to match the real kernel tracepoint definitions

Even if we change umode_t, size_t and all struct pointers to basic types, we still get:

definitions.h:8:3: error: unknown type name 's64' definitions.h:10:3: error: unknown type name 'u64' definitions.h:11:3: error: unknown type name 'u64' definitions.h:22:3: error: unknown type name 's64' definitions.h:24:3: error: unknown type name 'u64' definitions.h:25:3: error: unknown type name 'u64' definitions.h:35:3: error: unknown type name 's64' definitions.h:37:3: error: unknown type name 'u64'

Because of https://github.com/iovisor/bpftrace/blob/master/src/tracepoint_format_parser.cpp#L166-L181. Suggestions?

mmarchini · 2019-05-31T19:50:33Z

@ajor does it still looks good to you? I made a few changes to fix tests and to use mocks where possible.

danobi

I don't love doing string comparisons on LLVM IR; I'd much rather invest heavily on runtime tests and CI infra. But that's a discussion we should have in person sometime.

Patch LGTM except for an unused template thing.

danobi · 2019-06-06T20:10:20Z

tests/codegen/common.h

 const std::string header = R"HEAD(; ModuleID = 'bpftrace'
 source_filename = "bpftrace"
 target datalayout = "e-m:e-p:64:64-i64:64-n32:64-S128"
 target triple = "bpf-pc-linux"

 )HEAD";

+template <class TFP=TracepointFormatParser>


mmarchini · 2019-06-06T20:28:51Z

I don't love doing string comparisons on LLVM IR

I didn't like it at first, but it has grown on me, especially since I was able to catch bugs in my changes because the new IR didn't quite make sense.

I'd much rather invest heavily on runtime tests and CI infra

I agree we should invest more on runtime tests and CI infra.

ajor

Sticking this on here to make sure my previous review doesn't get missed

ajor

Continuing discussion from the inline comment here

Since these are just unit tests they don't need to have particularly realistic inputs to exercise other parts of the codebase (like tracepoint format/clang parsers). The sample tracepoint format files used in these tests can be simplified to just something like:

"	field:int flags;	offset:24;	size:4;	signed:0;\n"

"	field:int flags;	offset:32;	size:4;	signed:0;\n"

So only the particular field needed by the codegen test is included. To get rid of the s64 issue, the integer field sizes can be changed from 8 to 4.

Also, could you split the tests in args_multiple_tracepoints.cpp into separate files? The codegen tests have been separated to a file each because the diffs were getting unmanageable with combined files.

mmarchini · 2019-06-12T19:09:06Z

With the following patch:

diff --git a/tests/codegen/args_multiple_tracepoints.cpp b/tests/codegen/args_multiple_tracepoints.cpp
index 1d6e031..1b91139 100644
--- a/tests/codegen/args_multiple_tracepoints.cpp
+++ b/tests/codegen/args_multiple_tracepoints.cpp
@@ -31,7 +31,7 @@ TEST(codegen, args_multiple_tracepoints)
     "	field:int __syscall_nr;	offset:8;	size:4;	signed:1;\n"
     "	field:const char * filename;	offset:16;	size:8;	signed:0;\n"
     "	field:int flags;	offset:24;	size:8;	signed:0;\n"
-    "	field:umode_t mode;	offset:32;	size:8;	signed:0;\n"
+    "	field:int mode;	offset:32;	size:8;	signed:0;\n"
     "\n"
     "print fmt: \"filename: 0x%08lx, flags: 0x%08lx, mode: 0x%08lx\", ((unsigned long)(REC->filename)), ((unsigned long)(REC->flags)), ((unsigned long)(REC->mode))\n";
   std::istringstream sys_enter_open_format_file(sys_enter_open_input);
@@ -50,7 +50,7 @@ TEST(codegen, args_multiple_tracepoints)
     "	field:int dfd;	offset:16;	size:8;	signed:0;\n"
     "	field:const char * filename;	offset:24;	size:8;	signed:0;\n"
     "	field:int flags;	offset:32;	size:8;	signed:0;\n"
-    "	field:umode_t mode;	offset:40;	size:8;	signed:0;\n"
+    "	field:int mode;	offset:40;	size:8;	signed:0;\n"
     "\n"
     "print fmt: \"dfd: 0x%08lx, filename: 0x%08lx, flags: 0x%08lx, mode: 0x%08lx\", ((unsigned long)(REC->dfd)), ((unsigned long)(REC->filename)), ((unsigned long)(REC->flags)), ((unsigned long)(REC->mode))\n";
   std::istringstream sys_enter_openat_format_file(sys_enter_openat_input);
@@ -292,9 +292,9 @@ TEST(codegen, args_multiple_tracepoints_wild)
     "	field:int __syscall_nr;	offset:8;	size:4;	signed:1;\n"
     "	field:int fd;	offset:16;	size:8;	signed:0;\n"
     "	field:void * ubuf;	offset:24;	size:8;	signed:0;\n"
-    "	field:size_t size;	offset:32;	size:8;	signed:0;\n"
+    "	field:unsigned int size;	offset:32;	size:8;	signed:0;\n"
     "	field:unsigned int flags;	offset:40;	size:8;	signed:0;\n"
-    "	field:struct sockaddr * addr;	offset:48;	size:8;	signed:0;\n"
+    "	field:void * addr;	offset:48;	size:8;	signed:0;\n"
     "	field:int * addr_len;	offset:56;	size:8;	signed:0;\n"
     "\n"
     "print fmt: \"fd: 0x%08lx, ubuf: 0x%08lx, size: 0x%08lx, flags: 0x%08lx, addr: 0x%08lx, addr_len: 0x%08lx\", ((unsigned long)(REC->fd)), ((unsigned long)(REC->ubuf)), ((unsigned long)(REC->size)), ((unsigned long)(REC->flags)), ((unsigned long)(REC->addr)), ((unsigned long)(REC->addr_len))\n";
@@ -312,10 +312,10 @@ TEST(codegen, args_multiple_tracepoints_wild)
     "\n"
     "	field:int __syscall_nr;	offset:8;	size:4;	signed:1;\n"
     "	field:int fd;	offset:16;	size:8;	signed:0;\n"
-    "	field:struct mmsghdr * mmsg;	offset:24;	size:8;	signed:0;\n"
+    "	field:void * mmsg;	offset:24;	size:8;	signed:0;\n"
     "	field:unsigned int vlen;	offset:32;	size:8;	signed:0;\n"
     "	field:unsigned int flags;	offset:40;	size:8;	signed:0;\n"
-    "	field:struct timespec * timeout;	offset:48;	size:8;	signed:0;\n"
+    "	field:void * timeout;	offset:48;	size:8;	signed:0;\n"
     "\n"
     "print fmt: \"fd: 0x%08lx, mmsg: 0x%08lx, vlen: 0x%08lx, flags: 0x%08lx, timeout: 0x%08lx\", ((unsigned long)(REC->fd)), ((unsigned long)(REC->mmsg)), ((unsigned long)(REC->vlen)), ((unsigned long)(REC->flags)), ((unsigned long)(REC->timeout))\n";
   std::istringstream sys_enter_recvmmsg_format_file(sys_enter_recvmmsg_input);
@@ -332,7 +332,7 @@ TEST(codegen, args_multiple_tracepoints_wild)
     "\n"
     "	field:int __syscall_nr;	offset:8;	size:4;	signed:1;\n"
     "	field:int fd;	offset:16;	size:8;	signed:0;\n"
-    "	field:struct user_msghdr * msg;	offset:24;	size:8;	signed:0;\n"
+    "	field:void * msg;	offset:24;	size:8;	signed:0;\n"
     "	field:unsigned int flags;	offset:32;	size:8;	signed:0;\n"
     "\n"
     "print fmt: \"fd: 0x%08lx, msg: 0x%08lx, flags: 0x%08lx\", ((unsigned long)(REC->fd)), ((unsigned long)(REC->msg)), ((unsigned long)(REC->flags))\n";
diff --git a/tests/codegen/common.h b/tests/codegen/common.h
index 53e855a..f92d88f 100644
--- a/tests/codegen/common.h
+++ b/tests/codegen/common.h
@@ -53,16 +53,7 @@ static void test(
   ASSERT_EQ(driver.parse_str(input), 0);
 
   ClangParser clang;
-  // TODO (mmarchini): don't use kernel flags on tests
-  std::vector<std::string> extra_flags;
-  {
-    extra_flags.push_back("-isystem");
-    extra_flags.push_back(FIXTURES_DIR);
-
-    extra_flags.push_back("-include");
-    extra_flags.push_back("types.h");
-  }
-  clang.parse(driver.root_, bpftrace, extra_flags);
+  clang.parse(driver.root_, bpftrace);
 
   ASSERT_EQ(driver.parse_str(input), 0);

We get the following errors:

# ./tests/bpftrace_test --gtest_filter="codegen.args*"
Note: Google Test filter = codegen.args*
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from codegen
[ RUN      ] codegen.args_multiple_tracepoints
definitions.h:9:3: error: unknown type name 's64'
definitions.h:10:3: error: unknown type name 's64'
definitions.h:19:3: error: unknown type name 's64'
definitions.h:21:3: error: unknown type name 's64'
definitions.h:22:3: error: unknown type name 's64'
Struct/union of type '_tracepoint_syscalls_sys_enter_open' does not contain a field named 'filename'
Struct/union of type '_tracepoint_syscalls_sys_enter_openat' does not contain a field named 'filename'
str() only supports integer arguments (none provided)
/home/mmarchini/workspace/iovisor/bpftrace/tests/codegen/common.h:61: Failure
Expected equality of these values:
  semantics.analyse()
    Which is: 10
  0
[  FAILED  ] codegen.args_multiple_tracepoints (56 ms)
[ RUN      ] codegen.args_multiple_tracepoints_wild
definitions.h:8:3: error: unknown type name 's64'
definitions.h:10:3: error: unknown type name 'u64'
definitions.h:11:3: error: unknown type name 'u64'
definitions.h:22:3: error: unknown type name 's64'
definitions.h:24:3: error: unknown type name 'u64'
definitions.h:25:3: error: unknown type name 'u64'
definitions.h:35:3: error: unknown type name 's64'
definitions.h:37:3: error: unknown type name 'u64'
Struct/union of type '_tracepoint_syscalls_sys_enter_recvfrom' does not contain a field named 'flags'
Struct/union of type '_tracepoint_syscalls_sys_enter_recvmmsg' does not contain a field named 'flags'
Struct/union of type '_tracepoint_syscalls_sys_enter_recvmsg' does not contain a field named 'flags'
/home/mmarchini/workspace/iovisor/bpftrace/tests/codegen/common.h:61: Failure
Expected equality of these values:
  semantics.analyse()
    Which is: 10
  0
[  FAILED  ] codegen.args_multiple_tracepoints_wild (17 ms)
[----------] 2 tests from codegen (73 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (73 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] codegen.args_multiple_tracepoints
[  FAILED  ] codegen.args_multiple_tracepoints_wild

 2 FAILED TESTS

Because we turn all integers into Kernel types in our TracepointFormatParser (here and here, expanded below)

std::string TracepointFormatParser::adjust_integer_types(const std::string &field_type, int size)
{
  std::string new_type = field_type;
  // Adjust integer fields to correctly sized types
  if (size == 8)
  {
    if (field_type == "int")
      new_type = "s64";
    if (field_type == "unsigned int" || field_type == "unsigned" ||
        field_type == "u32" || field_type == "pid_t" ||
        field_type == "uid_t" || field_type == "gid_t")
      new_type = "u64";
  }

  return new_type;
}

And since our TracepointFormatParser is implemented as a set of static methods, we can't override them in the MockTracepointFormatParser class.

My suggestion is to keep the PR with the extra flags for now to restore these tests (which is the goal of this PR), and follow up with the required changes in TracepointFormatParser to make it more test-friendly.

I can break the test file into two, but changing TracepointFormatParser from static methods to instance methods is out of scope of this PR.

ajor · 2019-06-12T20:43:59Z

What I meant was replace the tracepoint format files with literally that single line. Only 8-byte integers are converted to s64/u64, so swapping their definitions out for 4-byte versions will solve that issue.

mmarchini · 2019-06-12T20:45:05Z

Oh, ok, got it. I'll try it later.

ajor · 2019-06-12T20:53:05Z

Might need a few fields before the ones we care about for padding actually, but the important thing is 4-byte ints

mmarchini · 2019-06-12T22:14:22Z

Ok, went with a slight different approach, but based on your suggestion: replaced all size 8 int fields with long, this way we still get the same codegen we would get by running these programs in the command line, and also we avoid using padding fields.

ajor · 2019-06-12T22:26:11Z

tests/codegen/builtin_func_wild.cpp

@@ -5,6 +5,7 @@ namespace test {
 namespace codegen {

 using ::testing::Return;
+using ::testing::_;


This isn't needed

Ok, I messed something up in my commits which re-added these. I'll remove and check if I missed anything else, I'll let you know when it's ready for review again.

ajor · 2019-06-12T22:27:01Z

tests/fixtures/types.h

@@ -0,0 +1,6 @@
+typedef long long int s64;


This file isn't used any more

ajor · 2019-06-12T22:38:16Z

tests/codegen/builtin_probe_wild.cpp

@@ -5,6 +5,7 @@ namespace test {
 namespace codegen {

 using ::testing::Return;
+using ::testing::_;


ajor · 2019-06-12T22:38:49Z

tests/codegen/general.cpp

 #include "gtest/gtest.h"
+#include "gmock/gmock.h"


They were in alphabetical order before

tests/codegen.cpp

@@ -1,7 +1,8 @@
 // https://github.com/iovisor/bpftrace/issues/229
 // this file exists solely to gather all the codegen tests into one compilation unit, for build performance

-#include "codegen/general.cpp"
+#include "codegen/args_multiple_tracepoints.cpp"
+#include "codegen/args_multiple_tracepoints_wild.cpp"


mmarchini · 2019-06-13T02:25:13Z

.travis.yml

    - name: "Static LLVM 5 Release"
-      env: LLVM_VERSION=5.0 BASE=alpine TYPE=Release STATIC_LINKING=ON TEST_ARGS="--gtest_filter=codegen.*:-codegen.call_ntop_char4:codegen.call_ntop_char16:codegen.call_printf:codegen.enum_declaration:codegen.macro_definition:codegen.printf_offsets:codegen.struct_*"
+      env: LLVM_VERSION=5.0 BASE=alpine TYPE=Release STATIC_LINKING=ON TEST_ARGS="--gtest_filter=codegen.*:-codegen.call_ntop_char4:codegen.call_ntop_char16:codegen.call_printf:codegen.enum_declaration:codegen.macro_definition:codegen.printf_offsets:codegen.struct_*:codegen.args_multiple_tracepoints_wild"


For some reason this test (codegen.args_multiple_tracepoints_wild) is working on my machine (with the alpine image, running just like Travis does) and it's not working on Travis. I just downloaded the apline image and built the builder image, so it shouldn't be a cache problem (afaik Travis don't keep cache of docker images).

I think I have a lead on the alpine test issues, but I'll pursue that in a separate PR.

mmarchini · 2019-06-14T18:19:19Z

Ok, fixed the commit mess I made. @ajor could you review it again?

ajor

Looks good! Just a few extra lines able to be removed now

tests/CMakeLists.txt

mmarchini force-pushed the add-missing-codegen-tests branch 2 times, most recently from 0391f02 to c965da7 Compare May 31, 2019 00:29

mmarchini commented May 31, 2019

View reviewed changes

mmarchini force-pushed the add-missing-codegen-tests branch 2 times, most recently from ebe47e0 to 552bf28 Compare May 31, 2019 19:49

mmarchini mentioned this pull request Jun 5, 2019

Help reviewing remaining PRs for 0.9.1 #725

Closed

12 tasks

danobi approved these changes Jun 6, 2019

View reviewed changes

ajor requested changes Jun 9, 2019

View reviewed changes

ajor reviewed Jun 11, 2019

View reviewed changes

mmarchini mentioned this pull request Jun 12, 2019

Make TracepointFormatParser more test-friendly #758

Open

mmarchini force-pushed the add-missing-codegen-tests branch from 552bf28 to 8bedff1 Compare June 12, 2019 22:14

ajor reviewed Jun 12, 2019

View reviewed changes

mmarchini force-pushed the add-missing-codegen-tests branch 2 times, most recently from 4f75356 to a795a6f Compare June 13, 2019 02:24

mmarchini commented Jun 13, 2019

View reviewed changes

mmarchini force-pushed the add-missing-codegen-tests branch from a795a6f to efafb8f Compare June 13, 2019 16:03

mmarchini requested a review from ajor June 14, 2019 17:16

mmarchini force-pushed the add-missing-codegen-tests branch from efafb8f to a6e2521 Compare June 14, 2019 18:17

ajor approved these changes Jun 16, 2019

View reviewed changes

tests/CMakeLists.txt Outdated Show resolved Hide resolved

[tests] add missing tests to codegen.cpp

8d03501

mmarchini force-pushed the add-missing-codegen-tests branch from a6e2521 to 8d03501 Compare June 17, 2019 15:47

mmarchini merged commit 012ebda into bpftrace:master Jun 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] add missing tests to codegen.cpp #574

[tests] add missing tests to codegen.cpp #574

mmarchini commented Apr 26, 2019

ajor commented Apr 27, 2019 •

edited

Loading

ajor commented May 1, 2019

mmarchini commented May 31, 2019

mmarchini May 31, 2019

mmarchini May 31, 2019

ajor Jun 2, 2019

mmarchini Jun 11, 2019

mmarchini commented May 31, 2019

danobi left a comment

danobi Jun 6, 2019

mmarchini commented Jun 6, 2019

ajor left a comment

ajor left a comment •

edited

Loading

mmarchini commented Jun 12, 2019

ajor commented Jun 12, 2019

mmarchini commented Jun 12, 2019

ajor commented Jun 12, 2019

mmarchini commented Jun 12, 2019

ajor Jun 12, 2019

mmarchini Jun 14, 2019

ajor Jun 12, 2019

ajor Jun 12, 2019

ajor Jun 12, 2019

This comment was marked as resolved.

mmarchini Jun 13, 2019

mmarchini Jun 14, 2019

mmarchini commented Jun 14, 2019

ajor left a comment

[tests] add missing tests to codegen.cpp #574

[tests] add missing tests to codegen.cpp #574

Conversation

mmarchini commented Apr 26, 2019

ajor commented Apr 27, 2019 • edited Loading

ajor commented May 1, 2019

mmarchini commented May 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented May 31, 2019

danobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented Jun 6, 2019

ajor left a comment

Choose a reason for hiding this comment

ajor left a comment • edited Loading

Choose a reason for hiding this comment

mmarchini commented Jun 12, 2019

ajor commented Jun 12, 2019

mmarchini commented Jun 12, 2019

ajor commented Jun 12, 2019

mmarchini commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented Jun 14, 2019

ajor left a comment

Choose a reason for hiding this comment

ajor commented Apr 27, 2019 •

edited

Loading

ajor left a comment •

edited

Loading