Re: Final intermodule patch

[Mark Mitchell <mark at codesourcery dot com>]

On Fri, 2003-07-11 at 18:28, Geoff Keating wrote:
> Mark Mitchell <mark@codesourcery.com> writes:
> 
> > On Fri, 2003-07-11 at 15:18, Neil Booth wrote:
> > > Geoffrey Keating wrote:-
> > > 
> > > > 	* cpplib.c (undefine_macros): New.
> > > > 	(cpp_undef_all): New.
> > > 
> > > This is problematic as memory is never freed.
> > 
> > I think the intermodule patch is somewhat flawed, despite it's very
> > laudable goals.
> 
> One memory leak does not a flawed patch make.

I'm sorry; because I quoted Neil's message my intent was unclear.

Certainly the one memory leak is a bug, but it is not a structural
defect!  So, I agree with your statement.

The flawed aspect of the patch, in my opinion, is the attempt to output
a single .s file from multiple input files.

> > The patch I commited to make_decl_rtl will break the new functionality,
> > in that static functions and variables will no longer be renamed apart. 
> > But, they should not be renamed anyhow.
> > 
> > Doing so will break debugging ("break foo" will not work)
> 
> I was hoping that DWARF-2 could handle this.  If not, I note that you
> just have to write 'break foo<tab>' in GDB, and GDB will add the
> suffix.  As a bonus, if you have two 'foo's, you can now specify which
> one you want to break in.

It's not just GDB.  It's nm, and anywhere else where people depend on
the names of functions with internal linkage.  It's valgrind or Purify,
when showing backtraces.  

And unless you compile your *entire* program in a single go, you may
still have two "foo" functions anyhow.

In C++, which I know is not affected by the current patch, demangling
would break, for example.  One could argue about whether or not the ABI
requires particular manglings for functions with internal linkage, but,
until now, every compiler has mangled internal and external functions
identically and this would change.  (And I presume that we should pick a
mechanism which works well for languages other than C.)

> and it breaks
> > the kinds of assembly tricks that crtstuff.c does -- and on which lots
> > of code relies.
> 
> We can easily find a solution for crtstuff.c.  My preferred solution
> is to add 'asm' names to anything that's explicitly referenced from
> assembler.
> 
> I do not believe that there is a lot of code doing the kind of tricks
> that crtstuff.c uses.  If there was, whoever owns it is very used to
> it breaking with every GCC release.

Why?  As far as I know, GCC has also named a static "foo" function as
"foo".  It's never been "foo.n" until now.

> > Unfortunately, if we try to generate a single .s file we have no choice
> > but to rename the functions apart.
> > 
> > The solution is to generate multiple .s files.  If, for example,
> > cross-module inlining is going to be performed, fine -- but you still
> > get multiple .s files out.  If a static entity in one .o file needs to
> > be reached from another .o file, an external-linkage alias for the
> > entity should be defined with a funny name.
> 
> That only works on systems that can make aliases of symbols.  Darwin
> can't, for instance.

It's unfortunate that Darwn and PE/COFF have these kinds of limitations,
but they do, and there's not much we can do about that.  

However, there is already functionality that only works on ELF (or
ELF-like) systems, and that's OK.  If this functionality only worked
there, that would be OK with me -- although of course for Apple that's
not a satisfactory solution.

Darwin could use a thunk instead of an alias.  Or perhaps rename the
static entity and give it external linkage, which is similar to your
current patch, but would then only change names on systems whose object
file formats could not support aliases.

> > (All of these issues arise in the idea of "template repositories" in
> > C++, and Cfront had lots of misadventures in this area, so we have prior
> > art to learn from when we add this new functionality.)
> 
> Um, this really isn't like template instantiation.  It's more like
> namespaces, but even there the parallel isn't exact.

I think that it is very much like template repositories.  The issue
there is that Cfront instantiated separate template instances into
separate object files -- but templates are allowed to reference entities
with internal linkage.  That's very much like performing
cross-translation-unit optimizations which might result in the need to
reference entities with internal linkage from functions that originally
were in a different translation unit.

> > I think this patch should probably be reverted until these issues get
> > sorted out.
> 
> It would have been better had this been discussed months ago when I
> first proposed this patch!

Indeed.  I didn't notice the original thread, I'm afraid.  It would be
better if we had a better mechanism for proposing major changes and
their designs and getting consensus on them -- an email list with
hundreds of messages per day is a little big for that.

However, that doesn't alter my opinion: this patch should be reverted
until we can sort out what is to be done.  Or, to save work in the event
the patch ends up staying in, we should leave it in but debate whether
or not to take it out and file a high-priority 3.4 PR to decide what to
do.

I am not sure how hard multiple .s file output would be to arrange. 
It's possible that it would be mechanical, but not too difficult.  For
each entity to be output, check its DECL_CONTEXT, and output to the .s
file with the appropriate TRANSLATION_UNIT_DECL.  The other bit of work
is the OS-specific hook to arrange aliasing for static entities, which
is simple on ELF.  If you were to agree to do this, I would drop my
objections to this patch, and go back to simply celebrating the fact
that we can now do cross-module optimization!

-- 
Mark Mitchell <mark@codesourcery.com>
CodeSourcery, LLC