This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFC: Make dllimport/dllexport imply default visibility

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

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.

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

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.

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

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.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]