Minor portability issues + fixes

Minor portability issues + fixes

[Daniel Richard G.]

Hello list,

I am building Git 2.26.2 on AIX. A few compilation errors arose, but
they are resolvable with a few minor changes that will improve overall
portability.

There were a few errors of this form:

    sha1-file.c: In function 'mmap_limit_check':
    sha1-file.c:940:12: error: 'SIZE_MAX' undeclared (first use in this function)
    sha1-file.c:940:12: note: each undeclared identifier is reported only once for each function it appears in

SIZE_MAX is defined in stdint.h, and adding that #include fixes this. It
is likely that this header is being pulled in on other platforms due to
transitive dependencies, but that does not occur on AIX.

The following files need #include<stdint.h>:

    sha1-file.c
    utf8.c
    vcs-svn/svndiff.c
    wrapper.c

(I am not sure of the correct place to add a header like this; I
have only verified that adding it to the .c file directly solves
the problem.)

Then, at link time, I saw this:

    rm -f xdiff/lib.a && ar rcs xdiff/lib.a xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o xdiff/xmerge.o xdiff/xpatience.o xdiff/xhistogram.o
    gcc  -D_ALL_SOURCE -D_THREAD_SAFE   -I. -D_LARGE_FILES -DGIT_HOST_CPU="\"00XXXXXXXXXX\"" -I/nfs/freeport/arch/aix32/include -DNO_CURL -I/nfs/freeport/arch/aix32/include -DNO_OPENSSL -DNO_D_TYPE_IN_DIRENT -DNO_NSEC -Dsockaddr_storage=sockaddr_in6 -DNO_ICONV -DOLD_ICONV -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK -DNO_PTHREADS -DHAVE_PATHS_H -DHAVE_STRINGS_H -DHAVE_CLOCK_GETTIME  -DSNPRINTF_RETURNS_BOGUS -DFREAD_READS_DIRECTORIES -DNO_STRCASESTR -DNO_STRLCPY -DNO_STRTOUMAX -DNO_SETENV -DNO_MKDTEMP -DNO_UNSETENV -DNO_PREAD -DNO_MEMMEM -DINTERNAL_QSORT -Icompat/regex -DFILENO_IS_A_MACRO -DNEED_ACCESS_ROOT_HANDLER -DDEFAULT_EDITOR='"nano"' -DDEFAULT_PAGER='"more"' -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -o git-credential-store   credential-store.o common-main.o libgit.a xdiff/lib.a  -L/nfs/freeport/arch/aix32/lib /nfs/freeport/arch/aix32/lib -L/nfs/freeport/arch/aix32/lib /nfs/freeport/arch/aix32/lib -lz -lintl
    ld: 0711-168 SEVERE ERROR: Input file: /nfs/freeport/arch/aix32/lib
            Input files must be regular files.
    collect2: error: ld returned 12 exit status
    Makefile:2456: recipe for target 'git-credential-store' failed
    gmake: *** [git-credential-store] Error 1

After some investigation, I found that this resulted from the following
configure-time determination:

    configure:4774: WARNING: linker does not support runtime path to dynamic libraries
    (from line 488 of configure.ac)

The problem is, in this case, CC_LD_DYNPATH is set to an empty value---
which is incorrect, in light of how it is used in the Makefile. Here is
a typical example:

    ifdef ZLIB_PATH
            BASIC_CFLAGS += -I$(ZLIB_PATH)/include
            EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH)$(ZLIB_PATH)/$(lib)
    endif
    EXTLIBS += -lz

Setting that variable to an empty value causes a bare directory to be
passed to the linker, which of course then errors out. I would suggest
setting it to "-L" instead.

(The libraries I am using are all static archives, so I am not hampered
by a lack of runtime library paths.)


--Daniel


-- 
Daniel Richard G. || skunk@iSKUNK.ORG
My ASCII-art .sig got a bad case of Times New Roman.

Re: Minor portability issues + fixes

[Jeff King]

On Mon, May 18, 2020 at 09:15:58PM -0400, Daniel Richard G. wrote:

