Clang tidy review crashes when writing back file

[maoliver-amd]

I recently started getting a internal crash when running cpp-linter with tidy+format reviews enabled. For some reason there is a particular filename that it doesnt like.

Here is the excerpt from the github action log:

Get list of specified source files
  INFO:CPP Linter:Fetching files list from url: https://api.github.com/repos/Radeon-Pro/Capsaicin/pulls/892
  INFO:CPP Linter:Giving attention to the following files:
  	src/core/src/capsaicin/capsaicin_internal.cpp
  	src/core/src/capsaicin/capsaicin_internal.h
  	src/core/src/render_techniques/combine/combine.cpp
  	src/core/src/render_techniques/combine/combine.h
  	src/core/src/render_techniques/gi_denoiser/gi_denoiser.cpp
  	src/core/src/render_techniques/gi_denoiser/gi_denoiser.h
  	src/core/src/render_techniques/restir_gi/restir_gi.cpp
  	src/core/src/renderers/gi20/gi20_renderer.cpp
INFO:CPP Linter:`D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\Scripts\clang-format.EXE --version`: 18.1.8
INFO:CPP Linter:clang-format Ignoring the following paths/files/patterns:
	./.github
INFO:CPP Linter:`D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\Scripts\clang-tidy.EXE --version`: 18.1.8
INFO:CPP Linter:clang-tidy Ignoring the following paths/files/patterns:
	./.github
concurrent.futures.process._RemoteTraceback: 
"""
Traceback (most recent call last):
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\concurrent\futures\process.py", line 246, in _process_worker
    r = call_item.fn(*call_item.args, **call_item.kwargs)
  File "D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\lib\site-packages\cpp_linter\clang_tools\__init__.py", line 53, in _run_on_single_file
    tidy_note = run_clang_tidy(
  File "D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\lib\site-packages\cpp_linter\clang_tools\clang_tidy.py", line 266, in run_clang_tidy
    Path(file_obj.name).write_bytes(original_buf)
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\pathlib.py", line 1143, in write_bytes
    with self.open(mode='wb') as f:
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\pathlib.py", line 1119, in open
    return self._accessor.open(self, mode, buffering, encoding, errors,
OSError: [Errno 22] Invalid argument: 'src\\core\\src\\capsaicin\\capsaicin_internal.h'
"""
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\Scripts\cpp-linter.exe\__main__.py", line 7, in <module>
  File "D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\lib\site-packages\cpp_linter\__init__.py", line 78, in main
    clang_versions = capture_clang_tools_output(files=files, args=args)
  File "D:\a\_actions\cpp-linter\cpp-linter-action\v2\venv\lib\site-packages\cpp_linter\clang_tools\__init__.py", line 163, in capture_clang_tools_output
    file_name, logs, tidy_advice, format_advice = future.result()
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\concurrent\futures\_base.py", line 451, in result
    return self.__get_result()
  File "C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\concurrent\futures\_base.py", line 403, in __get_result
    raise self._exception
OSError: [Errno 22] Invalid argument: 'src\\core\\src\\capsaicin\\capsaicin_internal.h'
Error: Process completed with exit code 1.

It doesnt like src\\core\\src\\capsaicin\\capsaicin_internal.h whereas src\\core\\src\\capsaicin\\capsaicin_internal.cpp is perfectly fine (along with every other file in the repo). Its not a privileges issue and there arent any special characters that could be an issue so im not sure why that exact filename causes problems.

Ive so far worked around it by adding a try/except around lines 263->268 in clang_tidy.py and ignoring the error. I then changed the order that format and tidy are run in __init__.py so that format is run first (if format has already run then does it even need to write back the original file contents?)

[2bndy5]

1. Are you sure it shouldn't be src/ instead of src/core/src/? I don't know the repo structure.
2. Are you passing clang-tidy a compilation database?
3. Is your CI otherwise altering src files while cpp-linter is running?

[2bndy5]

cpp-linter/cpp_linter/clang_tools/clang_tidy.py

Lines 262 to 266 in baee3c2

	if tidy_review:
	# store the modified output from clang-tidy
	advice.patched = Path(file_obj.name).read_bytes()
	# re-write original file contents
	Path(file_obj.name).write_bytes(original_buf)

