Re: RFC: Make dllimport/dllexport imply default visibility

[Mark Mitchell <mark at codesourcery dot com>]

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.

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.  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 don't know who created the SymbianOS semantics, but they are
implemented in ARM's RealView compiler, which is the compiler used to
build SymbianOS and most of the applications for that system.  Those
semantics go back to pre-ELF SymbianOS but are preserved in the current
ELF-based SymbianOS explicitly in terms of ELF visibility.  They're
documented, at least partially, in various places, including the ARM
EABI and the RealView compiler.  They can't be changed because there are
millions of lines of code that require them.

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

>>> I hope you don't mean that there are other system's compilers using the
>>> '__attribute__((visibility))' syntax in a way that's incompatible with
>>> GCC.  If there are, I think the appropriate response is a combination of
>>> fixincludes and a polite e-mail asking them to stop.
>>
>> I don't know if there are, but I certainly wouldn't be surprised.  The
>> GNU attribute syntax is implemented in the EDG front end and there are
>> lots of EDG-based compilers.
> 
> This doesn't quite count, because EDG implements it to be compatible
> with GCC, and I'm sure if you find it isn't then they'll fix it (or at
> least acknowledge it as a bug).

EDG licensees routinely modify the front end.  The attribute handling is
specifically designed to be easy to extend.  ARM has already confirmed
(on this list) that the RealView behavior is intentional.  It seems
unlikely that it will go away.

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

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?

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

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713