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

portabilty fixes for gcc 7.2 and Ubuntu 17.10 i686 #1296

Merged
merged 8 commits into from Dec 1, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Nov 30, 2017

I trashed my OpenBox Ubuntu VM and had to reinstall. For fun I chose a newer Ubuntu 32 bit image. This PR addresses some issues I ran into.

I'm not completely sure that the error tolerance increase I added to the cronodate unit test is completely above board, so that one could use some validation.

I have two other problems that I'll open issues on:

  • valgrind sharness test detects an invalid read of size 1 in libev/ev.c::3634, which is a use of the ECB_MEMORY_FENCE macro.
  • lua affinity test 16: large value to new causes numeric overflow is failing with "expected numeric overflow, got 0xff".

for now I'm just going to work around those.

garlick added some commits Nov 30, 2017

libsubprocess/zio: address parentheses warning
Problem: gcc 7.2.9 (Ubuntu 7.2.0-8ubuntu3) complains:

zio.c: In function ‘zio_destroy’:
zio.c:218:13: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
     assert (z->magic = ~ZIO_MAGIC);
             ^
zio.c: In function ‘zio_allocate’:
zio.c:251:13: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
     assert (z->magic = ZIO_MAGIC);
             ^

Add an extra set of parens there.
build: suppress parentheses warnings in liblsd
Problem: gcc 7.2.0 (Ubuntu 7.2.0-8ubuntu3) complains:

list.c: In function ‘list_create’:
list.c:231:12: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
     assert(l->magic = LIST_MAGIC);      /* set magic via assert abuse */
            ^

Update AM_CPPFLAGS to suppress these warnings for all of liblsd.
build: suppress parentheses warnings in libutil
Problem: gcc 7.2.0 (Ubuntu 7.2.0-8ubuntu3) complains:

base64.c: In function ‘base64_encode_final’:
base64.c:187:13: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
     assert (x->finalized = 1);
             ^
Update AM_CPPFLAGS to suppress these warnings for all of libutil.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 30, 2017

Hmm apparently this is a new warning option? I'll just see if I can address the pointer comparison warnings in hostlist.c directly so we can avoid the compiler option.

CC       flux-lua.lo
cc1: error: -Werror=pointer-compare: no option -Wpointer-compare
cc1: error: unrecognized command line option "-Wno-pointer-compare" [-Werror]
cc1: all warnings being treated as errors
make[5]: *** [flux-lua.lo] Error 1
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 30, 2017

In the valgrind test instead of using a heuristic to find the "real" libtool binary for the broker, @dun found how to use the libtool command to run the correct executable (it may be tricky to run under our flux(1) binary first, but maybe worth a try)

See the first answer to this stackoverflow question:

https://stackoverflow.com/questions/14136681/running-valgrind-on-project-compiled-with-autotools-outputs-multiple-heap-summar

garlick added some commits Nov 30, 2017

bindings/lua: address compiler warnings in hostlist.c
Problem: gcc 7.2.0 (Ubuntu 7.2.0-8ubuntu3) complains:

lua-hostlist/hostlist.c:372:48: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
     while (**str != '\0' && strchr(sep, **str) != '\0')
                                                ^~
lua-hostlist/hostlist.c:1066:12: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
     assert(new->magic = HOSTLIST_MAGIC);
            ^
Update AM_CPPFLAGS to suppress the parentheses warning for all of
bindings/lua, and address the pointer-compare warning directly.
libutil/cronodate/test: reduce precision to 10us
Problem: in virtualBox 32-bit Ubuntu 17.10 i386 VM
environment, the cronodate test fails consistently here:

not ok 157 - cronodate_remaining works: got 3599.700s
  Failed test 'cronodate_remaining works: got 3599.700s'
  at test/cronodate.c line 275.

After increasing the precision of the debug output, the
actual value is shown to be 3599.7000010013580322 compared
to the expected 3599.700s.  The almost_is() function requires
the error to be less than 1e-6 (+/-1uS).  The actual value
exceeds that by around a nanosecond.

Reduce the margin accepted by almost_is() to 1e-5 (+/-10 uS).
libflux/test: address bool-operation warning
Problem: gcc 7.2.0 (Ubuntu 7.2.0-8ubuntu3) complains:

test/reactor.c: In function ‘oneshot’:
test/reactor.c:221:17: error: increment of a boolean expression [-Werror=bool-operation]
     oneshot_runs++;
                 ^~

