Bug 210855
| Summary: | benchmarks/bonnie++ : use of volatile char messes up EOF comparison; more (armv6 with -mcpu=cortex-a7 for/on rpi2) - update to 1.98 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Mark Millard <marklmi26-fbsd> | ||||||||||||||||
| Component: | Individual Port(s) | Assignee: | Ganael LAPLANCHE <martymac> | ||||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||||
| Severity: | Affects Some People | CC: | linimon, w.schwarzenfeld | ||||||||||||||||
| Priority: | --- | Keywords: | needs-qa | ||||||||||||||||
| Version: | Latest | Flags: | bugzilla:
maintainer-feedback?
(kuriyama) |
||||||||||||||||
| Hardware: | Any | ||||||||||||||||||
| OS: | Any | ||||||||||||||||||
| URL: | http://beefy8.nyi.freebsd.org/data/head-armv6-default/p432094_s312610/logs/bonnie++-1.97.3.log | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
The build was of bonnie++-1.97_3 --which is still in place as of /usr/ports/ -r419343 . This error is still seen as of 20170128, although the port does build. ping! "New" version 1.98 https://doc.coker.com.au/projects/bonnie/ Created attachment 207097 [details]
svn-diff-bonnie++
Update to 1.98. Should solve the problem.
^Triage: Note: maintain timeouts (and assignee reset) apply from the date a proposal (patch) is submitted, not the original date of the issue/report @Ganael: I would wait for until 019-09-16 (14 days) to commit the (QA'd) patch with: Approved by: portmgr (maintainer timeout: 14 days) (In reply to Walter Schwarzenfeld from comment #5) ChangeLog: bonnie++ (1.98) unstable; urgency=medium * Fixed macros in bon_csv2html.cpp that had lower case due to excessive matching on a regex. * Changed debian/compat to level 10 * Allow specifying the number of random seeks and the number of seeker processes and store that in the CSV. * Changed bon_csv2html and bon_csv2txt to take the new CSV format. * Changed the text output to use KiB/MiB/GiB as units of measurement so we can fit NVMe results on screen. * Changed the HTML to be in MiB/s for the bulk IO tests. * Changed the default size for getc_putc to work with faster CPUs and more optimised libc. Created attachment 207131 [details]
svn-diff-bonnie++_v2
Two small changes cause of portlint.
Hello Walter, Thanks for your patch. Unfortunately, I could not build the port in poudriere due to patch-Makefile.in being removed. Find attached a v3 patch, where : - PORTDOCS, files/patch-Makefile.in and pkg-plist are preserved - as well as the two last chunks of patch-bonnie++.cpp - files/patch-bon_time.cpp has been introduced It builds fine and works on amd64. Can you tell me if it fixes the original bug ? Created attachment 207156 [details]
patch v3
Sorry no
the comparision is unchanged in the code:
TEST_FUNC_READ("getc_unlocked()", if( (c = getc_unlocked(fp)) == EOF), res[GetcUnlocked]);
I overlooked it, cause I got no warning.
Created attachment 207172 [details]
patch-getc_putc.cpp
Created attachment 207173 [details]
patch-getc_putc_helper.cpp
I think we need this two patches additional. bonnie++.cpp:709 and bon_io.cpp:161 are solved in the code. Hello Walter,
I still get warnings (on i386 and amd64) with your two patches :
getc_putc.cpp:209:47: warning: result of comparison of constant 255 with expression of type 'volatile char' is always false [-Wtautological-constant-out-of-range-compare]
TEST_FUNC_READ("getc()", if( (c = getc(fp)) == (unsigned char)EOF), res[Getc]);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
getc_putc.cpp:224:65: warning: result of comparison of constant 255 with expression of type 'volatile char' is always false [-Wtautological-constant-out-of-range-compare]
TEST_FUNC_READ("getc_unlocked()", if( (c = getc_unlocked(fp)) == (unsigned char)EOF), res[GetcUnlocked]);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
getc_putc_helper.cpp:48:57: warning: result of comparison of constant 255 with expression of type 'volatile char' is always false [-Wtautological-constant-out-of-range-compare]
TEST_FUNC_READ("getc() no thread", if( (c = getc(fp)) == (unsigned char)EOF), res[1]);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Why not just casting EOF to char (not unsigned), as variable 'c' is declared ? The test is not perfect (it should probably use feof(3)) but the casting is already done when the result of getc(fp) gets stored into c, so we should be comparing the same things.
So change it the cast to char or you want a new patch? The problem is I got no warnings, so I make a "blind-patch". (In reply to Ganael LAPLANCHE from comment #15) powerpc64 and powerpc have char as unsigned. Most contexts have char signed. Using (char) EOF produces a value that is a valid character encoding, making that character value be treated as EOF marker. The design of the library is that EOF is an out of range value relative to char instead. That is why the wider int type is used for the return value of getc and for EOF. The prototype for getc is: int getc( FILE *stream ); (It does not return a char.) As for EOF : QUOTE integer constant expression of type int and negative value (macro constant) END QUOTE (In reply to Mark Millard from comment #18) [I was interrupted and sent what I had.) The correct order is: int temp= getfc(fs); if (temp!=EOF) char_value= (char)temp; as far as extracting the character value goes. The EOF test comes before the conversion. Created attachment 207198 [details]
patch-getc_putc
I put both patches in one. I hope this is right.
Hello, Thanks to both of you for your help. TEST_FUNC is defined in getc_putc.h. From what I understand, it should use a code (XCODE) that uses the function being tested because it loops over it until size is reached or XCODE test is true. In your patch, you will call getc() or getc_unlocked() only once before the loop and test the value of b that will never change. I have reworked the patch and it should be better now. Can you tell me if it seems OK to you ? Created attachment 207210 [details]
Patch v4
Full patch, v4
Ok, seems I misunderstood the macro. A commit references this bug: Author: martymac Date: Thu Sep 5 20:16:58 UTC 2019 New revision: 511264 URL: https://svnweb.freebsd.org/changeset/ports/511264 Log: Update to 1.98 and fix type conversion warnings PR: 210855 Submitted by: Mark Millard <marklmi26-fbsd@yahoo.com> Reviewed by: Walter Schwarzenfeld <w.schwarzenfeld@utanet.at> Changes: head/benchmarks/bonnie++/Makefile head/benchmarks/bonnie++/distinfo head/benchmarks/bonnie++/files/patch-bon_time.cpp head/benchmarks/bonnie++/files/patch-bonnie++.cpp head/benchmarks/bonnie++/files/patch-getc_putc.cpp head/benchmarks/bonnie++/files/patch-getc_putc_helper.cpp I've committed the patch, thanks for your help and feedback! |
The main report is from what the building compiler reported: getc_putc.cpp:210:47: warning: comparison of constant -1 with expression of type 'volatile char' is always false [-Wtautological-constant-out-of-range-compare] TEST_FUNC_READ("getc()", if( (c = getc(fp)) == EOF), res[Getc]); ~~~~~~~~~~~~~~ ^ ~~~ ./getc_putc.h:26:31: note: expanded from macro 'TEST_FUNC_READ' TEST_FUNC("Reading", XNAME, XCODE, XRES) ^~~~~ ./getc_putc.h:13:5: note: expanded from macro 'TEST_FUNC' XCODE \ ^~~~~ getc_putc.cpp:225:65: warning: comparison of constant -1 with expression of type 'volatile char' is always false [-Wtautological-constant-out-of-range-compare] TEST_FUNC_READ("getc_unlocked()", if( (c = getc_unlocked(fp)) == EOF), res[GetcUnlocked]); ~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~ ./getc_putc.h:26:31: note: expanded from macro 'TEST_FUNC_READ' TEST_FUNC("Reading", XNAME, XCODE, XRES) ^~~~~ ./getc_putc.h:13:5: note: expanded from macro 'TEST_FUNC' XCODE \ ^~~~~ getc_putc_helper.cpp:48:57: warning: comparison of constant -1 with expression of type 'volatile char' is always false [-Wtautological-constant-out-of-range-com pare] TEST_FUNC_READ("getc() no thread", if( (c = getc(fp)) == EOF), res[1]); ~~~~~~~~~~~~~~ ^ ~~~ ./getc_putc.h:26:31: note: expanded from macro 'TEST_FUNC_READ' TEST_FUNC("Reading", XNAME, XCODE, XRES) ^~~~~ ./getc_putc.h:13:5: note: expanded from macro 'TEST_FUNC' XCODE \ ^~~~~ getc returns a wider type than char variants and signed even if char is not: it returns an int. This is part of enabling the EOF test protocol without reserving character values. bon_io.cpp:161:76: warning: format specifies type 'long' but the argument has type 'off_t' (aka 'long long') [-Wformat] sprintf(m_buf, "Error in lseek to chunk %d(" OFF_T_PRINTF ")", offset, real_offset); ^~~~~~~~~~~ The width mismatch leads to big-endian vs. little-endian (vs. pdp-endian) issues. powerpc, for example, being big-endian but 32 bit could have problems with just the "high half" being used here. Side notes: bonnie++.cpp:709:57: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare] if(globals.ram && directory_size * MaxDataPerFile * 2 > (globals.ram << 10)) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~