Closed
Bug 813241
Opened 12 years ago
Closed 12 years ago
Including and/or using ScopedNSSTypes.h sometimes causes linking errors due to GCC visibility features on Linux builds
Categories
(Firefox Build System :: General, defect, P1)
Firefox Build System
General
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)
People
(Reporter: briansmith, Assigned: briansmith)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
See bug 772365 comment 68.
This is most likely due to not having all the NSS and/or NSPR headers in config/system-headers. But, I also wrote a patch that added *all* NSS and NSPR headers to system-headers, and that broke the build in a different part (linking the MAR tools). So, more investigation is necessary.
Comment 1•12 years ago
|
||
Can you provide some error messages?
Assignee | ||
Comment 2•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=cafb6fbd584e (Include all NSS/NSPR headers in config/sytem-headers):
https://tbpl.mozilla.org/php/getParsedLog.php?id=17115428&tree=Try&full=1#error0
https://tbpl.mozilla.org/?tree=Try&rev=e535dcf9c4d0 (Add just secpkcs7.h to config/system-headers):
https://tbpl.mozilla.org/php/getParsedLog.php?id=17115953&tree=Try&full=1#error0
Comment 3•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e535dcf9c4d0 this fails to build on my machine with clang 3.1 (*without* -std=gnu++0x):
In file included from /home/mh/try/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp:17:
In file included from /home/mh/try/media/webrtc/signaling//./src/peerconnection/PeerConnectionImpl.h:26:
In file included from /home/mh/try/media/webrtc/signaling//../../../media/mtransport/dtlsidentity.h:14:
In file included from ../../../../dist/include/ScopedNSSTypes.h:12:
../../../../dist/include/mozilla/Scoped.h:261:9: error: call to function 'TypeSpecificDelete' that is neither visible in the template definition nor found by argument-dependent lookup
TypeSpecificDelete(value);
^
../../../../dist/include/mozilla/Scoped.h:141:15: note: in instantiation of member function 'mozilla::TypeSpecificScopedPointerTraits<PK11ContextStr>::release' requested here
Traits::release(value);
^
../../../../dist/include/mozilla/Scoped.h:138:14: note: in instantiation of member function 'mozilla::Scoped<mozilla::TypeSpecificScopedPointerTraits<PK11ContextStr> >::reset' requested here
return reset(other);
^
../../../../dist/include/mozilla/Scoped.h:265:1: note: in instantiation of member function 'mozilla::Scoped<mozilla::TypeSpecificScopedPointerTraits<PK11ContextStr> >::operator=' requested here
SCOPED_TEMPLATE(TypeSpecificScopedPointer, TypeSpecificScopedPointerTraits)
^
../../../../dist/include/mozilla/Scoped.h:170:14: note: expanded from macro 'SCOPED_TEMPLATE'
Super::operator=(ptr); \
^
../../../../dist/include/ScopedNSSTypes.h:173:13: note: in instantiation of member function 'mozilla::TypeSpecificScopedPointer<PK11ContextStr>::operator=' requested here
context = nullptr;
^
../../../../dist/include/ScopedNSSTypes.h:121:1: note: 'TypeSpecificDelete' should be declared prior to the call site or in the global namespace
MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPK11Context,
^
../../../../dist/include/mozilla/Scoped.h:250:13: note: expanded from macro 'MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE'
inline void TypeSpecificDelete(Type * value) { Deleter(value); } \
^
Comment 4•12 years ago
|
||
Happens with gcc 4.7 with -std=gnu++0x too...
Comment 5•12 years ago
|
||
Got to reproduce, and the problem is that the files ScopedNSSTypes.h includes are in the same directory (dist/include), which means that the compiler chooses to use the files in the same directory instead of the system_wrappers.
Comment 6•12 years ago
|
||
Moving ScopedNSSTypes.h under dist/include/mozilla should be enough to work around the issue.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5)
> Got to reproduce, and the problem is that the files ScopedNSSTypes.h
> includes are in the same directory (dist/include), which means that the
> compiler chooses to use the files in the same directory instead of the
> system_wrappers.
Thanks!
The issue in comment #3 is bug 812531.
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Version: unspecified → Trunk
Assignee | ||
Comment 8•12 years ago
|
||
Instead of moving ScopedNSSTypes.h to mozilla/, I moved the NSS headers out of dist/include into dist/include/public/nss. Besides fixing this bug, this also reduces the amount of work done during the build and makes the MOZ_NATIVE_NSS case more like the --use-system-nss case.
See https://tbpl.mozilla.org/?tree=Try&rev=f8bc60e6f0b4 for the try run. Note that it is OK for xpcshell to fail in test_signed_apps-marketplace.js.
Attachment #683861 -
Flags: review?(mh+mozilla)
Comment 9•12 years ago
|
||
Comment on attachment 683861 [details] [diff] [review]
Update config/system-headers and make wrapping of NSS headers more robust
Review of attachment 683861 [details] [diff] [review]:
-----------------------------------------------------------------
::: security/build/Makefile.in
@@ -335,5 @@
> $(INSTALL) -m 755 $(SDK_LIBS) $(DIST)/sdk/lib
> -# NSS installs headers to dist/public and we want them in dist/include
> - $(NSINSTALL) -D $(DIST)/include/nss
> - (cd $(DIST)/public/nss && tar $(TAR_CREATE_FLAGS) - .) | \
> - (cd $(DIST)/include && tar -xf -)
In fact, the obvious fix would be to make the above actually do what it was supposed to do: put the headers in dist/include/nss, which it doesn't. A bit of research shows it was actually the case before bug 488175.
Attachment #683861 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 683861 [details] [diff] [review]
> Update config/system-headers and make wrapping of NSS headers more robust
>
> Review of attachment 683861 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: security/build/Makefile.in
> @@ -335,5 @@
> > $(INSTALL) -m 755 $(SDK_LIBS) $(DIST)/sdk/lib
> > -# NSS installs headers to dist/public and we want them in dist/include
> > - $(NSINSTALL) -D $(DIST)/include/nss
> > - (cd $(DIST)/public/nss && tar $(TAR_CREATE_FLAGS) - .) | \
> > - (cd $(DIST)/include && tar -xf -)
>
> In fact, the obvious fix would be to make the above actually do what it was
> supposed to do: put the headers in dist/include/nss, which it doesn't. A bit
> of research shows it was actually the case before bug 488175.
Why is that better? It's just causing the build system to do more work for the non-benefit of avoiding the "public" subdirectory. I think we should avoid unnecessary work as part of the build.
Comment 11•12 years ago
|
||
(In reply to Brian Smith (:bsmith) from comment #10)
> Why is that better? It's just causing the build system to do more work for
> the non-benefit of avoiding the "public" subdirectory. I think we should
> avoid unnecessary work as part of the build.
"public" bears no meaning wrt what it contains. Don't forget dist/include is not only used internally at build time, it's also used to fill the SDK. That being said, the expectation from the SDK after bug 488175 is that everything is flat except the namespaced stuff, which leaves under mozilla. Maybe we should just keep it that way and move ScopedNSSTypes.h.
bsmedberg, since you're the one who did bug 488175, what do you think?
Comment 12•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #11)
> except the namespaced stuff, which leaves under mozilla.
(And other subdirectories, for that matter, but much less than in the past.)
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #11)
> "public" bears no meaning wrt what it contains. Don't forget dist/include is
> not only used internally at build time, it's also used to fill the SDK. That
> being said, the expectation from the SDK after bug 488175 is that everything
> is flat except the namespaced stuff, which leaves under mozilla. Maybe we
> should just keep it that way and move ScopedNSSTypes.h.
I am happy to move ScopedNSSTypes.h and other mozilla-namespaced headers to be under mozilla/. But, I really think it is bad to keep these system-header headers in dist/include because that would lead to others running into this kind of build failure, which is difficult to understand and difficult to fix or work around. So, in fact, I think we should do the same for NSPR. Plus, having NSPR and NSS in a separate directory (or separate directories) avoids accidental dependencies on them being in dist/include that would cause --use-system-nspr/nss to break.
I would like to remove the "public" subdirectory so that the files get put in dist/include/nss instead of dist/include/public/nss. But, I just don't like the extra work involved in copying the files around. I actually looked at modifying the NSS build system to let us omit the "public" subdirectory but a few parts are currently hard-coded to with $(DIST)/public/$(MODULE) and I have to land the dependent bugs' patches ASAP.
By the way, is NSS an intentional part of the SDK, or is it just an accidentally-exposed implementation detail of Gecko?
Comment 14•12 years ago
|
||
So if I understand this correctly, ScopedNSSTypes.h is an NSS header. The presumption is that all NSS headers are wrapped for external visibility, or indeed you will see this kind of problem. Why can't we wrap it?
When --use-system-* is specified we won't be installing the NSS or NSPR headers into dist/include so I don't think that's relevant to the problem here.
NSPR is certainly part of the gecko SDK because developers need it and typically wouldn't get it from elsewhere. NSS is much less important and we could certainly try removing it from the SDK and see what happens.
Comment 15•12 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #14)
> So if I understand this correctly, ScopedNSSTypes.h is an NSS header.
It's actually not. It's a C++ wrapper header that is part of PSM, not NSS.
Assignee | ||
Comment 16•12 years ago
|
||
I will update the patch to put NSS into dist/include/nss and I will try to put NSPR in dist/include/nspr for the same reason. I will also write a separate patch to move the mozilla-namespaced PSM headers (CryptoTask.h, ScopedNSSTypes.h, et al.) to dist/include/mozilla/.
When we redo the NSS build system, I will make sure it eliminates the need to copy the headers around during the build.
Assignee | ||
Comment 18•12 years ago
|
||
This patch puts NSS in dist/include/nss and NSPR in dist/include/nspr, and updates config/system-headers (and js/src/config/system-headers) to include all public NSPR and NSS headers. I put the NSPR and NSS header file names at the top of system-headers instead of sorting them into the list with the other items so that we can add+remove entries easier in the future as NSPR and NSS add/remove headers.
Attachment #684252 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 19•12 years ago
|
||
Also, here's the try run showing things looking good: https://tbpl.mozilla.org/?tree=Try&rev=3f6e0d9df38e
Updated•12 years ago
|
Attachment #684252 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #683861 -
Attachment is obsolete: true
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 684252 [details] [diff] [review]
Update config/system-headers and make wrapping of NSPR & NSS headers more robust [v2]
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9dc4387c9c
[Approval Request Comment]
User impact if declined: This is a prerequisite to packaged app signing support.
Risk to taking this patch (and alternatives if risky): None, this is just a build system fix.
String or UUID changes made by this patch: None
Attachment #684252 -
Flags: approval-mozilla-beta?
Attachment #684252 -
Flags: approval-mozilla-aurora?
Comment 21•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #684252 -
Flags: approval-mozilla-beta?
Attachment #684252 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/edcb32353d27
https://hg.mozilla.org/releases/mozilla-beta/rev/7a907d536d19
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•