|
From: Florian K. <fl...@ei...> - 2014-07-25 11:34:19
|
I have been looking at get_sym_name and its various wrapper
functions (such as get_fnname_w_offset). Those functions all take
a character buffer as argument for the symbol name they
are supposed to obtain and return a boolean result indicating
success or not. On failure, the symbol name is not written to
(debuginfo.c, line 1640).
That was a surprise to me in the sense that I would have expected
a return argument to always be initialised as that is semantically
easier and less fragile.
We are also missing an opportunity here to have the name of the
unknown symbol be unique and hard-wired in a single place
(in get_sym_name).
Currently there are at least three names for the unknown symbol
and I don't think they are that way by design :)
(a) m_sbprofile.c, line 87: name = ""
(b) m_translate.c, line 1418: name = "UNKNOWN_FUNCTION"
(c) m_translate.c, line 1393: name = "???"
I'm proposing to change get_sym_name to initialise its return buffer
with the name of the unknown symbol (patch below) and adjust all
call sites accordingly. We can then get rid of the various
sprinkles of
ok = VG_(get_fnname_w_offset)(addr, name2, sizeof(name2));
if (!ok) VG_(strcpy)(name2, "???");
that we have in several places.
Any objections?
Florian
Index: coregrind/m_debuginfo/debuginfo.c
===================================================================
--- coregrind/m_debuginfo/debuginfo.c (revision 14190)
+++ coregrind/m_debuginfo/debuginfo.c (working copy)
@@ -1636,8 +1636,10 @@
PtrdiffT offset;
search_all_symtabs ( a, &di, &sno, match_anywhere_in_sym, findText );
- if (di == NULL)
+ if (di == NULL) {
+ VG_(strncpy_safely)(buf, "???", nbuf); // unknown symbol
return False;
+ }
vg_assert(di->symtab[sno].pri_name);
VG_(demangle) ( do_cxx_demangling, do_z_demangling,
|
> + VG_(strncpy_safely)(buf, "???", nbuf); // unknown symbol Beware performance loss. strncpy _always_ writes nbuf bytes (3rd argument), so the execution of that code finishes with the equivalent of memset(&buf[3], 0, nbuf - 3); This will be much faster: memcpy(buf, "???", 3+1); as long as the other code requires only one terminating '\0'. [Dress up with the appropriate VG_() and other adornments.] |
|
From: Philippe W. <phi...@sk...> - 2014-07-25 22:15:20
|
On Fri, 2014-07-25 at 05:58 -0700, John Reiser wrote: > > + VG_(strncpy_safely)(buf, "???", nbuf); // unknown symbol > > Beware performance loss. strncpy _always_ writes nbuf bytes (3rd argument), > so the execution of that code finishes with the equivalent of > memset(&buf[3], 0, nbuf - 3); > > This will be much faster: > memcpy(buf, "???", 3+1); > as long as the other code requires only one terminating '\0'. > [Dress up with the appropriate VG_() and other adornments.] Funny that we just got a big performance regression due to the rewriting of VG_(strncpy_safely) : it was not zero-filling and since rev 14186, it is zero filling. That gave for some tests more than 50% perf regressions. Philippe |
|
From: Philippe W. <phi...@sk...> - 2014-07-25 22:24:41
|
On Fri, 2014-07-25 at 13:34 +0200, Florian Krohm wrote:
> I have been looking at get_sym_name and its various wrapper
> functions (such as get_fnname_w_offset). Those functions all take
> a character buffer as argument for the symbol name they
> are supposed to obtain and return a boolean result indicating
> success or not. On failure, the symbol name is not written to
> (debuginfo.c, line 1640).
>
> That was a surprise to me in the sense that I would have expected
> a return argument to always be initialised as that is semantically
> easier and less fragile.
>
> We are also missing an opportunity here to have the name of the
> unknown symbol be unique and hard-wired in a single place
> (in get_sym_name).
>
> Currently there are at least three names for the unknown symbol
> and I don't think they are that way by design :)
>
> (a) m_sbprofile.c, line 87: name = ""
> (b) m_translate.c, line 1418: name = "UNKNOWN_FUNCTION"
> (c) m_translate.c, line 1393: name = "???"
>
> I'm proposing to change get_sym_name to initialise its return buffer
> with the name of the unknown symbol (patch below) and adjust all
> call sites accordingly. We can then get rid of the various
> sprinkles of
>
> ok = VG_(get_fnname_w_offset)(addr, name2, sizeof(name2));
> if (!ok) VG_(strcpy)(name2, "???");
>
> that we have in several places.
>
> Any objections?
Looks a good idea to me.
Some feedback:
* need to avoid a similar perf regression as brought in by rev 14186
we might have to rewrite (and maybe rename) VG_(strncpy_safely)
to make it less strncpy like.
* Maybe some messages might become less understandable if replacing
"UNKNOWN_FUNCTION" by "???"
e.g.
VG_(printf("I got a problem in %s\n", fn);
might change of output from
I got a problem in UNKNOWN_FUNCTION
to
I got a problem in ???
So it might be needed to examine the callers and see what they do
with the result.
Philippe
|