Re: [PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

["Pierre-Loup A. Griffais" <pgriffais@nvidia.com>]

Thanks a lot for taking a look, Arnaldo; inline:

On 06/21/2012 10:28 AM, Arnaldo Carvalho de Melo wrote:

> Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
>> The .gnu_debuglink section is specified to contain the filename of the
>> debug info file, as well as a CRC that can be used to validate it.
> 
>> This provides more context:
>> http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
>>
>> +++ b/tools/perf/util/symbol.c
>> +static int filename__read_debuglink(const char *filename,
>> +				    char *path, size_t size)
>> +{
> 
> Isn't there any other function that opens an ELF file, looks for an
> specific session to then read its contents?
> 
> Couldn't it be reused here?


There are plenty, but they're all different enough that trying to fold
them into common code seemed involved and risky. The closest looks like
elf_read_build_id(), but in order to leverage that one the common
function would need to take a list of sections to try in sequence.
Unless you have strong feelings against using filename__read_debuglink()
as-is I would recommend that any such work happen outside of that patch
in the interest of minimizing risk.

> 
>> +		case SYMTAB__DEBUGLINK: {
>> +			char *last_slash;
>> +			strncpy(name, dso->long_name, size);
>> +			last_slash = name + dso->long_name_len;
>> +			while (last_slash && *last_slash != '/')
>> +				last_slash--;
> 
> Why the test for last_slash to be != NULL? How could it ever be? This is
> an optimization since we have the dso->long_name_len so that we avoid
> using strrchr that in turn would do an strlen?
> 
> So the test should be:
> 
> 			while (last_slash != name && *last_slash != '/')
> 
> To avoid underflowing, right?


Oops, yeah; not what I had in mind. Thanks for catching that.


>> +			if (last_slash)
>> +				last_slash++;
>> +			filename__read_debuglink(dso->long_name, last_slash,
> 
> How last_slash can point to the path? It looks like it points to the
> basename, no?
> 
> Yeah, it is, and then your algorithm will work because last_slash
> doesn't point to the _path_, but to a string _preceded_ by the path, so
> for /bin/ls, the debuglink content would be ls.debug and that is what
> will be stashed there, ending up with:
> 
> name = "/bin/\0"
> last_slash---^
> name = "/bin/ls.debug\0"
> 
> I.e. its not really the last slash, but where the debuglink content has
> to be stashed, concatenating with the same dirname as the associated
> binary.


Yes, that's correct; sorry for the ill-named variables.

> 
> Can you please rename "last_slash" to "debuglink"? And "path" to
> "debuglink" as well in the routine that reads the debuglink.


Done. I'm proposing to resend with:

			while (debuglink != name && *debuglink != '/')
				debuglink--;
			if (*debuglink == '/')
				debuglink++;

The underflow and subsequent "*debuglink == '/'" checks are only there
to do the right thing in case dso->long_name happens to just be a local
filename without any slashes in it. I realize that's probably never
supposed to happen, so I could remove them if you want. I'd rather err
on the side of caution, though.

> 
>> +						 size - (last_slash - name));
>> +			}
> 
> Other than that, thanks a lot for working on this, surely we have to
> support this feature!


Thanks a lot for walking through the logic. Looking forward to your
comments before I resend!
 - Pierre-Loup

> 
> - ARnaldo