This is the mail archive of the gcc-patches@gcc.gnu.org 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: IMA: backward-compatible behavior from lhd_set_decl_assembler_name



On 07/07/2004, at 12:44 PM, Zack Weinberg wrote:


Geoff Keating <geoffk@geoffk.org> writes:

I considered pretty much those exact options when I was writing the
patch that created c_static_assembler_name.  I rejected option (3) as
an unacceptable performance penalty in the single-file case.

I agree there.


I rejected option (2) because it adds extra paths in the C frontend
which would be very lightly tested---to fully test the compiler, you
would have to create an extra dejagnu C torture option that adds a
trivial .c file to the compile command.

I don't see how (1) and (2) are different here -- it's the same lightly-tested extra path, but in a different place.

No! (1) is five lines of code. (2) is every place in the compiler that looks at DECL_CONTEXT.


  To verify this I
implemented (2) -- patch is attached; most of the bulk is reverting
the ill-advised parts of the previous patch -- and indeed it was a
matter of adding one clause to the controlling expression of an
if-statement that already existed.

I do agree that we've got a lightly-tested code path here, but I mean
to fix that by adding an IMA test suite at the same time that I
actually turn IMA back on (hopefully that will be tomorrow).



So far as I know, all language-independent code:

1. Creates exactly one of whatever it creates, and therefore can
safely use NULL DECL_CONTEXT; or
2. Creates many instances and has some way of naming them uniquely
(like a counter) and therefore can also safely use NULL DECL_CONTEXT.

Ah, but no. Mudflap, for instance, *thinks* it is creating one of its things per object file, but because mudflap_finish_file is being called from c_objc_common_finish_file, it is actually creating one of its things per input translation unit, and since they all get the same symbol name, the assembler barfs.

The cure for this, however, is to move the call to mudflap_finish_file
to a point where it will only be called once per object file - that's
the next patch in my queue, once we get this current issue resolved.
In principle you are correct that language-independent code can safely
use NULL DECL_CONTEXT - which is why I said in my original message
that I thought (2) was now safe.

The bulk of the work I have been doing to date is to stop the C front
end - language *dependent* code - from instantiating
DECL_ASSEMBLER_NAME too early, when DECL_CONTEXT for file-scope
statics is still null merely because the TRANSLATION_UNIT_DECL hasn't
been created yet.

Right. That's why when I wrote IMA, I created the T_U_D before creating any decls, so that you don't have half-finished decls lying around. You changed that as part of a fix to, I believe, PCH, which was broken by your previous fix to a bunch of problems, which itself was necessary because of a previous fix of yours. I never did get an explanation as to what the original patch improved; I hope for your sake that it was worth it.


I believe I objected to at least half of those fixes. Your designs were not sound, and you are not following good software engineering practises. If you continue on this path, I expect that it will be months or years before we have C frontend that works in all the cases it used to.

As a final question, is there something that you're not telling us?
Based on the changes you've been making to the C frontend recently, it
looks like you're working on some significant enhancement or new
feature, because otherwise it makes no sense to make them; you seem to
be spending a lot of time breaking things and fixing them again with
little visible benefit.

Nope. I am doing no more and no less than is necessary to fix IMA, and you yourself warned me about most of the problems I have been fixing, the ones where DECL_ASSEMBLER_NAME would be set too early. Some of the patches should have compile-time performance benefits independent of IMA, but that's not a current goal of mine (my management has instructed me to finish fixing IMA first).

So, why on earth are you doing it? Wouldn't it be much faster and much easier to revert back to the C frontend as it existed before you started changing it?



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