RE: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Ankur Dwivedi <adwivedi@marvell.com>]

Hi Thomas,

Please let me know if there is any feedback on this patch.

Regards,
Ankur

>-----Original Message-----
>From: Ankur Dwivedi <adwivedi@marvell.com>
>Sent: Tuesday, March 7, 2023 5:35 PM
>To: dev@dpdk.org
>Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>Ankur Dwivedi <adwivedi@marvell.com>
>Subject: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch
>
>This patch adds a validation in checkpatch tool, to check if a tracepoint is
>present in any new function added in cryptodev, ethdev, eventdev and
>mempool library.
>
>In this patch, the build_map_changes function is copied from check-symbol-
>change.sh to check-tracepoint.sh. The check-tracepoint.sh script uses
>build_map_changes function to create a map of functions.
>In the map, the newly added functions, added in the experimental section are
>identified and their definition are checked for the presence of tracepoint. The
>checkpatch return error if the tracepoint is not present.
>
>For functions for which trace is not needed, they can be added to
>devtools/trace-skiplist.txt file. The above tracepoint check will be skipped for
>them.
>
>Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>---
> devtools/check-tracepoint.sh | 223 +++++++++++++++++++++++++++++++++++
> devtools/checkpatches.sh     |   9 ++
> devtools/trace-skiplist.txt  |   0
> 3 files changed, 232 insertions(+)
> create mode 100755 devtools/check-tracepoint.sh  create mode 100644
>devtools/trace-skiplist.txt
>
>diff --git a/devtools/check-tracepoint.sh b/devtools/check-tracepoint.sh new
>file mode 100755 index 0000000000..88d6b1fd53
>--- /dev/null
>+++ b/devtools/check-tracepoint.sh
>@@ -0,0 +1,223 @@
>+#!/bin/sh
>+# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2018 Neil Horman
>+<nhorman@tuxdriver.com> # Copyright(C) 2023 Marvell.
>+
>+selfdir=$(dirname $(readlink -f $0))
>+
>+# Library for trace check
>+libdir="cryptodev ethdev eventdev mempool"
>+
>+# Functions for which the trace check can be skipped
>+skiplist="$selfdir/trace-skiplist.txt"
>+
>+build_map_changes()
>+{
>+	local fname="$1"
>+	local mapdb="$2"
>+
>+	cat "$fname" | awk '
>+		# Initialize our variables
>+		BEGIN {map="";sym="";ar="";sec=""; in_sec=0; in_map=0}
>+
>+		# Anything that starts with + or -, followed by an a
>+		# and ends in the string .map is the name of our map file
>+		# This may appear multiple times in a patch if multiple
>+		# map files are altered, and all section/symbol names
>+		# appearing between a triggering of this rule and the
>+		# next trigger of this rule are associated with this file
>+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
>+
>+		# The previous rule catches all .map files, anything else
>+		# indicates we left the map chunk.
>+		/[-+] [ab]\// {in_map=0}
>+
>+		# Triggering this rule, which starts a line and ends it
>+		# with a { identifies a versioned section.  The section name is
>+		# the rest of the line with the + and { symbols removed.
>+		# Triggering this rule sets in_sec to 1, which actives the
>+		# symbol rule below
>+		/^.*{/ {
>+			gsub("+", "");
>+			if (in_map == 1) {
>+				sec=$(NF-1); in_sec=1;
>+			}
>+		}
>+
>+		# This rule identifies the end of a section, and disables the
>+		# symbol rule
>+		/.*}/ {in_sec=0}
>+
>+		# This rule matches on a + followed by any characters except a
>:
>+		# (which denotes a global vs local segment), and ends with a ;.
>+		# The semicolon is removed and the symbol is printed with its
>+		# association file name and version section, along with an
>+		# indicator that the symbol is a new addition.  Note this rule
>+		# only works if we have found a version section in the rule
>+		# above (hence the in_sec check) And found a map file (the
>+		# in_map check).  If we are not in a map chunk, do nothing.  If
>+		# we are in a map chunk but not a section chunk, record it as
>+		# unknown.
>+		/^+[^}].*[^:*];/ {gsub(";","");sym=$2;
>+			if (in_map == 1) {
>+				if (in_sec == 1) {
>+					print map " " sym " " sec " add"
>+				} else {
>+					print map " " sym " unknown add"
>+				}
>+			}
>+		}
>+
>+		# This is the same rule as above, but the rule matches on a
>+		# leading - rather than a +, denoting that the symbol is being
>+		# removed.
>+		/^-[^}].*[^:*];/ {gsub(";","");sym=$2;
>+			if (in_map == 1) {
>+				if (in_sec == 1) {
>+					print map " " sym " " sec " del"
>+				} else {
>+					print map " " sym " unknown del"
>+				}
>+			}
>+		}' > "$mapdb"
>+
>+		sort -u "$mapdb" > "$mapdb.2"
>+		mv -f "$mapdb.2" "$mapdb"
>+
>+}
>+
>+find_trace_fn()
>+{
>+	local fname="$1"
>+
>+	cat "$fname" | awk -v symname="$2 *\\\(" '
>+		# Initialize the variables.
>+		# The variable symname provides a pattern to match
>+		# "function_name(", zero or more spaces can be present
>+		# between function_name and (.
>+		BEGIN {state=0; ln_pth=0}
>+
>+		# Matches the function name, set state=1.
>+		$0 ~ symname {state=1}
>+
>+		# If closing parentheses with semicolon ");" is found, then it
>+		# is not the function definition.
>+		/) *;/ {
>+			if (state == 1) {
>+				state=0
>+			}
>+		}
>+
>+		/)/ {
>+			if (state == 1) {
>+				state=2
>+				ln_pth=NR
>+				next
>+			}
>+		}
>+
>+		# If closing parentheses and then opening braces is found in
>+		# adjacent line, then this is the start of function
>+		# definition. Check for tracepoint in the function definition.
>+		# The tracepoint has _trace_ in its name.
>+		/{/ {
>+			if (state == 2) {
>+				if (ln_pth != NR - 1) {
>+					state=0
>+					next
>+				}
>+				while (index($0, "}") != 2) {
>+					if (index($0, "_trace_") != 0) {
>+						exit 0
>+					}
>+					if (getline <= 0) {
>+						break
>+					}
>+				}
>+				exit 1
>+			}
>+		}
>+	'
>+}
>+
>+check_for_tracepoint()
>+{
>+	local patch="$1"
>+	local mapdb="$2"
>+	local skip_sym
>+	local libname
>+	local secname
>+	local mname
>+	local ret=0
>+	local pdir
>+	local libp
>+	local libn
>+	local sym
>+	local ar
>+
>+	while read -r mname symname secname ar; do
>+		pdir=$(echo $mname | awk 'BEGIN {FS="/"};{print $2}')
>+		libname=$(echo $mname | awk 'BEGIN {FS="/"};{print $3}')
>+		skip_sym=0
>+		libp=0
>+
>+		if [ "$pdir" != "lib" ]; then
>+			continue
>+		fi
>+
>+		for libn in $libdir; do
>+			if [ $libn = $libname ]; then
>+				libp=1
>+				break
>+			fi
>+		done
>+
>+		if [ $libp -eq 0 ]; then
>+			continue
>+		fi
>+
>+		for sym in $(cat $skiplist); do
>+			if [ $sym = $symname ]; then
>+				skip_sym=1
>+				break
>+			fi
>+		done
>+
>+		if [ $skip_sym -eq 1 ]; then
>+			continue
>+		fi
>+
>+		if [ "$ar" = "add" ] && [ "$secname" = "EXPERIMENTAL" ]; then
>+			# Check if new API is added with tracepoint
>+			find_trace_fn $patch $symname
>+			if [ $? -eq 1 ]; then
>+				ret=1
>+				echo -n "ERROR: New function $symname is
>added "
>+				echo -n "without a tracepoint. Please add a
>tracepoint "
>+				echo -n "or add the function $symname in "
>+				echo "devtools/trace-skiplist.txt to skip this
>error."
>+			fi
>+		fi
>+	done < "$mapdb"
>+
>+	return $ret
>+}
>+
>+trap clean_and_exit_on_sig EXIT
>+
>+mapfile=`mktemp -t dpdk.mapdb.XXXXXX`
>+patch=$1
>+exit_code=1
>+
>+clean_and_exit_on_sig()
>+{
>+	rm -f "$mapfile"
>+	exit $exit_code
>+}
>+
>+build_map_changes "$patch" "$mapfile"
>+check_for_tracepoint "$patch" "$mapfile"
>+exit_code=$?
>+rm -f "$mapfile"
>+
>+exit $exit_code
>diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh index
>1dee094c7a..471db1d21b 100755
>--- a/devtools/checkpatches.sh
>+++ b/devtools/checkpatches.sh
>@@ -10,6 +10,7 @@
> . $(dirname $(readlink -f $0))/load-devel-config
>
> VALIDATE_NEW_API=$(dirname $(readlink -f $0))/check-symbol-change.sh
>+VALIDATE_TRACEPOINT=$(dirname $(readlink -f $0))/check-tracepoint.sh
>
> # Enable codespell by default. This can be overwritten from a config file.
> # Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to
>a valid path @@ -386,6 +387,14 @@ check () { # <patch-file> <commit>
> 		ret=1
> 	fi
>
>+	! $verbose || printf '\nChecking API additions with tracepoint :\n'
>+	report=$($VALIDATE_TRACEPOINT "$tmpinput")
>+	if [ $? -ne 0 ] ; then
>+		$headline_printed || print_headline "$subject"
>+		printf '%s\n' "$report"
>+		ret=1
>+	fi
>+
> 	if [ "$tmpinput" != "$1" ]; then
> 		rm -f "$tmpinput"
> 		trap - INT
>diff --git a/devtools/trace-skiplist.txt b/devtools/trace-skiplist.txt new file
>mode 100644 index 0000000000..e69de29bb2
>--
>2.25.1