diff mbox series

libbpf: add support to GCC in CORE macro definitions

Message ID 87v86z150o.fsf@oracle.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series libbpf: add support to GCC in CORE macro definitions | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-10 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release

Commit Message

Cupertino Miranda Feb. 8, 2024, 1:04 p.m. UTC 
Hi everyone,

This is a patch to make CORE builtin macros work with builtin
implementation within GCC.

Looking forward to your comments.

Regards,
Cupertino

Comments

Andrii Nakryiko Feb. 8, 2024, 6:50 p.m. UTC | #1 
On Thu, Feb 8, 2024 at 5:07 AM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
>
> Hi everyone,
>
> This is a patch to make CORE builtin macros work with builtin
> implementation within GCC.
>
> Looking forward to your comments.
>

Can you please repost it as a proper patch email, not as an attachment?

But generally speaking, is there any way to change/fix GCC to allow a
much more straightforward way to capture type, similar to how Clang
does it? I'm not a big fan of extern declarations and using per-file
__COUNTER__. Externs are globally visible and we can potentially run
into name conflicts because __COUNTER__ is not globally unique.

And just in general, it seems like this shouldn't require such acrobatics.

Jose, do you have any thoughts on this?

> Regards,
> Cupertino
>
Jose E. Marchesi Feb. 8, 2024, 7:54 p.m. UTC | #2 
> On Thu, Feb 8, 2024 at 5:07 AM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>>
>> Hi everyone,
>>
>> This is a patch to make CORE builtin macros work with builtin
>> implementation within GCC.
>>
>> Looking forward to your comments.
>>
>
> Can you please repost it as a proper patch email, not as an attachment?
>
> But generally speaking, is there any way to change/fix GCC to allow a
> much more straightforward way to capture type, similar to how Clang
> does it? I'm not a big fan of extern declarations and using per-file
> __COUNTER__. Externs are globally visible and we can potentially run
> into name conflicts because __COUNTER__ is not globally unique.
>
> And just in general, it seems like this shouldn't require such
> acrobatics.
>
> Jose, do you have any thoughts on this?

Yes the macro is ugly and more elaborated than the clang version, but I
am afraid it is necessary in order to overcome the fact GCC
constant-folds enumerated values at parse-time.

Note however that the expression-statement itself to which the macro
expands is not elaborated, much like the null pointer dereference in the
clang version doesn't get elaborated.  These are just conveyed to the
builtins an the builtins use the TREE (IR in case of clang I guess) to
extract the type from it.

As far as I understand it the extern declaration in the macro is not
declaring an object with extern visibility, so it should not result in
any symbol being defined nor have any impact outside of the compilation
unit.  The __COUNTER__ is there just so you can use the macro more than
once in the same compilation unit, but that's all.

Cuper will correct me if I am wrong.

>
>> Regards,
>> Cupertino
>>
Cupertino Miranda Feb. 8, 2024, 8:11 p.m. UTC | #3 
Jose E. Marchesi writes:

>> On Thu, Feb 8, 2024 at 5:07 AM Cupertino Miranda
>> <cupertino.miranda@oracle.com> wrote:
>>>
>>>
>>> Hi everyone,
>>>
>>> This is a patch to make CORE builtin macros work with builtin
>>> implementation within GCC.
>>>
>>> Looking forward to your comments.
>>>
>>
>> Can you please repost it as a proper patch email, not as an attachment?
Apologies for that. Was unaware of the requirement.
>>
>> But generally speaking, is there any way to change/fix GCC to allow a
>> much more straightforward way to capture type, similar to how Clang
>> does it?

I tried, but due to GCC front-end specifics it is not possible without
overly change how GCC front-end works.

It is not only the constant folding of the enums as Jose suggests, but
also the cast of 0 gets optimized away by the parser itself. Leaving the
builtins expansion without a clue of the precise type used in a field
expression, as an example.

