Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug file pointed by .gnu_debuglink

[Matija Glavinic Pecotic]

On 08/26/2016 08:12 AM, Namhyung Kim wrote:
> On Tue, Aug 23, 2016 at 01:22:04PM +0200, Jiri Olsa wrote:
>> On Tue, Aug 23, 2016 at 07:09:18AM +0200, Matija Glavinic Pecotic wrote:
>>> diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
>>> index 97c0f8f..a1d3c93 100644
>>> --- a/tools/perf/util/unwind-libunwind-local.c
>>> +++ b/tools/perf/util/unwind-libunwind-local.c
>>> @@ -35,6 +35,7 @@
>>>  #include "util.h"
>>>  #include "debug.h"
>>>  #include "asm/bug.h"
>>> +#include "dso.h"
>>>  
>>>  extern int
>>>  UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
>>> @@ -296,6 +297,8 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>>>  {
>>>  	int fd;
>>>  	u64 ofs = dso->data.debug_frame_offset;
>>> +	char *debuglink = malloc(PATH_MAX);
>>> +	int ret = 0;
>>>  
>>>  	if (ofs == 0) {
>>>  		fd = dso__data_get_fd(dso, machine);
>>> @@ -304,8 +307,31 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>>>  
>>>  		/* Check the .debug_frame section for unwinding info */
>>>  		ofs = elf_section_offset(fd, ".debug_frame");
>>> -		dso->data.debug_frame_offset = ofs;
>>>  		dso__data_put_fd(dso);
>>> +
>>> +		if (!ofs) {
>>> +			/* If not found, try to lookup in debuglink */
>>> +			ret = dso__read_binary_type_filename(
>>> +				dso, DSO_BINARY_TYPE__DEBUGLINK,
>>> +				machine->root_dir, debuglink, PATH_MAX);
>>> +			if (!ret) {
>>> +				fd = open(debuglink, O_RDONLY);
>>> +				if (fd < 0)
>>> +					return -EINVAL;
>>> +
>>> +				ofs = elf_section_offset(fd, ".debug_frame");
>>> +				close(fd);
>>> +
>>> +				if (ofs) {
>>> +					dso->symsrc_filename = debuglink;
>>
>> symsrc_filename is initialized with file that has symtab,
>> which I'm not sure is guaranteed in here as well..
> 
> I'm also not sure it's guaranteed that having debuginfo implies having
> symtab.  But I didn't see any ELF binary which has debugginfo but no
> symtab.  Maybe we can add the check for sure.
> 
> Anyway, it needs to free the debuglink if not used.

Here is latest what I have on this:

diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 97c0f8f..5df4c74 100644
--- a/tools/perf/util/unwind-libunwind-local.c
+++ b/tools/perf/util/unwind-libunwind-local.c
@@ -35,6 +35,7 @@
 #include "util.h"
 #include "debug.h"
 #include "asm/bug.h"
+#include "dso.h"
 
 extern int
 UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
@@ -297,15 +298,58 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
 	int fd;
 	u64 ofs = dso->data.debug_frame_offset;
 
+	/* debug_frame can reside in:
+	 *  - dso
+	 *  - debug pointed by symsrc_filename
+	 *  - gnu_debuglink, which doesnt necessary
+	 *    has to be pointed by symsrc_filename
+	 */
 	if (ofs == 0) {
 		fd = dso__data_get_fd(dso, machine);
-		if (fd < 0)
-			return -EINVAL;
+		if (fd >= 0) {
+			ofs = elf_section_offset(fd, ".debug_frame");
+			dso__data_put_fd(dso);
+		}
+
+		if (ofs <= 0) {
+			fd = open(dso->symsrc_filename, O_RDONLY);
+			if (fd >= 0) {
+				ofs = elf_section_offset(fd, ".debug_frame");
+				close(fd);
+			}
+		}
+
+		if (ofs <= 0) {
+			char *debuglink = malloc(PATH_MAX);
+			int ret = 0;
+
+			ret = dso__read_binary_type_filename(
+				dso, DSO_BINARY_TYPE__DEBUGLINK,
+				machine->root_dir, debuglink, PATH_MAX);
+			if (!ret) {
+				fd = open(debuglink, O_RDONLY);
+				if (fd >= 0) {
+					ofs = elf_section_offset(fd,
+							".debug_frame");
+					close(fd);
+				}
+			}
+			if (ofs > 0) {
+				if (dso->symsrc_filename != NULL) {
+					pr_warning(
+						"%s: overwrite symsrc(%s,%s)\n",
+							__func__,
+							dso->symsrc_filename,
+							debuglink);
+					free(dso->symsrc_filename);
+				}
+				dso->symsrc_filename = debuglink;
+			} else {
+				free(debuglink);
+			}
+		}
 
-		/* Check the .debug_frame section for unwinding info */
-		ofs = elf_section_offset(fd, ".debug_frame");
 		dso->data.debug_frame_offset = ofs;
-		dso__data_put_fd(dso);
 	}
 
 	*offset = ofs;

Third case, when we actually try to read debuglink because .debug_frame wasn't
found in symsrc is questionable. What should not happen is that at this point
symsrc points to nothing, as symsrc is populated at dso__load, and debuglink is
one of the candidates. Unless debuglink really lacks of symtab, in which case
it will not be populated in symsrc_filename.

symsrc_filename populated, but not with debug_link will happen if multiple set
of debug files exist, e.g. debuglink and build id. That indicates missconfig
from several points of view, notably, one debug file clearly lacking debug_frame
as in that case debug link wouldn't tried to be read.

So this third case is more best effort which would result with warning thrown
and symsrc overwritten with new debuglink. Warning is there to note user that
something is wrong.

If you have some better idea to handle this situation, please feel free to suggest

Thanks,

Matija