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

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-10-03 18:47, Robin Jarry wrote:
> 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.
> 

Oops.

I think I'll just remove the strip_cpp() function (not sure why I 
thought that was ever needed). strip_comments() may be somewhat useful 
when looking for the 'extern "C"' string, if for some reason that string 
would be in a comment somewhere.

>> +
>> +
>> +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()
> 

What benefit would that construct be to this program?

>> +    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.
> 

Sure.

(The function name was derived from the script name, which in turn got 
its name inspired by chkincs "tool" (directory) name.)

>> +
>> +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.
> 

It's used a patch further down the set.

>> +    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()
> 
> 

I don't think you need to bring in the usual Python boiler plate. This 
script is not supposed to be invoked directly by a user - it's just an 
extension of the meson build scripts.

>> 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.
> 

Sure, but it would be required to invoke the script for every header 
file in the tree. Not sure I think that would be a net gain.

Thanks for the review.

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