Consider the following file: class AAA { public: virtual ~AAA(); }; class BBB { public: virtual ~BBB(); }; class xyz : public AAA, public BBB { public: xyz() : AAA(), BBB() { } }; Class xyz has no key method, so its vtable should be emitted only in translation units where it's used. With "gcc -O0 -c foo.cc" I get what I expect: the compiler doesn't emit _ZTV3xyz, _ZN3xyzD0Ev, or anything else. With "gcc -O2 -c foo.cc" I get incorrect behavior: the compiler emits xyz's destructor and vtable, thunks, and all of the things in AAA and BBB that are required to support them. This is a regression. In 3.2.2, O0 and O2 both behave correctly.
Mark/Jan, what is the expected behavior here, this is a regression from 3.3. It is caused by unit-at-a- time always trying to emit the vtable.
This is a bug in unit-at-a-time. I believe this actually makes the compiler non-conformant to the C++ standard because it will result in generating functions for which no generation is possible. That might be incorrect, but this is at the very least a serious QoI problem. Jan?
I haven't yet looked at whether this means the compiler fails to conform to the C++ standard, but I'm sure it means we now fail to conform to the Itanium C++ ABI.
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 austern at apple dot com wrote: >------- Additional Comments From austern at apple dot com 2004-03-19 04:02 ------- >I haven't yet looked at whether this means the compiler fails to conform to the C++ standard, but I'm >sure it means we now fail to conform to the Itanium C++ ABI. > > Yes, indeed.
Subject: Re: New: Incorrect emission of unused compiler-generated destructor at -O2 > Consider the following file: > class AAA { > public: > virtual ~AAA(); > }; > > class BBB { > public: > virtual ~BBB(); > }; > > class xyz : public AAA, public BBB { > public: > xyz() : AAA(), BBB() { } > }; > > > > > Class xyz has no key method, so its vtable should be emitted only in translation units where it's used. > With "gcc -O0 -c foo.cc" I get what I expect: the compiler doesn't emit _ZTV3xyz, _ZN3xyzD0Ev, or > anything else. With "gcc -O2 -c foo.cc" I get incorrect behavior: the compiler emits xyz's destructor > and vtable, thunks, and all of the things in AAA and BBB that are required to support them. > > This is a regression. In 3.2.2, O0 and O2 both behave correctly. This can be avoided by disabling following mark_referenced call: Index: cp/method.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/cp/method.c,v retrieving revision 1.279 diff -c -3 -p -r1.279 method.c *** cp/method.c 12 Mar 2004 17:09:01 -0000 1.279 --- cp/method.c 20 Mar 2004 15:27:33 -0000 *************** use_thunk (tree thunk_fndecl, bool emit_ *** 359,365 **** this translation unit. */ TREE_ADDRESSABLE (function) = 1; mark_used (function); ! mark_referenced (DECL_ASSEMBLER_NAME (function)); if (!emit_p) return; --- 359,365 ---- this translation unit. */ TREE_ADDRESSABLE (function) = 1; mark_used (function); ! /*mark_referenced (DECL_ASSEMBLER_NAME (function));*/ if (!emit_p) return; The problem is that C++ frotnend manages thunks "on the side" and does not show to backend what DECLs will be used by thunks and what thunks will be emit. This is dealt with handling the depenedencies conservativly. This line comes from replacing DECL_REFERENCED.* = 1 by mark_referenced call so I am not quite sure it is still necesary (testsuite passes without this line). However we likely need some way to explain backend the hidden dependencies. Would it be possible to explain me why exactly the thunk needs to be output and what DECLs will it reference? (I know that the thunks are associated to functions and are output at the same time the function is output and they become needed by use_thunk, but this all can be overly conservative now) Honza > > -- > Summary: Incorrect emission of unused compiler-generated > destructor at -O2 > Product: gcc > Version: 3.5.0 > Status: UNCONFIRMED > Severity: normal > Priority: P2 > Component: c++ > AssignedTo: unassigned at gcc dot gnu dot org > ReportedBy: austern at apple dot com > CC: gcc-bugs at gcc dot gnu dot org > GCC build triplet: i686-pc-linux-gnu > GCC host triplet: i686-pc-linux-gnu > GCC target triplet: i686-pc-linux-gnu > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639
Subject: Re: New: Incorrect emission of unused compiler-generated destructor at -O2 Jan Hubicka wrote: >>Consider the following file: >>class AAA { >>public: >> virtual ~AAA(); >>}; >> >>class BBB { >>public: >> virtual ~BBB(); >>}; >> >>class xyz : public AAA, public BBB { >>public: >> xyz() : AAA(), BBB() { } >>}; >> >> >> >> >>Class xyz has no key method, so its vtable should be emitted only in translation units where it's used. >>With "gcc -O0 -c foo.cc" I get what I expect: the compiler doesn't emit _ZTV3xyz, _ZN3xyzD0Ev, or >>anything else. With "gcc -O2 -c foo.cc" I get incorrect behavior: the compiler emits xyz's destructor >>and vtable, thunks, and all of the things in AAA and BBB that are required to support them. >> >>This is a regression. In 3.2.2, O0 and O2 both behave correctly. >> >> > >This can be avoided by disabling following mark_referenced call: > >Index: cp/method.c >=================================================================== >RCS file: /cvs/gcc/gcc/gcc/cp/method.c,v >retrieving revision 1.279 >diff -c -3 -p -r1.279 method.c >*** cp/method.c 12 Mar 2004 17:09:01 -0000 1.279 >--- cp/method.c 20 Mar 2004 15:27:33 -0000 >*************** use_thunk (tree thunk_fndecl, bool emit_ >*** 359,365 **** > this translation unit. */ > TREE_ADDRESSABLE (function) = 1; > mark_used (function); >! mark_referenced (DECL_ASSEMBLER_NAME (function)); > if (!emit_p) > return; > >--- 359,365 ---- > this translation unit. */ > TREE_ADDRESSABLE (function) = 1; > mark_used (function); >! /*mark_referenced (DECL_ASSEMBLER_NAME (function));*/ > if (!emit_p) > return; > >The problem is that C++ frotnend manages thunks "on the side" and does >not show to backend what DECLs will be used by thunks and what thunks >will be emit. This is dealt with handling the depenedencies >conservativly. This line comes from replacing DECL_REFERENCED.* = 1 by >mark_referenced call so I am not quite sure it is still necesary >(testsuite passes without this line). > >However we likely need some way to explain backend the hidden >dependencies. Would it be possible to explain me why exactly the thunk >needs to be output and what DECLs will it reference? >(I know that the thunks are associated to functions and are output at >the same time the function is output and they become needed by >use_thunk, but this all can be overly conservative now) > > Every thunk references exactly one function. The thunk is just a little stub that adjusts one incoming parameter, and then transfers control to the referenced function. From a FUNCTION_DECL, you can look at DECL_THUNKS to find all of its thunks. From a thunk, you can look at THUNK_TARGET to find the function to which it transfers control. It does make sense to me that we call mark_referenced in the case that emit_p is zero. If emit_p is 1, that does make sense. In the emit_p == 0 case, we should wait to emit the thunk until we know that we need to emit the vtable referencing it, which is what we used to do.
As mentioned in the audit trail, with this bug "we now fail to conform to the Itanium C++ ABI." Mark, wouldn't that make this bug a definite show stopper for 3.4.0, and can this bug therefore be retargeted for 3.4.0?
Subject: Re: New: Incorrect emission of unused compiler-generated destructor at -O2 > Every thunk references exactly one function. The thunk is just a little > stub that adjusts one incoming parameter, and then transfers control to > the referenced function. OK, that was my original understanding but somehow I got to impression that it is more complex. So when the function itself gets marked as necesary we automatically emit the thunk when the mark_used_thunk has been called, right? I guess we need two thinks - we need to manage that once we discover thunk to be necesary we need to mark the function it is refering to and we need to emit thunks iff they are really reachable (not just used, right)? It would be ideal if the thunk was behaving like ordinary function, but for some reason it is not. Is there way to recognize thunk in C++ independent way? I guess we will need kind of hooks - tell cgraph code that given decl is thunk and thunk is like function but not quite as it has no body and reffers to other function and it is not necesay to assemble it as it gets assembled magically with the function body. This is quite nasty (I really don't want to introduce new kind of function - we already have way too many of them), but does it at least sound right to you? > > From a FUNCTION_DECL, you can look at DECL_THUNKS to find all of its > thunks. From a thunk, you can look at THUNK_TARGET to find the function > to which it transfers control. It does make sense to me that we call > mark_referenced in the case that emit_p is zero. If emit_p is 1, that > does make sense. In the emit_p == 0 case, we should wait to emit the > thunk until we know that we need to emit the vtable referencing it, > which is what we used to do. Hmm, perhaps I am getting overzelaous here (hope so). I am not quite sure how emit_p works. When the emit_p is set? If it is at function expansion time, it is too late - we need to be finished this in advance. Honza > > -- > Mark Mitchell > CodeSourcery, LLC > (916) 791-8304 > mark@codesourcery.com
Subject: Re: New: Incorrect emission of unused compiler-generated destructor at -O2 > So when the function itself gets marked as necesary we automatically > emit the thunk when the mark_used_thunk has been called, right? Yes. Whenever a function is emittted, so are all of its associated thunks. > I guess we need two thinks - we need to manage that once we discover > thunk to be necesary we need to mark the function it is refering to and > we need to emit thunks iff they are really reachable (not just used, > right)? Yes. > It would be ideal if the thunk was behaving like ordinary function, but > for some reason it is not. Is there way to recognize thunk in C++ > independent way? No. But, you could always change things so that was possible, I suppose. Thunks cannot be represented as ordinary functions until we have multiple entry points, and we don't yet have that. > I guess we will need kind of hooks - tell cgraph code that given decl is > thunk and thunk is like function but not quite as it has no body and > reffers to other function and it is not necesay to assemble it as it > gets assembled magically with the function body. Yes. > Hmm, perhaps I am getting overzelaous here (hope so). I am not quite > sure how emit_p works. When the emit_p is set? If it is at function > expansion time, it is too late - we need to be finished this in advance. emit_p is set from emit_associated_thunks, which is called for a function's thunks right before the function itself is sent to tree_rest_of_compilation.
Subject: Re: New: Incorrect emission of unused compiler-generated destructor at -O2 > >So when the function itself gets marked as necesary we automatically > >emit the thunk when the mark_used_thunk has been called, right? > > Yes. Whenever a function is emittted, so are all of its associated thunks. > > >I guess we need two thinks - we need to manage that once we discover > >thunk to be necesary we need to mark the function it is refering to and > >we need to emit thunks iff they are really reachable (not just used, > >right)? > > Yes. > > >It would be ideal if the thunk was behaving like ordinary function, but > >for some reason it is not. Is there way to recognize thunk in C++ > >independent way? > > No. But, you could always change things so that was possible, I > suppose. Thunks cannot be represented as ordinary functions until we > have multiple entry points, and we don't yet have that. > > >I guess we will need kind of hooks - tell cgraph code that given decl is > >thunk and thunk is like function but not quite as it has no body and > >reffers to other function and it is not necesay to assemble it as it > >gets assembled magically with the function body. > > Yes. > > >Hmm, perhaps I am getting overzelaous here (hope so). I am not quite > >sure how emit_p works. When the emit_p is set? If it is at function > >expansion time, it is too late - we need to be finished this in advance. > > emit_p is set from emit_associated_thunks, which is called for a > function's thunks right before the function itself is sent to > tree_rest_of_compilation. Mark, I am trying to look into it, but I am not able to figure out testcase where the patch above fails. It should have function that is referenced by thunk. Additionally the thunk needs to be output but the function itself does not (if there wasn't the reference from the thunk). In that case I would expect us to fail to see the dependency in between thunk and function and not output it at all with the patch applied. Do you have idea how such testcase shall look like? I am really not very faimiliar with these details of C++ implementation making it somewhat dificult to track this down resonably. Thanks, Honza
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 >I am trying to look into it, but I am not able to figure out testcase >where the patch above fails. It should have function that is referenced >by thunk. Additionally the thunk needs to be output but the function >itself does not (if there wasn't the reference from the thunk). In that >case I would expect us to fail to see the dependency in between thunk >and function and not output it at all with the patch applied. > >Do you have idea how such testcase shall look like? I am really not >very faimiliar with these details of C++ implementation making it >somewhat dificult to track this down resonably. > > There are three kinds of entities in play: (1) Vtables (2) Virtual functions (3) Thunks A vtable is an array, containing pointers to either virtual functions or thunks. Thunks are little stubs that reference a virtual function. A virtual function and its associated thunks are always emitted together. Are you sure this is a problem with thunks and not with vtables? In the test case, the object file should be empty. There are no definitions of any functions except for the inline xyz::xyz method, and since it is inline it will not be emitted. I expect that the bug is that cgraph is incorrectly deciding to emit a vtable. Having decided that, you are emitting everything else because it is pointed to from the vtable. The way to solve this problem is to figure out what thing you are deciding to output first. Since the object file should be entirely empty, outputting anything at all is a bug. I've retargeted this at 3.4.0, as not solving this will cause incredible amounts of code bloat for C++ applications.
Retargeted at 3.4.0.
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 > > ------- Additional Comments From mark at codesourcery dot com 2004-03-23 21:25 ------- > Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect > emission of unused compiler-generated destructor at -O2 > > > >I am trying to look into it, but I am not able to figure out testcase > >where the patch above fails. It should have function that is referenced > >by thunk. Additionally the thunk needs to be output but the function > >itself does not (if there wasn't the reference from the thunk). In that > >case I would expect us to fail to see the dependency in between thunk > >and function and not output it at all with the patch applied. > > > >Do you have idea how such testcase shall look like? I am really not > >very faimiliar with these details of C++ implementation making it > >somewhat dificult to track this down resonably. > > > > > There are three kinds of entities in play: > > (1) Vtables > > (2) Virtual functions > > (3) Thunks > > A vtable is an array, containing pointers to either virtual functions or > thunks. Thunks are little stubs that reference a virtual function. A > virtual function and its associated thunks are always emitted together. > > Are you sure this is a problem with thunks and not with vtables? Yes. The way cgraph code works is giving cgraph_finalize_function/cgraph_finalize_variable calls that adds function/variable into cgraph datastructure. It is not directly output to file as we used to do, instead cgraph decide when it is good time to do so (at the end of compilation for unit-at-a-time or immediately in most cases for non-unit-at-a-time) and if do it at all. Both vtables and functions are not output until they are needed (either externally visiable or referenced by some datastructure or function that is already needed). Problem with thunks is that they go on side and they are neither functions nor variables. The old code contained several TREE_SYMBOL_REFERENCED(...)=1 calls that are now replaced by mark_referenced (...) calls that makes cgraph code to believe that the functions/vtables are referenced by some already output assembly and thus they get scheduled to be output as well unconditionally. > > In the test case, the object file should be empty. There are no > definitions of any functions except for the inline xyz::xyz method, and > since it is inline it will not be emitted. This happens with the patch I sent earlier. It just removes one of mark_referenced calls in the C++ backend. There are 4 such calls and I am unsure if they are actaully needed or not. I am seeking for testcase exhibiting that mark_referenced at that place is needed. (I've originally replaced them from the TREE_SYMBOL_REFERENCED and didn't really checked it without the change). > > I expect that the bug is that cgraph is incorrectly deciding to emit a > vtable. Having decided that, you are emitting everything else because > it is pointed to from the vtable. It goes by C++ frontend deciding to emit vtable in C++ frontend notion (so it is delayed until needed by cgraph), this makes C++ frontend to mark_used the thunk in the vtable that in turn makes mark_referenced the function and cgraph code gets fooled. We simply need to figure way to do the trick without using mark_referenced bit and instead making the dependency obvious to cgraph. The thunks are referenced only by vtables, right? Perhaps the thunks should not be mark_used at all during the C++ frontend is looking for used thinks and we can go directly to the thunked functions. Later we can we can catch it inside mark_referenced via C++ hook that will mark_used the thunk. That way we will again output only thunks that are referenced by vtables that are output to assembly file. At the moment cgraph code works by actually outputting all global variables until all needed functions are known, so such patch would work for 3.4. For 3.5 I would preffer to reorganize things in a way so variables are output very last so we can take into account the scenarios where variable used to be reachable but optimizers elliminated all references to it. But for 3.5 we may want to get the multiple entry points working too. > > The way to solve this problem is to figure out what thing you are > deciding to output first. Since the object file should be entirely > empty, outputting anything at all is a bug. > > I've retargeted this at 3.4.0, as not solving this will cause incredible > amounts of code bloat for C++ applications. How much incredible it is? (ie having testcase that exhibits a lot of unnecesary thunks/vtables would be usefull too). Honza > > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 >>In the test case, the object file should be empty. There are no >>definitions of any functions except for the inline xyz::xyz method, and >>since it is inline it will not be emitted. > > > This happens with the patch I sent earlier. It just removes one of > mark_referenced calls in the C++ backend. There are 4 such calls and I > am unsure if they are actaully needed or not. > I am seeking for testcase exhibiting that mark_referenced at that place > is needed. (I've originally replaced them from the > TREE_SYMBOL_REFERENCED and didn't really checked it without the change). Thanks for your explanation. I agree that it does not make sense for use_thunk to call "mark_referenced" on the function to which the thunk transfers control in the "emit_p == 0" case. That should only happen when the thunk is actually output, i.e, in the "emit_p == 1" case, because in that case the thunk has been directly generated into the assembly file and the function really is needed. (Eventually, it would be better to move that under the control of cgraph, but that is probably too invasive a change right now.) So, if you make that change does it fix the problem? > How much incredible it is? (ie having testcase that exhibits a lot of > unnecesary thunks/vtables would be usefull too). Just take the original test case, but add thousands of classes. That's a very typical situation for some class frameworks.
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 > > ------- Additional Comments From mark at codesourcery dot com 2004-03-24 18:44 ------- > Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect > emission of unused compiler-generated destructor at -O2 > > >>In the test case, the object file should be empty. There are no > >>definitions of any functions except for the inline xyz::xyz method, and > >>since it is inline it will not be emitted. > > > > > > This happens with the patch I sent earlier. It just removes one of > > mark_referenced calls in the C++ backend. There are 4 such calls and I > > am unsure if they are actaully needed or not. > > I am seeking for testcase exhibiting that mark_referenced at that place > > is needed. (I've originally replaced them from the > > TREE_SYMBOL_REFERENCED and didn't really checked it without the change). > > Thanks for your explanation. > > I agree that it does not make sense for use_thunk to call > "mark_referenced" on the function to which the thunk transfers control > in the "emit_p == 0" case. > > That should only happen when the thunk is actually output, i.e, in the > "emit_p == 1" case, because in that case the thunk has been directly > generated into the assembly file and the function really is needed. > (Eventually, it would be better to move that under the control of > cgraph, but that is probably too invasive a change right now.) > > So, if you make that change does it fix the problem? It will fix the problem because the thunk is only output at the time function is assembled. But it will make mark_referenced useless anyway as when the function is assembled it is already mark_reference-d. Removing the call as done by my patch should be fine then, but we must ensure that referencing the vtable that references the thunk makes cgraph code notice that the function referenced by the thunk is referenced too, so the dependencies goes right. I am not quite sure how to construct testcase, but I will try to dig into it. Obviously the testsuite works so it seems to be fine in majority of case but my experience is that there are common dead ends not covered by testsuite in these areas, so I still feel unsafe about that patch, but all my attempts to construct testcase seems to be failing. > > > How much incredible it is? (ie having testcase that exhibits a lot of > > unnecesary thunks/vtables would be usefull too). > > Just take the original test case, but add thousands of classes. That's > a very typical situation for some class frameworks. We obvisouly emit unnecesary thunks, lets see what we can do about this. I tried to compile mozilla with the patch fixing the testcase and measured no difference in code size tought... Honza > > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 hubicka at ucw dot cz wrote: > It will fix the problem because the thunk is only output at the time > function is assembled. But it will make mark_referenced useless anyway > as when the function is assembled it is already mark_reference-d. > Removing the call as done by my patch should be fine then, but we must > ensure that referencing the vtable that references the thunk makes > cgraph code notice that the function referenced by the thunk is > referenced too, so the dependencies goes right. I am not quite sure how > to construct testcase, but I will try to dig into it. Obviously the > testsuite works so it seems to be fine in majority of case but my > experience is that there are common dead ends not covered by testsuite > in these areas, so I still feel unsafe about that patch, but all my > attempts to construct testcase seems to be failing. I am not sure that the case you are worried about can happen. For that to happen, there would have to be a vtable that contains a reference to a thunk, but no references to the function to which the thunk refers. The only way a thunk to C::f can occur is in a vtable for C, or a base subobject of C. But, in that case, C::f will also be referenced directly from the vtable. >>>How much incredible it is? (ie having testcase that exhibits a lot of >>>unnecesary thunks/vtables would be usefull too). >> >>Just take the original test case, but add thousands of classes. That's >>a very typical situation for some class frameworks. > > > We obvisouly emit unnecesary thunks, lets see what we can do about this. > I tried to compile mozilla with the patch fixing the testcase and > measured no difference in code size tought... Many of the "unncessary" thunks are required by the ABI. The ABI guarantees that all thunks are put out with the function to which they transfer control. Based on our discussion, I think your patch is OK. Please check it in, but remove the call to mark_referenced copmletely, rather than commenting it out. Thanks,
Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 > > ------- Additional Comments From mark at codesourcery dot com 2004-03-26 08:23 ------- > Subject: Re: [3.4/3.5 Regression] [unit-at-a-time] Incorrect > emission of unused compiler-generated destructor at -O2 > > hubicka at ucw dot cz wrote: > > > It will fix the problem because the thunk is only output at the time > > function is assembled. But it will make mark_referenced useless anyway > > as when the function is assembled it is already mark_reference-d. > > Removing the call as done by my patch should be fine then, but we must > > ensure that referencing the vtable that references the thunk makes > > cgraph code notice that the function referenced by the thunk is > > referenced too, so the dependencies goes right. I am not quite sure how > > to construct testcase, but I will try to dig into it. Obviously the > > testsuite works so it seems to be fine in majority of case but my > > experience is that there are common dead ends not covered by testsuite > > in these areas, so I still feel unsafe about that patch, but all my > > attempts to construct testcase seems to be failing. > > I am not sure that the case you are worried about can happen. > > For that to happen, there would have to be a vtable that contains a > reference to a thunk, but no references to the function to which the > thunk refers. The only way a thunk to C::f can occur is in a vtable for > C, or a base subobject of C. But, in that case, C::f will also be > referenced directly from the vtable. > > >>>How much incredible it is? (ie having testcase that exhibits a lot of > >>>unnecesary thunks/vtables would be usefull too). > >> > >>Just take the original test case, but add thousands of classes. That's > >>a very typical situation for some class frameworks. > > > > > > We obvisouly emit unnecesary thunks, lets see what we can do about this. > > I tried to compile mozilla with the patch fixing the testcase and > > measured no difference in code size tought... > > Many of the "unncessary" thunks are required by the ABI. The ABI > guarantees that all thunks are put out with the function to which they > transfer control. > > Based on our discussion, I think your patch is OK. Please check it in, > but remove the call to mark_referenced copmletely, rather than > commenting it out. OK, thanks! I will try to investigate the remaining 3 calls if they can cause similar showstopper too. Honza > > Thanks, > > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
Subject: Bug 14639 CVSROOT: /cvs/gcc Module name: gcc Branch: gcc-3_4-branch Changes by: hubicka@gcc.gnu.org 2004-03-28 15:41:57 Modified files: gcc/cp : ChangeLog method.c Log message: PR C++/14639 * method.c (use_think): Do not mark thunk as referenced. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3892.2.88&r2=1.3892.2.89 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/method.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.275.4.1&r2=1.275.4.2
This bug still keeps parts of Qt from building on Darwin. The problem got worse after the tree-ssa merge. Before the merge, the unused symbols would get generated if the code fragment were compiled at -O2; the symbols wouldn't be generated if -fno-unit-at-a-time was added to the compile line. With the current top-of-tree, the symbols are always produced -- -O0, -O2, and with or without -fno-unit-at-a-time.
Actually it was the merge which changed how unit-at-a-time is done but rather Zack's patch which removed a large number of setting TREE_SYMBOL_REFERENCED.
Here is the patch for 3.5.0 after Zack's patch: Index: method.c =============================================================== ==== RCS file: /cvs/gcc/gcc/gcc/cp/method.c,v retrieving revision 1.283 diff -u -p -r1.283 method.c --- method.c 24 May 2004 21:07:42 -0000 1.283 +++ method.c 3 Jun 2004 23:34:11 -0000 @@ -353,7 +353,6 @@ use_thunk (tree thunk_fndecl, bool emit_ this translation unit. */ TREE_ADDRESSABLE (function) = 1; mark_used (function); - mark_decl_referenced (function); if (!emit_p) return; Should I apply this Jan/Mark?
Subject: Re: [3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 pinskia at gcc dot gnu dot org wrote: > ------- Additional Comments From pinskia at gcc dot gnu dot org 2004-06-03 23:35 ------- > Here is the patch for 3.5.0 after Zack's patch: > Index: method.c > =============================================================== > ==== > RCS file: /cvs/gcc/gcc/gcc/cp/method.c,v > retrieving revision 1.283 > diff -u -p -r1.283 method.c > --- method.c 24 May 2004 21:07:42 -0000 1.283 > +++ method.c 3 Jun 2004 23:34:11 -0000 > @@ -353,7 +353,6 @@ use_thunk (tree thunk_fndecl, bool emit_ > this translation unit. */ > TREE_ADDRESSABLE (function) = 1; > mark_used (function); > - mark_decl_referenced (function); > if (!emit_p) > return; > > Should I apply this Jan/Mark? If it passes tests, yes, please.
That makes sense. I hadn't seen the problem until recently.
Subject: Bug 14639 CVSROOT: /cvs/gcc Module name: gcc Changes by: pinskia@gcc.gnu.org 2004-06-03 23:45:46 Modified files: gcc/cp : ChangeLog method.c Log message: 2004-06-03 Andrew Pinski <pinskia@physics.uc.edu> Jan Hubicka <jh@suse.cz> PR c++/14639 * method.c (use_think): Do not mark thunk as referenced. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/ChangeLog.diff?cvsroot=gcc&r1=1.4080&r2=1.4081 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/method.c.diff?cvsroot=gcc&r1=1.283&r2=1.284
Fixed.
After applying Andrew's fix, compiling Matt's fragments at the top of the radar with gcc -c and checking symbols in the resulting .o file showed: With -O2, has unnecessary symbols. With -O0, the unnecessary symbols ARE generated. With -O2 -fno-unit-at-a-time, the unnecessar
After applying Andrew's fix, compiling Matt's fragments at the top of the radar with gcc -c and checking symbols in the resulting .o file showed: With -O2: does not generate unnecessary symbols. With -O0, generates unnecessary symbols. With -O2, generates unnecessary symbols With -O0, generates unnecessary symbols In addition, Qt still fails to build.
Reopening for now.
Ok this now only happens with -fno-unit-at-a-time, investigating.
For completeness, my report should have said: With -O2: does not generate unnecessary symbols. With -O0, generates unnecessary symbols. With -O2 -fno-unit-at-a-time, generates unnecessary symbols With -O0 -fno-unit-at-a-time, generates unnecessary symbols So with Andrew's first patch -O2 with unit-at-a-time worked, but all other conditions failed.
Now I really wish non unit-at-a-time goes away.
This should fix it by making non-unit-at-a-time more like unit-at-a-time also removes one ??? to fix the rest of the way because now we mark the variables when outputing the asm so there is no need to do it while analyzing the function. I have not tested or bootstrapped this yet, Robert could you test this as I have a test to study for/do? Index: passes.c =============================================================== ==== RCS file: /cvs/gcc/gcc/gcc/passes.c,v retrieving revision 2.17 diff -u -p -r2.17 passes.c --- passes.c 30 May 2004 18:32:27 -0000 2.17 +++ passes.c 4 Jun 2004 01:45:17 -0000 @@ -370,7 +370,7 @@ rest_of_decl_compilation (tree decl, || (flag_unit_at_a_time && DECL_INITIAL (decl))) && !DECL_EXTERNAL (decl)) { - if (flag_unit_at_a_time && !cgraph_global_info_ready + if (!cgraph_global_info_ready && TREE_CODE (decl) != FUNCTION_DECL && top_level) cgraph_varpool_finalize_decl (decl); else Index: cgraphunit.c =============================================================== ==== RCS file: /cvs/gcc/gcc/gcc/cgraphunit.c,v retrieving revision 1.64 diff -u -p -r1.64 cgraphunit.c --- cgraphunit.c 30 May 2004 18:32:24 -0000 1.64 +++ cgraphunit.c 4 Jun 2004 01:45:17 -0000 @@ -395,11 +395,6 @@ record_call_1 (tree *tp, int *walk_subtr switch (TREE_CODE (t)) { case VAR_DECL: - /* ??? Really, we should mark this decl as *potentially* referenced - by this function and re-examine whether the decl is actually used - after rtl has been generated. */ - if (TREE_STATIC (t)) - cgraph_varpool_mark_needed_node (cgraph_varpool_node (t)); break; case ADDR_EXPR:
The test case passes fine, but the first compilation in the Qt installation gets stuck in an infinite loop in finish_file. The first part of the patch (in passes.c) is in rest_of_decl_compilation, so I'd guess that's the cause. I'll try backing out that change, and see what happens. Here's part of the sample: 248 Thread_110b 248 start 248 _start 248 toplev_main 248 do_compile 248 compile_file 248 c_common_parse_file 248 finish_file 248 walk_namespaces_r 239 wrapup_global_declarations 150 wrapup_global_declarations 72 rest_of_decl_compilation 39 cgraph_varpool_finalize_decl 25 cgraph_varpool_node 13 htab_find_slot_with_hash 11 htab_find_slot_with_hash 2 eq_varpool_node 2 eq_varpool_node 7 cgraph_varpool_node 3 htab_find_slot 3 htab_find_slot
Can you try with my patch to fix PR 15721: <http://gcc.gnu.org/ml/gcc-patches/2004-06/ msg00088.html> to see if that helps, most likely what is going on now is that the wrapup_global_declarations is not being smart as it should be.
I should note that the my patch to toplevel.c is needed because that is where the outputing is happening in first place.
I applied the three patches suggested to a top-of-tree compiler -- the changes to method.c, the changes to passes.c and toplev.c, and the changes included in PR/15721. Qt fails to link when trying to compile moc, the first executable compiled. The failure shows many more symbols missing than before patching. On the bright side, the small test case attached to the bug still shows no symbols being output (as expected.) I did not attempt to run the testsuite on it or bootstrap.
The reappearance of the bug may have appeared after 5/27/04 because it doesn't appear on the lno- branch. The lno-branch merged with mainline around then.
The only patch which changed the C++ front-end was mine which changed how to check if a decl is needed or not :( I still think non-unit-at-a-time is should go away for 3.5.0.
I don't particularly care about -fno-unit-at-a-time; we were using it to workaround existing bugs. As long as the default behavior produces appropriate symbols, I don't care what happens to -fno-unit-at- a-time.
Subject: Re: [3.5 Regression] [unit-at-a-time] Incorrect emission of unused compiler-generated destructor at -O2 > > ------- Additional Comments From pinskia at gcc dot gnu dot org 2004-06-03 23:35 ------- > Here is the patch for 3.5.0 after Zack's patch: > Index: method.c > =============================================================== > ==== > RCS file: /cvs/gcc/gcc/gcc/cp/method.c,v > retrieving revision 1.283 > diff -u -p -r1.283 method.c > --- method.c 24 May 2004 21:07:42 -0000 1.283 > +++ method.c 3 Jun 2004 23:34:11 -0000 > @@ -353,7 +353,6 @@ use_thunk (tree thunk_fndecl, bool emit_ > this translation unit. */ > TREE_ADDRESSABLE (function) = 1; > mark_used (function); > - mark_decl_referenced (function); > if (!emit_p) > return; > > Should I apply this Jan/Mark? This is fine with me, but Mark approved the patch originally only for 3.4 (that is why i didn't applied it to mainline) but perhaps he meant both? :) Honza > > -- > What |Removed |Added > ---------------------------------------------------------------------------- > Keywords| |ABI > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
Subject: Re: [3.5 Regression] [non-unit-at-a-time] Incorrect emission of unused compiler-generated destructor > > ------- Additional Comments From pinskia at gcc dot gnu dot org 2004-06-04 01:50 ------- > This should fix it by making non-unit-at-a-time more like unit-at-a-time also removes one ??? to fix > the rest of the way because now we mark the variables when outputing the asm so there is no need to > do it while analyzing the function. I have not tested or bootstrapped this yet, Robert could you test this > as I have a test to study for/do? > > Index: passes.c > =============================================================== > ==== > RCS file: /cvs/gcc/gcc/gcc/passes.c,v > retrieving revision 2.17 > diff -u -p -r2.17 passes.c > --- passes.c 30 May 2004 18:32:27 -0000 2.17 > +++ passes.c 4 Jun 2004 01:45:17 -0000 > @@ -370,7 +370,7 @@ rest_of_decl_compilation (tree decl, > || (flag_unit_at_a_time && DECL_INITIAL (decl))) > && !DECL_EXTERNAL (decl)) > { > - if (flag_unit_at_a_time && !cgraph_global_info_ready > + if (!cgraph_global_info_ready > && TREE_CODE (decl) != FUNCTION_DECL && top_level) > cgraph_varpool_finalize_decl (decl); > else > Index: cgraphunit.c > =============================================================== > ==== > RCS file: /cvs/gcc/gcc/gcc/cgraphunit.c,v > retrieving revision 1.64 > diff -u -p -r1.64 cgraphunit.c > --- cgraphunit.c 30 May 2004 18:32:24 -0000 1.64 > +++ cgraphunit.c 4 Jun 2004 01:45:17 -0000 > @@ -395,11 +395,6 @@ record_call_1 (tree *tp, int *walk_subtr > switch (TREE_CODE (t)) > { > case VAR_DECL: > - /* ??? Really, we should mark this decl as *potentially* referenced > - by this function and re-examine whether the decl is actually used > - after rtl has been generated. */ > - if (TREE_STATIC (t)) > - cgraph_varpool_mark_needed_node (cgraph_varpool_node (t)); The mark call here must stay (it is the only place we do the reachability analysis in the case we will drop it here compiler will break at least if we get past the callback from the asm outputting code (I am currently testing patch to do it but it has problems still...) The "needed" in cgraph sense is "conservatively needed" so the Richard comment should be probably placed elsewhere as this is defect of callgraph code affecting both functions and variables. The problem is that at the moment cgraph code is trusting to "needed" flag for variables and output them early instead of later pass really checking whether they are really needed (ie still referenced by the optimized body of function). The reason why it is done this way is that I still rely on backend calling me back when variable is needed as it is quite non-trivial to do the reachability analysis on all kinds of datastructures output by the front-end. I am working on that as we speak, but it will take time (I just hit problems with Java and objective C bypassing the cgraph as I feared) Honza > break; > > case ADDR_EXPR: > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
I just tried the code fragment on a mainline build for 2004-05-26 07:00, and I get the behavior I'd seen for the last month -- things break with -O2, but -fno-unit-at-a-time makes things work right. This suggests something happened between 5/26 and today that triggered this specific problem. I'll see if I can further reduce where the problem appeared.
Subject: Re: [3.5 Regression] [non-unit-at-a-time] Incorrect emission of unused compiler-generated destructor > > ------- Additional Comments From hubicka at ucw dot cz 2004-06-04 21:11 ------- > Subject: Re: [3.5 Regression] [non-unit-at-a-time] Incorrect emission of unused compiler-generated destructor > > > > > ------- Additional Comments From pinskia at gcc dot gnu dot org 2004-06-04 01:50 ------- > > This should fix it by making non-unit-at-a-time more like unit-at-a-time also removes one ??? to fix > > the rest of the way because now we mark the variables when outputing the asm so there is no need to > > do it while analyzing the function. I have not tested or bootstrapped this yet, Robert could you test this > > as I have a test to study for/do? > > > > Index: passes.c > > =============================================================== > > ==== > > RCS file: /cvs/gcc/gcc/gcc/passes.c,v > > retrieving revision 2.17 > > diff -u -p -r2.17 passes.c > > --- passes.c 30 May 2004 18:32:27 -0000 2.17 > > +++ passes.c 4 Jun 2004 01:45:17 -0000 > > @@ -370,7 +370,7 @@ rest_of_decl_compilation (tree decl, > > || (flag_unit_at_a_time && DECL_INITIAL (decl))) > > && !DECL_EXTERNAL (decl)) > > { > > - if (flag_unit_at_a_time && !cgraph_global_info_ready > > + if (!cgraph_global_info_ready > > && TREE_CODE (decl) != FUNCTION_DECL && top_level) > > cgraph_varpool_finalize_decl (decl); > > else > > Index: cgraphunit.c > > =============================================================== > > ==== > > RCS file: /cvs/gcc/gcc/gcc/cgraphunit.c,v > > retrieving revision 1.64 > > diff -u -p -r1.64 cgraphunit.c > > --- cgraphunit.c 30 May 2004 18:32:24 -0000 1.64 > > +++ cgraphunit.c 4 Jun 2004 01:45:17 -0000 > > @@ -395,11 +395,6 @@ record_call_1 (tree *tp, int *walk_subtr > > switch (TREE_CODE (t)) > > { > > case VAR_DECL: > > - /* ??? Really, we should mark this decl as *potentially* referenced > > - by this function and re-examine whether the decl is actually used > > - after rtl has been generated. */ > > - if (TREE_STATIC (t)) > > - cgraph_varpool_mark_needed_node (cgraph_varpool_node (t)); > > The mark call here must stay (it is the only place we do the > reachability analysis in the case we will drop it here compiler will > break at least if we get past the callback from the asm outputting code > (I am currently testing patch to do it but it has problems still...) > > The "needed" in cgraph sense is "conservatively needed" so the Richard > comment should be probably placed elsewhere as this is defect of > callgraph code affecting both functions and variables. The problem is > that at the moment cgraph code is trusting to "needed" flag for > variables and output them early instead of later pass really checking > whether they are really needed (ie still referenced by the optimized > body of function). The reason why it is done this way is that I still > rely on backend calling me back when variable is needed as it is quite > non-trivial to do the reachability analysis on all kinds of > datastructures output by the front-end. I am working on that as we > speak, but it will take time (I just hit problems with Java and > objective C bypassing the cgraph as I feared) OK, while writing the long philophical reply I forgot to ask. Steven, did you tracked down what is going wrong? We might get around by not marking functions as needed in non-unit-at-a-time but this is also going to break with changes I want to do, so I would rather preffer to have some better sollution. Honza > > Honza > > break; > > > > case ADDR_EXPR: > > > > > > -- > > > > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > > > ------- You are receiving this mail because: ------- > > You are on the CC list for the bug, or are watching someone who is. > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14639 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is.
Problem also doesn't appear on 2004-06-01 version of mainline.
Problem does appear on 2004-06-03 version of mainline.
The problem appears to be with one of these two patches to cp-tree.h. Checking out mainline from 0602 20:30 causes the problem to appear; reverting cp-tree back to version 1.970 cures the problem. revision 1.972 date: 2004/06/02 20:24:30; author: pinskia; state: Exp; lines: +1 -1 2004-06-02 Andrew Pinski <pinskia@physics.uc.edu> * cp-tree.h: Fix typo. ---------------------------- revision 1.971 date: 2004/06/02 19:20:03; author: pinskia; state: Exp; lines: +5 -2 2004-06-02 Andrew Pinski <pinskia@physics.uc.edu> * cp-tree.h: Include cgraph.h (DECL_NEEDED_P): Use cgraph_*node on the decl instead of TREE_SYMBOL_REFERENCED on the DECL_ASSEMBLER_NAME of the decl.
I'd be happy either getting the bug fixed or the offending changes reverted. The situation before 5/29 at least had a workaround (-fno-unit-at-a-time), enabling us to test that the rest of Qt could build and do timing measurements of 3.5.
No that should not be reverted just yet, there is something else wrong, I think the decl is being marked too soon as needed as shown by my last patch. Support two ways of figuring out if a decl is need is too much a maintainiablity nightmare which is the current situation is which Zack is trying to remove in his patches and I am just helping doing and also speed up the compiler at the same time by removing calls to decl_assembler_name (DECL_ASSEMBLER_NAME).
Created attachment 6528 [details] patch to revert the other patch This patch reverts my patch which causes this regression. We will have to wait to get this done a differenet way.
Patch posted here: <http://gcc.gnu.org/ml/gcc-patches/2004-06/msg01094.html>.
Subject: Bug 14639 CVSROOT: /cvs/gcc Module name: gcc Changes by: pinskia@gcc.gnu.org 2004-06-15 20:52:01 Modified files: gcc/cp : ChangeLog cp-tree.h Log message: 2004-06-12 Andrew Pinski <apinski@apple.com> PR c++/14639 Revert: 2004-06-02 Andrew Pinski <pinskia@physics.uc.edu> * cp-tree.h: Fix typo. * cp-tree.h: Include cgraph.h (DECL_NEEDED_P): Use cgraph_*node on the decl instead of TREE_SYMBOL_REFERENCED on the DECL_ASSEMBLER_NAME of the decl. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/ChangeLog.diff?cvsroot=gcc&r1=1.4099&r2=1.4100 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cp/cp-tree.h.diff?cvsroot=gcc&r1=1.975&r2=1.976