Bug 296318

Created attachment 69724 [details]
Proposed patch (against SVN r12450)

I tested the patch on f12/x86, deb6/amd64 and f16/ppc64.
No regression, 6 more successful tests on f16/ppc64.

Due to lack of knowledge in debug info readers, cannot review it properly (and so no commit by me).

Just one remark: in m_debuginfo, all VG_(newXA) are using ML_(dinfo_zalloc/free) .
In the patch, a call to newXA is introduced using VG_(mallloc/free).

Hi Philippe, thanks for trying it out. Seeing it has helped with some tests on ppc64 was an unexpected nice surprise.

Good catch with the (de)alloc callbacks - I had actually found out about this m_debuginfo specialty and changed them elsewhere, but missed this newXA() call. I'll update the patch, also to keep it in line with the SVN.

Created attachment 71157 [details]
Proposed patch v2 (against SVN r12566)

> Created attachment 71157 [details]
> Proposed patch v2 (against SVN r12566)

Some initial comments:

* This contains a whole bunch of changes in
  coregrind/m_debuginfo/readpdb.c.  Why is that?  Are they related?
  I thought this patch was only for ELF object reading.

* i/j mixup
  +               for(j = 0; i < VG_(sizeXA)(di->fsm.maps); i++) {

* Am concerned that it will break OSX, which was hard to make work
  properly.  Eg, where did this get moved to?

  -   if (is_ro_map) {
  -      /* We have a r-- mapping.  Note the details (OSX 10.7, 32-bit only) */
  -      if (!di->fsm.have_ro_map) {
  -         di->fsm.have_ro_map = True;
  -         di->fsm.ro_map_avma = a;
  -         di->fsm.ro_map_size = seg->end + 1 - seg->start;
  -         di->fsm.ro_map_foff = seg->offset;
  -      } else {
  -         /* FIXME: complain about a second r-- mapping */
  -      }
  -   }

Overall, I feel like the fixes this patch does are good, but I am
concerned that it is risky -- going to cause breakage.

Created attachment 72476 [details]
Proposed patch v3 (against SVN r12734)

> * This contains a whole bunch of changes in
>   coregrind/m_debuginfo/readpdb.c.  Why is that?  Are they related?
>   I thought this patch was only for ELF object reading.
Well it started with ELF files with more mappings, but in order to
allow them in ELF, the common structures needed to change too and
that triggered modifications in other readers.
  Specifically in the Win/PDB stuff, it went like this: To be able to
even compile the readpdb.c with my changes, I needed to change the
part which goes through PE section headers and sets rx/rw_map_avma
etc., because dynamic arrays are used now instead. I noticed that
both rx_map_avma and text_bias are used at different places through
the rest of the code using the horrible BIAS_FOR_WHATEVER macros.
  So there, I could either patch it blindly or build Wine and see
how to adapt the PDB reader properly. After some debugging and
making it all work, I saw the biasing mess was not necessary, so
I bit the bullet and went to remove it alogether, name stuff like
unknown_purpose__reloc correctly and document it all properly.
  These changes were influenced directly by changing the way debug
info mappings are stored, even if PE-specific, so I included them in
the patch. However I see your point there are now some changes not
related to *just* the possibility of multiple mappings -- so I can
split the patch if you'd like.

> * i/j mixup
>   +               for(j = 0; i < VG_(sizeXA)(di->fsm.maps); i++) {
Bah, thanks! Hate that kind of errors... (Shows I had no debug
symbols in separate files on the systems I tested it.) Verified there
is no other for-typo like that.

> * Am concerned that it will break OSX, which was hard to make work
>   properly.  Eg, where did this get moved to?
>   -   if (is_ro_map) {
>   -      /* We have a r-- mapping.  Note the details (OSX 10.7, 32-bit only) */
>   -      if (!di->fsm.have_ro_map) {
>   -         di->fsm.have_ro_map = True;
>   -         di->fsm.ro_map_avma = a;
>   -         di->fsm.ro_map_size = seg->end + 1 - seg->start;
>   -         di->fsm.ro_map_foff = seg->offset;
>   -      } else {
>   -         /* FIXME: complain about a second r-- mapping */
>   -      }
>   -   }
This extra block is no longer necessary, because all mappings are
stored in the di->fsm.maps array in a generic matter.
+   map.ro   = is_ro_map;
+   VG_(addToXA)(di->fsm.maps, &map);
+   ...
+   di->fsm.have_ro_map |= is_ro_map;
Similarly, the counterpart in VG_(di_notify_vm_protect) which upgrades
the mapping finds the relevant RO mapping to change to RW in the array
and updates it.

> Overall, I feel like the fixes this patch does are good, but I am
> concerned that it is risky -- going to cause breakage.
I know, there is quite a lot of changes in code which, which is kind
of essential to make V useful. It's definitely useful though, so I hope
it will be possible to make it ready for commit.

Created attachment 72482 [details]
Proposed patch v4 (against SVN r12734) - only the necessary PDB reader changes

Created attachment 72483 [details]
PDB reader cleanup (against SVN r12734 + the main patch)

(In reply to comment #8)
> Created attachment 72482 [details]

Committed, r12735.

Tested with wine 1.5.0, needed recompiling with Valgrind headers available in order to compile in the necessary client callback.

Test program, compiled with Microsoft Visual C++ 2010, CFLAGS=/Od /Zi (no optimizations, debug info), LFLAGS=/debug (debug info).
---
#include <stdio.h>
#include <stdlib.h>

void* thisleaks()
{
  return malloc(1234);
}

int main()
{
  void* buffer = thisleaks();
  printf("Yay!\n");
  return 0;
}
---
Generates test.exe and test.pdb.

Results:
Program runs OK,
--trace-symtab=yes shows symbols being loaded correctly,
output contains relevant stack trace:
==9842== 1,234 bytes in 1 blocks are definitely lost in loss record 74 of 75
==9842==    at 0x477121C: RtlAllocateHeap (heap.c:254)
==9842==    by 0x4D143F6: MSVCRT_malloc (heap.c:312)
==9842==    by 0x40151D: thisleaks (in /home/jirka/dev/test/test.exe)
==9842==    by 0x401538: main (in /home/jirka/dev/test/test.exe)
I did not see this stack trace in the report before patching.

(In reply to comment #9)
> Created attachment 72483 [details]

Committed, r12736.  Thanks for the patches.

*** Bug 300195 has been marked as a duplicate of this bug. ***