Could probably be written to use the same Path obj (for better garbage collection):

    if tidy_review: 
        # store the modified output from clang-tidy
        temp = Path(file_obj.name)
        advice.patched = temp.read_bytes() 
        # re-write original file contents 
        temp.write_bytes(original_buf) 

[maoliver-amd]

1. Are you sure it shouldn't be src/ instead of src/core/src/? I don't know the repo structure.

Paths are 100% correct, multiple other files in the repo all work fine just for some reason that one causes a failure. It makes no sense to me as the read on the previous line works fine

2. Are you passing clang-tidy a compilation database?

Yes

3. Is your CI otherwise altering src files while cpp-linter is running?

No, just running the cpp-linter action

[maoliver-amd]

temp = Path(file_obj.name)

Gave this a try and still get the same error (was worth a try though)

[2bndy5]

Does it work better with the posix style path?

cpp-linter/cpp_linter/clang_tools/clang_tidy.py

Line 225 in baee3c2

filename = file_obj.name.replace("/", os.sep)

temp = Path(filename)

I just don't understand how it would work to read the binary contents and then fail to write those bytes to the same file. Very strange. You can also turn on debugging logs with --verbosity=debug.

[maoliver-amd]

Does it work better with the posix style path?

Tried that aswell, this is the code im currently using that atleast traps the exception and allows cpp-linter to continue (doesnt seem to have any issue with continuing as I also changed format to run before lint so I dont believe the output file is ever used again)

    if tidy_review:
        try:
            # store the modified output from clang-tidy
            temp = Path(filename)
            advice.patched = temp.read_bytes()
            # re-write original file contents
            temp.write_bytes(original_buf)
        except OSError:
            logger.error("Failed to restore original file contents: %s", filename)

Setting verbosity to debug doesnt show any additional information beyond the log I intially posted

[2bndy5]

I'm more inclined to lock the file and do it all at once before closing the file.

timeout = time.monotonic_ns() + 1000000
# wait 1s for file to become accessible
while time.monotonic_ns() < timeout:
    try:
        src = open(filename, "r+b")

        # wait 1s for file to become readable
        read_timeout = time.monotonic_ns() + 1000000
        while not src.readable() and time.monotonic_ns() < read_timeout:
            pass
        advice.patched = b"".join(src.readlines())

        src.seek(os.SEEK_SET)  # back to start of file

        # wait 1s for file to become writable
        write_timeout = time.monotonic_ns() + 1000000
        while not src.writeable() and time.monotonic_ns() < write_timeout:
            pass
        src.writelines([original_buf])
        src.close()
        break
    except OSError as exc:
        if time.monotonic_ns() < timeout:
            pass
        else:
            raise from exc

I think the real problem is that other instances of clang-tidy are concurrently opening the file (probably as an #included source). This is a problem rather specific to Windows.

[maoliver-amd]

I think the real problem is that other instances of clang-tidy are concurrently opening the file (probably as an #included source). This is a problem rather specific to Windows.

I think that is correct, the other files being run include the file thats crashing. Maybe clang-tidy should copy the input file to a temp one before running so that the input source is never actually modified

[2bndy5]

I had considered the temp file idea... I don't think its too feasible because of cyclical or relative #include paths.

[maoliver-amd]

I'm more inclined to lock the file and do it all at once before closing the file.

timeout = time.monotonic_ns() + 1000000
# wait 1s for file to become accessible
while time.monotonic_ns() < timeout:
    try:
        src = open(filename, "r+b")

        # wait 1s for file to become readable
        read_timeout = time.monotonic_ns() + 1000000
        while not src.readable() and time.monotonic_ns() < read_timeout:
            pass
        advice.patched = b"".join(src.readlines())

        src.seek(os.SEEK_SET)  # back to start of file

        # wait 1s for file to become writable
        write_timeout = time.monotonic_ns() + 1000000
        while not src.writeable() and time.monotonic_ns() < write_timeout:
            pass
        src.writelines([original_buf])
        src.close()
        break
    except OSError as exc:
        if time.monotonic_ns() < timeout:
            pass
        else:
            raise from exc

Tested and this seems to fix the issue. I created a PR with the tested changes (with a few typo fixes from the original) #130