Change variable from a bool to an int.
t/t5000-valgrind.t: run broker with "libtool e"
Problem: valgrind script fails due to missing lt-flux-broker
on libtool 2.4.6 i686.

The lt- prefix on .libs executables is missing on this platform.
libtool docs say it may or may not be present.

Use libtool --mode=execute (abbreviated as "libtool e") to get
the same effect portably, as described in the Libtool manual,
Section 3.4: Debugging Executables.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 30, 2017

That worked great! I'm going to force a push which changes the valgrind patch, and also the one that introduced the non-backwards-portable compiler option.

@garlick garlick force-pushed the garlick:gcc_72 branch from df32059 to 2f9144f Nov 30, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2017

Coverage Status

Changes Unknown when pulling 2f9144f on garlick:gcc_72 into ** on flux-framework:master**.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 30, 2017

Codecov Report

Merging #1296 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1296      +/-   ##
==========================================
+ Coverage   78.01%   78.26%   +0.24%     
==========================================
  Files         154      154              
  Lines       29104    27717    -1387     
==========================================
- Hits        22706    21693    -1013     
+ Misses       6398     6024     -374
Impacted Files Coverage Δ
src/common/libsubprocess/zio.c 74.4% <100%> (ø) ⬆️
src/bindings/lua/lua-hostlist/hostlist.c 62.73% <100%> (ø) ⬆️
src/bindings/lua/lua-affinity/lua-affinity.c 97.52% <100%> (ø) ⬆️
src/cmd/builtin/heaptrace.c 57.44% <0%> (-13.74%) ⬇️
src/common/libflux/heartbeat.c 81.81% <0%> (-5.69%) ⬇️
src/modules/connector-local/local.c 72.74% <0%> (-3.38%) ⬇️
src/modules/barrier/barrier.c 77.3% <0%> (-1.81%) ⬇️
src/common/libkvs/kvs_watch.c 87.55% <0%> (-0.89%) ⬇️
src/common/libutil/dirwalk.c 93.57% <0%> (-0.72%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
... and 8 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 30, 2017

I'm not completely sure that the error tolerance increase I added to the cronodate unit test is completely above board, so that one could use some validation.

I'm not sure what to think about the error tolerance increase. It seems perfectly fine to me, but I wonder why it was necessary. For now your change seems reasonable.

bindings/lua/lua-affinity: improve overflow detection
Problem: lua-affinity overflow test fails on 32-bit i686
system, returning 0xff instead of an overflow value.

On this system MAX_LUAINT is 0xfffff0 (2^24 - 16).
The overflow test value is 0xffffffffffffffff (2^64 - 1).
Overflow occurs if the lua_tointeger() >= MAX_LUAINT.

The lua5.1 documentation says of lua_tointeger() that
"if the a number is not an integer it is truncated
in some unspecified way".

Assuming a number that is too large to represent as
an integer might be considered "not an integer",
perhaps that explains the odd result of 0xff?

Since lua_tonumber() returns a double with greater
range than lua_tointeger(), add a check that the
two functions return equal values.  They should
match if the number is a valid integer.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 1, 2017

I just tacked on a proposed fix for the lua-affinity problem, which was solved another way for raspbian 32-bit in #975 but apparently didn't help here?

@grondo if this passes the ha ha test I will submit to the lua-affinity project as I'm sure you'd prefer.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 1, 2017

Coverage Status

Changes Unknown when pulling 80f08d9 on garlick:gcc_72 into ** on flux-framework:master**.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 1, 2017

@grondo if this passes the ha ha test I will submit to the lua-affinity project as I'm sure you'd prefer.

Seems like a good solution to me. Sorry about all the issues you've had to fix in that code! :-(

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 1, 2017

NP! OK I submitted grondo/lua-affinity#3.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 1, 2017

Thanks, merged it!

This PR ready too?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 1, 2017

Sure, thanks.

@grondo grondo merged commit 54205c6 into flux-framework:master Dec 1, 2017

5 checks passed

buildbot/core_standard Build done.
Details
codecov/patch 100% of diff hit (target 78.01%)
Details
codecov/project 78.26% (+0.24%) compared to ad747cf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 78.593%
Details

@garlick garlick deleted the garlick:gcc_72 branch Apr 2, 2018

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.