[Bug 1567219]
Ivo Raisr
ivosh at ivosh.net
Sat Apr 9 21:23:59 UTC 2016
Thank you for the patches and for addressing my comments.
Your work is really appreciated.
Overall it looks in a good shape now.
Please fold all the patches into one, so it's better maintainable.
I have only the following remaining comments:
+++ configure.ac
1) Please use autoconf macros AC_CHECK_TYPE/AC_CHECK_TYPES for checking Elf{32,64}_Chdr typedefs.
See for example:
https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Generic-Types.html#Generic-Types
+++ coregrind/m_debuginfo/image.c
2) When including "tinfl.c", do we want to define "TINFL_HEADER_FILE_ONLY"?
3) typo:
+ // Virtual size of the image = real size + size of uncopressed data
uncopressed => uncompressed
4) typo:
+ // Number of compressed slices are used
delete 'are'
5) CEnt.data has now non-fixed size. Why CACHE_ENTRY_SIZE is still used in various places
around image.c; for example in alloc_CEnt() and realloc_CEnt()?
6) In function find_cslc(), please use {} braces for better readability and put 'return' statement
to its own line. You can declare 'i' inside the for loop as we use C99.
7) In function find_cslc(), use proper type 'UInt' instead of 'int', to
match that of 'cslc_used'.
8) Width of all lines is 80 characters max - occurrences in
alloc_CEnt(), realloc_CEnt(), get_slowcase(),
ML_(img_mark_compressed_part)().
9) Should be two lines, really:
+ if (len > ce->size) len = ce->size;
10) typo:
+ // get copressed data
11) should be on separate lines:
+ if ((cslc != NULL) && (cslc->szD > CACHE_ENTRY_SIZE)) size = cslc->szD;
12) Please be explicit:
+ vg_assert(img);
=> vg_assert(img != NULL);
+++ coregrind/m_debuginfo/priv_image.h
13) Make it a proper sentence:
+/* Real size of the image */
=> +/* Real size of the image. */
14) Make it a proper sentence:
+ Returns (virtual) position in image from which decompressed data can be read */
=> Returns (virtual) position in image from which decompressed data can be read. */
15) Lines too long (exceed 80 chars):
Returns (virtual) position in image from which decompressed data can be read */
DiOffT ML_(img_mark_compressed_part)(DiImage* img, DiOffT offset, SizeT szC, SizeT szD);
+++ coregrind/m_debuginfo/readelf.c
16) Magic '12' is used multiple times, please make it a #define or a constant.
+ } else if (h->sh_size > 12) {
17) [multiple times] The exclamation mark is really unnecessary here,
the whole message you get is scary per se (check other occurrences of ML_(symerr)() in this module).
+ ML_(symerr)(di, True, " Unknown compression type!"); \
+++ memcheck/tests/Makefile.am
18) Use @FLAG_W_NO_UNINITIALIZED@ (see configure.ac) instead of plain -Wno-uninitialized.
+++ memcheck/tests/cdebug.c
19) A compiler could see that the program always returns 0 and might optimize 'if (x)' out.
Perhaps you can return different values?
20) I don't see any coregrind/Makefile changes to build
m_debuginfo/tinfl.c?
+++ coregrind/m_debuginfo/tinfl.c
21) What kind of license your modifications fall under? tinfl.c seems to be under "UNLICENSE" whereas
the rest of Valgrind is under GPLv2+.
22) We should use proper Valgrind types, not standard C ones here:
typedef unsigned char mz_uint8;
typedef signed short mz_int16;
typedef unsigned short mz_uint16;
typedef unsigned int mz_uint32;
typedef unsigned int mz_uint;
typedef unsigned long long mz_uint64;
I was able to get it built on amd64/Solaris and regression testing went fine.
However I don't have any system with toolchain supporting '-gz' at hand.
I assume you tested on MIPS. Anyone can test on a different architecture or distribution?
--
You received this bug notification because you are a member of Ubuntu
Foundations Bugs, which is subscribed to valgrind in Ubuntu.
https://bugs.launchpad.net/bugs/1567219
Title:
Valgrind does not support compressed debug info
Status in Ubuntu on IBM z Systems:
New
Status in Valgrind:
Unknown
Status in valgrind package in Ubuntu:
Confirmed
Bug description:
== Comment: #0 - Andreas Arnez <arnez at de.ibm.com> - 2016-04-01 12:23:20 ==
It seems that some Ubuntu debug packages are built with compressed debug info, which is not supported by Valgrind. In particular I've seen that this even applies to the dynamic loader (when libc6-dbg is installed):
$ ./vg-in-place /bin/true
==5907== Memcheck, a memory error detector
==5907== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5907== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==5907== Command: /bin/true
==5907==
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/ld-2.23.so:
--5907-- Ignoring non-Dwarf2/3/4 block in .debug_info
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/ld-2.23.so:
--5907-- Ignoring non-Dwarf2/3/4 block in .debug_info
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/ld-2.23.so:
--5907-- Ignoring non-Dwarf2/3/4 block in .debug_info
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/ld-2.23.so:
--5907-- Last block truncated in .debug_info; ignoring
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/ld-2.23.so:
--5907-- parse_CU_Header: is neither DWARF2 nor DWARF3 nor DWARF4
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/libc-2.23.so:
--5907-- Ignoring non-Dwarf2/3/4 block in .debug_info
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/libc-2.23.so:
--5907-- Ignoring non-Dwarf2/3/4 block in .debug_info
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/libc-2.23.so:
--5907-- Ignoring non-Dwarf2/3/4 block in .debug_info
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/libc-2.23.so:
--5907-- Last block truncated in .debug_info; ignoring
--5907-- WARNING: Serious error when reading debug info
--5907-- When reading debug info from /lib/s390x-linux-gnu/libc-2.23.so:
--5907-- parse_CU_Header: is neither DWARF2 nor DWARF3 nor DWARF4
==5907==
==5907== HEAP SUMMARY:
==5907== in use at exit: 0 bytes in 0 blocks
==5907== total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==5907==
==5907== All heap blocks were freed -- no leaks are possible
==5907==
==5907== For counts of detected and suppressed errors, rerun with: -v
==5907== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
== Comment: #1 - Andreas Arnez <arnez at de.ibm.com> - 2016-04-01 12:26:28 ==
Note that this issue should apply to all platforms. It has already been reported for upstream Valgrind, and a patch has been proposed:
https://bugs.kde.org/show_bug.cgi?id=303877
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu-z-systems/+bug/1567219/+subscriptions
More information about the foundations-bugs
mailing list