| Message ID | 4f161041-b299-f79a-e01b-cc00e2880850@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY | expand |
René Scharfe <l.s.r@web.de> writes: > Use __builtin_types_compatible_p to perform a full type check if > possible. Otherwise fall back to the old size comparison, but add a > non-evaluated assignment to catch more type mismatches. It doesn't flag > copies between arrays with different signedness, but that's as close to > a full type check as it gets without the builtin, as far as I can see. This seems to unfortunately break builds for compat/mingw.c cf. https://github.com/git/git/actions/runs/3865788736/jobs/6589504628#step:4:374 1848 | COPY_ARRAY(&argv2[1], &argv[1], argc); where the two arrays are "char *const *argv" in the parameter list, and a local variable #ifndef _MSC_VER const #endif char **argv2; It seems that (const char **) and (char **) are compatible but the pointers themselves being const breaks the type compatibility? Perhaps the latter should be "(optionally const) char *const *argv2"?
Am 08.01.2023 um 08:28 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Use __builtin_types_compatible_p to perform a full type check if >> possible. Otherwise fall back to the old size comparison, but add a >> non-evaluated assignment to catch more type mismatches. It doesn't flag >> copies between arrays with different signedness, but that's as close to >> a full type check as it gets without the builtin, as far as I can see. > > This seems to unfortunately break builds for compat/mingw.c > > cf. https://github.com/git/git/actions/runs/3865788736/jobs/6589504628#step:4:374 > > 1848 | COPY_ARRAY(&argv2[1], &argv[1], argc); > > where the two arrays are "char *const *argv" in the parameter list, and > a local variable > > #ifndef _MSC_VER > const > #endif > char **argv2; > > It seems that (const char **) and (char **) are compatible but the > pointers themselves being const breaks the type compatibility? > Perhaps the latter should be "(optionally const) char *const *argv2"? We compare the types of the elements, so effectively we do this: __builtin_types_compatible_p(__typeof__(const char *), __typeof__(char *)) ... which returns 0. We can remove the const like we already do for Visual Studio. But then we have to add two casts when passing on argv2, like in mingw_execv(), because adding a const to a pointer of a pointer must be done explicitly in C (even though Visual Studio seems to do it implicitly without complaining). Feels a bit silly. :-| --- >8 --- Subject: [PATCH 1.5/4] mingw: make argv2 in try_shell_exec() non-const Prepare for a stricter type check in COPY_ARRAY by removing the const qualifier of argv2, like we already do to placate Visual Studio. We have to add it back using explicit casts when actually using the variable, unfortunately, because GCC (rightly) refuses to add it implicitly. Similar casts are already used in mingw_execv(). Signed-off-by: René Scharfe <l.s.r@web.de> --- compat/mingw.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index d614f156df..e131eb9b07 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1839,16 +1839,13 @@ static int try_shell_exec(const char *cmd, char *const *argv) if (prog) { int exec_id; int argc = 0; -#ifndef _MSC_VER - const -#endif char **argv2; while (argv[argc]) argc++; ALLOC_ARRAY(argv2, argc + 1); argv2[0] = (char *)cmd; /* full path to the script file */ COPY_ARRAY(&argv2[1], &argv[1], argc); - exec_id = trace2_exec(prog, argv2); - pid = mingw_spawnv(prog, argv2, 1); + exec_id = trace2_exec(prog, (const char **)argv2); + pid = mingw_spawnv(prog, (const char **)argv2, 1); if (pid >= 0) { int status; if (waitpid(pid, &status, 0) < 0) -- 2.38.1.windows.1
René Scharfe <l.s.r@web.de> writes: > We compare the types of the elements, so effectively we do this: > > __builtin_types_compatible_p(__typeof__(const char *), __typeof__(char *)) > > ... which returns 0. True. I wonder if (const const char *) and (const char *) are deemed compatible? Even if so, probably we cannot write __builtin_types_compatible_p(const __typeof__(*(dst)), const __typeof__(*(src))) so that line of thoguht would lead nowhere X-<. > We can remove the const like we already do for Visual Studio. But > then we have to add two casts when passing on argv2, like in > mingw_execv(), because adding a const to a pointer of a pointer > must be done explicitly in C (even though Visual Studio seems to > do it implicitly without complaining). Feels a bit silly. :-| Indeed. Let's see what folks, whom "git blame" tells us to be area experts around here, think. The "if _MSC, add const" was added in 12fb9bd8 (msvc: mark a variable as non-const, 2019-06-19) by JeffH, and try_shell_exec() function itself came from f1a4dfb8 (Windows: Wrap execve so that shell scripts can be invoked., 2007-12-04), added by J6t. > --- >8 --- > Subject: [PATCH 1.5/4] mingw: make argv2 in try_shell_exec() non-const > > Prepare for a stricter type check in COPY_ARRAY by removing the const > qualifier of argv2, like we already do to placate Visual Studio. We > have to add it back using explicit casts when actually using the > variable, unfortunately, because GCC (rightly) refuses to add it > implicitly. Similar casts are already used in mingw_execv(). > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > compat/mingw.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index d614f156df..e131eb9b07 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1839,16 +1839,13 @@ static int try_shell_exec(const char *cmd, char *const *argv) > if (prog) { > int exec_id; > int argc = 0; > -#ifndef _MSC_VER > - const > -#endif > char **argv2; > while (argv[argc]) argc++; > ALLOC_ARRAY(argv2, argc + 1); > argv2[0] = (char *)cmd; /* full path to the script file */ > COPY_ARRAY(&argv2[1], &argv[1], argc); > - exec_id = trace2_exec(prog, argv2); > - pid = mingw_spawnv(prog, argv2, 1); > + exec_id = trace2_exec(prog, (const char **)argv2); > + pid = mingw_spawnv(prog, (const char **)argv2, 1); > if (pid >= 0) { > int status; > if (waitpid(pid, &status, 0) < 0) > -- > 2.38.1.windows.1
On 1/8/23 11:27 PM, Junio C Hamano wrote: > René Scharfe <l.s.r@web.de> writes: > >> We compare the types of the elements, so effectively we do this: >> >> __builtin_types_compatible_p(__typeof__(const char *), __typeof__(char *)) >> >> ... which returns 0. > > True. I wonder if (const const char *) and (const char *) are > deemed compatible? Even if so, probably we cannot write > > __builtin_types_compatible_p(const __typeof__(*(dst)), > const __typeof__(*(src))) > > so that line of thoguht would lead nowhere X-<. > >> We can remove the const like we already do for Visual Studio. But >> then we have to add two casts when passing on argv2, like in >> mingw_execv(), because adding a const to a pointer of a pointer >> must be done explicitly in C (even though Visual Studio seems to >> do it implicitly without complaining). Feels a bit silly. :-| > > Indeed. Let's see what folks, whom "git blame" tells us to be area > experts around here, think. The "if _MSC, add const" was added in > 12fb9bd8 (msvc: mark a variable as non-const, 2019-06-19) by JeffH, > and try_shell_exec() function itself came from f1a4dfb8 (Windows: > Wrap execve so that shell scripts can be invoked., 2007-12-04), > added by J6t. I added the ifNdef. The existing code already had the const that MSVC didn't like. I don't remember why I didn't just remove the const completely. If I had to guess I'd say that dropping probably caused a small cascade of caller/callee type changes and that would have distracted from the focus of my patch series at the time. Personally, I think we should just remove the const from all versions. Thanks, Jeff ... >> int exec_id; >> int argc = 0; >> -#ifndef _MSC_VER >> - const >> -#endif >> char **argv2; >> while (argv[argc]) argc++; >> ALLOC_ARRAY(argv2, argc + 1); >> argv2[0] = (char *)cmd; /* full path to the script file */ >> COPY_ARRAY(&argv2[1], &argv[1], argc); >> - exec_id = trace2_exec(prog, argv2); >> - pid = mingw_spawnv(prog, argv2, 1); >> + exec_id = trace2_exec(prog, (const char **)argv2); >> + pid = mingw_spawnv(prog, (const char **)argv2, 1); >> if (pid >= 0) { >> int status; >> if (waitpid(pid, &status, 0) < 0) >> -- >> 2.38.1.windows.1
diff --git a/git-compat-util.h b/git-compat-util.h index 940d03150c..e81bb14fc9 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -97,8 +97,14 @@ struct strbuf; # define BARF_UNLESS_AN_ARRAY(arr) \ BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \ __typeof__(&(arr)[0]))) +# define BARF_UNLESS_COPYABLE(dst, src) \ + BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \ + __typeof__(*(src)))) #else # define BARF_UNLESS_AN_ARRAY(arr) 0 +# define BARF_UNLESS_COPYABLE(dst, src) \ + BUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \ + sizeof(*(dst)) == sizeof(*(src))) #endif /* * ARRAY_SIZE - get the number of elements in a visible array @@ -1093,9 +1099,6 @@ int xstrncmpz(const char *s, const char *t, size_t len); #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x))) #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc))) -#define BARF_UNLESS_COPYABLE(dst, src) \ - BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))) - #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \ BARF_UNLESS_COPYABLE((dst), (src))) static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
Use __builtin_types_compatible_p to perform a full type check if possible. Otherwise fall back to the old size comparison, but add a non-evaluated assignment to catch more type mismatches. It doesn't flag copies between arrays with different signedness, but that's as close to a full type check as it gets without the builtin, as far as I can see. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- git-compat-util.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.39.0