Skip to content

Commit

Permalink
Strengthen tracepoint format parsing
Browse files Browse the repository at this point in the history 
There's issue in current RHEL real time kernel with tracepoint format,
which makes bpftrace to return wrong data.

There's a 'not described gap' in the sched_wakeup's format file and
probably in other formats as well:

  # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/format
  name: sched_wakeup
  ID: 310
  format:
          field:unsigned short common_type;       offset:0;       size:2; signed:0;
          field:unsigned char common_flags;       offset:2;       size:1; signed:0;
          field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
          field:int common_pid;   offset:4;       size:4; signed:1;
          field:unsigned char common_migrate_disable;     offset:8;       size:1; signed:0;
          field:unsigned char common_preempt_lazy_count;  offset:9;       size:1; signed:0;

          field:char comm[16];    offset:12;      size:16;        signed:1;
          field:pid_t pid;        offset:28;      size:4; signed:1;
          field:int prio; offset:32;      size:4; signed:1;
          field:int success;      offset:36;      size:4; signed:1;
          field:int target_cpu;   offset:40;      size:4; signed:1;

There's "common_preempt_lazy_count" field on offset 9 with size 1:
        common_preempt_lazy_count;  offset:9;       size:1;

and it's followed by "comm" field on offset 12:
        field:char comm[16];    offset:12;      size:16;        signed:1;

which makes 2 bytes gap in between, that might confuse some applications
like bpftrace.

The tracepoint parser makes struct out of the field descriptions,
but does not account for such gaps.

I posted patch to fix this [1] in RT kernel, but that might take a while,
and we could easily fix our tracepoint parser to workaround this issue.

Adding code to detect this gaps and add 1 byte __pad_X fields, where X is
the offset number.

The test code for the parser uses zero offset in tests, which will
not happen in real life, but we can live with that and limit the
gap generation only when offset is defined.

Adding new test for sched:sched_wakeup with the gap problem.

[1] https://lore.kernel.org/linux-rt-users/20200221153541.681468-1-jolsa@kernel.org/
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
  • Loading branch information
@olsajiri
olsajiri committed Mar 11, 2020
1 parent b120077 commit a2e3d5d
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 16 deletions.
27 changes: 24 additions & 3 deletions src/tracepoint_format_parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,11 @@ std::string TracepointFormatParser::get_struct_name(const std::string &category,
return "struct _tracepoint_" + category + "_" + event_name;
}

std::string TracepointFormatParser::parse_field(const std::string &line)
std::string TracepointFormatParser::parse_field(const std::string &line,
int *last_offset)
{
std::string extra = "";

auto field_pos = line.find("field:");
if (field_pos == std::string::npos)
return "";
Expand All @@ -143,6 +146,23 @@ std::string TracepointFormatParser::parse_field(const std::string &line)
return "";

int size = std::stoi(line.substr(size_pos + 5, size_semi_pos - size_pos - 5));
int offset = std::stoi(
line.substr(offset_pos + 7, offset_semi_pos - offset_pos - 7));

// If there'a gap between last field and this one,
// generate padding fields
if (offset && *last_offset)
{
int i, gap = offset - *last_offset;

for (i = 0; i < gap; i++)
{
extra += " char __pad_" + std::to_string(offset - gap + i) + ";\n";
}
}

*last_offset = offset + size;

std::string field = line.substr(field_pos + 6, field_semi_pos - field_pos - 6);
auto field_type_end_pos = field.find_last_of("\t ");
if (field_type_end_pos == std::string::npos)
Expand All @@ -160,7 +180,7 @@ std::string TracepointFormatParser::parse_field(const std::string &line)
if (field_name.find("[") == std::string::npos)
field_type = adjust_integer_types(field_type, size);

return " " + field_type + " " + field_name + ";\n";
return extra + " " + field_type + " " + field_name + ";\n";
}

