1192556 - Build broken on 32 bits in protobuf atomic handling

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Landry Breuil (:gaston)]

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.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:fitzgen, looks related to your work.

Comment 2

[Landry Breuil (:gaston)]

[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) {

Comment 3

[Landry Breuil (:gaston)]

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.

Comment 4

[Landry Breuil (:gaston)]

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));}

Comment 5

[Landry Breuil (:gaston)]

Definitely fallout of protobuf upgrade/import in 1024774.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This is in code generated by the protobuf compiler, so ideally the fix would happen upstream.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(Otherwise the issue will be re-introduced every time we regenerate protobuf sources)

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

WFM.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Not tracking as it doesn't seem that important but happy to accept an uplift if it arrives soon.

Comment 11

[Landry Breuil (:gaston)]

(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.

Comment 12

[Landry Breuil (:gaston)]

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.

Comment 13

[Landry Breuil (:gaston)]

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)

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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!

Comment 15

[Landry Breuil (:gaston)]

Attachment: Fix build on OpenBSD/i386

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.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b4b7af6cb77

ni myself to check the results later

Comment 18

[Landry Breuil (:gaston)]

There was an android failure but it was pip timing out, unrelated to this patch. setting c-n.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/70adea9b1272

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70adea9b1272