312366 - The configure test for the visibility("hidden") attribute doesn't work on Mac OS X.

Status: NEW

(NSPR :: NSPR, defect, P2)

Description

[Wan-Teh Chang]

I discovered this problem while testing my patch for
bug 312361 on Mac OS X Tiger with gcc 4.0.

I found that Mac OS X's gcc 4.0 supports the visibility("hidden")
attribute, but NSPR's configure test for the visibility("hidden")
attribute returns "no".  The configure test is:

****************** begin code ******************

dnl ===============================================================
dnl Check for .hidden assembler directive and visibility attribute.
dnl Borrowed from glibc configure.in
dnl ===============================================================
if test "$GNU_CC"; then
    AC_CACHE_CHECK(for visibility(hidden) attribute,
        ac_cv_visibility_hidden,
        [cat > conftest.c <<EOF
        int foo __attribute__ ((visibility ("hidden"))) = 1;
EOF
        ac_cv_visibility_hidden=no
        if ${CC-cc} -Werror -S conftest.c -o conftest.s >/dev/null 2>&1; then
            if grep '\.hidden.*foo' conftest.s >/dev/null; then
                ac_cv_visibility_hidden=yes
            fi
        fi
        rm -f conftest.[cs]
        ])

******************  end code  ******************

The reason it doesn't work on Mac OS X is that gcc's
assembly output doesn't use the .hidden directive there.
Instead, it uses the .private_extern directive.

Comment 1

[Wan-Teh Chang]

Attachment: Mac OS X Tiger gcc 4.0's assembly output of visibility("hidden")

Comment 2

[Wan-Teh Chang]

Attachment: Mac OS X Tiger gcc 4.0's assembly output of visibility("default") or no visibility attribute

Compare this with the output of visibility("hidden"), you'll
see that the only difference is this line:

--- C:/temp/foo.s.default	2005-10-13 14:42:20.000000000 -0700
+++ C:/temp/foo.s.hidden	2005-10-13 14:36:44.640625000 -0700
@@ -1,6 +1,7 @@
	.section __TEXT,__text,regular,pure_instructions
	.section __TEXT,__picsymbolstub1,symbol_stubs,pure_instructions,32
	.machine ppc
+	.private_extern _foo
	.globl _foo
	.data
	.align 2

Comment 3

[Wan-Teh Chang]

As a comparison, here is the diff between the same two assembly
files generated by gcc 4.0 on Red Hat Enterprise Linux 4 (x86):

--- foo.s.default       2005-10-13 14:49:18.749955203 -0700
+++ foo.s.hidden        2005-10-13 14:48:43.050850374 -0700
@@ -1,4 +1,5 @@
        .file   "foo.c"
+       .hidden foo
 .globl foo
        .data
        .align 4

It seems that the fix is to also grep '\.private_extern.*foo' conftest.s.

Comment 4

[Brian Ryner (not reading)]

I'm not going to have time to look at this, if it's still a problem.

Comment 5

[Benjamin Smedberg]

Attachment: Mozilla patch to treat NSPR like an independent library, rev. 1

This patch does two things: it allows us to detect some support for __attribute__ ((visibility ("hidden"))) on macos, and it wraps the NSPR headers like any other system library, so that NSPR is entirely decoupled from Mozilla in terms of linkage and configuration.

Comment 6

[Benjamin Smedberg]

Attachment: NSPR patch: use default visibility and explicitly hide internal methods, rev. 1

This patch changes the visibility strategy of NSPR to be much simpler: we never use or need to use -fvisibility=hidden or pragmas: we just (if the compiler supports it) mark internal functions/data as hidden where appropriate. This is good becuase:

* client code using NSPR doesn't have to think about hidden visibility at all
* NSPR doesn't have to worry about broken compiler support for visibility pragmas
* We can completely decouple the mozilla visibility checks from the NSPR visibility checks

Comment 7

[Wan-Teh Chang]

Ben, isn't it a regression to remove the "default" visibility attributes
from the function declarations in NSPR public header files?

It'd be a shame if Mozilla needed to wrap NSPR headers.  I want to continue
to use -fvisibility=hidden.  I don't need to use the visibility pragmas.

Comment 8

[Benjamin Smedberg]

What's the regression? The NSPR API is default visibility, the only question is the best way to "hide" the non-static internal functions/data. pragmas and -fvisibility-hidden are one way to do that, but not IMO the best way in this particular case, where we can explicitly mark hidden-visibility on the internal functions/data.

Comment 9

[Wan-Teh Chang]

The regression is that some NSPR-based products such
as the Mozilla clients will have to wrap NSPR public
headers because the NSPR patch (attachment 235259 [details] [diff] [review])
removes the "default" visibility attributes from the
public function declarations.

Comment 10

[Benjamin Smedberg]

