| Message ID | cd50ec73ddafaaeba04298ae79cbf625cc0d7697.1651859773.git.git@grubix.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | quell a few gcc warnings | expand |
Michael J Gruber <git@grubix.eu> writes: > Related to -Wstringop-overread. > > In fact, this may be a false positive, but reading until the correct end > is desirable here anyways. But the correct end is start + (end - start), not start + (end - start + 1), isn't it? We've stripped trailing junk like /.git and end is point at one byte beyond the end of URL to the repository. E.g. for "https://auth@host/", we have advanced start to point at "h" at the beginning of "host", and we have moved end back from pointing at the NUL at the end to point at "/" at the end of "host/". We are trying to make sure that the resulting "host" string between start and end do not have a slash to apply this special case. If the original URL were "https://auth@host:4321/", the end points at "/" at the end of "host:4321/", making the string to be checked to "host:4321" and we are trying to see it has no '/' in it (which is the case). By extending the string by one, memchr() will see the '/' at the end that is outside. This seems to be a behaviour breaking change and I am not sure what we are trying to achieve with it. Is this a suggestion made by a broken compiler you have, or something? Puzzled.... > Signed-off-by: Michael J Gruber <git@grubix.eu> > --- > dir.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/dir.c b/dir.c > index 26c4d141ab..32fcaae4c0 100644 > --- a/dir.c > +++ b/dir.c > @@ -3145,7 +3145,7 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare) > * result in a dir '2222' being guessed due to backwards > * compatibility. > */ > - if (memchr(start, '/', end - start) == NULL > + if (memchr(start, '/', end - start + 1) == NULL > && memchr(start, ':', end - start) != NULL) { > ptr = end; > while (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')
On Fri, May 06, 2022 at 08:04:05PM +0200, Michael J Gruber wrote: > Related to -Wstringop-overread. > > In fact, this may be a false positive Indeed it seems more like a bug[1] in gcc12, probably with their optimizer. Getting to the bottom of it with a minimized version of the code that would trigger it would be a good way to help it move forward instead of "fixing" git's codebase IMHO. It would be also nice if someone from the gcc team would confirm or deny if this is indeed something worth waiting for a fix on their side, or would need some workaround in ours, or maybe even a real fix. FWIW there is already a workaround of sorts in our codebase since 846a29afb0 (config.mak.dev: workaround gcc 12 bug affecting "pedantic" CI job, 2022-04-15) so that this warning should be expected when building with DEVELOPER=1 but it won't break the build as it would normally do. Carlo [1] https://bugzilla.redhat.com/show_bug.cgi?id=2075786
On Fri, May 06, 2022 at 01:21:33PM -0700, Junio C Hamano wrote: > Michael J Gruber <git@grubix.eu> writes: > > > Related to -Wstringop-overread. > > > > In fact, this may be a false positive, but reading until the correct end > > is desirable here anyways. > > But the correct end is start + (end - start), not start + (end - > start + 1), isn't it? We've stripped trailing junk like /.git and > end is point at one byte beyond the end of URL to the repository. > > E.g. for "https://auth@host/", we have advanced start to point at > "h" at the beginning of "host", and we have moved end back from > pointing at the NUL at the end to point at "/" at the end of > "host/". > > We are trying to make sure that the resulting "host" string between > start and end do not have a slash to apply this special case. > > If the original URL were "https://auth@host:4321/", the end points > at "/" at the end of "host:4321/", making the string to be checked > to "host:4321" and we are trying to see it has no '/' in it (which > is the case). By extending the string by one, memchr() will see the > '/' at the end that is outside. > > This seems to be a behaviour breaking change and I am not sure what > we are trying to achieve with it. Is this a suggestion made by a > broken compiler you have, or something? I agree with this reasoning; the change here does not seem correct to me, and the original version looks to be doing what it advertises. Thanks, Taylor
diff --git a/dir.c b/dir.c index 26c4d141ab..32fcaae4c0 100644 --- a/dir.c +++ b/dir.c @@ -3145,7 +3145,7 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare) * result in a dir '2222' being guessed due to backwards * compatibility. */ - if (memchr(start, '/', end - start) == NULL + if (memchr(start, '/', end - start + 1) == NULL && memchr(start, ':', end - start) != NULL) { ptr = end; while (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')
Related to -Wstringop-overread. In fact, this may be a false positive, but reading until the correct end is desirable here anyways. Signed-off-by: Michael J Gruber <git@grubix.eu> --- dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)