Closed Bug 1354207 Opened 3 years ago Closed 3 years ago

signaling disables valgrind

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file, 1 obsolete file)

It looks like even under valgrind builds signaling is specifying '-DNVALGRIND'. This is causing build failures due to -Werror-unused in code that uses the valgrind macros.

Example command line:

> [task 2017-04-05T19:31:22.998732Z] 19:31:22     INFO -  /usr/bin/ccache /home/worker/workspace/build/src/gcc/bin/gcc -std=gnu99 -o Unified_c_webrtc_signaling0.o -c -I/home/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /home/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG -DTRIMMED=1 -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DUSE_NSS=1 -DENABLE_ONE_CLICK_SIGNIN -DGTK_DISABLE_SINGLE_INCLUDES=1 -D_ISOC99_SOURCE=1 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DLOG4CXX_STATIC -D_NO_LOG4CXX -DUSE_SSLEAY -D_CPR_USE_EXTERNAL_LOGGER -DWEBRTC_RELATIVE_PATH -DHAVE_WEBRTC_VIDEO -DHAVE_WEBRTC_VOICE -DHAVE_STDINT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_UINT8_T=1 -DHAVE_UINT16_T=1 -DHAVE_UINT32_T=1 -DHAVE_UINT64_T=1 -DOS_LINUX -DSIP_OS_LINUX -DWEBRTC_POSIX -D_GNU_SOURCE -DLINUX -DGIPS_VER=3510 -DSECLIB_OPENSSL -D__STDC_FORMAT_MACROS -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/home/worker/workspace/build/src/media/webrtc/signaling -I/home/worker/workspace/build/src/obj-firefox/media/webrtc/signaling/signaling_ecc -I/home/worker/workspace/build/src/media/webrtc -I/home/worker/workspace/build/src/media/webrtc/signaling/src -I/home/worker/workspace/build/src/media/webrtc/signaling/src/common -I/home/worker/workspace/build/src/media/webrtc/signaling/src/common/browser_logging -I/home/worker/workspace/build/src/media/webrtc/signaling/src/common/time_profiling -I/home/worker/workspace/build/src/media/webrtc/signaling/src/media -I/home/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit -I/home/worker/workspace/build/src/media/webrtc/signaling/src/mediapipeline -I/home/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection -I/home/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc -I/home/worker/workspace/build/src/dom/base -I/home/worker/workspace/build/src/dom/media -I/home/worker/workspace/build/src/media/mtransport -I/home/worker/workspace/build/src/media/webrtc/trunk -I/home/worker/workspace/build/src/media/libyuv/libyuv/include -I/home/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/util/libekr -I/home/worker/workspace/build/src/netwerk/srtp/src/include -I/home/worker/workspace/build/src/netwerk/srtp/src/crypto/include -I/home/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/home/worker/workspace/build/src/ipc/chromium/src -I/home/worker/workspace/build/src/ipc/glue -I/home/worker/workspace/build/src/obj-firefox/dist/include    -fPIC  -include /home/worker/workspace/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/Unified_c_webrtc_signaling0.o.pp  -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe  -g -freorder-blocks -Os -fno-omit-frame-pointer  -Werror -I/home/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/home/worker/workspace/build/src/obj-firefox/dist/include/nss  /home/worker/workspace/build/src/obj-firefox/media/webrtc/signaling/signaling_ecc/Unified_c_webrtc_signaling0.c

Example error:

