Closed Bug 1246501 Opened 4 years ago Closed 4 years ago

jsep_track_unittest missing LockImpl signals

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
Blocking Flags:
backlog parking-lot

People

(Reporter: stevensn, Assigned: stevensn)

Details

Attachments

(1 file, 1 obsolete file)

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
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?
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
backlog: --- → parking-lot
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 ?
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.
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.
Assignee: nobody → steve
Status: NEW → ASSIGNED
Martin,

I expect you will hit the same linking issue on SPARC.
Attachment #8719232 - Flags: review?(benjamin)
Comment on attachment 8719232 [details] [diff] [review]
Add ppc specific atomic operations to ipc/chromium

Waldo is the atomics expert, I think.
Attachment #8719232 - Flags: review?(benjamin) → review?(jwalden+bmo)
Jeff, any update on this review? Is there someone else I should assign it to?
I haven't found a good, solid chunk of time to devote to it just yet, unfortunately.  Maybe tomorrow.
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.  :-)
Attachment #8719232 - Flags: review?(jwalden+bmo) → review+
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
Oh, bah, upstream.  I hadn't realized you got this from there.  Carry on without my requested changes, then.
Keywords: checkin-needed
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
Flags: needinfo?(steve)
Keywords: checkin-needed
Attachment #8719232 - Attachment is obsolete: true
Flags: needinfo?(steve)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4df6e54d8cf1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.