1377803 - Remove unnecessary plarena.h #includes

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P1)

Description

[Nicholas Nethercote [inactive]]

These files don't use PLArena or anything else from plarena.h.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove unnecessary plarena.h #includes

It would be nice to get rid of the plarena.h #include in
media/webrtc/trunk/webrtc/base/sanitizer.h, but that's a harder lift because
it's in third-party code.

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8882935 [details] [diff] [review]
Remove unnecessary plarena.h #includes

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

As jesup is on PTO.

The plarena.h include in sanitizer.h is not coming from upstream. We added that in our local clone. So as long as we can still build ASAN without it as per this comment:
http://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/base/sanitizer.h#15
I think we should remove it as part of this patch as well.

Comment 3

[Nicholas Nethercote [inactive]]

Thank you for taking the review.

> The plarena.h include in sanitizer.h is not coming from upstream. We added
> that in our local clone. So as long as we can still build ASAN without it as
> per this comment:
> http://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/base/sanitizer.h#15
> I think we should remove it as part of this patch as well.

I tried removing it here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d15650390b25cf14fc8fcec43a6da0878345034a

The ASAN jobs failed to build. There's a symbol visibility mismatch, but I don't understand it enough to diagnose.

glandium, do you understand these? It would be nice to remove that plarena.h include if we can.

Comment 4

[Nicholas Nethercote [inactive]]

Hmm, the problem is that media/webrtc/trunk/webrtc/base/sanitizer.h is including <sanitizer/asan_interface.h>, which declares the __asan_* functions, and also including "mozilla/MemoryChecking.h", which has its own MOZ_ASAN_VISIBILITY declarations of those functions. Somehow, including "plarena.h" -- which has its own declarations of the __asan_* functions -- avoids the problem.

Comment 5

[Mike Hommey [:glandium]]

You need to add sanitizer/asan_interface.h to config/system-headers, or make webrtc's sanitizer.h not use it.

> Somehow, including "plarena.h" -- which has its own declarations of the __asan_* functions -- avoids the problem.

Most likely reason is that plarena.h was included first, setting the visibility for those symbols to default. Now sanitizer/asan_interface.h comes first, and has the symbols set to visibility hidden because the header is not wrapped.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Remove an unnecessary plarena.h #include in WebRTC

We can avoid the symbol visibility problem by putting
sanitizer/asan_interface.h in the config/system-headers.

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8883135 [details] [diff] [review]
Remove an unnecessary plarena.h #include in WebRTC

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

LGTM

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66fb73c81ee666317c7ba26c666cdac785f22e5
Bug 1377803 - Remove unnecessary plarena.h #includes. r=drno.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b926d6497a9caacd770e82bae3106886efe280f
Bug 1377803 - Remove an unnecessary plarena.h #include in WebRTC. r=drno,glandium.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b66fb73c81ee
https://hg.mozilla.org/mozilla-central/rev/2b926d6497a9