Bug 211289 - benchmarks/iozone for armv6 and other ILP32 FreeBSD architectures: error: typedef redefinition with different types ('long' vs '__off64_t' (aka 'long long'))
Summary: benchmarks/iozone for armv6 and other ILP32 FreeBSD architectures: error: typ...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: arm Any
: --- Affects Only Me
Assignee: Josh Paetzel
Keywords: needs-patch, needs-qa
Depends on:
Reported: 2016-07-22 10:22 UTC by Mark Millard
Modified: 2016-07-30 10:18 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (jpaetzel)
koobs: merge-quarterly?


Note You need to log in before you can comment on or make changes to this bug.
Description Mark Millard 2016-07-22 10:22:42 UTC
Here is what the recently updated benchmarks/iozone fails like for armv6 builds based on the updates that were recently made (only listing the first few warnings but including the one error):

cc -c -pipe -mcpu=cortex-a7  -g -fno-strict-aliasing  -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO  -DHAVE_PREAD -DNAME='"freebsd"' -DSHARED_MEM  -pipe -
mcpu=cortex-a7  -g -fno-strict-aliasing iozone.c -o iozone_freebsd.o
iozone.c:363:14: error: typedef redefinition with different types ('long' vs '__off64_t' (aka 'long long'))
typedef long off64_t;
/usr/include/sys/types.h:178:19: note: previous definition is here
typedef __off64_t       off64_t;        /* file offset (alias) */
iozone.c:762:9: warning: 'CACHE_LINE_SIZE' macro redefined [-Wmacro-redefined]
#define CACHE_LINE_SIZE 32
/usr/include/machine/param.h:109:9: note: previous definition is here
#define CACHE_LINE_SIZE         (1 << CACHE_LINE_SHIFT)
iozone.c:1921:24: warning: format specifies type 'long *' but the argument has type 'off64_t *' (aka 'long long *') [-Wformat]
                                       ~~~  ^~~~~~~~~~~~
iozone.c:1945:68: warning: format specifies type 'long' but the argument has type 'off64_t' (aka 'long long') [-Wformat]
                        sprintf(splash[splash_line++],"\tFile size set to %ld kB\n",kilobytes64);
                                                                          ~~~       ^~~~~~~~~~~
iozone.c:2319:82: warning: format specifies type 'long' but the argument has type 'off64_t' (aka 'long long') [-Wformat]
                        sprintf(splash[splash_line++],"\tUsing minimum file size of %ld kilobytes.\n",minimum_file_size);
                                                                                    ~~~               ^~~~~~~~~~~~~~~~~

powerpc (32-bit) likely would get the same sort of messages, including the error. The same for other ILP32 FreeBSD architectures: long is only 32-bits but off_t is 64 bits.

-r418913 of /usr/ports was used for the above under 11.0-BETA2 (so clang 3.8.0).
Comment 1 Josh Paetzel freebsd_committer 2016-07-23 15:17:26 UTC
Investigating this now.
Comment 2 commit-hook freebsd_committer 2016-07-23 17:07:39 UTC
A commit references this bug:

Author: jpaetzel
Date: Sat Jul 23 17:06:53 UTC 2016
New revision: 418971
URL: https://svnweb.freebsd.org/changeset/ports/418971

  Unbreak on 32bit FreeBSD

  PR:	211289
  Sponsored by:	iXsystems
  Pointyhat:	jpaetzel
  Cluebat:	jhb

Comment 3 Mark Millard 2016-07-23 21:00:36 UTC
(In reply to commit-hook from comment #2)

This allows the compile to complete but iozone is still broken for ILP32 architectures: the warnings produces are real problems because of format vs. size mismatches and the like.

I've submitted 211317 for the problems with using long formats such as %ld for treating long long storage or values in an ILP32 FreeBSD archtiecture.

powerpc (32-bit) has more issues than armv6/i836 because of being big endian. But I'll not have access to the powerpc's again for weeks yet.
Comment 4 Josh Paetzel freebsd_committer 2016-07-24 00:40:14 UTC
Ok, so I have a couple questions:

1) Was this broken before I started mucking with it?

2) For a tool like iozone do we really care if it doesn't work on platforms that will most likely be network/router?

3) Will using j for the format specifier work? (it seems to work for amd64/i386 which are the only platforms I have access to)

Roger on the no upstreaming the fixes, *my* interest is keeping this working on amd64 / FreeNAS / ZFS.

