[v2,01/71] cocci/rte_memcpy: add script to eliminate fixed size rte_memcpy
Checks
Commit Message
Rte_memcpy should not be used for the simple case of copying
a fix size structure because it is slower and will hide problems
from code analysis tools. Coverity, fortify and other analyzers
special case memcpy().
Gcc (and Clang) are smart enough to inline copies which
will be faster.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
devtools/cocci/rte_memcpy.cocci | 11 +++++++++++
1 file changed, 11 insertions(+)
create mode 100644 devtools/cocci/rte_memcpy.cocci
Comments
On 2024-03-01 18:14, Stephen Hemminger wrote:
> Rte_memcpy should not be used for the simple case of copying
> a fix size structure because it is slower and will hide problems
> from code analysis tools. Coverity, fortify and other analyzers
> special case memcpy().
>
> Gcc (and Clang) are smart enough to inline copies which
> will be faster.
>
Are you suggesting rte_memcpy() calls aren't inlined?
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> devtools/cocci/rte_memcpy.cocci | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> create mode 100644 devtools/cocci/rte_memcpy.cocci
>
> diff --git a/devtools/cocci/rte_memcpy.cocci b/devtools/cocci/rte_memcpy.cocci
> new file mode 100644
> index 000000000000..fa1038fc066d
> --- /dev/null
> +++ b/devtools/cocci/rte_memcpy.cocci
> @@ -0,0 +1,11 @@
> +//
> +// rte_memcpy should not be used for simple fixed size structure
> +// because compiler's are smart enough to inline these.
> +//
What do you do in code where it's not known if the size will be constant
or not?
> +@@
> +expression src, dst; constant size;
> +@@
> +(
> +- rte_memcpy(dst, src, size)
> ++ memcpy(dst, src, size)
> +)
On Sat, 2 Mar 2024 12:19:13 +0100
Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> On 2024-03-01 18:14, Stephen Hemminger wrote:
> > Rte_memcpy should not be used for the simple case of copying
> > a fix size structure because it is slower and will hide problems
> > from code analysis tools. Coverity, fortify and other analyzers
> > special case memcpy().
> >
> > Gcc (and Clang) are smart enough to inline copies which
> > will be faster.
> >
>
> Are you suggesting rte_memcpy() calls aren't inlined?
With a simple made up example of copying an IPv6 address.
rte_memcpy() inlines to:
rte_copy_addr:
movdqu xmm0, XMMWORD PTR [rsi]
movups XMMWORD PTR [rdi], xmm0
movdqu xmm0, XMMWORD PTR [rsi]
movups XMMWORD PTR [rdi], xmm0
ret
Vs memcpy becomes.
copy_addr:
movdqu xmm0, XMMWORD PTR [rsi]
movups XMMWORD PTR [rdi], xmm0
ret
Looks like a bug in rte_memcpy_generic? Why is it not handling
16 bytes correctly.
For me, it was more about getting fortify and compiler warnings to
catch bugs. For most code, the output should be the same.
+To: x86 maintainers, another bug in rte_memcpy()
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 2 March 2024 18.02
>
> On Sat, 2 Mar 2024 12:19:13 +0100
> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> > On 2024-03-01 18:14, Stephen Hemminger wrote:
> > > Rte_memcpy should not be used for the simple case of copying
> > > a fix size structure because it is slower and will hide problems
> > > from code analysis tools. Coverity, fortify and other analyzers
> > > special case memcpy().
> > >
> > > Gcc (and Clang) are smart enough to inline copies which
> > > will be faster.
> > >
> >
> > Are you suggesting rte_memcpy() calls aren't inlined?
>
> With a simple made up example of copying an IPv6 address.
>
> rte_memcpy() inlines to:
> rte_copy_addr:
> movdqu xmm0, XMMWORD PTR [rsi]
> movups XMMWORD PTR [rdi], xmm0
> movdqu xmm0, XMMWORD PTR [rsi]
> movups XMMWORD PTR [rdi], xmm0
> ret
>
> Vs memcpy becomes.
>
> copy_addr:
> movdqu xmm0, XMMWORD PTR [rsi]
> movups XMMWORD PTR [rdi], xmm0
> ret
>
> Looks like a bug in rte_memcpy_generic? Why is it not handling
> 16 bytes correctly.
Looks like you have run into another bunch of off-by-one errors like this one:
https://git.dpdk.org/dpdk/commit/lib/eal/x86/include/rte_memcpy.h?id=2ef17be88e8b26f871cfb0265227341e36f486ea
>
> For me, it was more about getting fortify and compiler warnings to
> catch bugs. For most code, the output should be the same.
new file mode 100644
@@ -0,0 +1,11 @@
+//
+// rte_memcpy should not be used for simple fixed size structure
+// because compiler's are smart enough to inline these.
+//
+@@
+expression src, dst; constant size;
+@@
+(
+- rte_memcpy(dst, src, size)
++ memcpy(dst, src, size)
+)