1246501 - jsep_track_unittest missing LockImpl signals

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Steve Singer (:stevensn)]

The webrtc signalling unit tests don't compile on my ppc64 machine.

 8:53.57 ../../trunk/webrtc/modules/modules_rtp_rtcp/Unified_cpp_webrtc_modules1.o: In function `Lock::Acquire()':
 8:53.57 /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/ipc/chromium/src/base/lock.h:16: undefined reference to `LockImpl::Lock()'
 8:53.57 ../../trunk/webrtc/modules/modules_rtp_rtcp/Unified_cpp_webrtc_modules1.o: In function `Lock::Release()':
 8:53.57 /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/ipc/chromium/src/base/lock.h:17: undefined reference to `LockImpl::Unlock()'
 8:53.58 /usr/bin/ld: jsep_track_unittest: hidden symbol `_ZN8LockImpl6UnlockEv' isn't defined


This issue was introduced in commit 8ee641313088 for bug 1219339

Comment 1

[Steve Singer (:stevensn)]

Randell,

I don't have access to view the details of bug 1219339

Why aren't these unit tests pulling in required parts of ipc/chromium
Should I add LockImpl to FakeIPC.cpp or is there a better solution?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Sorry, no idea why they're not pulled in; I'd check the differences in the compile lines between ppc64 and x86 for this and for the file/dir that implements LockImpl in ipc.

Breaking the commandline at spaces, then using diff on the two may work better than straight diff

Comment 3

[Steve Singer (:stevensn)]

Randell,
Why isn't it the linking for the unit test picking up PlatformThread on x86 from ipc/chromium/src/base/platform_thread_posix.cc ?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Note that symbols from libxul are NOT generally exported and aren't accessible from C++ unit tests, except for a few white-listed ones.  See symbols.def.  That's why there's a lot of code with #ifdef MOZILLA_INTERNAL_API and stuff like that.

Comment 5

[Steve Singer (:stevensn)]

Bug 1219339 added includes of Singleton.h to a few places in webrtc/signalling

Singleton.h (in ipc/chromium) includes atomic_ops.h

For all the tier 1 platforms there is a platform specific file that this includes (ie atomicops_internals_arm_gcc.h) but otherwise it falls back to
atomicops_internals_mutex.h which includes lock.h

This is why I get the undefined symbols from lock.h ppc64 but tier1 platforms link fine.
It is unclear to me where I would have to whitelist these symbols so the unit test can see them. I don't see a file named symbols.def

There is a version of the atomicops for ppc  https://chromium.googlesource.com/v8/v8/+/4cbd63c9bfee2b734f67f5bd36b98dcab072021f/src/base/atomicops_internals_ppc_gcc.h

I will try adapting that.

Comment 6

[Steve Singer (:stevensn)]

Attachment: Add ppc specific atomic operations to ipc/chromium

Comment 7

[Steve Singer (:stevensn)]

Martin,

I expect you will hit the same linking issue on SPARC.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8719232 [details] [diff] [review]
Add ppc specific atomic operations to ipc/chromium

Waldo is the atomics expert, I think.

Comment 9

[Steve Singer (:stevensn)]

Jeff, any update on this review? Is there someone else I should assign it to?

Comment 10

[Jeff Walden [:Waldo]]

I haven't found a good, solid chunk of time to devote to it just yet, unfortunately.  Maybe tomorrow.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8719232 [details] [diff] [review]
Add ppc specific atomic operations to ipc/chromium

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

There's semi-pervasive use of inlines, here, before their definitions.  For example:

inline Atomic64 NoBarrier_AtomicIncrement(volatile Atomic64* ptr,
                                          Atomic64 increment) {
  return Barrier_AtomicIncrement(ptr, increment);
}
inline Atomic64 Barrier_AtomicIncrement(volatile Atomic64* ptr,
                                        Atomic64 increment) {
  ...
}

Presumably this works at all because atomicops.h declares all these functions, then this header just (later, in each translation unit) defines them.  Traditionally I believe GCC will warn about using an inline function that hasn't been defined yet, so this seems possibly like it'll be pretty spewy.

But hey -- this is PPC-only, so it's your problem if you get warnings out the wazoo from this.  :-)

::: ipc/chromium/src/base/atomicops_internals_ppc_gcc.h
@@ +37,5 @@
> +  }
> +}
> +inline Atomic32 Acquire_CompareAndSwap(volatile Atomic32* ptr,
> +                                       Atomic32 old_value, Atomic32 new_value) {
> +  return NoBarrier_CompareAndSwap(ptr, old_value, new_value);

This reads super-squirrely.  At the top of this file (inside the subtle namespace), please add

namespace detail {

inline Atomic32
FullyBarriered_CompareAndSwap(volatile Atomic32* ptr,
                              Atomic32 old_value, Atomic32 new_value) {
    ...current contents of NoBarrier_CompareAndSwap...
}

} // namespace detail

and then call detail::FullyBarriered_CompareAndSwap in *_CompareAndSwap.  It's of course perfectly fine to have more barriers than strictly required, for any of these methods.  It's emphatically *not* okay to have fewer.  And as currently written, it reeeeeeallly reads like there are fewer.  Let's have better names to not be confusing.

@@ +47,5 @@
> +inline void NoBarrier_Store(volatile Atomic32* ptr, Atomic32 value) {
> +  *ptr = value;
> +}
> +inline void MemoryBarrier() {
> +  __asm__ __volatile__("sync" : : : "memory"); }

Can you not use __sync_synchronize() here?  That seems preferable to asm-goo to me.  And put the closing brace on a separate line, let's at least try to be minimally style-consistent here.  :-)

Comment 12

[Steve Singer (:stevensn)]

Jeff, I copied this file from upstream (chromium/v8) and only made very minimal changes so it compiles.

I don't mind making the changes it is just a question of how much we want this to diverge from upstream. Is it possible that we would take a newer version of this code from upstream at some point in the future, and if so wouldn't these changes just make things harder to merge/update

Comment 13

[Jeff Walden [:Waldo]]

Oh, bah, upstream.  I hadn't realized you got this from there.  Carry on without my requested changes, then.

Comment 14

[Steve Singer (:stevensn)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59f5726db85c

Comment 15

[Carsten Book [:Tomcat]]

has problems to apply:

applying 98b40f7c3e38
patching file ipc/chromium/src/base/atomicops.h
Hunk #1 FAILED at 136
1 out of 1 hunks FAILED -- saving rejects to file ipc/chromium/src/base/atomicops.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 98b40f7c3e38

Comment 16

[Steve Singer (:stevensn)]

Attachment: Add ppc specific atomic operations to ipc/chromium

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4df6e54d8cf1

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4df6e54d8cf1