Re: [PATCH 0/4] Improve elfutils diagnostics

Re: [PATCH 0/4] Improve elfutils diagnostics

[Frank Ch. Eigler]

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

Hi -

On Thu, May 14, 2015 at 01:16:57PM +0200, Mark Wielaard wrote:
> [...]
> > How would such a list-of-strings be represented?  Considering that
> > this is C, and the list arity will vary, we'd be doing dynamic
> > allocation anyway, right?
> 
> Less allocation is better than more, especially if there are situations
> where the extra allocations might not be used anyway.

Note that a single concatenated string involves one allocated block at
a time; an array or linked list containing structs and strings(!)
involves many more.  It suffers in terms of copying only.


> [...]  So the ideal way to represent it might be an array plus
> length of pairs typedef struct Dwfl_ErrDetail { int errno, const
> char *detail }.  [...]

That "char *detail;" brings its own memory allocation / lifetime
headaches.  In fact, since the elfutils library would have to strdup
those strings internally (since they can't be assumed to be static),
it's really not much better than just constructing the formatted
string in the first place.


- FChE

Re: [PATCH 0/4] Improve elfutils diagnostics

[Mark Wielaard]

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

Hi Frank,

On Fri, 2015-05-15 at 12:10 -0400, Frank Ch. Eigler wrote:
> On Thu, May 14, 2015 at 01:16:57PM +0200, Mark Wielaard wrote:
> > [...]
> > > How would such a list-of-strings be represented?  Considering that
> > > this is C, and the list arity will vary, we'd be doing dynamic
> > > allocation anyway, right?
> > 
> > Less allocation is better than more, especially if there are situations
> > where the extra allocations might not be used anyway.
> 
> Note that a single concatenated string involves one allocated block at
> a time; an array or linked list containing structs and strings(!)
> involves many more.  It suffers in terms of copying only.

The array will most likely be preallocated already and not having to
turn each error number into a string and then concatenating it into the
file/detail string, plus any earlier such strings, is still a win.

> > [...]  So the ideal way to represent it might be an array plus
> > length of pairs typedef struct Dwfl_ErrDetail { int errno, const
> > char *detail }.  [...]
> 
> That "char *detail;" brings its own memory allocation / lifetime
> headaches.  In fact, since the elfutils library would have to strdup
> those strings internally (since they can't be assumed to be static),
> it's really not much better than just constructing the formatted
> string in the first place.

I think it is better. The detail string will just be the file name
already constructed and allocated, so ownership is clear, it won't have
to be strduped normally. Think of it as "this is the file I tried" plus
"this is the error that happened". Both values are already constructed
and handy. No need to do any additional memory allocation, the lifetime
is just transferred to the lifetime of the Dwfl_ErrDetail.

Cheers,

Mark

Re: [PATCH 0/4] Improve elfutils diagnostics

[Mark Wielaard]

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

Hi Jonathan,

Sorry for the brief reply, I am somewhat offline today, but wanted to
give some quick feedback. Apologies if it is too brief or not too well
thought out. In that case I'll take more time to think things through
tomorrow.

On Wed, May 13, 2015 at 02:44:50PM -0400, Jonathan Lebon wrote:
> > I am still pondering these two patches. The idea is sane. Some
> > operations are compound and a simple error code/string only tells you
> > something failed, but not really why or what was done. There are two or
> > three dwfl functions that would benefit from being able to report more
> > clearly what went wrong. dwfl_module_getdwarf () which you address in
> > patch 4. But also dwfl_module_getelf () and maybe dwfl_module_getsymtab
> > (). Which all do some try-fail-try-something-else lookups.
> 
> Right, I only added support for getdwarf(). Once we settle on an interface,
> I might have a look at the other ones if I have enough time.

Yes, lets make sure the interface works for all of them.

> > getdwarf and
> > getelf both allow the user to plug in their own lookup callbacks to make
> > things things even more interesting.
> 
> Yes, these patches only report what the default callbacks do. I guess it
> might be good to provide a way for callbacks to write their errors to the
> same spot, although I suppose user-provided callbacks also have their own
> way of reporting to the user things that went wrong.

Yes, we might want to provide such a function and document that if the
callback returns failure it might have called that "detailed_failures"
functions a couple of times to provide extra feedback.

> > One implementation detail that I like to see changed is to make the
> > details errmsg a list of strings instead of one big string (and
> > similarly for dw_tried_paths). That makes things a bit cheaper when the
> > result isn't actually used. You don't have to concatenate the strings
> > beforehand and the user can decide how to use the separate detail
> > messages.
> 
> E.g. a linked list of char* ?
> 
> > Also I wonder if this is mixing two issues. a) provide more information
> > (the tried path/file) when something goes wrong and b) providing a
> > "chain of errors" to explain why some call really failed when the
> > operation was really a compounded search result. So do we really need
> > just a string (or a list of strings)? Or do we need two separate things.
> > A way to provide extra (dynamic) detail when signaling a dwfl error. And
> > a way to chain multiple errors in case a function needs to report
> > multiple failure results when an operation failed to show everything
> > that was tried?
> 
> I'm not 100% sure what you mean exactly here re. chaining errors. Do you
> mean specifically for compound operations, the user should be able to
> easily have programmatic access to the different operations that took
> place (along with their respective errors)?
> 
> E.g. a linked list of struct { char* operation; char* errmsg } ?
> 
> Or do you mean that it should be easy in the elfutils codebase to "append"
> to the error message rather than having to accumulate the whole message
> before doing __libdw_seterrno_details(). In that case, we could get away
> with just adding a __libdw_append_details().