I'm assuming if I try hard enough I can get the other platforms working in qemu.
Comment 5 Mark Millard 2016-07-24 03:03:20 UTC
(In reply to Josh Paetzel from comment #4)

>Ok, so I have a couple questions:

>1) Was this broken before I started mucking with it?

211152 predated your working on benchmarks/iozone. armv6 and the like compiled but amd64 did not. Your initial change swapped that status.

w.schwarzenfeld@utanet.at was working on 211152 trying to make benchmarks/iozone better for FreeBSD generally, including for making things cleaner for FreeBSD upstream if his changes were accepted (once completed). He also was attempting to base things on the modern iozone releases (3.434 -> 3.444 at the time, now possibly 3.446).

You have been going back through some of the issues he had run into and had been figuring out and updating is patches for.

As far as I know w.schwarzenfeld@utanet.at may have been trying for just a one time update. He may not have been trying for eventually being the maintainer. (I've never asked about that.)

>2) For a tool like iozone do we really care if it doesn't work on platforms that will most likely be network/router?

I do not use the rpi2 as a network/router. Nor do I use the powerpc's for such whenI have access. To some extent the machines are used to check for a degree of code portability. But this tends to involve using other software (ports) in supporting development activities or checking the context's characteristics.

[I've no clue about proportions of users of various types.]

>3) Will using j for the format specifier work? (it seems to work for amd64/i386 which are the only platforms I have access to)

j format and intmax_t are too modern to go upstream. But for a FreeBSD specific fork would be fine. Fixing the unsafe sscanf usage with strtoll and strtol usage that did more validation and the like would have a similar status.

>*my* interest is keeping this working on amd64 / FreeNAS / ZFS

Sounds more like you want to have a special purpose fork of iozone. It does not sounds like you plan/want to track upstream updates but to diverge to be specific to specific FreeBSD-related architectures.

As the maintainer, how will you handle requests to apply patches that target allowing other FreeBSD architectures or cleaner/smaller updates when/if upstream adopts changes?

I have no clue how common it may be for ports to target only specific FreeBSD architectures when upstream targets a wider range of contexts generally.

[Note at this point upstream iozone 3.446 likely gets warnings for %lld vs. long for LP64 FreeBSD architectures but always with the layouts under FreeBSD always actually matching, no size mismatches. This is unlike the %ld changes that were recently made. w.schwarzenfeld@utanet.at was targeting avoiding most of the %lld vs. long warnings for LP64 as well.]

I will note that there is work upstream to handle new architectures, such as covering a NVIDIA GPU based port of iozone in the official iozone. As the upstream author put it: "So, more changes are on the way...".
Comment 6 Josh Paetzel freebsd_committer 2016-07-24 15:20:52 UTC
Ok, thanks for catching me up.

I'm fine if someone else wants to maintain this port, and I'm also fine with helping this work on multiple architectures.

I see there is yet another upstream version as of yesterday, so let me give that a shot and see what happens.  I'll also try to get some other archers running in qemu.
Comment 7 Walter Schwarzenfeld freebsd_triage 2016-07-24 15:53:43 UTC
I never has the intention to maintain this port. I also don't wanted to make     the patch in 211152. Cause as Kubilay ask if I provide a patch I tried it. My intention was to react, cause I know from the past  it seems  nobody will look in iozone PR's. In the past I made two patches for iozone. The first one was easy. The second one worked, but has a lot of warnings. Someone other helped me and made the libasync.c and pitserver.c patch.
With help from Mark, I think we made a good solution, except two things. I overlooked two hunks in the pitserver-patch (you provided in your patch) and the sscanf was left.

