Re: Protected visibility for glib



Hi Dan

Thanks for the reply.  Hopefully I can clarify some things.


On Mon, 2010-03-15 at 10:54 -0400, Dan Winship wrote:
> On 03/14/2010 10:52 PM, Ryan Lortie wrote:
> >   - replace G_BEGIN_DECLS/G_END_DECLS with "protected" visibility
> >     pragmas.  This means that any symbol declared in a header file
> >     will end up with protected visibility.
> 
> which means what exactly? The gcc docs only say that "`protected' and
> `internal' are pretty useless in real-world usage".

It means that they are public but can't be overridden with an
LD_PRELOAD.  This "won't be overridden" assumption makes it safe for the
compiler and linker to make all internal symbol usage static-links
without the PLT indirection.

> It looks to me like "must be used" here means "if you don't use it, it
> will still compile, and 'make check' will still pass, and then it will
> silently fall back to doing a PLT lookup at runtime", right? People
> *will* forget to use it in new code. It would be nice if you could turn
> that into a compile/link-time error. (Maybe by renaming or hiding the
> symbol in the header file when compiling glib?)

Actually, this:

/usr/bin/ld: .libs/gvarianttypeinfo.o: relocation R_X86_64_PC32 against
protected symbol `g_str_hash' can not be used when making a shared
object


I actually consider this to be annoying.  The linker could easily "just
work" here (albeit operating in violation of the ELF ABI -- which we
already do with our explicit runaround under both galias and the new
system).  There have been upstream bugs about this but there seems to be
no motion at present.  Resolving this issue would mean that we didn't
have to use the G_INTERNAL_FUNC() stuff at all and would still have all
the benefits.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19520
http://sources.redhat.com/bugzilla/show_bug.cgi?id=584

Note that the ld bug was "resolved" by simply disallowing the behaviour
we desire:

http://sourceware.org/ml/binutils-cvs/2005-02/msg00011.html

To be honest, I'm not sure how GCC could possibly solve the problem.  ld
could definitely solve it.  Probably there is room for a linker flag or
something here.


> Actually, if you renamed the symbol in the header file when compiling
> glib, would you even need G_INTERNAL_FUNC? Just have some macro
> before/after/around the declaration of g_str_hash that is a no-op when
> #including the installed header, but during glib compilation would
> "#define g_str_hash IA_g_str_hash" and change its visibility.
>
> Which I guess is sort of equivalent to: if there are only 8 functions
> that need renaming in the new system, then couldn't we just maintain
> galias.h by hand?

That's certainly a possibility but I avoided it specifically because I'm
trying to reduce the number of files in the tree that cause
total-rebuilds whenever they're touched.  The benefit here, of course,
would be that this file would be touched much less often.

The drawback here is that we encounter the existing
macro-and-function-with-same-name problem that we have for galias.

Also, I'm strongly against any change that adds any sort of cruft to the
public-installed header files.

> > I've only made changes to the glib/ directory -- not
> > gobject/ or gio/ (although those could easily use the same technique).
> 
> gobject and gio also have this problem where their pltcheck stuff needs
> to include a complete list of all of the libglib-2.0.so functions that
> they call, for some reason. Does that go away?

No.  The problem there is that since libglib and libgobject are separate
libraries the calls from gobject to glib are necessarily resolved at
runtime.  We could avoid having PLT entries for these calls but then
we'd be doing relocations directly on program text -- I'll take the PLT
entries, thanks. :)

Funny that you mention this though -- just this morning I was thinking
about this effort that was being considered a year or two ago to merge
all of the glib libraries into one big library specifically to fix this.

> >   - Makes it easier for people not to update the symbols file properly. 
That's true.

Cheers



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]