See also my message to Frank. I think we should combine these two things.
So a "detail" is an error code plus message (e.g. file name), that the
user can combine, call printf (stderr, "No Dwarf: %s %s\n",
			       dwfl_errmsg (d.err), d.detail);
And there is an int dwfl_details (Dwfl_Detail **details) function that
the user can call to get the number of details there is as an array.
Or maybe even as an opaque type that the user iterates over extracting
the err and msg with a separate function.

Cheers,

Mark

Re: [PATCH 0/4] Improve elfutils diagnostics

[Mark Wielaard]

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

On Wed, May 13, 2015 at 01:31:00PM -0400, Frank Ch. Eigler wrote:
> > One implementation detail that I like to see changed is to make the
> > details errmsg a list of strings instead of one big string (and
> > similarly for dw_tried_paths). [...]
> 
> How would such a list-of-strings be represented?  Considering that
> this is C, and the list arity will vary, we'd be doing dynamic
> allocation anyway, right?

Less allocation is better than more, especially if there are situations
where the extra allocations might not be used anyway.

Note, I am mostly concerned by the asprintf string concats. Those
seem to be done for two reasons that I like to see split out.
They concat because they want to represent the errno (string) and
an extra argument (e.g. file not found plus file name). In which case
I think it is better to store that as two separate items, the errno
and the detail, so no extra string needs to be constructed, unless
the user want to. And to add compound failures together. In that case
I think an array or list of failures (errno plus detail pairs) would
be better.

So the ideal way to represent it might be an array plus length of
pairs typedef struct Dwfl_ErrDetail { int errno, const char *detail }.
Then the array can be preallocated and realloced when necessary.

Cheers,

Mark

Re: [PATCH 0/4] Improve elfutils diagnostics

[Jonathan Lebon]

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

Hello Mark,

> > The first two patches enable elfutils to give a more specific error when
> > compressed sections fail to be decompressed.
> 
> These look good and useful.
> I added ChangeLog entries and pushed them to master.

Thanks!

> I am still pondering these two patches. The idea is sane. Some
> operations are compound and a simple error code/string only tells you
> something failed, but not really why or what was done. There are two or
> three dwfl functions that would benefit from being able to report more
> clearly what went wrong. dwfl_module_getdwarf () which you address in
> patch 4. But also dwfl_module_getelf () and maybe dwfl_module_getsymtab
> (). Which all do some try-fail-try-something-else lookups.

