* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop @ 2020-12-14 23:12 Randall S. Becker 2020-12-15 2:43 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Randall S. Becker @ 2020-12-14 23:12 UTC (permalink / raw) To: git, 'Junio C Hamano' 2.30.0-rc0 fails to compile strmap.o, and probably others, since this is in a header file: "git-compat-util.h", line 277: warning(1252): missing return statement at end of non-void function "setitimer" return strmap_remove(&map->map, str, 0); ^ "strmap.h", line 168: error(210): a void function may not return a value return strmap_remove(&set->map, str, 0); ^ "strmap.h", line 252: error(210): a void function may not return a value Aside from inlining bodies, this should not have compiled on any platform: static inline void strset_remove(struct strset *set, const char *str) { return strmap_remove(&set->map, str, 0); } What is really intended here? Sorry, Randall -- Brief whoami: NonStop developer since approximately 211288444200000000 UNIX developer since approximately 421664400 -- In my real life, I talk too much. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-14 23:12 [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Randall S. Becker @ 2020-12-15 2:43 ` Junio C Hamano 2020-12-15 2:52 ` Jeff King 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2020-12-15 2:43 UTC (permalink / raw) To: Randall S. Becker; +Cc: git "Randall S. Becker" <rsbecker@nexbridge.com> writes: > 2.30.0-rc0 fails to compile strmap.o, and probably others, since this is in > a header file: > > "git-compat-util.h", line 277: warning(1252): > missing return statement at end of non-void function "setitimer" Thanks for reporting. I guess nobody without setitimer bothered to test, since 15b52a44 (compat-util: type-check parameters of no-op replacement functions, 2020-08-06) which was a tad before 2.29 was tagged. diff --git c/git-compat-util.h w/git-compat-util.h index 7d509c5022..58cd0761be 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -273,7 +273,8 @@ struct itimerval { #ifdef NO_SETITIMER static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { - ; /* nothing */ + errno = ENOSYS; + return -1; /* not implemented */ } #endif Alternatively we could pretend that the call always succeeds by without touching errno and returning 0. That might be safer, but I dunno which one we want, and I do not have a system affected by the choice. > return strmap_remove(&map->map, str, 0); > ^ > "strmap.h", line 168: error(210): > a void function may not return a value > > return strmap_remove(&set->map, str, 0); > ^ > "strmap.h", line 252: error(210): > a void function may not return a value This is a GNU extension biting us, perhaps? Apparently these came from 4fa1d501 (strmap: add functions facilitating use as a string->int map, 2020-11-05). > Aside from inlining bodies, this should not have compiled on any platform: > > static inline void strset_remove(struct strset *set, const char *str) > { > return strmap_remove(&set->map, str, 0); > } > > What is really intended here? I think we should just drop "return"; a void function should be called in void context without requiring a value, even if that return expects no value. diff --git i/strmap.h w/strmap.h index c4c104411b..1e152d832d 100644 --- i/strmap.h +++ w/strmap.h @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct strintmap *map, const char *str) static inline void strintmap_remove(struct strintmap *map, const char *str) { - return strmap_remove(&map->map, str, 0); + strmap_remove(&map->map, str, 0); } static inline int strintmap_empty(struct strintmap *map) @@ -249,7 +249,7 @@ static inline int strset_contains(struct strset *set, const char *str) static inline void strset_remove(struct strset *set, const char *str) { - return strmap_remove(&set->map, str, 0); + strmap_remove(&set->map, str, 0); } static inline int strset_empty(struct strset *set) > Sorry, > Randall Thanks. ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 2:43 ` Junio C Hamano @ 2020-12-15 2:52 ` Jeff King 2020-12-15 15:55 ` Randall S. Becker 2020-12-15 16:21 ` Elijah Newren 0 siblings, 2 replies; 12+ messages in thread From: Jeff King @ 2020-12-15 2:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: Elijah Newren, Randall S. Becker, git On Mon, Dec 14, 2020 at 06:43:36PM -0800, Junio C Hamano wrote: > diff --git c/git-compat-util.h w/git-compat-util.h > index 7d509c5022..58cd0761be 100644 > --- c/git-compat-util.h > +++ w/git-compat-util.h > @@ -273,7 +273,8 @@ struct itimerval { > > #ifdef NO_SETITIMER > static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { > - ; /* nothing */ > + errno = ENOSYS; > + return -1; /* not implemented */ > } > #endif > > Alternatively we could pretend that the call always succeeds by > without touching errno and returning 0. That might be safer, but I > dunno which one we want, and I do not have a system affected by the > choice. I think this is a sensible choice. Before the conversion to an inline, the code was removed entirely! So anybody checking the return value would have seen an error, and we do not have to worry much about breaking them. For new callers, anybody checking the return value would probably appreciate the warning that support for the function is optional (OTOH, they would probably not find out themselves, but rather when Randall tells them ;) ). It would be nice to have a way to warn them even on platforms that have setitimer(), but I can't think of an easy way. > > Aside from inlining bodies, this should not have compiled on any platform: > > > > static inline void strset_remove(struct strset *set, const char *str) > > { > > return strmap_remove(&set->map, str, 0); > > } > > > > What is really intended here? > > I think we should just drop "return"; a void function should be > called in void context without requiring a value, even if that > return expects no value. Yeah, I think that is right. I checked earlier iterations of the series to see if perhaps strmap_remove() had previously returned a value, but it never did in any on the list. > diff --git i/strmap.h w/strmap.h > index c4c104411b..1e152d832d 100644 > --- i/strmap.h > +++ w/strmap.h > @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct strintmap *map, const char *str) > > static inline void strintmap_remove(struct strintmap *map, const char *str) > { > - return strmap_remove(&map->map, str, 0); > + strmap_remove(&map->map, str, 0); > } So yeah, I think that is the right fix. +cc Elijah for any other insight. -Peff ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 2:52 ` Jeff King @ 2020-12-15 15:55 ` Randall S. Becker 2020-12-15 16:21 ` Elijah Newren 1 sibling, 0 replies; 12+ messages in thread From: Randall S. Becker @ 2020-12-15 15:55 UTC (permalink / raw) To: 'Jeff King', 'Junio C Hamano' Cc: 'Elijah Newren', git On December 14, 2020 9:53 PM, Peff wrote: > On Mon, Dec 14, 2020 at 06:43:36PM -0800, Junio C Hamano wrote: > > > diff --git c/git-compat-util.h w/git-compat-util.h index > > 7d509c5022..58cd0761be 100644 > > --- c/git-compat-util.h > > +++ w/git-compat-util.h > > @@ -273,7 +273,8 @@ struct itimerval { > > > > #ifdef NO_SETITIMER > > static inline int setitimer(int which, const struct itimerval *value, struct > itimerval *newvalue) { > > - ; /* nothing */ > > + errno = ENOSYS; > > + return -1; /* not implemented */ > > } > > #endif > > > > Alternatively we could pretend that the call always succeeds by > > without touching errno and returning 0. That might be safer, but I > > dunno which one we want, and I do not have a system affected by the > > choice. > > I think this is a sensible choice. Before the conversion to an inline, the code > was removed entirely! So anybody checking the return value would have > seen an error, and we do not have to worry much about breaking them. > > For new callers, anybody checking the return value would probably > appreciate the warning that support for the function is optional (OTOH, they > would probably not find out themselves, but rather when Randall tells them ;) > ). > > It would be nice to have a way to warn them even on platforms that have > setitimer(), but I can't think of an easy way. Strangely, we do have setitimer() on NonStop. I'm not sure how we get into this situation, unless it's not being detected correctly. > > > Aside from inlining bodies, this should not have compiled on any > platform: > > > > > > static inline void strset_remove(struct strset *set, const char > > > *str) { > > > return strmap_remove(&set->map, str, 0); } > > > > > > What is really intended here? > > > > I think we should just drop "return"; a void function should be called > > in void context without requiring a value, even if that return expects > > no value. > > Yeah, I think that is right. I checked earlier iterations of the series to see if > perhaps strmap_remove() had previously returned a value, but it never did in > any on the list. > > > diff --git i/strmap.h w/strmap.h > > index c4c104411b..1e152d832d 100644 > > --- i/strmap.h > > +++ w/strmap.h > > @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct > > strintmap *map, const char *str) > > > > static inline void strintmap_remove(struct strintmap *map, const char > > *str) { > > - return strmap_remove(&map->map, str, 0); > > + strmap_remove(&map->map, str, 0); > > } > > So yeah, I think that is the right fix. +cc Elijah for any other insight. FYI: We successfully built 2.29.2 without issues. I'm not sure how this all happened but seems like it was at 1201eb628a. Thanks, Randall ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 2:52 ` Jeff King 2020-12-15 15:55 ` Randall S. Becker @ 2020-12-15 16:21 ` Elijah Newren 2020-12-15 21:07 ` Junio C Hamano 2020-12-15 21:08 ` Junio C Hamano 1 sibling, 2 replies; 12+ messages in thread From: Elijah Newren @ 2020-12-15 16:21 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Randall S. Becker, Git Mailing List On Mon, Dec 14, 2020 at 6:52 PM Jeff King <peff@peff.net> wrote: > > On Mon, Dec 14, 2020 at 06:43:36PM -0800, Junio C Hamano wrote: > > > diff --git c/git-compat-util.h w/git-compat-util.h > > index 7d509c5022..58cd0761be 100644 > > --- c/git-compat-util.h > > +++ w/git-compat-util.h > > @@ -273,7 +273,8 @@ struct itimerval { > > > > #ifdef NO_SETITIMER > > static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { > > - ; /* nothing */ > > + errno = ENOSYS; > > + return -1; /* not implemented */ > > } > > #endif > > > > Alternatively we could pretend that the call always succeeds by > > without touching errno and returning 0. That might be safer, but I > > dunno which one we want, and I do not have a system affected by the > > choice. > > I think this is a sensible choice. Before the conversion to an inline, > the code was removed entirely! So anybody checking the return value > would have seen an error, and we do not have to worry much about > breaking them. > > For new callers, anybody checking the return value would probably > appreciate the warning that support for the function is optional (OTOH, > they would probably not find out themselves, but rather when Randall > tells them ;) ). > > It would be nice to have a way to warn them even on platforms that have > setitimer(), but I can't think of an easy way. > > > > Aside from inlining bodies, this should not have compiled on any platform: > > > > > > static inline void strset_remove(struct strset *set, const char *str) > > > { > > > return strmap_remove(&set->map, str, 0); > > > } > > > > > > What is really intended here? > > > > I think we should just drop "return"; a void function should be > > called in void context without requiring a value, even if that > > return expects no value. > > Yeah, I think that is right. I checked earlier iterations of the series > to see if perhaps strmap_remove() had previously returned a value, but > it never did in any on the list. > > > diff --git i/strmap.h w/strmap.h > > index c4c104411b..1e152d832d 100644 > > --- i/strmap.h > > +++ w/strmap.h > > @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct strintmap *map, const char *str) > > > > static inline void strintmap_remove(struct strintmap *map, const char *str) > > { > > - return strmap_remove(&map->map, str, 0); > > + strmap_remove(&map->map, str, 0); > > } > > So yeah, I think that is the right fix. +cc Elijah for any other > insight. Doh, sorry for the bug. Yeah, that's the exact fix I'd make. Junio, do you want to just use the changes you've already got, or would you like me to submit a patch removing these two 'return's? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 16:21 ` Elijah Newren @ 2020-12-15 21:07 ` Junio C Hamano 2020-12-15 21:08 ` Junio C Hamano 1 sibling, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2020-12-15 21:07 UTC (permalink / raw) To: Elijah Newren; +Cc: Jeff King, Randall S. Becker, Git Mailing List ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 16:21 ` Elijah Newren 2020-12-15 21:07 ` Junio C Hamano @ 2020-12-15 21:08 ` Junio C Hamano 2020-12-15 21:25 ` [PATCH] strmap: make callers of strmap_remove() to call it in void context Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 12+ messages in thread From: Junio C Hamano @ 2020-12-15 21:08 UTC (permalink / raw) To: Elijah Newren; +Cc: Jeff King, Randall S. Becker, Git Mailing List Elijah Newren <newren@gmail.com> writes: > Doh, sorry for the bug. Yeah, that's the exact fix I'd make. Junio, > do you want to just use the changes you've already got, or would you > like me to submit a patch removing these two 'return's? Nah, the other half of the reported issue was mine, so let me see if I can quickly whip up a two-patch series. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] strmap: make callers of strmap_remove() to call it in void context 2020-12-15 21:08 ` Junio C Hamano @ 2020-12-15 21:25 ` Junio C Hamano 2020-12-15 21:35 ` Randall S. Becker 2020-12-15 21:26 ` [PATCH] compat-util: pretend that stub setitimer() always succeeds Junio C Hamano 2020-12-15 21:37 ` [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Jeff King 2 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2020-12-15 21:25 UTC (permalink / raw) To: Elijah Newren, Randall S. Becker; +Cc: Jeff King, Git Mailing List Two "static inline" functions, both of which return void, call strmap_remove() and tries to return the value it returns as their return value, which is just bogus, as strmap_remove() returns void itself. Call it in the void context and fall-thru the control to the end instead. Reported-by: Randall S. Becker <rsbecker@nexbridge.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- strmap.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/strmap.h b/strmap.h index c4c104411b..1e152d832d 100644 --- a/strmap.h +++ b/strmap.h @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct strintmap *map, const char *str) static inline void strintmap_remove(struct strintmap *map, const char *str) { - return strmap_remove(&map->map, str, 0); + strmap_remove(&map->map, str, 0); } static inline int strintmap_empty(struct strintmap *map) @@ -249,7 +249,7 @@ static inline int strset_contains(struct strset *set, const char *str) static inline void strset_remove(struct strset *set, const char *str) { - return strmap_remove(&set->map, str, 0); + strmap_remove(&set->map, str, 0); } static inline int strset_empty(struct strset *set) -- 2.30.0-rc0-186-g20447144ec ^ permalink raw reply related [flat|nested] 12+ messages in thread
* RE: [PATCH] strmap: make callers of strmap_remove() to call it in void context 2020-12-15 21:25 ` [PATCH] strmap: make callers of strmap_remove() to call it in void context Junio C Hamano @ 2020-12-15 21:35 ` Randall S. Becker 0 siblings, 0 replies; 12+ messages in thread From: Randall S. Becker @ 2020-12-15 21:35 UTC (permalink / raw) To: 'Junio C Hamano', 'Elijah Newren' Cc: 'Jeff King', 'Git Mailing List' > -----Original Message----- > From: Junio C Hamano <gitster@pobox.com> > Sent: December 15, 2020 4:26 PM > To: Elijah Newren <newren@gmail.com>; Randall S. Becker > <rsbecker@nexbridge.com> > Cc: Jeff King <peff@peff.net>; Git Mailing List <git@vger.kernel.org> > Subject: [PATCH] strmap: make callers of strmap_remove() to call it in void > context > > Two "static inline" functions, both of which return void, call > strmap_remove() and tries to return the value it returns as their return value, > which is just bogus, as strmap_remove() returns void itself. Call it in the void > context and fall-thru the control to the end instead. > > Reported-by: Randall S. Becker <rsbecker@nexbridge.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > strmap.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/strmap.h b/strmap.h > index c4c104411b..1e152d832d 100644 > --- a/strmap.h > +++ b/strmap.h > @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct strintmap > *map, const char *str) > > static inline void strintmap_remove(struct strintmap *map, const char *str) { > - return strmap_remove(&map->map, str, 0); > + strmap_remove(&map->map, str, 0); > } > > static inline int strintmap_empty(struct strintmap *map) @@ -249,7 +249,7 > @@ static inline int strset_contains(struct strset *set, const char *str) > > static inline void strset_remove(struct strset *set, const char *str) { > - return strmap_remove(&set->map, str, 0); > + strmap_remove(&set->map, str, 0); > } > > static inline int strset_empty(struct strset *set) > -- > 2.30.0-rc0-186-g20447144ec Looks good from here. Regards, Randall ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] compat-util: pretend that stub setitimer() always succeeds 2020-12-15 21:08 ` Junio C Hamano 2020-12-15 21:25 ` [PATCH] strmap: make callers of strmap_remove() to call it in void context Junio C Hamano @ 2020-12-15 21:26 ` Junio C Hamano 2020-12-15 21:37 ` [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Jeff King 2 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2020-12-15 21:26 UTC (permalink / raw) To: Elijah Newren; +Cc: Jeff King, Randall S. Becker, Git Mailing List When 15b52a44 (compat-util: type-check parameters of no-op replacement functions, 2020-08-06) turned a handful of no-op C-preprocessor macros into static inline functions to give the callers a better type checking for their parameters, it forgot to return anything from the stubbed out setitimer() function, even though the function was defined to return an int just like the real thing. Since the original C-preprocessor macro implementation was to just turn the call to the function an empty statement, we know that the existing callers do not check the return value from it, and it does not matter what value we return. But it is safer to pretend that the call succeeded by returning 0 than making it fail by returning -1 and clobbering errno with some value. Reported-by: Randall S. Becker <rsbecker@nexbridge.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- git-compat-util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index 7a0fb7a045..421c90b5d8 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -273,7 +273,7 @@ struct itimerval { #ifdef NO_SETITIMER static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) { - ; /* nothing */ + return 0; /* pretend success */ } #endif -- 2.30.0-rc0-186-g20447144ec ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 21:08 ` Junio C Hamano 2020-12-15 21:25 ` [PATCH] strmap: make callers of strmap_remove() to call it in void context Junio C Hamano 2020-12-15 21:26 ` [PATCH] compat-util: pretend that stub setitimer() always succeeds Junio C Hamano @ 2020-12-15 21:37 ` Jeff King 2020-12-16 17:27 ` Junio C Hamano 2 siblings, 1 reply; 12+ messages in thread From: Jeff King @ 2020-12-15 21:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Elijah Newren, Randall S. Becker, Git Mailing List On Tue, Dec 15, 2020 at 01:08:18PM -0800, Junio C Hamano wrote: > Elijah Newren <newren@gmail.com> writes: > > > Doh, sorry for the bug. Yeah, that's the exact fix I'd make. Junio, > > do you want to just use the changes you've already got, or would you > > like me to submit a patch removing these two 'return's? > > Nah, the other half of the reported issue was mine, so let me see if > I can quickly whip up a two-patch series. Thanks, both look OK to me. I probably would have kept "return -1" in the setitimer one, but I doubt it really matters much either way. -Peff ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 2020-12-15 21:37 ` [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Jeff King @ 2020-12-16 17:27 ` Junio C Hamano 0 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2020-12-16 17:27 UTC (permalink / raw) To: Jeff King; +Cc: Elijah Newren, Randall S. Becker, Git Mailing List Jeff King <peff@peff.net> writes: > On Tue, Dec 15, 2020 at 01:08:18PM -0800, Junio C Hamano wrote: > >> Elijah Newren <newren@gmail.com> writes: >> >> > Doh, sorry for the bug. Yeah, that's the exact fix I'd make. Junio, >> > do you want to just use the changes you've already got, or would you >> > like me to submit a patch removing these two 'return's? >> >> Nah, the other half of the reported issue was mine, so let me see if >> I can quickly whip up a two-patch series. > > Thanks, both look OK to me. I probably would have kept "return -1" in > the setitimer one, but I doubt it really matters much either way. Yeah, we know that the return value does not matter as nobody looks at it. I just was a bit hesitant to clobber errno with ENOSYS. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-12-16 17:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-14 23:12 [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Randall S. Becker 2020-12-15 2:43 ` Junio C Hamano 2020-12-15 2:52 ` Jeff King 2020-12-15 15:55 ` Randall S. Becker 2020-12-15 16:21 ` Elijah Newren 2020-12-15 21:07 ` Junio C Hamano 2020-12-15 21:08 ` Junio C Hamano 2020-12-15 21:25 ` [PATCH] strmap: make callers of strmap_remove() to call it in void context Junio C Hamano 2020-12-15 21:35 ` Randall S. Becker 2020-12-15 21:26 ` [PATCH] compat-util: pretend that stub setitimer() always succeeds Junio C Hamano 2020-12-15 21:37 ` [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Jeff King 2020-12-16 17:27 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).