[PATCH] Fix regression in debuginfo processing

[Maynard Johnson]

This patch fixes a regression in debuginfo processing resulting from filtering out symbols from non-SEC_LOAD sections (see June 23 2009 change).

This patch is necessary since we're now filtering out symbols whose section flag does not include SEC_LOAD.  Unfortunately, this is true for *all* symbols obtained from a debuginfo file.  The fix involves mapping the symbol sections from the debuginfo bfd to those of the real image.

This is a pretty serious problem, and we may end up putting out a new release sooner than normal to fix this.

*Eran*, please remove any temporary workarounds you added to your oprofile source and apply this patch; then let me know how it works for you.


Thanks.
-Maynard


diff -paur oprofile/ChangeLog op-debuginfo/ChangeLog
--- oprofile/ChangeLog 2009-09-14 15:18:06.000000000 -0500
+++ op-debuginfo/ChangeLog 2009-10-07 15:42:16.000000000 -0500
@@ -1,3 +1,10 @@
[phone]  Maynard Johnson  <maynardj@...>
+
+ * libutil++/bfd_support.h:
+ * libutil++/bfd_support.cpp: Fix regression in debuginfo processing
+  resulting from filtering out symbols from non-SEC_LOAD sections
+  (see June 23, 2009 change below)
+
 2009-09-14  Suravee Suthikulpanit <suravee.suthikulpanit@...>
 
  * utils/opcontrol: Fix timer mode
diff -paur oprofile/libutil++/bfd_support.cpp op-debuginfo/libutil++/bfd_support.cpp
--- oprofile/libutil++/bfd_support.cpp 2009-07-20 08:24:46.000000000 -0500
+++ op-debuginfo/libutil++/bfd_support.cpp 2009-10-08 09:05:20.000000000 -0500
@@ -432,21 +432,16 @@ void bfd_info::close()
  bfd_close(abfd);
 }
 
-
-#if SYNTHESIZE_SYMBOLS
-/* On ppc64 platforms where there is no symbol information in the image bfd,
- * the debuginfo syms need to be mapped back to the sections of the image bfd
- * in order to gather enough information about the symbol.  That's the purpose
- * of the translate_debuginfo_syms() function.
- */
 void bfd_info::translate_debuginfo_syms(asymbol ** dbg_syms, long nr_dbg_syms)
 {
-#define MAX_SECT 1024
- bfd_section * image_sect[MAX_SECT];
- int img_sect_cnt = 0;
+ bfd_section ** image_sect;
+ unsigned int img_sect_cnt = 0;
  bfd * image_bfd = image_bfd_info->abfd;
 
- for (bfd_section * sect = image_bfd->sections; sect && img_sect_cnt < MAX_SECT;
+ image_sect = (bfd_section **) malloc(image_bfd->section_count * (sizeof(bfd_section *)));
+
+ for (bfd_section * sect = image_bfd->sections;
+     sect && img_sect_cnt < image_bfd->section_count;
      sect = sect->next) {
  // A comment section marks the end of the needed sections
  if (strstr(sect->name, ".comment") == sect->name)
@@ -464,8 +459,10 @@ void bfd_info::translate_debuginfo_syms(
  }
  }
  }
+ free(image_sect);
 }
 
+#if SYNTHESIZE_SYMBOLS
 bool bfd_info::get_synth_symbols()
 {
  extern const bfd_target bfd_elf64_powerpc_vec;
@@ -533,13 +530,6 @@ bool bfd_info::get_synth_symbols()
 {
  return false;
 }
-
-void bfd_info::translate_debuginfo_syms(asymbol ** dbg_syms __attribute__ ((unused)),
- long nr_dbg_syms __attribute__ ((unused)))
-{
- return;
-}
-
 #endif /* SYNTHESIZE_SYMBOLS */
 
 
@@ -569,6 +559,9 @@ void bfd_info::get_symbols()
 
  nr_syms = bfd_canonicalize_symtab(abfd, syms.get());
 
+ if (image_bfd_info)
+ translate_debuginfo_syms(syms.get(), nr_syms);
+
  cverb << vbfd << "bfd_canonicalize_symtab: " << dec
       << nr_syms << hex << endl;
 }
diff -paur oprofile/libutil++/bfd_support.h op-debuginfo/libutil++/bfd_support.h
--- oprofile/libutil++/bfd_support.h 2009-07-20 08:24:46.000000000 -0500
+++ op-debuginfo/libutil++/bfd_support.h 2009-10-08 09:11:46.000000000 -0500
@@ -72,13 +72,17 @@ private:
  */
  bfd_info * image_bfd_info;
 
- /**
- * This function is intended to use the cached image bfd as needed
- * when processing debuginfo files.  The implementation is not
- * necessarily architecture-independent; e.g., see the implementation
- * for the elf64_powerpc[le] bfd targets in bfd_support.cpp.
+ /* To address a different issue, we discard symbols whose section
+ * flag does not contain SEC_LOAD.  But since this is true for symbols
+ * found in debuginfo files, we must run those debuginfo symbols
+ * through the function below to prevent them from being inadvertently
+ * discarded.  This function maps the sections from the symbols in
+ * the debuginfo bfd to those of the real image bfd.  Then, when
+ * we later do symbol filtering, we see the sections from the real
+ * bfd, which do contain SEC_LOAD in the section flag.
  */
  void translate_debuginfo_syms(asymbol ** dbg_syms, long nr_dbg_syms);
