Add type checking to format strings used in the client library logging routines and fix the errors that it triggers #2380
Conversation
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
ae9b35c to
e6f75e2
Compare
bf0b7bc to
d54785d
Compare
3db52b7 to
ae68ace
Compare
ae68ace to
dd5f2e2
Compare
Contributor
Author
|
The fourth commit (related to changes in the devel branch) was split out to a separate PR #2412. |
amadio
reviewed
Jan 24, 2025
dd5f2e2 to
ec098cb
Compare
Member
|
I would be fine with it but when you look at the documentation many times it does not list %z making you wonder whether or not it's supported. Given that you really don't need to use %z since the replacement is a trivial type cast,; we can avoid all of this uncertainty here by using that approach. |
The format for some size_t variables were recently changed from %d to %llu. This fixed a problem on 64 bit architectures, since there size_t is 64 bits wide and %d is the format for a 32 bit type and %llu is the format for a 64 bit type. However, this change introduced a regression on 32 bit architectures where size_t is 32 bits wide. This change caused some of the tests to fail with a segmentation fault on the Debian armhf architecture: The following tests FAILED: 108 - XRootD::noauth::test (Failed) 111 - XRootD::host::test (Failed) 114 - XRootD::unix::test (Failed) 117 - XRootD::sss::test (Failed) 120 - XRootD::http::test (Failed) This commit changes these format strings to use %zu for size_t values.
so that the compiler can do type checking for format strings
ec098cb to
acab969
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
The format for some size_t variables were recently changed from %d to %llu. This fixed a problem on 64 bit architectures, since there size_t is 64 bits wide and %d is the format for a 32 bit type and %llu is the format for a 64 bit type. However, this change introduced a regression on 32 bit architectures where size_t is 32 bits wide.
This change caused some of the tests to fail with a segmentation fault on the Debian armhf architecture:
The following tests FAILED:
108 - XRootD::noauth::test (Failed)
111 - XRootD::host::test (Failed)
114 - XRootD::unix::test (Failed)
117 - XRootD::sss::test (Failed)
120 - XRootD::http::test (Failed)
This first commit adds a type cast so that the new format works for all architectures.
It is possible to get warnings from the compilers for bad format strings that don't match the types of the variables by using atribute((format)). The second commit adds this attribute to the logging routines in the client library.
The third commit fixes the errors from this type checking produced by the compiler for the master branch.
The fourth commit contains additional fixes for the devel branch relared to the "timeout uses time_t" change. This commit should no go to mastar untill that change does.
The first three commits should go to master.