Ticket #3960 (closed defect: fixed)

Opened 6 years ago

Last modified 2 months ago

MC 4.8.22 doesn't compile on AIX 7.2

Reported by: dp Owned by: zaytsev
Priority: major Milestone: 4.8.32
Component: mc-core Version:
Keywords: Cc: jax, IBMJesseG
Blocked By: #4542 Blocking: #3983
Branch state: merged Votes for changeset: committed-master

Description

Hi there,

The compilation fails on AIX 7.2 (7200-03-01-1838) with the following error messages:

make[3]: Entering directory '/home/dp/rpmbuild/BUILD/mc-4.8.22/src/filemanager'
  CC       achown.lo
achown.c: In function 'chl_callback':
achown.c:491:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
         switch (parm)
         ^~~~~~
achown.c:505:5: note: here
     default:
     ^~~~~~~
  CC       boxes.lo
  CC       chmod.lo
  CC       chown.lo
  CC       cmd.lo
  CC       command.lo
In function 'examine_cd',
    inlined from 'do_cd_command' at command.c:440:16:
command.c:172:25: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-overflow=]
                         strncpy (r, t, tlen + 1);
                         ^~~~~~~~~~~~~~~~~~~~~~~~
command.c: In function 'do_cd_command':
command.c:168:28: note: length computed here
                     tlen = strlen (t);
                            ^~~~~~~~~~
  CC       dir.lo
  CC       ext.lo
  CC       file.lo
file.c: In function 'get_times':
file.c:899:17: error: incompatible types when assigning to type 'struct timespec' from type 'st_timespec_t' {aka 'const struct st_timespec'}
     (*times)[0] = sb->st_atim;
                 ^
file.c:900:17: error: incompatible types when assigning to type 'struct timespec' from type 'st_timespec_t' {aka 'const struct st_timespec'}
     (*times)[1] = sb->st_mtim;
                 ^
make[3]: *** [Makefile:604: file.lo] Error 1
make[3]: Leaving directory '/home/dp/rpmbuild/BUILD/mc-4.8.22/src/filemanager'
make[2]: *** [Makefile:738: all-recursive] Error 1
make[2]: Leaving directory '/home/dp/rpmbuild/BUILD/mc-4.8.22/src'
make[1]: *** [Makefile:625: all-recursive] Error 1
make[1]: Leaving directory '/home/dp/rpmbuild/BUILD/mc-4.8.22'
make: *** [Makefile:553: all] Error 2

The full listing is attached.

Attachments

compilation.log (31.7 KB) - added by dp 6 years ago.
aix_fix.patch (458 bytes) - added by andrew_b 6 years ago.
compilation2.log (31.7 KB) - added by dp 6 years ago.

Change History

Changed 6 years ago by dp

comment:1 follow-up: ↓ 4 Changed 6 years ago by zaytsev

  • Cc jax, IBMJesseG added

