alex/esp-idf
alex/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2024-10-05 20:47:46 -04:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

fix(ci): Simplify and document public header checker

Browse Source 
Documented specific checks/subchecks for header file verification
Simplified the process, now we use simple regex to remove macros from
the current header. Before, we re-ran preprocess_one_header()
function with removed `#include ...`s from the header under test, so
we were looking into the actual header (rather than included headers)
when checking for `extern "C"` keyword.
The procedure is easier to follow without this recursion (mostly because
in the second execution we might encounter compilation failers and
ignore them).
Note that this procedure is not 100% correct (we might see some false
positive and false negatives)
This commit is contained in:
David Cermak 2024-03-06 17:28:46 +01:00
parent bcd753994b
commit 4d40751158
2 changed files with 96 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

171
tools/ci/check_public_headers.py
View File

@ -23,7 +23,7 @@ from typing import Union
class HeaderFailed(Exception):
"""Base header failure exeption"""
"""Base header failure exception"""
pass
@ -80,15 +80,6 @@ def exec_cmd(what: List, out_file: Union['tempfile._TemporaryFileWrapper[bytes]'
class PublicHeaderChecker:
# Intermediate results
COMPILE_ERR_REF_CONFIG_HDR_FAILED = 1 # -> Cannot compile and failed with injected SDKCONFIG #error (header FAILs)
COMPILE_ERR_ERROR_MACRO_HDR_OK = 2 # -> Cannot compile, but failed with "#error" directive (header seems OK)
COMPILE_ERR_HDR_FAILED = 3 # -> Cannot compile with another issue, logged if verbose (header FAILs)
PREPROC_OUT_ZERO_HDR_OK = 4 # -> Both preprocessors produce zero out (header file is OK)
PREPROC_OUT_SAME_HRD_FAILED = 5 # -> Both preprocessors produce the same, non-zero output (header file FAILs)
PREPROC_OUT_DIFFERENT_WITH_EXT_C_HDR_OK = 6 # -> Both preprocessors produce different, non-zero output with extern "C" (header seems OK)
PREPROC_OUT_DIFFERENT_NO_EXT_C_HDR_FAILED = 7 # -> Both preprocessors produce different, non-zero output without extern "C" (header fails)
HEADER_CONTAINS_STATIC_ASSERT = 8 # -> Header file contains _Static_assert instead of static_assert or ESP_STATIC_ASSERT
def log(self, message: str, debug: bool=False) -> None:
if self.verbose or debug:
@ -109,6 +100,8 @@ class PublicHeaderChecker:
self.auto_soc_header = re.compile(r'components/soc/esp[a-z0-9_]+(?:/\w+)?/include/(soc|modem)/[a-zA-Z0-9_]+.h')
self.assembly_nocode = r'^\s*(\.file|\.text|\.ident|\.option|\.attribute).*$'
self.check_threads: List[Thread] = []
self.stdc = '--std=c99'
self.stdcpp = '--std=c++17'
self.job_queue: queue.Queue = queue.Queue()
self.failed_queue: queue.Queue = queue.Queue()
@ -152,54 +145,17 @@ class PublicHeaderChecker:
while t.is_alive() and not self.terminate.is_set():
t.join(1) # joins with timeout to respond to keyboard interrupt
# Checks one header calling:
# - preprocess_one_header() to test and compare preprocessor outputs
# - check_no_code() to test if header contains some executable code
# Procedure
# 1) Preprocess the include file with C preprocessor and with CPP preprocessor
# - Pass the test if the preprocessor outputs are the same and whitespaces only (#define only header)
# - Fail the test if the preprocessor outputs are the same (but with some code)
# - If outputs different, continue with 2)
# 2) Strip out all include directives to generate "temp.h"
# 3) Preprocess the temp.h the same way in (1)
# - Pass the test if the preprocessor outputs are the same and whitespaces only (#include only header)
# - Fail the test if the preprocessor outputs are the same (but with some code)
# - If outputs different, pass the test
# 4) If header passed the steps 1) and 3) test that it produced zero assembly code
# Checks one header procedure:
# - Preprocess the include file with C preprocessor and with CPP preprocessor, compare check for possible issues
# - Compile the header with both C and C++ compiler
def check_one_header(self, header: str, num: int) -> None:
res = self.preprocess_one_header(header, num)
if res == self.COMPILE_ERR_REF_CONFIG_HDR_FAILED:
raise HeaderFailedSdkconfig()
elif res == self.COMPILE_ERR_ERROR_MACRO_HDR_OK:
return self.compile_one_header(header)
elif res == self.COMPILE_ERR_HDR_FAILED:
raise HeaderFailedPreprocessError()
elif res == self.PREPROC_OUT_ZERO_HDR_OK:
return self.compile_one_header(header)
elif res == self.PREPROC_OUT_SAME_HRD_FAILED:
raise HeaderFailedCppGuardMissing()
elif res == self.HEADER_CONTAINS_STATIC_ASSERT:
raise HeaderFailedContainsStaticAssert()
else:
self.compile_one_header(header)
temp_header = None
try:
_, _, _, temp_header, _ = exec_cmd_to_temp_file(['sed', '/#include/d; /#error/d', header], suffix='.h')
res = self.preprocess_one_header(temp_header, num, ignore_common_issues=True)
if res == self.PREPROC_OUT_SAME_HRD_FAILED:
raise HeaderFailedCppGuardMissing()
elif res == self.PREPROC_OUT_DIFFERENT_NO_EXT_C_HDR_FAILED:
raise HeaderFailedCppGuardMissing()
finally:
if temp_header:
os.unlink(temp_header)
self.preprocess_one_header(header, num)
self.compile_one_header_with(self.gcc, self.stdc, header)
self.compile_one_header_with(self.gpp, self.stdcpp, header)
def compile_one_header(self, header: str) -> None:
self.compile_one_header_with(self.gcc, header)
self.compile_one_header_with(self.gpp, header)
def compile_one_header_with(self, compiler: str, header: str) -> None:
rc, out, err, cmd = exec_cmd([compiler, '-S', '-o-', '-include', header, self.main_c] + self.include_dir_flags)
# Checks if the header contains some assembly code and whether it is compilable
def compile_one_header_with(self, compiler: str, std_flags: str, header: str) -> None:
rc, out, err, cmd = exec_cmd([compiler, std_flags, '-S', '-o-', '-include', header, self.main_c] + self.include_dir_flags)
if rc == 0:
if not re.sub(self.assembly_nocode, '', out, flags=re.M).isspace():
raise HeaderFailedContainsCode()
@ -209,58 +165,96 @@ class PublicHeaderChecker:
self.log('\nCompilation command failed:\n{}\n'.format(cmd), True)
raise HeaderFailedBuildError(compiler)
def preprocess_one_header(self, header: str, num: int, ignore_common_issues: bool=False) -> int:
# Checks one header using preprocessing and parsing
# 1) Remove comments and check
# - if we have some `CONFIG_...` macros (for test 2)
# - static asserts
# 2) Preprocess with C++ flags and test orphan Kconfig macros, or compiler error
# 3) Preprocess with C flags and test for compiler errors
# 4) Compare outputs from steps 2) and 3)
# - outputs are the same, but contain only whitespaces -> header is OK (contains only preprocessor directives)
# - outputs are the same, but contain some code -> FAIL the test, we're missing `extern "C"` somewhere
# - outputs are different:
# - check for `extern "C"` in the diff
# - Not present? -> FAIL the test, we're missing `extern "C"` somewhere
# - Present? -- extern "C" is there, but could be from included headers
# - check for `extern "C"` in the orig header (output of step 1)
# - Present? -> header is OK (we're have the `extern "C"` in the header under test)
# - Not present? -- we're missing `extern "C"` in our header, but do we really need it?
# - Remove all directives and harmless macro invocations from our current header
# - We still have some code? -> FAIL the test (our header needs extern "C")
# - Only whitespaces -> header is OK (it contains only macros and directives)
def preprocess_one_header(self, header: str, num: int) -> None:
all_compilation_flags = ['-w', '-P', '-E', '-DESP_PLATFORM', '-include', header, self.main_c] + self.include_dir_flags
# just strip comments to check for CONFIG_... macros or static asserts
rc, out, err, _ = exec_cmd([self.gcc, '-fpreprocessed', '-dD', '-P', '-E', header] + self.include_dir_flags)
if not ignore_common_issues: # We ignore issues on sdkconfig and static asserts, as we're looking at "preprocessed output"
if re.search(self.kconfig_macro, out):
# enable defined #error if sdkconfig.h not included
all_compilation_flags.append('-DIDF_CHECK_SDKCONFIG_INCLUDED')
# If the file contain _Static_assert or static_assert, make sure it does't not define ESP_STATIC_ASSERT and that it
# is not an automatically generated soc header file
grp = re.search(self.static_assert, out)
# Normalize the potential A//B, A/./B, A/../A, from the name
normalized_path = os.path.normpath(header)
if grp and not re.search(self.defines_assert, out) and not re.search(self.auto_soc_header, normalized_path):
self.log('{}: FAILED: contains {}. Please use ESP_STATIC_ASSERT'.format(header, grp.group(1)), True)
return self.HEADER_CONTAINS_STATIC_ASSERT
# we ignore the rc here, as the `-fpreprocessed` flag expects the file to have macros already expanded, so we might get some errors
# here we use it only to remove comments (even if the command returns non-zero code it produces the correct output)
if re.search(self.kconfig_macro, out):
# enable defined #error if sdkconfig.h not included
all_compilation_flags.append('-DIDF_CHECK_SDKCONFIG_INCLUDED')
# If the file contain _Static_assert or static_assert, make sure it doesn't not define ESP_STATIC_ASSERT and that it
# is not an automatically generated soc header file
grp = re.search(self.static_assert, out)
# Normalize the potential A//B, A/./B, A/../A, from the name
normalized_path = os.path.normpath(header)
if grp and not re.search(self.defines_assert, out) and not re.search(self.auto_soc_header, normalized_path):
self.log('{}: FAILED: contains {}. Please use ESP_STATIC_ASSERT'.format(header, grp.group(1)), True)
raise HeaderFailedContainsStaticAssert()
try:
# compile with C++, check for errors, outputs for a temp file
rc, cpp_out, err, cpp_out_file, cmd = exec_cmd_to_temp_file([self.gpp, '--std=c++17'] + all_compilation_flags)
rc, cpp_out, err, cpp_out_file, cmd = exec_cmd_to_temp_file([self.gpp, self.stdcpp] + all_compilation_flags)
if rc != 0:
if re.search(self.error_macro, err):
if re.search(self.error_orphan_kconfig, err):
self.log('{}: CONFIG_VARS_USED_WHILE_SDKCONFIG_NOT_INCLUDED'.format(header), True)
return self.COMPILE_ERR_REF_CONFIG_HDR_FAILED
raise HeaderFailedSdkconfig()
self.log('{}: Error directive failure: OK'.format(header))
return self.COMPILE_ERR_ERROR_MACRO_HDR_OK
return
self.log('{}: FAILED: compilation issue'.format(header), True)
self.log(err, True)
self.log('\nCompilation command failed:\n{}\n'.format(cmd), True)
return self.COMPILE_ERR_HDR_FAILED
raise HeaderFailedPreprocessError()
# compile with C compiler, outputs to another temp file
rc, _, err, c99_out_file, _ = exec_cmd_to_temp_file([self.gcc, '--std=c99'] + all_compilation_flags)
rc, _, err, c_out_file, _ = exec_cmd_to_temp_file([self.gcc, self.stdc] + all_compilation_flags)
if rc != 0:
self.log('{} FAILED should never happen'.format(header))
return self.COMPILE_ERR_HDR_FAILED
self.log('{} FAILED: compilation in C (while C++ compilation passes)'.format(header))
raise HeaderFailedPreprocessError()
# diff the two outputs
rc, diff, err, _ = exec_cmd(['diff', c99_out_file, cpp_out_file])
rc, diff, err, _ = exec_cmd(['diff', c_out_file, cpp_out_file])
if not diff or diff.isspace():
if not cpp_out or cpp_out.isspace():
self.log('{} The same, but empty out - OK'.format(header))
return self.PREPROC_OUT_ZERO_HDR_OK
return
self.log('{} FAILED C and C++ preprocessor output is the same!'.format(header), True)
return self.PREPROC_OUT_SAME_HRD_FAILED
raise HeaderFailedCppGuardMissing()
if re.search(self.extern_c, diff):
self.log('{} extern C present - OK'.format(header))
return self.PREPROC_OUT_DIFFERENT_WITH_EXT_C_HDR_OK
self.log('{} extern C present in the diff'.format(header))
# now check the extern "C" is really in the unprocessed header
if re.search(self.extern_c, out):
self.log('{} extern C present in the actual header, too - OK'.format(header))
return
# at this point we know that the header itself is missing extern-C, so we need to check if it contains an actual *code*
# we remove all preprocessor's directive to check if there's any code besides macros
macros = re.compile(r'(?m)^\s*#(?:.*\\\r?\n)*.*$') # Matches multiline preprocessor directives
without_macros = macros.sub('', out)
if without_macros.isspace():
self.log("{} Header doesn't need extern-C, it's all just macros - OK".format(header))
return
# at this point we know that the header is not only composed of macro definitions, but could just contain some "harmless" macro calls
# let's remove them and check again
macros_calls = r'(.*?)ESP_STATIC_ASSERT[^;]+;' # static assert macro only, we could add more if needed
without_macros = re.sub(macros_calls, '', without_macros, flags=re.DOTALL)
if without_macros.isspace():
self.log("{} Header doesn't need extern-C, it's all macros definitions and calls - OK".format(header))
return
self.log('{} Different but no extern C - FAILED'.format(header), True)
return self.PREPROC_OUT_DIFFERENT_NO_EXT_C_HDR_FAILED
raise HeaderFailedCppGuardMissing()
finally:
os.unlink(cpp_out_file)
try:
os.unlink(c99_out_file)
os.unlink(c_out_file)
except Exception:
pass
@ -277,9 +271,18 @@ class PublicHeaderChecker:
except FileNotFoundError:
pass
subprocess.check_call(['idf.py', '-B', build_dir, f'-DSDKCONFIG={sdkconfig}', 'reconfigure'], cwd=project_dir)
def get_std(json: List, extension: str) -> str:
# compile commands for the files with specified extension, containing C(XX) standard flag
command = [c for c in j if c['file'].endswith('.' + extension) and '-std=' in c['command']][0]
return str([s for s in command['command'].split() if 'std=' in s][0]) # grab the std flag
build_commands_json = os.path.join(build_dir, 'compile_commands.json')
with open(build_commands_json, 'r', encoding='utf-8') as f:
build_command = json.load(f)[0]['command'].split()
j = json.load(f)
self.stdc = get_std(j, 'c')
self.stdcpp = get_std(j, 'cpp')
build_command = j[0]['command'].split()
include_dir_flags = []
include_dirs = []
# process compilation flags (includes and defines)

10
tools/ci/check_public_headers_exceptions.txt
View File

@ -24,6 +24,7 @@ components/esp_rom/include/esp32s2/rom/rsa_pss.h
components/lwip/lwip/src/include/lwip/priv/memp_std.h
components/lwip/include/lwip/sockets.h
components/lwip/lwip/src/include/lwip/prot/nd6.h
components/lwip/lwip/src/include/netif/ppp/
components/spi_flash/include/spi_flash_chip_issi.h
components/spi_flash/include/spi_flash_chip_mxic.h
@ -48,7 +49,7 @@ components/mbedtls/port/dynamic/esp_mbedtls_dynamic_impl.h
components/esp-tls/private_include/
components/protobuf-c/
components/protocomm/proto-c/
components/fatfs/vfs/vfs_fat_internal.h
components/fatfs/src/ffconf.h
@ -77,6 +78,7 @@ components/esp_gdbstub/include/esp_gdbstub.h
components/esp_hw_support/include/esp_memprot.h
components/esp_hw_support/include/esp_private/esp_memprot_internal.h
components/esp_hw_support/include/esp_private/esp_riscv_intr.h
### Here are the files that use CONFIG_XXX values but don't include sdkconfig.h
#
@ -139,3 +141,9 @@ components/espcoredump/include/port/xtensa/esp_core_dump_summary_port.h
components/riscv/include/esp_private/panic_reason.h
components/riscv/include/riscv/interrupt.h
components/riscv/include/riscv/rvruntime-frames.h
# should be private include, but 'private_include' is a subdir of public includes
components/console/private_include/console_private.h
# Missing extern "C"
components/esp_driver_spi/include/esp_private/spi_master_internal.h