std::string TracepointFormatParser::adjust_integer_types(const std::string &field_type, int size)
Expand All @@ -183,10 +203,11 @@ std::string TracepointFormatParser::adjust_integer_types(const std::string &fiel
std::string TracepointFormatParser::get_tracepoint_struct(std::istream &format_file, const std::string &category, const std::string &event_name)
{
std::string format_struct = get_struct_name(category, event_name) + "\n{\n";
int last_offset = 0;

for (std::string line; getline(format_file, line); )
{
format_struct += parse_field(line);
format_struct += parse_field(line, &last_offset);
}

format_struct += "};\n";
Expand Down
2 changes: 1 addition & 1 deletion src/tracepoint_format_parser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class TracepointFormatParser
const std::string &event_name);

private:
static std::string parse_field(const std::string &line);
static std::string parse_field(const std::string &line, int *last_offset);
static std::string adjust_integer_types(const std::string &field_type,
int size);
static std::set<std::string> struct_list;
Expand Down
73 changes: 61 additions & 12 deletions tests/tracepoint_format_parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,21 @@ TEST(tracepoint_format_parser, tracepoint_struct)
"\n"
"print fmt: \"fd: 0x%08lx, buf: 0x%08lx, count: 0x%08lx\", ((unsigned long)(REC->fd)), ((unsigned long)(REC->buf)), ((unsigned long)(REC->count))\n";

std::string expected =
"struct _tracepoint_syscalls_sys_enter_read\n"
"{\n"
" unsigned short common_type;\n"
" unsigned char common_flags;\n"
" unsigned char common_preempt_count;\n"
" int common_pid;\n"
" int __syscall_nr;\n"
" u64 fd;\n"
" char * buf;\n"
" size_t count;\n"
"};\n";
std::string expected = "struct _tracepoint_syscalls_sys_enter_read\n"
"{\n"
" unsigned short common_type;\n"
" unsigned char common_flags;\n"
" unsigned char common_preempt_count;\n"
" int common_pid;\n"
" int __syscall_nr;\n"
" char __pad_12;\n"
" char __pad_13;\n"
" char __pad_14;\n"
" char __pad_15;\n"
" u64 fd;\n"
" char * buf;\n"
" size_t count;\n"
"};\n";

std::istringstream format_file(input);

Expand Down Expand Up @@ -149,6 +152,52 @@ TEST(tracepoint_format_parser, adjust_integer_types)
EXPECT_EQ(expected, result);
}

TEST(tracepoint_format_parser, padding)
{
std::string input =
" field:unsigned short common_type; offset:0; size:2; "
"signed:0;\n"
" field:unsigned char common_flags; offset:2; size:1; "
"signed:0;\n"
" field:unsigned char common_preempt_count; offset:3; "
"size:1; signed:0;\n"
" field:int common_pid; offset:4; size:4; signed:1;\n"
" field:unsigned char common_migrate_disable; offset:8; "
"size:1; signed:0;\n"
" field:unsigned char common_preempt_lazy_count; offset:9; "
"size:1; signed:0;\n"

" field:char comm[16]; offset:12; size:16; signed:1;\n"
" field:pid_t pid; offset:28; size:4; signed:1;\n"
" field:int prio; offset:32; size:4; signed:1;\n"
" field:int success; offset:36; size:4; signed:1;\n"
" field:int target_cpu; offset:40; size:4; signed:1;\n";

std::string expected = "struct _tracepoint_sched_sched_wakeup\n"
"{\n"
" unsigned short common_type;\n"
" unsigned char common_flags;\n"
" unsigned char common_preempt_count;\n"
" int common_pid;\n"
" unsigned char common_migrate_disable;\n"
" unsigned char common_preempt_lazy_count;\n"
" char __pad_10;\n"
" char __pad_11;\n"
" char comm[16];\n"
" pid_t pid;\n"
" int prio;\n"
" int success;\n"
" int target_cpu;\n"
"};\n";

std::istringstream format_file(input);

std::string result = MockTracepointFormatParser::get_tracepoint_struct_public(
format_file, "sched", "sched_wakeup");

EXPECT_EQ(expected, result);
}

} // namespace tracepoint_format_parser
} // namespace test
} // namespace bpftrace

0 comments on commit a2e3d5d

Please sign in to comment.