Sounds like compilation on AIX got busted by PASE changes :( Does any of you have access to AIX / can help with that?

comment:2 Changed 6 years ago by andrew_b

Warning are fixed in the 3955_cleanup branch.

I guess the fix of compilation error for AIX should be the same as fix for IBM i: [a45337672be6f32df2a598f3fdc03e3c0b8f53ac].

Changed 6 years ago by andrew_b

comment:3 follow-up: ↓ 5 Changed 6 years ago by zaytsev

dp, could you please try the attached patch out? thanks!

comment:4 in reply to: ↑ 1 Changed 6 years ago by dp

Replying to zaytsev:

Sounds like compilation on AIX got busted by PASE changes :( Does any of you have access to AIX / can help with that?

Yes, of course. I can help you. I have an unlimited access to AIX.

comment:5 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 6 years ago by dp

Replying to zaytsev:

dp, could you please try the attached patch out? thanks!

I have done it, but the compilation failed again.((
Please find the new attached file compilation2.log.

Last edited 6 years ago by dp (previous) (diff)

Changed 6 years ago by dp

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 6 years ago by andrew_b

Replying to dp:

I have done it, but the compilation failed again.((
Please find the new attached file compilation2.log.

There is no difference between logs.
Did you run autoreconf after apply patch to configure.ac?

comment:7 in reply to: ↑ 6 Changed 6 years ago by dp

Replying to andrew_b:

Replying to dp:

I have done it, but the compilation failed again.((
Please find the new attached file compilation2.log.

There is no difference between logs.
Did you run autoreconf after apply patch to configure.ac?

Sorry, no. I didn't know it. This time mc has been successfully compiled.

comment:8 Changed 6 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.23

comment:9 Changed 6 years ago by andrew_b

  • Votes for changeset set to dp andrew_b
  • Branch state changed from on review to approved

comment:10 Changed 6 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from dp andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:11 Changed 6 years ago by andrew_b

  • Status changed from testing to closed

comment:12 Changed 6 months ago by zaytsev

  • Status changed from closed to reopened
  • Votes for changeset committed-master deleted
  • Version 4.8.22 deleted
  • Branch state changed from merged to no branch
  • Milestone changed from 4.8.23 to 4.8.32
  • Resolution fixed deleted

GCC has AIX 7.1 and AIX 7.3 machines. Sadly no IBM i access. But I suspect that the problem is/was the same as on macOS. They implemented nanosecond access via st_atimespec instead of st_atim, so instead of this patch we should check whether this is true and maybe still implement the ultimate support.

comment:13 Changed 6 months ago by zaytsev

  • Status changed from reopened to accepted
  • Owner changed from andrew_b to zaytsev

comment:14 Changed 6 months ago by zaytsev

  • Blocked By 4542 added

comment:15 Changed 6 months ago by zaytsev

So I think I've got mc @ 4542_nanoseconds_cleanup to build on AIX 7.3 with glib-2.43.92 and latest gettext / libffi / slang.

It even seems to even mostly work correctly in as far as timestamps are concerned, which is impressive. The original patches were wrong, the kernel actually support everything almost correctly, only structure name is different for binary compatibility reasons (!?), so barring a hard cast one could just assign field by field and be done with it.

If anyone has interest to test on AIX or PASE, you are most welcome. I could also get my hands on AIX 7.1, but the machines are so slow and AIX is so painful to work with.

The magic sauce for mc is (it finds termcap, but can't link?):

GLIB_LIBS="-L$HOME/opt/glib/lib -lglib-2.0" \
GLIB_CFLAGS="-I$HOME/opt/glib/include/glib-2.0 -I$HOME/opt/glib/lib/glib-2.0/include" \
SLANG_LIBS="-L$HOME/opt/slang/lib -lslang" \
SLANG_CFLAGS=-I$HOME/opt/slang/include \
  ../configure --prefix=$HOME/opt/mc

Slang must be compiled with make static and make install-static.

LDFLAGS=-static ./configure --prefix=$HOME/opt/slang
gmake static
gmake install-static

glib-2.43.92 (need to patch out -werror and codegen stuff):

LIBFFI_LIBS=$HOME/opt/libffi/lib \
LIBFFI_CFLAGS=-I$HOME/opt/libffi/include \
LDFLAGS=-L$HOME/opt/gettext/lib \
CPPFLAGS=-I$HOME/opt/gettext/include \
  ../configure --disable-static --enable-shared --prefix=$HOME/opt/glib

libffi and gettext are ok. Next time maybe still a good idea to install pkg-config...

P.S. Broken termcap detection is fixed on master.

Last edited 4 months ago by zaytsev (previous) (diff)

comment:16 Changed 4 months ago by zaytsev

AIX can show nanosecond timestamps with ls -als --full-time if GNU ls is available.

comment:17 follow-up: ↓ 18 Changed 4 months ago by zaytsev

Interesting remaining warnings: probably some size_t type of thing? Not sure how to fix this.

../../../src/filemanager/cmd.c: In function 'compare_files':
../../../src/filemanager/cmd.c:224:36: warning: 'memcmp' specified size 4294967295 exceeds maximum object size 2147483647 [-Wstringop-overflow=]
  224 |             result = (n1 != n2) || memcmp (buf1, buf2, n1);
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~

The rest looks really surprisingly good. Just a few warnings from GNU code and some gettext stuff.

comment:18 in reply to: ↑ 17 Changed 4 months ago by andrew_b

Replying to zaytsev:

Interesting remaining warnings: probably some size_t type of thing? Not sure how to fix this.

Probably change int to ssize_t (as read(2) returns ssize_t), initialization and size_t/ssize_t casting:

  • src/filemanager/cmd.c

    index 70602db04..760f79975 100644
    a b compare_files (const vfs_path_t *vpath1, const vfs_path_t *vpath2, off_t size) 
    210210#else 
    211211            /* Don't have mmap() :( Even more ugly :) */ 
    212212            char buf1[BUFSIZ], buf2[BUFSIZ]; 
    213             int n1, n2; 
     213            ssize_t n1 = 0; 
     214            ssize_t n2 = 0; 
    214215 
    215216            rotate_dash (TRUE); 
    216217            do 
    compare_files (const vfs_path_t *vpath1, const vfs_path_t *vpath2, off_t size) 
    220221                while ((n2 = read (file2, buf2, sizeof (buf2))) == -1 && errno == EINTR) 
    221222                    ; 
    222223            } 
    223             while (n1 == n2 && n1 == sizeof (buf1) && memcmp (buf1, buf2, sizeof (buf1)) == 0); 
    224             result = (n1 != n2) || memcmp (buf1, buf2, n1); 
     224            while (n1 == n2 && n1 == (ssize_t) sizeof (buf1) 
     225                   && memcmp (buf1, buf2, sizeof (buf1)) == 0); 
     226            result = (n1 != n2) || memcmp (buf1, buf2, (size_t) n1); 
    225227#endif /* !HAVE_MMAP */ 
    226228            close (file2); 
    227229        } 

comment:19 Changed 4 months ago by zaytsev

So this problem is caused by GCC bug, but I would suggest the following patch for clarity anyways:

diff --git a/src/filemanager/cmd.c b/src/filemanager/cmd.c
index 70602db04..133c8a168 100644
--- a/src/filemanager/cmd.c
+++ b/src/filemanager/cmd.c
@@ -210,7 +210,7 @@ compare_files (const vfs_path_t *vpath1, const vfs_path_t *vpath2, off_t size)
 #else
             /* Don't have mmap() :( Even more ugly :) */
             char buf1[BUFSIZ], buf2[BUFSIZ];
-            int n1, n2;
+            ssize_t n1, n2;
 
             rotate_dash (TRUE);
             do

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100571

But the real problem is that our mmap-code is not used, and instead a fallback is used. There is an unsafe way to override it, but I would delete it:

diff --git a/configure.ac b/configure.ac
index a18defa63..7f99554e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -317,19 +317,6 @@ dnl replacing lstat with statlstat on sco makes it more portable between
 dnl sco clones
 AC_CHECK_FUNCS(statlstat)
 
-dnl Overriding mmap support.  This has to be before AC_FUNC_MMAP is used.
-dnl We use only part of the functionality of mmap, so on AIX,
-dnl it's possible to use mmap, even if it doesn't pass the autoconf test.
-AC_ARG_WITH([mmap],
-       AS_HELP_STRING([--with-mmap], [Use the mmap call @<:@yes if found@:>@]))
-if test x$with_mmap != xno; then
-    if test x$with_mmap = x; then
-       AC_FUNC_MMAP
-    else
-       AC_DEFINE(HAVE_MMAP, 1)
-    fi
-fi
-
 mc_GET_FS_INFO

It's only used in file comparison nowadays, and even though mmap version is probably more performant, I'm not sure if that really matters.

The problem here is that the Autoconf test fails:

|   if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,                                                                  
|                    MAP_PRIVATE | MAP_FIXED, fd, 0L))                                                                           
|     return 10;                                                                                                                 

But it's not clear to me whether the test is bad, or AIX has been broken for decades. I've asked on the autoconf list, but probably it's on pre-moderation.

comment:21 Changed 4 months ago by zaytsev

So it seems that the facts are as follows:

  1. mmap macro was introduced to autoconf by grep people and does nothing wrong
  2. mmap on AIX has been broken at least in a sense of MAP_FIXED support for decades
  3. grep later got rid of mmap, because in the end it didn't make things any faster
  4. mc used mmap in the viewer, for file comparison and in other places
  5. mc provided --with-mmap to force using broken mmap implementations
  6. mc viewer is now rewritten and remaining code has an okayish fallback

Having that said, I would just remove mmap code, the override and whatever else has to do with that and can be removed. Then we can close this ticket... for now. Sounds good?

comment:22 Changed 4 months ago by andrew_b

Yes.

comment:23 Changed 4 months ago by zaytsev

  • Branch state changed from no branch to on review

Branch: 3960_remove_mmap
Initial changeset:5b335920aa59a8a120914899b71a752acb29f1d9

I hope IO_BUFSIZE is okay for the stack...?

Also, I took time to update the documentation: mmap isn't used in the viewer, so I removed that. Some other advice is either obsolete or wrong. So I deleted extra files and updated the links. Also I have removed some general tutorial text on how to use autotools, and replaced it with specific advice for bootstrapping and porting. I hope that this is uncontroversial.

comment:24 Changed 4 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:25 Changed 4 months ago by zaytsev

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:26 Changed 4 months ago by zaytsev

  • Status changed from testing to closed

comment:27 Changed 2 months ago by zaytsev

  • Blocking 3983 added

comment:28 Changed 2 months ago by zaytsev

Incidentally, fixed #3983.

Note: See TracTickets for help on using tickets.