From: Dave N. <dc...@us...> - 2007-10-22 21:57:05
|
I'm revising the problem of mis-annotation of a static inline function: http://marc.info/?l=oprofile-list&m=115838605919567&w=2 <http://marc.info/?l=oprofile-list&m=115838605919567&w=2> The submitter is pushing back that they need this problem fixed. In the testing that I've done the call to bfd_find_nearest_line() (in libutil++/bfd_support.cpp:find_nearest_line) seems to be finding the correct line number (of the inlined function) but is_correct_function() is noticing that sym.name() and the function name do not match and returns FALSE which causes the sample to be attributed to main(). I'm wondering if the following change to bfd_support.cpp wouldn't be sufficient to fix this problem: --- save/bfd_support.cpp 2007-10-22 10:55:20.000000000 -0500 +++ bfd_support.cpp 2007-10-22 16:13:45.000000000 -0500 @@ -542,7 +542,7 @@ find_nearest_line(bfd_info const & b, op if (!ret || !cfilename || !function) goto fail; - if (!is_correct_function(function, sym.name())) + if (!is_correct_function(function, sym.name()) && linenr == 0) goto fail; if (linenr == 0) { Do you see any problems with this fix? It will allow is_correct_function() to handle whatever special case it was designed to handle, and if that fails, trust the non-zero linenr returned by bfd_find_nearest_line. |
From: John L. <le...@mo...> - 2007-10-22 22:22:19
|
On Mon, Oct 22, 2007 at 02:55:39PM -0700, Dave Nomura wrote: > I'm revising the problem of mis-annotation of a static inline function: > http://marc.info/?l=oprofile-list&m=115838605919567&w=2 > <http://marc.info/?l=oprofile-list&m=115838605919567&w=2> > The submitter is pushing back that they need this problem fixed. In the > testing that I've done the call to bfd_find_nearest_line() (in > libutil++/bfd_support.cpp:find_nearest_line) seems to be finding the > correct line number (of the inlined function) but is_correct_function() > is noticing that sym.name() and the function name do not match and > returns FALSE which causes the sample to be attributed to main(). > > I'm wondering if the following change to bfd_support.cpp wouldn't be > sufficient to fix this problem: > --- save/bfd_support.cpp 2007-10-22 10:55:20.000000000 -0500 > +++ bfd_support.cpp 2007-10-22 16:13:45.000000000 -0500 > @@ -542,7 +542,7 @@ find_nearest_line(bfd_info const & b, op > if (!ret || !cfilename || !function) > goto fail; > > - if (!is_correct_function(function, sym.name())) > + if (!is_correct_function(function, sym.name()) && linenr == 0) > goto fail; > > if (linenr == 0) { > > Do you see any problems with this fix? It will allow > is_correct_function() to handle whatever special case it was designed to > handle, and if that fails, trust the non-zero linenr returned by > bfd_find_nearest_line. This won't work. In the case fixed by is_correct_function() (which is commented above), we will have a linenr. So you're just regressing the previous problem (if it's still a problem - maybe libbfd got better?) I don't know of a way to fix this without parsing DWARF ourselves. regards john |
From: Dave N. <dc...@us...> - 2007-10-22 23:06:56
|
John Levon wrote: > On Mon, Oct 22, 2007 at 02:55:39PM -0700, Dave Nomura wrote: > > >> .... >> I'm wondering if the following change to bfd_support.cpp wouldn't be >> sufficient to fix this problem: >> --- save/bfd_support.cpp 2007-10-22 10:55:20.000000000 -0500 >> +++ bfd_support.cpp 2007-10-22 16:13:45.000000000 -0500 >> @@ -542,7 +542,7 @@ find_nearest_line(bfd_info const & b, op >> if (!ret || !cfilename || !function) >> goto fail; >> >> - if (!is_correct_function(function, sym.name())) >> + if (!is_correct_function(function, sym.name()) && linenr == 0) >> goto fail; >> >> if (linenr == 0) { >> >> Do you see any problems with this fix? It will allow >> is_correct_function() to handle whatever special case it was designed to >> handle, and if that fails, trust the non-zero linenr returned by >> bfd_find_nearest_line. >> > > This won't work. In the case fixed by is_correct_function() (which is > commented above), we will have a linenr. So you're just regressing the > previous problem (if it's still a problem - maybe libbfd got better?) > So, I guess the regression case would be when is_correct_function()==false and linenr != 0. Do you have any details on what this case is, so that I could look into a solution that handles both? > I don't know of a way to fix this without parsing DWARF ourselves. > Is parsing DWARF out of the question? gdb seems to be able to correctly find the source line info for static inlines by looking at (bfd)->tdata.elf_obj_data but I'm not sure if it had to do some work to initialize that structure. If we have to choose between find_nearest_line() handling this other special case, or static inlines, which is the more common case? I would think static inlines would be pretty common since optimization creates them, and the reason someone is using oprofile, is to improve their program's performance. |
From: John L. <le...@mo...> - 2007-10-22 23:48:58
|
On Mon, Oct 22, 2007 at 04:06:19PM -0700, Dave Nomura wrote: > > This won't work. In the case fixed by is_correct_function() (which is > > commented above), we will have a linenr. So you're just regressing the > > previous problem (if it's still a problem - maybe libbfd got better?) > > > So, I guess the regression case would be when > is_correct_function()==false and linenr != 0. > Do you have any details on what this case is, so that I could look into > a solution that handles both? Like I said it's described in the comment above and in the bug listed there too. Phil might be able to remember more. > > I don't know of a way to fix this without parsing DWARF ourselves. > > Is parsing DWARF out of the question? It would totally kill performance I think. > gdb seems to be able to correctly > find the source line info for static inlines by looking at > (bfd)->tdata.elf_obj_data but I'm not sure if it had to do some work to > initialize that structure. !! So gdb just ignores the API and digs into bfd's inner workings??? That is not nice. > If we have to choose between find_nearest_line() handling this other > special case, or static inlines, which is the more common case? > > I would think static inlines would be pretty common since optimization > creates them, and the reason someone is using oprofile, is to improve > their program's performance. It's probably more common. I don't think we knew they were broken previously. john |
From: Dave N. <dc...@us...> - 2007-10-23 17:48:42
|
John Levon wrote: > On Mon, Oct 22, 2007 at 04:06:19PM -0700, Dave Nomura wrote: > > .... > >>> I don't know of a way to fix this without parsing DWARF ourselves. >>> >> Is parsing DWARF out of the question? >> > > It would totally kill performance I think. > > >> gdb seems to be able to correctly >> find the source line info for static inlines by looking at >> (bfd)->tdata.elf_obj_data but I'm not sure if it had to do some work to >> initialize that structure. >> > > !! > > So gdb just ignores the API and digs into bfd's inner workings??? That > is not nice. > I'm guessing that there is a place in that tdata structure to store a pointer to whatever you want. But it appears that gdb has to do a bunch of processing of the dwarf information and reference the condensed information in the tdata structure. I agree that this is probably too expensive. > >> If we have to choose between find_nearest_line() handling this other >> special case, or static inlines, which is the more common case? >> >> I would think static inlines would be pretty common since optimization >> creates them, and the reason someone is using oprofile, is to improve >> their program's performance. >> > > It's probably more common. I don't think we knew they were broken > previously. > So it appears we have two cases: 1) due to lack of symbolic debug information in some modules, bfd_find_nearest_line finds some unrelated line number and the samples get attributed to that line number, 2) if there is a static inline function called then the samples get annotated to the caller rather than the inline function source. In older versions of gcc(3.3.3), the function name returned by bfd_get_nearest_line for an inlined function was the same as the caller, so symbol_name == function_name and is_correct_function() returned true, and we got the correct line number. In newer versions of gcc(4.1.2) the inlined function_name is correctly identified. 1) is clearly misleading but perhaps not that common, and 2) is probably more common, but at least the attribution can be rationalized. If we can't handle both cases without the DWARF processing overhead, do you want to stick with handling 1), or switch to handling the more common case 2)? -- Dave Nomura LTC Linux Power Toolchain |
From: John L. <le...@mo...> - 2007-10-23 19:25:54
|
On Tue, Oct 23, 2007 at 10:40:15AM -0700, Dave Nomura wrote: > >So gdb just ignores the API and digs into bfd's inner workings??? That > >is not nice. > > > I'm guessing that there is a place in that tdata structure to store a > pointer to whatever you want. But it appears that gdb has to do a bunch > of processing of the dwarf information and reference the condensed > information in the tdata structure. I agree that this is probably too > expensive. Can you investigate this in more detail? > 1) is clearly misleading but perhaps not that common, and 2) is probably > more common, but at least the attribution can be rationalized. > If we can't handle both cases without the DWARF processing overhead, do > you want to stick with handling 1), or switch to handling the more > common case 2)? If we have to, we'll choose the more common case. regards john |
From: Santos, J. R. G <jos...@hp...> - 2007-10-23 19:47:33
|
=20 >> So it appears we have two cases: >> 1) due to lack of symbolic debug information in some=20 >> modules, bfd_find_nearest_line finds some unrelated line=20 >> number and the samples get attributed to that line number, >> 2) if there is a static inline function called then the=20 >> samples get annotated to the caller rather than the inline=20 >> function source. >> In older versions of gcc(3.3.3), the function name returned=20 >> by bfd_get_nearest_line for an inlined function was the same=20 >> as the caller, so symbol_name =3D=3D function_name and=20 >> is_correct_function() returned true, and we got the correct=20 >> line number. In newer versions of gcc(4.1.2) the inlined=20 >> function_name is correctly identified. >>=20 >> 1) is clearly misleading but perhaps not that common, and 2)=20 >> is probably more common, but at least the attribution can be=20 >> rationalized. >> If we can't handle both cases without the DWARF processing=20 >> overhead, do you want to stick with handling 1), or switch=20 >> to handling the more common case 2)? >>=20 Dave and John If case2 is addressed, it would be nice to have a command option to enable the user to choose how inline function samples should be accounted (either attributed to themselves or to the parent function). I found this "bug" is sometimes a "feature" and was very handy in some of my experiments. Sometimes is good to have a smaller number of functions aggregating a larger number of samples. I would be disappointed if this "feature" was removed. :) Not sure if you were already planning this... Thanks Renato |
From: Anton B. <an...@sa...> - 2007-10-25 05:58:48
|
Hi, > If case2 is addressed, it would be nice to have a command option to > enable the user to choose how inline function samples should be > accounted (either attributed to themselves or to the parent function). I > found this "bug" is sometimes a "feature" and was very handy in some of > my experiments. Sometimes is good to have a smaller number of functions > aggregating a larger number of samples. I would be disappointed if this > "feature" was removed. :) Not sure if you were already planning this... I don't see how it could be considered a feature :) You are depending on compiler behaviour here and the function may not get inlined even if you mark it. Build to build your samples could get attributed to the function or the parent function, leading to very confusing results. Could you use callgraphs for this instead? Anton |
From: Santos, J. R. G <jos...@hp...> - 2007-10-25 16:15:28
|
=20 >> -----Original Message----- >> From: Anton Blanchard [mailto:an...@sa...]=20 >> Sent: Wednesday, October 24, 2007 10:54 PM >> To: Santos, Jose Renato G >> Cc: dc...@us...; John Levon;=20 >> opr...@li... >> Subject: Re: mis-annotated inlined function >>=20 >>=20 >> Hi, >> =20 >> > If case2 is addressed, it would be nice to have a command=20 >> option to=20 >> > enable the user to choose how inline function samples should be=20 >> > accounted (either attributed to themselves or to the=20 >> parent function).=20 >> > I found this "bug" is sometimes a "feature" and was very=20 >> handy in some=20 >> > of my experiments. Sometimes is good to have a smaller number of=20 >> > functions aggregating a larger number of samples. I would be=20 >> > disappointed if this "feature" was removed. :) Not sure if=20 >> you were already planning this... >>=20 >> I don't see how it could be considered a feature :) You are=20 When you profile the kernel you usually have a large number of functions to look at. It is very difficult to reason about what is going on looking at so many functions. We really need a way to group functions that are related in classes such as skbuff, mem, scheduler, network, etc... The current behaviour of inline functions is just useful to help you with this complexity while we cannot group multiple functions in meaningfull classes. =20 >> depending on compiler behaviour here and the function may >> not get inlined even if you mark it. Build to build your=20 I am not depending on it, just benefiting from it when it happens. >> samples could get attributed to the function or the parent=20 >> function, leading to very confusing results. >>=20 =20 Not really confusing if you know what is happening. It has been very useful to me.=20 I agree that by default samples to inline functions should be assigned to themselves to avoid confusions to the less experienced user. All I am saying is to enable the user to change the behaviour if he wants to. I think giving more choices to the user is better than enforcing one particular behaviour.=20 =20 >> Could you use callgraphs for this instead? =20 Yes, calgraphs can help, but it does not reduce the complexity of a report with a large number of funtions that need to be interpreted. Moreover, sometimes you cannot use calgraph (e.g. if you do not have frame pointers). Regards Renato |
From: John L. <le...@mo...> - 2007-10-26 20:09:54
|
On Fri, Oct 26, 2007 at 08:07:01AM -0700, Dave Nomura wrote: > Do you want me to track down the gdb code for processing dwarf and see > how it impacts performance? Well I'd like to see what gdb is actually doing for sure but you don't have to implement it and try it regards john |
From: Dave N. <dc...@us...> - 2007-11-06 03:09:59
|
John Levon wrote: > On Mon, Nov 05, 2007 at 12:46:59PM -0800, Dave Nomura wrote: > > >>> AIUI the patch is makes is_correct_function() entirely pointless, so I >>> would like a cleaned-up version >>> >>> >> I'm not sure I know what you mean be "makes is_correct_function() >> entirely pointless" or how you want this changed. I added the "&& >> linenr == 0" to get: >> if (!is_correct_function(function, sym.name()) && linenr == 0) >> goto fail; >> > > The case line number is never zero in the case that > !is_correct_function() is testing for, as far as I know. > So, you think that whenever is_correct_function() returns FALSE, linenr !=0? Then, I guess we would be to simply want to remove the lines: > if (!is_correct_function(function, sym.name()) > goto fail; and maybe add a comment like: /* the case "if (!is_correct_function(function, sym.name()) goto fail" has been removed * because even when is_correct_function() returns FALSE, we want to use the non-zero * linenr that may refer to an inlined function source line. it may also refer to a * source line from the nearest line based on available symbolic information, and under * certain situations may be the wrong line (if there is an intervening function with no * symbolic information), but the inlined function case is much more common. */ -- Dave Nomura LTC Linux Power Toolchain |
From: John L. <le...@mo...> - 2007-11-06 14:05:24
|
On Mon, Nov 05, 2007 at 07:10:30PM -0800, Dave Nomura wrote: > So, you think that whenever is_correct_function() returns FALSE, linenr > !=0? Phil - this is right, yes? > Then, I guess we would be to simply want to remove the lines: Remove all related code in fact (and don't comment what used to happen, but what /currently/ happens: that is, document where we get things wrong) cheers john |
From: Dave N. <dc...@us...> - 2007-10-30 22:52:01
|
Did you want me to do anything to help you evaluate this? John Levon wrote: > On Fri, Oct 26, 2007 at 08:07:01AM -0700, Dave Nomura wrote: > > >> Do you want me to track down the gdb code for processing dwarf and see >> how it impacts performance? >> > > Well I'd like to see what gdb is actually doing for sure but you don't > have to implement it and try it > > regards > john > |
From: John L. <le...@mo...> - 2007-10-31 10:32:20
|
On Tue, Oct 30, 2007 at 03:51:09PM -0700, Dave Nomura wrote: > Did you want me to do anything to help you evaluate this? I wasn't clear, I won't be doing anything myself :) I'd like to know what exactly gdb does. regards john |
From: Dave N. <dc...@us...> - 2007-11-06 18:39:03
|
One side effect of removing the call to is_correct_function() is removing the warning message: cerr << "warning: some functions compiled without " << "debug information may have incorrect source " << "line attributions" << endl; in the case where is_correct_function() uses its more lax matching criteria. I wouldn't think you'd want to lose that warning. John Levon wrote: > On Mon, Nov 05, 2007 at 07:10:30PM -0800, Dave Nomura wrote: > > >> So, you think that whenever is_correct_function() returns FALSE, linenr >> !=0? >> > > Phil - this is right, yes? > > >> Then, I guess we would be to simply want to remove the lines: >> > > Remove all related code in fact (and don't comment what used to happen, > but what /currently/ happens: that is, document where we get things > wrong) > > cheers > john > |
From: Dave N. <dc...@us...> - 2007-10-31 22:28:50
|
I'm not sure anyone knows exactly what GDB does. :) There is a bunch of code in gdb/dwarfread.c but it is not clear that GDB uses any of this for dealing with inlines. I think my experiment of seeing whether or not GDB could find the correct line number for an inlined function was somewhat misleading. I think GDB found the correct line number(of the inlined source), because bfd_find_nearest_line found the correct line number, as is the case in opannotate. Can I convince you to handle the inlined code problem correctly in favor over the less common module-with-no-debug-info leading to a bogus line number case, or do you want to stick with the status quo? The other alternative would be for me to dig into the dwarfread.c code and try and figure out what is involved in reading the dwarf code to find the inline information. As for performance, I wouldn't think it would be any bigger issue for opannotate than it is for gdb. If the dwarf information could be accessed on the fly, then you would only need to process the dwarf information when this inline case arises in is_correct_function(). John Levon wrote: > I'd like to know what exactly gdb does. > > regards > john > > I wasn't clear, I won't be doing anything myself :) |
From: John L. <le...@mo...> - 2007-11-06 22:04:24
|
On Tue, Nov 06, 2007 at 10:37:35AM -0800, Dave Nomura wrote: > One side effect of removing the call to is_correct_function() is > removing the warning message: > cerr << "warning: some functions compiled without " > << "debug information may have incorrect source " > << "line attributions" << endl; > in the case where is_correct_function() uses its more lax matching criteria. > > I wouldn't think you'd want to lose that warning. But that warning is for exactly the case that we're regressing, so this now becomes a permanent warning we don't detect, right? regards john |
From: John L. <le...@mo...> - 2007-10-31 22:48:55
|
On Wed, Oct 31, 2007 at 03:27:53PM -0700, Dave Nomura wrote: > Can I convince you to handle the inlined code problem correctly in favor > over the less common module-with-no-debug-info leading to a bogus line > number case, or do you want to stick with the status quo? Let's go with the fix. regards john |
From: Philippe E. <ph...@wa...> - 2007-11-07 18:54:17
|
On Tue, 06 Nov 2007 at 22:04 +0000, John Levon wrote: > On Tue, Nov 06, 2007 at 10:37:35AM -0800, Dave Nomura wrote: > > > One side effect of removing the call to is_correct_function() is > > removing the warning message: > > cerr << "warning: some functions compiled without " > > << "debug information may have incorrect source " > > << "line attributions" << endl; > > in the case where is_correct_function() uses its more lax matching criteria. > > > > I wouldn't think you'd want to lose that warning. > > But that warning is for exactly the case that we're regressing, so this > now becomes a permanent warning we don't detect, right? John, why to regress ? I don't understand why you want to remove this warning, users can run into this problem of misattribution very easily, link statically with a library w/o debug info is sufficient. I remember than the problem came with the kernel too, some .S wasn't instrumented with proper debug info, on x86 it's probably safe nowadays but I doubt all platform where oprofile can be used has their entry.S instrumented with debug info. I think the && linenr == 0 make sense. linenr != 0 mean we have debug info for this eip so is_correct_function() should really be ignored and the return value from bfd_find_nearest_line() trusted. The best fix is: if (linenr == 0 && !is_correct_function(function, sym.name())) goto fail; rather the proposed: if (!is_correct_func...() && linenr == 0) to clarify the intent. If you look the other use of is_correct_function() in fixup_linenr() if (ret && cfilename && function && linenr != 0 && is_correct_function(function, name)) { /* it's not a failure */ so it's already done in this way, the proposed change look like a bug fix rather than a new feature. -- regards, Phe |
From: Philippe E. <ph...@wa...> - 2007-11-07 19:36:30
|
On Wed, 07 Nov 2007 at 19:49 +0000, Philippe Elie wrote: > On Tue, 06 Nov 2007 at 22:04 +0000, John Levon wrote: > > > On Tue, Nov 06, 2007 at 10:37:35AM -0800, Dave Nomura wrote: > > > > > One side effect of removing the call to is_correct_function() is > > > removing the warning message: > > > cerr << "warning: some functions compiled without " > > > << "debug information may have incorrect source " > > > << "line attributions" << endl; > > > in the case where is_correct_function() uses its more lax matching criteria. > > > > > > I wouldn't think you'd want to lose that warning. > > > > But that warning is for exactly the case that we're regressing, so this > > now becomes a permanent warning we don't detect, right? > > John, why to regress ? I don't understand why you want to remove this > warning, users can run into this problem of misattribution very easily, > link statically with a library w/o debug info is sufficient. I remember > than the problem came with the kernel too, some .S wasn't instrumented > with proper debug info, on x86 it's probably safe nowadays but I doubt > all platform where oprofile can be used has their entry.S instrumented > with debug info. > > I think the && linenr == 0 make sense. linenr != 0 mean we have debug > info for this eip so is_correct_function() should really be ignored and > the return value from bfd_find_nearest_line() trusted. The best fix is: > > if (linenr == 0 && !is_correct_function(function, sym.name())) > goto fail; > > rather the proposed: if (!is_correct_func...() && linenr == 0) to clarify > the intent. > > If you look the other use of is_correct_function() in fixup_linenr() > > if (ret && cfilename && function && linenr != 0 > && is_correct_function(function, name)) { > /* it's not a failure */ > > so it's already done in this way, the proposed change look like a bug > fix rather than a new feature. After discussing on IRC we agreed to go for the: if (linenr == 0 && !is_correct_function(function, sym.name())) the intent is to fix the inline case w/o breaking the no-debug info case. -- regards, Phe |
From: Maynard J. <may...@us...> - 2007-11-05 16:59:15
|
John Levon wrote: > On Wed, Oct 31, 2007 at 03:27:53PM -0700, Dave Nomura wrote: > > >> Can I convince you to handle the inlined code problem correctly in favor >> over the less common module-with-no-debug-info leading to a bogus line >> number case, or do you want to stick with the status quo? >> > > Let's go with the fix. > Sorry, I wasn't following this thread very closely last week. Would you like me to commit the patch? -Maynard > regards > john > > ------------------------------------------------------------------------- > This SF.net email is sponsored by: Splunk Inc. > Still grepping through log files to find problems? Stop. > Now Search log events and configuration files using AJAX and a browser. > Download your FREE copy of Splunk now >> http://get.splunk.com/ > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > |
From: John L. <le...@mo...> - 2007-11-05 19:29:36
|
On Mon, Nov 05, 2007 at 10:59:19AM -0600, Maynard Johnson wrote: > >> Can I convince you to handle the inlined code problem correctly in favor > >> over the less common module-with-no-debug-info leading to a bogus line > >> number case, or do you want to stick with the status quo? > >> > > > > Let's go with the fix. > > > Sorry, I wasn't following this thread very closely last week. Would you > like me to commit the patch? AIUI the patch is makes is_correct_function() entirely pointless, so I would like a cleaned-up version Also it would be good to see some mention in the XML documentation about the problem case that we are regressing. regards john |
From: Dave N. <dc...@us...> - 2007-10-24 14:07:03
|
How about a "--inline / -I" option to opannotate that triggers find_nearest_line() to use the non-zero line number found by bfd_find_nearest_line()? Santos, Jose Renato G wrote: > > >>> So it appears we have two cases: >>> 1) due to lack of symbolic debug information in some >>> modules, bfd_find_nearest_line finds some unrelated line >>> number and the samples get attributed to that line number, >>> 2) if there is a static inline function called then the >>> samples get annotated to the caller rather than the inline >>> function source. >>> In older versions of gcc(3.3.3), the function name returned >>> by bfd_get_nearest_line for an inlined function was the same >>> as the caller, so symbol_name == function_name and >>> is_correct_function() returned true, and we got the correct >>> line number. In newer versions of gcc(4.1.2) the inlined >>> function_name is correctly identified. >>> >>> 1) is clearly misleading but perhaps not that common, and 2) >>> is probably more common, but at least the attribution can be >>> rationalized. >>> If we can't handle both cases without the DWARF processing >>> overhead, do you want to stick with handling 1), or switch >>> to handling the more common case 2)? >>> >>> > > Dave and John > > If case2 is addressed, it would be nice to have a command option to > enable the user to choose how inline function samples should be > accounted (either attributed to themselves or to the parent function). I > found this "bug" is sometimes a "feature" and was very handy in some of > my experiments. Sometimes is good to have a smaller number of functions > aggregating a larger number of samples. I would be disappointed if this > "feature" was removed. :) Not sure if you were already planning this... > Thanks > > Renato > > -- Dave Nomura LTC Linux Power Toolchain |
From: Dave N. <dc...@us...> - 2007-10-26 15:06:58
|
Did you have an opinion on my last idea of making an option to give a preference for the inline case? Based on your last response it sounded like you were leaning toward choosing the inline case over the no-debug-info case. Do you want me to track down the gdb code for processing dwarf and see how it impacts performance? Maybe it could be done on the fly as needed (rather than on every single object file) only in the cases where is_correct_file returns FALSE. If the performance is acceptable for GDB, I would think it would be OK for opannotate as well. Dave Nomura wrote: > How about a "--inline / -I" option to opannotate that triggers > find_nearest_line() to use the non-zero line number found by > bfd_find_nearest_line()? > > Santos, Jose Renato G wrote: > >> >> >> >>>> So it appears we have two cases: >>>> 1) due to lack of symbolic debug information in some >>>> modules, bfd_find_nearest_line finds some unrelated line >>>> number and the samples get attributed to that line number, >>>> 2) if there is a static inline function called then the >>>> samples get annotated to the caller rather than the inline >>>> function source. >>>> In older versions of gcc(3.3.3), the function name returned >>>> by bfd_get_nearest_line for an inlined function was the same >>>> as the caller, so symbol_name == function_name and >>>> is_correct_function() returned true, and we got the correct >>>> line number. In newer versions of gcc(4.1.2) the inlined >>>> function_name is correctly identified. >>>> >>>> 1) is clearly misleading but perhaps not that common, and 2) >>>> is probably more common, but at least the attribution can be >>>> rationalized. >>>> If we can't handle both cases without the DWARF processing >>>> overhead, do you want to stick with handling 1), or switch >>>> to handling the more common case 2)? >>>> >>>> >>>> >> Dave and John >> >> If case2 is addressed, it would be nice to have a command option to >> enable the user to choose how inline function samples should be >> accounted (either attributed to themselves or to the parent function). I >> found this "bug" is sometimes a "feature" and was very handy in some of >> my experiments. Sometimes is good to have a smaller number of functions >> aggregating a larger number of samples. I would be disappointed if this >> "feature" was removed. :) Not sure if you were already planning this... >> Thanks >> >> Renato >> >> >> > > > -- Dave Nomura LTC Linux Power Toolchain |
From: Dave N. <dc...@us...> - 2007-11-05 20:46:29
|
John Levon wrote: > AIUI the patch is makes is_correct_function() entirely pointless, so I > would like a cleaned-up version > I'm not sure I know what you mean be "makes is_correct_function() entirely pointless" or how you want this changed. I added the "&& linenr == 0" to get: if (!is_correct_function(function, sym.name()) && linenr == 0) goto fail; is_correct_function returns true if function == sym.name() or one of the objective C matching criteria is met. So, if is_correct_function() returns TRUE, then the above modified expression will behave exactly as it did without the patch and therefore not "entirely pointless". In the case is_correct_function() returns FALSE, then the code will behave exactly is it did previously except when linenr != 0, which is exactly the scenario where I want the inlined function's linenr to be used. > Also it would be good to see some mention in the XML documentation about > the problem case that we are regressing. > > regards > john > -- Dave Nomura LTC Linux Power Toolchain |