# Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop 12 messages from 2020-12-14 to 2020-12-16. Participants: Randall S. Becker, Junio C Hamano, Jeff King, Elijah Newren. Thread: https://gitlist.dev/t/54825 ## Randall S. Becker, 2020-12-14 23:12 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: <035d01d6d26e$8c255260$a46ff720$@nexbridge.com> URL: https://gitlist.dev/e/035d01d6d26e%248c255260%24a46ff720%24%40nexbridge.com ``` 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. ``` ## Junio C Hamano, 2020-12-15 02:43 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/xmqqeejraiw7.fsf%40gitster.c.googlers.com In-Reply-To: <035d01d6d26e$8c255260$a46ff720$@nexbridge.com> ``` "Randall S. Becker" 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. ``` ## Jeff King, 2020-12-15 02:52 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/X9glBJnlLMTW6Dns%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Randall S. Becker, 2020-12-15 15:55 Subject: RE: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: <001e01d6d2fa$bc1afab0$3450f010$@nexbridge.com> URL: https://gitlist.dev/e/001e01d6d2fa%24bc1afab0%243450f010%24%40nexbridge.com In-Reply-To: ``` 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 ``` ## Elijah Newren, 2020-12-15 16:21 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/CABPp-BEnkQahdVJP%2BrfQho%2BS_hcS3y-o3cZBUMhU-EQHjJTLOQ%40mail.gmail.com In-Reply-To: ``` On Mon, Dec 14, 2020 at 6:52 PM Jeff King 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? ``` ## Junio C Hamano, 2020-12-15 21:07 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/xmqq4kkm93sm.fsf%40gitster.c.googlers.com In-Reply-To: ``` ``` ## Junio C Hamano, 2020-12-15 21:08 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/xmqqzh2e7p6l.fsf%40gitster.c.googlers.com In-Reply-To: ``` Elijah Newren 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. ``` ## Junio C Hamano, 2020-12-15 21:25 Subject: [PATCH] strmap: make callers of strmap_remove() to call it in void context Message-ID: URL: https://gitlist.dev/e/xmqqtusm7odr.fsf_-_%40gitster.c.googlers.com In-Reply-To: ``` 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 Signed-off-by: Junio C Hamano --- 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 ``` ## Junio C Hamano, 2020-12-15 21:26 Subject: [PATCH] compat-util: pretend that stub setitimer() always succeeds Message-ID: URL: https://gitlist.dev/e/xmqqpn3a7ocm.fsf_-_%40gitster.c.googlers.com In-Reply-To: ``` 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 Signed-off-by: Junio C Hamano --- 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 ``` ## Randall S. Becker, 2020-12-15 21:35 Subject: RE: [PATCH] strmap: make callers of strmap_remove() to call it in void context Message-ID: <004a01d6d32a$3b92ec60$b2b8c520$@nexbridge.com> URL: https://gitlist.dev/e/004a01d6d32a%243b92ec60%24b2b8c520%24%40nexbridge.com In-Reply-To: ``` > -----Original Message----- > From: Junio C Hamano > Sent: December 15, 2020 4:26 PM > To: Elijah Newren ; Randall S. Becker > > Cc: Jeff King ; Git Mailing List > 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 > Signed-off-by: Junio C Hamano > --- > 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 ``` ## Jeff King, 2020-12-15 21:37 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/X9ksfyBujBrYWmAW%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Dec 15, 2020 at 01:08:18PM -0800, Junio C Hamano wrote: > Elijah Newren 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 ``` ## Junio C Hamano, 2020-12-16 17:27 Subject: Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop Message-ID: URL: https://gitlist.dev/e/xmqqsg854q5y.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jeff King writes: > On Tue, Dec 15, 2020 at 01:08:18PM -0800, Junio C Hamano wrote: > >> Elijah Newren 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. ```