[Bug c/13134] New: Visibility attribute is ignored

[Bug c/13134] New: Visibility attribute is ignored

[bryner at brianryner dot com]

The change to visibility handling that was checked in on Nov 6 seems to have
broken visibility pretty badly.  Setting the visibility attribute on a C
function declaration no longer causes the function to be emitted with the given
visibility.

To reproduce this, compile the attached testcase:
gcc -c -o visibility.o visibility.c

and check the output with readelf:
readelf -s visibility.o

It will show visibility DEFAULT for all of the functions in this object file:
    7: 00000000      5 FUNC    GLOBAL DEFAULT        1 func1
    8: 00000005      5 FUNC    GLOBAL DEFAULT        1 func2
    9: 0000000a      5 FUNC    GLOBAL DEFAULT        1 func3
   10: 0000000f      5 FUNC    GLOBAL DEFAULT        1 func4
   11: 00000014      5 FUNC    GLOBAL DEFAULT        1 func5
   12: 00000019      5 FUNC    GLOBAL DEFAULT        1 func6
   13: 0000001e      5 FUNC    GLOBAL DEFAULT        1 func7
   14: 00000023      5 FUNC    GLOBAL DEFAULT        1 func8

-- 
           Summary: Visibility attribute is ignored
           Product: gcc
           Version: 3.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: c
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: bryner at brianryner dot com
                CC: austern at apple dot com,gcc-bugs at gcc dot gnu dot org
  GCC host triplet: i686-pc-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] Visibility attribute is ignored

[bryner at brianryner dot com]

------- Additional Comments From bryner at brianryner dot com  2003-11-20 00:58 -------
Created an attachment (id=5175)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=5175&action=view)
testcase


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[bryner at brianryner dot com]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
            Summary|Visibility attribute is     |[3.4 regression] Visibility
                   |ignored                     |attribute is ignored


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2003-11-20 06:08 -------
Matt, some how the bits in the decl are not being copied for C (it works for c++).
It looks like you forgot to update c-decl.c's duplicate_decls.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |austern at apple dot com
                   |dot org                     |
           Severity|normal                      |critical
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|                            |1
   Last reconfirmed|0000-00-00 00:00:00         |2003-11-20 06:08:08
               date|                            |
   Target Milestone|---                         |3.4


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[bryner at brianryner dot com]

------- Additional Comments From bryner at brianryner dot com  2003-11-20 20:30 -------
There's another case which my patch does not fix:

------------------------------------------------------------------
extern int data1 __attribute__((visibility ("hidden")));

int data1 = 5;
------------------------------------------------------------------

data1 is emitted with default visbility, but in gcc 3.3 is given hidden visibility.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[bryner at brianryner dot com]

------- Additional Comments From bryner at brianryner dot com  2003-11-22 08:13 -------
To fix that last problem I mentioned, it looks like copying the DECL_VISIBILITY
needs to be moved outside of the (TREE_CODE (decl) == FUNCTION_DECL) block.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[bryner at brianryner dot com]

------- Additional Comments From bryner at brianryner dot com  2003-11-22 08:25 -------
There's another interesting problem here.  Suppose you had (in the global scope):

int i __attribute__((visibility ("hidden")));
int i __attribute__((visibility ("default"))) = 5;

Prior to DECL_VISIBILITY, this would cause i to have default visibility because
the later attribute value would override the earlier one.

To preserve the old behavior, you have to apply the previous decl's visibility
only if visibility was not explicitly set on the new decl.  It seems to me that
the only way to do that now would be to either:

a) Find the previous decl and apply its visibility prior to attribute handling
b) Add another bit to say whether visibility was explicitly set on a decl


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[austern at apple dot com]

------- Additional Comments From austern at apple dot com  2003-11-23 18:06 -------
I'm testing a patch right now that copies visibility from old declarations to
new ones, and that allows visibility on new declarations to override old ones,
*but* I'm treating default visibility as just the same as no visibility
declaration.  I don't like the idea of having something that's called "default"
but that gives you different behavior than leaving it out.  That strikes me as
an artifact of the old implementation.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[bryner at brianryner dot com]

