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

update to libev 4.25, ensure valgrind runs clean on i686 #1898

Merged
merged 5 commits into from Dec 29, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Dec 28, 2018

This PR includes a gratuitous update of libev from 4.24 to 4.25. I pulled it in to see if it would fix #1897 because it happened to touch the ECB_MEMORY_FENCE macros, but then left it in since the changes were fairly minor and it's probably useful to track upstream releases. (maybe the darwin fix is useful when @trws gets back to flux mac port?)

   4.25 Fri Dec 21 07:49:20 CET 2018
      - INCOMPATIBLE CHANGE: EV_THROW was renamed to EV_NOEXCEPT
        (EV_THROW sitll provided) and now uses noexcept on C++11 or newer.
      - move the darwin select workaround higher in ev.c, as newer versions of
        darwin managed to break their broken select even more.
      - ANDROID => __ANDROID__ (reported by enh@google.com).
      - disable epoll_create1 on android because it has broken header files
        and google is unwilling to fix them (reported by enh@google.com).
      - avoid a minor compilation warning on win32.
      - c++: remove deprecated dynamic throw() specifications.
      - c++: improve the (unsupported) bad_loop exception class.
      - backport perl ev_periodic example to C, untested.
      - update libecb, biggest change is to include a memory fence
        in ECB_MEMORY_FENCE_RELEASE on x86/amd64.
      - minor autoconf/automake modernisation.

The other fixes are to get the valgrind test to run successfully on an i686 test box. I expect the shutdown grace increase and FLUX_LOCAL_CONNECTOR_RETRY_COUNT fix may improve reliability in travis also. The shutdown grace increase does slow down the test some.

garlick added some commits Dec 28, 2018

libev: update embedded library to version 4.25
Update libev from 4.24 to 4.25.

libev embedding instructions:
  http://pod.tst.eu/http://cvs.schmorp.de/libev/ev.pod#FILESETS

libev Changes file entry
4.25 Fri Dec 21 07:49:20 CET 2018
  - INCOMPATIBLE CHANGE: EV_THROW was renamed to EV_NOEXCEPT
    (EV_THROW sitll provided) and now uses noexcept on C++11 or newer.
  - move the darwin select workaround highe rin ev.c, as newer versions of
    darwin managed to break their broken select even more.
  - ANDROID => __ANDROID__ (reported by enh@google.com).
  - disable epoll_create1 on android because it has broken header files
    and google is unwilling to fix them (reported by enh@google.com).
  - avoid a minor compilation warning on win32.
  - c++: remove deprecated dynamic throw() specifications.
  - c++: improve the (unsupported) bad_loop exception class.
  - backport perl ev_periodic example to C, untested.
  - update libecb, biggets change is to include a memory fence
    in ECB_MEMORY_FENCE_RELEASE on x86/amd64.
  - minor autoconf/automake modernisation.
testsuite: add valgrind suppression
Problem: libev triggers a valgrind invalid read error on i686.

==18884== Invalid read of size 1
==18884==    at 0x4883F75: ev_run (ev.c:3634)
==18884==    by 0x4850086: flux_reactor_run (reactor.c:140)
==18884==    by 0x4859884: flux_future_wait_for (future.c:483)
==18884==    by 0x48599E0: flux_future_get (future.c:510)
==18884==    by 0x877C6B5: op_event_subscribe (shmem.c:120)
==18884==    by 0x484F07D: flux_event_subscribe (handle.c:774)
==18884==    by 0x113822: register_event (modservice.c:201)
==18884==    by 0x113822: modservice_register (modservice.c:238)
==18884==    by 0x111679: module_thread (module.c:144)
==18884==    by 0x4A0A3BC: start_thread (pthread_create.c:463)
==18884==    by 0x4B87E15: clone (clone.S:108)
==18884==  Address 0x7f72c5f is on thread 5's stack
==18884==  1 bytes below stack pointer
==18884==

ev.c:3634 corresponds to an ECP_MEMORY_FENCE call.

The libev author claims this is an innocuous error here

http://lists.schmorp.de/pipermail/libev/2013q2/002173.html

so just add a suppression.

Fixes #1897
broker: fix mismatched varargs
Problem: on i686, valgrind reports a memory init error
in signal_cb() -> shutdown_arm().

==16803== Conditional jump or move depends on uninitialised value(s)
==16803==    at 0x4AD879B: vfprintf (vfprintf.c:1642)
==16803==    by 0x4B9683C: __vsnprintf_chk (vsnprintf_chk.c:63)
==16803==    by 0x116430: vsnprintf (stdio2.h:77)
==16803==    by 0x116430: shutdown_vencode (shutdown.c:157)
==16803==    by 0x1166FD: shutdown_arm (shutdown.c:206)
==16803==    by 0x10F8A3: signal_cb (broker.c:1691)
==16803==    by 0x484F7DF: signal_cb (reactor.c:1004)
==16803==    by 0x4881152: ev_invoke_pending (ev.c:3322)
==16803==    by 0x488466C: ev_run (ev.c:3726)
==16803==    by 0x4850086: flux_reactor_run (reactor.c:140)
==16803==    by 0x10E5CE: main (broker.c:628)
==16803==

The shutdown_arm() format string expects 3 args but only two
are provided.  Drop the third token from the format string.
testsuite: fix environment in valgrind test
Problem: flux module load in rc1 times out on slow
i686 system.

FLUX_LOCAL_CONNECTOR_RETRY_COUNT is set in the environment
of the test script in an attempt to compensate for slow
socket creation when running under valgrind, but the variable
is not inherited by the shell running rc1 script.

Add "export" to the variable setting.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 28, 2018

Rerunning this, it seems I didn't quite make the shutdown grace long enough to be reliable. I'll bump it once more.

testsuite: increase shutdown grace in valgrind test
Problem: valgrind test fails to shutdown cleanly
on i686 test system.

Increase shutdown grace period from 4s to 16s.

@garlick garlick force-pushed the garlick:libev_4.25 branch from de3a70c to 32de749 Dec 28, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 28, 2018

Codecov Report

Merging #1898 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1898      +/-   ##
==========================================
+ Coverage   80.13%   80.14%   +<.01%     
==========================================
  Files         196      196              
  Lines       35064    35064              
==========================================
+ Hits        28098    28101       +3     
+ Misses       6966     6963       -3
Impacted Files Coverage Δ
src/broker/broker.c 77.41% <ø> (ø) ⬆️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
src/broker/modservice.c 78.84% <0%> (-0.97%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.24%) ⬇️
src/common/libflux/message.c 81.64% <0%> (+0.12%) ⬆️
src/modules/connector-local/local.c 74.81% <0%> (+1.03%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 28, 2018

This seems reasonable. Need someone to press the button?

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Dec 29, 2018

I don't know the valgrind suppress stuff that well, but LGTM too.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 29, 2018

Sure if someone wants to press the button that would be great.

@grondo grondo merged commit 7518e94 into flux-framework:master Dec 29, 2018

3 checks passed

codecov/patch Coverage not affected when comparing d59ce0a...32de749
Details
codecov/project 80.14% (+<.01%) compared to d59ce0a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 29, 2018

Thanks!

@garlick garlick deleted the garlick:libev_4.25 branch Jan 4, 2019

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.