[task 2017-04-05T19:31:58.991728Z] 19:31:58     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h: In instantiation of 'mozilla::ArenaAllocator<ArenaSize, Alignment>::ArenaChunk* mozilla::ArenaAllocator<ArenaSize, Alignment>::AllocateChunk(size_t) [with long unsigned int ArenaSize = 4096ul; long unsigned int Alignment = 8ul; size_t = long unsigned int]':
[task 2017-04-05T19:31:58.991857Z] 19:31:58     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:214:73:   required from 'void* mozilla::ArenaAllocator<ArenaSize, Alignment>::InternalAllocate(size_t) [with long unsigned int ArenaSize = 4096ul; long unsigned int Alignment = 8ul; size_t = long unsigned int]'
[task 2017-04-05T19:31:58.991995Z] 19:31:58     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:72:47:   required from 'void* mozilla::ArenaAllocator<ArenaSize, Alignment>::Allocate(size_t, const mozilla::fallible_t&) [with long unsigned int ArenaSize = 4096ul; long unsigned int Alignment = 8ul; size_t = long unsigned int]'
[task 2017-04-05T19:31:58.992131Z] 19:31:58     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:77:39:   required from 'void* mozilla::ArenaAllocator<ArenaSize, Alignment>::Allocate(size_t) [with long unsigned int ArenaSize = 4096ul; long unsigned int Alignment = 8ul; size_t = long unsigned int]'
[task 2017-04-05T19:31:58.992196Z] 19:31:58     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/BSPTree.h:71:32:   required from here
[task 2017-04-05T19:31:58.992276Z] 19:31:58     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:185:5: error: statement has no effect [-Werror=unused-value]
[task 2017-04-05T19:31:58.992320Z] 19:31:58     INFO -       MOZ_MAKE_MEM_NOACCESS((void*)arena->header.offset,
I'm not a build expert, but we appear to get that NVALGRIND from the webrtc.org build system: http://searchfox.org/mozilla-central/search?q=NVALGRIND&case=false&regexp=false&path=webrtc
Seems like maybe we're not setting build_for_tool: "memcheck" [1]? I'm not really sure how our gyp integration works unfortunately.

[1] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/media/webrtc/trunk/build/common.gypi#1359
Also note this is blocking bug 1351426 from landing.
Blocks: 1351426
So removing all three occurrences of NVALGRIND from the build files shows that the define no longer gets used during the build.
I just did a local debug build and there is no NVALGRIND define to be seen:

15:37.98 /usr/bin/clang -std=gnu99 -o Unified_c_webrtc_signaling0.o -c -fvisibility=hidden -fvisibility-inlines-hidden -DDEBUG=1 -DTRACING=1 -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DLOG4CXX_STATIC -D_NO_LOG4CXX -DUSE_SSLEAY -D_CPR_USE_EXTERNAL_LOGGER -DWEBRTC_RELATIVE_PATH -DHAVE_WEBRTC_VIDEO -DHAVE_WEBRTC_VOICE -DHAVE_STDINT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_UINT8_T=1 -DHAVE_UINT16_T=1 -DHAVE_UINT32_T=1 -DHAVE_UINT64_T=1 -DWEBRTC_POSIX -DOS_MACOSX -DSIP_OS_OSX -DOSX -D_FORTIFY_SOURCE=2 -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling -I/Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/media/webrtc/signaling/signaling_ecc -I/Users/nohlmeier/src/mozilla-central/media/webrtc -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/common -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/common/browser_logging -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/common/time_profiling -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/media -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/media-conduit -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/mediapipeline -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/peerconnection -I/Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc -I/Users/nohlmeier/src/mozilla-central/dom/base -I/Users/nohlmeier/src/mozilla-central/dom/media -I/Users/nohlmeier/src/mozilla-central/media/mtransport -I/Users/nohlmeier/src/mozilla-central/media/webrtc/trunk -I/Users/nohlmeier/src/mozilla-central/media/libyuv/libyuv/include -I/Users/nohlmeier/src/mozilla-central/media/mtransport/third_party/nrappkit/src/util/libekr -I/Users/nohlmeier/src/mozilla-central/netwerk/srtp/src/include -I/Users/nohlmeier/src/mozilla-central/netwerk/srtp/src/crypto/include -I/Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/ipc/ipdl/_ipdlheaders -I/Users/nohlmeier/src/mozilla-central/ipc/chromium/src -I/Users/nohlmeier/src/mozilla-central/ipc/glue -I/Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/dist/include    -fPIC  -include /Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/Unified_c_webrtc_signaling0.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wclass-varargs -Wloop-analysis -Wstring-conversion -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wno-gnu-zero-variadic-macro-arguments -fno-strict-aliasing -fno-math-errno -pthread -pipe  -g -fno-omit-frame-pointer   -I/Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/dist/include/nspr -I/Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/dist/include/nss -Wno-inconsistent-missing-override -Wno-macro-redefined  /Users/nohlmeier/src/mozilla-central/objdir-x86_64-apple/media/webrtc/signaling/signaling_ecc/Unified_c_webrtc_signaling0.c
MozReview-Commit-ID: 859LIn8n80i
Nils, I think attachment 8855483 [details] [diff] [review] should fix this specific issue, but again I'm not well versed in gyp. Arguably we should fix it for asan as well. Can you try it out?
Flags: needinfo?(drno)
Optimized local build has the NVALGRIND. So it looks because we optimize our Valgrind builds it pulls in the NVALGRIND
Comment on attachment 8855483 [details] [diff] [review]
WIP: Set memcheck flag when building under valgrind

Review of attachment 8855483 [details] [diff] [review]:
-----------------------------------------------------------------

Nope, sorry does not work. I still see NVALGRIND in the build log with this patch applied.
Dan do you know/understand what we need to do to get rid of the NVALGRIND if MOZ_VALGRIND is set?
Flags: needinfo?(drno) → needinfo?(dminor)
(In reply to Nils Ohlmeier [:drno] from comment #9)
> Comment on attachment 8855483 [details] [diff] [review]
> WIP: Set memcheck flag when building under valgrind
> 
> Review of attachment 8855483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nope, sorry does not work. I still see NVALGRIND in the build log with this
> patch applied.

Even for signaling? I finally got a build locally, but it's possible my Werror setup is different than the build machine.
The webrtc gyp files have a 'build_for_tool' flag that controls among other
things what defines are provided at build time. This meant that during a
firefox valgrind build webrtc would still specify NVALGRIND, thus disabling
some valgrind macros. Similarly there are flags for asan and tsan that we
should probably have been specifying as well. This patch sets the
'build_for_tool' flag to the appropriate value when building under valgrind,
asan, and tsan.
Attachment #8855572 - Flags: review?(rjesup)
Attachment #8855483 - Attachment is obsolete: true
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Kicked off a build including patches from the blocked bug at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=455c1849e68b4f378b2fa0a4510f792c7ca2f55b
(In reply to Eric Rahm [:erahm] from comment #11)
> (In reply to Nils Ohlmeier [:drno] from comment #9)
> > Comment on attachment 8855483 [details] [diff] [review]
> > WIP: Set memcheck flag when building under valgrind
> > 
> > Review of attachment 8855483 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Nope, sorry does not work. I still see NVALGRIND in the build log with this
> > patch applied.
> 
> Even for signaling? I finally got a build locally, but it's possible my
> Werror setup is different than the build machine.

Sorry for Unified_c_webrtc_signaling0.c there is no NVALGRIND any more, but NVALGRIND is still present in a lot of other compiler lines. So I guess depends what exactly you need here.
(In reply to Nils Ohlmeier [:drno] from comment #14)
> Sorry for Unified_c_webrtc_signaling0.c there is no NVALGRIND any more, but
> NVALGRIND is still present in a lot of other compiler lines. So I guess
> depends what exactly you need here.

I mostly just care about unbreaking signaling, we can chase down the rest some other time. The patch I put up should apply to the rest of webrtc, so hopefully that covers enough.
Comment on attachment 8855572 [details] [diff] [review]
Set webrtc build flags when building under valgrind, etc

Review of attachment 8855572 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but adding a build peer for review
Attachment #8855572 - Flags: review?(rjesup)
Attachment #8855572 - Flags: review?(mh+mozilla)
Attachment #8855572 - Flags: review+
Attachment #8855572 - Flags: review?(mh+mozilla) → review+
Flags: needinfo?(dminor)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3ecca7252bef0a57c6607de8bdd58812b3e4c7
Bug 1354207 - Set webrtc build flags when building under valgrind, etc. r=jesup
https://hg.mozilla.org/mozilla-central/rev/4d3ecca7252b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.