This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
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
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.