|
From: Alex C. <ac...@ca...> - 2012-11-27 00:57:45
|
Hello,
(apologies if this is the wrong list, valgrind-developers seemed
like it was just svn commits?)
This is a patch that teaches valgrind how to search a
user-specified path for debug symbols.
The use cases are:
- user may not have superuser permissions to install debug
packages on their system.
- even if user does have sudo, user might not want to install
10s (or 100s) of debug packages on their system
- same as above but replace user with 'nightly automation script'
A bit more context. In Ubuntu, we're making a large push to
engage our community in helping us reduce overall memory
consumption. Obviously valgrind is a great tool to help do this,
and we're trying to make it easier for moderately technical users
to help contribute data.
At the same time, we don't want to be innundated with lame bug
reports with valgrind logs full of missing symbols.
So in a parallel effort, we're extending one of our bug reporting
tools (apport) to act as a nice user-friendly wrapper for
valgrind. We have a rough proof-of-concept working now, and when
it's all done, it will look something like this:
- user wants to profile an application
- user runs 1 script: apport-valgrind <name of binary>
- a log is created and automatically uploaded to our bug tracker
apport-valgrind does several things, but the hard bit is:
- map a binary name back to a corresponding Ubuntu package
- recursively find, download, and unpack debug packages for all
dependencies, including transitive dependencies
Debug packages are *unpacked* into a temporary sandbox (not to be
confused with installing into the system). Unpacking is fast and
can occur wherever the user has write permissions (think /tmp).
After unpacking, you end up with a filesystem hierarchy that
vaguely resembles a chroot, but it's not a full chroot, just
unpacked libraries.
This is where my valgrind patch comes into play. Valgrind looks
in some hard-coded locations for debug files, and in my
experimentation with LD_PRELOAD and LD_LIBRARY_PATH, changing
those variables to point at the sandbox didn't seem to help
valgrind find the symbols.
So I add a new command line parameter, --include-debugsyms-path,
and it tells valgrind to search a user-supplied path *in
addition* to the existing well-known search paths.
Now valgrind can be told about the sandbox; it will pick up the
debug symbols and we can generate better logs. I don't change any
of the CRC checking logic, so any sort of validity checks should
still hold.
It's not the most beautiful patch, but I tried to balance
straightforward against too much repetition of code. Certainly
the gotos could be removed, but would require a bit more
copy/paste of code (I think).
I think the patch is pretty good, and I have tested it quite a
bit, ensuring that both the new use case works while of course
not regressing existing behavior.
[nb, I did see the find_ad_hoc_debug_image() function, and my
naive understanding is that my patch might be a better approach
precisely because I keep the existing CRC checks]
Would love to see this patch get accepted, or tell me how to
rework it so it would be acceptable.
Thanks!
/ac
---
m_debuginfo/readelf.c | 41 ++++++++++++++++++++++++++++++++++++++---
m_main.c | 6 ++++++
m_options.c | 1 +
pub_core_options.h | 3 +++
4 files changed, 48 insertions(+), 3 deletions(-)
---
Index: coregrind/m_debuginfo/readelf.c
===================================================================
--- coregrind/m_debuginfo/readelf.c (revision 13143)
+++ coregrind/m_debuginfo/readelf.c (working copy)
@@ -1212,6 +1212,8 @@
/*OUT*/SizeT* n_dimage )
{
HChar* debugpath = NULL;
+ HChar* extrapath = VG_(clo_include_debugsyms_path);
+ Bool triedextra = False;
Addr addr = 0;
UWord size = 0;
@@ -1220,16 +1222,25 @@
if (buildid != NULL) {
debugpath = ML_(dinfo_zalloc)(
"di.fdf.1",
- VG_(strlen)(buildid) + 33);
+ VG_(strlen)(buildid) + 33 +
+ (extrapath ? VG_(strlen)(extrapath) : 0));
VG_(sprintf)(debugpath, "/usr/lib/debug/.build-id/%c%c/%s.debug",
buildid[0], buildid[1], buildid + 2);
+tryextra1:
if ((addr = open_debug_file(debugpath, buildid, 0,
rel_ok, &size)) == 0) {
+ if (extrapath && triedextra == False) {
+ triedextra = True;
+ VG_(sprintf)(debugpath, "%s/usr/lib/debug/.build-id/%c%c/%s.debug",
+ extrapath, buildid[0], buildid[1], buildid + 2);
+ goto tryextra1;
+ }
ML_(dinfo_free)(debugpath);
debugpath = NULL;
}
+ triedextra = False;
}
if (addr == 0 && debugname != NULL && !rel_ok) {
@@ -1241,15 +1252,39 @@
debugpath = ML_(dinfo_zalloc)(
"di.fdf.3",
- VG_(strlen)(objdir) + VG_(strlen)(debugname) + 32);
+ VG_(strlen)(objdir) + VG_(strlen)(debugname) + 32 +
+ (extrapath ? VG_(strlen)(extrapath) : 0));
VG_(sprintf)(debugpath, "%s/%s", objdir, debugname);
+tryextra2:
if ((addr = open_debug_file(debugpath, NULL, crc, rel_ok, &size)) == 0) {
+ if (extrapath && triedextra == False) {
+ triedextra = True;
+ VG_(sprintf)(debugpath, "%s/%s/%s", extrapath, objdir, debugname);
+ goto tryextra2;
+ }
+ triedextra = False;
+
VG_(sprintf)(debugpath, "%s/.debug/%s", objdir, debugname);
+tryextra3:
if ((addr = open_debug_file(debugpath, NULL, crc, rel_ok, &size)) == 0) {
+ if (extrapath && triedextra == False) {
+ triedextra = True;
+ VG_(sprintf)(debugpath, "%s/%s/.debug/%s", extrapath,
+ objdir, debugname);
+ goto tryextra3;
+ }
+ triedextra = False;
+
VG_(sprintf)(debugpath, "/usr/lib/debug%s/%s", objdir, debugname);
- addr = open_debug_file(debugpath, NULL, crc, rel_ok, &size);
+ if ((addr = open_debug_file(debugpath, NULL, crc, rel_ok, &size)) == 0) {
+ if (extrapath && triedextra == False) {
+ VG_(sprintf)(debugpath, "%s/usr/lib/debug%s/%s", extrapath,
+ objdir, debugname);
+ addr = open_debug_file(debugpath, NULL, crc, rel_ok, &size);
+ }
+ }
}
}
Index: coregrind/m_main.c
===================================================================
--- coregrind/m_main.c (revision 13143)
+++ coregrind/m_main.c (working copy)
@@ -172,6 +172,9 @@
" part of the path after 'string'. Allows removal\n"
" of path prefixes. Use this flag multiple times\n"
" to specify a set of prefixes to remove.\n"
+" --include-debugsyms-path=path absolute path to search for additional\n"
+" debug symbols, in addition to existing default\n"
+" well known search paths.\n"
" --smc-check=none|stack|all|all-non-file [stack]\n"
" checks for self-modifying code: none, only for\n"
" code found in stacks, for all code, or for all\n"
@@ -678,6 +681,9 @@
VG_(clo_n_fullpath_after)++;
}
+ else if VG_STR_CLO (arg, "--include-debugsyms-path",
+ VG_(clo_include_debugsyms_path)) {}
+
else if VG_STR_CLO(arg, "--require-text-symbol", tmp_str) {
if (VG_(clo_n_req_tsyms) >= VG_CLO_MAX_REQ_TSYMS) {
VG_(fmsg_bad_option)(arg,
Index: coregrind/m_options.c
===================================================================
--- coregrind/m_options.c (revision 13143)
+++ coregrind/m_options.c (working copy)
@@ -79,6 +79,7 @@
const HChar* VG_(clo_suppressions)[VG_CLO_MAX_SFILES];
Int VG_(clo_n_fullpath_after) = 0;
const HChar* VG_(clo_fullpath_after)[VG_CLO_MAX_FULLPATH_AFTER];
+HChar* VG_(clo_include_debugsyms_path) = NULL;
UChar VG_(clo_trace_flags) = 0; // 00000000b
UChar VG_(clo_profile_flags) = 0; // 00000000b
Int VG_(clo_trace_notbelow) = -1; // unspecified
Index: coregrind/pub_core_options.h
===================================================================
--- coregrind/pub_core_options.h (revision 13143)
+++ coregrind/pub_core_options.h (working copy)
@@ -126,6 +126,9 @@
extern Int VG_(clo_n_fullpath_after);
extern const HChar* VG_(clo_fullpath_after)[VG_CLO_MAX_FULLPATH_AFTER];
+/* Full path to additional path to search for debug symbols */
+extern HChar* VG_(clo_include_debugsyms_path);
+
/* DEBUG: print generated code? default: 00000000 ( == NO ) */
extern UChar VG_(clo_trace_flags);
/* DEBUG: do bb profiling? default: 00000000 ( == NO ) */
|