+
 };
 
 

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[Felipe Contreras]

On Thu, Oct 8, 2009 at 5:14 PM, Maynard Johnson <maynardj@...> wrote:
> This patch fixes a regression in debuginfo processing resulting from filtering out symbols from non-SEC_LOAD sections (see June 23 2009 change).
>
> This patch is necessary since we're now filtering out symbols whose section flag does not include SEC_LOAD.  Unfortunately, this is true for *all* symbols obtained from a debuginfo file.  The fix involves mapping the symbol sections from the debuginfo bfd to those of the real image.
>
> This is a pretty serious problem, and we may end up putting out a new release sooner than normal to fix this.

Indeed, I just discovered that OProfile is not finding debug symbols
in Fedora 11.

This patch fixes the problem for me... however, I get a build error:
-if (sym->section->index < img_sect_cnt) {
+if ((unsigned int)sym->section->index < img_sect_cnt) {

Please apply this patch ASAP.

--
Felipe Contreras

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[Maynard Johnson]

Maynard Johnson wrote:
> This patch fixes a regression in debuginfo processing resulting from filtering out symbols from non-SEC_LOAD sections (see June 23 2009 change).

Will,
Can you reveiw the debuginfo patch I posted on Oct 8?  As you may have seen,
Felipe Contreras just posted a message to the oprofile-list yesterday about the
need for this patch on Fedora 11.  It's an important fix, and I'd like to get it
reviewed and checked in as soon as possible.  John L suggested you might be a
good reviewer as you've had some involvement in that part of the code in the past.

Thanks.
-Maynard

>
> This patch is necessary since we're now filtering out symbols whose section flag does not include SEC_LOAD.  Unfortunately, this is true for *all* symbols obtained from a debuginfo file.  The fix involves mapping the symbol sections from the debuginfo bfd to those of the real image.
>
> This is a pretty serious problem, and we may end up putting out a new release sooner than normal to fix this.
>
> *Eran*, please remove any temporary workarounds you added to your oprofile source and apply this patch; then let me know how it works for you.
>
>
> Thanks.
> -Maynard
>
>
>
> ------------------------------------------------------------------------
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry(R) Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9 - 12, 2009. Register now!
> http://p.sf.net/sfu/devconference
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> oprofile-list mailing list
> oprofile-list@...
> https://lists.sourceforge.net/lists/listinfo/oprofile-list

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[Maynard Johnson]

Felipe Contreras wrote:

> On Thu, Oct 8, 2009 at 5:14 PM, Maynard Johnson <maynardj@...> wrote:
>> This patch fixes a regression in debuginfo processing resulting from filtering out symbols from non-SEC_LOAD sections (see June 23 2009 change).
>>
>> This patch is necessary since we're now filtering out symbols whose section flag does not include SEC_LOAD.  Unfortunately, this is true for *all* symbols obtained from a debuginfo file.  The fix involves mapping the symbol sections from the debuginfo bfd to those of the real image.
>>
>> This is a pretty serious problem, and we may end up putting out a new release sooner than normal to fix this.
>
> Indeed, I just discovered that OProfile is not finding debug symbols
> in Fedora 11.
>
> This patch fixes the problem for me... however, I get a build error:
> -if (sym->section->index < img_sect_cnt) {
> +if ((unsigned int)sym->section->index < img_sect_cnt) {

Thanks for catching that.  I'll make sure to fix it, assuming the patch review
is positive.

-Maynard
>
> Please apply this patch ASAP.
>


------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[Eran Rom]

> *Eran*, please remove any temporary workarounds you added to your oprofile
source and apply this patch;
> then let me know how it works for you.
Works like charm.
I assume that the patch goes beyond just ignoring the SEC_LOAD check:
       if (!(sym->section->flags & SEC_LOAD))
-                return false;
+                return true;

Do you think I can rely on profiling data I got with the above?

Thanks very much,
Eran


------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[Maynard Johnson]

Eran Rom wrote:

>> *Eran*, please remove any temporary workarounds you added to your oprofile
> source and apply this patch;
>> then let me know how it works for you.
> Works like charm.
> I assume that the patch goes beyond just ignoring the SEC_LOAD check:
>        if (!(sym->section->flags & SEC_LOAD))
> -                return false;
> +                return true;
>
> Do you think I can rely on profiling data I got with the above?

Yes.  The !SEC_LOAD filter was added for symbols whose computed file position
was greater than file size.  These are typically data addressing symbols (i.e.,
not function symbols).  Without this filter, opreport was choking on such
symbols with the error:
"samples_range(): start > end something wrong with kernel or module layout ?"
So if you don't see that error, then the binaries you're profiling don't have
any such symbols, and your profiling data is perfectly valid.

-Maynard

>
> Thanks very much,
> Eran
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry(R) Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9 - 12, 2009. Register now!
> http://p.sf.net/sfu/devconference
> _______________________________________________
> oprofile-list mailing list
> oprofile-list@...
> https://lists.sourceforge.net/lists/listinfo/oprofile-list

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[William Cohen]

Maynard Johnson wrote:

> Maynard Johnson wrote:
>> This patch fixes a regression in debuginfo processing resulting from filtering out symbols from non-SEC_LOAD sections (see June 23 2009 change).
>
> Will,
> Can you reveiw the debuginfo patch I posted on Oct 8?  As you may have seen,
> Felipe Contreras just posted a message to the oprofile-list yesterday about the
> need for this patch on Fedora 11.  It's an important fix, and I'd like to get it
> reviewed and checked in as soon as possible.  John L suggested you might be a
> good reviewer as you've had some involvement in that part of the code in the past.
>
> Thanks.
> -Maynard
>> This patch is necessary since we're now filtering out symbols whose section flag does not include SEC_LOAD.  Unfortunately, this is true for *all* symbols obtained from a debuginfo file.  The fix involves mapping the symbol sections from the debuginfo bfd to those of the real image.
>>
>> This is a pretty serious problem, and we may end up putting out a new release sooner than normal to fix this.
>>
>> *Eran*, please remove any temporary workarounds you added to your oprofile source and apply this patch; then let me know how it works for you.
>>
>>
>> Thanks.
>> -Maynard

Hi Maynard,

I have been looking at this debuginfo patch fix today. It has been a long time
since I looked at the bfd_support code and less familar with the ppc aspects. I
tried the patch out on a couple rhel5 (ppc 32-bit user/64-bit kernel and i386)
systems with locally built 0.9.5 with debuginfo and without the patch and it
didn't see any differences. Similar results with f-11 i686 and f-12 x86_64.

I have been looking through the email archives to get a better idea when the
problem is encountered. This occurs with newer distributions on ppc?

The removal of hard limit MAX_SECT is translate_debuginfo_syms() is a good idea.

Overall the patch looks sane.

I put a scatch build through the koji build system to check to make sure that it
compiles on various machines. And everything built without error for F-11:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1765440

-Will

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list

[Maynard Johnson]

William Cohen wrote:

> Maynard Johnson wrote:
>> Maynard Johnson wrote:
>>> This patch fixes a regression in debuginfo processing resulting from filtering out symbols from non-SEC_LOAD sections (see June 23 2009 change).
>> Will,
>> Can you reveiw the debuginfo patch I posted on Oct 8?  As you may have seen,
>> Felipe Contreras just posted a message to the oprofile-list yesterday about the
>> need for this patch on Fedora 11.  It's an important fix, and I'd like to get it
>> reviewed and checked in as soon as possible.  John L suggested you might be a
>> good reviewer as you've had some involvement in that part of the code in the past.
>>
>> Thanks.
>> -Maynard
>>> This patch is necessary since we're now filtering out symbols whose section flag does not include SEC_LOAD.  Unfortunately, this is true for *all* symbols obtained from a debuginfo file.  The fix involves mapping the symbol sections from the debuginfo bfd to those of the real image.
>>>
>>> This is a pretty serious problem, and we may end up putting out a new release sooner than normal to fix this.
>>>
>>> *Eran*, please remove any temporary workarounds you added to your oprofile source and apply this patch; then let me know how it works for you.
>>>
>>>
>>> Thanks.
>>> -Maynard
>
> Hi Maynard,
>
> I have been looking at this debuginfo patch fix today. It has been a long time
> since I looked at the bfd_support code and less familar with the ppc aspects. I
> tried the patch out on a couple rhel5 (ppc 32-bit user/64-bit kernel and i386)
> systems with locally built 0.9.5 with debuginfo and without the patch and it
> didn't see any differences. Similar results with f-11 i686 and f-12 x86_64.

Will, thanks much for looking at this.  The regression is in 0.9.5, but would only show up if:
1) you were looking for symbols in a super-stripped binary where a separate debuginfo file is available;
or
2) you generate a --debuginfo report and expect to see source file/line # info for samples in a binary that has debuginfo stripped for which you have a separate debuginfo file avaialble.

The patch has already been shown to resolve the above two issues, so I'm not concerned that you could not reproduce the problem.  The main thing I was wanting from you was a sanity check of the logic in the patch.  Thanks for doing that.  I'll commit the patch.

-Maynard

>
> I have been looking through the email archives to get a better idea when the
> problem is encountered. This occurs with newer distributions on ppc?
>
> The removal of hard limit MAX_SECT is translate_debuginfo_syms() is a good idea.
>
> Overall the patch looks sane.
>
> I put a scatch build through the koji build system to check to make sure that it
> compiles on various machines. And everything built without error for F-11:
>
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1765440
>
> -Will

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
oprofile-list mailing list
oprofile-list@...
https://lists.sourceforge.net/lists/listinfo/oprofile-list