From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id ED5F045E60; Tue, 10 Dec 2024 01:58:34 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8C86A4029B; Tue, 10 Dec 2024 01:58:34 +0100 (CET) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id D281A40264 for ; Tue, 10 Dec 2024 01:58:32 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.88.214]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Y6gK21pVzz1kvff; Tue, 10 Dec 2024 08:56:06 +0800 (CST) Received: from kwepemk500009.china.huawei.com (unknown [7.202.194.94]) by mail.maildlp.com (Postfix) with ESMTPS id 10C1A1A016C; Tue, 10 Dec 2024 08:58:30 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by kwepemk500009.china.huawei.com (7.202.194.94) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Tue, 10 Dec 2024 08:58:29 +0800 Message-ID: Date: Tue, 10 Dec 2024 08:58:29 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [EXTERNAL] Re: [PATCH 1/2] lib/dmadev: eliminate undefined behavior To: Jerin Jacob , Bruce Richardson , Andre Muezerie CC: Kevin Laatz , "dev@dpdk.org" References: <1733513273-25550-1-git-send-email-andremue@linux.microsoft.com> <1733513273-25550-2-git-send-email-andremue@linux.microsoft.com> Content-Language: en-US From: fengchengwen In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemk500009.china.huawei.com (7.202.194.94) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2024/12/10 7:21, Jerin Jacob wrote: > > >> -----Original Message----- >> From: Bruce Richardson >> Sent: Monday, December 9, 2024 4:58 AM >> To: Andre Muezerie ; Jerin Jacob >> >> Cc: Chengwen Feng ; Kevin Laatz >> ; dev@dpdk.org >> Subject: [EXTERNAL] Re: [PATCH 1/2] lib/dmadev: eliminate undefined behavior >> >> On Fri, Dec 06, 2024 at 11: 27: 52AM -0800, Andre Muezerie wrote: > MSVC >> compiler issues warnings like the one below: > > >> lib\dmadev\rte_dmadev_trace_fp. h(36): > warning C5101: use of preprocessor >> directive in > function-like macro >> On Fri, Dec 06, 2024 at 11:27:52AM -0800, Andre Muezerie wrote: >>> MSVC compiler issues warnings like the one below: >>> >>> lib\dmadev\rte_dmadev_trace_fp.h(36): >>> warning C5101: use of preprocessor directive in >>> function-like macro argument list is undefined behavior >>> >>> Indeed, looking at C99 section 6.10.3 Macro replacement, paragraph 11: >>> "If there are sequences of preprocessing tokens within the list of >>> arguments that would otherwise act as preprocessing directives, the >>> behavior is undefined." >>> >>> The fix proposed in this patch moves the ifdef to the outside. >>> This results in no perf impact, but some lines end up being >>> duplicated, which seems to be a reasonable trade-off. >>> >>> Signed-off-by: Andre Muezerie >>> --- >>> lib/dmadev/rte_dmadev_trace.h | 62 ++++++++++++++++++++++++++++-- >> -- >>> lib/dmadev/rte_dmadev_trace_fp.h | 52 +++++++++++++++++++++++---- >>> 2 files changed, 102 insertions(+), 12 deletions(-) >>> >>> diff --git a/lib/dmadev/rte_dmadev_trace.h >>> b/lib/dmadev/rte_dmadev_trace.h index be089c065c..264a464928 100644 >>> --- a/lib/dmadev/rte_dmadev_trace.h >>> +++ b/lib/dmadev/rte_dmadev_trace.h >>> @@ -19,13 +19,12 @@ >>> extern "C" { >>> #endif >>> >>> +#ifdef _RTE_TRACE_POINT_REGISTER_H_ >>> RTE_TRACE_POINT( >>> rte_dma_trace_info_get, >>> RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info >> *dev_info), >>> -#ifdef _RTE_TRACE_POINT_REGISTER_H_ > > > + @Chengwen Feng > > This kind of patten is not used other places like ethdev traces, Why we need this kind of pattern in dmadev? > Looks like, it can be fixed by caller of this function by initializing struct rte_dma_info. So may not need a fixup patch to begin with It's strange that no other library doesn't have this problem. When I first add tracepoints support for dmadev, there is no such macro (just like other library), but the CI report ASAN error. The rootcause is that register: RTE_TRACE_POINT_REGISTER(rte_dma_trace_info_get, lib.dmadev.info_get) it will invoke : __rte_trace_point_register(&__rte_dma_trace_info_get, __rte_dma_trace_info_get_lib.dmadev.info_get, (void (*)(void)rte_dma_trace_info_get) { rte_dma_trace_info_get(); } But rte_dma_trace_info_get() it was defined with parameters: int16_t dev_id, struct rte_dma_info *dev_info If we force invoke rte_dma_trace_info_get() without pass any parameter, it may lead to ASAN problem because the parameter's corresponding register was not set (and it's value undefine). Because it was happened only when register, so add such macro for it. > > >>> struct rte_dma_info __dev_info = {0}; >>> dev_info = &__dev_info; >>> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */ >>> rte_trace_point_emit_i16(dev_id); >>> rte_trace_point_emit_string(dev_info->dev_name); >>> rte_trace_point_emit_u64(dev_info->dev_capa); >>> @@ -37,15 +36,30 @@ RTE_TRACE_POINT( >>> rte_trace_point_emit_u16(dev_info->nb_vchans); >>> rte_trace_point_emit_u16(dev_info->nb_priorities); >>> ) >>> +#else >>> +RTE_TRACE_POINT( >>> + rte_dma_trace_info_get, >>> + RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info >> *dev_info), >>> + rte_trace_point_emit_i16(dev_id); >>> + rte_trace_point_emit_string(dev_info->dev_name); >>> + rte_trace_point_emit_u64(dev_info->dev_capa); >>> + rte_trace_point_emit_u16(dev_info->max_vchans); >>> + rte_trace_point_emit_u16(dev_info->max_desc); >>> + rte_trace_point_emit_u16(dev_info->min_desc); >>> + rte_trace_point_emit_u16(dev_info->max_sges); >>> + rte_trace_point_emit_i16(dev_info->numa_node); >>> + rte_trace_point_emit_u16(dev_info->nb_vchans); >>> + rte_trace_point_emit_u16(dev_info->nb_priorities); >>> +) >>> +#endif /* _RTE_TRACE_POINT_REGISTER_H_ */ >>> >> >> +Jerin >> >> I'm unfortunately not familiar with the internals of the traceing library. >> Can someone perhaps explain (and maybe add in the code as a comment), why >> we need conditional compilation here? Specifically, when would >> _RTE_TRACE_POINT_REGISTER_H_ be defined, and when not defined? (or why >> not >> defined?) How does having it defined or not affect the expansion of the macro >> here? >> >> Thanks, >> /Bruce >