File index given line (libdw)

File index given line (libdw)

[Sasha Da Rocha Pinheiro]

Hi guys, 
when we get the files given a Die and want to know the file given the line, the only way to do this is comparing the return of dwarf_linesrc with the list of filenames we had from dwarf_getsrcfiles?
Why can't we get the index of the file given the line? Just return the unsigned int file in Dwarf_Line_s.


/* Find line information for address.  */
extern const char *dwarf_linesrc (Dwarf_Line *line,
                                             Dwarf_Word *mtime, Dwarf_Word *length);


* Get the file source files used in the CU.  */
extern int dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files,
			                     size_t *nfiles)
__nonnull_attribute__ (2);


Regards,
Sasha

Re: File index given line (libdw)

[Mark Wielaard]

Hi Sasha,

On Thu, 2017-07-13 at 00:25 +0000, Sasha Da Rocha Pinheiro wrote:
> when we get the files given a Die and want to know the file given the
> line, the only way to do this is comparing the return of dwarf_linesrc
> with the list of filenames we had from dwarf_getsrcfiles?
> Why can't we get the index of the file given the line? Just return the unsigned int file in Dwarf_Line_s.

There could be a good reason for that. But I might be missing the bigger
picture of what you are trying to do. Could you give a more complete
code example? What extra information do you want by having the
Dwarf_Files index of the Dwarf_Line?

Thanks,

Mark

Re: File index given line (libdw)

[Sasha Da Rocha Pinheiro]

Hi Mark, 

given a Die, I want to associate each line (from dwarf_getsrclines) to a file (from dwarf_getsrcfiles). The only way to do this is get the filename returned by dwarf_linesrc, and search in the filenames given by dwarf_getsrcfiles and dwarf_filesrc. But this is expensive since this has to be done for each line. Instead, if I could get the file index in the array "struct Dwarf_Fileinfo_s info", I wouldn't need to search.


  
From: Mark Wielaard <mark@klomp.org>
Sent: Thursday, July 13, 2017 4:11:09 AM
To: Sasha Da Rocha Pinheiro
Cc: elfutils-devel@sourceware.org
Subject: Re: File index given line (libdw)
    
Hi Sasha,

On Thu, 2017-07-13 at 00:25 +0000, Sasha Da Rocha Pinheiro wrote:
> when we get the files given a Die and want to know the file given the
> line, the only way to do this is comparing the return of dwarf_linesrc
> with the list of filenames we had from dwarf_getsrcfiles?
> Why can't we get the index of the file given the line? Just return the unsigned int file in Dwarf_Line_s.

There could be a good reason for that. But I might be missing the bigger
picture of what you are trying to do. Could you give a more complete
code example? What extra information do you want by having the
Dwarf_Files index of the Dwarf_Line?

Thanks,

Mark
    

Re: File index given line (libdw)

[Mark Wielaard]

On Thu, 2017-07-13 at 17:33 +0000, Sasha Da Rocha Pinheiro wrote:
> given a Die, I want to associate each line (from dwarf_getsrclines) to
> a file (from dwarf_getsrcfiles). The only way to do this is get the
> filename returned by dwarf_linesrc, and search in the filenames given
> by dwarf_getsrcfiles and dwarf_filesrc. But this is expensive since
> this has to be done for each line. Instead, if I could get the file
> index in the array "struct Dwarf_Fileinfo_s info", I wouldn't need to
> search.

But why do you want to do that?
If we would add int dwarf_line_index (Dwarf_Line *line, size_t *idx) how
exactly would you use it?
What would be a good testcase for this new functionality?
A small example program would help to see what the exact semantics
should be.

Thanks,

Mark

Re: File index given line (libdw)

[Sasha Da Rocha Pinheiro]

Hi Mark,

>But why do you want to do that?

Performance and save memory space.


>If we would add int dwarf_line_index (Dwarf_Line *line, size_t *idx) how
>exactly would you use it?

I would get idx and save it in my data structure so I don't have to save the file name repeatedly for each line.


>What would be a good testcase for this new functionality?
This new function would behave similar to:

int dwarf_lineno (Dwarf_Line *line, int *linep)
{
  if (line == NULL)
    return -1;

  *linep =  line->line;

  return 0;
}

But would get line->file instead of line->line;
This way, since I already got the files previously, I now can refer to some line file source by its id.


>A small example program would help to see what the exact semantics
>should be.
How it's currently being done :

