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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Mike Hommey [:glandium]]

Can you provide some error messages?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

Happens with gcc 4.7 with -std=gnu++0x too...

Comment 5

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

Moving ScopedNSSTypes.h under dist/include/mozilla should be enough to work around the issue.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Update config/system-headers and make wrapping of NSS headers more robust

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.

Comment 9

[Mike Hommey [:glandium]]

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.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[Mike Hommey [:glandium]]

(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

[Mike Hommey [:glandium]]

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

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[Benjamin Smedberg]

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

[Mike Hommey [:glandium]]

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

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 17

[Lucas Adamski [:ladamski]]

Necessary for basecamp-blocker bug 804663.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Update config/system-headers and make wrapping of NSPR & NSS headers more robust [v2]

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.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, here's the try run showing things looking good: https://tbpl.mozilla.org/?tree=Try&rev=3f6e0d9df38e

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a9dc4387c9c

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/edcb32353d27
https://hg.mozilla.org/releases/mozilla-beta/rev/7a907d536d19