[Change] Git build issue on NonStop

[Change] Git build issue on NonStop

[rsbecker]

Just a quick FYI. The addition of uintptr_t in clar tests has broken my CI
build
on NonStop x86. I will be fixing this locally. It may take a patch series
unless
a quick workaround is possible, which I am hoping.

For those on the list from my platform who are monitoring, this looks like
-D__NSK_OPTIONAL_TYPES__ is now required for the build. I am unsure
what else may be needed.

--Randall

--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.

Re: [Change] Git build issue on NonStop

[Jeff King]

On Wed, Sep 17, 2025 at 10:16:13PM -0400, rsbecker@nexbridge.com wrote:

> Just a quick FYI. The addition of uintptr_t in clar tests has broken
> my CI build on NonStop x86. I will be fixing this locally. It may take
> a patch series unless a quick workaround is possible, which I am
> hoping.
> 
> For those on the list from my platform who are monitoring, this looks like
> -D__NSK_OPTIONAL_TYPES__ is now required for the build. I am unsure
> what else may be needed.

We use uintptr_t in lots of places in the regular code. I guess this bit
in compat/posix.h is what makes it work:

  #ifdef NO_INTPTR_T
  /*
   * On I16LP32, ILP32 and LP64 "long" is the safe bet, however
   * on LLP86, IL33LLP64 and P64 it needs to be "long long",
   * while on IP16 and IP16L32 it is "int" (resp. "short")
   * Size needs to match (or exceed) 'sizeof(void *)'.
   * We can't take "long long" here as not everybody has it.
   */
  typedef long intptr_t;
  typedef unsigned long uintptr_t;
  #endif

But clar has its own compatibility layer. So it would need to do
something similar. I see the clar line in question also uses PRIxPTR,
which I can imagine might not be available everywhere either. We don't
use that ourselves at all.

I kind of wonder if just:

diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
index 80c5359425..f408af850f 100644
--- a/t/unit-tests/clar/clar.c
+++ b/t/unit-tests/clar/clar.c
@@ -875,8 +875,8 @@ void clar__assert_equal(
 		void *p1 = va_arg(args, void *), *p2 = va_arg(args, void *);
 		is_equal = (p1 == p2);
 		if (!is_equal)
-			p_snprintf(buf, sizeof(buf), "0x%"PRIxPTR" != 0x%"PRIxPTR,
-				   (uintptr_t)p1, (uintptr_t)p2);
+			p_snprintf(buf, sizeof(buf), "0x%"PRIuMAX" != 0x%"PRIuMAX,
+				   (uintmax_t)p1, (uintmax_t)p2);
 	}
 	else {
 		int i1 = va_arg(args, int), i2 = va_arg(args, int);

would be sufficient.

-Peff

RE: [Change] Git build issue on NonStop

[rsbecker]

On September 17, 2025 10:29 PM, Jeff King wrote:
>On Wed, Sep 17, 2025 at 10:16:13PM -0400, rsbecker@nexbridge.com wrote:
>
>> Just a quick FYI. The addition of uintptr_t in clar tests has broken
>> my CI build on NonStop x86. I will be fixing this locally. It may take
>> a patch series unless a quick workaround is possible, which I am
>> hoping.
>>
>> For those on the list from my platform who are monitoring, this looks
>> like -D__NSK_OPTIONAL_TYPES__ is now required for the build. I am
>> unsure what else may be needed.
>
>We use uintptr_t in lots of places in the regular code. I guess this bit in
>compat/posix.h is what makes it work:
>
>  #ifdef NO_INTPTR_T
>  /*
>   * On I16LP32, ILP32 and LP64 "long" is the safe bet, however
>   * on LLP86, IL33LLP64 and P64 it needs to be "long long",
>   * while on IP16 and IP16L32 it is "int" (resp. "short")
>   * Size needs to match (or exceed) 'sizeof(void *)'.
>   * We can't take "long long" here as not everybody has it.
>   */
>  typedef long intptr_t;
>  typedef unsigned long uintptr_t;
>  #endif
>
>But clar has its own compatibility layer. So it would need to do something similar. I
>see the clar line in question also uses PRIxPTR, which I can imagine might not be
>available everywhere either. We don't use that ourselves at all.
>
>I kind of wonder if just:
>
>diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c index
>80c5359425..f408af850f 100644
>--- a/t/unit-tests/clar/clar.c
>+++ b/t/unit-tests/clar/clar.c
>@@ -875,8 +875,8 @@ void clar__assert_equal(
> 		void *p1 = va_arg(args, void *), *p2 = va_arg(args, void *);
> 		is_equal = (p1 == p2);
> 		if (!is_equal)
>-			p_snprintf(buf, sizeof(buf), "0x%"PRIxPTR" !=
>0x%"PRIxPTR,
>-				   (uintptr_t)p1, (uintptr_t)p2);
>+			p_snprintf(buf, sizeof(buf), "0x%"PRIuMAX" !=
>0x%"PRIuMAX,
>+				   (uintmax_t)p1, (uintmax_t)p2);
> 	}
> 	else {
> 		int i1 = va_arg(args, int), i2 = va_arg(args, int);
>
>would be sufficient.

Yes, it would work. uintmax_t is part of the standard set while uintptr_t is
considered an extension. Not my decision on this grouping. I'm setting
the -D in CFLAGS to see if that works, I would be fine going that way, 
although better would be adding it into config.uname.mak in the NONSTOP
section.

Re: [Change] Git build issue on NonStop

[Patrick Steinhardt]

On Wed, Sep 17, 2025 at 11:20:05PM -0400, rsbecker@nexbridge.com wrote:
> On September 17, 2025 10:29 PM, Jeff King wrote:
> >On Wed, Sep 17, 2025 at 10:16:13PM -0400, rsbecker@nexbridge.com wrote:
> >
> >> Just a quick FYI. The addition of uintptr_t in clar tests has broken
> >> my CI build on NonStop x86. I will be fixing this locally. It may take
> >> a patch series unless a quick workaround is possible, which I am
> >> hoping.
> >>
> >> For those on the list from my platform who are monitoring, this looks
> >> like -D__NSK_OPTIONAL_TYPES__ is now required for the build. I am
> >> unsure what else may be needed.
> >
> >We use uintptr_t in lots of places in the regular code. I guess this bit in
> >compat/posix.h is what makes it work:
> >
> >  #ifdef NO_INTPTR_T
> >  /*
> >   * On I16LP32, ILP32 and LP64 "long" is the safe bet, however
> >   * on LLP86, IL33LLP64 and P64 it needs to be "long long",
> >   * while on IP16 and IP16L32 it is "int" (resp. "short")
> >   * Size needs to match (or exceed) 'sizeof(void *)'.
> >   * We can't take "long long" here as not everybody has it.
> >   */
> >  typedef long intptr_t;
> >  typedef unsigned long uintptr_t;
> >  #endif
> >
> >But clar has its own compatibility layer. So it would need to do something similar. I
> >see the clar line in question also uses PRIxPTR, which I can imagine might not be
> >available everywhere either. We don't use that ourselves at all.
> >
> >I kind of wonder if just:
> >
> >diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c index
> >80c5359425..f408af850f 100644
> >--- a/t/unit-tests/clar/clar.c
> >+++ b/t/unit-tests/clar/clar.c
> >@@ -875,8 +875,8 @@ void clar__assert_equal(
> > 		void *p1 = va_arg(args, void *), *p2 = va_arg(args, void *);
> > 		is_equal = (p1 == p2);
> > 		if (!is_equal)
> >-			p_snprintf(buf, sizeof(buf), "0x%"PRIxPTR" !=
> >0x%"PRIxPTR,
> >-				   (uintptr_t)p1, (uintptr_t)p2);
> >+			p_snprintf(buf, sizeof(buf), "0x%"PRIuMAX" !=
> >0x%"PRIuMAX,
> >+				   (uintmax_t)p1, (uintmax_t)p2);
> > 	}
> > 	else {
> > 		int i1 = va_arg(args, int), i2 = va_arg(args, int);
> >
> >would be sufficient.
> 
> Yes, it would work. uintmax_t is part of the standard set while uintptr_t is
> considered an extension. Not my decision on this grouping. I'm setting
> the -D in CFLAGS to see if that works, I would be fine going that way, 
> although better would be adding it into config.uname.mak in the NONSTOP
> section.

That should work alright, yeah. Peff, do you want to create a PR in
https://github.com/clar-test/clar to fix this? Otherwise I can handle
this.

Thanks, both!

Patrick

Re: [Change] Git build issue on NonStop

[Jeff King]

On Thu, Sep 18, 2025 at 07:37:34AM +0200, Patrick Steinhardt wrote:

> > >diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c index
> > >80c5359425..f408af850f 100644
> > >--- a/t/unit-tests/clar/clar.c
> > >+++ b/t/unit-tests/clar/clar.c
> > >@@ -875,8 +875,8 @@ void clar__assert_equal(
> > > 		void *p1 = va_arg(args, void *), *p2 = va_arg(args, void *);
> > > 		is_equal = (p1 == p2);
> > > 		if (!is_equal)
> > >-			p_snprintf(buf, sizeof(buf), "0x%"PRIxPTR" !=
> > >0x%"PRIxPTR,
> > >-				   (uintptr_t)p1, (uintptr_t)p2);
> > >+			p_snprintf(buf, sizeof(buf), "0x%"PRIuMAX" !=
> > >0x%"PRIuMAX,
> > >+				   (uintmax_t)p1, (uintmax_t)p2);
> > > 	}
> > > 	else {
> > > 		int i1 = va_arg(args, int), i2 = va_arg(args, int);
> > >
> > >would be sufficient.
> > 
> > Yes, it would work. uintmax_t is part of the standard set while uintptr_t is
> > considered an extension. Not my decision on this grouping. I'm setting
> > the -D in CFLAGS to see if that works, I would be fine going that way, 
> > although better would be adding it into config.uname.mak in the NONSTOP
> > section.
> 
> That should work alright, yeah. Peff, do you want to create a PR in
> https://github.com/clar-test/clar to fix this? Otherwise I can handle
> this.

I'd be happy if you take it from here. Note in what I posted above it
should probably be PRIxMAX to show hex (not "u").

-Peff

Re: [Change] Git build issue on NonStop

[Patrick Steinhardt]

On Thu, Sep 18, 2025 at 02:31:52AM -0400, Jeff King wrote:
> On Thu, Sep 18, 2025 at 07:37:34AM +0200, Patrick Steinhardt wrote:
> 
> > > >diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c index
> > > >80c5359425..f408af850f 100644
> > > >--- a/t/unit-tests/clar/clar.c
> > > >+++ b/t/unit-tests/clar/clar.c
> > > >@@ -875,8 +875,8 @@ void clar__assert_equal(
> > > > 		void *p1 = va_arg(args, void *), *p2 = va_arg(args, void *);
> > > > 		is_equal = (p1 == p2);
> > > > 		if (!is_equal)
> > > >-			p_snprintf(buf, sizeof(buf), "0x%"PRIxPTR" !=
> > > >0x%"PRIxPTR,
> > > >-				   (uintptr_t)p1, (uintptr_t)p2);
> > > >+			p_snprintf(buf, sizeof(buf), "0x%"PRIuMAX" !=
> > > >0x%"PRIuMAX,
> > > >+				   (uintmax_t)p1, (uintmax_t)p2);
> > > > 	}
> > > > 	else {
> > > > 		int i1 = va_arg(args, int), i2 = va_arg(args, int);
> > > >
> > > >would be sufficient.
> > > 
> > > Yes, it would work. uintmax_t is part of the standard set while uintptr_t is
> > > considered an extension. Not my decision on this grouping. I'm setting
> > > the -D in CFLAGS to see if that works, I would be fine going that way, 
> > > although better would be adding it into config.uname.mak in the NONSTOP
> > > section.
> > 
> > That should work alright, yeah. Peff, do you want to create a PR in
> > https://github.com/clar-test/clar to fix this? Otherwise I can handle
> > this.
> 
> I'd be happy if you take it from here. Note in what I posted above it
> should probably be PRIxMAX to show hex (not "u").

One thing I missed: `uintmax_t` doesn't work on 32 bit systems:

    ::error file=clar.c,line=879::clar.c:879:8: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
      879 |        (uintmax_t)p1, (uintmax_t)p2);
          |        ^

I'm inclined to just use "%p" instead and accept that this has
platform-dependent behaviour. Means we'll have to drop the test for
this, but that's the lesser evil from my point of view.

Patrick

Re: [Change] Git build issue on NonStop

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> One thing I missed: `uintmax_t` doesn't work on 32 bit systems:
>
>     ::error file=clar.c,line=879::clar.c:879:8: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>       879 |        (uintmax_t)p1, (uintmax_t)p2);
>           |        ^
>
> I'm inclined to just use "%p" instead and accept that this has
> platform-dependent behaviour. Means we'll have to drop the test for
> this, but that's the lesser evil from my point of view.

As long as %p works everywhere and with stable output, that is the
most appropriate solution, I would think.  After all, this is used
only for "oops, the test expects these two pointers are pointing at
the same address, but they differ; they point at these places...".

To test such a test, wouldn't it be sufficient to perform "does it
give a bit of output or not?" check in isolation?

RE: [Change] Git build issue on NonStop

[rsbecker]

On September 18, 2025 10:48 AM, Junio C Hamano wrote:
>Patrick Steinhardt <ps@pks.im> writes:
>
>> One thing I missed: `uintmax_t` doesn't work on 32 bit systems:
>>
>>     ::error file=clar.c,line=879::clar.c:879:8: cast from pointer to
integer of different
>size [-Werror=pointer-to-int-cast]
>>       879 |        (uintmax_t)p1, (uintmax_t)p2);
>>           |        ^
>>
>> I'm inclined to just use "%p" instead and accept that this has
>> platform-dependent behaviour. Means we'll have to drop the test for
>> this, but that's the lesser evil from my point of view.
>
>As long as %p works everywhere and with stable output, that is the most
>appropriate solution, I would think.  After all, this is used only for
"oops, the test
>expects these two pointers are pointing at the same address, but they
differ; they
>point at these places...".
>
>To test such a test, wouldn't it be sufficient to perform "does it give a
bit of output
>or not?" check in isolation?

Surprisingly, this actually works on NonStop x86 in both memory models
despite
warnings to the contrary in their man page.. Thanks :)