clang_parser: workaround for asm goto in Kernel 5+ headers

[mmarchini]

Fixes: #611

[mmarchini]

I couldn't test it yet because I don't have a Kernel 5+ readily available now.

@joestringer @olsajiri if you could try it that would be really helpful, since you both were suffering from this issue.

[mmarchini]

Also, I'll need to update codegen and parser tests later.

[olsajiri]

I couldn't test it yet because I don't have a Kernel 5+ readily available now.

@joestringer @olsajiri if you could try it that would be really helpful, since you both were suffering from this issue.

with the extra change below, your fix is working for me,
we need to include linux/types.h to load the compiler headers

I wonder we should put it to extra unsaved_files header,
like they did in samples/bpf/asm_goto_workaround.h

diff --git a/src/clang_parser.cpp b/src/clang_parser.cpp
index 509d01453032..98e788c7ad49 100644
--- a/src/clang_parser.cpp
+++ b/src/clang_parser.cpp
@@ -294,7 +294,8 @@ bool ClangParser::parse(ast::Program *program, BPFtrace &bpftrace, std::vector<s
   // able to parse our headers.
   //
   // From: https://github.com/iovisor/bcc/pull/2133/files
-  input = "#ifdef asm_volatile_goto\n"
+  input = "#include <linux/types.h>\n"
+          "#ifdef asm_volatile_goto\n"
           "#undef asm_volatile_goto\n"
           "#define asm_volatile_goto(x...) asm volatile(\"invalid use of asm_volatile_goto\")\n"
           "#endif\n\n" + input;

[mmarchini]

we need to include linux/types.h to load the compiler headers

It is already included (or at least should be, unless something changed recently).

[olsajiri]

yes, but I think it needs to come before the #undef, so it cant get disabled

[mmarchini]

I was looking at bcc's implementation. After looking at samples/bpf/asm_goto_workaround.h I agree it would be safer if the macro came after including types.h. What about adding our workaround after the C definitions input?

diff --git a/src/clang_parser.cpp b/src/clang_parser.cpp
index 509d014..9498a9c 100644
--- a/src/clang_parser.cpp
+++ b/src/clang_parser.cpp
@@ -294,10 +294,10 @@ bool ClangParser::parse(ast::Program *program, BPFtrace &bpftrace, std::vector<s
   // able to parse our headers.
   //
   // From: https://github.com/iovisor/bcc/pull/2133/files
-  input = "#ifdef asm_volatile_goto\n"
+  input = input + "#ifdef asm_volatile_goto\n"
           "#undef asm_volatile_goto\n"
           "#define asm_volatile_goto(x...) asm volatile(\"invalid use of asm_volatile_goto\")\n"
-          "#endif\n\n" + input;
+          "#endif\n\n";
 
   CXUnsavedFile unsaved_files[] =
   {

This way we guarantee that nothing inside the program will override our definiton (and as a plus we don't need to change our tests :) ).

[olsajiri]

that still fails for me with the same errors,
I pushed out temporary fix which I'm using at the moment
for reference: olsajiri@641162a

[mmarchini]

@olsajiri thank you for sharing your fix. I made a few changes and updated the PR, should be working now.

[ajor]

Should normally wrap the contents of a #define in brackets for preprocessor safety, but since ASM_GOTO_WORKAROUND_PATH is only used once anyway we might as well just get rid of this define. That would match how the other builtin headers are done too.

[ajor]

Suggested change

	.Filename = ASM_GOTO_WORKAROUND_PATH,
	.Filename = "/bpftrace/include/" ASM_GOTO_WORKAROUND_H,

To try and clarify what I was talking about in the other comment

[joestringer]

Got around to testing this out, it works beautifully thanks folks!