Bug 210855 - benchmarks/bonnie++ : use of volatile char messes up EOF comparison; more (armv6 with -mcpu=cortex-a7 for/on rpi2) - update to 1.98
Summary: benchmarks/bonnie++ : use of volatile char messes up EOF comparison; more (ar...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Ganael LAPLANCHE
URL: http://beefy8.nyi.freebsd.org/data/he...
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2016-07-05 20:56 UTC by Mark Millard
Modified: 2019-09-05 20:19 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (kuriyama)


Attachments
svn-diff-bonnie++ (6.39 KB, patch)
2019-09-02 11:59 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff-bonnie++_v2 (6.32 KB, patch)
2019-09-03 05:53 UTC, Walter Schwarzenfeld
no flags Details | Diff
patch v3 (3.01 KB, patch)
2019-09-03 17:55 UTC, Ganael LAPLANCHE
no flags Details | Diff
patch-getc_putc.cpp (720 bytes, text/plain)
2019-09-04 01:52 UTC, Walter Schwarzenfeld
no flags Details
patch-getc_putc_helper.cpp (413 bytes, text/plain)
2019-09-04 01:52 UTC, Walter Schwarzenfeld
no flags Details
patch-getc_putc (1.31 KB, patch)
2019-09-04 23:06 UTC, Walter Schwarzenfeld
no flags Details | Diff
Patch v4 (4.78 KB, patch)
2019-09-05 10:17 UTC, Ganael LAPLANCHE
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Millard 2016-07-05 20:56:55 UTC
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))
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
Comment 1 Mark Millard 2016-08-02 08:37:12 UTC
The build was of bonnie++-1.97_3 --which is still in place as of /usr/ports/ -r419343 .
Comment 2 Mark Linimon freebsd_committer freebsd_triage 2017-02-03 12:56:24 UTC
This error is still seen as of 20170128, although the port does build.
Comment 3 Walter Schwarzenfeld 2018-03-12 23:32:39 UTC
ping!
Comment 4 Walter Schwarzenfeld 2019-09-02 11:24:05 UTC
"New" version 1.98

https://doc.coker.com.au/projects/bonnie/
Comment 5 Walter Schwarzenfeld 2019-09-02 11:59:15 UTC
Created attachment 207097 [details]
svn-diff-bonnie++

Update to 1.98. Should solve the problem.
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-03 01:18:31 UTC
^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)
Comment 7 Walter Schwarzenfeld 2019-09-03 05:34:32 UTC
(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.
Comment 8 Walter Schwarzenfeld 2019-09-03 05:53:12 UTC
Created attachment 207131 [details]
svn-diff-bonnie++_v2

Two small changes cause of portlint.
Comment 9 Ganael LAPLANCHE freebsd_committer freebsd_triage 2019-09-03 17:54:28 UTC
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 ?
Comment 10 Ganael LAPLANCHE freebsd_committer freebsd_triage 2019-09-03 17:55:02 UTC
Created attachment 207156 [details]
patch v3
Comment 11 Walter Schwarzenfeld 2019-09-03 18:49:03 UTC
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.
Comment 12 Walter Schwarzenfeld 2019-09-04 01:52:18 UTC
Created attachment 207172 [details]
patch-getc_putc.cpp
Comment 13 Walter Schwarzenfeld 2019-09-04 01:52:56 UTC
Created attachment 207173 [details]
patch-getc_putc_helper.cpp
Comment 14 Walter Schwarzenfeld 2019-09-04 01:53:59 UTC
I think we need this two patches additional.

bonnie++.cpp:709 and bon_io.cpp:161 are solved in the code.
Comment 15 Ganael LAPLANCHE freebsd_committer freebsd_triage 2019-09-04 20:21:06 UTC
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.
Comment 16 Walter Schwarzenfeld 2019-09-04 20:37:07 UTC
So change it the cast to char or you want a new patch?
Comment 17 Walter Schwarzenfeld 2019-09-04 20:38:01 UTC
The problem is I got no warnings, so I make a "blind-patch".
Comment 18 Mark Millard 2019-09-04 20:49:52 UTC
(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
Comment 19 Mark Millard 2019-09-04 20:58:56 UTC
(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.
Comment 20 Walter Schwarzenfeld 2019-09-04 23:06:04 UTC
Created attachment 207198 [details]
patch-getc_putc

I put both patches in one. I hope this is right.
Comment 21 Ganael LAPLANCHE freebsd_committer freebsd_triage 2019-09-05 10:16:18 UTC
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 ?
Comment 22 Ganael LAPLANCHE freebsd_committer freebsd_triage 2019-09-05 10:17:18 UTC
Created attachment 207210 [details]
Patch v4

Full patch, v4
Comment 23 Walter Schwarzenfeld 2019-09-05 11:55:07 UTC
Ok, seems I misunderstood the macro.
Comment 24 commit-hook freebsd_committer freebsd_triage 2019-09-05 20:17:40 UTC
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
Comment 25 Ganael LAPLANCHE freebsd_committer freebsd_triage 2019-09-05 20:19:32 UTC
I've committed the patch, thanks for your help and feedback!