Re: [PATCH v12 1/7] buildtools/chkincs: relax C linkage requirement

["Robin Jarry" <rjarry@redhat.com>]

Mattias Rönnblom, Sep 20, 2024 at 12:47:
> Relax chkincs requirement of all DPDK header files having to contain
> 'extern "C"'.
>
> Instructing a C++ toolchain to use C linkage is only necessarily if the
> header file declares symbols (i.e., functions or global variables).
>
> With this change, chkincs tries to find if any functions or references
> to global variables are declared in the header file, and if not, no C
> linkage is required.
>
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>
> --
>
> PATCH v11:
>  * Detect functions from the Windows POSIX wrappers, to avoid false
>    positives for redundant 'extern "C"'.
> ---

Hi Mattias,

I have some remarks on this script. Please see below:

>  buildtools/chkincs/chkextern.py | 88 +++++++++++++++++++++++++++++++++
>  buildtools/chkincs/meson.build  | 14 +++---
>  2 files changed, 95 insertions(+), 7 deletions(-)
>  create mode 100755 buildtools/chkincs/chkextern.py
>
> diff --git a/buildtools/chkincs/chkextern.py b/buildtools/chkincs/chkextern.py
> new file mode 100755
> index 0000000000..5374ce1c72
> --- /dev/null
> +++ b/buildtools/chkincs/chkextern.py
> @@ -0,0 +1,88 @@
> +#! /usr/bin/env python3
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Ericsson AB
> +
> +import sys
> +import re
> +
> +def strip_cpp(header):
> +    no_cpp = ""
> +    header = header.replace("\\\n", " ")
> +
> +    for line in header.split("\n"):
> +        if re.match(r'^\s*#.*', line) is None and len(line) > 0:
> +            no_cpp += "%s\n" % line
> +
> +    return no_cpp
> +
> +
> +def strip_comments(header):
> +    no_c_comments = re.sub(r'/\*.*?\*/', '', header, flags=re.DOTALL)
> +    no_cxx_comments = re.sub(r'//.*', '', no_c_comments)
> +    return no_cxx_comments
> +
> +
> +def strip(header):
> +    header = strip_comments(header)
> +    header = strip_cpp(header)
> +    return header

None of these three strip* functions are used.

> +
> +
> +def has_extern_c(header):
> +    return header.find('extern "C"') != -1
> +
> +
> +def has_vars(header):
> +    return re.search(r'^extern\s+[a-z0-9_]+\s.*;', header, flags=re.MULTILINE) is not None
> +
> +
> +FUNCTION_RES = [
> +    r'rte_[a-z0-9_]+\(',
> +    r'cmdline_[a-z0-9_]+\(',
> +    r'vt100_[a-z0-9_]+\(',
> +    r'rdline_[a-z0-9_]+\(',
> +    r'cirbuf_[a-z0-9_]+\(',
> +    # Windows UNIX compatibility
> +    r'pthread_[a-z0-9_]+\(',
> +    r'regcomp\(',
> +    r'count_cpu\('
> +]
> +
> +
> +def has_functions(header):
> +    for function_re in FUNCTION_RES:
> +        if re.search(function_re, header) is not None:
> +            return True
> +    return False
> +
> +
> +def has_symbols(header):
> +    return has_functions(header) or has_vars(header)
> +
> +
> +def chk_missing(filename):
> +    header = open(filename).read()

with open(filename) as f:
    header = f.read()

> +    if has_symbols(header) and not has_extern_c(header):
> +        print(filename)
> +
> +
> +def chk_redundant(filename):
> +    header = open(filename).read()

with open(filename) as f:
    header = f.read()

> +    if not has_symbols(header) and has_extern_c(header):
> +        print(filename)

Can we rename these functions to check_missing and check_redundant? 
I don't think the abbreviation is needed here.

> +
> +if len(sys.argv) < 3:
> +    print("%s missing|redundant <header-file> ..." % sys.argv[0])
> +    sys.exit(1)
> +
> +op = sys.argv[1]
> +headers = sys.argv[2:]
> +
> +for header in headers:
> +    if op == 'missing':
> +        chk_missing(header)
> +    elif op == 'redundant':
> +        chk_redundant(header)

I don't see the 'redundant' op being used here.

> +    else:
> +        print("Unknown operation.")
> +        sys.exit(1)

I know it is a simple python script but it would be better to add 
a proper main() function and use argparse to handle errors. E.g.:

def main():
    parser = argparse.ArgumentParser(description=__doc__)
    parser.add_argument("op", choices=("missing", "redundant"))
    parser.add_argument("headers", nargs="+")
    args = parser.parse_args()

    for h in args.headers:
        if op == "missing":
            chk_missing(h)
        elif op == "redundant":
            chk_redundant(h)

if __name__ == "__main__":
    main()


> diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
> index f2dadcae18..762f85efe5 100644
> --- a/buildtools/chkincs/meson.build
> +++ b/buildtools/chkincs/meson.build
> @@ -38,13 +38,13 @@ if not add_languages('cpp', required: false)
>  endif
>  
>  # check for extern C in files, since this is not detected as an error by the compiler
> -grep = find_program('grep', required: false)
> -if grep.found()
> -    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> -            check: false, capture: true).stdout().split()
> -    if errlist != []
> -        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> -    endif
> +chkextern = find_program('chkextern.py')
> +
> +missing_extern_headers = run_command(chkextern, 'missing', dpdk_chkinc_headers,
> +      capture: true, check: true).stdout().split()
> +
> +if missing_extern_headers != []
> +    error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(missing_extern_headers))

Instead of just relying on this script output, it would be better if it 
exited with a non-zero status when something is wrong. That way you 
would not have to capture stdout at all and you could leave meson do the 
work.

>  endif
>  
>  gen_cpp_files = generator(gen_c_file_for_header,
> -- 
> 2.43.0