1224166 - Fix Bluetooth linker problems

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

BluetoothCommon.o is only build on FxOS with BT enabled. We've see linker problems on other systems. We should build this file unconditionally.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1224166: Define Bluetooth debug flag in BluetoothCommon.cpp

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1224166: Build BluetoothCommon.cpp unconditionally

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This patch set is required before landing bug 1220121. It's part of the try session at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea97abfe664

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #0)
> BluetoothCommon.o is only build on FxOS with BT enabled. We've see linker
> problems on other systems. We should build this file unconditionally.

Hi,
Do you mind providing the context? What's the linker problem you've seen?

Comment 5

[Bruce Sun [:brsun]]

Is this the same issue as stated on bug 1220121 comment 42?

Comment 6

[Bruce Sun [:brsun]]

Hi Thomas,

It is not clear to me why |gBluetoothDebugFlag| and other symbols in |BluetoothCommon| are needed on other platforms now. Could you help point out the major difference before/after your changes regarding to the linking problem?

Could this issue be solved by putting every configuration of |dom/bluetooth/moz.build| under the scope of |MOZ_B2G_BT|?

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Shawn, Bruce,

regarding previous linker problems: When we landed the recent update to |BluetoothUuid|, we had a number of static constants in it that described various values, such as the SDP base UUID or a UUID of zero. There's this 'Linux asan' target on Treeherder, which never built with them, so we had to remove the constants.

With bug 1220121, |BluetoothAddress| is used in the IPDL code and we see a similar problem, where we cannot link against address constants (|BluetoothAddress::ANY|) if Bluetooth is not built. [1] (Bug 1220121 Comment 42 refers to the try session which includes the fix.)

Several external headers include |BluetoothCommon.h| and expect the contained symbols to be linked and the IDL code apparently uses them. Besides the address constants, |gBluetoothDebugFlag| is declared in BluetoothCommon.h. So I moved its definition into BluetoothCommon.cpp, to prevent another possible linker error.

I don't know if everything in our moz.build can be protected by MOZ_B2G_BT, especially the IDL sources. No other module does this. They all build their IDL unconditionally. I looked at the Telephony code under dom/telephony, which builds a fair share unconditionally and only hides a few platform-dependend files. RIL told me that this was intentionally.

The overhead of these constants in the binary is small and I wouldn't be surprised if the linker removes constants that are not used anywhere. Seemed like a good deal to me.

Is this helpful to you?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=3343337&repo=b2g-inbound

Comment 8

[Bruce Sun [:brsun]]

> The overhead of these constants in the binary is small and I wouldn't be
> surprised if the linker removes constants that are not used anywhere. Seemed
> like a good deal to me.
> 

It makes sense to me. But I would like to suggest Shawn as the reviewer, since he is more familiar with the build system.

Hi Shawn,
Would you mind helping review these patches?

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Several external headers include |BluetoothCommon.h| and expect the
> contained symbols to be linked and the IDL code apparently uses them.
> Besides the address constants, |gBluetoothDebugFlag| is declared in
> BluetoothCommon.h. So I moved its definition into BluetoothCommon.cpp, to
> prevent another possible linker error.
> 
> I don't know if everything in our moz.build can be protected by MOZ_B2G_BT,
> especially the IDL sources. No other module does this. They all build their
> IDL unconditionally. I looked at the Telephony code under dom/telephony,
> which builds a fair share unconditionally and only hides a few
> platform-dependend files. RIL told me that this was intentionally.
> 
I guess you mean IPDL not IDL?
I think that makes sense to me to expose BluetoothCommon and it doesn't cost much.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> > Several external headers include |BluetoothCommon.h| and expect the
> > contained symbols to be linked and the IDL code apparently uses them.
> > Besides the address constants, |gBluetoothDebugFlag| is declared in
> > BluetoothCommon.h. So I moved its definition into BluetoothCommon.cpp, to
> > prevent another possible linker error.
> > 
> > I don't know if everything in our moz.build can be protected by MOZ_B2G_BT,
> > especially the IDL sources. No other module does this. They all build their
> > IDL unconditionally. I looked at the Telephony code under dom/telephony,
> > which builds a fair share unconditionally and only hides a few
> > platform-dependend files. RIL told me that this was intentionally.
> > 
> I guess you mean IPDL not IDL?
> I think that makes sense to me to expose BluetoothCommon and it doesn't cost
> much.

Oh. I see. You mean dom/webidl/moz.build? Yes, i remembered this is added by qDot or Ben Turner.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> > > 
> > I guess you mean IPDL not IDL?
> > I think that makes sense to me to expose BluetoothCommon and it doesn't cost
> > much.

Yes, IPDL; not IDL. Sorry for the confusion.

> 
> Oh. I see. You mean dom/webidl/moz.build? Yes, i remembered this is added by
> qDot or Ben Turner.

I'm not sure what you mean, that's probably just the confusion with IDL. The problem is not related to WebIDL, but IPDL, which is in dom/bluetooth/ipc.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I restarted the Windows builds in https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea97abfe664 It looks like there was an infrastructure problem yesterday.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/74d1c6a51028
https://hg.mozilla.org/integration/b2g-inbound/rev/3bb32aa4fb4f

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=3bb32aa4fb4f

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/74d1c6a51028
https://hg.mozilla.org/mozilla-central/rev/3bb32aa4fb4f