Right, I only added support for getdwarf(). Once we settle on an interface,
I might have a look at the other ones if I have enough time.

> getdwarf and
> getelf both allow the user to plug in their own lookup callbacks to make
> things things even more interesting.

Yes, these patches only report what the default callbacks do. I guess it
might be good to provide a way for callbacks to write their errors to the
same spot, although I suppose user-provided callbacks also have their own
way of reporting to the user things that went wrong.

> One implementation detail that I like to see changed is to make the
> details errmsg a list of strings instead of one big string (and
> similarly for dw_tried_paths). That makes things a bit cheaper when the
> result isn't actually used. You don't have to concatenate the strings
> beforehand and the user can decide how to use the separate detail
> messages.

E.g. a linked list of char* ?

> Also I wonder if this is mixing two issues. a) provide more information
> (the tried path/file) when something goes wrong and b) providing a
> "chain of errors" to explain why some call really failed when the
> operation was really a compounded search result. So do we really need
> just a string (or a list of strings)? Or do we need two separate things.
> A way to provide extra (dynamic) detail when signaling a dwfl error. And
> a way to chain multiple errors in case a function needs to report
> multiple failure results when an operation failed to show everything
> that was tried?

I'm not 100% sure what you mean exactly here re. chaining errors. Do you
mean specifically for compound operations, the user should be able to
easily have programmatic access to the different operations that took
place (along with their respective errors)?

E.g. a linked list of struct { char* operation; char* errmsg } ?

Or do you mean that it should be easy in the elfutils codebase to "append"
to the error message rather than having to accumulate the whole message
before doing __libdw_seterrno_details(). In that case, we could get away
with just adding a __libdw_append_details().

> There is also the detail that not all lookups need to be path/file based
> (find_elf and find_debuginfo aren't properly documented unfortunately).
> But the lookups don't need to create the Elf or Dwarf from an actual
> file (name). They can also create them from a file descriptor. Or even
> create an Elf image "out of thin air". Which find_elf might actually do
> by pulling the image from remote memory of the process when it cannot be
> found on disk. And I can imagine a find_debuginfo callback that gets
> debuginfo files from some remote server.

Right, the actual usage of the dynamic error message capabilities is of
course on a case-by-case basis. What I added in patch 4 was just to
address the common case. But we can use this for other types of errors in
other places.

Thanks,

Jonathan

Re: [PATCH 0/4] Improve elfutils diagnostics

[Frank Ch. Eigler]

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

Hi, Mark -

> [...]  I am still pondering these two patches. The idea is
> sane. Some operations are compound and a simple error code/string
> only tells you something failed, but not really why or what was
> done. [...]

Yup.

> One implementation detail that I like to see changed is to make the
> details errmsg a list of strings instead of one big string (and
> similarly for dw_tried_paths). [...]

How would such a list-of-strings be represented?  Considering that
this is C, and the list arity will vary, we'd be doing dynamic
allocation anyway, right?


> Also I wonder if this is mixing two issues. a) provide more
> information (the tried path/file) when something goes wrong and b)
> providing a "chain of errors" to explain why some call really failed
> when the operation was really a compounded search result. [...]

An error chaining facility would be great, but if each link is permitted
to carry extra text per chain link (file name or operation attempted),
then it can be built -on top of- this scheme, rather than vice versa.


> There is also the detail that not all lookups need to be path/file
> based [...]  But the lookups don't need to create the Elf or Dwarf
> from an actual file (name). They can also create them from a file
> descriptor.

If errors in these originate from elfutils code, that code could
accumulate verbose error messages too.

> [...]  And I can imagine a find_debuginfo callback that gets
> debuginfo files from some remote server.

If the errors come from outside elfutils code (invoked by a callback),
then perhaps the callbacks could use an elfutils hook to append error
message fragments.  (Or the the app could store those errors
out-of-band, and combine it later with the infra-elfutils messages.) 


- FChE

Re: [PATCH 0/4] Improve elfutils diagnostics

[Mark Wielaard]

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

