From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 382B045AAA; Fri, 4 Oct 2024 09:32:05 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BF83E4027F; Fri, 4 Oct 2024 09:32:04 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 5419340268 for ; Fri, 4 Oct 2024 09:32:03 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id DDE573E47 for ; Fri, 4 Oct 2024 09:32:02 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id B4ED43DD2; Fri, 4 Oct 2024 09:32:02 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.2 Received: from [192.168.1.86] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 6C2103D5D; Fri, 4 Oct 2024 09:31:59 +0200 (CEST) Message-ID: Date: Fri, 4 Oct 2024 09:31:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v12 1/7] buildtools/chkincs: relax C linkage requirement To: Robin Jarry , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org Cc: Heng Wang , Stephen Hemminger , Tyler Retzlaff , =?UTF-8?Q?Morten_Br=C3=B8rup?= , Jack Bond-Preston , David Marchand , Chengwen Feng References: <20240920062437.738706-2-mattias.ronnblom@ericsson.com> <20240920104754.739033-1-mattias.ronnblom@ericsson.com> <20240920104754.739033-2-mattias.ronnblom@ericsson.com> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >> >> -- >> >> 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 ..." % 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 >