Fix configure.py crash when clang reports no parseable version - #126064
Open
VaggelisGian wants to merge 5 commits into
Open
Fix configure.py crash when clang reports no parseable version#126064VaggelisGian wants to merge 5 commits into
VaggelisGian wants to merge 5 commits into
Conversation
retrieve_clang_version returns None when the clang executable does not
report a parseable version, which happens with wrappers such as ccache
or any tool whose --version banner lacks a usable version token.
disable_clang_offsetof_extension then crashed with AttributeError while
deciding whether to add -Wno-gnu-offsetof-extensions for clang 16 and
17. Non-numeric version strings would crash the same way with
ValueError. Both cases now skip the flag, matching every confirmed
version outside 16 and 17; the existing "current clang installation
version unknown" warning still prints.
Test Plan:
python -m py_compile configure.py
python -m pylint --rcfile=tensorflow/tools/ci_build/pylintrc \
configure.py # rated 10.00/10, same as master
Direct call matrix on disable_clang_offsetof_extension: None, empty
string and 'unknown' return without writing; '16.0.0' and '17.1.8'
write build --copt=-Wno-gnu-offsetof-extensions; '22.1.8', '15.x'
and '18.x' write nothing, byte identical to master bazelrc output.
On master, None reproduced "AttributeError: 'NoneType' object has no
attribute 'split'" from issue 125939.
retrieve_clang_version indexed the first character of whatever the clang executable printed, so a wrapper that exits with nothing on stdout crashed with IndexError. The parsed result is now always handled as a token list: an empty or unrecognized banner reaches the existing unknown-version warning and returns None, and an adjacent or truncated "clang version " marker does the same instead of raising IndexError. Together with the previous commit this makes configure survive any clang whose --version output cannot be parsed. Version-bearing outputs are unchanged. One deliberate difference: banners without a version token no longer print the misleading "not a release version" warning that master produced by accident, because that check now runs over tokens instead of characters. A token-free banner starting with a digit used to be reported as a bogus one digit version; it is now reported as unknown. Test Plan: Monkeypatched run_shell harness over retrieve_clang_version and the retrieve plus disable_clang_offsetof_extension flow on this branch: empty stdout, whitespace only stdout, banner without a version token, digit-initial token-free banner, adjacent "clang version " separators and a truncated marker all return None with the unknown-version warning and no crash; "clang version 22.1.8" returns 22.1.8; "Ubuntu clang version 18.0.0git" returns 18.0.0 with the prerelease warning. On master the empty-output input reproduced "IndexError: string index out of range". pylint --rcfile=tensorflow/tools/ci_build/pylintrc rates configure.py 10.00/10.
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of Clang version detection and parsing in configure.py by handling empty tokens, missing version strings, and non-integer version formats gracefully. The feedback suggests a cleaner approach to handle cases where the 'clang version ' string is not found in the output, recommending consistently returning None with a warning instead of wrapping the unrecognized version in a list.
Comment on lines
+916
to
+923
| if len(curr_version_split) > 1: | ||
| curr_version = curr_version_split[1].split()[0].split('git') | ||
| tokens = curr_version_split[1].split() | ||
| if not tokens: | ||
| print('WARNING: current clang installation version unknown.\n') | ||
| return None | ||
| curr_version = tokens[0].split('git') | ||
| else: | ||
| curr_version = [curr_version] |
Contributor
There was a problem hiding this comment.
Instead of falling back to wrapping the unrecognized version string in a list (curr_version = [curr_version]), we can explicitly handle the case where 'clang version ' is not found in the output. This allows us to print the unknown version warning and return None consistently, which is much cleaner and more robust.
if len(curr_version_split) <= 1:
print('WARNING: current clang installation version unknown.\n')
return None
tokens = curr_version_split[1].split()
if not tokens:
print('WARNING: current clang installation version unknown.\n')
return None
curr_version = tokens[0].split('git')
Author
|
Note on CI: the two failing checks are pre-existing on master and unrelated to this change.
This PR only edits |
Split the no "clang version " marker case into its own early return instead of wrapping the raw banner in a single-element list. Same observable behavior on every input, clearer control flow. Test Plan: Monkeypatched run_shell matrix rerun after the change: empty stdout, whitespace only, banner without token, digit-initial token-free banner, adjacent separators and truncated marker all return None with the unknown-version warning; 'clang version 22.1.8' returns 22.1.8; 'Ubuntu clang version 18.0.0git' returns 18.0.0 with the prerelease warning. python -m py_compile exit 0; pylint with tensorflow/tools/ci_build/pylintrc rates configure.py 10.00/10.
Author
|
Addressed the review feedback in 4e43c59: the missing-marker case now returns None with the unknown-version warning through its own early return instead of wrapping the raw banner in a list. Re-ran the behavior matrix afterwards; all eight input classes return exactly what they did before. |
dmiltr3
self-requested a review
August 25, 2026 15:19
dmiltr3
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #125939
Summary
./configure.pyaborted with a traceback when the clang executable does not report a parseable version, which happens with wrappers such as ccache (the reporter'swhich clangresolves to/usr/lib64/ccache/clang). Two crashes were reachable:retrieve_clang_versionreturnsNoneafter printing "WARNING: current clang installation version unknown.", anddisable_clang_offsetof_extensionthen crashed onint(clang_version.split('.')[0])withAttributeError: 'NoneType' object has no attribute 'split'. Non-numeric version strings crashed the same line withValueError.run_shellreturns an empty string andretrieve_clang_versionitself crashed withIndexError: string index out of rangewhile taking the first character of the output.The fix guards the consumer so unknown or non-numeric versions skip the
-Wno-gnu-offsetof-extensionsflag exactly like every confirmed version outside 16 and 17, and normalizes the parsing inretrieve_clang_versionso empty, whitespace-only, unrecognized, adjacent-marker and truncated banners all flow into the existing unknown-version warning instead of raising.One deliberate behavior change worth flagging: banners without a version token no longer print the misleading "current clang installation is not a release version" warning that master produced by accident (that check used to run over characters of a string instead of tokens), and a token-free banner starting with a digit is now reported as an unknown version instead of as a bogus one-digit version. Version-bearing outputs behave identically to master.
Testing
Direct call matrix on
disable_clang_offsetof_extension:On master,
Nonereproduced the exact error from the issue:Monkeypatched
run_shellharness overretrieve_clang_versionplus the full retrieve-and-consume flow:On master the empty-output input reproduced:
Lint and compile:
No bazel run was needed:
configure.pysits outside the bazel graph (the root BUILD exports it as a plain data file) and every check above exercises the changed functions directly.