1. Dwarf_Files dfs <- dwarf_getsrcfiles
2. for each file in dfs get name (with dwarf_filesrc) -> save in vector filenames
3. Dwarf_Lines dls <- dwarf_getsrclines
4. for each line in dls get file name and "search this name in vector filenames"

We want to eliminate the "search this name in vector filenames", to make it from L F log(F) to L, by getting the index and consulting the file name of a line in the vector filenames directly.

Regards,
Sasha

Re: File index given line (libdw)

[Mark Wielaard]

On Sat, 2017-07-15 at 01:22 +0000, Sasha Da Rocha Pinheiro wrote:
> >But why do you want to do that?
> 
> Performance and save memory space.
> 
> >If we would add int dwarf_line_index (Dwarf_Line *line, size_t *idx) how
> >exactly would you use it?
> 
> I would get idx and save it in my data structure so I don't have to save the file name repeatedly for each line.

size_t and char * are the same size. Whether you use an index or a
string pointer doesn't mean the underlying strings are identical or not.
If you really need to know if they are equal you still need to compare
the actual strings.

> >A small example program would help to see what the exact semantics
> >should be.
> How it's currently being done :
> 
> 1. Dwarf_Files dfs <- dwarf_getsrcfiles
> 2. for each file in dfs get name (with dwarf_filesrc) -> save in vector filenames
> 3. Dwarf_Lines dls <- dwarf_getsrclines
> 4. for each line in dls get file name and "search this name in vector filenames"
> 
> We want to eliminate the "search this name in vector filenames", to
> make it from L F log(F) to L, by getting the index and consulting the
> file name of a line in the vector filenames directly.

So you want to keep a vector with filenames for a particular CU. And
then given Dwarf_Lines you want to associate each Dwarf_Line with a
particular filename from that vector.

Do you get the Dwarf_Line from dwarf_onesrcline() or dwarf_getsrc_die()?

I assume you then use dwarf_linesrc() to get the filename associated
with the Dwarf_Line.

Why is it important to match this particular filename to one in the
vector you created from the Dwarf_Files?

There is indeed an association between the DwarfLines and the
Dwarf_Files. But currently that is an implementation detail. If we
expose the Dwarf_Line filename index associated with the Dwarf_Files
then it becomes a public interface.

I am not really that opposed to exposing this information. It might be
fine. But I would like to understand why you need/want this information.

Cheers,

Mark

Re: File index given line (libdw)

[Sasha Da Rocha Pinheiro]

