Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Randall S. Becker]

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.

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Junio C Hamano]

"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.

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Jeff King]

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

RE: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Randall S. Becker]

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

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Elijah Newren]

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?

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Junio C Hamano]

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Junio C Hamano]

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.

[PATCH] strmap: make callers of strmap_remove() to call it in void context

[Junio C Hamano]

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

RE: [PATCH] strmap: make callers of strmap_remove() to call it in void context

[Randall S. Becker]

> -----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

[PATCH] compat-util: pretend that stub setitimer() always succeeds

[Junio C Hamano]

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

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Jeff King]

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

Re: [ANNOUNCE] git-2.30.0-rc0 - Compile Fails on HPE NonStop

[Junio C Hamano]

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.