Well, I suppose that the attribute("default") bits could be kept (though I really think they are unneeded, and plan to wrap the NSPR headers anyway, since that's what should be done in general with library headers).

Comment 11

[Benjamin Smedberg]

Attachment: NSPR patch: use default visibility and explicitly hide internal methods, rev. 1.1

This keeps the visibility("default") setting in prtypes, even though it's the default, so to be compatible with older mozillas.

Comment 12

[Wan-Teh Chang]

Comment on attachment 235394 [details] [diff] [review]
NSPR patch: use default visibility and explicitly hide internal methods, rev. 1.1

Ben,

I don't understand why you want to wrap NSPR headers.
I spent a lot of time on adding the visibility("default")
attributes to NSPR public header files.  For example, there
were hours of research behind this test in prtypes.h that
only depends on compiler pre-defined macros:

 /* GCC 3.3 and later support the visibility attribute. */
 #if (__GNUC__ >= 4) || \
     (__GNUC__ == 3 && __GNUC_MINOR__ >= 3)
 #define PR_VISIBILITY_DEFAULT __attribute__((visibility("default")))
 #else
 #define PR_VISIBILITY_DEFAULT
 #endif

So I'd like Mozilla to take advantage of my work and not
wrap NSPR headers.  If you find anything broken, I'd like
to fix it rather than having you work around it.

In NSPR I want to continue using -fvisibility=hidden.  I
don't want to use the visibility("hidden" attribute.  The
size of this patch is exactly the reason.  The complete
patch would be bigger because primpl.h is not the only NSPR
header that declares internal functions.

Comment 13

[Wan-Teh Chang]

(In reply to comment #11)
> Created an attachment (id=235394) [edit]
> NSPR patch: use default visibility and explicitly hide internal methods, rev.
> 1.1
> 
> This keeps the visibility("default") setting in prtypes, even though it's the
> default, so to be compatible with older mozillas.

The reason prtypes.h uses the visibility("default") setting explicitly
is that the default can be changed with -fvisibility or pragma.  When
I worked on it, this was the recommended practice for library headers.
I hope this still is.

Comment 14

[Benjamin Smedberg]

> I don't understand why you want to wrap NSPR headers.

Because that is how we treat all independent libraries. There is no need (or desire) to know anything about how NSPR does visibility, as long as the functions in the public headers are available.

You've already mentioned several times why -fvisibility=hidden is not ideal (because it doesn't assume anything about the declaration of functions, only the definitions, and therefore doesn't provide the codesize benefits of hidden visibility), and using #pragmas is impossible with any currently released GCC because of compiler bugs.

Close if you like, but explicitly declaring hidden visibility still has benefits that -fvisibility=hidden doesn't.

Comment 15

[Wan-Teh Chang]

Comment on attachment 235394 [details] [diff] [review]
NSPR patch: use default visibility and explicitly hide internal methods, rev. 1.1

I want to take the portions of your patch that implement a test for .private_extern on Mac OS X.

I never fully understood the benefits of visibility pragmas
over -fvisibility=hidden.  -fvisibility=hidden is easy to
use and is well documented, so it has become my preferred
method.

Comment 16

[Wan-Teh Chang]

Comment on attachment 235257 [details] [diff] [review]
Mozilla patch to treat NSPR like an independent library, rev. 1

I hope you will back out the change to config/system-headers
in this patch.  Don't let my work on the NSPR headers go in
vain.

Comment 17

[Benjamin Smedberg]

> I never fully understood the benefits of visibility pragmas
> over -fvisibility=hidden.  -fvisibility=hidden is easy to

The problem is that -fvisibility=hidden only affects the implementation: let's say you had the following declaration:

PRStatus _PR_Internal_GetStatus();

With -fvisibility=hidden, the implementation of this method would have hidden visibility. But any callers would still call the method as if it were default-visibility, which means loading the PIC register and calling through the PLT. If you use pragmas, or explicitly decorate the method signature, the callers can make optimizations by calling the method "directly" and in some cases avoid loading the PIC register. This is where the codesize/speed benefits of hidden visibility are found, as opposed to simply isolating the DSO. In C code you're unlikely to see the benefits much, but for certain functions such as the PR_Atomic* functions which have very high callcounts and call internal functions, it could make a big difference.

Comment 18

[Wan-Teh Chang]

Ben, thank you for the clear explanation.  Let's add the
hidden visibility attribute to the _PR_MD_ATOMIC_*
(or _MD_ATOMIC_*) declarations.  I now remember this is
what bryner's original NSPR patch (attachment 148377 [details] [diff] [review] in
bug 227537) does.  We can gradually add the hidden
visibility attribute to the other function declarations
where it makes a big difference.

One thing that bryner's patch shows is that we need to
add the .hidden assembler directives to the assembly code
files.  Since NSPR uses linker version scripts to control
symbol export and versioning, perhaps the lack of .hidden
assembler directives isn't a problem.