Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MC 4.8.22 doesn't compile on AIX 7.2 #3960

Closed
mc-butler opened this issue Jan 10, 2019 · 32 comments
Closed

MC 4.8.22 doesn't compile on AIX 7.2 #3960

mc-butler opened this issue Jan 10, 2019 · 32 comments
Assignees
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Milestone

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3960
Reporter dp (adxru@….ru)
Mentions jax (softwoehr@….com), IBMJesseG (jgorzins@….ibm.com)

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.

Note

Original attachments:

@mc-butler
Copy link
Author

Changed by dp (adxru@….ru) on Jan 10, 2019 at 15:48 UTC

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jan 12, 2019 at 11:39 UTC (comment 1)

  • Cc set to jax, IBMJesseG

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 12, 2019 at 11:50 UTC (comment 2)

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: [a453376].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 12, 2019 at 11:53 UTC

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jan 12, 2019 at 12:01 UTC (comment 3)

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

@mc-butler
Copy link
Author

Changed by dp (adxru@….ru) on Jan 15, 2019 at 7:17 UTC (comment 1.4)

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.

@mc-butler
Copy link
Author

Changed by dp (adxru@….ru) on Jan 15, 2019 at 7:20 UTC (comment 3.5)

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.

@mc-butler
Copy link
Author

Changed by dp (adxru@….ru) on Jan 15, 2019 at 7:21 UTC

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 15, 2019 at 9:55 UTC (comment 5.6)

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?

@mc-butler
Copy link
Author

Changed by dp (adxru@….ru) on Jan 15, 2019 at 11:28 UTC (comment 6.7)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 19, 2019 at 16:09 UTC (comment 8)

  • 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

Branch: 3960_aix
[56320a3]

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 19, 2019 at 16:10 UTC (comment 9)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 19, 2019 at 16:11 UTC (comment 10)

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

Merged to master: [56320a3].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 19, 2019 at 16:12 UTC (comment 11)

  • Status changed from testing to closed

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 31, 2024 at 8:31 UTC (comment 12)

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

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.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 31, 2024 at 20:26 UTC (comment 13)

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 31, 2024 at 20:28 UTC (comment 14)

  • Blocked by set to #4542

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jun 2, 2024 at 21:39 UTC (comment 15)

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.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 27, 2024 at 19:29 UTC (comment 16)

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 27, 2024 at 20:07 UTC (comment 17)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 28, 2024 at 7:23 UTC (comment 17.18)

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:

index 70602db04..760f79975 100644
--- a/src/filemanager/cmd.c
+++ b/src/filemanager/cmd.c
@@ -210,7 +210,8 @@ 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 = 0;
+            ssize_t n2 = 0;
 
             rotate_dash (TRUE);
             do
@@ -220,8 +221,9 @@ compare_files (const vfs_path_t *vpath1, const vfs_path_t *vpath2, off_t size)
                 while ((n2 = read (file2, buf2, sizeof (buf2))) == -1 && errno == EINTR)
                     ;
             }
-            while (n1 == n2 && n1 == sizeof (buf1) && memcmp (buf1, buf2, sizeof (buf1)) == 0);
-            result = (n1 != n2) || memcmp (buf1, buf2, n1);
+            while (n1 == n2 && n1 == (ssize_t) sizeof (buf1)
+                   && memcmp (buf1, buf2, sizeof (buf1)) == 0);
+            result = (n1 != n2) || memcmp (buf1, buf2, (size_t) n1);
 #endif /* !HAVE_MMAP */
             close (file2);
         }

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 28, 2024 at 7:50 UTC (comment 19)

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.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 28, 2024 at 16:27 UTC (comment 20)

https://lists.gnu.org/archive/html/autoconf/2024-07/msg00006.html

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 29, 2024 at 6:46 UTC (comment 21)

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?

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jul 29, 2024 at 8:43 UTC (comment 22)

Yes.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Aug 1, 2024 at 9:39 UTC (comment 23)

  • Branch state changed from no branch to on review

Branch: 3960_remove_mmap
Initial [5b33592]

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Aug 1, 2024 at 17:06 UTC (comment 24)

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Aug 2, 2024 at 5:44 UTC (comment 25)

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Aug 23, 2024 at 17:30 UTC (comment 26)

  • Status changed from testing to closed

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Sep 25, 2024 at 7:19 UTC (comment 27)

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Sep 25, 2024 at 7:20 UTC (comment 28)

Incidentally, fixed #3983.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 22, 2025 at 11:29 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Development

No branches or pull requests

2 participants