Make use of counted_by gcc/clang attribute
#938
+47
−21
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
The next commit wants to use utils.h in darray.h. Signed-off-by: Ran Benita <ran@unusedvar.com>
This is kind-of nice self-documentation, and can be utilized for static analysis and dynamic buffer overflow protection (such as `-fsanitize=bounds` and `__builtin_dynamic_object_size` used by `_FORTIFY_SOURCE=3`). Clang<19.1.0 and GCC<=15.2 only support `counted_by` on flexible array members, not pointer fields as used in xkbcommon, therefore we need a configure check that annotating pointer fields is supported, not merely that the attribute exists. Clang (unlike GCC) currently requires that the count field come before the pointer field. They have a flag to `-fexperimental-late-parse-attributes` to enable this, but since it's experimental I don't want to pass it. So we need to reorder some fields. Another needed code adaption is to ensure that the length field is always correctly set *before* the array is accessed. So e.g. the sequence "alloc buffer, assign, set length field" is changed to "alloc buffer, set length field, assign", otherwise the dynamic bounds check during assignment fails. I have verified that it works well with `-fsanitize=undefined` by inserting some out-of-bounds access to `keymap->keys` and seeing that it goes undetected before and detected after. See: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-counted_005fby-variable-attribute https://www.gnu.org/software/libc/manual/html_node/Source-Fortification.html#Fortification-of-function-calls https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html#index-_005f_005fbuiltin_005fdynamic_005fobject_005fsize https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dbounds Signed-off-by: Ran Benita <ran@unusedvar.com>
Member
|
Rebased after merging the independent commits in #939. |
Member
|
@bluetech that seems a very interesting feature. Are there any relevant runtime costs though? |
Member
Author
Good question. There is a performance overhead due to additional bounds checking when In bench/compile-keymap I don't see a measurable difference. I do see a ~3% difference in bench/compose.c. To "pay" for it, I sent #940. For the benchmarking I used Arch Linux's default flags (importantly |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Since I've read about
counted_byI wanted to try it, it seems to work as expected!There are some preparatory commits, and the last commit contains the details.
@wismill I'll leave the decision whether to merge it to you. The main catch is that it requires some care that the len is always valid even during initialization (see commit for details). I see that CI already has a run with
-fsanitize=undefined, so at least covered code should be safe (I fixed what it caught already), but uncovered code might not be.