Re: [PATCH 21/28] mlx5: Avoid gcc 5.4 warning -Wempty-body

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

On Wed, Sep 14, 2016 at 10:55:19AM -0600, Jason Gunthorpe wrote:
> On Wed, Sep 14, 2016 at 07:25:25PM +0300, Yishai Hadas wrote:
> > >Which is functionally okay, but strange.
> >
> > As you pointed, there is no functional issue with that.
>
> Yet is a common enough errnonous construction to attract a compiler
> warning. Don't do it.
>
> Further, it is just a way to attract bit rot in your dbg macros if you
> don't check the format string during a normal compile.
>
> > >It is much better to make mlx5_dbg an empty static inline, this way it
> > >continues to do parameter type validation even when disabled, and we
> > >can drop the sprinkling of #ifdef MLX5_DEBUG everywhere.
> >
> > The idea was to have an 'empty' code when this macro is used as some code is
> > done in the data path, see below notes.
>
> Yes, I know what the intent was, this change does exactly the same thing.
>
> > >+static inline void mlx5_dbg(FILE *fp, uint32_t mask, const char *fmt, ...)
> > >+{
> > >+}
> > > #endif
> >
> > Inline is not guaranteed by the compiler, calling empty function in some
> > data data path flows should be prevented.
>
> No compiler we support will leave behind a function call for an empty
> static inline.
>
> I just checked, the assembly is the same before/after modulo compiler
> non-determinism. There are no extra function calls.

There is always inline attribute to tell to GCC to inline it.
#define ALWAYS_INLINE inline __attribute__ ((always_inline))

>
> > > enum {
> > >diff --git a/libmlx5/src/qp.c b/libmlx5/src/qp.c
> > >index c805fcae4123..23270e50af7a 100644
> > >+++ b/libmlx5/src/qp.c
> > >@@ -356,9 +356,7 @@ static inline int copy_eth_inline_headers(struct ibv_qp *ibqp,
> > > 	int inl_hdr_size = MLX5_ETH_L2_INLINE_HEADER_SIZE;
> > > 	int inl_hdr_copy_size = 0;
> > > 	int j = 0;
> > >-#ifdef MLX5_DEBUG
> > > 	FILE *fp = to_mctx(ibqp->context)->dbg_fp;
> > >-#endif
> >
> > This is data path flow, dropping the #ifdef may cause a redundant assignment
> > for 'fp' here.
>
> Nope:
>
> $ nm --size libmlx4-rdmv2.so
> [before]
> 0000000000001637 t _mlx5_post_send
> [after]
> 0000000000001637 t _mlx5_post_send
>
> The compilers are very smart, you don't need to hand hold them.
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]