------- Additional Comments From bryner at brianryner dot com  2003-11-24 03:09 -------
(In reply to comment #8)
> I'm testing a patch right now that copies visibility from old declarations to
> new ones, and that allows visibility on new declarations to override old ones,
> *but* I'm treating default visibility as just the same as no visibility
> declaration.  I don't like the idea of having something that's called "default"
> but that gives you different behavior than leaving it out.  That strikes me as
> an artifact of the old implementation

Unfortunately, that comes straight from the ELF visibility types.  DEFAULT is
one of these types.  So from that point of view, it should not work any
differently from the other visibility types.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[austern at apple dot com]

------- Additional Comments From austern at apple dot com  2003-11-24 06:16 -------
Subject: Re:  [3.4 regression] Visibility attribute is ignored

On Nov 23, 2003, at 7:09 PM, bryner at brianryner dot com wrote:

>
> ------- Additional Comments From bryner at brianryner dot com  
> 2003-11-24 03:09 -------
> (In reply to comment #8)
>> I'm testing a patch right now that copies visibility from old 
>> declarations to
>> new ones, and that allows visibility on new declarations to override 
>> old ones,
>> *but* I'm treating default visibility as just the same as no 
>> visibility
>> declaration.  I don't like the idea of having something that's called 
>> "default"
>> but that gives you different behavior than leaving it out.  That 
>> strikes me as
>> an artifact of the old implementation
>
> Unfortunately, that comes straight from the ELF visibility types.  
> DEFAULT is
> one of these types.  So from that point of view, it should not work any
> differently from the other visibility types.

To some extent it does work differently already: it's what you get if 
you don't
specify something else.  What I find inelegant is the idea that you get 
DEFAULT
either by saying nothing or by explicitly saying visibility("default"), 
but that
saying nothing means a slightly weaker thing than saying it explicitly. 
  I'd
really rather avoid having that if possible.

			--Matt



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[geoffk at gcc dot gnu dot org]

------- Additional Comments From geoffk at gcc dot gnu dot org  2003-11-24 22:08 -------
I think Bryan's testcase,

int i __attribute__((visibility ("hidden")));
int i __attribute__((visibility ("default"))) = 5;

should be an error.  Between the declarations, GCC will generate code based on the assumption 
that 'i' is defined in this shared object, which will be wrong.

ISO C makes similar constructs undefined behaviour for exactly this reason:

> If, within a translation unit, the same identifier appears with both internal and external linkage, 
the behavior is undefined. 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

Re: [Bug c/13134] [3.4 regression] Visibility attribute is ignored

[Richard Henderson]

On Mon, Nov 24, 2003 at 06:16:15AM -0000, austern at apple dot com wrote:
> What I find inelegant is the idea that you get DEFAULT either by
> saying nothing or by explicitly saying visibility("default")...

The default setting exists in gcc so that one might override the
"default" (ahem) setting with a command-line option.  Eg.

	-fvisibility=hidden

would mark all symbols hidden unless overridden by an attribute
on the actual decl.  The command-line switch never got implemented,
unfortunately.

I agree with Geoff that conflicting attributes for a given decl
ought to generate an error.


r~

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[rth at redhat dot com]

------- Additional Comments From rth at redhat dot com  2003-11-25 01:55 -------
Subject: Re:  [3.4 regression] Visibility attribute is ignored

On Mon, Nov 24, 2003 at 06:16:15AM -0000, austern at apple dot com wrote:
> What I find inelegant is the idea that you get DEFAULT either by
> saying nothing or by explicitly saying visibility("default")...

The default setting exists in gcc so that one might override the
"default" (ahem) setting with a command-line option.  Eg.

	-fvisibility=hidden

would mark all symbols hidden unless overridden by an attribute
on the actual decl.  The command-line switch never got implemented,
unfortunately.

I agree with Geoff that conflicting attributes for a given decl
ought to generate an error.


r~


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2003-11-30 00:32 -------
*** Bug 13227 has been marked as a duplicate of this bug. ***

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlo at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[carlo at gcc dot gnu dot org]

------- Additional Comments From carlo at gcc dot gnu dot org  2003-11-30 01:19 -------
Pleases note that the test case of PR 13227 was NOT fixed by the patch proposed
in this thread.

It _was_ fixed by reverting the patch of 6 Nov.

Can the one who writes a patch to fix this PR (Matt?)
please also run the test case of PR 13227
on an i686-pc-linux-gnu box?



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[pinskia at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[austern at apple dot com]

------- Additional Comments From austern at apple dot com  2003-12-01 01:10 -------
Subject: Re:  [3.4 regression] Visibility attribute is ignored

On Nov 29, 2003, at 5:19 PM, carlo at gcc dot gnu dot org wrote:

>
> ------- Additional Comments From carlo at gcc dot gnu dot org  
> 2003-11-30 01:19 -------
> Pleases note that the test case of PR 13227 was NOT fixed by the patch 
> proposed
> in this thread.
>
> It _was_ fixed by reverting the patch of 6 Nov.
>
> Can the one who writes a patch to fix this PR (Matt?)
> please also run the test case of PR 13227
> on an i686-pc-linux-gnu box?

I will indeed provide test cases when I fix this bug.  The main problem
right now is figuring out exactly what "fix" means: that is, what should
the semantics be for multiple declarations of the same symbol with
different visibilities?  The old behavior before my patch, and the new
behavior with my patch, both seem to be accidental implementation
artifacts.  Neither is documented anywhere and neither seems very easy
to justify.

There's discussion of this on the gcc@gcc.gnu.org mailing list.  I've
seen two proposals that seem reasonable: (1) It is an error for multiple
declarations to have different visibilities.  (2) A latter declaration
may override the visibility of an earlier declaration, but only by
making the visibility more restrictive.  There's some justification for
option 2 from the ELF gABI.

			--Matt



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[carlo at alinoe dot com]

------- Additional Comments From carlo at alinoe dot com  2003-12-01 15:48 -------
Subject: Re:  [3.4 regression] Visibility attribute is ignored

On Mon, Dec 01, 2003 at 01:10:19AM -0000, austern at apple dot com wrote:
> There's discussion of this on the gcc@gcc.gnu.org mailing list.  I've
> seen two proposals that seem reasonable: (1) It is an error for multiple
> declarations to have different visibilities.  (2) A latter declaration
> may override the visibility of an earlier declaration, but only by
> making the visibility more restrictive.  There's some justification for
> option 2 from the ELF gABI.
> 
> 			--Matt

Assuming that 'visibility' can be hidden or non-hidden, and that the
former means visible to all compilation units within the same shared
object and non-hidden means exported as normal, then

(1) is true when the declarations occur in different compilation units.
(2) is true when these declarations occur in the same compilation unit.

Now I assume you mean fixing compiling a single compilation unit:
the compiler only sees one compilation unit at a time - so it would not
be incorrect to go for (2).  But, given the fact multiple compilation
units exist, and therefor there is the danger of running into (1), and
(2) will only mean anything for practical cases for two declarations
where one is in the header and the other in the body of the source, I'd
go for option (1).  Because if you allow the header to specify 'non-hidden'
and only specify a hidden visibility for the definition, then why use a 
header in the first place? ... unless you intend to use that header
elsewhere too - which would announce the wrong visibility no matter
how you put it.

So, what about:

(3) If (in the same compilation unit) a declaration or definition (B)
follows a previous declarion (A), then:

It is an error if:
	- A has no visibility attribute, and B does.
	- A has a different visibility attribute than B.
It is not an error when:
	- A and B have the same visibility attribute.
	- A has a visibility attribute and B has none specified.

I hope this analysis helps.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[austern at apple dot com]

------- Additional Comments From austern at apple dot com  2003-12-01 20:15 -------
Subject: Re:  [3.4 regression] Visibility attribute is ignored

I would prefer to avoid distinguishing between "B has no visibility 
attribute specified" and "B has default visibility specified".  Any 
solution that requires distinguishing between those two 
almost-identical things means that a declaration's visibility can have 
five possible states, not four, i.e. that it requires three bits to 
describe.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[carlo at alinoe dot com]

------- Additional Comments From carlo at alinoe dot com  2003-12-01 20:37 -------
Subject: Re:  [3.4 regression] Visibility attribute is ignored

On Mon, Dec 01, 2003 at 08:16:04PM -0000, austern at apple dot com wrote:
> 
> ------- Additional Comments From austern at apple dot com  2003-12-01 20:15 -------
> Subject: Re:  [3.4 regression] Visibility attribute is ignored
> 
> I would prefer to avoid distinguishing between "B has no visibility 
> attribute specified" and "B has default visibility specified".  Any 
> solution that requires distinguishing between those two 
> almost-identical things means that a declaration's visibility can have 
> five possible states, not four, i.e. that it requires three bits to 
> describe.

I was merely thinking about the parallel with 'inline' and how I treat
that when I proposed that.  There I do not put 'inline' at a declaration
because you can't see whether or not it should be inlined:

int foo(int x);
int bar(int y);

but I add it later, at the definition of the function:

inline int
foo(x)
{
  return x + 1;
}

int bar(int y)
{
  // Huge body of 400 lines.
}

The parallel that I saw what that it is to be prefered to have
the specification only in one place, but - in the case of visibility
it would be other way around (specified in the header, and not at
the definition).  So, if you want you can argue that at the moment
you write the definition you already know the visibility (as it
follows the declaration) and might as well be forced to be specified,
just like in the case of constness of class member functions.

In other words, I see no objections to do it your prefered way.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[aj at gcc dot gnu dot org]

------- Additional Comments From aj at gcc dot gnu dot org  2003-12-02 05:43 -------
Note that with this problem glibc compilation is broken completly.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aj at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2003-12-10 06:34 -------
Subject: Bug 13134

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	austern@gcc.gnu.org	2003-12-10 06:34:46

Modified files:
	gcc            : ChangeLog c-decl.c 
	gcc/cp         : ChangeLog decl.c 
	gcc/testsuite  : ChangeLog 
	gcc/testsuite/lib: gcc-dg.exp target-supports.exp 
Added files:
	gcc/testsuite/g++.dg/ext: visibility-1.C visibility-2.C 
	                          visibility-3.C visibility-4.C 
	                          visibility-5.C visibility-6.C 
	                          visibility-7.C 
	gcc/testsuite/gcc.dg: visibility-1.c visibility-2.c 
	                      visibility-3.c visibility-4.c 
	                      visibility-5.c visibility-6.c 
	                      visibility-7.c 

Log message:
	PR c/13134
	* c--decl.c (duplicate_decls): Copy visibility flag when appropriate.
	* cp/decl.c (duplicate_decls): Copy visibility flag when appropriate.
	* testsuite/lib/gcc-dg.exp (dg-require-visibility): Define.
	* testsuite/lib/target-supports (check_visibility_available): Define.
	* testsuite/gcc.dg/visibility-1.c: New test.
	* testsuite/gcc.dg/visibility-2.c: Likewise.
	* testsuite/gcc.dg/visibility-3.c: Likewise.
	* testsuite/gcc.dg/visibility-4.c: Likewise.
	* testsuite/gcc.dg/visibility-5.c: Likewise.
	* testsuite/gcc.dg/visibility-6.c: Likewise.
	* testsuite/g++.dg/ext/visibility-1.C: Likewise.
	* testsuite/g++.dg/ext/visibility-2.C: Likewise.
	* testsuite/g++.dg/ext/visibility-3.C: Likewise.
	* testsuite/g++.dg/ext/visibility-4.C: Likewise.
	* testsuite/g++.dg/ext/visibility-5.C: Likewise.
	* testsuite/g++.dg/ext/visibility-6.C: Likewise.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.1969&r2=2.1970
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/c-decl.c.diff?cvsroot=gcc&r1=1.458&r2=1.459
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/ChangeLog.diff?cvsroot=gcc&r1=1.3791&r2=1.3792
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/decl.c.diff?cvsroot=gcc&r1=1.1158&r2=1.1159
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3239&r2=1.3240
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-1.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-2.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-3.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-4.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-5.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-6.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/ext/visibility-7.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-2.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-3.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-4.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-5.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-6.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/visibility-7.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/lib/gcc-dg.exp.diff?cvsroot=gcc&r1=1.24&r2=1.25
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/lib/target-supports.exp.diff?cvsroot=gcc&r1=1.7&r2=1.8



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134

[Bug c/13134] [3.4 regression] Visibility attribute is ignored

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2003-12-10 15:05 -------
Fixed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13134