| Message ID | f3b9e9be-06ef-3773-a496-da5e479f9d49@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY | expand |
René Scharfe <l.s.r@web.de> writes: > Extend the element size comparison between source and destination with > a full type check using an assignment. It is not actually evaluated and > even optimized out, but compilers check the types before getting to that > point, and report mismatches. It is not actually evaluated and is (even) optimized out, which is a neat and clever trick. The above made me read it twice, though, as I misread on my first read that you are saying that the code is left in the binary but in an unreachable form (i.e. "it is not (evaluated or optimized out"), though. > The stricter check improves safety, as it catches attempts to copy > between different types that happen to have the same size. The size > check is still needed to avoid allowing copies from a array with a > smaller element type to a bigger one, e.g. from a char array to an int > array, which would be allowed by the assignment check alone. Do you mean int *ip; char *cp; (0 ? (*(ip) = *(cp), 0) : 0); /* silently allowed */ (0 ? (*(cp) = *(ip), 0) : 0); /* truncation noticed */ Presumably we cannot catch int *ip; unsinged *up; (0 ? (*(ip) = *(up), 0) : 0); (0 ? (*(up) = *(ip), 0) : 0); with the approach? I think what you ideally want to enforce is that typeof(dst) is exactly typeof(src), or typeof(src) sans constness (i.e. you can populate non-const array from a const template)? > #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \ > + (0 ? (*(dst) = *(src), 0) : 0) + \ > BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))) > static inline void copy_array(void *dst, const void *src, size_t n, size_t size) > { > @@ -1102,6 +1103,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size) > } > > #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \ > + (0 ? (*(dst) = *(src), 0) : 0) + \ > BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))) > static inline void move_array(void *dst, const void *src, size_t n, size_t size) > { > -- > 2.39.0
Junio C Hamano <gitster@pobox.com> writes: > ... I think what you ideally want to enforce is that > typeof(dst) is exactly typeof(src), or typeof(src) sans constness > (i.e. you can populate non-const array from a const template)? IOW, I am wondering if something like this is an improvement. git-compat-util.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git c/git-compat-util.h w/git-compat-util.h index a76d0526f7..be435615f0 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -97,8 +97,13 @@ struct strbuf; # define BARF_UNLESS_AN_ARRAY(arr) \ BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \ __typeof__(&(arr)[0]))) +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \ + (BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \ + __typeof__(dst))) + \ + (0 ? *(dst) = *(src) : 0)) #else # define BARF_UNLESS_AN_ARRAY(arr) 0 +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0)) #endif /* * ARRAY_SIZE - get the number of elements in a visible array
Am 01.01.23 um 04:59 schrieb Junio C Hamano: > Junio C Hamano <gitster@pobox.com> writes: > >> ... I think what you ideally want to enforce is that >> typeof(dst) is exactly typeof(src), or typeof(src) sans constness >> (i.e. you can populate non-const array from a const template)? Yes. Moving the type check shared between COPY_ARRAY and MOVE_ARRAY to a new macro is a good idea. Using compiler extensions when available, as we already do for other purposes, is a good idea as well. I managed to ignore the existing use somehow. > > IOW, I am wondering if something like this is an improvement. > > git-compat-util.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git c/git-compat-util.h w/git-compat-util.h > index a76d0526f7..be435615f0 100644 > --- c/git-compat-util.h > +++ w/git-compat-util.h > @@ -97,8 +97,13 @@ struct strbuf; > # define BARF_UNLESS_AN_ARRAY(arr) \ > BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \ > __typeof__(&(arr)[0]))) > +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \ > + (BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \ > + __typeof__(dst))) + \ > + (0 ? *(dst) = *(src) : 0)) > #else > # define BARF_UNLESS_AN_ARRAY(arr) 0 > +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0)) > #endif > /* > * ARRAY_SIZE - get the number of elements in a visible array
Am 01.01.23 um 08:41 schrieb René Scharfe: > Am 01.01.23 um 04:59 schrieb Junio C Hamano: >> Junio C Hamano <gitster@pobox.com> writes: >> >>> ... I think what you ideally want to enforce is that >>> typeof(dst) is exactly typeof(src), or typeof(src) sans constness >>> (i.e. you can populate non-const array from a const template)? > > Yes. > > Moving the type check shared between COPY_ARRAY and MOVE_ARRAY to a new > macro is a good idea. > > Using compiler extensions when available, as we already do for other > purposes, is a good idea as well. I managed to ignore the existing use > somehow. On second thought, what do we gain by using __builtin_types_compatible_p here? Does it catch cases that the assignment check plus the element size check wouldn't? > >> >> IOW, I am wondering if something like this is an improvement. >> >> git-compat-util.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git c/git-compat-util.h w/git-compat-util.h >> index a76d0526f7..be435615f0 100644 >> --- c/git-compat-util.h >> +++ w/git-compat-util.h >> @@ -97,8 +97,13 @@ struct strbuf; >> # define BARF_UNLESS_AN_ARRAY(arr) \ >> BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \ >> __typeof__(&(arr)[0]))) >> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \ >> + (BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \ >> + __typeof__(dst))) + \ We actually need to compare the types of the elements here, because otherwise we'd disallow copies between arrays (int arr[7]) and pointers (int *p), which would be unnecessarily strict. >> + (0 ? *(dst) = *(src) : 0)) You dropped the ", 0" after the assignment, but we need it to make the type of this expression int instead of whatever type *dst and *src have. >> #else >> # define BARF_UNLESS_AN_ARRAY(arr) 0 >> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0)) >> #endif >> /* >> * ARRAY_SIZE - get the number of elements in a visible array
René Scharfe <l.s.r@web.de> writes: > On second thought, what do we gain by using __builtin_types_compatible_p > here? Does it catch cases that the assignment check plus the element > size check wouldn't? I was reacting to this part of an earlier message in the thread: Presumably we cannot catch int *ip; unsigned *up; (0 ? (*(ip) = *(up), 0) : 0); (0 ? (*(up) = *(ip), 0) : 0); with the approach? i.e. *ip and *up are of the same size. Would the assignment check trigger? > We actually need to compare the types of the elements here, because > otherwise we'd disallow copies between arrays (int arr[7]) and pointers > (int *p), which would be unnecessarily strict. Yup. Thanks for inferring what I meant to give, despite the two typoes (the other one is ", 0").
Am 01.01.23 um 13:11 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> On second thought, what do we gain by using __builtin_types_compatible_p >> here? Does it catch cases that the assignment check plus the element >> size check wouldn't? > > I was reacting to this part of an earlier message in the thread: > > Presumably we cannot catch > > int *ip; > unsigned *up; > > (0 ? (*(ip) = *(up), 0) : 0); > (0 ? (*(up) = *(ip), 0) : 0); > > with the approach? > > i.e. *ip and *up are of the same size. Would the assignment check > trigger? Ah, right, __builtin_types_compatible_p returns 0 in this case and an assignment is silently allowed. René
René Scharfe <l.s.r@web.de> writes: > Ah, right, __builtin_types_compatible_p returns 0 in this case and an > assignment is silently allowed. Thanks. There is another thing I forgot to mention. I think the side that can use __builtin_types_compatible_p() can lose the assignment check (what I wrote had the GCC extension in addition to the assignment check instead), i.e. the assignment check from your original patch can be considered as a fallback position for compilers without the GCC extension.
diff --git a/git-compat-util.h b/git-compat-util.h index 76e4b11131..8d04832988 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1094,6 +1094,7 @@ int xstrncmpz(const char *s, const char *t, size_t len); #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc))) #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \ + (0 ? (*(dst) = *(src), 0) : 0) + \ BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))) static inline void copy_array(void *dst, const void *src, size_t n, size_t size) { @@ -1102,6 +1103,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size) } #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \ + (0 ? (*(dst) = *(src), 0) : 0) + \ BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))) static inline void move_array(void *dst, const void *src, size_t n, size_t size) {
Extend the element size comparison between source and destination with a full type check using an assignment. It is not actually evaluated and even optimized out, but compilers check the types before getting to that point, and report mismatches. The stricter check improves safety, as it catches attempts to copy between different types that happen to have the same size. The size check is still needed to avoid allowing copies from a array with a smaller element type to a bigger one, e.g. from a char array to an int array, which would be allowed by the assignment check alone. Signed-off-by: René Scharfe <l.s.r@web.de> --- git-compat-util.h | 2 ++ 1 file changed, 2 insertions(+) -- 2.39.0