Hi Jonathan,

On Mon, 2015-05-11 at 15:38 -0400, Jonathan Lebon wrote:
> This patch series attempts to improve elfutils feedback to help guide
> the user towards fixing erroneous situations.

Thanks, this is really helpful.

> The first two patches enable elfutils to give a more specific error when
> compressed sections fail to be decompressed.

These look good and useful.
I added ChangeLog entries and pushed them to master.

> The third patch add the new function dwfl_errmsg_details(), which can be
> used to provide dynamic freeform information to the user to supplement
> the static error message from dwfl_errmsg().
> 
> The fourth patch makes use of this facility to provide the user with all
> the paths that were attempted while looking for the debug file.

I am still pondering these two patches. The idea is sane. Some
operations are compound and a simple error code/string only tells you
something failed, but not really why or what was done. There are two or
three dwfl functions that would benefit from being able to report more
clearly what went wrong. dwfl_module_getdwarf () which you address in
patch 4. But also dwfl_module_getelf () and maybe dwfl_module_getsymtab
(). Which all do some try-fail-try-something-else lookups. getdwarf and
getelf both allow the user to plug in their own lookup callbacks to make
things things even more interesting.

One implementation detail that I like to see changed is to make the
details errmsg a list of strings instead of one big string (and
similarly for dw_tried_paths). That makes things a bit cheaper when the
result isn't actually used. You don't have to concatenate the strings
beforehand and the user can decide how to use the separate detail
messages.

Also I wonder if this is mixing two issues. a) provide more information
(the tried path/file) when something goes wrong and b) providing a
"chain of errors" to explain why some call really failed when the
operation was really a compounded search result. So do we really need
just a string (or a list of strings)? Or do we need two separate things.
A way to provide extra (dynamic) detail when signaling a dwfl error. And
a way to chain multiple errors in case a function needs to report
multiple failure results when an operation failed to show everything
that was tried?

There is also the detail that not all lookups need to be path/file based
(find_elf and find_debuginfo aren't properly documented unfortunately).
But the lookups don't need to create the Elf or Dwarf from an actual
file (name). They can also create them from a file descriptor. Or even
create an Elf image "out of thin air". Which find_elf might actually do
by pulling the image from remote memory of the process when it cannot be
found on disk. And I can imagine a find_debuginfo callback that gets
debuginfo files from some remote server.

Cheers,

Mark

[PATCH 0/4] Improve elfutils diagnostics

[Jonathan Lebon]

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

Hi,

This patch series attempts to improve elfutils feedback to help guide
the user towards fixing erroneous situations.

The first two patches enable elfutils to give a more specific error when
compressed sections fail to be decompressed.

The third patch add the new function dwfl_errmsg_details(), which can be
used to provide dynamic freeform information to the user to supplement
the static error message from dwfl_errmsg().

The fourth patch makes use of this facility to provide the user with all
the paths that were attempted while looking for the debug file.

Suggestions and comments welcome!

Related: RHBZ507682, RHBZ1184245.

Jonathan Lebon (4):
  dwarf_begin_elf: decouple section searching from reading
  dwarf_begin_elf: new error for compression failure
  dwfl_error: support error details
  dwfl_module_getdwarf: report paths tried

 libdw/dwarf_begin_elf.c          | 195 ++++++++++++++++++++++-----------------
 libdw/dwarf_error.c              |   1 +
 libdw/libdw.map                  |   1 +
 libdw/libdwP.h                   |   1 +
 libdwfl/dwfl_build_id_find_elf.c |  21 +++++
 libdwfl/dwfl_error.c             |  27 ++++++
 libdwfl/dwfl_module.c            |   3 +
 libdwfl/dwfl_module_getdwarf.c   |  10 ++
 libdwfl/find-debuginfo.c         | 120 +++++++++++++++++-------
 libdwfl/libdwfl.h                |   3 +
 libdwfl/libdwflP.h               |   3 +
 11 files changed, 269 insertions(+), 116 deletions(-)

-- 
2.1.0