Closed Bug 1192556 Opened 4 years ago Closed 4 years ago

Build broken on 32 bits in protobuf atomic handling

Categories

(DevTools :: General, defect)

x86
OpenBSD
defect
Not set

Tracking

(firefox41- wontfix, firefox42- affected, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox41 - wontfix
firefox42 - affected
firefox48 --- fixed

People

(Reporter: gaston, Assigned: gaston)

References

Details

Attachments

(1 file)

15:29.68 In file included from /home/buildslave-i386/mozilla-aurora-i386/build/toolkit/devtools/server/CoreDump.pb.cc:5:
15:29.68 In file included from /home/buildslave-i386/mozilla-aurora-i386/build/toolkit/devtools/server/CoreDump.pb.h:22:
15:29.68 In file included from ../../../dist/include/google/protobuf/generated_message_util.h:44:
15:29.68 ../../../dist/include/google/protobuf/stubs/once.h:125:30: error: cannot initialize a parameter of type 'const volatile Atomic32 *' (aka 'const volatile int *') with an lvalue of type 'ProtobufOnceType *' (aka 'long *')
15:29.68   if (internal::Acquire_Load(once) != ONCE_STATE_DONE) {
15:29.68                              ^~~~
15:29.68 ../../../dist/include/google/protobuf/stubs/atomicops_internals_x86_gcc.h:163:55: note: passing argument to parameter 'ptr' here
15:29.68 inline Atomic32 Acquire_Load(volatile const Atomic32* ptr) {

This has been happening since a while, i'd say two or three cycles. Have to test 41beta to confirm if affected too, aurora is broken now.
:fitzgen, looks related to your work.
Flags: needinfo?(nfitzgerald)
[Tracking Requested - why for this release]: Failure to build on 32-bits toolchains

The error message is not exactly the same but the build also fails on powerpc 32 bits, and this is with 41beta7:

/usr/ports/pobj/firefox-41.0beta7/mozilla-beta/toolkit/devtools/server/CoreDump.pb.cc
In file included from ../../../dist/include/google/protobuf/generated_message_util.h:44:0,
                 from /usr/ports/pobj/firefox-41.0beta7/mozilla-beta/toolkit/devtools/server/CoreDump.pb.h:22,
                 from /usr/ports/pobj/firefox-41.0beta7/mozilla-beta/toolkit/devtools/server/CoreDump.pb.cc:5:
../../../dist/include/google/protobuf/stubs/once.h: In function 'void google::protobuf::GoogleOnceInit(google::protobuf::Proto$
ufOnceType*, void (*)())':
../../../dist/include/google/protobuf/stubs/once.h:125:34: error: invalid conversion from 'google::protobuf::ProtobufOnceType* 
{aka long int*}' to 'const volatile Atomic32* {aka const volatile int*}' [-fpermissive]
   if (internal::Acquire_Load(once) != ONCE_STATE_DONE) {
Sigh. Looks like more header copypasted all around, and this reminded me of https://bugzilla.mozilla.org/show_bug.cgi?id=787933#6

mozilla-beta/ $find . -name atomicops.h 
./media/webrtc/trunk/webrtc/base/atomicops.h
./ipc/chromium/src/base/atomicops.h
./toolkit/components/protobuf/src/google/protobuf/stubs/atomicops.h
./security/sandbox/chromium/base/atomicops.h

seriously ? ok, the one in webrtc is different.... but the 3 other ones are very similar, only some small  differences, the main one to me being http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/atomicops.h#48

Guess i'll try adding the similar ifdef goo, but that's still horrible.
Ok with a similar change to atomicops.h as the one in ipc/chromium, builds goes much further but plugin-container linking fails because of missing syms in libxul.so:

../../toolkit/library/libxul.so.58.0: undefined reference to `__atomic_store_8'
../../toolkit/library/libxul.so.58.0: undefined reference to `__atomic_fetch_add_8'
../../toolkit/library/libxul.so.58.0: undefined reference to `__atomic_load_8'

Probably because atomicops_internals_generic_gcc.h is used, since that is still on powerpc, i'll have to check on i386 when i get access...

Anyway, on OpenBSD 32 bits platforms, sizeof(intptr_t) is 4, checked with this.

#include <stdio.h>
#include <unistd.h>
int main()  { printf("%d\n",sizeof(intptr_t));}
Definitely fallout of protobuf upgrade/import in 1024774.
Blocks: 1024744
This is in code generated by the protobuf compiler, so ideally the fix would happen upstream.
Flags: needinfo?(nfitzgerald)
(Otherwise the issue will be re-introduced every time we regenerate protobuf sources)
Nick, FF 41 is in RC week and I do not think we will take a fix for this as the RC bar is pretty high and we cannot afford to regress the ship quality. Wontfix'ing for 41. Please let me know if there are any concerns.
Flags: needinfo?(nfitzgerald)
WFM.
Flags: needinfo?(nfitzgerald)
Not tracking as it doesn't seem that important but happy to accept an uplift if it arrives soon.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #6)
> This is in code generated by the protobuf compiler, so ideally the fix would
> happen upstream.

I doubt atomicops.h and friends is code generated by the protobuf compiler, since there are now 3 diverging copies of it in tree. Why do we keep importing copies of the same crap over and over ? I already had to fix the one in ipc/chromium years ago...

reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Not tracking as it doesn't seem that important but happy to accept an uplift
> if it arrives soon.

Of course it's not important for tracking, just a poor tier3 target OS on two poor 32-bits archs nobody cares about, since nobody is building natively on 32-bits anymore. Yes, i'm bitter, it's always the same mess when third-party code is imported...

Oh well, guess i'll have to do the work.
Fwiw, i tried backporting https://github.com/google/protobuf/pull/804 to fix powerpc, but alas it changed nothing, linking plugin-container with libxul still complains about undefined refs to __atomic_load_8/etc, so *somewhere* *something* calls into atomic funcs with a 64-bits value instead of only 32-bits values.

I have a working fix for i386 though.
I had totally forgot about this issue until i stumbled upon it again, building tb 45.0b2 on i386.

My fix for firefox was :

--- toolkit/components/protobuf/src/google/protobuf/stubs/atomicops.h.orig      Fri Sep 18 12:01:02 2015
+++ toolkit/components/protobuf/src/google/protobuf/stubs/atomicops.h   Fri Sep 18 12:03:13 2015
@@ -78,7 +78,11 @@ typedef intptr_t Atomic64;
 
 // Use AtomicWord for a machine-sized pointer.  It will use the Atomic32 or
 // Atomic64 routines below, depending on your architecture.
+#if defined(__OpenBSD__) && !defined(GOOGLE_PROTOBUF_ARCH_64_BIT)
+typedef Atomic32 AtomicWord;
+#else
 typedef intptr_t AtomicWord;
+#endif

How would this get accepted within m-c ? (and no, dont make me go through the hassle of having to deal with upstream protobuf)
Flags: needinfo?(nfitzgerald)
Hi Landry,

Can you fold the m-c specific changes into this file https://dxr.mozilla.org/mozilla-central/source/toolkit/components/protobuf/m-c-changes.patch add then add the combined patch to this bug as an attachment so I can mark it "r+"? Then we can land it in m-c.

Thanks!
Flags: needinfo?(nfitzgerald) → needinfo?(landry)
Groovy, hadnt noticed we had a patch for our local changes. If i got it right, here's a patch fixing the real version and appending the local diff to the patch.

And for the record, the fix is copypasted/shoplifted/adapted from the first attachment in bug 648735. Now i feel old.
Assignee: nobody → landry
Flags: needinfo?(landry)
Attachment #8727606 - Flags: review?(nfitzgerald)
Comment on attachment 8727606 [details] [diff] [review]
Fix build on OpenBSD/i386

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

Thanks! I will make a try push in a second and after that comes back green we can land this.
Attachment #8727606 - Flags: review?(nfitzgerald) → review+
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b4b7af6cb77

ni myself to check the results later
Flags: needinfo?(nfitzgerald)
There was an android failure but it was pip timing out, unrelated to this patch. setting c-n.
Keywords: checkin-needed
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/mozilla-central/rev/70adea9b1272
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.