Now, I think it will be better to split the port. Make one for FreeBSD, Dragonfly and all systems you can find in the definitions of iozone.c. And one other for other platforms. I think it will be clearer for some typedefs and defintions in the program.
Comment 8 Walter Schwarzenfeld freebsd_triage 2016-07-24 16:23:11 UTC
Or we make it simple and say ONLY_FOR_ARCHS=... or NOT_FOR_ARCHS=....
Comment 9 Mark Millard 2016-07-24 16:41:39 UTC
(In reply to Josh Paetzel from comment #6)

Updating to 3.446 sounds good.

I do not know if you will want to cut down on the number of warnings or not. If you did want to . . .

3.446 could still use updates for %lld usage such as:

-                       sprintf(splash[splash_line++],"\tFile size set to %lld kB\n",kilobytes64);
+                       sprintf(splash[splash_line++],"\tFile size set to %lld kB\n",(long long)kilobytes64);

(make the change via a cast instead of to the %lld format).

This would get rid of unnecessary sprintf warnings in a way that deals with both long long (ILP64 FreeBSD) vs. long (LP64 FreeBSD) for signed 63-bit precision (64-bits total) in a way that should be good outside FreeBSD and so be acceptable upstream.

It is LP64 (for example amd64) that would produce the warnings without such changes: long would be in use for 64-bit. ILP32 will not produce the warnings either way (long long is in use for 64-bit).

If the printf's that produce warnings are taken care of via such changes that would leave a couple of sscanf's that produce warnings for %lld use.

That number of warnings may be more acceptable. But an option would be code like (ignore formatting details here):

                    {long long tmp;
                    kilobytes64 = tmp;

This would allow layout variations for LP64 FreeBSD when variables like kilobytes64 are long instead of long long: it leaves any conversion for the final storage update explicitly to the compiler. It would apply to other (non-FreeBSD) off64_t contexts as well.

Another way to deal with the 2 %lld sscanf's is code changes such as:

-                       sscanf(optarg,"%lld",&kilobytes64);
+                       sscanf(optarg,"%lld",(long long*)&kilobytes64);

This presumes the storage layout are the same for long and long long for LP64 FreeBSD (and other off64_t contexts): no potential storage layout conversion by the compiler.
Comment 10 Josh Paetzel freebsd_committer 2016-07-25 14:18:57 UTC
So trying to update to the newest version blows your patches up, which I can rejigger, however, have you seen the iozone license?

I'm not entirely sure it allows us to patch the source code and still call it iozone.

Did you get permission for those patches?

This is turning in to a bit of a hassle, but that's fine.  I'm going to reach out to the iozone maintainers and see if I can talk to them.

Can anyone send me an intro email to them?
Comment 11 Walter Schwarzenfeld freebsd_triage 2016-07-25 18:44:47 UTC
3.449 is showing following errors:
Build iozone for FreeBSD

/usr/local/libexec/ccache/world/cc -c -O2 -pipe  -fstack-protector -fno-strict-aliasing  -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO  -DHAVE_PREAD -DNAME='"freebsd"' -DSHARED_MEM  -O2 -pipe  -fstack-protector -fno-strict-aliasing iozone.c -o iozone_freebsd.o
iozone.c:997:30: error: function cannot return function type '__sighandler_t' (aka 'void (int)')
__sighandler_t signal_handler(void(int));       /* clean up if user interrupts us */
iozone.c:1865:28: error: use of undeclared identifier 'sighandler_t'; did you mean '__sighandler_t'?
        signal((int) SIGINT, (sighandler_t) signal_handler);            /* handle user interrupt */
/usr/include/sys/signal.h:142:14: note: '__sighandler_t' declared here
typedef void __sighandler_t(int);
iozone.c:1866:29: error: use of undeclared identifier 'sighandler_t'; did you mean '__sighandler_t'?
        signal((int) SIGTERM, (sighandler_t) signal_handler);   /* handle kill from shell */
/usr/include/sys/signal.h:142:14: note: '__sighandler_t' declared here
typedef void __sighandler_t(int);
iozone.c:3595:1: error: unknown type name 'sighandler_t'; did you mean '__sighandler_t'?
sighandler_t signal_handler(int)
/usr/include/sys/signal.h:142:14: note: '__sighandler_t' declared here
typedef void __sighandler_t(int);
iozone.c:3595:28: error: function cannot return function type '__sighandler_t' (aka 'void (int)')
sighandler_t signal_handler(int)
iozone.c:3595:32: error: parameter name omitted
sighandler_t signal_handler(int)

I sendt a mail to iozone.org.
Comment 12 Walter Schwarzenfeld freebsd_triage 2016-07-25 22:27:55 UTC
Will be fixed in version 3.452 coming in the next 24 hours.
Comment 13 Mark Millard 2016-07-26 01:07:45 UTC
(In reply to w.schwarzenfeld from comment #12)

Don C. sent the updated source and it will not work. He has more to do if it is to work.

I wrote to him:

__sighandler_t is from the implementation namespace and is not required to exist with that spelling. Nor is its definition standardized.

In Mac OS X 10.11.6 with the command line tools downloaded and installed (that creates and fills in /usr/include ) I find:

#if !defined(_POSIX_C_SOURCE) || defined(_DARWIN_C_SOURCE)
typedef void (*sig_t)(int);     /* type of signal function */

in sys/signal.h . Note that sig_t has the pointer to the function in the type.

I do not find sighandler_t or __sighandler_t anywhere in Mac OS X's /usr/include . In fact:

markmi$ find /usr/include -type f -exec grep sigh {} \; -print | more
/* Define to 1 if you have the `sighold' function. */
               /* [sigh] some implementations return the "illegal" op for unsupported ops */ \
int     sighold(int);

So no variation of "sighandler" at all.

I do not have OpenBSD, DragonFly, or NetBSD available.

As for FreeBSD it does have a __sighandler_t but it does not have the pointer to a function. Instead sig_t that also involves the pointer:

# find /usr/include/ -type f -exec grep sigh {} \; -print | more
               __sighandler_t          *sf_handler;
               __sighandler_t          *sf_handler;
/* TODO: Handle Linux stat32/stat64 ugliness. <sigh> */
#define SIG_DFL         ((__sighandler_t *)0)
#define SIG_IGN         ((__sighandler_t *)1)
#define SIG_ERR         ((__sighandler_t *)-1)
/* #define      SIG_CATCH       ((__sighandler_t *)2) See signalvar.h */
#define SIG_HOLD        ((__sighandler_t *)3)
* programs can avoid the warnings by casting to (__sighandler_t *) or
typedef void __sighandler_t(int);
typedef __sighandler_t  *sig_t; /* type of pointer to a signal function */
       __sighandler_t *sv_handler;     /* signal handler */
__sighandler_t *signal(int, __sighandler_t *);
int     signalcontext(ucontext_t *, int, __sighandler_t *);
#define SIG_CATCH       ((__sighandler_t *)2)
/* #define SIG_HOLD        ((__sighandler_t *)3) See signal.h */
int     sighold(int);

The easiest thing may be to define:

typedef void (*my_sig_t)(int);     /* type of signal function */

and use my_sig_t. This would avoid all the varied ways of having a typedef that was never standardized. It also avoids use of C/system implementation-space names that are subject to change without notification.
Comment 14 Walter Schwarzenfeld freebsd_triage 2016-07-28 12:10:12 UTC
Today 3,457 is out.    _sighandler_t is fixed. The warnings seems all fixed. (Now is no additional patch needed, patch-pitserver.c is obsolete).
I am not sure  if ALL things Mark critisized are fixed, but it ssems so.
Comment 15 Josh Paetzel freebsd_committer 2016-07-28 14:17:22 UTC
3.457 compiles with no patches and no warnings on HEAD amd64 and i386.  I'm going to commit that.
Comment 16 commit-hook freebsd_committer 2016-07-28 14:41:34 UTC
A commit references this bug:

Author: jpaetzel
Date: Thu Jul 28 14:40:45 UTC 2016
New revision: 419222
URL: https://svnweb.freebsd.org/changeset/ports/419222

  Update to latest version

  Incorporates port patches
  Fixes type warnings on 32 bit and 64 bit with clang
  Should fix iozone on some of the more "esoteric" arches

  PR:	211289
  Sponsored by:	iXsystems

Comment 17 Walter Schwarzenfeld freebsd_triage 2016-07-30 10:02:14 UTC
Mark can you/we close this too? Or is anything still open?
Comment 18 Mark Millard 2016-07-30 10:10:58 UTC
(In reply to commit-hook from comment #16)

I updated to -r419226 for /usr/ports on the armv6 (rpi2 cortext-a7 actually) in a 11.0-BETA2 context.

The rebuild got no warnings. An "iozone -a" ran to completion. Lots of figures make sense only based on in-memory caching, including all the read-related KBytes/sec figures. Other than "record rewrite" for <= 64K reclen: the write figures for large files fit with the expected rpi2 USB 2.0 interface speed (given the fast SSD in use).

The maximum file size may well not have been large enough to use up the RAM so this is not unexpected.

Side note: For FreeBSD the "Compiled for 32 bit mode" iozone output notice is always generated does not indicate either:

A) file system position-addressing/size handling
B) LP64 vs. ILP32 processor/FreeBSD architecture.

For (A) FreeBSD is always 64 bit (signed).

For (B) it matches the host it was built on: the iozone source auto adjusts based on __LP64__ and/or _LP64 vs. neither being defined.

The same is true if the port had used freebsd64 instead of freebsd as the target, although it would then always say "Compiled for 64 bit mode".
Comment 19 Mark Millard 2016-07-30 10:13:05 UTC
(In reply to w.schwarzenfeld from comment #17)

I've closed it after experimenting on the rpi2, per comment 18 that was being composed while you sent comment 17.
Comment 20 Mark Millard 2016-07-30 10:18:45 UTC
(In reply to Mark Millard from comment #18)

I probably should have noted that I use noatime on the mount of the SSD in /etc/fstab .