> I am building Git 2.26.2 on AIX. A few compilation errors arose, but
> they are resolvable with a few minor changes that will improve overall
> portability.
> 
> There were a few errors of this form:
> 
>     sha1-file.c: In function 'mmap_limit_check':
>     sha1-file.c:940:12: error: 'SIZE_MAX' undeclared (first use in this function)
>     sha1-file.c:940:12: note: each undeclared identifier is reported only once for each function it appears in
> 
> SIZE_MAX is defined in stdint.h, and adding that #include fixes this. It
> is likely that this header is being pulled in on other platforms due to
> transitive dependencies, but that does not occur on AIX.

All system defines in Git should be pulled in via git-compat-util.h.
That does include stdint.h, but only if NO_INTTYPES_H is defined
(otherwise we prefer inttypes.h). And POSIX (2004) says:

  The <inttypes.h> header shall include the <stdint.h> header.

But perhaps that is not so on AIX (it wouldn't be the first time we've
seen a platform that does not strictly follow POSIX).

Does building with:

  make NO_INTTYPES_H=YesPlease

work?

If so, then perhaps it should be added to the AIX defines in
config.mak.uname.

> The problem is, in this case, CC_LD_DYNPATH is set to an empty value---
> which is incorrect, in light of how it is used in the Makefile. Here is
> a typical example:
> 
>     ifdef ZLIB_PATH
>             BASIC_CFLAGS += -I$(ZLIB_PATH)/include
>             EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH)$(ZLIB_PATH)/$(lib)
>     endif
>     EXTLIBS += -lz
> 
> Setting that variable to an empty value causes a bare directory to be
> passed to the linker, which of course then errors out. I would suggest
> setting it to "-L" instead.

That would just be redundant with the earlier argument. That might be
the easiest way to turn it into a noop, but we can probably do better
with $(if) or similar, which would allow somebody to build with:

  make CC_LD_DYNPATH=

even without using the autoconf script.

I do wonder, though, if configure.ac could be extended for AIX to
support whatever syntax the system linker uses for setting the run-path.
I understand that you don't care either way about this feature, but this
might be a good opportunity to fix it.

-Peff

Re: Minor portability issues + fixes

[Carlo Marcelo Arenas Belón]

On Mon, May 18, 2020 at 09:15:58PM -0400, Daniel Richard G. wrote:
> 
> I am building Git 2.26.2 on AIX. A few compilation errors arose, but
> they are resolvable with a few minor changes that will improve overall
> portability.

which version of AIX is this?

> There were a few errors of this form:
> 
>     sha1-file.c: In function 'mmap_limit_check':
>     sha1-file.c:940:12: error: 'SIZE_MAX' undeclared (first use in this function)
>     sha1-file.c:940:12: note: each undeclared identifier is reported only once for each function it appears in

we include inttypes.h which is supposed to include stdint.h per POSIX[1]

could you take a look at that header and see if there is some macro definition
preventing that to happen?

Carlo

[1] https://pubs.opengroup.org/onlinepubs/009696799/basedefs/inttypes.h.html

Re: Minor portability issues + fixes

[Daniel Richard G.]

Hi Jeff,

On Mon, 2020 May 18 22:49-04:00, Jeff King wrote:
> 
> All system defines in Git should be pulled in via git-compat-util.h.
> That does include stdint.h, but only if NO_INTTYPES_H is defined
> (otherwise we prefer inttypes.h). And POSIX (2004) says:
> 
>   The <inttypes.h> header shall include the <stdint.h> header.
> 
> But perhaps that is not so on AIX (it wouldn't be the first time we've
> seen a platform that does not strictly follow POSIX).

On AIX 4.3, where I am building, inttypes.h does not #include stdint.h,
and stdint.h is not present under /usr/include/. (This is an old system
used for ABI compatibility-testing purposes.)

This is not general to AIX, however, because on 5.3, both stdint.h and
its #include are there.

That said, my build is using GCC 4.7, which provides a modern stdint.h.
The GCC tree contains a fixincluded version of inttypes.h, but this also
lacks the <stdint.h> #include.

> Does building with:
> 
>   make NO_INTTYPES_H=YesPlease
> 
> work?
>
> If so, then perhaps it should be added to the AIX defines in
> config.mak.uname.

