Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2b84fe80-60b6-c9cb-369d-47fb0995aed3@fb.com>
Date:   Sat, 10 Sep 2022 17:06:56 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.13.0
Subject: Re: [PATCH] libbpf: define bpf_tail_call_static when __clang__ is not
 defined
Content-Language: en-US
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>,
        James Hilliard <james.hilliard1@gmail.com>
Cc:     bpf@vger.kernel.org, Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev
References: <20220909224544.3702931-1-james.hilliard1@gmail.com>
 <CAEf4Bzbm_wq=n8+ve95aBtJkK-WcsUmKM_LT57XU0D9zS9gXuQ@mail.gmail.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <CAEf4Bzbm_wq=n8+ve95aBtJkK-WcsUmKM_LT57XU0D9zS9gXuQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QkVmSm0vNjNQazR5OURITU54dUl5aWdjVkVNYzVkeXpVZlVpVU1JS1JSbWNR?=
 =?utf-8?B?blNZNUUvSlIvOWZXRERsekd1ZEgyRVk2RzdrTlhUbHVuWkZqREIvZzRQWjha?=
 =?utf-8?B?V0JSV0o2UFI2K1p3RU91WkhpaVEyYkxRN3pSdGVMRDRmRjJNcWFLcVllTDlP?=
 =?utf-8?B?VXl3cjZzZUUvQVl0WWtubWlLS1IxSFBmOTczT3lZV01GOGN5SkIwemZxTUVv?=
 =?utf-8?B?ek1lOUowbGJxNGhZVVZyRmZLQUYwd3JRekNKa1VkWTBlSGZjYlNoMEJnY2pP?=
 =?utf-8?B?WWxZam83OFI3Nmd0Zi80ZU05QmRibStxeXF1cGpuVW13bVJlcmtwdkxPaXdD?=
 =?utf-8?B?TVVtdFU5TUxZM09VNk42a0t6dVEzTTJ2Q1lueDN3TEVvWkVrSmwzY3hmQ0I2?=
 =?utf-8?B?QVQxY1YrK3ZjVExVV0tqUUxOQWNMOFh5emlhQlhhWXI3SXNkSElteFRmcENE?=
 =?utf-8?B?enY2MkdIOXdkb1N3Qmx5cURZUUIrdjFOVnlpTDFrSWkwR1lVa3Z6cHM4ZkFs?=
 =?utf-8?B?OENHRzg0UERGMmdmVFFLd09XTXBKVGxVQ2FDcEtvUlhuWmhLZ1BiSVNCTTdF?=
 =?utf-8?B?YjJhRXg4MDdQRDJacGRUWDhVbG04aW5JR3ZaTFNPWTJ5Ti9FSVlwY3RUTUJX?=
 =?utf-8?B?SXFkYkFFNENuRUExVlpGQ3VralRJT09rb015MUhLYmZVaGl5c0MrU2g2aDlk?=
 =?utf-8?B?U2NEUjExUTZJYmY2Z3dYK2I1WjlpdUorWjlhSVpORDF4cCtlVjZVL2dHcUtk?=
 =?utf-8?B?NERUYmlvVVQvYTRTeVkzLzRCRGkvalExUTVFbURieHVtRHM2dUVqanRUd2dE?=
 =?utf-8?B?aDlqRFNvaE0vUFRIcVdtK3JjeTZobXJBSEZtTCtnZWczdm8vS3lLN3FPZ3BT?=
 =?utf-8?B?VTN5YnkwcVY1a1ZMSnV0Y3FRaUl4eUcxTVI2V2EwMXk3dktZZkpyM05RemQ0?=
 =?utf-8?B?UlVYQmlqeStQMmpwZklibXpJN0cxc0ZtTFJXZEtJaHVpQ29xZldtNVNLSk9t?=
 =?utf-8?B?NWgxSGdXbUFiLzNGQ21zOEtmQ2svVFR3eVhDY2h6Qi9zUnBBZElRVkJmTFlQ?=
 =?utf-8?B?YjBHTnNXRkpHNXcvRjNRZjZzMjB4MEd4WmMvYkRUT2p4R3IxdWlGVVpKeFd1?=
 =?utf-8?B?RTJIYXROU1lsOVR2cnlUVVFWcVh5enhOeGVpazZ5WWo5SktQMWs0UDc1cG5h?=
 =?utf-8?B?NzNpNWkyREZJa2t2Ym9vbDFoTXhaUkpnQjlIWmtSdFVGZHVFYzI4S0lzNzh4?=
 =?utf-8?B?K2pMMG8yRWt0TGU3MW4wWUpORFY5SDg3ODV6U3UzRWp6ekJLNTBtYWF4ZndL?=
 =?utf-8?B?b0IxRDcwWmVZaUZqdHl1TTFnckFmUDZNalQzTExrQ3FiUEM4cTAyR2FBbFZP?=
 =?utf-8?B?aUV3Z3EyYnM1TGpiajdhT2VRY2NOMVZ3UDZIcnRZczhaVngvZWFwZnU0cTIy?=
 =?utf-8?B?aVRaTzBPWHFkaWJNUlVmTkJ0ekFDeVk2K1RqOWxZOVlueDY5WnBtU0w5cE5S?=
 =?utf-8?B?c2pINFFiVXlDb1ZOTlE5RUFDNnZZMGJDOTM3TkZYWlBUZDB1SFhIV2M2Ry95?=
 =?utf-8?B?WVFFZVMrVXI2bTZObnVENDYzdHl0T2RreUYwOFhkR29tdHIyOXBJZmZoKzho?=
 =?utf-8?B?YUZaRFJFN2U1T0x0aDJ6UXkvMWFFZFBDTGhMdnZ3MWd6N1MwUDJycmkycWwz?=
 =?utf-8?B?TFVQYy9oUzZZWWR0TjFRTnJrT21QUkwwUWtISUZoZy9ZT0syZFJ1UmpsWXk2?=
 =?utf-8?B?RDBlZUlpOUpUcllLczRaZFdKQm5ocTJmeWN5TkZPNlFiQWxtYUtEMTJCUlZ0?=
 =?utf-8?B?ZDE0c2NmVUlGaDE2clFDN0IzNHJvejE1bXFZa2p0cVFCM1JZTmVMemM1blNV?=
 =?utf-8?B?RnR0OHJGRlRPSEdTYnBkNXRXWHdCWFpBdGE4bll0cmVleEZXWDVkamhocGc5?=
 =?utf-8?B?MFBra2N3aG8zT1dsaHFZcy9TZ3BUYS96V1VPSDNvUEp1WjFBeERGQnhVdFJC?=
 =?utf-8?B?MUZ3ZURNVHRkUmZTUWV3UU43OHppS2ZNN3RmZ0pHRklrSkhCY1JoV2xFaW5S?=
 =?utf-8?B?c1hjVWdoZlBJTURvajViSlpsNWRoUTNuWXllZno0OXhHMzRxMktIeFAyYy9H?=
 =?utf-8?Q?zDUW8Q9K0YME1EW0Gq03LW+Ob?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 37d88dd8-32dd-4a90-3640-08da93898d9d
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Sep 2022 00:07:02.3289
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ADRX/W+/6/R43qAbINBdIE2GgYKu3Rhm8+c/4jpia/4+xsCW0cR9HH8lQpDnuER6
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR15MB2436
X-Proofpoint-ORIG-GUID: ttNaPzvTOlOOh2kvrpqMC2BikcUHGjCG
X-Proofpoint-GUID: ttNaPzvTOlOOh2kvrpqMC2BikcUHGjCG
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-10_12,2022-09-09_01,2022-06-22_01
X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/9/22 5:26 PM, Andrii Nakryiko wrote:
> On Fri, Sep 9, 2022 at 3:46 PM James Hilliard <james.hilliard1@gmail.com> wrote:
>>
>> The bpf_tail_call_static function is currently not defined unless
>> using clang >= 8.
>>
>> To support bpf_tail_call_static on GCC we can check if __clang__ is
>> not defined to enable bpf_tail_call_static.
>>
>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>>   tools/lib/bpf/bpf_helpers.h | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>> index 7349b16b8e2f..30fc95e7cd76 100644
>> --- a/tools/lib/bpf/bpf_helpers.h
>> +++ b/tools/lib/bpf/bpf_helpers.h
>> @@ -131,7 +131,7 @@
>>   /*
>>    * Helper function to perform a tail call with a constant/immediate map slot.
>>    */
>> -#if __clang_major__ >= 8 && defined(__bpf__)
>> +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__)
>>   static __always_inline void
>>   bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>>   {
>> @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>>                  __bpf_unreachable();
>>
>>          /*
>> -        * Provide a hard guarantee that LLVM won't optimize setting r2 (map
>> -        * pointer) and r3 (constant map index) from _different paths_ ending
>> +        * Provide a hard guarantee that the compiler won't optimize setting r2
>> +        * (map pointer) and r3 (constant map index) from _different paths_ ending
>>           * up at the _same_ call insn as otherwise we won't be able to use the
>>           * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel
>>           * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key
>> @@ -148,8 +148,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
>>           *
>>           * Note on clobber list: we need to stay in-line with BPF calling
>>           * convention, so even if we don't end up using r0, r4, r5, we need
>> -        * to mark them as clobber so that LLVM doesn't end up using them
>> -        * before / after the call.
>> +        * to mark them as clobber so that the compiler doesn't end up using
>> +        * them before / after the call.
>>           */
>>          asm volatile("r1 = %[ctx]\n\t"
>>                       "r2 = %[map]\n\t"
> 
> will this compile as is on GCC-BPF? I'm trying to understand what's
> the point. Once GCC supports this ASM syntax we can add similar check
> to __clang_major__, instead of allowing it for all GCC versions?
> 
> We must have done __clang_major__ check for a reason, old Clangs
> probably had some problems compiling this. Maybe Daniel remembers?

Yes, clang >= 8 is needed to ensure 'slot' is propagated as 'const' 
instead of variables. clang 6 added support for inline asm but clang 8
is needed for above 'const' propagation into asm code.

> 
>> --
>> 2.34.1
>>