Including and/or using ScopedNSSTypes.h sometimes causes linking errors due to GCC visibility features on Linux builds

RESOLVED FIXED in Firefox 18

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

(Blocks 1 bug)

Trunk
B2G C2 (20nov-10dec)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Can you provide some error messages?
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); } \
            ^
Happens with gcc 4.7 with -std=gnu++0x too...
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.
Moving ScopedNSSTypes.h under dist/include/mozilla should be enough to work around the issue.
(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
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 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-
(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.
(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?
(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.)
(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?
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.
(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.
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.
Necessary for basecamp-blocker bug 804663.
blocking-basecamp: --- → +
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)
Also, here's the try run showing things looking good: https://tbpl.mozilla.org/?tree=Try&rev=3f6e0d9df38e
Attachment #684252 - Flags: review?(mh+mozilla) → review+
Attachment #683861 - Attachment is obsolete: true
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?
https://hg.mozilla.org/mozilla-central/rev/8a9dc4387c9c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #684252 - Flags: approval-mozilla-beta?
Attachment #684252 - Flags: approval-mozilla-aurora?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.