I've confirmed that this works. But would it not be safe to #include
both inttypes.h and stdint.h explicitly if both are present, rather than
cater to AIX specifically? I could see this similarly arising in, say,
an old version of Solaris.

> > Setting that variable to an empty value causes a bare directory to be
> > passed to the linker, which of course then errors out. I would suggest
> > setting it to "-L" instead.
> 
> That would just be redundant with the earlier argument. That might be
> the easiest way to turn it into a noop, but we can probably do better
> with $(if) or similar, which would allow somebody to build with:
> 
>   make CC_LD_DYNPATH=
> 
> even without using the autoconf script.

That is a fair point. CC_LD_DYNPATH is used only a handful of times, so
this wouldn't be much more work.

> I do wonder, though, if configure.ac could be extended for AIX to
> support whatever syntax the system linker uses for setting the run-path.
> I understand that you don't care either way about this feature, but this
> might be a good opportunity to fix it.

This could be a bit complicated, I'm afraid.

The applicable value for CC_LD_DYNPATH on AIX is "-Wl,-blibpath:".
However, have a look at the description for this option in the
ld(1) man page:

       libpath:Path
            Uses Path as the library path when writing the loader
            section of the output file. Path is neither checked for
            validity nor used when searching for libraries specified by
            the -l flag. Path overrides any library paths generated when
            the -L flag is used.

            If you do not specify any -L flags, or if you specify the
            nolibpath option, the default library path information is
            written in the loader section of the output file. The
            default library path information is the value of the LIBPATH
            environment variable if it is defined, and /usr/lib:/lib,
            otherwise.

(excerpted from an AIX 7.1 system)

That implies that when you use this option, you must specify not only
the path you're interested in, but also /usr/lib:/lib! And when I look
at an existing AIX build of ours, that makes use of Libtool, I see that
it is in fact passing in a rather lengthy path for -Wl,-blibpath:, that
ends with ":/usr/lib:/lib".


--Daniel


-- 
Daniel Richard G. || skunk@iSKUNK.ORG
My ASCII-art .sig got a bad case of Times New Roman.

Re: Minor portability issues + fixes

[Daniel Richard G.]

Hi Carlo,

On Mon, 2020 May 18 23:04-04:00, Carlo Marcelo Arenas Belón wrote:
> On Mon, May 18, 2020 at 09:15:58PM -0400, Daniel Richard G. wrote:
> > 
> > I am building Git 2.26.2 on AIX. A few compilation errors arose, but
> > they are resolvable with a few minor changes that will improve overall
> > portability.
> 
> which version of AIX is this?

4.3, believe it or not :-]  This system is used for compatibility
testing of a legacy product.

> we include inttypes.h which is supposed to include stdint.h per POSIX[1]
> 
> could you take a look at that header and see if there is some macro definition
> preventing that to happen?

There's no #include at all, and stdint.h is not present under
/usr/include/. But I am building with GCC 4.7 (as the original
vendor compiler has outlived its usefulness), and that provides a
modern stdint.h.

The original inttypes.h is fixincluded by GCC, but this did not add the
stdint.h #include.

As I remarked to Jeff, would it be kosher to #include both inttypes.h
and stdint.h if both are present? It seems that most of the time, the
latter would be a no-op.


--Daniel


-- 
Daniel Richard G. || skunk@iSKUNK.ORG
My ASCII-art .sig got a bad case of Times New Roman.

Re: Minor portability issues + fixes

[Jeff King]

On Tue, May 19, 2020 at 12:22:00AM -0400, Daniel Richard G. wrote:

> > If so, then perhaps it should be added to the AIX defines in
> > config.mak.uname.
> 
> I've confirmed that this works. But would it not be safe to #include
> both inttypes.h and stdint.h explicitly if both are present, rather than
> cater to AIX specifically? I could see this similarly arising in, say,
> an old version of Solaris.

Yes, as long as we check that both are present, I think that would do
the right thing. And the autoconf check could cover that. I don't think
there's an easy way to have a general Makefile knob that can check which
files are present, though (and we generally consider the Makefile and
its knobs to be the primary mechanism; autoconf to turn those knobs is
generally implemented on top).

