Re: RFC: Make dllimport/dllexport imply default visibility

[Geoff Keating]

On 03/07/2007, at 9:18 PM, Mark Mitchell wrote:
Geoffrey Keating wrote:
On 03/07/2007, at 7:37 PM, Mark Mitchell wrote:

Geoffrey Keating wrote:

Yes.  __attribute__((visibility)) has consistent GNU semantics, and
other features (eg. -fvisibility-ms-compat, __dllspec) match other
compilers.

The only semantics that make sense on SymbianOS are the ones that  
allow
default visibility within a hidden class.

I think we're talking past each other.  What semantics exactly are
these?  Who created them?  Where are they implemented?  Are they
documented?  Who documented them?  Why can't they be changed?

SymbianOS allows you to declare a member dllimport or dllexport even
though the class is declared with hidden visibility.  dllimport and
dllexport imply default visibility.

OK.  So we are *not* talking about __attribute__((visibility)), we are  
talking about dllimport, dllexport, and (importantly) notshared.

The semantics, as I described earlier in this thread, are simple: the
visibility you specify for the class is the default for all members,
including vtables and other compiler-generated stuff, but the user may
explicitly override that by specifying a visibility for the member.

... these are not exactly 'semantics'.  They are a description of an  
implementation, and rely on other undescribed features of the  
implementation, like "this implementation does not strictly check the  
ODR".  If the implementation changed in future, it would be very  
unclear as to how, or whether, to preserve these properties.

 The
semantics I describe are a conservative extension to the semantics you
describe; they are operationally identical, except that the semantics
you describe forbid giving a member more visibility than its class.   
The
change that I made was to stop G++ from silently ignoring such a  
request.

I'm not sure you've really described the semantics.  For this  
extension to be actually useful, you're going to want an ODR exception  
like the one for -fvisibility-ms-compat, otherwise you won't be able  
to do things like access fields of the class or other members without  
invoking undefined behaviour.  Then, what happens if you write this  
and it turns out to not quite be the same as what the other compiler  
does, but people are already relying on the meaning you documented/ 
implemented?

 So, whether or not there's a
command-line option, it's going to be on by default, and therefore  
is
going to be inconsistent with a system on which GCC disallows (or
ignores) that.

Maybe, and maybe not.  In particular, an option that changes  
defaults is
one thing, but if the user overrides the default with GCC-specific
syntax and the compiler does something different, that's another  
thing
altogether.

Here, the compiler was silently ignoring an attribute explicitly given
by the user.  I changed the compiler not to ignore the attribute.   
That
did not alter the GNU semantics, unless we consider it an intentional
feature that we ignored the attribute.  (I can't imagine that we would
consider that a feature; if for some reason we are going to refuse to
let users declare members with more visibility than specified for the
class, we should at least issue an error message.)

Yes, you've convinced me that there should be an error message.

RealView 3.0.x doesn't support the visibility attribute, but it  
does
support other GNU attributes.

So it can't conflict.

I don't find that a convincing argument.  The two compilers have
different syntaxes for specifying ELF visibility.  But, they meant the
same thing in all respects (so far as I know) except for this case.

I expect they're different in a lot of other cases too.

For example, in the GNU semantics it's clear that you can have:

struct S __attribute__((visibility("hidden")));
void f(struct S *p) { };

in two different shared libraries, and they do not conflict, and it so  
happens that on ELF systems this is implemented by automatically  
marking 'f' as hidden.

It also so happens that if you write

struct S __attribute__((visibility("hidden")));
inline void f() {
  struct S * p;
};

that the compiler is permitted to optimise this by making 'f' hidden,  
because 'f' can be defined and used only in this shared object.  The  
compiler does not presently do this, but it could, and in future it  
probably will.

This is why I was asking about documentation.  From GCC's visibility  
documentation, you can deduce what is a valid program, what is an  
invalid program, and what the valid programs do.

 We
could invent some alternative attribute to mark a class "hidden, but  
in
the SymbianOS way that allows you to override the visibility of  
members,
not in the GNU way that doesn't", but that seems needlessly complex.

It's really not that complex.  In fact, we already have syntax for  
this attribute, "__declspec(notshared)".  Or, at least, it's only as  
complex as implementing the semantics in the first place.

Concretely, are you arguing that my patch was a bad change?  If so, do
you feel that silently discarding the attribute on members was a good
thing?  Do you feel that preventing users from making members more
visible than classes must be an error, rather than just considering it
in questionable taste?

I note that it's not documented.  I think it resembles a number of  
other 'undocumented GCC extensions' (in many cases, just fossilised  
bugs) which have caused trouble in the past.  I think that if you  
tried to actually document it and explain how it modifies the  
language, you would find that this was complex and that at the end  
there's no guarantee that what you finally documented was equivalent  
to what other SymbianOS compilers do.

These same problems were encountered when documenting -fvisibility-ms- 
compat.  The complexity was dealt with by hard thinking.  The problem  
of being inconsistent with the other compiler was dealt with by  
explicitly saying from the start that the aim is to be consistent, and  
so it's a bug if it's not consistent.  This lets users know (a) what  
is a bug and (b) that they can't rely on those bugs being preserved.

If you have -fvisibility-ms-compat switched on, then

struct S {
 void f() __dllspec(dllimport);
};

That, however, is not quite the case in question; rather it's:

 struct S __declspec(notshared) {
   void f() __declspec(dllimport); // Or dllexport
 };

The class itself is explicitly declared with hidden visibility.   
And, as
far as I know, there's no desire on SymbianOS to make vtables and such
visible, even when class members are not, which seems to be the goal  
of
-fvisibility-ms-compat.

The goal of -fvisibility-ms-compat is written in the documentation: it  
is to be compatible with Visual Studio.  It is not to twiddle certain  
bits in the output .o files.  It is not to make certain internal data  
structures, visible or not visible.  It provides certain user-visible  
semantics; how it does this is an implementation detail.

This goal was reached through a fair bit of experience.  For example,  
originally some people thought that -fvisibility=hidden would produce  
compatibility with some other compilers (I think it was actually  
Visual Studio that time too).  But they didn't write this as an  
explicit goal anywhere, and it turned out that (a) it wasn't  
compatible and (b) now other users of GCC were relying on what they  
did write down and so it shouldn't be changed. This turned out to be a  
good thing, because the current meaning of -fvisibility=hidden is IMO  
better and the Visual Studio semantics are IMO broken, but it meant  
there was a delay in getting them what they wanted.


So, I think that in this case, the right thing to do is to:

1. Document __declspec(notshared).  Say that on ARM it is supposed to  
be compatible with the other SymbianOS compilers.  Say how you think  
it works, perhaps in terms of visibility(hidden), perhaps sui  
generis.  Either way, the documentation should be complete enough to  
let any user know what programs are valid and what they do.
2. Implement it to do this.  That shouldn't be hard compared to step  
(1).
3. Produce an error when a user tries to explicitly mark something as  
visible when doing so is not useful in the GNU semantics.  In the case  
we've been discussing, to be most helpful the error should say that  
probably the class should have been marked as visible.