Re: [PATCH bpf-next 3/5] selftests/bpf: fix test_cpp compilation failure with clang

[Yonghong Song <yhs@fb.com>]

On 4/11/21 11:06 PM, Sedat Dilek wrote:
> On Mon, Apr 12, 2021 at 7:42 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 4/11/21 9:47 PM, Sedat Dilek wrote:
>>> On Sun, Apr 11, 2021 at 9:08 PM Yonghong Song <yhs@fb.com> wrote:
>>> [ ... ]
>>>>> BTW, did you check (llvm-)objdump output?
>>>>>
>>>>> $ /opt/llvm-toolchain/bin/llvm-objdump-12 -Dr test_cpp | grep core_extern
>>>>
>>>> This is what I got with g++ compiled test_cpp:
>>>>
>>>> $ llvm-objdump -Dr test_cpp | grep core_extern
>>>>      406a80: e8 5b 01 00 00                callq   0x406be0
>>>> <_ZL25test_core_extern__destroyP16test_core_extern>
>>>>      406ab9: e8 22 01 00 00                callq   0x406be0
>>>> <_ZL25test_core_extern__destroyP16test_core_extern>
>>>> 0000000000406be0 <_ZL25test_core_extern__destroyP16test_core_extern>:
>>>>      406be3: 74 1a                         je      0x406bff
>>>> <_ZL25test_core_extern__destroyP16test_core_extern+0x1f>
>>>>      406bef: 74 05                         je      0x406bf6
>>>> <_ZL25test_core_extern__destroyP16test_core_extern+0x16>
>>>>
>>>
>>> What is the output when compiling with clang++ in your bpf-next environment?
>>
>> $ llvm-objdump -Dr test_cpp | grep core_extern
>> $
>>
>> So looks like all test_core_extern_*() functions are inlined.
>> This can be confirmed by looking at assembly code.
>> while for gcc, there is still the call to
>>     _ZL25test_core_extern__destroyP16test_core_extern
>> which is
>>     test_core_extern__destroy(test_core_extern*)
>>
>> This is just a difference between compiler optimizations
>> between gcc and clang. We don't need to worry about this.
>>
> 
> ( My previous comment was from my samrtphone - so I started into my desktop. )
> 
> Thanks for your analysis and hint about inlining.
> 
> My inbox is full with that different handling of inlining "GCC vs. LLVM/Clang".
> 
> When I recall correctly and I have not to care about the inlining
> optimization of clang++, then we can drop
> "$(OUTPUT)/test_core_extern.skel.h" from the BPF selftests Makefile:
> 
> [ tools/testing/selftests/bpf/Makefile ]
> 
> # Make sure we are able to include and link libbpf against c++.
> $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
> $(call msg,CXX,,$@)
> $(Q)$(CXX) $(CFLAGS) test_cpp.cpp $(BPFOBJ) $(LDLIBS) -o $@
> 
> Note: This is with your patchset applied against Linus Git
> 
> As we have the include here:
> 
> [ tools/testing/selftests/bpf/test_cpp.cpp ]
> 
> /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> #include <iostream>
> #include <bpf/libbpf.h>
> #include <bpf/bpf.h>
> #include <bpf/btf.h>
> #include "test_core_extern.skel.h"
> ...
> 
> With and without keeping "test_core_extern.skel.h" I got the same
> output with g++ and llvm-objdump.
> Compiling with clang++ did not show that "CPP-file and C-header"
> build-error when dropping "test_core_extern.skel.h" from the Makefile.

Thanks for testing. Great to see all g++/clang++ compilations passed!

> 
> As said here all my testings with Linus Git not bpf-next.
> 
> Thanks for your precious time!
> 
> - Sedat -
>