So probably we'd want something like (in this order):

  - NEEDS_EXPLICIT_STDINT gets passed from the Makefile into the
    compiler via -D, which then triggers stdint.h being included
    unconditionally in git-compat-util.h

  - optionally set that in config.mak.uname for AIX (checking uname_R
    since it sounds like only old versions need it)

  - add an autoconf rule that sets it, either strictly (when a
    test-program decides it's needed) or loosely (when we see that it's
    available at all)

Even just the first one would let you build by setting the knob
manually; the rest is gravy on top, if you or somebody else chooses to
do it.

> > I do wonder, though, if configure.ac could be extended for AIX to
> > support whatever syntax the system linker uses for setting the run-path.
> > I understand that you don't care either way about this feature, but this
> > might be a good opportunity to fix it.
> 
> This could be a bit complicated, I'm afraid.
> 
> The applicable value for CC_LD_DYNPATH on AIX is "-Wl,-blibpath:".
> However, have a look at the description for this option in the
> ld(1) man page:

OK, gross. :) I agree it's not worth going too far into this rabbit
hole. I do wonder if you could just be using GNU ld along with gcc, but
maybe that's not practical.

You should be able to build with:

  make CC_LD_DYNPATH=-L

as a workaround, but it would be nice if the Makefile handled this
correctly. It looks like CC_LD_DYNPATH gets used in a lot of places, so
I suspect we'd want a Makefile function to help out. Something like:


  # usage: $(call linker_lib,PATH)
  # Create linker args for looking for libraries in PATH at both link-time
  # and run-time.
  linker_lib = -L$1 $(if $(CC_LD_DYNPATH),$(CC_LD_DYNPATH)$1)

which would allow:

  EXTLIBS += $(call linker_lib,$(LIBPCREDIR)/$(lib))

etc. This would be our first foray into Makefile functions, but I think
we've determined that most platforms have a recent enough GNU make for
it to be OK (and we already require GNU make).

-Peff

Re: Minor portability issues + fixes

[Daniel Richard G.]

On Wed, 2020 May 20 00:28-04:00, Jeff King wrote:
>
> > I've confirmed that this works. But would it not be safe to #include
> > both inttypes.h and stdint.h explicitly if both are present, rather than
> > cater to AIX specifically? I could see this similarly arising in, say,
> > an old version of Solaris.
> 
> Yes, as long as we check that both are present, I think that would do
> the right thing. And the autoconf check could cover that. I don't think
> there's an easy way to have a general Makefile knob that can check which
> files are present, though (and we generally consider the Makefile and
> its knobs to be the primary mechanism; autoconf to turn those knobs is
> generally implemented on top).

Okay, I see, so the whole phalanx of HAVE_BLAH_H symbols from Autoconf
isn't available.

> So probably we'd want something like (in this order):
> 
>   - NEEDS_EXPLICIT_STDINT gets passed from the Makefile into the
>     compiler via -D, which then triggers stdint.h being included
>     unconditionally in git-compat-util.h
> 
>   - optionally set that in config.mak.uname for AIX (checking uname_R
>     since it sounds like only old versions need it)
> 
>   - add an autoconf rule that sets it, either strictly (when a
>     test-program decides it's needed) or loosely (when we see that it's
>     available at all)
> 
> Even just the first one would let you build by setting the knob
> manually; the rest is gravy on top, if you or somebody else chooses to
> do it.

Hmm... that's a fairly specific knob, which I would think is less than
ideal. The rest is reasonable, but would have to be written in terms
of the knob.

I can put something together, knowing that this is the approach you'd
want to see, but it'll need some more work. Eventually, I'll need to get
Git up and running on a few other old systems I have here, so that will
undoubtedly figure into it.

Thanks for sketching out how this should work, however; this is helpful
to keep in mind.

> > The applicable value for CC_LD_DYNPATH on AIX is "-Wl,-blibpath:".
> > However, have a look at the description for this option in the
> > ld(1) man page:
> 
> OK, gross. :) I agree it's not worth going too far into this rabbit
> hole. I do wonder if you could just be using GNU ld along with gcc, but
> maybe that's not practical.

Using the GNU linker can help in some cases, and hurt in others. We try
to avoid it unless necessary.