[Resending cause it seems it didn't go]

You understood what I need when you said:
"So you want to keep a vector with filenames for a particular CU. And
then given Dwarf_Lines you want to associate each Dwarf_Line with a
particular filename from that vector."

I agree with this "But currently that is an implementation detail. "
But I don't see why this would change. Deliberately sort the files names so it doesn't match with the lines? I don't think so.

What I need is direct access to the files I already have from a previous call to eliminate unnecessary search.

"Why is it important to match this particular filename to one in the vector you created from the Dwarf_Files?"
Because Dyninst needs to make its own parsing and fill its own data structures in the most efficient way.





From: Mark Wielaard <mark@klomp.org>
Sent: Saturday, July 15, 2017 8:56 AM
To: Sasha Da Rocha Pinheiro
Cc: elfutils-devel@sourceware.org
Subject: Re: File index given line (libdw)
    
On Sat, 2017-07-15 at 01:22 +0000, Sasha Da Rocha Pinheiro wrote:
> >But why do you want to do that?
> 
> Performance and save memory space.
> 
> >If we would add int dwarf_line_index (Dwarf_Line *line, size_t *idx) how
> >exactly would you use it?
> 
> I would get idx and save it in my data structure so I don't have to save the file name repeatedly for each line.

size_t and char * are the same size. Whether you use an index or a
string pointer doesn't mean the underlying strings are identical or not.
If you really need to know if they are equal you still need to compare
the actual strings.

> >A small example program would help to see what the exact semantics
> >should be.
> How it's currently being done :
> 
> 1. Dwarf_Files dfs <- dwarf_getsrcfiles
> 2. for each file in dfs get name (with dwarf_filesrc) -> save in vector filenames
> 3. Dwarf_Lines dls <- dwarf_getsrclines
> 4. for each line in dls get file name and "search this name in vector filenames"
> 
> We want to eliminate the "search this name in vector filenames", to
> make it from L F log(F) to L, by getting the index and consulting the
> file name of a line in the vector filenames directly.

So you want to keep a vector with filenames for a particular CU. And
then given Dwarf_Lines you want to associate each Dwarf_Line with a
particular filename from that vector.

Do you get the Dwarf_Line from dwarf_onesrcline() or dwarf_getsrc_die()?

I assume you then use dwarf_linesrc() to get the filename associated
with the Dwarf_Line.

Why is it important to match this particular filename to one in the
vector you created from the Dwarf_Files?

There is indeed an association between the DwarfLines and the
Dwarf_Files. But currently that is an implementation detail. If we
expose the Dwarf_Line filename index associated with the Dwarf_Files
then it becomes a public interface.

I am not really that opposed to exposing this information. It might be
fine. But I would like to understand why you need/want this information.

Cheers,

Mark
    

Re: File index given line (libdw)

[Mark Wielaard]

On Mon, 2017-07-17 at 04:10 +0000, Sasha Da Rocha Pinheiro wrote:
> [Resending cause it seems it didn't go]

Probably because it had an HTML attachement.
The mailinglist rejects emails with HTML.

> You understood what I need when you said:
> "So you want to keep a vector with filenames for a particular CU. And
> then given Dwarf_Lines you want to associate each Dwarf_Line with a
> particular filename from that vector."
> 
> I agree with this "But currently that is an implementation detail. "
> But I don't see why this would change. Deliberately sort the files
> names so it doesn't match with the lines? I don't think so.

But newer versions of DWARF could have a different way of sharing the
source files or directory table between CUs. The issue is that the index
from the Dwarf_Lines only makes sense if you know which CU the
corresponding Dwarf_Files came from. So instead of providing the
relationship between a Dwarf_Line and a Dwarf_File I rather would not
depend on something indirect like the index into some Dwarf_Files table.
If we do provide it then I think we should be explicit about what it is
an index into. So maybe a function like:

extern int *
dwarf_line_files (Dwarf_Line *line, Dwarf_Files **files, size_t *idx);

Where files and idx are returned in a way that make them useful with
dwarf_filesrc. Would the above be helpful for your usecase?

Cheers,

Mark

Re: File index given line (libdw)

[Sasha Da Rocha Pinheiro]

I understand your argument. Since I was doing:
   
   dwarf_getsrclines(&cuDIE, &lineBuffer, &lineCount)
   dwarf_getsrcfiles(&cuDIE, &files, &filecount)

I knew they were related because I used the same DIE. But someone trying to use a function which returns only the index could be confusing about what Dwarf_Files it came from. 

But, with

extern int *
dwarf_line_files (Dwarf_Line *line, Dwarf_Files **files, size_t *idx);

they would know what Dwarf_Files the line is related to. For me, though, I could pass NULL for files, since I already knew what it is.




From: Mark Wielaard <mark@klomp.org>
Sent: Monday, July 17, 2017 8:04 AM
To: Sasha Da Rocha Pinheiro
Cc: elfutils-devel@sourceware.org
Subject: Re: File index given line (libdw)
    
On Mon, 2017-07-17 at 04:10 +0000, Sasha Da Rocha Pinheiro wrote:
> [Resending cause it seems it didn't go]

Probably because it had an HTML attachement.
The mailinglist rejects emails with HTML.

> You understood what I need when you said:
> "So you want to keep a vector with filenames for a particular CU. And
> then given Dwarf_Lines you want to associate each Dwarf_Line with a
> particular filename from that vector."
> 
> I agree with this "But currently that is an implementation detail. "
> But I don't see why this would change. Deliberately sort the files
> names so it doesn't match with the lines? I don't think so.

But newer versions of DWARF could have a different way of sharing the
source files or directory table between CUs. The issue is that the index
from the Dwarf_Lines only makes sense if you know which CU the
corresponding Dwarf_Files came from. So instead of providing the
relationship between a Dwarf_Line and a Dwarf_File I rather would not
depend on something indirect like the index into some Dwarf_Files table.
If we do provide it then I think we should be explicit about what it is
an index into. So maybe a function like:

extern int *
dwarf_line_files (Dwarf_Line *line, Dwarf_Files **files, size_t *idx);

Where files and idx are returned in a way that make them useful with
dwarf_filesrc. Would the above be helpful for your usecase?

Cheers,

Mark
    

Re: File index given line (libdw)

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

On Mon, 2017-07-17 at 17:28 +0000, Sasha Da Rocha Pinheiro wrote:
> I understand your argument. Since I was doing:
>    
>    dwarf_getsrclines(&cuDIE, &lineBuffer, &lineCount)
>    dwarf_getsrcfiles(&cuDIE, &files, &filecount)
> 
> I knew they were related because I used the same DIE. But someone
> trying to use a function which returns only the index could be
> confusing about what Dwarf_Files it came from. 
> 
> But, with
> 
> extern int *
> dwarf_line_files (Dwarf_Line *line, Dwarf_Files **files, size_t *idx);
> 
> they would know what Dwarf_Files the line is related to. For me,
> though, I could pass NULL for files, since I already knew what it is.

I would always double check your assumption that the Dwarf_Files match
with what you expect. And I am still not really convinced storing the
index as a proxy for the actual file name/path associated with a
Dwarf_Line is really a good idea.

But I was working on adding support for DWARF5 .debug_lines and since
files (and directories) can have arbitrary attributes in that version it
makes sense to go from Dwarf_Line to the associated Dwarf_Files/index.
Instead of duplicating the accessor functions (like we now have both
dwarf_filesrc and dwarf_linesrc).

Note that I changed the name to dwarf_line_file. (Dropping the s.)

This does show that we might want to change how we internally store the
Dwarf_Files though. Currently we resolve the whole file path immediately
and don't keep track of the original file name and directory name/path.
You can only get the full file path (through dwarf_filesrc) but not the
associated directory. I am cleaning that up while adding the DWARF5
support. And will add a more generic interface for access file and
directory properties.

Cheers,

Mark

[-- Attachment #2: 0001-libdw-Add-dwarf_line_file.patch --]
[-- Type: text/x-patch, Size: 5893 bytes --]

From 84ded5d99cbb20de864ecbca8d0d860510da4234 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Fri, 21 Jul 2017 21:23:07 +0200
Subject: [PATCH] libdw: Add dwarf_line_file.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdw/ChangeLog         |  7 +++++++
 libdw/Makefile.am       |  2 +-
 libdw/dwarf_line_file.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
 libdw/libdw.h           |  5 +++++
 libdw/libdw.map         |  5 +++++
 tests/ChangeLog         |  4 ++++
 tests/get-lines.c       | 23 ++++++++++++++++++++++
 7 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 libdw/dwarf_line_file.c

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 1e282e4..5d9091a 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,10 @@
+2017-07-21  Mark Wielaard  <mark@klomp.org>
+
+	* dwarf_line_file.c: New file.
+	* Makefile.am (libdw_a_SOURCES): Add dwarf_line_file.c.
+	* libdw.h (dwarf_line_file): New function declaration.
+	* libdw.map (ELFUTILS_0.167): New. Add dwarf_line_file.
+
 2017-04-20  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* libdw.h: Remove attribute macro declarations and use
diff --git a/libdw/Makefile.am b/libdw/Makefile.am
index 082d96c..bde8856 100644
--- a/libdw/Makefile.am
+++ b/libdw/Makefile.am
@@ -65,7 +65,7 @@ libdw_a_SOURCES = dwarf_begin.c dwarf_begin_elf.c dwarf_end.c dwarf_getelf.c \
 		  dwarf_lineendsequence.c dwarf_lineblock.c \
 		  dwarf_lineprologueend.c dwarf_lineepiloguebegin.c \
 		  dwarf_lineisa.c dwarf_linediscriminator.c \
-		  dwarf_lineop_index.c \
+		  dwarf_lineop_index.c dwarf_line_file.c \
 		  dwarf_onesrcline.c dwarf_formblock.c \
 		  dwarf_getsrcfiles.c dwarf_filesrc.c dwarf_getsrcdirs.c \
 		  dwarf_getlocation.c dwarf_getstring.c dwarf_offabbrev.c \
diff --git a/libdw/dwarf_line_file.c b/libdw/dwarf_line_file.c
new file mode 100644
index 0000000..e2df642
--- /dev/null
+++ b/libdw/dwarf_line_file.c
@@ -0,0 +1,52 @@
+/* Find line information for address.
+   Copyright (C) 2017 Red Hat, Inc.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+     * the GNU Lesser General Public License as published by the Free
+       Software Foundation; either version 3 of the License, or (at
+       your option) any later version
+
+   or
+
+     * the GNU General Public License as published by the Free
+       Software Foundation; either version 2 of the License, or (at
+       your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include "libdwP.h"
+
+
+int
+dwarf_line_file (Dwarf_Line *line, Dwarf_Files **files, size_t *idx)
+{
+  if (line == NULL)
+    return -1;
+
+  if (line->file >= line->files->nfiles)
+    {
+      __libdw_seterrno (DWARF_E_INVALID_DWARF);
+      return -1;
+    }
+
+  *files = line->files;
+  *idx = line->file;
+
+  return 0;
+}
diff --git a/libdw/libdw.h b/libdw/libdw.h
index 9ae80eb..4903b55 100644
--- a/libdw/libdw.h
+++ b/libdw/libdw.h
@@ -640,6 +640,11 @@ extern const char *dwarf_linesrc (Dwarf_Line *line,
 extern const char *dwarf_filesrc (Dwarf_Files *file, size_t idx,
 				  Dwarf_Word *mtime, Dwarf_Word *length);
 
+/* Return the Dwarf_Files and index associated with the given Dwarf_Line.  */
+extern int dwarf_line_file (Dwarf_Line *line,
+			    Dwarf_Files **files, size_t *idx)
+    __nonnull_attribute__ (2, 3);
+
 /* Return the directory list used in the file information extracted.
    (*RESULT)[0] is the CU's DW_AT_comp_dir value, and may be null.
    (*RESULT)[0..*NDIRS-1] are the compile-time include directory path
diff --git a/libdw/libdw.map b/libdw/libdw.map
index 83cb1d9..44e096a 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -338,3 +338,8 @@ ELFUTILS_0.167 {
     dwelf_strent_str;
     dwelf_strtab_free;
 } ELFUTILS_0.165;
+
+ELFUTILS_0.170 {
+  global:
+    dwarf_line_file;
+} ELFUTILS_0.167;
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 3dd6f2a..920084f 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,7 @@
+2017-07-21  Mark Wielaard  <mark@klomp.org>
+
+	* get-lines.c (main): Add dwarf_line_file test.
+
 2017-07-14  Mark Wielaard  <mark@klomp.org>
 
 	* run-strip-remove-keep.sh: New test.
diff --git a/tests/get-lines.c b/tests/get-lines.c
index c361a2c..188d016 100644
--- a/tests/get-lines.c
+++ b/tests/get-lines.c
@@ -24,6 +24,7 @@
 #include <libelf.h>
 #include ELFUTILS_HEADER(dw)
 #include <stdio.h>
+#include <string.h>
 #include <unistd.h>
 
 
@@ -100,6 +101,28 @@ main (int argc, char *argv[])
 	      printf ("%" PRIx64 ": %s:%d:", (uint64_t) addr,
 		      file ?: "???", line);
 
+	      /* Getting the file path through the Dwarf_Files should
+		 result in the same path.  */
+	      Dwarf_Files *files;
+	      size_t idx;
+	      if (dwarf_line_file (l, &files, &idx) != 0)
+		{
+		  printf ("%s: cannot get file from line (%zd): %s\n",
+			  argv[cnt], i, dwarf_errmsg (-1));
+		  result = 1;
+		  break;
+		}
+	      const char *path = dwarf_filesrc (files, idx, NULL, NULL);
+	      if ((path == NULL && file != NULL)
+		  || (path != NULL && file == NULL)
+		  || (strcmp (file, path) != 0))
+		{
+		  printf ("%s: line %zd srcline (%s) != file srcline (%s)\n",
+			  argv[cnt], i, file ?: "???", path ?: "???");
+		  result = 1;
+		  break;
+		}
+
 	      int column;
 	      if (dwarf_linecol (l, &column) != 0)
 		column = 0;
-- 
1.8.3.1

Re: File index given line (libdw)

[Mark Wielaard]

On Fri, 2017-07-21 at 21:29 +0200, Mark Wielaard wrote:
> But I was working on adding support for DWARF5 .debug_lines and since
> files (and directories) can have arbitrary attributes in that version it
> makes sense to go from Dwarf_Line to the associated Dwarf_Files/index.
> Instead of duplicating the accessor functions (like we now have both
> dwarf_filesrc and dwarf_linesrc).
> 
> Note that I changed the name to dwarf_line_file. (Dropping the s.)

I pushed this new function to git master now.

Cheers,

Mark