>> I'm not a big fan of extern declarations and using per-file
>> __COUNTER__. Externs are globally visible and we can potentially run
>> into name conflicts because __COUNTER__ is not globally unique.

The symbols with the __COUNTER__ are consumed by the builtins expansion
and will never reach the output.

>>
>> And just in general, it seems like this shouldn't require such
>> acrobatics.
>>
>> Jose, do you have any thoughts on this?
>
> Yes the macro is ugly and more elaborated than the clang version, but I
> am afraid it is necessary in order to overcome the fact GCC
> constant-folds enumerated values at parse-time.
>
> Note however that the expression-statement itself to which the macro
> expands is not elaborated, much like the null pointer dereference in the
> clang version doesn't get elaborated.  These are just conveyed to the
> builtins an the builtins use the TREE (IR in case of clang I guess) to
> extract the type from it.
>
> As far as I understand it the extern declaration in the macro is not
> declaring an object with extern visibility, so it should not result in
> any symbol being defined nor have any impact outside of the compilation
> unit.  The __COUNTER__ is there just so you can use the macro more than
> once in the same compilation unit, but that's all.
>
> Cuper will correct me if I am wrong.
>
>>
>>> Regards,
>>> Cupertino
>>>
diff mbox series

Patch

>From c1a3a09c5949363a888f6159fa3cc16650e61c07 Mon Sep 17 00:00:00 2001
From: Cupertino Miranda <cupertino.miranda@oracle.com>
Date: Wed, 7 Feb 2024 15:01:03 +0000
Subject: [PATCH] libbpf: add support to GCC in CORE macro definitions

Due to internal differences between LLVM and GCC the current
implementation for the CO-RE macros does not fit GCC parser, as it will
optimize those expressions even before those would be accessible by the
BPF backend.

As examples, the following would be optimized out with the original
definitions:
  - As enums are converted to their integer representation during
  parsing, the IR would not know how to distinguish an integer
  constant from an actual enum value.
  - Types need to be kept as temporary variables, as the existing type
  casts of the 0 address (as expanded for LLVM), are optimized away by
  the GCC C parser, never really reaching GCCs IR.

Although, the macros appear to add extra complexity, the expanded code
is removed from the compilation flow very early in the compilation
process, not really affecting the quality of the generated assembly.

Signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
---
 tools/lib/bpf/bpf_core_read.h | 46 ++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index 0d3e88bd7d5f..074f1f4e4d2b 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -81,6 +81,23 @@  enum bpf_enum_value_kind {
 	val;								      \
 })
 
+/* Differentiator between compilers builtin implementations. This is a
+ * requirement due to the compiler parsing differences where GCC optimizes
+ * early in parsing those constructs of type pointers to the builtin specific
+ * type, resulting in not being possible to collect the required type
+ * information in the builtin expansion.
+ */
+#ifdef __clang__
+#define bpf_type_for_compiler(type) ((typeof(type) *) 0)
+#else
+#define COMPOSE_VAR(t, s) t##s
+#define bpf_type_for_compiler1(type, NR) ({ \
+	extern  typeof(type) *COMPOSE_VAR(bpf_type_tmp_, NR); \
+	COMPOSE_VAR(bpf_type_tmp_, NR); \
+})
+#define bpf_type_for_compiler(type) bpf_type_for_compiler1(type, __COUNTER__)
+#endif
+
 /*
  * Extract bitfield, identified by s->field, and return its value as u64.
  * This version of macro is using direct memory reads and should be used from
@@ -145,8 +162,13 @@  enum bpf_enum_value_kind {
 	}								\
 })
 
+#ifdef __clang__
 #define ___bpf_field_ref1(field)	(field)
-#define ___bpf_field_ref2(type, field)	(((typeof(type) *)0)->field)
+#define ___bpf_field_ref2(type, field)	(bpf_type_for_compiler(type)->field)
+#else
+#define ___bpf_field_ref1(field)	(&(field))
+#define ___bpf_field_ref2(type, field)	(&(bpf_type_for_compiler(type)->field))
+#endif
 #define ___bpf_field_ref(args...)					    \
 	___bpf_apply(___bpf_field_ref, ___bpf_narg(args))(args)
 
@@ -196,7 +218,7 @@  enum bpf_enum_value_kind {
  * BTF. Always succeeds.
  */
 #define bpf_core_type_id_local(type)					    \