> You should be able to build with:
> 
>   make CC_LD_DYNPATH=-L
> 
> as a workaround

Yep, this works handily as a no-op :)

> but it would be nice if the Makefile handled this correctly. It looks
> like CC_LD_DYNPATH gets used in a lot of places, so I suspect we'd
> want a Makefile function to help out. Something like:
> 
>   # usage: $(call linker_lib,PATH)
>   # Create linker args for looking for libraries in PATH at both link-time
>   # and run-time.
>   linker_lib = -L$1 $(if $(CC_LD_DYNPATH),$(CC_LD_DYNPATH)$1)
> 
> which would allow:
> 
>   EXTLIBS += $(call linker_lib,$(LIBPCREDIR)/$(lib))
> 
> etc. This would be our first foray into Makefile functions, but I think
> we've determined that most platforms have a recent enough GNU make for
> it to be OK (and we already require GNU make).

Why not just a variable to wrap the conditional? Something like

    CC_LD_DYNPATH_flag = $(if $(CC_LD_DYNPATH),$(CC_LD_DYNPATH),-L)

    ...

    EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH_flag)$(ZLIB_PATH)/$(lib)

Makefile functions are quite powerful, but they feel like a sledgehammer
to this fly. (I've used them in the past to generate Make rules
programmatically. It wasn't pretty, but it sure beat writing out
makefile fragments and then include-ing them afterward!)


--Daniel


-- 
Daniel Richard G. || skunk@iSKUNK.ORG
My ASCII-art .sig got a bad case of Times New Roman.

Re: Minor portability issues + fixes

[Jeff King]

On Thu, May 21, 2020 at 12:29:18AM -0400, Daniel Richard G. wrote:

> > So probably we'd want something like (in this order):
> > 
> >   - NEEDS_EXPLICIT_STDINT gets passed from the Makefile into the
> >     compiler via -D, which then triggers stdint.h being included
> >     unconditionally in git-compat-util.h
> > 
> >   - optionally set that in config.mak.uname for AIX (checking uname_R
> >     since it sounds like only old versions need it)
> > 
> >   - add an autoconf rule that sets it, either strictly (when a
> >     test-program decides it's needed) or loosely (when we see that it's
> >     available at all)
> > 
> > Even just the first one would let you build by setting the knob
> > manually; the rest is gravy on top, if you or somebody else chooses to
> > do it.
> 
> Hmm... that's a fairly specific knob, which I would think is less than
> ideal. The rest is reasonable, but would have to be written in terms
> of the knob.

Yeah, it is. I do wonder if just setting the existing NO_INTTYPES_H is a
simpler solution for your case (since it sounds like that works). If you
want to tweak the knob for older AIX in config.mak.uname, that would be
fine (or if you think other people aren't likely to be building on the
platform, maybe tweak your own "make" invocation and let this
conversation serve to document for anybody else).

> > but it would be nice if the Makefile handled this correctly. It looks
> > like CC_LD_DYNPATH gets used in a lot of places, so I suspect we'd
> > want a Makefile function to help out. Something like:
> > 
> >   # usage: $(call linker_lib,PATH)
> >   # Create linker args for looking for libraries in PATH at both link-time
> >   # and run-time.
> >   linker_lib = -L$1 $(if $(CC_LD_DYNPATH),$(CC_LD_DYNPATH)$1)
> > 
> > which would allow:
> > 
> >   EXTLIBS += $(call linker_lib,$(LIBPCREDIR)/$(lib))
> > 
> > etc. This would be our first foray into Makefile functions, but I think
> > we've determined that most platforms have a recent enough GNU make for
> > it to be OK (and we already require GNU make).
> 
> Why not just a variable to wrap the conditional? Something like
> 
>     CC_LD_DYNPATH_flag = $(if $(CC_LD_DYNPATH),$(CC_LD_DYNPATH),-L)
> 
>     ...
> 
>     EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH_flag)$(ZLIB_PATH)/$(lib)

I was trying to actually remove the argument, rather than turn it into a
duplicate "-L". But you're probably right that it's not worth the
effort.

I think we could just normalize an empty CC_LD_DYNPATH to "-L" in the
same variable, and then we wouldn't even have to change the sites which
reference it.

-Peff