Bug 211152 - benchmarks/iozone: Build fails on typedef redefinition with different types ('long long' vs '__off64_t' (aka 'long'))
Summary: benchmarks/iozone: Build fails on typedef redefinition with different types (...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Josh Paetzel
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-07-16 07:50 UTC by Mark Millard
Modified: 2016-07-28 21:03 UTC (History)
4 users (show)

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


Attachments
patch-Makefile_iozone (612 bytes, patch)
2016-07-16 19:42 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff_benchmark_iozone (5.38 KB, patch)
2016-07-16 22:00 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff_benchmarks_iozone (5.50 KB, patch)
2016-07-16 23:39 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff_benchmark_iozone (20.48 KB, patch)
2016-07-17 05:09 UTC, Walter Schwarzenfeld
no flags Details | Diff
patch-benchmarks_iozone__iozone_c (19.48 KB, text/x-csrc)
2016-07-18 07:46 UTC, Walter Schwarzenfeld
no flags Details
svn-diff_benchmark_iozone_update_3.444 (26.65 KB, patch)
2016-07-18 08:37 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff_benchmark_iozone_new (1.33 KB, patch)
2016-07-18 23:33 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff_benchmark_iozone_3.444_new (53.93 KB, patch)
2016-07-18 23:49 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff_benchmark_iozone_3.444_next (54.33 KB, patch)
2016-07-19 22:23 UTC, Walter Schwarzenfeld
koobs: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Millard 2016-07-16 07:50:12 UTC
From a amd64 11.0-BETA1 context:

cc -c -pipe  -g -fstack-protector -fno-strict-aliasing  -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO  -DHAVE_PREAD -DNAME='"freebsd"' -DSHARED_MEM  -pipe  -g -fstack-protector -fno-strict-aliasing iozone.c -o iozone_freebsd.o
iozone.c:363:19: error: typedef redefinition with different types ('long long' vs '__off64_t' (aka 'long'))
typedef long long off64_t;
                  ^
/usr/include/sys/types.h:178:19: note: previous definition is here
typedef __off64_t       off64_t;        /* file offset (alias) */
                        ^

Context details:

# uname -apKU
FreeBSD FreeBSDx64 11.0-BETA1 FreeBSD 11.0-BETA1 #7 r302457M: Sat Jul  9 19:28:27 PDT 2016     markmi@FreeBSDx64:/usr/obj/clang/amd64.amd64/usr/src/sys/GENERIC-NODEBUG  amd64 amd64 1100120 1100120

# svnlite info /usr/ports/
Path: /usr/ports
Working Copy Root Path: /usr/ports
URL: svn://svn.freebsd.org/ports/head
Relative URL: ^/head
Repository Root: svn://svn.freebsd.org/ports
Repository UUID: 35697150-7ecd-e111-bb59-0022644237b5
Revision: 418253
Node Kind: directory
Schedule: normal
Last Changed Author: feld
Last Changed Rev: 418253
Last Changed Date: 2016-07-08 18:09:44 -0700 (Fri, 08 Jul 2016)
Comment 1 Walter Schwarzenfeld freebsd_triage 2016-07-16 17:34:03 UTC
I think it is this definition in iozone.c
#ifndef solaris
#ifndef off64_t
#ifndef _OFF64_T
#ifndef __AIX__
#ifndef __off64_t_defined
#ifndef SCO_Unixware_gcc
#ifndef UWIN
#ifndef __DragonFly__
typedef long long off64_t;   <===
#endif
#endif
#endif
#endif
#endif
#endif
#endif
#endif

but I don't find the right syntax with a conditional like ! defined __FREEBSD__ or something like this.
Comment 2 Walter Schwarzenfeld freebsd_triage 2016-07-16 17:38:10 UTC
I had a "hackish" workaround (delete the line from iozone.c)
for the Makefile
post-patch:
.if ${OPSYS} == FreeBSD && ${OSVERSION} >= 1100000
        @${REINPLACE_CMD} -e '363d' ${WRKSRC}/iozone.c
.endif

this compiles, but throws a lot of warning (printf formatting).

But surely not the right way.
Comment 3 Walter Schwarzenfeld freebsd_triage 2016-07-16 17:44:04 UTC
If it maybe not the right way, but above was incomplete:

+.include <bsd.port.pre.mk>

+post-patch:
+.if ${OPSYS} == FreeBSD && ${OSVERSION} >= 1100000
+       @${REINPLACE_CMD} -e '363d' ${WRKSRC}/iozone.c
+.endif

post-patch-SSH-on:
        @${REINPLACE_CMD} -e 's|shell\,\"rsh\"|shell\,\"ssh\"|' \
                ${WRKSRC}/iozone.c

do-install:
        ${INSTALL_PROGRAM} ${WRKSRC}/iozone ${STAGEDIR}${PREFIX}/bin
        ${INSTALL_MAN} ${WRKDIR}/${DISTNAME}/docs/iozone.1 \
                ${STAGEDIR}${MAN1PREFIX}/man/man1

-.include <bsd.port.mk>
+.include <bsd.port.post.mk>
Comment 4 VK freebsd_triage 2016-07-16 18:50:24 UTC
Thanks guys. The port has no maintainer. Can you provide a patch? I'm assuming it needs one, from your comments.
Comment 5 Walter Schwarzenfeld freebsd_triage 2016-07-16 19:42:03 UTC
Created attachment 172588 [details]
patch-Makefile_iozone
Comment 6 Walter Schwarzenfeld freebsd_triage 2016-07-16 19:46:30 UTC
I provide the patch, but I am not really sure about it, cause I got warnings like that: (But it compiles without problems in port and poudriere).

iozone.c:1920:25: warning: format specifies type 'long long *' but the argument has type 'off64_t *' (aka 'long *') [-Wformat]
                        sscanf(optarg,"%lld",&kilobytes64);
                                       ~~~~  ^~~~~~~~~~~~
                                       %ld
iozone.c:1944:69: warning: format specifies type 'long long' but the argument has type 'off64_t' (aka 'long') [-Wformat]
                        sprintf(splash[splash_line++],"\tFile size set to %lld kB\n",kilobytes64);
                                                                          ~~~~       ^~~~~~~~~~~
                                                                          %ld

don't know if this will  cause errors in use.
Comment 7 Walter Schwarzenfeld freebsd_triage 2016-07-16 22:00:07 UTC
Created attachment 172593 [details]
svn-diff_benchmark_iozone
Comment 8 Walter Schwarzenfeld freebsd_triage 2016-07-16 22:02:09 UTC
Provide an additional patch. Cast some of the printf variables to long long.
Should now compile without errors. (Test with poudriere 11amd64 and port 103amd64).
(Keep the old Makefile patch, tell me if I should obsolete it).
Comment 9 Walter Schwarzenfeld freebsd_triage 2016-07-16 23:39:04 UTC
Created attachment 172598 [details]
svn-diff_benchmarks_iozone
Comment 10 Walter Schwarzenfeld freebsd_triage 2016-07-16 23:40:18 UTC
I used in the REPLACE_CMD not the text, but the line-number. So I added a warning to the Makefile, for future updates.
Comment 11 Mark Millard 2016-07-17 00:58:52 UTC
(In reply to w.schwarzenfeld from comment #10)

I do not have a FreeBSD 10.x or earlier context to check. So I've not tested if the "delete line" issue is 11.0+ specific or not. My guess is that it is not. So the version test may be wrong.

The following would cause the Makefile to be very fragile relative to iozone.c updates: nothing checks that the correct line is being deleted or reports when things have likely changed. It is now commented for the issue but. . .

.if ${OPSYS} == FreeBSD && ${OSVERSION} >= 1100000
	@${REINPLACE_CMD} -e '363d' ${WRKSRC}/iozone.c
.endif

usually C source code checks its own context via macro tests. Why adjust the Makefile instead of iozone.c itself?

Looking around shows iozone has no explicit support for 64-bit for freebsd:

/usr/ports/benchmarks/iozone/Makefile has:

MAKEFILE=       makefile
MAKE_ARGS=      ${MAKE_ENV}
ALL_TARGET=     freebsd

which uses:

. . ./iozone3_434/src/current/makefile

# grep -i freebsd /usr/obj/portswork/usr/ports/benchmarks/iozone/work/iozone3_434/src/current/makefile
#		convex, FreeBSD, OpenBSD, OSFV3, OSFV4, OSFV5, SCO
	@echo "        ->   freebsd              (32bit)   <-"
# GNU C compiler FreeBSD build with no threads, no largefiles, no async I/O
freebsd:	iozone_freebsd.o libbif.o fileop_freebsd.o libasync.o pit_server.o
	$(CC) $(LDFLAGS) iozone_freebsd.o libbif.o -lpthread libasync.o \
	$(CC)  -O fileop_freebsd.o -o fileop
fileop_freebsd.o:	fileop.c
	@echo "Building fileop for FreeBSD"
	$(CC) -c -O $(CFLAGS) fileop.c -o fileop_freebsd.o
iozone_freebsd.o:	iozone.c libbif.c libasync.c
	@echo "Build iozone for FreeBSD"
	$(CC) -c ${CFLAGS}  -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO \
		-DHAVE_PREAD -DNAME='"freebsd"' -DSHARED_MEM \
		$(CFLAGS) iozone.c -o iozone_freebsd.o
	$(CC) -c ${CFLAGS} -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO \
	$(CC) -c ${CFLAGS} -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO \

The the types that should be involved likely change for:

TARGET_ARCH=amd64 or powerpc64 or aarch64
vs.
TARGET_ARCH=i386 or powerpc or armv6

(not a comprehensive list).

So LP64 vs. ILP32, little endian vs. big endian, possibly alignment. FreeBSD has a wide range. (I currently have access only to examples of amd64 and armv6.)

Looking around NO_PRINT_LLD is not defined for freebsd:

# grep -i NO_PRINT_ /usr/obj/portswork/usr/ports/benchmarks/iozone/work/iozone3_434/src/current/makefile
		-DNAME='"OSFV3"' -DNO_PRINT_LLD -DOSF_64 $(CFLAGS) iozone.c \
		-DNO_PRINT_LLD  -DOSF_64 $(CFLAGS) libbif.c -o libbif.o
		-DNO_PRINT_LLD -DOSF_64 $(CFLAGS) libasync.c -o libasync.o
		-DNAME='"OSFV4"' -DNO_PRINT_LLD -DOSF_64 $(CFLAGS) iozone.c \
		-DNO_PRINT_LLD  -DOSF_64 $(CFLAGS) libbif.c -o libbif.o
		-DNO_PRINT_LLD -DOSF_64 $(CFLAGS) libasync.c -o libasync.o
		-DNAME='"OSFV5"' -DNO_PRINT_LLD -DOSF_64 $(CFLAGS) iozone.c \
		-DNO_PRINT_LLD  -DOSF_64 $(CFLAGS) libbif.c -o libbif.o
		-DNO_PRINT_LLD -DOSF_64 $(CFLAGS) libasync.c -o libasync.o
		-DNAME='"TRU64"' -DNO_PRINT_LLD -DOSF_64 -pthread $(CFLAGS) iozone.c \
		-DNO_PRINT_LLD  -DOSF_64 $(CFLAGS) libbif.c -o libbif.o
		-DNO_PRINT_LLD -DOSF_64 $(CFLAGS) libasync.c -o libasync.o

So only OSF_64 variants involve NO_PRINT_LLD being defined. So it appears a bunch of changes are irrelevant to FreeBSD but might be important for non-FreeBSD. I've not tried anything under non-FreeBSD.

Ignoring that for the moment relative to FreeBSD, take the example:

 #ifdef NO_PRINT_LLD
-			sscanf(optarg,"%ld",&kilobytes64);
+			sscanf(optarg,"%ld",(long long *)&kilobytes64);

%ld is not a long long format. It is a long int format. For ILP32 contexts long int and long long are not sized the same. There may not be examples yet but for LP64 long long could be more than 64-bits instead of matching long int's size.

Changing the type of the pointer where the value is to be stored is generally dangerous. The format must match the actual storage layout involved (including size, alignment). Mismatched type sizes between the format and the storage can get into endianness handling problems (format too small for the storage), truncation problems (format too small for the storage), memory overwrite problems (format too big for the storage). There could be alignment issues as well.

For the variations of scan: Having "full/larger-than-needed width" in the target type locally and then validating the values would fit in the desired final storage and then converting if things are okay would be more generic to variations like LP64 vs. IPL32 and such.

As for the printf variations:

+ #ifdef NO_PRINT_LLD
+-			sprintf(splash[splash_line++],"\tUsing minimum file size of %ld kilobytes.\n",minimum_file_size);
++			sprintf(splash[splash_line++],"\tUsing minimum file size of %ld (long long)kilobytes.\n",minimum_file_size);

This example added a "(long long)" to the string text. Why?

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

%ld is not a long long format. It is a long int format. For ILP32 contexts long int and long long are not sized the same. There may not be examples yet but for LP64 long long could be more than 64-bits instead of matching long int's size.

Incorrect value displays can happen when formats to not match storage layouts. Endianness handling problems (format too small for the storage), truncation problems (format too small for the storage), memory access problems (format too big for the storage). There could be alignment issues as well.


It appears to me the the scan code with %lld (a long long format) might also have some of these sorts of issues for various TARGET_ARCH's. The prints with %lld and a matching (long long) cast for the value likely survive until long or int happens to be be bigger than 64 bits.

(I've not looked for signed vs. unsigned issues.)
Comment 12 Walter Schwarzenfeld freebsd_triage 2016-07-17 01:31:57 UTC
Ok, I was complete wrong. Thanks, for detailed explanation.
Comment 13 Mark Millard 2016-07-17 02:36:56 UTC
(In reply to w.schwarzenfeld from comment #12)

It may not have been clear but when I wrote:

"The prints with %lld and a matching (long long) cast for the value likely survive until long or int happens to be be bigger than 64 bits."

I was indicating that those changes are basically good and would likely survive for a significant time.

A technical alternative for C code that is allowed to be modern enough is use of (intmax_t) casts and j based formats for printf variants.

(Similarly for local intmax_t storage and j formats for scan variants, where the intmax_t value is later range validated and conditionally converted into the intended storage if the value fits.)

I have no clue how old of a C variant iozone's source code intends to support. intmax_t might be too modern for simple direct use over iozone's intended C vintage range, at least viewed for source also spanning outside a FreeBSD context.
Comment 14 Walter Schwarzenfeld freebsd_triage 2016-07-17 05:07:49 UTC
(In reply to Mark Millard from comment #13)
The "%ld" casts were erronousely, I don't wanted, seems I was in a wrong line or so. The Makefile-hack was weak,
and I wrote it above.
Comment 15 Walter Schwarzenfeld freebsd_triage 2016-07-17 05:09:30 UTC
Created attachment 172600 [details]
svn-diff_benchmark_iozone
Comment 16 Walter Schwarzenfeld freebsd_triage 2016-07-17 05:10:41 UTC
Second try, I hope this patch is right now (if not I have to guess, I have a bigger     misunderstood and will let it be).
Comment 17 Mark Millard 2016-07-18 02:52:43 UTC
(In reply to w.schwarzenfeld from comment #16)

I do not have all that much time today or tomorrow for this but I took a quick look today.

The new iozone.c updates address most of my concerns. As far as FreeBSD amd64 and armv6 goes it appears to compile only with a complaint about CACHE_LINE_SIZE being redefined. I expect that powerpc, powerpc64, and the rest would likely get the same results. (There is a separate, earlier submittal (210857) reporting the CACHE_LINE_SIZE override.)

The only part that might not fit as an upstream update to the iozone.c source code might be the sscanf use of (long long *) casts if off64_t can ever have a different memory layout from long long. (Not a likely issue, even outside FreeBSD?)

So an updated /usr/ports/benchmarks/iozone/files/patch-iozone.c that has both its old changes to iozone.c and your new changes to iozone.c covered would be an improvement to the FreeBSD benchmarks/iozone port.
Comment 18 Walter Schwarzenfeld freebsd_triage 2016-07-18 04:24:12 UTC
Sorry, but I have no CACHE_LINE_SIZE redefined error message or warning.
Comment 19 Mark Millard 2016-07-18 05:05:26 UTC
(In reply to w.schwarzenfeld from comment #18)

The CACHE_LINE_SIZE issue is failed as a separate defect and you do not need to deal with it for this defect.

Just for your information if you care:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=210857 reports:

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)
        ^

(The line number in FreeBSD's param.h is possibly machine-type specific.)

FYI:

# grep CACHE_LINE_SIZE /usr/include/machine/param.h
 * CACHE_LINE_SIZE is the compile-time maximum cache line size for an
#define	CACHE_LINE_SIZE		(1 << CACHE_LINE_SHIFT)

(Both amd64 and armv6 show this grep result.)

If /usr/include/machine/param.h is (indirectly) included by the header file handling then there is likely a CACHE_LINE_SIZE macro definition present in FreeBSD based compiles for those architectures that have cache lines.

I do not know what compile context you use but I've been using the system clang 3.8.0 from 11.0 (-STABLE these days). Other compilers or compiler vintages may not necessarily report the redefinition by default.

My /etc/make.conf has just:

WANT_QT_VERBOSE_CONFIGURE=1
#
DEFAULT_VERSIONS+=perl5=5.22
WRKDIRPREFIX=/usr/obj/portswork
WITH_DEBUG=
WITH_DEBUG_FILES=
MALLOC_PRODUCTION=

If you care you could check your /usr/include/machine/param.h to see if it has a CACHE_LINE_SIZE define.

But, again, it is a separate submittal with distinct consequences/issues so you may want to ignore the potential CACHE_LINE_SIZE notices from some types of contexts completely.
Comment 20 Walter Schwarzenfeld freebsd_triage 2016-07-18 07:46:19 UTC
Created attachment 172627 [details]
patch-benchmarks_iozone__iozone_c
Comment 21 Walter Schwarzenfeld freebsd_triage 2016-07-18 07:47:50 UTC
Removed cast of sscanf  (leave the two warnings, as they are).
The "merge" of the two patches, does not work, for reasons I don't know.
But it works "together".
Comment 22 Walter Schwarzenfeld freebsd_triage 2016-07-18 08:37:59 UTC
Created attachment 172630 [details]
svn-diff_benchmark_iozone_update_3.444
Comment 23 Walter Schwarzenfeld freebsd_triage 2016-07-18 08:54:08 UTC
Found an update to 3.444. Make things a little more simpler (makes one of the two iozone.c patches obsolete)..
portlint OK.
Compiles fine with 103amd64, 93amd64, 11amd64, 93i386, 103i386 without problems and no addiional warnings.

Found no ChangeLog.
Comment 24 Mark Millard 2016-07-18 23:04:34 UTC
(In reply to w.schwarzenfeld from comment #23)

When I applied the new patch for 3.444 use to benchmarks/iozone in -r418253 of /user/ports the messages were normal (see later below after I show the build problem) but when I tried to build benchmarks/iozone it failed with problems for applying patch-libasync.c :

===>  Found saved configuration for iozone-3.434
===>   iozone-3.444 depends on file: /usr/local/sbin/pkg - found
===> Fetching all distfiles required by iozone-3.444 for building
===>  Extracting for iozone-3.444
=> SHA256 Checksum OK for iozone3_444.tgz.
===>  Patching for iozone-3.444
===>  Applying FreeBSD patches for iozone-3.444
No such line 1217 in input file, ignoring
32 out of 33 hunks failed--saving rejects to libasync.c.rej
=> Patch patch-libasync.c failed to apply cleanly.
=> Patch(es) patch-iozone.c applied cleanly.
*** Error code 1

Stop.
make[1]: stopped in /usr/ports/benchmarks/iozone
*** Error code 1

Stop.
make: stopped in /usr/ports/benchmarks/iozone

Your patch does not include anything for libasync.c and so leaves the older patch-libasync.c in place.

FYI. . .

Applying your patch to -r418253 of /usr/ports and its benchmarks/iozone reported:

Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: Makefile
|===================================================================
|--- Makefile	(revision 418690)
|+++ Makefile	(working copy)
--------------------------
Patching file Makefile using Plan A...
Hunk #1 succeeded at 2 with fuzz 1.
Hunk #2 succeeded at 12.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: distinfo
|===================================================================
|--- distinfo	(revision 418690)
|+++ distinfo	(working copy)
--------------------------
Patching file distinfo using Plan A...
Hunk #1 succeeded at 1.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: files/patch-iozone.c
|===================================================================
|--- files/patch-iozone.c	(revision 418690)
|+++ files/patch-iozone.c	(working copy)
--------------------------
Patching file files/patch-iozone.c using Plan A...
Hunk #1 succeeded at 1.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: files/patch-pit_server.c
|===================================================================
|--- files/patch-pit_server.c	(revision 418690)
|+++ files/patch-pit_server.c	(working copy)
--------------------------
Patching file files/patch-pit_server.c using Plan A...
Hunk #1 succeeded at 6.
done

(I cleaned out the 4 generated .orig files after this.)
Comment 25 Walter Schwarzenfeld freebsd_triage 2016-07-18 23:33:00 UTC
Created attachment 172682 [details]
svn-diff_benchmark_iozone_new
Comment 26 Walter Schwarzenfeld freebsd_triage 2016-07-18 23:33:47 UTC
Sorry, forgot to delete libaync-path from svn.
Comment 27 Mark Millard 2016-07-18 23:42:05 UTC
(In reply to w.schwarzenfeld from comment #26)

When I download this new patch I get:

svn-diff_security_racoon2.txt

with content that does not apply to benchmarks/iozone . It starts with:

Index: /usr/ports/security/racoon2/files/patch-pskgen-pskgen.in
===================================================================
--- /usr/ports/security/racoon2/files/patch-pskgen-pskgen.in	(revision 414516)
+++ /usr/ports/security/racoon2/files/patch-pskgen-pskgen.in	(working copy)
Comment 28 Walter Schwarzenfeld freebsd_triage 2016-07-18 23:49:23 UTC
Created attachment 172683 [details]
svn-diff_benchmark_iozone_3.444_new
Comment 29 Mark Millard 2016-07-19 00:09:54 UTC
(In reply to w.schwarzenfeld from comment #28)

[I've only tried amd64 so far, not armv6.]

The new patch created a zero length files/patch-libasync.c :

# ls -lt /usr/ports/benchmarks/iozone/files/
total 14
-rw-r--r--  1 root  wheel    126 Jul 18 16:56 patch-pit_server.c
-rw-r--r--  1 root  wheel      0 Jul 18 16:56 patch-libasync.c
-rw-r--r--  1 root  wheel  19945 Jul 18 16:56 patch-iozone.c

and that lead to:

===>  Found saved configuration for iozone-3.434
===>   iozone-3.444 depends on file: /usr/local/sbin/pkg - found
===> Fetching all distfiles required by iozone-3.444 for building
===>  Extracting for iozone-3.444
=> SHA256 Checksum OK for iozone3_444.tgz.
===>  Patching for iozone-3.444
===>  Applying FreeBSD patches for iozone-3.444
  I can't seem to find a patch in there anywhere.
=> Patch patch-libasync.c failed to apply cleanly.
=> Patch(es) patch-iozone.c applied cleanly.
*** Error code 1

Stop.
make[1]: stopped in /usr/ports/benchmarks/iozone
*** Error code 1

Stop.
make: stopped in /usr/ports/benchmarks/iozone

===>>> make build failed for benchmarks/iozone
===>>> Aborting update


Retrying after a manual . . .

# rm /usr/ports/benchmarks/iozone/files/patch-libasync.c 

did build. It reported what our past exchanges would indicate as expected for amd64:

cc -c -pipe  -g -fstack-protector -fno-strict-aliasing  -DFreeBSD -Dunix -Dbsd4_4 -DHAVE_ANSIC_C -DASYNC_IO  -DHAVE_PREAD -DNAME='"freebsd"' -DSHARED_MEM  -pipe  -g -fstack-protector -fno-strict-aliasing iozone.c -o iozone_freebsd.o
iozone.c:768:9: warning: 'CACHE_LINE_SIZE' macro redefined [-Wmacro-redefined]
#define CACHE_LINE_SIZE 32
        ^
/usr/include/machine/param.h:93:9: note: previous definition is here
#define CACHE_LINE_SIZE         (1 << CACHE_LINE_SHIFT)
        ^
iozone.c:1927:25: warning: format specifies type 'long long *' but the argument has type 'off64_t *' (aka 'long *') [-Wformat]
                        sscanf(optarg,"%lld",&kilobytes64);
                                       ~~~~  ^~~~~~~~~~~~
                                       %ld
iozone.c:2456:28: warning: format specifies type 'long long *' but the argument has type 'off64_t *' (aka 'long *') [-Wformat]
                                                sscanf(subarg,"%lld",&burst_size_kb_64);
                                                               ~~~~  ^~~~~~~~~~~~~~~~~
                                                               %ld
3 warnings generated.



Note: armv6 would not have the long long* vs. long * messages because off64_t (really off_t for FreeBSD) would translated to long long because long is only 32-bits for armv6. FreeBSD only uses long for such when long is 64-bits.

Non-FreeBSD might have a wider variety of off64_t definitions overall.
Comment 30 Walter Schwarzenfeld freebsd_triage 2016-07-19 01:12:48 UTC
> # rm /usr/ports/benchmarks/iozone/files/patch-libasync.c 

This is svn (and I hate it). Don't know why, maybe, someone other can correct it.
Comment 31 Mark Millard 2016-07-19 01:40:27 UTC
(In reply to w.schwarzenfeld from comment #30)

The issue was just me messing up: I used patch instead of "svnlite patch . . .".

Using svnlite patch worked fine.

(Of course the 3 warnings were produced.)
Comment 32 Kubilay Kocak freebsd_committer freebsd_triage 2016-07-19 10:05:39 UTC
If we can separate the update from the fix, that would ensure the fix can be merged to the quarterly branch.

Also:

* Are/can these changes be sent upstream?
* Pending QA results
Comment 33 Walter Schwarzenfeld freebsd_triage 2016-07-19 15:39:36 UTC
I would say we should take this patch and open on pipermail a discussion for the "portable way" of the sscanf format and CACHE_LINE_SIZE. I think on pipermail we got quicker a result as here on bugzilla.
Comment 34 Mark Millard 2016-07-19 20:42:44 UTC
(In reply to w.schwarzenfeld from comment #33)

It may be appropriate to also consider the sscanf issues and alternatives indicated in:

https://www.securecoding.cert.org/confluence/display/c/INT05-C.+Do+not+use+input+functions+to+convert+character+data+if+they+cannot+handle+all+possible+inputs

sscanf is not good for handling/avoiding problems in the text being converted well.


Has anyone contacted the upstream author ( capps@iozone.org ) to see what their criteria are for changes to go upstream? For example: what range of vintages of C are to be supported? That criteria might eliminate what I reference above for changes going upstream for all I know.
Comment 35 Mark Millard 2016-07-19 21:03:39 UTC
(In reply to Kubilay Kocak from comment #32)

I believe that the following part of the overall changes are all that was required to deal with the original build failure reported (iozone 434 or 444 vintages):

 typedef off_t off64_t;
 #endif
 
+#if defined(__FreeBSD__)
+#define __off64_t_defined
+typedef off_t off64_t;
+#endif
 
 #ifndef solaris
 #ifndef off64_t

and optionally (__off64_t_defined was sufficient to avoid the typedef):

 #ifndef SCO_Unixware_gcc
 #ifndef UWIN
 #ifndef __DragonFly__
+#ifndef __FreeBSD__
 typedef long long off64_t;
 #endif
 #endif

with its matching:

 #endif
 #endif
 #endif
+#endif

But the __DragonFly__ in the original code base was also optional because of
__off64_t_defined already being defined for that context.
So the above optional part-pair maintains the style of handling in the original.

One possibility for the CACHE_LINE_SIZE issue for FreeBSD is:

+#if !defined(CACHE_LINE_SIZE)
 #define CACHE_LINE_SIZE 32
+#endif



Has anyone contacted the upstream author ( capps@iozone.org ) to see what their criteria are for changes to go upstream? For example: what range of vintages of C are to be supported?
Comment 36 Walter Schwarzenfeld freebsd_triage 2016-07-19 22:23:10 UTC
Created attachment 172736 [details]
svn-diff_benchmark_iozone_3.444_next
Comment 37 Walter Schwarzenfeld freebsd_triage 2016-07-19 22:23:45 UTC
Ok, I changed this.
Comment 38 Kubilay Kocak freebsd_committer freebsd_triage 2016-07-20 08:31:40 UTC
@Walter, can we separate the version update from the fix? 

Please also provide confirmation of QA passing

Thank you again for helping to fix this unmaintained port
Comment 39 Walter Schwarzenfeld freebsd_triage 2016-07-20 10:50:31 UTC
(In reply to Kubilay Kocak from comment #38)
It was "simpler" to make it with the verision update (I wrote it above).
Yes, I confirm.
Comment 40 Kubilay Kocak freebsd_committer freebsd_triage 2016-07-20 12:05:30 UTC
Can't merge quarterly version updates
Comment 41 Kubilay Kocak freebsd_committer freebsd_triage 2016-07-20 12:06:10 UTC
Comment on attachment 172736 [details]
svn-diff_benchmark_iozone_3.444_next

Port is unmaintained, implicit approval
Comment 42 Mark Millard 2016-07-20 17:50:02 UTC
Notes from Don Capps (the upstream owner)



Most recent note:

	Revision 3.446 will appear on the Iozone web site within 24 hours.  



Slightly older material:

	The changes for :
	* FreeBSD handling of definition of __off_t 
	* Definition CACHE_LINE_SIZE changed to MY_CACHE_LINE_SIZE
	Have been merged into the official Iozone source tree and will
appear in 3.445 and later releases.

	It is unclear how you wish to handle the printf and scanf issues
that are related to long longs.  It would be not be acceptable to change the
format so that only more modern compilers would work.  Many of the systems
that compile and run Iozone span 2 decades of compilers.  Breaking backwards
compatibility would not be permitted.

	If you have a way to handle this without breaking previous builds,
please send the diffs, and we'll put this in the queue of pending merges. I
suspect that we may be doing a bunch of casting, so that we can maintain
full backwards compatibility.  



I'll note that 2016-20==1996 so anything post-dating C89/C90 is unacceptable for upstream. And anything not commonly implemented a few years after C89/C90 would also likely be unacceptable.
Comment 43 Walter Schwarzenfeld freebsd_triage 2016-07-20 17:56:39 UTC
There is a commit for 3.434
https://svnweb.freebsd.org/ports?view=revision&revision=418841
Comment 44 Mark Millard 2016-07-20 18:21:09 UTC
(In reply to w.schwarzenfeld from comment #43)

https://svnweb.freebsd.org/ports?view=revision&revision=418841 is broken on armv6, powerpc and all other ILP32 FreeBSD architectures:

-typedef long long off64_t;
+typedef long off64_t;

long is not 64-bits but only 32-bits for ILP32 architectures.
Comment 45 Walter Schwarzenfeld freebsd_triage 2016-07-20 18:26:06 UTC
Someone other writes this patch. (I am not sure , but I doubt it is the right solution).
Comment 46 Jan Beich freebsd_committer freebsd_triage 2016-07-20 19:42:05 UTC
(Assign to the new maintainer since ports r418841.)
Comment 47 Mark Millard 2016-07-21 08:46:50 UTC
3.446 is out and http://www.iozone.org/src/current/iozone.c has:

#if defined(__FreeBSD__)
#define __off64_t_defined
typedef off_t off64_t;
#endif

(before dragonfly's equivalent)

It has:

#ifndef solaris
#ifndef off64_t
#ifndef _OFF64_T
#ifndef __AIX__
#ifndef __off64_t_defined
#ifndef SCO_Unixware_gcc
#ifndef UWIN
#ifndef __DragonFly__
#ifndef __FreeBSD__
typedef long long off64_t;
#endif
#endif
#endif
#endif
#endif
#endif
#endif
#endif
#endif

(So __FreeBSD__ was added here as well, although the __off64_t_defined would have covered it.)

It has:

#define MY_CACHE_LINE_SIZE 32

so CACHE_LINE_SIZE from FreeBSD will no longer be redefined.


It generally does not have %lld related casting changes from what I see.
Comment 48 Mark Millard 2016-07-21 08:56:20 UTC
(In reply to Mark Millard from comment #47)

FYI for the 3.446 update:

[Latest tarball] [Latest files] [Stable tarball] [Stable files]

on http://www.iozone.org all link into http://www.iozone.org/src/current/ but http://www.iozone.org/src/stable/ is older (2009).

It appears that going to http://www.iozone.org/src/stable/ is not currently the right thing to do.
Comment 49 Mark Millard 2016-07-22 10:16:44 UTC
(In reply to Mark Millard from comment #44)

Here is what the updated iozone fails like on 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]
                        sscanf(optarg,"%ld",&kilobytes64);
                                       ~~~  ^~~~~~~~~~~~
                                       %lld
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);
                                                                          ~~~       ^~~~~~~~~~~
                                                                          %lld
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);
                                                                                    ~~~               ^~~~~~~~~~~~~~~~~
                                                                                    %lld

powerpc (32-bit) likely would get the same sort of messages, including the error. The same for other ILP32 FreeBSD architectures.
Comment 50 Mark Millard 2016-07-24 01:54:59 UTC
(In reply to Mark Millard from comment #48)

http://www.iozone.org/src/stable/ has been up-dated to have the 3.446 source.
Comment 51 Mark Millard 2016-07-24 03:05:57 UTC
(In reply to Josh Paetzel from 211289's comment #4)

[I've copied this here as more interested parties are in the CC list.)

>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 52 Walter Schwarzenfeld freebsd_triage 2016-07-28 19:47:52 UTC
I think this one could be closed, with the update to 3.457.