-	__builtin_btf_type_id(*(typeof(type) *)0, BPF_TYPE_ID_LOCAL)
+	__builtin_btf_type_id(*bpf_type_for_compiler(type), BPF_TYPE_ID_LOCAL)
 
 /*
  * Convenience macro to get BTF type ID of a target kernel's type that matches
@@ -206,7 +228,7 @@  enum bpf_enum_value_kind {
  *    - 0, if no matching type was found in a target kernel BTF.
  */
 #define bpf_core_type_id_kernel(type)					    \
-	__builtin_btf_type_id(*(typeof(type) *)0, BPF_TYPE_ID_TARGET)
+	__builtin_btf_type_id(*bpf_type_for_compiler(type), BPF_TYPE_ID_TARGET)
 
 /*
  * Convenience macro to check that provided named type
@@ -216,7 +238,7 @@  enum bpf_enum_value_kind {
  *    0, if no matching type is found.
  */
 #define bpf_core_type_exists(type)					    \
-	__builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_EXISTS)
+	__builtin_preserve_type_info(*bpf_type_for_compiler(type), BPF_TYPE_EXISTS)
 
 /*
  * Convenience macro to check that provided named type
@@ -226,7 +248,7 @@  enum bpf_enum_value_kind {
  *    0, if the type does not match any in the target kernel
  */
 #define bpf_core_type_matches(type)					    \
-	__builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_MATCHES)
+	__builtin_preserve_type_info(*bpf_type_for_compiler(type), BPF_TYPE_MATCHES)
 
 /*
  * Convenience macro to get the byte size of a provided named type
@@ -236,7 +258,7 @@  enum bpf_enum_value_kind {
  *    0, if no matching type is found.
  */
 #define bpf_core_type_size(type)					    \
-	__builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_SIZE)
+	__builtin_preserve_type_info(*bpf_type_for_compiler(type), BPF_TYPE_SIZE)
 
 /*
  * Convenience macro to check that provided enumerator value is defined in
@@ -246,8 +268,14 @@  enum bpf_enum_value_kind {
  *    kernel's BTF;
  *    0, if no matching enum and/or enum value within that enum is found.
  */
+#ifdef __clang__
 #define bpf_core_enum_value_exists(enum_type, enum_value)		    \
 	__builtin_preserve_enum_value(*(typeof(enum_type) *)enum_value, BPF_ENUMVAL_EXISTS)
+#else
+#define bpf_core_enum_value_exists(enum_type, enum_value)		    \
+	__builtin_preserve_enum_value(bpf_type_for_compiler(enum_type), \
+				      enum_value, BPF_ENUMVAL_EXISTS)
+#endif
 
 /*
  * Convenience macro to get the integer value of an enumerator value in
@@ -257,8 +285,14 @@  enum bpf_enum_value_kind {
  *    present in target kernel's BTF;
  *    0, if no matching enum and/or enum value within that enum is found.
  */
+#ifdef __clang__
 #define bpf_core_enum_value(enum_type, enum_value)			    \
 	__builtin_preserve_enum_value(*(typeof(enum_type) *)enum_value, BPF_ENUMVAL_VALUE)
+#else
+#define bpf_core_enum_value(enum_type, enum_value)			    \
+	__builtin_preserve_enum_value(bpf_type_for_compiler(enum_type), \
+				      enum_value, BPF_ENUMVAL_VALUE)
+#endif
 
 /*
  * bpf_core_read() abstracts away bpf_probe_read_kernel() call and captures
-- 
2.39.2