Re: eu-stacktrace: roadmap and discussion thread

2023-05-09 Thread Milian Wolff
On Montag, 8. Mai 2023 14:33:57 CEST Serhei Makarov wrote:
> Hello all,
> 
> I wanted to open up public discussion on a project I'm looking to
> develop in elfutils, tentatively named eu-stacktrace. I've started to
> write code on branch users/serhei/eu-stacktrace.
> 
> eu-stacktrace will be a utility to process a stream of raw stack
> samples (such as those obtained from the Linux kernel's
> PERF_SAMPLE_STACK facility) into a stream of stack traces (such as the
> ones obtained from PERF_SAMPLE_CALLCHAIN), freeing various profiling
> utilities from having to implement their own backtracing logic.
> 
> My initial goal is to make the tool work with a (slightly modified)
> version of the sysprof profiler. If all goes well, I hope to produce a
> demonstration of sysprof using elfutils eu-stacktrace and eh_frame
> data to produce useful profiles on code compiled with
> -fomit-frame-pointer. (I'm aware of the problem of profiling
> -fomit-frame-pointer programs being a topic of some fairly contentious
> recent discussion, which I'm not looking to rehash; I'm just
> interested to see if I can add a viable technical solution to the
> mix.) I'm cc:ing chergert and posting a link to this thread on
> GNOME Discourse so that sysprof developers can keep track of the
> discussion.
> 
> For the time being, eu-stacktrace is meant to be fed data from a
> profiling tool via a pipe or fifo. We will see how well this idea
> works as implementation proceeds.
> 
> The eventual goal is to work with various profiler data formats. After
> sysprof, supporting perf's native data format is an obvious
> prerequisite for merging the users/serhei/eu-stacktrace branch into
> elfutils. Ideally, I would like for eu-stacktrace to also convert
> between different profile data formats (e.g. taking sysprof data as
> input and emitting perf data, and vice-versa), but this may be
> out-of-scope given the amount of code that would need to be written to
> handle profile data other than stack traces.



Hey Serhey,

sounds like a fun project. If you want to see some prior art in that area, do 
have a look at perfparser [1], esp. [2], and [3]. We solved quite a few of the 
problems you might encounter in this area. Esp. for good performance, you'll 
need quite a bit of caching on the stacktrace side, such as [4] and [5].

[1]: 
https://codereview.qt-project.org/gitweb?p=qt-creator%2Fperfparser.git;a=summary
[2]: https://codereview.qt-project.org/gitweb?p=qt-creator/
perfparser.git;a=blob;f=app/
perfunwind.cpp;h=9c09740c756beabac43b45ed6f34bbcfc77e0860;hb=refs/heads/master
[3]: https://codereview.qt-project.org/gitweb?p=qt-creator/
perfparser.git;a=blob;f=app/
perfunwind.cpp;h=9c09740c756beabac43b45ed6f34bbcfc77e0860;hb=refs/heads/master
[4]: https://codereview.qt-project.org/gitweb?p=qt-creator/
perfparser.git;a=blob;f=app/
perfaddresscache.cpp;h=8ff26d09a71aff0343378ff062c8fb2fcf601c08;hb=refs/heads/
master
[5]:  https://codereview.qt-project.org/gitweb?p=qt-creator/
perfparser.git;a=blob;f=app/
perfdwarfdiecache.cpp;h=10e432e64cdce6c5e9e6a72a49a1cd73ddb9e7ce;hb=refs/
heads/master

Good luck!
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Building Elfutils with Mingw32

2023-09-16 Thread Milian Wolff
On Samstag, 16. September 2023 09:17:44 CEST Ulf Hermann via Elfutils-devel 
wrote:
> Hi,
> 
> I'll answer some questions here:
> > Given that Windows doesn't even use ELF why would you even want elfutils
> > on
> > such a platform?
> 
> There is a large number of developers who build software for ELF-based
> embedded systems on windows and test/debug/profile/deploy it over some
> network or USB connection. Those people want to have elfutils for their
> debugging and profiling. They want to use elfutils on the host (the
> windows box) because that is much faster.
> 
> Those people are not me and I'm not talking about sanity.
> 
> Qt Creator (the Qt IDE) has a profiling view that takes data produced by
> perf and paints pretty graphs from that. The actual perf data is
> recorded on the target (potentially an embedded device), then streamed
> or copied to the host, then run through perfparser to produce meaningful
> stack traces. perfparser uses elfutils for the actual work. That work is
> quite heavy. A sample frequency of 1000Hz is not uncommon. Few embedded
> device can do that "on the side" while still producing meaningful
> results for the actual task. Finally, Qt Creator takes the processed
> data and displays a UI to explore it. There is another UI for the same
> data, "Hotspot" by Milian, but I don't know if that can run on windows.

In theory it could - but elfutils doesn't officially support Windows which 
blocks this effort. I would love to see this change.

One potential approach that should actually be workable is using heaptrack via 
WSL. Then no porting work is needed on the elfutils side - not an option 
probably for QtCreator, but it would be "good enough" for my purposes I think.

> > And why aren't people simply using cygwin for such a port?
> 
> If we built perfparser with cygwin, the people who use it would also
> need cygwin. People building embedded devices generally have a colorful
> mess of different tools, none of which you can rely on in advance. I
> haven't considered shipping cygwin with perfparser. Is that actually
> possible? It looks like it needs some installation procedure I would
> have to burden the user with.
> 
> > Without it you don't even have a normal POSIX like system.
> 
> Indeed, but we have gnulib and the various adapters I wrote myself (some
> of which should probably be upstreamed to gnulib). That deals with the
> problem. We get binaries that only depend on a basic windows system.
> 
> This comes at the price of using the most basic C library that Microsoft
> offers, msvcrt.dll. In order to use the elfutils libraries built that
> way you have to jump through the open/close and malloc/free hoops we
> provide in eu_compat.lib. However, that is only a problem for perfparser
> or other software linked against elfutils, not for the user.
> 
> > And when using mingw do people still use a normal gcc compiler (to cross
> > build)? Or is the goal to build with some alternative windows
> > compiler?
> 
> The gold standard would indeed be the ability to build it with Microsoft
> compilers and their associated C libraries. Then we could get  rid of
> the malloc/free and open/close hacks. However, given that Microsoft
> compilers have a rather different idea of C than elfutils, I didn't go
> there. Building it with gcc is more overhead for me when producing the
> binary. I cannot cleanly integrate it into the Qt Creator build system
> since that assumes it can use the same compiler for everything. However,
> for the users it doesn't make a difference.

Another option could be to use clang, which is working very well on Windows 
too. Not perfect for QtCreator, but in theory you could build elfutils with 
clang and provide it externally.

> > But if there is consensus (among the Windows hackers) about using one
> > common target for the port then maybe we should have an official
> > branch on sourceware?
> 
> Such a thing would certainly be welcome from my point of view!

I also think so. In general, it's quite sad to see that the code used here is 
so platform dependent on such low levels.

As a wild idea: I know that C++ is used at least in the debuginfod code base - 
could we use that in more places outside of that? That would at least easily 
allow us to  access the filesystem in a platform agnostic way. At work I work 
on software that supports macos, windows and linux, and very rarely if ever do 
I have to consciously  think about these targets, thanks to C++ (and Qt).

> > Also there is a mingw container setup on builder.sourceware.org which
> > we might use for doing CI on the port?
> > https://sourceware.org/cgit/builder/tree/builder/containers/C

Optimizing elfutils usage for unwinding

2024-02-01 Thread Milian Wolff
Hey all,

I'm working on perfparser/hotspot which ingests perf.data files and does 
unwinding and symbolication etc.

We got a bug report by a user [1] with a worst-case performance situation in 
usage of elfutils, which I do not know how to handle - thus me reaching out to 
you all here.

The problem is that the perf.data file for the workload there contains samples 
for tens of thousands of short lived processes - but overall there are only 
about a hundred _different_ binaries being executed.

When we analyze the data, we currently have one dwfl* per process. We already 
employ excessive caching on the symbolication end, which allows us to only 
look at inline frames and demangling once per executable or library, instead 
of once per process.

But this kind of caching across dwfl* is not possible for what 
dwfl_thread_getframes does internally. Profiling our analysis, I see that most 
of the time is spent by this stack:

```
dwfl_thread_getframes
__libdwfl_frame_unwind
handle_cfi
dwarf_cfi_addrframe
__libdw_find_fde
intern_fde
```

Another big chunk is then later on when `dwfl_end` cleans up the modules and 
we get into `__libdw_destroy_frame_cache`.

So, there already seems to be a cache of sorts being build - but it's tied to 
the `dwfl*` structure. In our case we have tens of thousands of these 
structures, each very short lived (as the processes underneath are 
shortlived).

I understand that each process will have its own custom address space mapping, 
but is the CFI/FDE data also tied to such process-specific data? Or could it 
in theory be reuses across `dwfl*` instances?

Thanks

[1]: https://github.com/KDAB/hotspot/issues/394
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH 0/9 v2] Fix thread-safety for elfutils

2024-07-18 Thread Milian Wolff
On Donnerstag, 18. Juli 2024 00:33:59 MESZ Aaron Merey wrote:
> v1 can be found at
> https://sourceware.org/pipermail/elfutils-devel/2023q3/006329.html
> 
> Heather McIntyre is the original author of v1 of these patches.
> Heather and myself co-wrote v2.

Hey you all,

this sounds very promising! I have some high level questions:

Could you please add some documentation on the thread safety guarantees when 
elfutils was compiled with `--enable-thread-safety`? For my purposes in 
maintaining profilers (heaptrack, hotspot), it would be extremely cool if we 
could eventually use multiple threads to analyze separate samples. In theory, 
I would hope that "write" operations (such as `dwfl_report_{begin,end,elf}` 
will require synchronization, but subsequent "read" operations (such as those 
needed for symbolication 
(`dwfl_{addrmodule,module_getsymtab,module_getsym_info,module_nextcu,module_info,module_address_section,
 
get_debuginfod_client,...`, 
`dwarf_{tag,getscopes_die,formstring,fromudata,diename,getsrc_die,dieoffset,linesrc,lineno,getsrcfiles,attr_integrate,...`)
 
or unwinding (`dwfl_{thread_state_registers,frame_pc,getthread_frames}) could 
happen in parallel. Is that the case already after this patch set?

Generally it seems like the documentation could see some love in better 
explaining this feature.

How can I query at runtime whether elfutils was compiled with thread safety 
guarantees or not, such that I can adapt my consumer side accordingly (i.e. 
enable multi-threading or not)?

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH 0/9 v2] Fix thread-safety for elfutils

2024-07-18 Thread Milian Wolff
On Donnerstag, 18. Juli 2024 17:56:39 MESZ Aaron Merey wrote:
> Hi Milian,
> 
> On Thu, Jul 18, 2024 at 6:14 AM Milian Wolff  wrote:
> > On Donnerstag, 18. Juli 2024 00:33:59 MESZ Aaron Merey wrote:
> > > v1 can be found at
> > > https://sourceware.org/pipermail/elfutils-devel/2023q3/006329.html
> > > 
> > > Heather McIntyre is the original author of v1 of these patches.
> > > Heather and myself co-wrote v2.
> > 
> > Hey you all,
> > 
> > this sounds very promising! I have some high level questions:
> > 
> > Could you please add some documentation on the thread safety guarantees
> > when elfutils was compiled with `--enable-thread-safety`?
> 
> This is definitely needed.  I've been working on adding man pages for
> elfutils public library functions.  We should indicate whether each
> function is MT-Safe.  We should also have man pages for libelf, libdw,
> etc, themselves and these should include thread safety information and
> constraints. I will prioritize this.
> 
> > For my purposes in
> > maintaining profilers (heaptrack, hotspot), it would be extremely cool if
> > we could eventually use multiple threads to analyze separate samples. In
> > theory, I would hope that "write" operations (such as
> > `dwfl_report_{begin,end,elf}` will require synchronization, but
> > subsequent "read" operations (such as those needed for symbolication
> > (`dwfl_{addrmodule,module_getsymtab,module_getsym_info,module_nextcu,modul
> > e_info,module_address_section, get_debuginfod_client,...`,
> > `dwarf_{tag,getscopes_die,formstring,fromudata,diename,getsrc_die,dieoffse
> > t,linesrc,lineno,getsrcfiles,attr_integrate,...`) or unwinding
> > (`dwfl_{thread_state_registers,frame_pc,getthread_frames}) could happen
> > in parallel. Is that the case already after this patch set?
>
> This patch set is not entirely comprehensive.  It focuses on thread safety
> in libelf and libdw (not libdwfl) and there is still at least one data race
> in libelf (see https://sourceware.org/bugzilla/show_bug.cgi?id=31967).
> However what you describe should be possible when this feature is more
> fully implemented.
> 
> In the v1 of this patch set there is a patch which removed the
> "experimental" notice from the --enable-thread-safety configure option.
> I left this patch in v2 but I think that we should defer that patch and
> instead keep thread safety "experimental" until it is more fully tested
> and production ready.

OK, good to know. If you are looking for feedback once this is further along 
and includes libdwfl, then I'm more than happy to play around with it. But I 
guess I'll have to wait until libdwfl is thread safe to some degree.
 
> > Generally it seems like the documentation could see some love in better
> > explaining this feature.
> > 
> > How can I query at runtime whether elfutils was compiled with thread
> > safety
> > guarantees or not, such that I can adapt my consumer side accordingly
> > (i.e.
> > enable multi-threading or not)?
> 
> libelf is compiled with pthread only when elfutils is built with
> the --enable-thread-safety configure option.  So ATM I believe that
> checking the libelf binary for pthread_rwlock_* symbols with
> {eu-}readelf or another tool is your best option.
> 
> I should also mention that libdw is built with pthread unconditionally,
> whether or not --enable-thread-safety is given.  However libelf and
> libdw should always be installed and upgraded in lockstep, so if
> libelf contains pthread_rwlock_* symbols then it's reasonable to
> assume that libdw was also built with --enable-thread-safety.
> 
> However this is not ideal.  Maybe we should add some way to verify
> thread safety to the public library API.

Yes please. Looking up `pthread_rwlock_*` symbols in a lib linking against 
libdw just to figure out if things are threadsafe or not is super brittle. 
Please add an explicit API that forwards the `--enable-thread-safety` compile 
time flag such that we can query it at runtime.

thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


dwfl_module_addrinfo and @plt entries

2017-01-03 Thread Milian Wolff
Hello,

how do I get symbol information for @plt entries? Consider the following case:

~~~
$ objdump -j .plt -S lab_mandelbrot | head

lab_mandelbrot: file format elf64-x86-64


Disassembly of section .plt:

2aa0 <_ZN7QWidget4showEv@plt-0x10>:
2aa0:   ff 35 62 35 20 00   pushq  0x203562(%rip)# 206008 
<_GLOBAL_OFFSET_TABLE_+0x8>
2aa6:   ff 25 64 35 20 00   jmpq   *0x203564(%rip)# 206010 
<_GLOBAL_OFFSET_TABLE_+0x10>
2aac:   0f 1f 40 00 nopl   0x0(%rax)
~~~

Now I report dwfl the above binary at address 0x56360eaff000. Then I try to 
get information about the address 0x56360EB01AA0 (i.e. at offset 0x2aa0, 
corresponding to the @plt entry above). dwfl_module_addrinfo returns a NULL 
string, and offset equals the input address.

So, how do I use the dwfl API to also get sym names for @plt entries like in 
the case above?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: dwfl_module_addrinfo and @plt entries

2017-01-06 Thread Milian Wolff
On Wednesday, January 4, 2017 2:42:23 PM CET Mark Wielaard wrote:
> On Wed, Jan 04, 2017 at 01:41:26AM +0100, Milian Wolff wrote:
> > how do I get symbol information for @plt entries?
> 
> Short answer. You cannot with the dwfl_module_getsym/addr*
> functions. And we don't have accessors for "fake" symbols
> (yet). Sorry.

Thanks for the clarification Mark.

> Longer answer. An address pointing into the PLT does
> really point to an ELF symbol.

You mean: does _not_
Right?

> The PLT/GOT contains entries
> that are architecture specific "jump targets" that contain
> (self-modifying) code/data on first access. A PLT entry
> is code that sets up the correct (absolute) address of
> a function on first access. And on second access it fetches
> that function address and jumps to it. Since this is
> architecture specific we would need a backend function
> that translates an address pointing into the PLT into
> an actual function address. You would then be able to
> fetch the actual ELF symbol that address is associated
> with.
> 
> If we have such a backend function then we could even
> do what BFD apparently does. Which is to then create a
> "fake" symbol with as name real_function@plt. But I am
> not sure such fake symbols are very useful (and will
> quickly become confusing since they aren't real ELF
> symbols).

So the objdump command I used is leveraging BFD internally to give me the @plt 
names? I noticed that I also see @plt in perf, which is also probably using 
BFD internally. That at least clarifies why it works in some tools but not in 
when using dwfl.

> Hope that helps. And maybe inspires someone (you?) to
> write up such a backend function and corresponding
> dwfl frontend function.

It does help, thanks. I'm interested in contributing such functionality, but, 
sadly, I'm not sure when I'll get the time to actually do it.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de


resolving kernel addresses to symbol names via /proc/kallsyms

2017-01-31 Thread Milian Wolff
Hey all,

should I be able to resolve kernel addresses to symbol names with elfutils 
libdwfl? I did something like this:

m_dwfl = dwfl_begin(m_callbacks);
auto i_k = dwfl_linux_kernel_report_kernel(m_dwfl);
auto i_m = dwfl_linux_kernel_report_modules(m_dwfl);

Both i_k and i_m returned 0. Then, I picked a random address from my /proc/
kallsyms, e.g.:

81001000 T hypercall_page

and try to find that symbol:

auto module = dwfl_addrmodule(m_dwfl, 0x81001000ll);

The module is non-null and points me at this module (comparing to output I get 
from dwfl_getmodules):

0x6140fa40 kernel 8100

But when I try to get a symbol, I get a nullptr:

auto sym = dwfl_module_addrname(module, 0x81001000ll);

So, what am I missing?

Also, it doesn't seem to be possible to specify the path to the kallsyms file 
with the current dwfl_linux_kernel_report_kernel API - how is this intended to 
be handled? Should one instead use dwfl_linux_kernel_report_offline when one 
wants to support cross-machine analysis?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de


dwfl_attach_state alternative taking Ebl?

2017-03-29 Thread Milian Wolff
Hey all,

would you be willing to accept a patch that adds an alternative to 
dwfl_attach_state taking an Ebl pointer instead of the Elf* to guess the 
architecture from?

This would simplify the code for cases where we know the architecture, but 
don't necessarily have a corresponding Elf* at hand (yet). If there'd be a 
version that takes the Ebl pointer directly, one could use e.g. 
ebl_openbackend_emulation or _machine to find the backend handle for the known 
target architecture directly.

If this would be acceptable, I will write a patch up for this. I would need 
suggestions for a fitting name though, in C++ I'd simply use an overload but 
here with plain C... ;-)

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: dwfl_attach_state alternative taking Ebl?

2017-03-29 Thread Milian Wolff
On Mittwoch, 29. März 2017 21:48:08 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Wed, 2017-03-29 at 16:50 +0200, Milian Wolff wrote:
> > would you be willing to accept a patch that adds an alternative to
> > dwfl_attach_state taking an Ebl pointer instead of the Elf* to guess the
> > architecture from?
> 
> I rather not expose an Ebl handle to the public interface.
> It is really an internal interface that we can change anytime.
> There is also no way a user can create or get at an Ebl handle using the
> public interfaces.

Ah right, I only looked at the implementation of dwfl_attach_state and related 
sources without reading the comment saying that Ebl is internal. No-go then.

> > This would simplify the code for cases where we know the architecture, but
> > don't necessarily have a corresponding Elf* at hand (yet). If there'd be a
> > version that takes the Ebl pointer directly, one could use e.g.
> > ebl_openbackend_emulation or _machine to find the backend handle for the
> > known target architecture directly.
> > 
> > If this would be acceptable, I will write a patch up for this. I would
> > need
> > suggestions for a fitting name though, in C++ I'd simply use an overload
> > but here with plain C... ;-)
> 
> Would it help your use case if there was a dwfl_init_state (Dwfl *dwfl,
> int e_machine, unsigned char ei_class, unsigned char ei_data, ...)?

What magic values do I pass to e_machine, ei_class, ei_data? I guess the ebl 
API that takes the Elf architecture or archicture name would be better.

> And what exactly is your use case? Maybe we can come up with a better
> interface.

The use-case is parsing profiler data, e.g. in perfparser by Ulf / TQC. We 
don't mess with Elf* anywhere, but need it to let dwfl_attach_state figure out 
the target architecture. We do know the architecture already so this is a lot 
of jumping through hoops, to find a fitting Elf* that can be used for dwfl 
then...

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: dwfl_attach_state alternative taking Ebl?

2017-03-30 Thread Milian Wolff
On Thursday, March 30, 2017 12:57:33 PM CEST Mark Wielaard wrote:
> On Wed, 2017-03-29 at 23:57 +0200, Milian Wolff wrote:
> > On Mittwoch, 29. März 2017 21:48:08 CEST Mark Wielaard wrote:
> > > Would it help your use case if there was a dwfl_init_state (Dwfl *dwfl,
> > > int e_machine, unsigned char ei_class, unsigned char ei_data, ...)?
> > 
> > What magic values do I pass to e_machine, ei_class, ei_data?
> 
> That would be one of the EM e_machine architecture constants, ELFCLASS32
> or ELFCLASS64 for ei_class and ELFDATA2LSB or ELFDATA2MSB for ei_data.
> (e_machine could arguably be a GElf_Half).
> 
> > I guess the ebl  API that takes the Elf architecture or archicture
> > name would be better.
> 
> I think we should extend the ebl_openbackend calls with a variant that
> takes all three machine/class/data constants. If you look at the
> machines table in libebl/eblopenbackend.c you see that given just the EM
> architecture constant or (emulation) name without an Elf handle given we
> cannot distinguish between e.g. ppc64 (EM_PPC64/ELFCLASS64/ELFDATA2MSB)
> and ppc64le (EM_PPC64/ELFCLASS64/ELFDATA2LSB). You may obviously counter
> that just means that table isn't complete. But then we have to document
> (and maybe export?) the emulation names that people can rely on. Which
> is why I was suggesting we rely on the machine/class/data triple to
> uniquely identify the architecture. Maybe that is inconvenient though?

Ah ok, such a triple would be OK as well, I guess. But see below.

> > > And what exactly is your use case? Maybe we can come up with a better
> > > interface.
> > 
> > The use-case is parsing profiler data, e.g. in perfparser by Ulf / TQC. We
> > don't mess with Elf* anywhere, but need it to let dwfl_attach_state figure
> > out the target architecture. We do know the architecture already so this
> > is a lot of jumping through hoops, to find a fitting Elf* that can be
> > used for dwfl then...
> 
> OK. How do you know the Elf architecture in that case? How and by what
> is it given? Is that an EM constant or some architecture string?

In our case we either get it from perf, or the user specifies it directly on 
the command line. In both cases it is a string like "x86_64", "arm" or 
"aarch64". We map that to a list of architectures we know about, see e.g.:

http://code.qt.io/cgit/qt-creator/perfparser.git/tree/app/
perfregisterinfo.h#n29

So, any API that would allow us to map these architectures directly to a dwfl/
elf counterpart would simplify our code, or at least would make it easier to 
understand, as we wouldn't have to wait for an Elf file we can open before 
calling dwfl_attach_state.

Bye

-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: frame unwinding patches

2017-04-03 Thread Milian Wolff
On Thursday, February 16, 2017 12:33:27 AM CEST Mark Wielaard wrote:
> Hi,
> 
> I put all three frame pointer unwinding fallback patches on
> the mjw/fp-unwind branch. I'll also sent them to the list using
> git send-mail --annotate taking out the binary file patches.
> Hopefully that will make them appear on the list, bypassing the
> spam filters.
> 
> [PATCH 1/3] Add frame pointer unwinding as fallback on x86_64
> [PATCH 2/3] Add frame pointer unwinding as fallback on arm
> [PATCH 3/3] Add frame pointer unwinding for aarch64
> 
> I had to hand apply a few things because of whitespace adjustments.
> Hopefully I did it right and this is how Ulf intended the patches.
> If not, my apologies, and please let me know what changes you did
> intend.

Ping? Any progress on merging this functionality upstream? It can make quite a 
difference in unwinding.

I just got a report from a colleague. As-is, elfutils would fail to unwind 
from the following location in his application:

0x1137ca4

With the x86_64 patch applied, he got a proper backtrace:

0x1137ca4
0xc45466
0xc4fffa
0xda76f3
0xd78181
0xd8069c
0xd846bd
0xd995b7
QOpenGLFunctions::glTexImage2D qopenglfunctions.h:1022
... 

So, please consider reviewing and merging this.

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: frame unwinding patches

2017-04-04 Thread Milian Wolff
On Monday, April 3, 2017 11:23:25 PM CEST Jan Kratochvil wrote:
> On Mon, 03 Apr 2017 11:00:03 +0200, Milian Wolff wrote:
> > I just got a report from a colleague. As-is, elfutils would fail to unwind
> > from the following location in his application:
> > 
> > 0x1137ca4
> 
> > With the x86_64 patch applied, he got a proper backtrace:
>
> S/he has something wrong with the compiler.  With
> -fasynchronous-unwind-tables frame pointer unwinding is never needed
> and gcc defaults to -fasynchronous-unwind-tables on x86_64.
> 
> This is why I haven't implemented it originally as it only paper overs the
> real problem and it leads to unreliable backtraces in longterm.

Please reconsider:

- In the example above, the address points into libnvidia-glcore.so and as 
such not compiled by my colleague but rather provided by NVidia as a binary 
blob. When you only got a binary blob and have to make do with it, you cannot 
tell people to "just fix the compiler invocation".

- Some JIT compilers, like QV4, actually embed frame pointers into their 
dynamic code, but do not go the extra mile for generating DWARF data or 
asynchronous unwind tables. That is another case where the patches by Ulf 
excel and make elfutils much more useful.

Please, as a user of elfutils I strongly hope that this patch set gets 
accepted. In general, similar patches that make it more resilient in the face 
of broken setups should also at least get considered instead of right-out 
rejected because "something is broken". Things break all the time, but 
developers often have to live with the brokenness.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: dwfl_attach_state alternative taking Ebl?

2017-04-05 Thread Milian Wolff
On Wednesday, April 5, 2017 2:46:34 PM CEST Mark Wielaard wrote:
> On Thu, 2017-03-30 at 13:14 +0200, Milian Wolff wrote:
> > > OK. How do you know the Elf architecture in that case? How and by what
> > > is it given? Is that an EM constant or some architecture string?
> > 
> > In our case we either get it from perf, or the user specifies it directly
> > on the command line. In both cases it is a string like "x86_64", "arm" or
> > "aarch64". We map that to a list of architectures we know about, see
> > e.g.:
> > 
> > http://code.qt.io/cgit/qt-creator/perfparser.git/tree/app/
> > perfregisterinfo.h#n29
> > 
> > So, any API that would allow us to map these architectures directly to a
> > dwfl/ elf counterpart would simplify our code, or at least would make it
> > easier to understand, as we wouldn't have to wait for an Elf file we can
> > open before calling dwfl_attach_state.
> 
> So you map from simple architecture name like "x86" or "powerpc". But
> what mechanism do you have to whether that is 32 or 64 bit, and big or
> little endian?

As Ulf said, we can add the required mappings. So from my side, I guess your 
approach with the three machine/class/data constants would be a good 
improvement already.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de


dwfl_module_addrdie fails for binaries built with clang++

2017-05-04 Thread Milian Wolff
Hey all,

I noticed that elfutils fails to handle clang binaries when we want to find a 
DIE for a certain address. I.e. dwfl_module_addrdie returns nullptr, and eu-
addr2line fails to resolve inlined frames. 

To reproduce this:

~~~
$ cat test.cpp
#include 
#include 
#include 

using namespace std;

int main()
{
uniform_real_distribution uniform(-1E5, 1E5);
default_random_engine engine;
double s = 0;
for (int i = 0; i < 1000; ++i) {
s += uniform(engine);
}
cout << "random sum: " << s << '\n';
return 0;
}
$ clang++ -O2 -g test.cpp -o test --std=c++11
$ objdump -Sl test | grep random.h -A2

  400a10:   48 89 f0mov%rsi,%rax
--
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:1818
operator()(_UniformRandomNumberGenerator& __urng,
   const param_type& __p)
$ addr2line -e test -i 400a10
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:143
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:151
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:332
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.tcc:3332
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:183
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:1818
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/
bits/random.h:1809
/tmp/test.cpp:13
$ eu-addr2line -e test -i 400a10
??:0
~

This also affects us in our perfparser. Not being able to find a cudie means 
not finding inlined frames nor file/line mappings, which is quite a set-back.

I have noticed that backward-cpp contains a (partially) work-around for this:

https://github.com/bombela/backward-cpp/blob/master/backward.hpp#L1216

Is this the right approach and also what the non-eu addr2line does? If so, can 
that be added upstream too, such that dwfl_module_addrdie can be relied on?

I've seen it on clang 3.6, 4 and 5. Neither passing -g3 nor -gdwarf-aranges 
helps.

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: dwfl_module_addrdie fails for binaries built with clang++

2017-05-06 Thread Milian Wolff
On Freitag, 5. Mai 2017 15:06:48 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Thu, 2017-05-04 at 18:05 +0200, Milian Wolff wrote:
> > I noticed that elfutils fails to handle clang binaries when we want to
> > find a DIE for a certain address. I.e. dwfl_module_addrdie returns
> > nullptr, and eu- addr2line fails to resolve inlined frames.
> >
> > To reproduce this:
> >[...]
> >
> > This also affects us in our perfparser. Not being able to find a cudie
> > means not finding inlined frames nor file/line mappings, which is quite a
> > set-back.
> > 
> > I have noticed that backward-cpp contains a (partially) work-around for
> > this:
> > 
> > https://github.com/bombela/backward-cpp/blob/master/backward.hpp#L1216
> 
> O urgh how utterly broken (not backward-cpp, but the bogus DWARF clang
> generates). As that comment says:
> 
> // Sadly clang does not generate the section .debug_aranges,
> thus
> // dwfl_module_addrdie will fail early. Clang doesn't either set
> // the lowpc/highpc/range info for every compilation unit.
> //
> // So in order to save the world:
> // for every compilation unit, we will iterate over every single
> // DIEs. Normally functions should have a lowpc/highpc/range, which
> // we will use to infer the compilation unit.
> 
> // note that this is probably badly inefficient.
> 
> And indeed having to scan through every CU to find a matching function
> DIE is badly inefficient :{

But this code comment is relatively old. Are we sure it's really still the 
case?

> > Is this the right approach and also what the non-eu addr2line does? If so,
> > can that be added upstream too, such that dwfl_module_addrdie can be
> > relied on?
> > 
> > I've seen it on clang 3.6, 4 and 5. Neither passing -g3 nor
> > -gdwarf-aranges
> > helps.
> 
> Thanks for reporting this. I think this might be the same issue seen
> here: https://sourceware.org/bugzilla/show_bug.cgi?id=21247
> ... or at least it seems related. The function/address not found in that
> case also comes from a CU generated by clang. It does have a lowpc and
> ranges, but the lowpc looks bogus (zero) and the ranges don't seem to
> cover the function in question. So it seems even worse than your example
> where there are no lowpc/ranges. We cannot even trust them if they are
> there. Sigh.

So the situation is different from the comment in backward-cpp...

> I have to think about how to handle this. We clearly need something that
> just ignores the lowpc/highpc/ranges on CUs and parses every CU till the
> function/address DIE is found to know which CU and line_table to use.
> But that is so inefficient that I don't want to do that by default.

So, if this is really that bad - what are the binutils doing - does anyone 
know? Also, if it's really against all your expectations, shouldn't we report 
this upstream at clang and ask for input there? I can't believe they knowingly 
break their generated code in such a way. Rather, I believe it's either done 
unknowingly, or there is some alternative way to interpret the data that we 
are not aware of?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


overflows in Dwfl_Thread_Callbacks::memory_read callback

2017-06-01 Thread Milian Wolff
uot;,", abi_cfi=abi_cfi@entry=false, loc=21649, 
find_pc=23177) at /home/milian/projects/src/elfutils/libdw/cfi.c:260
260   register_rule (operand, val_offset, offset);
(rr) list
255   get_uleb128 (operand, program, end);
256   cfi_assert (program < end);
257   get_uleb128 (offset, program, end);
258   offset *= cie->data_alignment_factor;
259 val_offset:
260   register_rule (operand, val_offset, offset);
261   continue;
262
263 case DW_CFA_val_offset_sf:
264   get_uleb128 (operand, program, end);
(rr) print offset
$10 = 
(rr) print val_offset
No symbol "val_offset" in current context.
(rr) print operand
$11 = 
(rr) print operand
$12 = 

~~

So from the above I notice a couple of... "interesting" behavior:

- op->number is an unsigned value, but gets the signed value -16 written to 
it, thereby producing the large value 0xFFF0

- even if the above would be signed, the expression "15 - 16" is always going 
to result in a bogus address

- has anyone a clue of what may be going wrong here? Is one of our callbacks 
returning wrong values before? Is the DWARF info broken? Is there any kind of 
DWARF validator or anything like that which I could use here to check? I 
notice that this issue often arises with frames in libc and ld.so. But, so 
far, I have not come up with a good way to write a MWE that reliably tests 
this behavior... Is there maybe something in the elfutils source code repo 
that I could use as a basis?

If anyone else wants to try to reproduce this, I guess it should also be 
doable with vanilla perf:

- compile it with `make NO_LIBUNWIND=1` to get the libdw based unwinder
- record some data with `perf record --call-graph dwarf`
- inspect the data with `perf script -vvv`, you'll notice a lot of lines of 
the form

unwind: access_mem 0xffd7 not inside range 
0x7ffca0a86aa8-0x7ffca0a88aa8

which is also what we see in our perfparser utility.

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


How to debug broken unwinding?

2017-06-01 Thread Milian Wolff
)
-  78299 _start (/home/milian/projects/compiled/other/bin/
heaptrack_gui)
+ 72499f [unknown] ([unknown])
 
NOTE: a mixture of missing frames in the middle as well as broken backtrace at 
the end

Since this is (sadly) very easy for me to reproduce, I'd be willing to invest 
some time to get this properly fixed. I fail to come up with a way to put this 
into a MWE that does not depend on tons of DSOs on my machine. I fear it's the 
only way to reproduce it though... But since I'm far from an expert in DWARF I 
have no clue on how to even begin to tackle this issue. Would someone more 
involved with this matter accept a `perf archive` to investigate what perf + 
libdw are doing with my DSOs?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to debug broken unwinding?

2017-06-02 Thread Milian Wolff
On Donnerstag, 1. Juni 2017 22:57:12 CEST Milian Wolff wrote:
> Hey all,
> 
> on my ArchLinux box I regularly see cases where libdw fails to unwind
> properly. I can reproduce this both with upstream perf as well as with the
> perfparser utility.
> 
> How should I debug this, or how can I report a good bug report for this? I
> guess I could upload a perf archive and document the steps required to build
> perf with libdw as the unwinder, as it allows to easily compare libunwind
> and libdw for unwinding. When I then diff the output of `perf script` for
> the two unwinders for one and the same perf.data file, I see issues like
> this:
> 
> $ diff -u script.libunwind script.elfutils
> --- script.libunwind2017-06-01 22:30:52.418029474 +0200
> +++ script.elfutils22017-06-01 22:35:10.987823055 +0200
> @@ -510,10 +510,6 @@
> e8ed _dl_fixup (/usr/lib/ld-2.25.so)
>15f06 _dl_runtime_resolve_sse_vex (/usr/lib/ld-2.25.so)
>ed94c KDynamicJobTracker::KDynamicJobTracker
> (/home/milian/ projects/compiled/kf5/lib64/libKF5KIOWidgets.so.5.35.0)
> -  608f3 _GLOBAL__sub_I_kdynamicjobtracker.cpp
> (/home/milian/ projects/compiled/kf5/lib64/libKF5KIOWidgets.so.5.35.0)
> -   f199 call_init.part.0 (/usr/lib/ld-2.25.so)
> -   f2a5 _dl_init (/usr/lib/ld-2.25.so)
> -db9 _dl_start_user (/usr/lib/ld-2.25.so)
> 
> NOTE: it seems as if unwinding through _GLOBAL__sub* always fails?

This part I now investigated with extensive debug output and figured out the 
issue: For the last frame that works in both cases, i.e. ed94c, libdwfl says 
it knows that this address belongs to /usr/lib/ld-2.25.so. In reality, it 
belongs to libKF5KIOWidgets.so.5.35.0.

Previously, perf just checked whether any module is known to libdwfl for a 
given address and then trusted it to do the right thing. Now I created a patch 
that double-checks whether the mapping known to libdwfl matches what perf 
knows. If not, we report the correct map (as known to perf) and this fixes the 
issue.

In general, I believe that libdwfl's API is lacking here. Both perf and 
perfparser know the exact mapping of a file, i.e. the file, it's start and end  
address, as well as the pgoff. But the integration with dwfl simply calls 
dwfl_report_elf, which only takes a start address. For things like ld-2.25.so 
this is often not enough. Is there any chance to expand the API to let us set 
the explicit mapping addresses?

I see there's dwfl_report_module, which at least takes start and end address, 
but so far I always failed to use it for unwinding - it seems as if that 
function is not setting up the internal ELF file and thus all of the functions 
relying on that will break.

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to debug broken unwinding?

2017-06-02 Thread Milian Wolff
On Donnerstag, 1. Juni 2017 22:57:12 CEST Milian Wolff wrote:
> Hey all,



>  heaptrack_gui  2228 135073.400474: 613969 cycles:
>   108c8e [unknown] (/usr/lib/libQt5Core.so.5.8.0)
> @@ -533,8 +529,6 @@
>   2170af [unknown] (/usr/lib/libQt5Core.so.5.8.0)
>   297c53 QCoreApplicationPrivate::init (/usr/lib/
> libQt5Core.so.5.8.0)
>f7cde QGuiApplicationPrivate::init
> (/usr/lib/libQt5Gui.so. 5.8.0)
> - 1589e8 QApplicationPrivate::init
> (/usr/lib/libQt5Widgets.so. 5.8.0)
> -  78622 main (/home/milian/projects/compiled/other/bin/
> heaptrack_gui)
>20439 __libc_start_main (/usr/lib/libc-2.25.so)
>78299 _start (/home/milian/projects/compiled/other/bin/
> heaptrack_gui)
> 
> NOTE: this is super odd, it simply misses two frames in the middle?!

This is really quite odd - looking at the debug output, the frames in the 
middle are really just skipped for some reason:

unwind: access_mem addr 0x7ffca0a88330, val 4edc50, offset 2808
unwind: access_mem addr 0x7ffca0a88338, val 7f69bfce443a, offset 2816
unwind: pc: = 0x7f69c10fecde
found map: 7f69c1007000 7f69c1766000
dso found: libQt5Gui.so.5.8.0 /usr/lib/libQt5Gui.so.5.8.0
reported: libQt5Gui.so.5.8.0 /usr/lib/libQt5Gui.so.5.8.0, 1
unwind: QGuiApplicationPrivate::init():ip = 0x7f69c10fecde (0xf7cde)

-> so far so good, this frame is properly found inside libQt5Gui, but then:

unwind: pc: = 0x7f69bfce4439
found map: 7f69bfcc4000 7f69c0069000
dso found: libc-2.25.so /usr/lib/libc-2.25.so
reported: libc-2.25.so /usr/lib/libc-2.25.so, 1
unwind: __libc_start_main:ip = 0x7f69bfce4439 (0x20439)

-> the next frame is is supposedly the one in libc, but what happened to the 
two frames in QApplicationPrivate::init and main? I also note that no calls to 
access_mem are occuring - is this maybe some (wrong) caching in libdw or so 
that interfers here?

Any insight would be appreciated, thanks!
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to debug broken unwinding?

2017-06-02 Thread Milian Wolff
On Freitag, 2. Juni 2017 15:26:10 CEST Milian Wolff wrote:
> On Donnerstag, 1. Juni 2017 22:57:12 CEST Milian Wolff wrote:
> > Hey all,
> 
> 
> 
> >  heaptrack_gui  2228 135073.400474: 613969 cycles:
> >   108c8e [unknown] (/usr/lib/libQt5Core.so.5.8.0)
> > 
> > @@ -533,8 +529,6 @@
> > 
> >   2170af [unknown] (/usr/lib/libQt5Core.so.5.8.0)
> >   297c53 QCoreApplicationPrivate::init (/usr/lib/
> > 
> > libQt5Core.so.5.8.0)
> > 
> >f7cde QGuiApplicationPrivate::init
> > 
> > (/usr/lib/libQt5Gui.so. 5.8.0)
> > - 1589e8 QApplicationPrivate::init
> > (/usr/lib/libQt5Widgets.so. 5.8.0)
> > -  78622 main (/home/milian/projects/compiled/other/bin/
> > heaptrack_gui)
> > 
> >20439 __libc_start_main (/usr/lib/libc-2.25.so)
> >78299 _start (/home/milian/projects/compiled/other/bin/
> > 
> > heaptrack_gui)
> > 
> > NOTE: this is super odd, it simply misses two frames in the middle?!
> 
> This is really quite odd - looking at the debug output, the frames in the
> middle are really just skipped for some reason:
> 
> unwind: access_mem addr 0x7ffca0a88330, val 4edc50, offset 2808
> unwind: access_mem addr 0x7ffca0a88338, val 7f69bfce443a, offset 2816
> unwind: pc: = 0x7f69c10fecde
> found map: 7f69c1007000 7f69c1766000
> dso found: libQt5Gui.so.5.8.0 /usr/lib/libQt5Gui.so.5.8.0
> reported: libQt5Gui.so.5.8.0 /usr/lib/libQt5Gui.so.5.8.0, 1
> unwind: QGuiApplicationPrivate::init():ip = 0x7f69c10fecde (0xf7cde)
> 
> -> so far so good, this frame is properly found inside libQt5Gui, but then:
> 
> unwind: pc: = 0x7f69bfce4439
> found map: 7f69bfcc4000 7f69c0069000
> dso found: libc-2.25.so /usr/lib/libc-2.25.so
> reported: libc-2.25.so /usr/lib/libc-2.25.so, 1
> unwind: __libc_start_main:ip = 0x7f69bfce4439 (0x20439)
> 
> -> the next frame is is supposedly the one in libc, but what happened to the
> two frames in QApplicationPrivate::init and main? I also note that no calls
> to access_mem are occuring - is this maybe some (wrong) caching in libdw or
> so that interfers here?
> 
> Any insight would be appreciated, thanks!

Some more debugging and going after my gut feeling brings me to the following 
conclusion: The real issue seems to be the on-demand reporting of the elf 
file. We used to do:

   Dwarf_Addr pc;
   bool isactivation;
 
   if (!dwfl_frame_pc(state, &pc, &isactivation)) {
   pr_err("%s", dwfl_errmsg(-1));
   return DWARF_CB_ABORT;
   }

   // report the module before we query for isactivation
   report_module(pc, ui);

This looks safe and fine and actually works most of the time. But passing a 
non-null isactivation flag to dwfl_frame_pc potentially leads to a second 
unwind step, before we got the change to report the module! I can workaround 
this by instead doing

   Dwarf_Addr pc;
   bool isactivation;
 
   if (!dwfl_frame_pc(state, &pc, NULL)) {
   pr_err("%s", dwfl_errmsg(-1));
   return DWARF_CB_ABORT;
   }

   // report the module before we query for isactivation
   report_module(pc, ui);

   if (!dwfl_frame_pc(state, &pc, &isactivation)) {
   pr_err("%s", dwfl_errmsg(-1));
   return DWARF_CB_ABORT;
   }

This fixes all the issues in my original email. So sorry for the noise - it 
doesn't see as if the unwinding code in elfutils is broken - quite the 
contrary! It's just our misuse of the API that is to blame.

May I suggest though to move the isactivation code into a separate function to 
prevent this issue from arising in the future? I.e. it would be nice if the 
code above could read:


   Dwarf_Addr pc;
   bool isactivation;
 
   if (!dwfl_frame_pc(state, &pc)) {
   pr_err("%s", dwfl_errmsg(-1));
   return DWARF_CB_ABORT;
   }

   // report the module before we query for isactivation
   report_module(pc, ui);

   if (!dwfl_frame_is_activation(state)) {
   --pc; 
   }


Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to debug broken unwinding?

2017-06-13 Thread Milian Wolff
On Dienstag, 13. Juni 2017 16:20:25 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Thu, 2017-06-01 at 22:57 +0200, Milian Wolff wrote:
> > How should I debug this, or how can I report a good bug report for this? I
> > guess I could upload a perf archive and document the steps required to
> > build perf with libdw as the unwinder, as it allows to easily compare
> > libunwind and libdw for unwinding.
> 
> I am reading the rest of you messages in this thread now and it seems
> you did find the issue already. But the above would still be useful if
> you happen to have the instructions already written down. I was actually
> looking at perf recently and couldn't immediately figure out how to
> build it. It seems it depends on a whole kernel build. But if there is
> an easy way to just build perf (with a variant that uses libunwind and
> libdw) that would be useful in general I think.

Perf is, you could say, a two-fold software.

On one hand you have the kernel side of things, which does the PMU 
configuration and actual sampling and data recording.

One the other hand you have a bunch of userspace tooling that tells the kernel 
side to start recording, but - more importantly - to analyse the recorded 
data.

The first part in the kernel is pretty stable - no need to compile your own 
kernel. Esp. on a recent distro this is not required.

For the second side, compiling perf's userland is pretty easy to get the 
latest and greatest features. And it also allows us to compare libunwind vs. 
elfutils:

git clone git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux \
-b perf/core
cd linux/tools/perf
make # will output what dependencies where found
cp perf perf-libunwind
make NO_LIBUNWIND=1
cp perf perf-elfutils

# record data with either version
./perf record --call-graph dwarf 

Then you can use different "perf" versions to compare the unwinders. I usually 
diff the output of `perf script` or `perf report`, e.g.:

./perf-libunwind stat --inline > libunwind.txt
./perf-elfutils stat --inline > elfutils.txt
diff -u libunwind.txt elfutils.txt

Hope this helps!

> More in general debugging the unwinder is indeed too hard. There are
> some failures that are ignored because it isn't immediately clear
> whether they are fatal. e.g. you might be unable to recover some
> register value that then isn't ever used for a simple backtrace, or you
> might not find the CFI for a pc address, but want to use the fallback
> unwinder in that case. Maybe we should add a debug mode configuration
> that compiles in tracing output of the unwinder operations?

That would be helpful, I guess. Most notably for laymen like me who still have 
trouble understanding what exactly is going on here. If there would be some 
magic flag I could use (like libunwind's UNW_DEBUG_LEVEL=15) to generate a 
trace I could append to a bug report, that would be good to have!

Bye
-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: overflows in Dwfl_Thread_Callbacks::memory_read callback

2017-06-13 Thread Milian Wolff
On Dienstag, 13. Juni 2017 16:06:01 CEST Mark Wielaard wrote:
> On Thu, 2017-06-01 at 22:46 +0200, Milian Wolff wrote:
> > in the perfparser that Ulf wrote, and to which I'm contributing too, we
> > often see abnormal data being passed to the memory_read callback we
> > define.
> > 
> > I.e. our callback gets invoked with addr=0x which clearly
> > isn't an expected or valid address. So far we ignored this, as things seem
> > to work okayish, but I still suspect something is wrong somewhere... Out
> > of interest, I would like to ask if anyone has ever seen something like
> > that?
> Yes, you sometimes see that some register or location value is zero for
> some reason, but the expression parser wants to read some memory at an
> offset from that register. e.g. the SP being zero and trying to read at
> SP +/- offset to get some value from the stack.
> 
> Maybe the expression parser should notice this situation and not even
> invoke the memory_read callback in that case since it is obvious it will
> fail.

That would be good to do, I guess. Or at least document that this can happen 
and that it is expected behavior. I.e. right now I simply don't know whether 
our code is buggy and triggering this situation, or whether we are 
encountering something "normal" which just is expected to fail and then some 
fallback code will continue to do the right thing.



> I am not following the above trace completely, but what is going on
> seems to be that we have CFI and want to get a register value. So we
> call dwarf_frame_register to determine the DWARF expression operations
> that we need to execute to get the register value. dwarf_frame_register
> determines that that the register is described by a register offset
> value rule, so it generates operations saying see an the CFA
> (DW_OP_call_frame_cfa) plus some offset (DW_OP_plus_uconst) as a value
> (so read the value from cfa + offset, which is somewhere on the stack).
> But then the cfa comes out as 15? That is obviously bogus. But I don't
> really understand how that happened. It should be a value somewhere near
> the current stack. Then reading 15 - offset (16) clearly fails.

Right, so that cfa value - is that in the DWARF info? So is the dso "broken"? 
Or is this some runtime value that we returned earlier in our register or 
memory callback implementation?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: dwfl_module_addrinfo and @plt entries

2017-07-05 Thread Milian Wolff
On Friday, January 6, 2017 8:17:53 PM CEST Mark Wielaard wrote:
> On Fri, Jan 06, 2017 at 11:28:25AM +0100, Milian Wolff wrote:
> > On Wednesday, January 4, 2017 2:42:23 PM CET Mark Wielaard wrote:
> > > Longer answer. An address pointing into the PLT does
> > > really point to an ELF symbol.
> > 
> > You mean: does _not_
> > Right?
> 
> Yes, I meant "does not point".
> 
> > > If we have such a backend function then we could even
> > > do what BFD apparently does. Which is to then create a
> > > "fake" symbol with as name real_function@plt. But I am
> > > not sure such fake symbols are very useful (and will
> > > quickly become confusing since they aren't real ELF
> > > symbols).
> > 
> > So the objdump command I used is leveraging BFD internally to give me the
> > @plt names? I noticed that I also see @plt in perf, which is also
> > probably using BFD internally. That at least clarifies why it works in
> > some tools but not in when using dwfl.
> 
> binutils objdump certainly does.
> 
> > > Hope that helps. And maybe inspires someone (you?) to
> > > write up such a backend function and corresponding
> > > dwfl frontend function.
> > 
> > It does help, thanks. I'm interested in contributing such functionality,
> > but, sadly, I'm not sure when I'll get the time to actually do it.
> 
> Thanks, wish I had spare time myself :)

I have now looked into this issue again and have found a way to workaround 
this limitation outside of elfutils, by manually resolving the address in a 
.plt section to a symbol. See:

https://github.com/KDAB/perfparser/commit/
885f88f3d66904cd94af65f802232f6c6dc339f4

This seems to work in my limited tests (only on X86_64). Beside the 32bit/
64bit difference, it isn't really platform dependent, is it? Or was this what 
you had in mind when you said the elfutils code would be "architecture 
specific [and] we would need a backend function that translates an address 
pointing into the PLT into an actual function address"?

If my code is roughly OK, then I'll try to put it into a patch for elfutils 
and submit it there. If it's fundamentally broken, please tell me. I still 
plan to get this functionality upstream into elfutils.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de


Re: dwfl_module_addrinfo and @plt entries

2017-07-10 Thread Milian Wolff
On Freitag, 7. Juli 2017 13:03:32 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> First congrats on https://www.kdab.com/hotspot-gui-linux-perf-profiler/
> Very cool.
> 
> On Wed, 2017-07-05 at 15:34 +0200, Milian Wolff wrote:
> > On Friday, January 6, 2017 8:17:53 PM CEST Mark Wielaard wrote:
> > I have now looked into this issue again and have found a way to workaround
> > this limitation outside of elfutils, by manually resolving the address in
> > a
> > .plt section to a symbol. See:
> > 
> > https://github.com/KDAB/perfparser/commit/
> > 885f88f3d66904cd94af65f802232f6c6dc339f4
> > 
> > This seems to work in my limited tests (only on X86_64). Beside the 32bit/
> > 64bit difference, it isn't really platform dependent, is it? Or was this
> > what you had in mind when you said the elfutils code would be
> > "architecture specific [and] we would need a backend function that
> > translates an address pointing into the PLT into an actual function
> > address"?
> > 
> > If my code is roughly OK, then I'll try to put it into a patch for
> > elfutils
> > and submit it there. If it's fundamentally broken, please tell me. I still
> > plan to get this functionality upstream into elfutils.
> 
> Thanks for the research. I don't know if the PLT/GOT resolving works
> identical for all architectures. But yes, it does look like what you
> came up with is in general architecture independent.
> 
> In general it would be nice if we could avoid any name based section
> lookups (or only do them as fallbacks) since we might not have section
> headers (for example if you got the ELF image from memory).

Yes, the name comparison is ugly but I don't know any alternative. The sh_type 
is just SHT_PROGBITS afair and I couldn't find anything else to use. From what 
I gathered online, one could even (theoretically) change the name of the 
section and it would still work fine but my mapping would break. That said, at 
least this works for the common case.

> I wonder if we can get all the information needed from the dynamic
> segment. For example it seems we have a DT_JMPREL that points directly
> at the .plt table, DT_PLTREL gives you what kind of relocation entries
> REL or RELA it contains and DT_PLTRELSZ gives the size of the plt
> table.
> 
> In your code you get the GOT address through DT_PLTGOT, but then use
> that address to lookup the .got.plt section and use its sh_addr to index
> into the table. Why is that? Isn't that address equal to what you
> already got through DT_PLTGOT?

Indeed, that is convoluted. I tried to reverse-engineer the code from elf-
dissector, which does this mapping in reverse (no pun intended). Maybe I over-
complicated it. I'll research this when I'm back from vacation in two weeks.

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: dwfl_module_addrinfo and @plt entries

2017-08-28 Thread Milian Wolff
On Monday, July 10, 2017 1:06:27 PM CEST Milian Wolff wrote:
> On Freitag, 7. Juli 2017 13:03:32 CEST Mark Wielaard wrote:
> > Hi Milian,
> > 
> > First congrats on https://www.kdab.com/hotspot-gui-linux-perf-profiler/
> > Very cool.
> > 
> > On Wed, 2017-07-05 at 15:34 +0200, Milian Wolff wrote:
> > > On Friday, January 6, 2017 8:17:53 PM CEST Mark Wielaard wrote:
> > > I have now looked into this issue again and have found a way to
> > > workaround
> > > this limitation outside of elfutils, by manually resolving the address
> > > in
> > > a
> > > .plt section to a symbol. See:
> > > 
> > > https://github.com/KDAB/perfparser/commit/
> > > 885f88f3d66904cd94af65f802232f6c6dc339f4
> > > 
> > > This seems to work in my limited tests (only on X86_64). Beside the
> > > 32bit/
> > > 64bit difference, it isn't really platform dependent, is it? Or was this
> > > what you had in mind when you said the elfutils code would be
> > > "architecture specific [and] we would need a backend function that
> > > translates an address pointing into the PLT into an actual function
> > > address"?
> > > 
> > > If my code is roughly OK, then I'll try to put it into a patch for
> > > elfutils
> > > and submit it there. If it's fundamentally broken, please tell me. I
> > > still
> > > plan to get this functionality upstream into elfutils.
> > 
> > Thanks for the research. I don't know if the PLT/GOT resolving works
> > identical for all architectures. But yes, it does look like what you
> > came up with is in general architecture independent.
> > 
> > In general it would be nice if we could avoid any name based section
> > lookups (or only do them as fallbacks) since we might not have section
> > headers (for example if you got the ELF image from memory).
> 
> Yes, the name comparison is ugly but I don't know any alternative. The
> sh_type is just SHT_PROGBITS afair and I couldn't find anything else to
> use. From what I gathered online, one could even (theoretically) change the
> name of the section and it would still work fine but my mapping would
> break. That said, at least this works for the common case.
> 
> > I wonder if we can get all the information needed from the dynamic
> > segment. For example it seems we have a DT_JMPREL that points directly
> > at the .plt table, DT_PLTREL gives you what kind of relocation entries
> > REL or RELA it contains and DT_PLTRELSZ gives the size of the plt
> > table.
> > 
> > In your code you get the GOT address through DT_PLTGOT, but then use
> > that address to lookup the .got.plt section and use its sh_addr to index
> > into the table. Why is that? Isn't that address equal to what you
> > already got through DT_PLTGOT?
> 
> Indeed, that is convoluted. I tried to reverse-engineer the code from elf-
> dissector, which does this mapping in reverse (no pun intended). Maybe I
> over- complicated it. I'll research this when I'm back from vacation in two
> weeks.

Hey Mark,

more than two weeks passed, but I finally had some time to investigate the 
above. I have a hard time justifying what I wrote, I can only explain what I'm 
seeing. Can you maybe add your comments in the below? I think I'm just missing 
something that you have in your mind to shortcut my code:

- find dynamic segment via SHT_DYNAMIC (1st loop)
- in there, find address of PLTGOT segment via DT_PLTGOT
- find corresponding segment (2nd loop) for PLTGOT
- in there, find address for requested symbol index, offset by two
- ...

I mean, searching the address in the first loop is not a goal per se. What I'm 
looking for is the Scn/Shdr that contains the PLTGOT. The first loop allows me 
to identify the PLTGOT via it's address, but to actually get my hands on the 
corresponding Scn/Shdr I still need the second loop, no? Or can I somehow 
translate the PLTGOT address to a Scn/Shdr directly?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de




How to associate Elf with Dwfl_Module returned by dwfl_report_module

2018-03-17 Thread Milian Wolff
Hey there,

a recurring issue in the libdwfl integration of perf and perfparser are 
supposedly overlapping modules. The perf data file contains the exact mappings 
for all files corresponding to the actual mmap events that occurred during 
runtime, e.g.:

```
$ perf script --show-mmap-events | grep MMAP | grep stdc
heaptrack_print 13962 87163.483450: PERF_RECORD_MMAP2 13962/13962: 
[0x7fd0aea84000(0x387000) @ 0 08:03 413039 3864781083]: r-xp /usr/lib/libstdc+
+.so.6.0.24
heaptrack_print 13962 87163.483454: PERF_RECORD_MMAP2 13962/13962: 
[0x7fd0aebfc000(0x1ff000) @ 0x178000 08:03 413039 3864781083]: ---p /usr/lib/
libstdc++.so.6.0.24
heaptrack_print 13962 87163.483458: PERF_RECORD_MMAP2 13962/13962: 
[0x7fd0aedfb000(0xd000) @ 0x177000 08:03 413039 3864781083]: rw-p /usr/lib/
libstdc++.so.6.0.24
heaptrack_print 13962 87163.484860: PERF_RECORD_MMAP2 13962/13962: 
[0x7fd0aedfb000(0xc000) @ 0x177000 08:03 413039 3864781083]: r--p /usr/lib/
libstdc++.so.6.0.24
```
So far, both perf and perfparser are using dwfl_report_elf to report the file. 
But that API is deducing the mapping addresses internally, which may or may 
not be what we saw at runtime. I suspect that this is the reason for some 
issues we are seeing, such as supposedly overlapping modules. 

Looking at the Dwfl API, I cannot figure out how to feed the mapping directly. 
There's dwfl_report_module, but how would I associate an Elf* and int fd with 
it, as done by dwfl_report_elf?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to associate Elf with Dwfl_Module returned by dwfl_report_module

2018-03-21 Thread Milian Wolff
On Dienstag, 20. März 2018 23:05:49 CET Mark Wielaard wrote:
> Hi Milian,

Hey Mark :)

> On Sat, Mar 17, 2018 at 02:14:48PM +0100, Milian Wolff wrote:
> > a recurring issue in the libdwfl integration of perf and perfparser are
> > supposedly overlapping modules. The perf data file contains the exact
> > mappings for all files corresponding to the actual mmap events that
> > occurred during runtime, e.g.:
> > 
> > ```
> > $ perf script --show-mmap-events | grep MMAP | grep stdc
> > heaptrack_print 13962 87163.483450: PERF_RECORD_MMAP2 13962/13962:
> > [0x7fd0aea84000(0x387000) @ 0 08:03 413039 3864781083]: r-xp
> > /usr/lib/libstdc+ +.so.6.0.24
> > heaptrack_print 13962 87163.483454: PERF_RECORD_MMAP2 13962/13962:
> > [0x7fd0aebfc000(0x1ff000) @ 0x178000 08:03 413039 3864781083]: ---p
> > /usr/lib/ libstdc++.so.6.0.24
> > heaptrack_print 13962 87163.483458: PERF_RECORD_MMAP2 13962/13962:
> > [0x7fd0aedfb000(0xd000) @ 0x177000 08:03 413039 3864781083]: rw-p
> > /usr/lib/
> > libstdc++.so.6.0.24
> > heaptrack_print 13962 87163.484860: PERF_RECORD_MMAP2 13962/13962:
> > [0x7fd0aedfb000(0xc000) @ 0x177000 08:03 413039 3864781083]: r--p
> > /usr/lib/
> > libstdc++.so.6.0.24
> > ```
> > So far, both perf and perfparser are using dwfl_report_elf to report the
> > file. But that API is deducing the mapping addresses internally, which
> > may or may not be what we saw at runtime. I suspect that this is the
> > reason for some issues we are seeing, such as supposedly overlapping
> > modules.
> 
> How exactly are you calling dwfl_report_elf?

Here's the code for the perf tools:

https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/
perf/util/unwind-libdw.c?h=perf/core#n52

Here's the code for the perfparser:

http://code.qt.io/cgit/qt-creator/perfparser.git/tree/app/
perfsymboltable.cpp#n479

Let's concentrate on perf for now, but perfparser has similar logic:

We parse the mmap events in the perf.data file and store that information. 
Note that the perf.data file does not contain events for munmap calls. Then 
while unwinding the callstack of a perf sample, we lookup the most recent mmap 
event for every given instruction pointer address, and ensure that the 
corresponding ELF was registered with libdw.

> Specifically are you using false for the add_p_vaddr argument?

Yes, we are.

> And could you provide some example where the reported address is
> wrong/different from the start address of the Dwfl_Module?

I don't think it's the start address that is wrong, rather it's the end 
address. But it's hard for me to come up with a small selfcontained example at 
this stage. I am regularly seeing broken backtraces for samples where I have 
the gut feeling that missing reported ELFs are to blame. But we report 
everything, except for scenarios where the mmap events seemingly overlap. This 
overlapping is, as far as I can see, actually a side effect of remapping 
taking place in the dynamic linker (i.e. a single dlopen/dynamic linked 
library can yield multiple mmap events). One way or another, we end up with a 
situation where we cannot report an ELF to dwfl due to two issues:

a) either ELF tells us we are overlapping some module and just stops which is 
bad, since we would actually much prefer the newly reported ELF to take 
precedence

b) we find an mmap event that with a non-zero pgoff, and have no clue how to 
call dwfl_report_elf and just give up.

In both cases, I was hopeing for dwfl_report_module to help since it seemingly 
allows me to exactly recreate the mapping that was traced originally.

> > Looking at the Dwfl API, I cannot figure out how to feed the mapping
> > directly. There's dwfl_report_module, but how would I associate an Elf*
> > and int fd with it, as done by dwfl_report_elf?
> 
> When using dwfl_report_module the find_elf callback will be called that
> you registered with dwfl_begin. That callback is called "lazily" the
> first time dwfl_module_getelf is called. The callback can then set the
> Elf*. But that does mean you have to keep track yourself (or immediately
> call dwfl_module_getelf).

Ah, thanks!

> I would like to understand better what is really going wrong with
> dwfl_report_elf before diving into using dwfl_module_report.

See above, I would very much value your input. I'm still far from having fully 
grasped this situation.

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to associate Elf with Dwfl_Module returned by dwfl_report_module

2018-03-21 Thread Milian Wolff
On Mittwoch, 21. März 2018 14:01:41 CET Milian Wolff wrote:
> On Dienstag, 20. März 2018 23:05:49 CET Mark Wielaard wrote:
> > Hi Milian,
> 
> Hey Mark :)
> 
> > On Sat, Mar 17, 2018 at 02:14:48PM +0100, Milian Wolff wrote:
> > > a recurring issue in the libdwfl integration of perf and perfparser are
> > > supposedly overlapping modules. The perf data file contains the exact
> > > mappings for all files corresponding to the actual mmap events that
> > > occurred during runtime, e.g.:
> > > 
> > > ```
> > > $ perf script --show-mmap-events | grep MMAP | grep stdc
> > > heaptrack_print 13962 87163.483450: PERF_RECORD_MMAP2 13962/13962:
> > > [0x7fd0aea84000(0x387000) @ 0 08:03 413039 3864781083]: r-xp
> > > /usr/lib/libstdc+ +.so.6.0.24
> > > heaptrack_print 13962 87163.483454: PERF_RECORD_MMAP2 13962/13962:
> > > [0x7fd0aebfc000(0x1ff000) @ 0x178000 08:03 413039 3864781083]: ---p
> > > /usr/lib/ libstdc++.so.6.0.24
> > > heaptrack_print 13962 87163.483458: PERF_RECORD_MMAP2 13962/13962:
> > > [0x7fd0aedfb000(0xd000) @ 0x177000 08:03 413039 3864781083]: rw-p
> > > /usr/lib/
> > > libstdc++.so.6.0.24
> > > heaptrack_print 13962 87163.484860: PERF_RECORD_MMAP2 13962/13962:
> > > [0x7fd0aedfb000(0xc000) @ 0x177000 08:03 413039 3864781083]: r--p
> > > /usr/lib/
> > > libstdc++.so.6.0.24
> > > ```
> > > So far, both perf and perfparser are using dwfl_report_elf to report the
> > > file. But that API is deducing the mapping addresses internally, which
> > > may or may not be what we saw at runtime. I suspect that this is the
> > > reason for some issues we are seeing, such as supposedly overlapping
> > > modules.
> > 
> > How exactly are you calling dwfl_report_elf?
> 
> Here's the code for the perf tools:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/
> perf/util/unwind-libdw.c?h=perf/core#n52
> 
> Here's the code for the perfparser:
> 
> http://code.qt.io/cgit/qt-creator/perfparser.git/tree/app/
> perfsymboltable.cpp#n479
> 
> Let's concentrate on perf for now, but perfparser has similar logic:
> 
> We parse the mmap events in the perf.data file and store that information.
> Note that the perf.data file does not contain events for munmap calls. Then
> while unwinding the callstack of a perf sample, we lookup the most recent
> mmap event for every given instruction pointer address, and ensure that the
> corresponding ELF was registered with libdw.
> 
> > Specifically are you using false for the add_p_vaddr argument?
> 
> Yes, we are.
> 
> > And could you provide some example where the reported address is
> > wrong/different from the start address of the Dwfl_Module?
> 
> I don't think it's the start address that is wrong, rather it's the end
> address. But it's hard for me to come up with a small selfcontained example
> at this stage. I am regularly seeing broken backtraces for samples where I
> have the gut feeling that missing reported ELFs are to blame. But we report
> everything, except for scenarios where the mmap events seemingly overlap.
> This overlapping is, as far as I can see, actually a side effect of
> remapping taking place in the dynamic linker (i.e. a single dlopen/dynamic
> linked library can yield multiple mmap events). One way or another, we end
> up with a situation where we cannot report an ELF to dwfl due to two
> issues:
> 
> a) either ELF tells us we are overlapping some module and just stops which
> is bad, since we would actually much prefer the newly reported ELF to take
> precedence
> 
> b) we find an mmap event that with a non-zero pgoff, and have no clue how to
> call dwfl_report_elf and just give up.
> 
> In both cases, I was hopeing for dwfl_report_module to help since it
> seemingly allows me to exactly recreate the mapping that was traced
> originally.

Here's one way to investigate where perf and dwfl disagree on the file 
mappings:

diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
index 7bdd239c795c..739c68b73772 100644
--- a/tools/perf/util/unwind-libdw.c
+++ b/tools/perf/util/unwind-libdw.c
@@ -48,11 +48,17 @@ static int __report_module(struct addr_location *al, u64 
ip,
mod = 0;
}
 
-   if (!mod)
+   if (!mod) {
+   Dwarf_Addr s, e;
mod = dwfl_report_elf(ui->dwfl, dso->short_name,
  (dso->symsrc_filename ? 
dso->symsrc_filename : dso-
>long_name), -1, al->map->start,
  false);
 
+

Re: How to associate Elf with Dwfl_Module returned by dwfl_report_module

2018-03-22 Thread Milian Wolff
On Mittwoch, 21. März 2018 22:21:13 CET Mark Wielaard wrote:
> Hi Milian,
> 
> On Wed, Mar 21, 2018 at 02:01:41PM +0100, Milian Wolff wrote:
> > Here's the code for the perf tools:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/
> > perf/util/unwind-libdw.c?h=perf/core#n52
> > 
> > Here's the code for the perfparser:
> > 
> > http://code.qt.io/cgit/qt-creator/perfparser.git/tree/app/
> > perfsymboltable.cpp#n479
> > 
> > Let's concentrate on perf for now, but perfparser has similar logic:
> > 
> > We parse the mmap events in the perf.data file and store that information.
> > Note that the perf.data file does not contain events for munmap calls.
> > Then
> > while unwinding the callstack of a perf sample, we lookup the most recent
> > mmap event for every given instruction pointer address, and ensure that
> > the corresponding ELF was registered with libdw.
> 
> So, modules are never deregistered?
> In that case, that might explain the issue.

No, they are deregistered - that is not the issue. Perf actually starts with a 
clean dwfl on every sample and registers whatever modules are relevant for the 
given sample. perfparser tries to be a bit smarter and caches more, but also 
has code to deregister if something goes amiss.

> But I see there is a check if there is already something at the address.
> The interface to "remove" a module might not be immediately clear.
> The idea is that if modules need to be remove you'll call
> dwfl_report_begin, possibly dwfl_report_elf for any new module and then
> dwfl_report_end has a callback that gets all old modules and decides
> whether to re-report them, or they'll get removed. You might want to
> experiment with doing that and not re-report any module that overlaps
> with the new module. (See the libdwfl.h documentation for a hopefully
> clearer description.)
> 
> > > Specifically are you using false for the add_p_vaddr argument?
> > 
> > Yes, we are.
> > 
> > > And could you provide some example where the reported address is
> > > wrong/different from the start address of the Dwfl_Module?
> > 
> > I don't think it's the start address that is wrong, rather it's the end
> > address. But it's hard for me to come up with a small selfcontained
> > example at this stage. I am regularly seeing broken backtraces for
> > samples where I have the gut feeling that missing reported ELFs are to
> > blame. But we report everything, except for scenarios where the mmap
> > events seemingly overlap. This overlapping is, as far as I can see,
> > actually a side effect of remapping taking place in the dynamic linker
> > (i.e. a single dlopen/dynamic linked library can yield multiple mmap
> > events). One way or another, we end up with a situation where we cannot
> > report an ELF to dwfl due to two issues:
> > 
> > a) either ELF tells us we are overlapping some module and just stops which
> > is bad, since we would actually much prefer the newly reported ELF to
> > take precedence
> > 
> > b) we find an mmap event that with a non-zero pgoff, and have no clue how
> > to call dwfl_report_elf and just give up.
> > 
> > In both cases, I was hopeing for dwfl_report_module to help since it
> > seemingly allows me to exactly recreate the mapping that was traced
> > originally.
> If you could add some logging and post that plus the eu-readelf -l
> output of the ELF file, that might help track down what is really going
> on.

Yes, I will try to find the time to write a more elaborate reproducer for this 
issue, to better figure out what is going on here.

Bye

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to associate Elf with Dwfl_Module returned by dwfl_report_module

2018-03-22 Thread Milian Wolff
On Mittwoch, 21. März 2018 22:31:07 CET Mark Wielaard wrote:
> On Wed, Mar 21, 2018 at 03:23:51PM +0100, Milian Wolff wrote:
> > Here's one way to investigate where perf and dwfl disagree on the file
> > mappings:
> > 
> > diff --git a/tools/perf/util/unwind-libdw.c
> > b/tools/perf/util/unwind-libdw.c index 7bdd239c795c..739c68b73772 100644
> > --- a/tools/perf/util/unwind-libdw.c
> > +++ b/tools/perf/util/unwind-libdw.c
> > @@ -48,11 +48,17 @@ static int __report_module(struct addr_location *al,
> > u64 ip,
> > 
> > mod = 0;
> > 
> > }
> > 
> > -   if (!mod)
> > +   if (!mod) {
> > +   Dwarf_Addr s, e;
> > 
> > mod = dwfl_report_elf(ui->dwfl, dso->short_name,
> > 
> >   (dso->symsrc_filename ? 
> > dso->symsrc_filename : dso-
> > >
> > >long_name), -1, al->map->start,
> > >
> >   false);
> > 
> > +   dwfl_module_info(mod, NULL, &s, &e, NULL, NULL, NULL, NULL);
> > +   if (al->map->start != s || al->map->end != e)
> > +   pr_warning("MEH: %s | mmap: %zx %zx | dwfl: %zx %zx\n", 
> > dso-
> > 
> > >short_name, al->map->start, al->map->end, s, e);
> > 
> > +   }
> > +
> > 
> > return mod && dwfl_addrmodule(ui->dwfl, ip) == mod ? 0 : -1;
> >  
> >  }
> > 
> > On one of my perf.data files with many broken backtraces this gives me:
> > 
> > MEH: heaptrack_print | mmap: 56166e9d4000 56166ea39000 | dwfl:
> > 56166e9d4000
> > 56166ea38880
> > MEH: ld-2.26.so | mmap: 7fd0afc6c000 7fd0afe93000 | dwfl: 7fd0afc6c000
> > 7fd0afe920f8
> > MEH: libc-2.26.so | mmap: 7fd0ae16a000 7fd0ae521000 | dwfl: 7fd0ae16a000
> > 7fd0ae5208f0
> > MEH: libstdc++.so.6.0.24 | mmap: 7fd0aea84000 7fd0aee0b000 | dwfl:
> > 7fd0aea84000 7fd0aee0a640
> > MEH: libzstd.so.1.3.3 | mmap: 7fd0aee0b000 7fd0af087000 | dwfl:
> > 7fd0aee0b000 7fd0af086030
> > 
> > Interestingly, here the mmap events observed by perf are actually always
> > *larger* than what dwfl sees...
> 
> I think that is simply the end address being extended to the next page.
> Where the page size seems to be 4K (0x1000). I assume if you look at
> the actual LOAD segment sizes of the files (eu-readelf -l) they match
> with what dwfl reports, and if it extends to the nearest 4k page it
> matches whatp mmap reports.

Yes, I agree - the above does not explain the issues I am seeing. It is most 
probably just noise and can be ignored.

I'll try to dig deeper to figure out what is going on and then come back here. 
For now, it seems like using dwfl_report_elf is fine.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: How to associate Elf with Dwfl_Module returned by dwfl_report_module

2018-03-22 Thread Milian Wolff
On Donnerstag, 22. März 2018 10:11:29 CET Ulf Hermann wrote:
> Hi Milian,
> 
> > I am regularly seeing broken backtraces for samples where I have
> > the gut feeling that missing reported ELFs are to blame. But we report
> > everything, except for scenarios where the mmap events seemingly overlap.
> 
> Actually, at least for perfparser that's not quite true. When perfparser
> encounters an overlap error, it will throw out the entire set of mappings
> and restart reporting, with the addresses from the current sample (see
> PerfSymbolTable::reportElf() and PerfSymbolTable::clearCache()).

Yes, I know :) I was more talking about the libdwfl integraiton in perf there.

> If that
> still gives you overlapping ranges, it means perf has not sent all the mmap
> events and therefore we're reporting the wrong ELF for some address in your
> sample. That wrong ELF may be larger than the one we actually want and
> therefore it can overlap some other ELF an address in your sample points
> to.
> 
> I've seen that happen. Make sure to keep your sample rate low enough to
> prevent perf from dropping anything.
> 
> I realize we could optimize the reporting a bit, with the dwfl_report_end
> callback Mark mentioned, but if you have addresses into two overlapping
> ELFs in one sample, that's fundamentally impossible to unwind.

In the concrete data file I have at hand, no chunks got lost, so I don't think 
that mmap events could have been lost?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Relative path X full path

2018-03-26 Thread Milian Wolff
On Montag, 26. März 2018 18:45:13 CEST Sasha Da Rocha Pinheiro wrote:
> Hi,
> 
> I have noticed that when we compile out-of-source, the paths returned from
> libdw are gonna be relative. Otherwise, they show up as full path. Have you
> noticed it?

You are talking about source file paths, right? From what I've seen, libdw 
just returns the paths that have been embedded by the compiler in the debug 
section. If you compile with relative paths, then the compiler (sadly) adds 
relative paths too. The only "fix" I'm aware of is to use a build system that 
enforces absolute paths everywhere.

HTH
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-09-26 Thread Milian Wolff
On Friday, September 21, 2018 3:07:29 PM CEST Mark Wielaard wrote:
> On Wed, 2018-09-19 at 14:24 +0200, Ulf Hermann wrote:
> > > We suspect perf to offset its recording-addresses of mmapped
> > > dsos/executables starting with a specific section, such that they
> > > denote
> > > their pointers with this pg_offset parameter. (e.g. skipping a
> > > library's
> > > header and setting pgoff to the headersize). Although we are not
> > > 100%
> > > sure about this information.
> > 
> > According to my understanding, the pgoff is not perf's invention.
> > Rather, the libary loader for the target application does not mmap()
> > the full ELF file, but only the parts it's interested in. Those
> > partial mappings are then reported through perf.
> 
> OK, so pgoff is like the offset argument of the mmap call?
> Is it just recording all user space mmap events that have PROT_EXEC in
> their prot argument? What about if the mapping was later changed with
> mprotect? Or does PERF_RECORD_MMAP only map to some internal kernel
> mmap action?
> 
> >  We then try to recreate the memory mapping with perfparser, but run
> > 
> > into problems because dwfl_report_elf() doesn't let us do partial
> > mappings. You can only map complete files with that function. There
> > probably is some way to manually map the relevant sections using
> > other functions in libdw and libelf, but I haven't figured out how to
> > do this, yet. If there is a simple trick I'm missing, I'd be happy to
> > hear about it. 
> > 
> > And, yes, a function that works like dwfl_report_elf, but takes a
> > pgoff and length as additional parameters is sorely missing from the
> > API.
> 
> dwfl_report_elf indeed does assume the whole ELF file is mapped in
> according to the PHDRs in the file and the given base address. But what
> you are actually seeing (I think, depending on the answers on the
> questions above) is the dynamic loader mapping in the file in pieces.
> And so you would like an interface where you can report the module
> piece wise while it is being mapped in. So what would be most
> convenient would be some kind of dwfl_report_elf_mmap function that you
> can use to either get a new Dwfl_Module or to extend an existing one.
> 
> I have to think how that interacts with mprotect and mmunmap.

Hey Mark,

I can only second what Christoph and Ulf said so far. I want to add though 
that this limitation essentially makes elfutils unusable for usage in perf. 
I.e., perf can be build with either libunwind or elfutils for unwinding 
callstacks. Using the former works like a charm. The latter runs into the same 
problems like perfparser / hotspot. To reproduce, use a modern distribution 
with recent userland and kernel, then do something like this:

~
$ cat test.cpp
#include 
#include 
#include 
#include 

using namespace std;

int main()
{
uniform_real_distribution uniform(-1E5, 1E5);
default_random_engine engine;
double s = 0;
for (int i = 0; i < 1000; ++i) {
s += norm(complex(uniform(engine), uniform(engine)));
}
cout << s << '\n';
return 0;
}
$ g++ -O2 -g test.cpp -o test
$ perf record --call-graph dwarf ./test
$ perf report --stdio -vv
...
overlapping maps:
 55a1cdd87000-55a1cdd89000 1000 /ssd/milian/projects/kdab/rnd/hotspot/tests/
test-clients/cpp-inlining/test
 55a1cdd85000-55a1cdd89000 0 /ssd/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/test
 55a1cdd85000-55a1cdd87000 0 /ssd/milian/projects/kdab/rnd/hotspot/tests/test-
clients/cpp-inlining/test
overlapping maps:
 7fdda469-7fdda46af000 2000 /usr/lib/ld-2.28.so
 7fdda468e000-7fdda46ba000 0 /usr/lib/ld-2.28.so
 7fdda468e000-7fdda469 0 /usr/lib/ld-2.28.so
 7fdda46af000-7fdda46ba000 0 /usr/lib/ld-2.28.so
...
57.14% 0.00%  test [unknown] [.] 0xe775b17d50ae8cff
|
---0xe775b17d50ae8cff
~

To build perf against elfutils, do:

~
git clone --branch=perf/core git://git.kernel.org/pub/scm/linux/kernel/git/
acme/linux.git
cd linux/tools/perf
make NO_LIBUNWIND=1
~

The perf binary in the last folder can than be used as a drop-in replacement.

Since I consider this issue a serious blocker, I would like to see it fixed 
sooner rather than later. Would it maybe be possible for you to create a proof 
of concept for the new proposed dwfl_report_elf_mmap? I can then try to take 
it from there to fill in the missing bits and pieces and to make it actually 
work for our purposes.

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de




Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-09 Thread Milian Wolff
On Mittwoch, 26. September 2018 16:38:43 CEST Milian Wolff wrote:
> On Friday, September 21, 2018 3:07:29 PM CEST Mark Wielaard wrote:
> > On Wed, 2018-09-19 at 14:24 +0200, Ulf Hermann wrote:
> > > > We suspect perf to offset its recording-addresses of mmapped
> > > > dsos/executables starting with a specific section, such that they
> > > > denote
> > > > their pointers with this pg_offset parameter. (e.g. skipping a
> > > > library's
> > > > header and setting pgoff to the headersize). Although we are not
> > > > 100%
> > > > sure about this information.
> > > 
> > > According to my understanding, the pgoff is not perf's invention.
> > > Rather, the libary loader for the target application does not mmap()
> > > the full ELF file, but only the parts it's interested in. Those
> > > partial mappings are then reported through perf.
> > 
> > OK, so pgoff is like the offset argument of the mmap call?
> > Is it just recording all user space mmap events that have PROT_EXEC in
> > their prot argument? What about if the mapping was later changed with
> > mprotect? Or does PERF_RECORD_MMAP only map to some internal kernel
> > mmap action?
> > 
> > >  We then try to recreate the memory mapping with perfparser, but run
> > > 
> > > into problems because dwfl_report_elf() doesn't let us do partial
> > > mappings. You can only map complete files with that function. There
> > > probably is some way to manually map the relevant sections using
> > > other functions in libdw and libelf, but I haven't figured out how to
> > > do this, yet. If there is a simple trick I'm missing, I'd be happy to
> > > hear about it.
> > > 
> > > And, yes, a function that works like dwfl_report_elf, but takes a
> > > pgoff and length as additional parameters is sorely missing from the
> > > API.
> > 
> > dwfl_report_elf indeed does assume the whole ELF file is mapped in
> > according to the PHDRs in the file and the given base address. But what
> > you are actually seeing (I think, depending on the answers on the
> > questions above) is the dynamic loader mapping in the file in pieces.
> > And so you would like an interface where you can report the module
> > piece wise while it is being mapped in. So what would be most
> > convenient would be some kind of dwfl_report_elf_mmap function that you
> > can use to either get a new Dwfl_Module or to extend an existing one.
> > 
> > I have to think how that interacts with mprotect and mmunmap.
> 
> Hey Mark,
> 
> I can only second what Christoph and Ulf said so far. I want to add though
> that this limitation essentially makes elfutils unusable for usage in perf.
> I.e., perf can be build with either libunwind or elfutils for unwinding
> callstacks. Using the former works like a charm. The latter runs into the
> same problems like perfparser / hotspot. To reproduce, use a modern
> distribution with recent userland and kernel, then do something like this:
> 
> ~
> $ cat test.cpp
> #include 
> #include 
> #include 
> #include 
> 
> using namespace std;
> 
> int main()
> {
> uniform_real_distribution uniform(-1E5, 1E5);
> default_random_engine engine;
> double s = 0;
> for (int i = 0; i < 1000; ++i) {
> s += norm(complex(uniform(engine), uniform(engine)));
> }
> cout << s << '\n';
> return 0;
> }
> $ g++ -O2 -g test.cpp -o test
> $ perf record --call-graph dwarf ./test
> $ perf report --stdio -vv
> ...
> overlapping maps:
>  55a1cdd87000-55a1cdd89000 1000 /ssd/milian/projects/kdab/rnd/hotspot/tests/
> test-clients/cpp-inlining/test
>  55a1cdd85000-55a1cdd89000 0
> /ssd/milian/projects/kdab/rnd/hotspot/tests/test- clients/cpp-inlining/test
>  55a1cdd85000-55a1cdd87000 0
> /ssd/milian/projects/kdab/rnd/hotspot/tests/test- clients/cpp-inlining/test
> overlapping maps:
>  7fdda469-7fdda46af000 2000 /usr/lib/ld-2.28.so
>  7fdda468e000-7fdda46ba000 0 /usr/lib/ld-2.28.so
>  7fdda468e000-7fdda469 0 /usr/lib/ld-2.28.so
>  7fdda46af000-7fdda46ba000 0 /usr/lib/ld-2.28.so
> ...
> 57.14% 0.00%  test [unknown] [.] 0xe775b17d50ae8cff
> 
> ---0xe775b17d50ae8cff
> ~
> 
> To build perf against elfutils, do:
> 
> ~
> git clone --branch=perf/core git://git.kernel.org/pub/scm/linux/kernel/git/
> acme/linux.git
> cd linux/tools/perf
> make NO_LIBUNWIND=1
> ~
> 
> The perf binary in the last folder can than be used as a drop-in
&g

Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-11 Thread Milian Wolff
On Donnerstag, 11. Oktober 2018 19:37:07 CEST Mark Wielaard wrote:
> Hi,
> 
> My apologies for not having looked deeper at this.
> It is a bit tricky and I just didnt have enough time to
> really sit down and think it all through yet.
> 
> On Thu, Oct 11, 2018 at 05:02:18PM +, Ulf Hermann wrote:
> > is there any pattern in how the loader maps the ELF sections into
> > memory? What sections does it actually map and which of those do we need
> > for unwinding?
> 
> Yes, it would be helpful to have some examples of mmap events plus
> the associated segment header (eu-readelf -l) of the ELF file.
> 
> Note that the kernel and dynamic loader will use the (PT_LOAD) segments,
> not the sections, to map things into memory. Each segment might contain
> multiple sections.
> 
> libdwfl then tries to associate the correct sections (and address bias)
> with how the ELF file was mapped into memory.
> 
> > I hope that only one of those MMAPs per ELF is actually meaningful and
> > we can simply add that one's pgoff as an extra member to Dwfl_Module and
> > use it whenever we poke the underlying file.
> 
> One "trick" might be to just substract the pgoff from the load address.
> And so report as if the ELF file was being mapped from the start. This
> isn't really correct, but it might be interesting to see if that makes
> libdwfl able to just associate the whole ELF file with the correct
> address map.

I'll try to come up with some minimal code examples we can use to test all of 
this. But from what I remember, neither of the above suggestions will be 
sufficient as we can still run into overlapping module errors from elfutils 
when we always load everything. I.e. I believe we've seen mappings that 
eventually become partially obsoleted by a future mmap event. At that point, 
we somehow need to be able to only map parts of a file, not all of it. So just 
subtracting or honoring pgoff is not enough, I believe we also need to be able 
to explicitly say how much of a file to map.

But to make this discussion easier to follow for others, I'll create some 
standalone cpp code that takes a `perf script --show-mmap-events  | grep 
PERF_RECORD_MMAP` input file and then runs this through elfutils API to 
reproduce the issues we are facing.

I'll get back to you all once this is done.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-15 Thread Milian Wolff
On Donnerstag, 11. Oktober 2018 20:14:43 CEST Milian Wolff wrote:
> On Donnerstag, 11. Oktober 2018 19:37:07 CEST Mark Wielaard wrote:
> > Hi,
> > 
> > My apologies for not having looked deeper at this.
> > It is a bit tricky and I just didnt have enough time to
> > really sit down and think it all through yet.
> > 
> > On Thu, Oct 11, 2018 at 05:02:18PM +, Ulf Hermann wrote:
> > > is there any pattern in how the loader maps the ELF sections into
> > > memory? What sections does it actually map and which of those do we need
> > > for unwinding?
> > 
> > Yes, it would be helpful to have some examples of mmap events plus
> > the associated segment header (eu-readelf -l) of the ELF file.
> > 
> > Note that the kernel and dynamic loader will use the (PT_LOAD) segments,
> > not the sections, to map things into memory. Each segment might contain
> > multiple sections.
> > 
> > libdwfl then tries to associate the correct sections (and address bias)
> > with how the ELF file was mapped into memory.
> > 
> > > I hope that only one of those MMAPs per ELF is actually meaningful and
> > > we can simply add that one's pgoff as an extra member to Dwfl_Module and
> > > use it whenever we poke the underlying file.
> > 
> > One "trick" might be to just substract the pgoff from the load address.
> > And so report as if the ELF file was being mapped from the start. This
> > isn't really correct, but it might be interesting to see if that makes
> > libdwfl able to just associate the whole ELF file with the correct
> > address map.
> 
> I'll try to come up with some minimal code examples we can use to test all
> of this. But from what I remember, neither of the above suggestions will be
> sufficient as we can still run into overlapping module errors from elfutils
> when we always load everything. I.e. I believe we've seen mappings that
> eventually become partially obsoleted by a future mmap event. At that
> point, we somehow need to be able to only map parts of a file, not all of
> it. So just subtracting or honoring pgoff is not enough, I believe we also
> need to be able to explicitly say how much of a file to map.
> 
> But to make this discussion easier to follow for others, I'll create some
> standalone cpp code that takes a `perf script --show-mmap-events  | grep
> PERF_RECORD_MMAP` input file and then runs this through elfutils API to
> reproduce the issues we are facing.
> 
> I'll get back to you all once this is done.

Hey all,

here's one example of mmap events recorded by perf:

0x7fac5ec0b000 to 0x7fac5ed9a000, len =   0x18f000, offset =0   
r--p/usr/lib/libstdc++.so.6.0.25
0x7fac5ec94000 to 0x7fac5ed8a000, len =0xf6000, offset =  0x89000   
---p/usr/lib/libstdc++.so.6.0.25
0x7fac5ec94000 to 0x7fac5ed4c000, len =0xb8000, offset =  0x89000   
r-xp/usr/lib/libstdc++.so.6.0.25
0x7fac5ed4c000 to 0x7fac5ed89000, len =0x3d000, offset = 0x141000   
r--p/usr/lib/libstdc++.so.6.0.25
0x7fac5ed8a000 to 0x7fac5ed97000, len = 0xd000, offset = 0x17e000   
rw-p/usr/lib/libstdc++.so.6.0.25

this is noteworthy in multiple ways:

- the first mapping we receive is for pgoff = 0 for the full file size aligned 
to the page boundary
- the first mapping isn't executable yet
- the last mappings have a huge offset which actually lies beyond the 
initially mmaped region?!

And to make things worse, when we report the file at address 0x7fac5ec0b000 
via dwfl, we get:

reported module /usr/lib/libstdc++.so.6.0.25
expected: 0x7fac5ec0b000 to 0x7fac5ed9a000 (0x18f000)
actual:   0x7fac5ec0b000 to 0x7fac5ed99640 (0x18e640)

So now dwfl won't ever be able to map any addresses into this module when they 
come after 0x7fac5ed99640, but the mmap events above seem to indicate that 
this could be possible?

I'll now upload my code to enable you all to play around with this yourself.

Bye
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-15 Thread Milian Wolff
On Donnerstag, 11. Oktober 2018 20:14:43 CEST Milian Wolff wrote:
> On Donnerstag, 11. Oktober 2018 19:37:07 CEST Mark Wielaard wrote:
> > Hi,
> > 
> > My apologies for not having looked deeper at this.
> > It is a bit tricky and I just didnt have enough time to
> > really sit down and think it all through yet.
> > 
> > On Thu, Oct 11, 2018 at 05:02:18PM +, Ulf Hermann wrote:
> > > is there any pattern in how the loader maps the ELF sections into
> > > memory? What sections does it actually map and which of those do we need
> > > for unwinding?
> > 
> > Yes, it would be helpful to have some examples of mmap events plus
> > the associated segment header (eu-readelf -l) of the ELF file.
> > 
> > Note that the kernel and dynamic loader will use the (PT_LOAD) segments,
> > not the sections, to map things into memory. Each segment might contain
> > multiple sections.
> > 
> > libdwfl then tries to associate the correct sections (and address bias)
> > with how the ELF file was mapped into memory.
> > 
> > > I hope that only one of those MMAPs per ELF is actually meaningful and
> > > we can simply add that one's pgoff as an extra member to Dwfl_Module and
> > > use it whenever we poke the underlying file.
> > 
> > One "trick" might be to just substract the pgoff from the load address.
> > And so report as if the ELF file was being mapped from the start. This
> > isn't really correct, but it might be interesting to see if that makes
> > libdwfl able to just associate the whole ELF file with the correct
> > address map.
> 
> I'll try to come up with some minimal code examples we can use to test all
> of this. But from what I remember, neither of the above suggestions will be
> sufficient as we can still run into overlapping module errors from elfutils
> when we always load everything. I.e. I believe we've seen mappings that
> eventually become partially obsoleted by a future mmap event. At that
> point, we somehow need to be able to only map parts of a file, not all of
> it. So just subtracting or honoring pgoff is not enough, I believe we also
> need to be able to explicitly say how much of a file to map.
> 
> But to make this discussion easier to follow for others, I'll create some
> standalone cpp code that takes a `perf script --show-mmap-events  | grep
> PERF_RECORD_MMAP` input file and then runs this through elfutils API to
> reproduce the issues we are facing.
> 
> I'll get back to you all once this is done.

I've pushed a preliminary POC for a reproducer:

https://github.com/milianw/perf_mmaps_to_elfutils

Note that it's not really exhibiting any dwfl errors as-is. We would need to 
feed it also all the instruction pointer addresses that perf encounters, then 
try to find the matching module via libdwfl. This isn't easily done, and I 
hope the current output already exemplifies some of the issues with the 
current libdwfl API.

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-15 Thread Milian Wolff
On Montag, 15. Oktober 2018 23:04:52 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Mon, 2018-10-15 at 22:38 +0200, Milian Wolff wrote:
> > here's one example of mmap events recorded by perf:
> > 
> > 0x7fac5ec0b000 to 0x7fac5ed9a000, len =   0x18f000, offset
> > =0r--p/usr/lib/libstdc++.so.6.0.25
> > 0x7fac5ec94000 to 0x7fac5ed8a000, len =0xf6000, offset
> > =  0x89000---p/usr/lib/libstdc++.so.6.0.25
> > 0x7fac5ec94000 to 0x7fac5ed4c000, len =0xb8000, offset
> > =  0x89000r-xp/usr/lib/libstdc++.so.6.0.25
> > 0x7fac5ed4c000 to 0x7fac5ed89000, len =0x3d000, offset
> > = 0x141000r--p/usr/lib/libstdc++.so.6.0.25
> > 0x7fac5ed8a000 to 0x7fac5ed97000, len = 0xd000, offset
> > = 0x17e000rw-p/usr/lib/libstdc++.so.6.0.25
> 
> Could you also post the matching phdr output for the file?
> eu-readelf -l /usr/lib/libstdc++.so.6.0.25 should show it.
> That way we can see how the PT_LOAD segments map to the mmap events.

Sure:

$ eu-readelf -l /usr/lib/libstdc++.so.6.0.25
Program Headers:
  Type   Offset   VirtAddr   PhysAddr   FileSiz  
MemSiz   Flg Align
  LOAD   0x00 0x 0x 0x088fa8 
0x088fa8 R   0x1000
  LOAD   0x089000 0x00089000 0x00089000 0x0b7ae1 
0x0b7ae1 R E 0x1000
  LOAD   0x141000 0x00141000 0x00141000 0x03cfe0 
0x03cfe0 R   0x1000
  LOAD   0x17e8e0 0x0017f8e0 0x0017f8e0 0x00b8b8 
0x00ed60 RW  0x1000
  DYNAMIC0x1873a8 0x001883a8 0x001883a8 0x0001e0 
0x0001e0 RW  0x8
  NOTE   0x0002a8 0x02a8 0x02a8 0x24 
0x24 R   0x4
  NOTE   0x17dfc0 0x0017dfc0 0x0017dfc0 0x20 
0x20 R   0x8
  TLS0x17e8e0 0x0017f8e0 0x0017f8e0 0x00 
0x20 R   0x8
  GNU_EH_FRAME   0x149558 0x00149558 0x00149558 0x007f04 
0x007f04 R   0x4
  GNU_STACK  0x00 0x 0x 0x00 
0x00 RW  0x10
  GNU_RELRO  0x17e8e0 0x0017f8e0 0x0017f8e0 0x00b720 
0x00b720 R   0x1

 Section to Segment mapping:
  Segment Sections...
   00  [RO: .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version 
.gnu.version_d .gnu.version_r .rela.dyn]
   01  [RO: .init .text .fini]
   02  [RO: .rodata .eh_frame_hdr .eh_frame .gcc_except_table 
.note.gnu.property]
   03  [RELRO: .tbss .init_array .fini_array .data.rel.ro .dynamic .got] 
.got.plt .data .bss
   04  [RELRO: .dynamic]
   05  [RO: .note.gnu.build-id]
   06  [RO: .note.gnu.property]
   07  [RELRO: .tbss]
   08  [RO: .eh_frame_hdr]
   09     
   10  [RELRO: .tbss .init_array .fini_array .data.rel.ro .dynamic .got]

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-17 Thread Milian Wolff
On Montag, 15. Oktober 2018 23:06:07 CEST Milian Wolff wrote:
> On Montag, 15. Oktober 2018 23:04:52 CEST Mark Wielaard wrote:
> > Hi Milian,
> > 
> > On Mon, 2018-10-15 at 22:38 +0200, Milian Wolff wrote:
> > > here's one example of mmap events recorded by perf:
> > > 
> > > 0x7fac5ec0b000 to 0x7fac5ed9a000, len =   0x18f000, offset
> > > =0r--p/usr/lib/libstdc++.so.6.0.25
> > > 0x7fac5ec94000 to 0x7fac5ed8a000, len =0xf6000, offset
> > > =  0x89000---p/usr/lib/libstdc++.so.6.0.25
> > > 0x7fac5ec94000 to 0x7fac5ed4c000, len =0xb8000, offset
> > > =  0x89000r-xp/usr/lib/libstdc++.so.6.0.25
> > > 0x7fac5ed4c000 to 0x7fac5ed89000, len =0x3d000, offset
> > > = 0x141000r--p/usr/lib/libstdc++.so.6.0.25
> > > 0x7fac5ed8a000 to 0x7fac5ed97000, len = 0xd000, offset
> > > = 0x17e000rw-p/usr/lib/libstdc++.so.6.0.25
> > 
> > Could you also post the matching phdr output for the file?
> > eu-readelf -l /usr/lib/libstdc++.so.6.0.25 should show it.
> > That way we can see how the PT_LOAD segments map to the mmap events.
> 
> Sure:
> 
> $ eu-readelf -l /usr/lib/libstdc++.so.6.0.25
> Program Headers:
>   Type   Offset   VirtAddr   PhysAddr   FileSiz
> MemSiz   Flg Align
>   LOAD   0x00 0x 0x 0x088fa8
> 0x088fa8 R   0x1000
>   LOAD   0x089000 0x00089000 0x00089000 0x0b7ae1
> 0x0b7ae1 R E 0x1000
>   LOAD   0x141000 0x00141000 0x00141000 0x03cfe0
> 0x03cfe0 R   0x1000
>   LOAD   0x17e8e0 0x0017f8e0 0x0017f8e0 0x00b8b8
> 0x00ed60 RW  0x1000
>   DYNAMIC0x1873a8 0x001883a8 0x001883a8 0x0001e0
> 0x0001e0 RW  0x8
>   NOTE   0x0002a8 0x02a8 0x02a8 0x24
> 0x24 R   0x4
>   NOTE   0x17dfc0 0x0017dfc0 0x0017dfc0 0x20
> 0x20 R   0x8
>   TLS0x17e8e0 0x0017f8e0 0x0017f8e0 0x00
> 0x20 R   0x8
>   GNU_EH_FRAME   0x149558 0x00149558 0x00149558 0x007f04
> 0x007f04 R   0x4
>   GNU_STACK  0x00 0x 0x 0x00
> 0x00 RW  0x10
>   GNU_RELRO  0x17e8e0 0x0017f8e0 0x0017f8e0 0x00b720
> 0x00b720 R   0x1
> 
>  Section to Segment mapping:
>   Segment Sections...
>00  [RO: .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version
> .gnu.version_d .gnu.version_r .rela.dyn]
>01  [RO: .init .text .fini]
>02  [RO: .rodata .eh_frame_hdr .eh_frame .gcc_except_table
> .note.gnu.property]
>03  [RELRO: .tbss .init_array .fini_array .data.rel.ro .dynamic .got]
> .got.plt .data .bss
>04  [RELRO: .dynamic]
>05  [RO: .note.gnu.build-id]
>06  [RO: .note.gnu.property]
>07  [RELRO: .tbss]
>08  [RO: .eh_frame_hdr]
>09
>10  [RELRO: .tbss .init_array .fini_array .data.rel.ro .dynamic .got]

So, Mark - any chance you could have a look at the above and give us your 
feedback?

When I compare the actual mmap events with the LOAD segments, there are some 
similarities, but also some discrepancies. Note how the mmap sizes always 
differ from the FileSiz header value. And the offsets also sometimes mismatch, 
e.g. for the last segment / mmap event we get 0x17f8e0 in the header, but 
0x17e000 in the mmap event...:

LOAD   0x17e8e0 0x0017f8e0 0x0017f8e0 0x00b8b8
 0x00ed60 RW  0x1000

0x7fac5ed8a000 to 0x7fac5ed97000, len = 0xd000, offset = 0x17e000   
 
rw-p/usr/lib/libstdc++.so.6.0.25

I'm pretty confused here!

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Handling pgoff in perf elf mmap/mmap2 elf info

2018-10-20 Thread Milian Wolff
On Montag, 15. Oktober 2018 22:38:53 CEST Milian Wolff wrote:
> On Donnerstag, 11. Oktober 2018 20:14:43 CEST Milian Wolff wrote:
> > On Donnerstag, 11. Oktober 2018 19:37:07 CEST Mark Wielaard wrote:
> > > Hi,
> > > 
> > > My apologies for not having looked deeper at this.
> > > It is a bit tricky and I just didnt have enough time to
> > > really sit down and think it all through yet.
> > > 
> > > On Thu, Oct 11, 2018 at 05:02:18PM +, Ulf Hermann wrote:
> > > > is there any pattern in how the loader maps the ELF sections into
> > > > memory? What sections does it actually map and which of those do we
> > > > need
> > > > for unwinding?
> > > 
> > > Yes, it would be helpful to have some examples of mmap events plus
> > > the associated segment header (eu-readelf -l) of the ELF file.
> > > 
> > > Note that the kernel and dynamic loader will use the (PT_LOAD) segments,
> > > not the sections, to map things into memory. Each segment might contain
> > > multiple sections.
> > > 
> > > libdwfl then tries to associate the correct sections (and address bias)
> > > with how the ELF file was mapped into memory.
> > > 
> > > > I hope that only one of those MMAPs per ELF is actually meaningful and
> > > > we can simply add that one's pgoff as an extra member to Dwfl_Module
> > > > and
> > > > use it whenever we poke the underlying file.
> > > 
> > > One "trick" might be to just substract the pgoff from the load address.
> > > And so report as if the ELF file was being mapped from the start. This
> > > isn't really correct, but it might be interesting to see if that makes
> > > libdwfl able to just associate the whole ELF file with the correct
> > > address map.
> > 
> > I'll try to come up with some minimal code examples we can use to test all
> > of this. But from what I remember, neither of the above suggestions will
> > be
> > sufficient as we can still run into overlapping module errors from
> > elfutils
> > when we always load everything. I.e. I believe we've seen mappings that
> > eventually become partially obsoleted by a future mmap event. At that
> > point, we somehow need to be able to only map parts of a file, not all of
> > it. So just subtracting or honoring pgoff is not enough, I believe we also
> > need to be able to explicitly say how much of a file to map.
> > 
> > But to make this discussion easier to follow for others, I'll create some
> > standalone cpp code that takes a `perf script --show-mmap-events  | grep
> > PERF_RECORD_MMAP` input file and then runs this through elfutils API to
> > reproduce the issues we are facing.
> > 
> > I'll get back to you all once this is done.
> 
> Hey all,
> 
> here's one example of mmap events recorded by perf:
> 
> 0x7fac5ec0b000 to 0x7fac5ed9a000, len =   0x18f000, offset =   
> 0 r--p/usr/lib/libstdc++.so.6.0.25
> 0x7fac5ec94000 to 0x7fac5ed8a000, len =0xf6000, offset = 
> 0x89000 ---p/usr/lib/libstdc++.so.6.0.25
> 0x7fac5ec94000 to 0x7fac5ed4c000, len =0xb8000, offset = 
> 0x89000 r-xp/usr/lib/libstdc++.so.6.0.25
> 0x7fac5ed4c000 to 0x7fac5ed89000, len =0x3d000, offset =
> 0x141000 r--p/usr/lib/libstdc++.so.6.0.25
> 0x7fac5ed8a000 to 0x7fac5ed97000, len = 0xd000, offset =
> 0x17e000 rw-p/usr/lib/libstdc++.so.6.0.25

Spending more time on this issue, I've come up with a seemingly viable 
approach to workaround the libdwfl API limitations. Most notably, one must not 
take the raw mmap events and try to report them. Instead, we now associate the 
"base mmap", i.e. the first one with pgoff = 0, with the following mmaps for 
this file. Then, when we'd otherwise try to report one of the following mmaps, 
we lookup the base mmap addr and use that in our interaction with libdwfl.

Now I'm not seeing any "overlapping address" errors anymore, and unwinding 
seems to work fine again for perf files with mmap events like in the above.

There are still quite a few broken backtraces, but so far I can't say what the 
reason for this is.

So, from my POV, I'm fine - I have a viable workaround. But from the POV of 
future users of libdwfl, I still believe it would be useful to have more 
control over what gets reported and where. I.e. instead of having libdwfl 
analyze the PT_LOAD sections, offer an API that would allow us to feed the 
mmaped regions directly with pgoff etc.

Thanks for the input everyone

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


[PATCH] Also find CFI in sections of type SHT_X86_64_UNWIND

2018-10-29 Thread Milian Wolff
On my system with g++ (GCC) 8.2.1 20180831 with GNU gold (GNU Binutils
2.31.1) 1.16, the .eh_frame section does not have type PROGBITS
but rather is using X86_64_UNWIND nowadays:

```
$ echo "int main(){ return 0; }" > test.c
$ gcc test.c
$ readelf --sections a.out | grep .eh_frame
  [14] .eh_frame X86_64_UNWIND0670  0670
  [15] .eh_frame_hdr X86_64_UNWIND0724  0724
```

Without this patch, libdw refuses to use the available unwind
information, leading to broken backtraces while unwinding. With the
patch applied, unwinding works once more in such situations.

Signed-off-by: Milian Wolff 
---
 libdw/dwarf_getcfi_elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdw/dwarf_getcfi_elf.c b/libdw/dwarf_getcfi_elf.c
index 315cc02f..4bcfe5cd 100644
--- a/libdw/dwarf_getcfi_elf.c
+++ b/libdw/dwarf_getcfi_elf.c
@@ -298,7 +298,7 @@ getcfi_shdr (Elf *elf, const GElf_Ehdr *ehdr)
}
  else if (!strcmp (name, ".eh_frame"))
{
- if (shdr->sh_type == SHT_PROGBITS)
+ if (shdr->sh_type == SHT_PROGBITS || shdr->sh_type == 
SHT_X86_64_UNWIND)
return getcfi_scn_eh_frame (elf, ehdr, scn, shdr,
hdr_scn, hdr_vaddr);
  else
-- 
2.19.1


[PATCH] Fix CFI interpretation for locations on DW_CFA_*_loc boundaries

2018-11-01 Thread Milian Wolff
According to the DWARF v3 standard §6.4.3 3., all call frame
instructions up to L1 <= L2 should be interpreted for an FDE.
Elfutils currently only interprets L1 < L2, potentially missing
some instructions when L1 directly points at a DW_CFA_*_loc boundary.

This patch changes the behavior and makes elfutils behave like
libunwind in that regard.
---
 libdw/cfi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdw/cfi.c b/libdw/cfi.c
index 341e055b..332c6b8b 100644
--- a/libdw/cfi.c
+++ b/libdw/cfi.c
@@ -125,7 +125,7 @@ execute_cfi (Dwarf_CFI *cache,
 fs->regs[regno].value = (r_value); \
   } while (0)
 
-  while (program < end)
+  while (program <= end)
 {
   uint8_t opcode = *program++;
   Dwarf_Word regno;
-- 
2.19.1


Re: [PATCH] Fix CFI interpretation for locations on DW_CFA_*_loc boundaries

2018-11-01 Thread Milian Wolff
Please ignore this patch for now - I only looked at one specific case where 
this changed the behavior to be in line with libunwind. Sadly, it breaks other 
previously working situations. I need to look at this in more detail.

Cheers

On Donnerstag, 1. November 2018 09:48:18 CET Milian Wolff wrote:
> According to the DWARF v3 standard §6.4.3 3., all call frame
> instructions up to L1 <= L2 should be interpreted for an FDE.
> Elfutils currently only interprets L1 < L2, potentially missing
> some instructions when L1 directly points at a DW_CFA_*_loc boundary.
> 
> This patch changes the behavior and makes elfutils behave like
> libunwind in that regard.
> ---
>  libdw/cfi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdw/cfi.c b/libdw/cfi.c
> index 341e055b..332c6b8b 100644
> --- a/libdw/cfi.c
> +++ b/libdw/cfi.c
> @@ -125,7 +125,7 @@ execute_cfi (Dwarf_CFI *cache,
>  fs->regs[regno].value = (r_value);   \
>} while (0)
> 
> -  while (program < end)
> +  while (program <= end)
>  {
>uint8_t opcode = *program++;
>Dwarf_Word regno;


-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts

smime.p7s
Description: S/MIME cryptographic signature


Re: [PATCH] Fix CFI interpretation for locations on DW_CFA_*_loc boundaries

2018-11-01 Thread Milian Wolff
On Donnerstag, 1. November 2018 10:12:41 CET Milian Wolff wrote:
> Please ignore this patch for now - I only looked at one specific case where
> this changed the behavior to be in line with libunwind. Sadly, it breaks
> other previously working situations. I need to look at this in more detail.

Yep, that patch is indeed utterly broken - please ignore it and excuse the 
noise.

I was apparently very confused by the different access patterns in libunwind 
vs. elfutils. Elfutils is validating every location referenced in the FDE (cf. 
frame_unwind.c:501). Libunwind on the other hand doesn't do this - it only 
accesses the memory to read the location referenced by the return address 
register.

Cheers

> On Donnerstag, 1. November 2018 09:48:18 CET Milian Wolff wrote:
> > According to the DWARF v3 standard §6.4.3 3., all call frame
> > instructions up to L1 <= L2 should be interpreted for an FDE.
> > Elfutils currently only interprets L1 < L2, potentially missing
> > some instructions when L1 directly points at a DW_CFA_*_loc boundary.
> > 
> > This patch changes the behavior and makes elfutils behave like
> > libunwind in that regard.
> > ---
> > 
> >  libdw/cfi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libdw/cfi.c b/libdw/cfi.c
> > index 341e055b..332c6b8b 100644
> > --- a/libdw/cfi.c
> > +++ b/libdw/cfi.c
> > @@ -125,7 +125,7 @@ execute_cfi (Dwarf_CFI *cache,
> > 
> >  fs->regs[regno].value = (r_value); \
> >
> >} while (0)
> > 
> > -  while (program < end)
> > +  while (program <= end)
> > 
> >  {
> >  
> >uint8_t opcode = *program++;
> >Dwarf_Word regno;


-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH] Also find CFI in sections of type SHT_X86_64_UNWIND

2018-11-05 Thread Milian Wolff
On Montag, 5. November 2018 00:04:32 CET Mark Wielaard wrote:
> On Mon, 2018-10-29 at 16:21 +0100, Milian Wolff wrote:
> > On my system with g++ (GCC) 8.2.1 20180831 with GNU gold (GNU
> > Binutils
> > 2.31.1) 1.16, the .eh_frame section does not have type PROGBITS
> 
> > but rather is using X86_64_UNWIND nowadays:
> Urgh, who thought it would be a good idea to have a architecture
> specific (instead of a GNU specific) section type for unwind tables...
> And why is there no separate type for .eh_frame_hdr. Now you still need
> to check the name anyway...
> 
> But, given that we have that now, yes, lets deal with it.
> 
> > ```
> > $ echo "int main(){ return 0; }" > test.c
> > $ gcc test.c
> > $ readelf --sections a.out | grep .eh_frame
> >   [14] .eh_frame X86_64_UNWIND0670  0670
> >   [15] .eh_frame_hdr X86_64_UNWIND0724  0724
> > ```
> > 
> > Without this patch, libdw refuses to use the available unwind
> > information, leading to broken backtraces while unwinding. With the
> > patch applied, unwinding works once more in such situations.
> 
> Three questions:
> 
> - What testcase did you use?
>   In theory dwarf_getcfi_elf () should fall back to using phdrs and
>   find the PT_GNU_EH_FRAME data instead.

For unwinding, I used this full example:

https://paste.kde.org/p5rogntox

Then I compiled with `g++ -g O2` and recorded it with perf (`perf record --
call-graph dwarf`). Finally, I looked at the unwinding results via `perf 
script`. I compiled perf from sources with `NO_LIBUNWIND=1` to have it use 
libdw instead.

Interestingly, when I try to reproduce this on my laptop (i.e. compile even 
the trivial C example), then I cannot reproduce this at all anymore - the 
.eh_frame sections show up as PROGBITS. My desktop at work still shows this 
behavior though (also see below). I can't quite explain this difference...

> - It might be better to change the check to shdr->sh_type != SHT_NOBITS
>   The idea is probably that we don't want to look at the data in case
>   this is a .debug file which has it removed. This might be better than
>   adding a check for X86_64_UNWIND since then we would also need to
>   check the arch. Does != SHT_NOBITS work for you?

Yes, since SHT_NOBITS is not equal to SHT_X86_64_UNWIND :)

> - What does eu-readelf -S show?
>   I think we need a x86_64_section_type_name () ebl hook to show it
>   correctly.

Yes, that looks like it:

```
$ cat test.c
int main() { return 0; }
$ gcc test.c
$ readelf -S a.out | grep eh_frame
  [14] .eh_frame X86_64_UNWIND0670  0670
  [15] .eh_frame_hdr X86_64_UNWIND0724  0724
$ eu-readelf -S a.out | grep eh_frame
[14] .eh_frameSHT_LOPROC+1 0670 0670 00b4  0 A  

0   0  8
[15] .eh_frame_hdrSHT_LOPROC+1 0724 0724 002c  0 A  

0   0  4
```

Cheers

-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts

smime.p7s
Description: S/MIME cryptographic signature


Re: [PATCH] Also find CFI in sections of type SHT_X86_64_UNWIND

2018-11-07 Thread Milian Wolff
On Dienstag, 6. November 2018 12:06:57 CET Mark Wielaard wrote:
> Hi Milian,
> 
> On Tue, 2018-11-06 at 00:12 +0100, Milian Wolff wrote:
> > On Montag, 5. November 2018 00:04:32 CET Mark Wielaard wrote:
> > 
> > Interestingly, when I try to reproduce this on my laptop (i.e. compile
> > even
> > the trivial C example), then I cannot reproduce this at all anymore - the
> > .eh_frame sections show up as PROGBITS. My desktop at work still shows
> > this
> > behavior though (also see below). I can't quite explain this difference...
> 
> It seems to only happen with a specific combination of gcc and the gold
> linker, I could only generate the SHT_X86_64_UNWIND sections only on
> fedora 29 with gcc 8.2.1 and gold version 2.31.1-13.fc29 (1.16).

At least on my system, that doesn't seem to be enough. Both my desktop and my 
laptop are running on ArchLinux with the same versions of GCC and Gold, yet 
one shows this behavior while the other one isn't...

What I noticed is that ccache seems to influence it for me, which makes this 
even stranger:

```
$ which gcc
/usr/lib/ccache/bin/gcc
$ gcc --version
gcc (GCC) 8.2.1 20180831
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ gcc test.c
$ readelf -S a.out | grep eh_frame
  [14] .eh_frame_hdr PROGBITS 2004  2004
  [15] .eh_frame PROGBITS 2030  2030

$ /usr/bin/gcc test.c
$ /usr/bin/gcc --version
gcc (GCC) 8.2.1 20180831
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ /usr/bin/gcc test.c
$ readelf -S a.out | grep eh_frame
  [14] .eh_frame X86_64_UNWIND0670  0670
  [15] .eh_frame_hdr X86_64_UNWIND0724  0724
```

Anyhow, that's unrelated to the patches at hand here. See below.

> > > - It might be better to change the check to shdr->sh_type != SHT_NOBITS
> > > 
> > >   The idea is probably that we don't want to look at the data in case
> > >   this is a .debug file which has it removed. This might be better than
> > >   adding a check for X86_64_UNWIND since then we would also need to
> > >   check the arch. Does != SHT_NOBITS work for you?
> > 
> > Yes, since SHT_NOBITS is not equal to SHT_X86_64_UNWIND :)
> 
> OK, then lets change your patch to do that as attached.
> 
> > > - What does eu-readelf -S show?
> > > 
> > >   I think we need a x86_64_section_type_name () ebl hook to show it
> > >   correctly.
> > 
> > Yes, that looks like it:
>
> And the other attached patch should clean that up.

Both patches work for me:

Tested-by: Milian Wolff 

Thanks
-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts

smime.p7s
Description: S/MIME cryptographic signature


Difference between dwarf_getscopes and dwarf_getscopes_die

2020-06-13 Thread Milian Wolff
Hey all,

can someone explain me the difference between dwarf_getscopes and 
dwarf_getscopes_die? Ideally, this should then be added to the documentation 
too.

Currently, the documentation states:

```
/* Return scope DIEs containing PC address.
   Sets *SCOPES to a malloc'd array of Dwarf_Die structures,
   and returns the number of elements in the array.
   (*SCOPES)[0] is the DIE for the innermost scope containing PC,
   (*SCOPES)[1] is the DIE for the scope containing that scope, and so on.
   Returns -1 for errors or 0 if no scopes match PC.  */
extern int dwarf_getscopes (Dwarf_Die *cudie, Dwarf_Addr pc,
Dwarf_Die **scopes);

/* Return scope DIEs containing the given DIE.
   Sets *SCOPES to a malloc'd array of Dwarf_Die structures,
   and returns the number of elements in the array.
   (*SCOPES)[0] is a copy of DIE.
   (*SCOPES)[1] is the DIE for the scope containing that scope, and so on.
   Returns -1 for errors or 0 if DIE is not found in any scope entry.  */
extern int dwarf_getscopes_die (Dwarf_Die *die, Dwarf_Die **scopes);
```

Most notably, both say:

```
   (*SCOPES)[1] is the DIE for the scope containing that scope, and so on.
```

But in practice, there seems to be a difference. Inspired by the code form eu-
addr2line we resolve inline frames like this:

```
Dwarf_Addr bias = 0;
Dwarf_Die *cudie = dwfl_module_addrdie(module, ip, &bias);

Dwarf_Die *subroutine = nullptr;
Dwarf_Die *scopes = nullptr;
int nscopes = dwarf_getscopes(cudie, ip - bias, &scopes);
for (int i = 0; i < nscopes; ++i) {
Dwarf_Die *scope = &scopes[i];
const int tag = dwarf_tag(scope);
if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
subroutine = scope;
break;
}
}

Dwarf_Die *scopes_die = nullptr;
int nscopes_die = dwarf_getscopes_die(subroutine, &scopes_die);
for (int i = 0; i < nscopes_die; ++i) {
Dwarf_Die *scope = &scopes_die[i];
const int tag = dwarf_tag(scope);
if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
// do stuff
}
}

free(scopes_die);
free(scopes);
```

In the above, subroutine points to somewhere in dwarf_getscopes. As such, 
passing it to dwarf_getscopes_die, one would assume from the documentation 
(see above) that we get the sames scopes we already found via 
`dwarf_getscopes` (with an offset), but that is clearly no the case. Here is 
an example:

```
dwarf_getscopes: n = 2
i = 1, tag = 0x11, name = "../../../manual/clients/vector.cpp"
i = 0, tag = 0x1d, name = "_ZN9__gnu_cxx13new_allocatorIdE10deallocateEPdm"
```

Then, we use the DIE at i = 0 from above to call dwarf_getscopes_die, which 
yields:

```
dwarf_getscopes_die, n = 6
i = 5, tag = 0x11, name = "../../../manual/clients/vector.cpp"
i = 4, tag = 0x2e, name = 
"_ZNSt6vectorIdSaIdEE17_M_realloc_insertIJdEEEvN9__gnu_cxx17__normal_iteratorIPdS1_EEDpOT_"
i = 3, tag = 0x1d, name = "_ZNSt12_Vector_baseIdSaIdEE13_M_deallocateEPdm"
i = 2, tag = 0x0b
i = 1, tag = 0x1d, name =  
"_ZNSt16allocator_traitsISaIdEE10deallocateERS0_Pdm"
i = 0, tag = 0x1d, name = "_ZN9__gnu_cxx13new_allocatorIdE10deallocateEPdm"
```

As we can see, dwarf_getscopes_die returns more scopes than dwarf_getscopes. 
For me, the documentation for `dwarf_getscopes` is confusing. I would have 
expected that it should be a superset of dwarf_getscopes_die, but apparently 
that is not the case.

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Can dwarf_getscopes{,_die} performance be improved?

2020-06-13 Thread Milian Wolff
Hey all!

In perfparser we are running into a performance issue with elfutils when we 
try to resolve inline frames. We are following the procedure outlined by eu-
addr2line, i.e. for every IP we basically do:

```
Dwarf_Addr bias = 0;
Dwarf_Die *cudie = dwfl_module_addrdie(module, ip, &bias);

Dwarf_Die *subroutine = nullptr;
Dwarf_Die *scopes = nullptr;
int nscopes = dwarf_getscopes(cudie, ip - bias, &scopes);
for (int i = 0; i < nscopes; ++i) {
Dwarf_Die *scope = &scopes[i];
const int tag = dwarf_tag(scope);
if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
subroutine = scope;
break;
}
}

Dwarf_Die *scopes_die = nullptr;
int nscopes_die = dwarf_getscopes_die(subroutine, &scopes_die);
for (int i = 0; i < nscopes_die; ++i) {
Dwarf_Die *scope = &scopes_die[i];
const int tag = dwarf_tag(scope);
if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
// do stuff
}
}

free(scopes_die);
free(scopes);
```

Profiling shows that both, the calls to dwarf_getscopes and 
dwarf_getscopes_die can take a really long time. I have seen cases (in 
libclang.so.11) where a single call can take up to ~50ms on a fast desktop 
machine (Ryzen 3900X CPU, fast SSD, 32GB of RAM).

Now while 50ms may not sound sound too problematic, we have to repeat these 
calls for every IP we encounter in a perf.data file. We already apply heavy 
caching, but even then we can easily encounter tens of thousands of individual 
addresses, which can add up to minutes or even hours of time required to 
process the data - only to get information on inlined frames.

I have learned that the DWARF format is mostly meant for efficient storage and 
that one cannot assume that it is efficient for such mass post processing. 
This is the reason why I'm writing this email:

Has anyone an idea on how to to post-process the DWARF data to optimize the 
lookup of inlined frames?

Or is there some room for optimizations / caching within elfutils to amortize 
the repeated DWARF hierarchy walking that happens when one calls 
dwarf_getscopes{,_die}? From what I'm understanding, both calls will start a 
top-down search within the CU DIE via __libdw_visit_scopes. Once a match is 
found, the DIE scope chain is reported. I guess many times (parts of the) DIE 
scope chain will be shared across different IPs, but I don't see any way to 
leverage this to speed up the processing task.

Thanks, any input would be welcome
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Can dwarf_getscopes{,_die} performance be improved?

2020-06-22 Thread Milian Wolff
On Montag, 15. Juni 2020 18:54:41 CEST Josh Stone wrote:
> On 6/13/20 10:40 AM, Milian Wolff wrote:
> > Has anyone an idea on how to to post-process the DWARF data to optimize
> > the
> > lookup of inlined frames?
> 
> SystemTap implements its own cache for repeated lookups -- see
> dwflpp::get_die_parents().

Thanks, I've come up with something similar over the weekend before reading 
your mail. The performance boost is huge (5x and more).

Looking at your code, I think that I'm not yet handling a few corner cases 
(such as imported units). That, paired with the fact that at least three users 
of this API have apparently by now come up with a similar solution clearly 
makes a case for upstreaming this into a common API.

As Mark wrote:

> It would probably make sense to build a parent DIE chain cache for a
> CU. The question is when/how we build the cache. There are also lots of
> programs that won't need it, or only for some, but not all CUs. It will
> take space and time to create.
> 
> The dwarf_getscopes[_die] calls might be a good trigger to create/keep
> them. And/or have some explicit way to create them, maybe triggered by
> some helper function to get at the parent of a DIE.

I believe that there is a lot of data that potentially needs to be cached. 
Additionally, doing it behind the scenes may raise questions regarding multi 
threaded usage of the API (see my other mail relating to that).

Which means: an explicit API to opt-in to this behavior is safest and best I 
believe. Maybe something as simple as the following would be sufficient?

```
/* Cache parent DIEs chain to speed up repeated dwarf_getscopes calls.

   Returns -1 for errors or 0 if the parent chain was cached already. */
extern int dwarf_cache_parent_dies(Dwarf_Die *cudie);
```

Alternatively a function that returns the cache could be considered, which 
would then require new versions of dwarf_getscopes* that take the cache as an 
argument.

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


0x1000 offset in symbol resolution?

2020-12-30 Thread Milian Wolff
Hey all,

I stumbled upon a strange behavior while symbolizing a perf.data file with 
hotspot/perfparser/elfutils that I have trouble understanding. I hope it's ok 
to send this call for help here.

I'm running with elfutils 0.181, g++ 10.2, glibc 2.32 on archlinux with kernel 
5.9.14.

Code for the executable:
https://github.com/KDAB/hotspot/blob/master/tests/test-clients/cpp-inlining/
main.cpp

Then compile it and run with perf:

```
$ g++ -O2 -g main.cpp
$ perf record ./a.out
$ perf script --show-mmap-events | grep -E 'main|MMAP' | head
   a.out 108684 15892.199058: PERF_RECORD_MMAP2 108684/108684: 
[0x561875084000(0x1000) @ 0 fe:00 7997193 372428903]: r-xp /home/milian/
projects/kdab/rnd/hotspot/build/a.out
...
   a.out 108684 15892.200561: 391517 cycles:u:  561875084c1e 
main+0x1de (/home/milian/projects/kdab/rnd/hotspot/build/a.out)
```

The first MMAP event show us that there's a mapping starting at 0x561875084000 
with a size of 0x1000 that belongs to the main executable, pgoff is 0.

Later on, we see e.g. the instruction pointer address 0x561875084c1e being 
somehow mapped to main+0x1de by perf using the binutils libraries.

What's odd is that this should give us the offset of main:

```
0x561875084c1e - 0x561875084000 = 0xc1e
0xc1e - 0x1de = 0xa40
```

But look at what we get from nm:

```
$ eu-nm ./a.out | grep main
main |
1a40|GLOBAL|FUNC|02e5|main.cpp:33|.text
```

Note that the address in the symbol table is actually 0x1000 offset from the 
0xa40 value we computed above... Can anyone explain that?

GDB shows a similar behavior:

```
$ gdb a.out
(gdb) b main
Breakpoint 1 at 0x1a40: file ../tests/test-clients/cpp-inlining/main.cpp, line 
34.
(gdb) r
Starting program: /home/milian/projects/kdab/rnd/hotspot/build/a.out 

Breakpoint 1, main () at ../tests/test-clients/cpp-inlining/main.cpp:34
34  {
(gdb) p/x $rip
$1 = 0x5a40
(gdb) p/x &main
$2 = 0x5a40
(gdb) info proc mappings 
process 109516
Mapped address spaces:

  Start Addr   End Addr   Size Offset objfile
  0x4000 0x5000 0x10000x0 /home/milian/
projects/kdab/rnd/hotspot/build/a.out
  0x5000 0x6000 0x10000x0 /home/milian/
projects/kdab/rnd/hotspot/build/a.out
  0x6000 0x8000 0x20000x0 /home/milian/
projects/kdab/rnd/hotspot/build/a.out
  0x8000 0x9000 0x1000 0x1000 /home/milian/
projects/kdab/rnd/hotspot/build/a.out
...
(gdb) 
```

So here, the address `0x5a40` should come from the second mapping, 
which has an offset 0x0 and starts at `0x5000` and has a size of 
`0x1000`. How can that possibly map to the `main` symbol which has an offset 
of `0x1a40`?

Does anyone know what's going on here?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: 0x1000 offset in symbol resolution?

2021-01-11 Thread Milian Wolff
On Montag, 11. Januar 2021 17:05:30 CET Mark Wielaard wrote:
> Hi Milian,
> 
> On Wed, 2020-12-30 at 16:51 +0100, Milian Wolff wrote:
> > I stumbled upon a strange behavior while symbolizing a perf.data file
> > with
> > hotspot/perfparser/elfutils that I have trouble understanding. I hope it's
> > ok to send this call for help here.
> > 
> > I'm running with elfutils 0.181, g++ 10.2, glibc 2.32 on archlinux with
> > kernel 5.9.14.
> > [...]
> > So here, the address `0x5a40` should come from the second mapping,
> > which has an offset 0x0 and starts at `0x5000` and has a size of
> > `0x1000`. How can that possibly map to the `main` symbol which has an
> > offset of `0x1a40`?
> > 
> > Does anyone know what's going on here?
> 
> Isn't this simply the ELF file being loaded/mmapped in separate
> (overlapping) chunks?

Ah, indeed - thanks for this question! It actually seems like perf only 
records this one single mmap event. I.e.:

```
$ perf record ./a.out
$ perf script --show-mmap-events | grep -E 'MMAP.*a.out' 
   a.out 139334 16992.223215: PERF_RECORD_MMAP2 139334/139334: 
[0x55bb21e7(0x1000) @ 0 fe:00 8009266 4286144336]: r-xp /home/milian/
projects/kdab/rnd/hotspot/build/a.out
```

And that is probably the bug. Because when I run the following slightly 
changed perf session, the issue doesn't occur - and I see more mmap events 
too:

```
$ perf record --call-graph dwarf ./a.out
$ perf script --show-mmap-events 2>/dev/null | grep -E 'MMAP.*a.out' 
a.out 139453 17065.223302: PERF_RECORD_MMAP2 139453/139453: 
[0x5589a3711000(0x5000) @ 0 fe:00 8009266 4286144336]: r--p /home/milian/
projects/kdab/rnd/hotspot/build/a.out
a.out 139453 17065.223305: PERF_RECORD_MMAP2 139453/139453: 
[0x5589a3712000(0x1000) @ 0 fe:00 8009266 4286144336]: r-xp /home/milian/
projects/kdab/rnd/hotspot/build/a.out
a.out 139453 17065.223306: PERF_RECORD_MMAP2 139453/139453: 
[0x5589a3713000(0x2000) @ 0 fe:00 8009266 4286144336]: rw-p /home/milian/
projects/kdab/rnd/hotspot/build/a.out
a.out 139453 17065.223307: PERF_RECORD_MMAP2 139453/139453: 
[0x5589a3715000(0x1000) @ 0x1000 fe:00 8009266 4286144336]: rw-p /home/milian/
projects/kdab/rnd/hotspot/build/a.out
a.out 139453 17065.223994: PERF_RECORD_MMAP2 139453/139453: 
[0x5589a3713000(0x2000) @ 0 fe:00 8009266 4286144336]: r--p /home/milian/
projects/kdab/rnd/hotspot/build/a.out
```

Thank you Mark, I'll take this over to the perf mailing list, as it doesn't 
seem to be a problem with elfutils.

> What does eu-readelf -l show?

Probably irrelevant now, but here is the output nevertheless:

```
Program Headers:
  Type   Offset   VirtAddr   PhysAddr   FileSiz  
MemSiz   Flg Align
  PHDR   0x40 0x0040 0x0040 0x000268 
0x000268 R   0x8
  INTERP 0x0002a8 0x02a8 0x02a8 0x1c 
0x1c R   0x1
[Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
  LOAD   0x00 0x 0x 0x000934 
0x000934 R   0x1000
  LOAD   0x000940 0x1940 0x1940 0x000530 
0x000530 R E 0x1000
  LOAD   0x000e70 0x2e70 0x2e70 0x000218 
0x000218 RW  0x1000
  LOAD   0x001088 0x4088 0x4088 0x50 
0x0001c8 RW  0x1000
  DYNAMIC0x000e88 0x2e88 0x2e88 0x0001d0 
0x0001d0 RW  0x8
  GNU_RELRO  0x000e70 0x2e70 0x2e70 0x000218 
0x001190 R   0x1
  GNU_EH_FRAME   0x000828 0x0828 0x0828 0x34 
0x34 R   0x4
  GNU_STACK  0x00 0x 0x 0x00 
0x00 RW  0x0
  NOTE   0x0002c4 0x02c4 0x02c4 0x38 
0x38 R   0x4

 Section to Segment mapping:
  Segment Sections...
   00 
   01  [RO: .interp]
   02  [RO: .interp .note.ABI-tag .note.gnu.build-id .dynsym .gnu.version 
.gnu.version_r .gnu.hash .dynstr .rela.dyn .rela.plt .rodata .eh_frame_hdr 
.eh_frame]
   03  [RO: .text .init .fini .plt]
   04  [RELRO: .fini_array .init_array .dynamic .got]
   05  .data .got.plt .bss
   06  [RELRO: .dynamic]
   07  [RELRO: .fini_array .init_array .dynamic .got]
   08  [RO: .eh_frame_hdr]
   09 
   10  [RO: .note.ABI-tag .note.gnu.build-id]
```

> Is the address/offset range loaded/mmapped executable?

See above, I now believe the problem is the lack of reported mmap event to 
begin with.

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Questions regarding debuginfod.h API

2022-04-08 Thread Milian Wolff
Hey all,

now that archlinux is supporting debuginfod, I have finally tried it out. It's 
such a game changer, many thanks for everyone involved in working on this!

Now to my question: In applications using elfutils, we will now automatically 
download debug information when DEBUGINFOD_URLS is defined. But doing that can 
take a very long time. I would like to give the user feedback about this 
situation and ideally provide a means to cancel the download.

Looking at `debuginfod.h` I `debuginfod_set_progressfn` which looks ideal. But 
how do I access the default `debuginfod_client *` which seems to exist without 
me ever calling `debuginfod_begin` anywhere? Or do I need to create a new 
client here via `debuginfod_begin`?

Then, how would I cancel an ongoing download job? GDB seems to do that when I 
press `CTRL+C`, but I do not see any API I could use for that purpose?

Finally, what is the recommended way to check whether debuginfod is available? 
Should one rely on the build system to discover the `debuginfod.h` header 
file, or is some other compile time check suggested to detect it? I'm asking, 
because while I want to add the above API to get a better debuginfod 
experience, I do not necessarily want to raise the minimum elfutils version 
requirement just for that purpose.

Many thanks again!
-- 
Milian Wolff
http://milianw.de




Re: Questions regarding debuginfod.h API

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 15:44:32 CEST Frank Ch. Eigler wrote:
> Hi -



> > Now to my question: In applications using elfutils, we will now
> > automatically download debug information when DEBUGINFOD_URLS is
> > defined. But doing that can take a very long time.
> 
> (See also the DEBUGINFOD_MAXTIME and DEBUGINFOD_MAXSIZE env vars
> that can limit this.)

I did come across those, but what are suggested best practices in setting 
those? When using GDB or a profiler on larger non-trivial UI applications on 
Linux for the first time, we would start to download dozens of debug packages, 
taking hundreds of megabytes of space. This simply takes time (in total), 
whereas the two environment variables above would be per-library, no? And 
usually one does not know a priori which libraries one needs - the size is not 
really a good criterium to decide whether to download a libraries debug 
information or not...

I'm not really complaining here - just thinking out loud. I believe for novice 
users the status quo is acceptable - they'll just need to wait for the 
download to finish (potentially taking minutes). More advanced users may want 
to manually skip some (larger) libs that they know they won't use for whatever 
they are looking at...

> > I would like to give the user feedback about this situation and
> > ideally provide a means to cancel the download.
> 
> OK.
> 
> > Looking at `debuginfod.h` I `debuginfod_set_progressfn` which looks
> > ideal. But how do I access the default `debuginfod_client *` which
> > seems to exist without me ever calling `debuginfod_begin` anywhere?
> > Or do I need to create a new client here via `debuginfod_begin`?
> 
> You do need to create a new client object.  You can reuse it.

Will the default code that uses debuginfod from within dwfl then pick up my 
new client object and use that? I feel like there's a fundamental confusion 
about this on my side about this. More on that at the end of this mail below.

> > Then, how would I cancel an ongoing download job? GDB seems to do
> > that when I press `CTRL+C`, but I do not see any API I could use for
> > that purpose?
> 
> See [man debuginfod_set_progressfn].  The return code from that
> progressfn callback is the ticket.

```
$ man debuginfod_set_progressfn
No manual entry for debuginfod_set_progressfn
```

My `debuginfod.h` also does not show any (useful) inline API documentation for 
most of that file. Could this please be improved? The doxygen for dwfl is 
great and can be read directly together with the code, I never used `man` for 
that purpose in the past?

> > Finally, what is the recommended way to check whether debuginfod is
> > available?  Should one rely on the build system to discover the
> > `debuginfod.h` header file, or is some other compile time check
> > suggested to detect it?  [...]
> 
> To decide whether or not to compile in support, you'd need a
> compile-time check such as for the debuginfod.h header.  (Alternately,
> you could opt not to compile in support at all, and instead call out
> to the debuginfod-find(1) program.)  To decide at run time whether or
> not to use it, you could just use the *_find APIs and get back an
> error code if things are not set up.  Or you can check the
> DEBUGINFOD_URLS for being set/unset before you call in.

I don't see where I would use debuginfod-find or any of the 
`debuginfod_find_*` API directly. I'm using dwfl which uses debuginfod 
internally. I guess that happens via the `dwfl_*find_*` standard callbacks 
provided in `libdwfl.h`. That works perfectly fine. I simply want a bit more 
control over the debuginfod usage that happens internally in dwfl.

Is maybe a `debuginfod_client *dwfl_debuginfod(void)` function missing that 
gives access to the client used by dwfl internally?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Questions regarding debuginfod.h API

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 21:44:32 CEST Frank Ch. Eigler wrote:
> Hi -
> 
> > > (See also the DEBUGINFOD_MAXTIME and DEBUGINFOD_MAXSIZE env vars
> > > that can limit this.)
> > 
> > I did come across those, but what are suggested best practices in
> > setting those? When using GDB or a profiler on larger non-trivial UI
> > applications on Linux for the first time, we would start to download
> > dozens of debug packages, taking hundreds of megabytes of
> > space. [...]
> 
> Yes, and each of those downloads can be limited by these criteria.  An
> application such as gdb could adds its own throttling on top, if it is
> going to do a long series of downloads.  (Alternately, gdb can try not
> to download all this stuff early; we're investigating gdbindex-only
> fetch operations.)
> 
> > > > Looking at `debuginfod.h` I `debuginfod_set_progressfn` which looks
> > > > ideal. But how do I access the default `debuginfod_client *` which
> > > > seems to exist without me ever calling `debuginfod_begin` anywhere?
> > > > Or do I need to create a new client here via `debuginfod_begin`?
> > > 
> > > You do need to create a new client object.  You can reuse it.
> > 
> > Will the default code that uses debuginfod from within dwfl then pick up
> > my
> > new client object and use that? I feel like there's a fundamental
> > confusion
> > about this on my side about this. More on that at the end of this mail
> > below.
>
> Ah yes, I didn't realize you were talking about the hidden debuginfod
> client usage in libdwfl.  Yes, you have not much control over that,
> because it is   hidden & automatic.  There is a
> DEBUGINFOD_PROGRESS env var with which it can activate some
> notification to stderr, but that's about it.  No API to get at the
> internal transient objects.
> 
> If you wish to take control of this part, you could probably still use
> dwfl.  You would do all the debuginfod client API calls yourself, then
> "report" the dwarf file content to dwfl via dwarf_begin_elf(),
> dwarf_begin(), dwarf_setalt() etc.

OK thank you, that is helpful. Would you say adding dwfl API to get access to 
the internal debuginfod client would be a good path forward? I guess more 
projects would potentially like to benefit from the ready made integration, 
but add a bit of control on top of it?

> > ```
> > $ man debuginfod_set_progressfn
> > No manual entry for debuginfod_set_progressfn
> > ```
> 
> Well go complain to your distro for this packaging bug. :-)
> It's an alias of [man debuginfod_find_debuginfo].

Found it now - it's packaged together with `debuginfod`, even though it should 
be part of `elfutils`.

> > My `debuginfod.h` also does not show any (useful) inline API
> > documentation for most of that file. Could this please be improved?
> > The doxygen for dwfl is great and can be read directly together with
> > the code,
> 
> As they say, patches welcome. :-) The header contains some curt
> comments documenting each function.

Would you be OK with me simply copying over the contents from the man page 
over to doxygen? Or is there a better process in place to prevent such kind of 
documentation duplication? I would have expected that the man pages for API 
documentation are generated from e.g. doxygen which does not seem to be the 
case here?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


caching failed lookups of debuginfo?

2022-04-08 Thread Milian Wolff
Hey all,

another debuginfod related question, but unrelated to the other thread I 
started earlier today. In a work branch I have ported my heaptrack profiler 
over to elfutils. I have then run the analyzer that uses elfutils (and thus 
debuginfod internally via dwfl) on a recorded data file to have it download 
all the debug info files it can find.

Now, on consecutive runs, nothing should be downloaded anymore. But I notice 
that there is still a huge slowdown incurred when enabled `DEBUGINFOD_URLS`:

With DEBUGINFOD_URLS set:
```
perf stat ./heaptrack_interpret ...
  5,765.36 msec task-clock#0.707 CPUs utilized  

...
   8.154873275 seconds time elapsed

   5.477158000 seconds user
   0.271662000 seconds sys
```

Without DEBUGINFOD_URLS set:
```
perf stat ./heaptrack_interpret ...

  2,460.09 msec task-clock#0.999 CPUs utilized  

...
   2.461661784 seconds time elapsed

   2.334429000 seconds user
   0.119698000 seconds sys
```

Note the significantly increased wall-time in the first case. The second case 
(without debuginfod) has basically zero off-CPU time and significantly reduced 
on-CPU time too.

Looking at the off-CPU time flame graph, and enabling verbose debuginfod 
output lets me come to the conclusion, that the issue above is due to some 
libraries simply not having any debug information uploaded. I.e. I see dozens 
of reports such as:

```
debuginfod_find_debuginfo 2e245c2bf12f95fd8ab79b3a4be99524677cbd70
server urls "https://debuginfod.archlinux.org/";
checking build-id
checking cache dir /home/milian/.debuginfod_client_cache
using timeout 90
init server 0 https://debuginfod.archlinux.org/buildid
url 0 https://debuginfod.archlinux.org/buildid/
2e245c2bf12f95fd8ab79b3a4be99524677cbd70/debuginfo
query 1 urls in parallel
Downloading |
server response HTTP response code said error
url 0 The requested URL returned error: 404
not found No such file or directory (err=-2)
```

These negative lookups are not cached. Meaning rerunning the same process 
using dwfl and debuginfod on the same data would always incur a significant 
slowdown, as we would again and again try to look for something that's not 
there. The lookups take roughly ~200ms for me to realize the data is not on 
the server.

What's worse, I'm seeing multiple lookups for the same buildid *within the 
same process*. I.e.:

```
export DEBUGINFOD_VERBOSE=1
./heaptrack_interpret ... |& egrep "^url 0 https" | sort | uniq -c | sort
...
  6 url 0 https://debuginfod.archlinux.org/buildid/
7f4b16b4b407cbae2d7118d6f99610e29a18a56a/debuginfo
  8 url 0 https://debuginfod.archlinux.org/buildid/
c09c6f50f6bcec73c64a0b4be77eadb8f7202410/debuginfo
 14 url 0 https://debuginfod.archlinux.org/buildid/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo
```

Here, we are paying roughly `14 * 0.2s = 2.8s` just for a single library.

Can we find a way to improve this situation somehow generically? I would 
personally even be OK with caching the 404 error case locally for some time 
(say, one hour or one day or ...). Then at least we would at most pay this 
cost once per library, and not multiple times. And rerunning the analysis a 
second time would become much faster again.

Was there a deliberate decision against caching negative server side lookups?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: caching failed lookups of debuginfo?

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 22:05:27 CEST Frank Ch. Eigler wrote:
> Hi -
> 
> > another debuginfod related question, but unrelated to the other thread I
> > started earlier today. In a work branch I have ported my heaptrack
> > profiler
> > over to elfutils. I have then run the analyzer that uses elfutils (and
> > thus
> > debuginfod internally via dwfl) on a recorded data file to have it
> > download
> > all the debug info files it can find.
> 
> Nice.
> 
> > These negative lookups are not cached. Meaning rerunning the same process
> > using dwfl and debuginfod on the same data would always incur a
> > significant
> > slowdown, as we would again and again try to look for something that's not
> > there. The lookups take roughly ~200ms for me to realize the data is not
> > on
> > the server.
> 
> That's not correct, as of elfutils 0.184 (commit 5f72c51a7e5c0),
> with some more recent tweaks in (commit 7d64173fb11c6).

I am running elfutils 0.186 here and I definitely saw this happening (that's 
where the output from my previous mail came from after all). I've now wiped my 
local debuginfod cache and tried to replicate it, but magically the repeated 
lookups are gone. This is very mysterious...

Nevertheless, I clearly still see a significant overhead of having 
DEBUGINFOD_URLS set, even after the first run that should have populated the 
cache, no? I.e. check this out:

```
$ export DEBUGINFOD_URLS=https://debuginfod.archlinux.org/
$ perf stat -r 5 ./heaptrack_interpret ...
  5,993.67 msec task-clock#0.696 CPUs utilized  
  
( +-  0.22% )
   301  context-switches  #   50.567 /sec   
  
( +-  7.47% )
 3  cpu-migrations#0.504 /sec   
  
( +- 19.44% )
   145,574  page-faults   #   24.456 K/sec  
  
( +-  0.01% )
25,026,712,955  cycles#4.204 GHz
  
( +-  0.14% )  (83.27%)
   357,786,453  stalled-cycles-frontend   #1.43% frontend cycles 
idle ( +-  1.77% )  (83.39%)
 4,851,145,553  stalled-cycles-backend#   19.44% backend cycles 
idle  ( +-  0.99% )  (83.35%)
64,893,827,473  instructions  #2.60  insn per cycle 

  #0.07  stalled cycles 
per insn  ( +-  0.05% )  (83.29%)
14,234,732,114  branches  #2.391 G/sec  
  
( +-  0.04% )  (83.30%)
   111,326,964  branch-misses #0.78% of all branches
  
( +-  0.17% )  (83.44%)

8.6057 +- 0.0667 seconds time elapsed  ( +-  0.77% )
```

versus:

```
$ unset DEBUGINFOD_URLS
$ perf stat -r 5 ./heaptrack_interpret ...
  2,505.36 msec task-clock#1.009 CPUs utilized  
  
( +-  0.48% )
   542  context-switches  #  218.486 /sec   
  
( +- 17.87% )
 1  cpu-migrations#0.403 /sec   
  
( +- 24.49% )
50,712  page-faults   #   20.443 K/sec  
  
( +-  0.02% )
10,821,713,676  cycles#4.362 GHz
  
( +-  0.15% )  (83.24%)
   129,975,157  stalled-cycles-frontend   #1.20% frontend cycles 
idle ( +-  4.86% )  (83.37%)
 1,719,256,978  stalled-cycles-backend#   15.93% backend cycles 
idle  ( +-  1.33% )  (83.38%)
28,947,652,758  instructions  #2.68  insn per cycle 

  #0.06  stalled cycles 
per insn  ( +-  0.03% )  (83.40%)
 6,118,310,875  branches  #2.466 G/sec  
  
( +-  0.03% )  (83.38%)
46,907,166  branch-misses #0.77% of all branches
  
( +-  0.16% )  (83.30%)

2.4833 +- 0.0124 seconds time elapsed  ( +-  0.50% )
```

There is clearly still a significant slowdown imposed by having debuginfod 
enabled - even after the first run. Is this expected? My assumption was: Only 
the first run would be potentially slow, then everything is cached locally.

In the off-CPU flame graphs I clearly see this callchain when DEBUGINFOD_URLS 
is set:

```
dwfl_standard_find_debuginfo > debuginfod_query_server > curl_multi_wait
```

This accounts for ~2.5s of off-CPU time in my case. If you are saying that 
negative lookups are cached, then what is this? Why would a second run of the 
same dwfl-using application with the same input data keep querying the server?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


parallel downloads of multiple debuginfo files

2022-04-08 Thread Milian Wolff
Hey all,

one more debuginfod question: Would it be possible to extend the API to allow 
downloading of multiple debug info files in parallel?

The `debuginfod_find_*` API currently only supports looking at multiple server 
urls in parallel. I would like to ask multiple files in parallel.

The use case would be profiling tools like the ones I'm working on 
(perfparser, heaptrack, ...). There, you usually have a list of libraries that 
you know you'll hit sooner or later. Thus it would be more efficient to start 
downloading all debug information files in parallel directly. At least 
compared to the current status quo with dwfl where we would iteratively find 
individual debug information files, each time blocking the process.

Parallel downloads from a single server would definitely increase the load 
there though, when done naively. Have you thought about adding support for 
something like HTTP/3's ability to download mutiple assets over a single 
connection (maybe even in parallel)?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: parallel downloads of multiple debuginfo files

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 22:54:35 CEST Frank Ch. Eigler wrote:
> Hi -
> 
> > one more debuginfod question: Would it be possible to extend the API
> > to allow downloading of multiple debug info files in parallel?  The
> > `debuginfod_find_*` API currently only supports looking at multiple
> > server urls in parallel. I would like to ask multiple files in
> > parallel.
> 
> Spin up N threads, do one set of debuginfod_begin/_find/_end ops in each.
> Bob is your uncle!

OK, got it.

But once again - isn't this a problem that everyone using dwfl is going to 
encounter? Should we not try to find a general solution to this problem and 
fix it for all consumers of the API?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: caching failed lookups of debuginfo?

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 22:59:55 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Fri, Apr 08, 2022 at 10:45:10PM +0200, Milian Wolff wrote:
> > In the off-CPU flame graphs I clearly see this callchain when
> > DEBUGINFOD_URLS is set:
> > 
> > ```
> > dwfl_standard_find_debuginfo > debuginfod_query_server > curl_multi_wait
> > ```
> > 
> > This accounts for ~2.5s of off-CPU time in my case. If you are saying that
> > negative lookups are cached, then what is this? Why would a second run of
> > the same dwfl-using application with the same input data keep querying
> > the server?
>
> That is certainly not what should happen once the cache is filled.
> Could you run with DEBUGINFOD_VERBOSE=1 to see what is being fetched and
> why?

My first mail showed this already. But here is one such block:

```
debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
server urls "https://debuginfod.archlinux.org/";
checking build-id
checking cache dir /home/milian/.cache/debuginfod_client
using timeout 90
init server 0 https://debuginfod.archlinux.org/buildid
url 0 https://debuginfod.archlinux.org/buildid/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo
query 1 urls in parallel
server response HTTP response code said error
url 0 The requested URL returned error: 404
not found No such file or directory (err=-2)
```

I see this repeated multiple times - both for different files and for the same 
file. I.e.:

```
$ /home/milian/projects/compiled/other/lib/heaptrack/libexec/
heaptrack_interpret < heaptrack.kate.52068.raw |& egrep "^url 0 https" | sort 
| uniq -c | sort -n
```

Produces this output here locally for me: https://invent.kde.org/-/snippets/
2153

I can reproduce it now suddenly with debuginfod-find too:

```
$
debuginfod-find debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
server urls "https://debuginfod.archlinux.org/";
checking build-id
checking cache dir /home/milian/.cache/debuginfod_client
using timeout 90
init server 0 https://debuginfod.archlinux.org/buildid
url 0 https://debuginfod.archlinux.org/buildid/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo
query 1 urls in parallel
server response HTTP response code said error
url 0 The requested URL returned error: 404
not found No such file or directory (err=-2)
Server query failed: No such file or directory
```

I do see an empty `/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo` file. But the server is 
still queried (i.e. rerunning the above command always produces the same 
output for me). The lookup is costly too at ~70ms overall:

```
$ perf stat -r 5 debuginfod-find debuginfo 
85766e9d8458b16e9c7ce6e07c712c02b8471dbc
  7.83 msec task-clock#0.114 CPUs utilized  
  
( +-  4.75% )
...
   0.06857 +- 0.00187 seconds time elapsed  ( +-  2.73% )

```

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: caching failed lookups of debuginfo?

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 23:34:06 CEST Aaron Merey wrote:
> Hi Milian,
> 
> On Fri, Apr 8, 2022 at 5:08 PM Milian Wolff  wrote:
> > I can reproduce it now suddenly with debuginfod-find too:
> > 
> > ```
> > $
> > debuginfod-find debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
> > debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
> > server urls "https://debuginfod.archlinux.org/";
> > checking build-id
> > checking cache dir /home/milian/.cache/debuginfod_client
> > using timeout 90
> > init server 0 https://debuginfod.archlinux.org/buildid
> > url 0 https://debuginfod.archlinux.org/buildid/
> > 85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo
> > query 1 urls in parallel
> > server response HTTP response code said error
> > url 0 The requested URL returned error: 404
> > not found No such file or directory (err=-2)
> > Server query failed: No such file or directory
> > ```
> > 
> > I do see an empty `/home/milian/.cache/debuginfod_client/
> > 85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo` file. But the server
> > is
> > still queried (i.e. rerunning the above command always produces the same
> 
> > output for me). The lookup is costly too at ~70ms overall:
> I am not able to reproduce this on Fedora 35 using commit 8db849976f070.
> 
> The first time I run debuginfod-find I get similar output to you:
> ```
> debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
> server urls "https://debuginfod.archlinux.org/";
> checking build-id
> checking cache dir /home/amerey/.cache/debuginfod_client
> using timeout 90
> init server 0 https://debuginfod.archlinux.org/buildid
> url 0
> https://debuginfod.archlinux.org/buildid/85766e9d8458b16e9c7ce6e07c712c02b8
> 471dbc/debuginfo query 1 urls in parallel
> server response HTTP response code said error
> url 0 The requested URL returned error: 404
> not found No such file or directory (err=-2)
> Server query failed: No such file or directory
> ```
> 
> But when I rerun debuginfod-find I get output indicating that the cache was
> checked:
> ```
> debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc
> server urls "https://debuginfod.archlinux.org/";
> checking build-id
> checking cache dir /home/amerey/.cache/debuginfod_client
> not found No such file or directory (err=-2)
> Server query failed: No such file or directory
> ```
> 
> No calls to curl_multi_wait occur. Is there a "cache_miss_s" file in the top
> level of your debuginfod cache? It should contain the number of seconds
> that must elapse after a failed attempt before another query would be
> attempted (default is 600 seconds).

Yes that file exists and it contains 600.

Running through strace, I see that it actually removes the empty file first. 
This looks totally bogus:

```
strace -e file -f debuginfod-find debuginfo 
85766e9d8458b16e9c7ce6e07c712c02b8471dbc |& grep "cache/debuginfod_client"
checking cache dir /home/milian/.cache/debuginfod_client
newfstatat(AT_FDCWD, "/home/milian/.cache/debuginfod_client", 
{st_mode=S_IFDIR|0755, st_size=12288, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
cache_clean_interval_s", {st_mode=S_IFREG|0644, st_size=5, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
cache_clean_interval_s", {st_mode=S_IFREG|0644, st_size=5, ...}, 0) = 0
openat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
cache_clean_interval_s", O_RDONLY) = 5
newfstatat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo", {st_mode=S_IFREG|000, 
st_size=0, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/cache_miss_s", 
{st_mode=S_IFREG|0644, st_size=3, ...}, 0) = 0
openat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/cache_miss_s", 
O_RDONLY) = 5
unlink("/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo") = 0
openat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo", O_RDONLY) = -1 ENOENT (No 
such file or directory)
mkdir("/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc", 0700) = -1 EEXIST (File exists)
openat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo.KdSnXl", O_RDWR|O_CREAT|
O_EXCL, 0600) = 5
openat(AT_FDCWD, "/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo", O_RDONLY|O_CREAT|O_EXCL, 
000) = 7
unlink("/home/milian/.cache/debuginfod_client/
85766e9d8458b16e9c7ce6e07

Re: caching failed lookups of debuginfo?

2022-04-08 Thread Milian Wolff
On Freitag, 8. April 2022 23:56:15 CEST Milian Wolff wrote:



> Which in turn points at the code that does cache cleanup in
> `debuginfod_query_server`. I now used `rr` to record such a bogus run and I
> clearly see that `(time(NULL) - st.st_mtime <= cache_miss)` is false and it
> goes into the unlink case.
> 
> I'll try to debug this further now - I definitely do not wait 600s inbetween
> these runs here...

I compiled elfutils with debug output, and here's what I can see when I run 
`debuginfod-find`:

time(NULL) = 1649455510
st.st_mtime = 1649448650
delta = 6860
cache_miss = 600

The longer I wait, the bigger the delta becomes - i.e. for every second I 
wait, the `delta` increases by one.

And I think I know where this comes from:

```
# first we stat the target cache path
if (stat(target_cache_path, &st) == 0
  {

  # then we pass _the same st_ to
  debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st)

  # which internally will do 
  stat(config_path, st)

  # then we check the time delta
  time(NULL) - st.st_mtime <= cache_miss
```

I.e. when we check the time delta, we only take the time stamp of the 
`config_path` into account - the time stamp of the `target_cache_path` is 
ignored.

I mount my filesystems with relatime (old advise for ssd's, probably not 
relevant anymore?). I guess that's the issue then?

Can we change the above code to store the `st.st_mtime` for 
`target_cache_path` and use that for comparison purposes? That fixes the issue 
for my case. If this is acceptable, I'll provide a patch.

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH] PR29022: 000-permissions files cause problems for backups

2022-04-09 Thread Milian Wolff
On Samstag, 9. April 2022 01:37:11 CEST Aaron Merey via Elfutils-devel wrote:
> I've revised this patch so that the negative-hit file's mtime is used
> to calculate time since last download attempt instead of the cache_miss_s
> file. I've also added a check for old 000-permission files so that they
> are unlinked immediately if found.

Ah, since this fixes my issue, I don't need to provide another patch for that 
purpose I guess. Mark, will you merge this then please? I tested it too, so 
feel free to include the following, if you want:

Tested-by: Milian Wolff 

Cheers, and thanks for the fix Aaron!

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Expanding control over debuginfod usage from dwfl [was: Re: Questions regarding debuginfod.h API]

2022-04-09 Thread Milian Wolff
On Freitag, 8. April 2022 21:50:18 CEST Milian Wolff wrote:
> On Freitag, 8. April 2022 21:44:32 CEST Frank Ch. Eigler wrote:
> > > Will the default code that uses debuginfod from within dwfl then pick up
> > > my
> > > new client object and use that? I feel like there's a fundamental
> > > confusion
> > > about this on my side about this. More on that at the end of this mail
> > > below.
> > 
> > Ah yes, I didn't realize you were talking about the hidden debuginfod
> > client usage in libdwfl.  Yes, you have not much control over that,
> > because it is   hidden & automatic.  There is a
> > DEBUGINFOD_PROGRESS env var with which it can activate some
> > notification to stderr, but that's about it.  No API to get at the
> > internal transient objects.
> > 
> > If you wish to take control of this part, you could probably still use
> > dwfl.  You would do all the debuginfod client API calls yourself, then
> > "report" the dwarf file content to dwfl via dwarf_begin_elf(),
> > dwarf_begin(), dwarf_setalt() etc.
> 
> OK thank you, that is helpful. Would you say adding dwfl API to get access
> to the internal debuginfod client would be a good path forward? I guess
> more projects would potentially like to benefit from the ready made
> integration, but add a bit of control on top of it?

I looked into this a bit more, and would like to expand the dwfl API to give 
more control over how it uses debuginfod internally.

Without any API changes, I currently can not disable debuginfod usage in dwfl.
Well, the only option would be to unset the environment variable before any 
call into dwfl. But if I would roll my own debuginfod client code, it would 
still depend on the environment variable - thus the code would become quite 
ugly with repeated setenv/unsetenv pairs.

Additionally, the debuginfod client code in dwfl is good and well tested. 
Rolling my own just to get progress reporting and cancellation sounds like a 
waste of time to me.

Thus my proposal, and RFC: 

```
/* Let us mirror the debuginfod progressfn for dwfl and forward it to
   the internal debuginfod client, if available.
   This way, dwfl's usage of debuginfod can stay optional and we would not
   need to link to debuginfod directly from code using dwfl.
 */
typedef int (*dwfl_debuginfod_progressfn_t)(Dwfl *dwfl, long a, long b);
extern void dwfl_set_debuginfod_progressfn(Dwfl *dwfl,
   dwfl_debuginfod_progressfn_t fn);
```

This would already greatly help me. Better yet, we would eventually also add 
some more information to the progress callback, such as the name of the 
library that has triggered the download. But that could be done in a follow up 
patch, in a fashion similar to `debuginfod_get_url` I believe.

Alternatively:

```
/* We could just give full access to the internal client
   but doing so may clash with future usages of e.g. 
   debuginfod_{set,get}_user_data in dwfl and users of dwfl.
   Otherwise, this is obviously easiest and gives most flexibility.
   We just need to forward get_client from debuginfod-client.c
 */
extern debuginfod_client *dwfl_debuginfod_client (Dwfl *);
```

What do you all think?

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


symbol resolution differences with -flto

2022-04-15 Thread Milian Wolff
Hey there,

a user reported broken symbol resolution in hotspot/perfparser which uses 
elfutils when he's using it on code compiled with LTO.

I think I can reproduce this, and now spent some time trying to figure out the 
breakage. I found at least one situation that is seemingly related to the 
user's issue. Below are some observation of mine and then some questions, as I 
cannot explain this situation and hope that someone around here has more 
knowledge on how to handle this correctly.


First of all, the issue arises in resolving this symbol:
```
$ nm -S /home/milian/projects/compiled/kf5/lib/libKDevPlatformUtil.so.
5.8.220770 | grep addPath
0003d490 0207 T _ZN8KDevelop4Path7addPathERK7QString
```

In perfparser, we employ some tricks to increase symbol resolution 
performance. One of those is to put all symbols into a sorted list for faster 
lookups. I.e. we iterate over dwfl_module_getsymtab and 
dwfl_module_getsym_info. For the above symbol, we actually find two matches 
there. First the same that we obtain via `nm` above:

```
0x3d490 0x207 _ZN8KDevelop4Path7addPathERK7QString
```

But then there's a second match for this symbol, but that one has a zero 
`addr` and `size` returned by `dwfl_module_getsym_info`...

```
0x0 0x0 _ZN8KDevelop4Path7addPathERK7QString
```

I'm unsure if this is related, but it's odd for sure.

To make this situation even more odd: The address we fail to resolve is at 
offset `0x68590` which is _very_ far away from the symbol address reported 
above. Yet `eu-addr2line` has no problem in resolving it, and it also claims 
it's not inlined?

```
eu-addr2line -f -i -C -e /home/milian/projects/compiled/kf5/lib/
libKDevPlatformUtil.so.5.8.220770 -a 68590
0x00068590
KDevelop::Path::addPath(QString const&)
/home/milian/projects/kde/src/kdevelop/kdevplatform/util/path.cpp:408:14
```

How can this code location map to a symbol that is outside the range specified 
in the symtab?

Furthermore, when I look at the dissassembly at the offset `0x68590`, then the 
code is seemingly in a totally different part... I.e.:

```
00068283 <_ZL9splitPathRK7QString>:
...
   683bd:   c3  ret

void Path::addPath(const QString& path)
{
   683be:   55 

   6858c:   e8 fb f9 ff ff  call   67f8c 
<_ZL9cleanPathP7QVectorI7QStringEb>
}
   68591:   bb 01 00 00 00  mov$0x1,%ebx
   68596:   48 8d 85 50 ff ff fflea-0xb0(%rbp),%rax
   6859d:   48 89 c7mov%rax,%rdi
```

>From my understanding of the disassembly output, the function 
<_ZL9splitPathRK7QString> starts at 00068283. But what does this `void 
Path::addPath` line mean - is that inlined? Why then does addr2line only show 
addPath and not the splitPath function it gets inlined in?

It would be great if someone could educate me on how this is handled and how 
`0x68590` is mapped to `addPath` directly.

Thanks
-- 
Milian Wolff
http://milianw.de




Re: symbol resolution differences with -flto

2022-04-22 Thread Milian Wolff
On Freitag, 15. April 2022 14:09:02 CEST Milian Wolff wrote:
> Hey there,
> 
> a user reported broken symbol resolution in hotspot/perfparser which uses
> elfutils when he's using it on code compiled with LTO.
> 
> I think I can reproduce this, and now spent some time trying to figure out
> the breakage. I found at least one situation that is seemingly related to
> the user's issue. Below are some observation of mine and then some
> questions, as I cannot explain this situation and hope that someone around
> here has more knowledge on how to handle this correctly.

To answer my own question: The issue arises for functions with debug 
information that are not exported. Those won't have an entry in the symbol 
table, but by looking at the dwarf information we can still assign them a 
scope name from the respective DIEs, just like one would do for inline frames 
too.

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: Expanding control over debuginfod usage from dwfl [was: Re: Questions regarding debuginfod.h API]

2022-07-06 Thread Milian Wolff
On Samstag, 9. April 2022 15:44:34 CEST Milian Wolff wrote:
> On Freitag, 8. April 2022 21:50:18 CEST Milian Wolff wrote:
> > On Freitag, 8. April 2022 21:44:32 CEST Frank Ch. Eigler wrote:
> > > > Will the default code that uses debuginfod from within dwfl then pick
> > > > up
> > > > my
> > > > new client object and use that? I feel like there's a fundamental
> > > > confusion
> > > > about this on my side about this. More on that at the end of this mail
> > > > below.
> > > 
> > > Ah yes, I didn't realize you were talking about the hidden debuginfod
> > > client usage in libdwfl.  Yes, you have not much control over that,
> > > because it is   hidden & automatic.  There is a
> > > DEBUGINFOD_PROGRESS env var with which it can activate some
> > > notification to stderr, but that's about it.  No API to get at the
> > > internal transient objects.
> > > 
> > > If you wish to take control of this part, you could probably still use
> > > dwfl.  You would do all the debuginfod client API calls yourself, then
> > > "report" the dwarf file content to dwfl via dwarf_begin_elf(),
> > > dwarf_begin(), dwarf_setalt() etc.
> > 
> > OK thank you, that is helpful. Would you say adding dwfl API to get access
> > to the internal debuginfod client would be a good path forward? I guess
> > more projects would potentially like to benefit from the ready made
> > integration, but add a bit of control on top of it?
> 
> I looked into this a bit more, and would like to expand the dwfl API to give
> more control over how it uses debuginfod internally.
> 
> Without any API changes, I currently can not disable debuginfod usage in
> dwfl. Well, the only option would be to unset the environment variable
> before any call into dwfl. But if I would roll my own debuginfod client
> code, it would still depend on the environment variable - thus the code
> would become quite ugly with repeated setenv/unsetenv pairs.
> 
> Additionally, the debuginfod client code in dwfl is good and well tested.
> Rolling my own just to get progress reporting and cancellation sounds like a
> waste of time to me.
> 
> Thus my proposal, and RFC:
> 
> ```
> /* Let us mirror the debuginfod progressfn for dwfl and forward it to
>the internal debuginfod client, if available.
>This way, dwfl's usage of debuginfod can stay optional and we would not
>need to link to debuginfod directly from code using dwfl.
>  */
> typedef int (*dwfl_debuginfod_progressfn_t)(Dwfl *dwfl, long a, long b);
> extern void dwfl_set_debuginfod_progressfn(Dwfl *dwfl,
>  dwfl_debuginfod_progressfn_t fn);
> ```
> 
> This would already greatly help me. Better yet, we would eventually also add
> some more information to the progress callback, such as the name of the
> library that has triggered the download. But that could be done in a follow
> up patch, in a fashion similar to `debuginfod_get_url` I believe.
> 
> Alternatively:
> 
> ```
> /* We could just give full access to the internal client
>but doing so may clash with future usages of e.g.
>debuginfod_{set,get}_user_data in dwfl and users of dwfl.
>Otherwise, this is obviously easiest and gives most flexibility.
>We just need to forward get_client from debuginfod-client.c
>  */
> extern debuginfod_client *dwfl_debuginfod_client (Dwfl *);
> ```
> 
> What do you all think?

Ping, could I get some feedback on the above please? I have some time the next 
days and would like to work on this. Which way would you prefer?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

smime.p7s
Description: S/MIME cryptographic signature


Re: Expanding control over debuginfod usage from dwfl [was: Re: Questions regarding debuginfod.h API]

2022-07-06 Thread Milian Wolff
On Mittwoch, 6. Juli 2022 20:40:09 CEST Frank Ch. Eigler wrote:
> Hi -
> 
> > > Thus my proposal, and RFC:
> > > 
> > > ```
> > > /* Let us mirror the debuginfod progressfn for dwfl and forward it to
> > > 
> > >the internal debuginfod client, if available.
> > >This way, dwfl's usage of debuginfod can stay optional and we would
> > >not
> > >need to link to debuginfod directly from code using dwfl.
> > >  
> > >  */
> > > 
> > > typedef int (*dwfl_debuginfod_progressfn_t)(Dwfl *dwfl, long a, long b);
> > > extern void dwfl_set_debuginfod_progressfn(Dwfl *dwfl,
> > > 
> > >  dwfl_debuginfod_progressfn_t fn);
> > > 
> > > ```
> 
> (Don't quite see how this extension would let you disable or enable
> debuginfod support on a given dwfl.  By the time a progressfn is
> called, some debuginfod traffic would be attempted.)n
> 
> > Alternately:
> > [...]
> > 
> > > /* We could just give full access to the internal client
> > > 
> > >but doing so may clash with future usages of e.g.
> > >debuginfod_{set,get}_user_data in dwfl and users of dwfl.
> > >Otherwise, this is obviously easiest and gives most flexibility.
> > >We just need to forward get_client from debuginfod-client.c
> > >  
> > >  */
> > > 
> > > extern debuginfod_client *dwfl_debuginfod_client (Dwfl *);
> > > ```
> > > What do you all think?
> 
> In order to -influence- things, would you not need to -change- the
> client somehow?  We don't have debuginfod-client apis to disable or
> reconfigure any particular client object.  Such an API wouldn't let
> you replace it with a hook object of your own either.
> 
> 
> So, dunno, could you perhaps remind us what your current usage
> interests are?
> 
> To enable/disable it on a per-dwfl basis?  If yes, and if statically,
> maybe a new Dwfl_Callbacks option for .find_debuginfo() could be your
> ticket: a dwfl_standard_find_debuginfo variant sans debuginfod at the
> end.

I have two goals:

a) Notifying the user that a download is ongoing. Right now, it feels like the 
tool is frozen as no feedback is given to the user.

b) Allow to disable debuginfod. But that is already doable by unsetting the 
DEBUGINFOD_URLS env var, and as such I don't necessarily need any additional 
API for that?

As such, only a) is something that needs immediate attention imo, and what my 
API proposal is covering.

Thanks

-- 
Milian Wolff
http://milianw.de




Re: Expanding control over debuginfod usage from dwfl [was: Re: Questions regarding debuginfod.h API]

2022-07-06 Thread Milian Wolff
On Mittwoch, 6. Juli 2022 21:41:33 CEST Frank Ch. Eigler wrote:
> Hi -
> 
> > a) Notifying the user that a download is ongoing. Right now, it feels like
> > the tool is frozen as no feedback is given to the user.
> 
> Right, can you explain how DEBUGINFOD_PROGRESS=1 is not a good fit in your
> case?

As you said yourself:

> There is a
> DEBUGINFOD_PROGRESS env var with which it can activate some
> notification to stderr, but that's about it.

I'm working on GUI applications (hotspot [1], gammaray [2]), people do not 
look at the command line output. I want to show the download progress and 
status graphically instead.

[1]: https://github.com/KDAB/hotspot/
[2]: https://github.com/KDAB/gammaray

-- 
Milian Wolff
http://milianw.de




[PATCH] Introduce public dwfl_get_debuginfod_client API

2022-07-07 Thread Milian Wolff
Dwfl can use debuginfod internally, which was so far totally opaque
to the outside. While the functionality is great for users of the
dwfl API, the long wait times induced by downloading of data over
debuginfod lead to complaints by endusers. To offer them a bit more
insight into the internal ongoings, one can now use e.g.
`debuginfod_set_progressfn` on the handle returned by
`dwfl_get_debuginfod_client` to report download progress.

Rename get_client to dwfl_get_debuginfod_client and make it public.
Unconditionally compile debuginfod-client.c and stub the new public
function and always return NULL when debuginfod integration was
disabled.

Signed-off-by: Milian Wolff 
---
 libdw/libdw.map |  5 +
 libdwfl/ChangeLog   |  5 +
 libdwfl/Makefile.am |  5 +
 libdwfl/debuginfod-client.c | 23 ++-
 libdwfl/libdwfl.h   | 10 ++
 libdwfl/libdwflP.h  |  1 +
 6 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/libdw/libdw.map b/libdw/libdw.map
index 4f530378..3fdf3f93 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -366,3 +366,8 @@ ELFUTILS_0.186 {
 dwarf_linecontext;
 dwarf_linefunctionname;
 } ELFUTILS_0.177;
+
+ELFUTILS_0.187 {
+  global:
+dwfl_get_debuginfod_client;
+} ELFUTILS_0.186;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index b3ca56cb..890df156 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2022-06-22  Milian Wolff 
+
+   * libdwfl.h, debuginfod-client.c (dwfl_get_debuginfod_client):
+   Rename get_client to dwfl_get_debuginfod_client and make it public.
+
 2022-05-15  Mark Wielaard  
 
* libdwfl.h (dwfl_module_addrinfo): Update docs and nonnull
diff --git a/libdwfl/Makefile.am b/libdwfl/Makefile.am
index a0013e41..3278358d 100644
--- a/libdwfl/Makefile.am
+++ b/libdwfl/Makefile.am
@@ -70,7 +70,7 @@ libdwfl_a_SOURCES = dwfl_begin.c dwfl_end.c dwfl_error.c 
dwfl_version.c \
link_map.c core-file.c open.c image-header.c \
dwfl_frame.c frame_unwind.c dwfl_frame_pc.c \
linux-pid-attach.c linux-core-attach.c dwfl_frame_regs.c \
-   gzip.c
+   gzip.c debuginfod-client.c
 
 if BZLIB
 libdwfl_a_SOURCES += bzip2.c
@@ -81,9 +81,6 @@ endif
 if ZSTD
 libdwfl_a_SOURCES += zstd.c
 endif
-if LIBDEBUGINFOD
-libdwfl_a_SOURCES += debuginfod-client.c
-endif
 
 libdwfl = $(libdw)
 libdw = ../libdw/libdw.so
diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
index 153260c3..813043b1 100644
--- a/libdwfl/debuginfod-client.c
+++ b/libdwfl/debuginfod-client.c
@@ -32,6 +32,9 @@
 #endif
 
 #include "libdwflP.h"
+
+#ifdef ENABLE_LIBDEBUGINFOD
+
 #include 
 #include 
 
@@ -46,8 +49,8 @@ static pthread_once_t init_control = PTHREAD_ONCE_INIT;
 
 /* NB: this is slightly thread-unsafe */
 
-static debuginfod_client *
-get_client (Dwfl *dwfl)
+debuginfod_client *
+dwfl_get_debuginfod_client (Dwfl *dwfl)
 {
   if (dwfl->debuginfod != NULL)
 return dwfl->debuginfod;
@@ -71,7 +74,7 @@ __libdwfl_debuginfod_find_executable (Dwfl *dwfl,
   int fd = -1;
   if (build_id_len > 0)
 {
-  debuginfod_client *c = get_client (dwfl);
+  debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
   if (c != NULL)
fd = (*fp_debuginfod_find_executable) (c, build_id_bits,
   build_id_len, NULL);
@@ -88,7 +91,7 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl,
   int fd = -1;
   if (build_id_len > 0)
 {
-  debuginfod_client *c = get_client (dwfl);
+  debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
   if (c != NULL)
fd = (*fp_debuginfod_find_debuginfo) (c, build_id_bits,
  build_id_len, NULL);
@@ -105,7 +108,7 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
 }
 
 /* Try to get the libdebuginfod library functions.
-   Only needs to be called once from get_client.  */
+   Only needs to be called once from dwfl_get_debuginfod_client.  */
 static void
 __libdwfl_debuginfod_init (void)
 {
@@ -134,3 +137,13 @@ __libdwfl_debuginfod_init (void)
}
 }
 }
+
+#else // ENABLE_LIBDEBUGINFOD
+
+debuginfod_client *
+dwfl_get_debuginfod_client (Dwfl *)
+{
+  return NULL;
+}
+
+#endif // ENABLE_LIBDEBUGINFOD
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index c55a8eaa..b323e8fb 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -49,6 +49,9 @@ typedef struct Dwfl_Thread Dwfl_Thread;
PC location described by an FDE belonging to Dwfl_Thread.  */
 typedef struct Dwfl_Frame Dwfl_Frame;
 
+/* Handle for debuginfod-client connection.  */
+typedef struct debuginfod_client debuginfod_client;
+
 /* Callbacks.  */
 typedef struct
 {
@@ -795,6 +798,13 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
 bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
   __nonnull_attribute__ (1, 2);
 
+

Re: [PATCH] Introduce public dwfl_get_debuginfod_client API

2022-07-07 Thread Milian Wolff
On Donnerstag, 7. Juli 2022 18:40:05 CEST Aaron Merey wrote:
> Hi Milian,
> 
> On Thu, Jul 7, 2022 at 10:47 AM Milian Wolff  wrote:
> > Dwfl can use debuginfod internally, which was so far totally opaque
> > to the outside. While the functionality is great for users of the
> > dwfl API, the long wait times induced by downloading of data over
> > debuginfod lead to complaints by endusers. To offer them a bit more
> > insight into the internal ongoings, one can now use e.g.
> > `debuginfod_set_progressfn` on the handle returned by
> > `dwfl_get_debuginfod_client` to report download progress.
> > 
> > Rename get_client to dwfl_get_debuginfod_client and make it public.
> > Unconditionally compile debuginfod-client.c and stub the new public
> > function and always return NULL when debuginfod integration was
> > disabled.
> 
> Thanks for the patch. This looks ok and I was able to successfully
> run the testsuite with and without debuginfod enabled.
> 
> > @@ -70,7 +70,7 @@ libdwfl_a_SOURCES = dwfl_begin.c dwfl_end.c dwfl_error.c
> > dwfl_version.c \
> 
> Some line breaks may have accidentally snuck into the patch. I had to
> manually remove the line break right after "dwfl_error.c" for git to
> apply the patch without error.

Ah yes sorry, I did not think about that when copy'n'pasting the git format-
patch output into my mail client. Should I resend with the line breaks fixed?

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


[PATCH] Introduce public dwfl_get_debuginfod_client API

2022-07-07 Thread Milian Wolff
Dwfl can use debuginfod internally, which was so far totally opaque
to the outside. While the functionality is great for users of the
dwfl API, the long wait times induced by downloading of data over
debuginfod lead to complaints by endusers. To offer them a bit more
insight into the internal ongoings, one can now use e.g.
`debuginfod_set_progressfn` on the handle returned by
`dwfl_get_debuginfod_client` to report download progress.

Rename get_client to dwfl_get_debuginfod_client and make it public.
Unconditionally compile debuginfod-client.c and stub the new public
function and always return NULL when debuginfod integration was
disabled.

Signed-off-by: Milian Wolff 
---
 libdw/libdw.map |  5 +
 libdwfl/ChangeLog   |  5 +
 libdwfl/Makefile.am |  5 +
 libdwfl/debuginfod-client.c | 23 ++-
 libdwfl/libdwfl.h   | 10 ++
 libdwfl/libdwflP.h  |  1 +
 6 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/libdw/libdw.map b/libdw/libdw.map
index 4f530378..3fdf3f93 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -366,3 +366,8 @@ ELFUTILS_0.186 {
 dwarf_linecontext;
 dwarf_linefunctionname;
 } ELFUTILS_0.177;
+
+ELFUTILS_0.187 {
+  global:
+dwfl_get_debuginfod_client;
+} ELFUTILS_0.186;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index b3ca56cb..890df156 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2022-06-22  Milian Wolff 
+
+   * libdwfl.h, debuginfod-client.c (dwfl_get_debuginfod_client):
+   Rename get_client to dwfl_get_debuginfod_client and make it public.
+
 2022-05-15  Mark Wielaard  
 
* libdwfl.h (dwfl_module_addrinfo): Update docs and nonnull
diff --git a/libdwfl/Makefile.am b/libdwfl/Makefile.am
index a0013e41..3278358d 100644
--- a/libdwfl/Makefile.am
+++ b/libdwfl/Makefile.am
@@ -70,7 +70,7 @@ libdwfl_a_SOURCES = dwfl_begin.c dwfl_end.c dwfl_error.c 
dwfl_version.c \
link_map.c core-file.c open.c image-header.c \
dwfl_frame.c frame_unwind.c dwfl_frame_pc.c \
linux-pid-attach.c linux-core-attach.c dwfl_frame_regs.c \
-   gzip.c
+   gzip.c debuginfod-client.c
 
 if BZLIB
 libdwfl_a_SOURCES += bzip2.c
@@ -81,9 +81,6 @@ endif
 if ZSTD
 libdwfl_a_SOURCES += zstd.c
 endif
-if LIBDEBUGINFOD
-libdwfl_a_SOURCES += debuginfod-client.c
-endif
 
 libdwfl = $(libdw)
 libdw = ../libdw/libdw.so
diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
index 153260c3..813043b1 100644
--- a/libdwfl/debuginfod-client.c
+++ b/libdwfl/debuginfod-client.c
@@ -32,6 +32,9 @@
 #endif
 
 #include "libdwflP.h"
+
+#ifdef ENABLE_LIBDEBUGINFOD
+
 #include 
 #include 
 
@@ -46,8 +49,8 @@ static pthread_once_t init_control = PTHREAD_ONCE_INIT;
 
 /* NB: this is slightly thread-unsafe */
 
-static debuginfod_client *
-get_client (Dwfl *dwfl)
+debuginfod_client *
+dwfl_get_debuginfod_client (Dwfl *dwfl)
 {
   if (dwfl->debuginfod != NULL)
 return dwfl->debuginfod;
@@ -71,7 +74,7 @@ __libdwfl_debuginfod_find_executable (Dwfl *dwfl,
   int fd = -1;
   if (build_id_len > 0)
 {
-  debuginfod_client *c = get_client (dwfl);
+  debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
   if (c != NULL)
fd = (*fp_debuginfod_find_executable) (c, build_id_bits,
   build_id_len, NULL);
@@ -88,7 +91,7 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl,
   int fd = -1;
   if (build_id_len > 0)
 {
-  debuginfod_client *c = get_client (dwfl);
+  debuginfod_client *c = dwfl_get_debuginfod_client (dwfl);
   if (c != NULL)
fd = (*fp_debuginfod_find_debuginfo) (c, build_id_bits,
  build_id_len, NULL);
@@ -105,7 +108,7 @@ __libdwfl_debuginfod_end (debuginfod_client *c)
 }
 
 /* Try to get the libdebuginfod library functions.
-   Only needs to be called once from get_client.  */
+   Only needs to be called once from dwfl_get_debuginfod_client.  */
 static void
 __libdwfl_debuginfod_init (void)
 {
@@ -134,3 +137,13 @@ __libdwfl_debuginfod_init (void)
}
 }
 }
+
+#else // ENABLE_LIBDEBUGINFOD
+
+debuginfod_client *
+dwfl_get_debuginfod_client (Dwfl *)
+{
+  return NULL;
+}
+
+#endif // ENABLE_LIBDEBUGINFOD
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index c55a8eaa..b323e8fb 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -49,6 +49,9 @@ typedef struct Dwfl_Thread Dwfl_Thread;
PC location described by an FDE belonging to Dwfl_Thread.  */
 typedef struct Dwfl_Frame Dwfl_Frame;
 
+/* Handle for debuginfod-client connection.  */
+typedef struct debuginfod_client debuginfod_client;
+
 /* Callbacks.  */
 typedef struct
 {
@@ -795,6 +798,13 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
 bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
   __nonnull_attribute__ (1, 2);
 
+

runtime validation of DT_SYMTAB lookups - why is there no DT_SYMSZ?

2022-07-11 Thread Milian Wolff
Hey there,

in heaptrack I have code to runtime attach to a program and then rewrite the 
various rel / rela / jmprel tables to intercept calls to malloc & friends.

This works, but now I have received a crash report for what seems to be an 
invalid DSO file: The jmprel table contains an invalid entry which points to 
an out-of-bounds symbol, leading to a crash when we try to look at the 
symbol's name.

I would like to protect against this crash by detecting the invalid symbols. 
But to do that, I would need to know the size of the symbol table, which is 
much harder than I would have hoped:

We have:

```
#define DT_SYMTAB   6   /* Address of symbol table */
#define DT_SYMENT   11  /* Size of one symbol table entry */
```

But there is no `DT_SYMSZ` or similar, which we would need to validate symbol 
indices. Am I overlooking something or is that really missing? Does anyone 
know why? The other tables have that, e.g.:

```
#define DT_PLTRELSZ 2   /* Size in bytes of PLT relocs */
#define DT_RELASZ   8   /* Total size of Rela relocs */
#define DT_STRSZ10  /* Size of string table */
#define DT_RELSZ18  /* Total size of Rel relocs */
```

Why is this missing for the symtab?

The only viable alternative seems to be to mmap the file completely to access 
the Elf header and then iterate over the Elf sections to query the size of the 
SHT_DYNSYM section. This is pretty complicated, and costly. Does anyone have a 
better solution that would allow me to validate symbol indices?

Thanks

PS: eu-elflint reports this for the broken DSOs e.g.:
```
$ eu-elflint libQt5Qml.so.5.12
section [ 3] '.dynsym': symbol 1272: st_value out of bounds
section [ 3] '.dynsym': symbol 3684: st_value out of bounds
section [29] '.symtab': _GLOBAL_OFFSET_TABLE_ symbol size 0 does not match 
.got section size 18340
section [29] '.symtab': _DYNAMIC symbol size 0 does not match dynamic segment 
size 336
section [29] '.symtab': symbol 25720: st_value out of bounds
section [29] '.symtab': symbol 27227: st_value out of bounds
```

Does anyone know how this can happen? Is this a bug in the toolchain?
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH] Introduce public dwfl_get_debuginfod_client API

2022-07-13 Thread Milian Wolff
On Mittwoch, 13. Juli 2022 20:20:04 CEST Aaron Merey wrote:
> Hi Milian,
> 
> There weren't any concerns with the patch so I've gone ahead and merged
> it as commit a4b1839c3c46.

Thank you Aaron!

I got spammed by a flood of mails from buildbot, but from what I can tell the 
issues are all unrelated to my patch? Or did I break something? See e.g.:

[1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [PATCH] Introduce public dwfl_get_debuginfod_client API

2022-07-13 Thread Milian Wolff
On Mittwoch, 13. Juli 2022 22:38:51 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Wed, Jul 13, 2022 at 09:36:45PM +0200, Milian Wolff wrote:
> > On Mittwoch, 13. Juli 2022 20:20:04 CEST Aaron Merey wrote:
> > > There weren't any concerns with the patch so I've gone ahead and merged
> > > it as commit a4b1839c3c46.
> 
> Yeah, sorry, I should have spoken up on the list not just mumbled
> something on irc. My only concern was that I was afraid to pull in
> libdebuginfod.h api by default. But you handled to by making
> debuginfod_client handle an opaque pointer, which it of course is. So
> my worry was unfounded.
> 
> But I did just now see it has a typo in libdw.map:
> 
> ELFUTILS_0.187 {
>   global:
> dwfl_get_debuginfod_client;
> } ELFUTILS_0.186;
> 
> That should be ELFUTILS_0.188 we already released 0.187.

Ah, thanks for spotting that.

> Also (micro optimazition to prevent a PLT call) the internal calls
> should use INTUSE (dwfl_get_debuginfod_client).

TIL, thanks. I probably will never feel at home in C code bases with all these 
arcane macros :D

> I made both changes and added a NEWS entry. See attached.

LGTM.

> > I got spammed by a flood of mails from buildbot, but from what I can tell
> > the issues are all unrelated to my patch? Or did I break something? See
> > e.g.:
> > 
> > [1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41
> 
> Those are unrelated to your patch

OK, thanks.

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: runtime validation of DT_SYMTAB lookups - why is there no DT_SYMSZ?

2022-07-27 Thread Milian Wolff
On Dienstag, 26. Juli 2022 17:28:11 CEST Mark Wielaard wrote:
> Hi Milian,
> 
> On Mon, 2022-07-11 at 18:40 +0200, Milian Wolff wrote:
> > in heaptrack I have code to runtime attach to a program and then
> > rewrite the
> > various rel / rela / jmprel tables to intercept calls to malloc & friends.
> > 
> > This works, but now I have received a crash report for what seems to
> > be an
> > invalid DSO file: The jmprel table contains an invalid entry which
> > points to
> > an out-of-bounds symbol, leading to a crash when we try to look at
> > the
> > symbol's name.
> > 
> > I would like to protect against this crash by detecting the invalid
> > symbols.
> > But to do that, I would need to know the size of the symbol table,
> > which is
> > much harder than I would have hoped:
> > 
> > We have:
> > 
> > ```
> > #define DT_SYMTAB   6   /* Address of symbol table */
> > #define DT_SYMENT   11  /* Size of one symbol table
> > entry */
> > ```
> > 
> > But there is no `DT_SYMSZ` or similar, which we would need to
> > validate symbol
> > indices. Am I overlooking something or is that really missing? Does
> > anyone
> > know why? The other tables have that, e.g.:
> > 
> > ```
> > #define DT_PLTRELSZ 2   /* Size in bytes of PLT relocs */
> > #define DT_RELASZ   8   /* Total size of Rela relocs */
> > #define DT_STRSZ10  /* Size of string table */
> > #define DT_RELSZ18  /* Total size of Rel relocs
> > */
> > ```
> > 
> > Why is this missing for the symtab?
> > 
> > The only viable alternative seems to be to mmap the file completely
> > to access
> > the Elf header and then iterate over the Elf sections to query the
> > size of the
> > SHT_DYNSYM section. This is pretty complicated, and costly. Does
> > anyone have a
> > better solution that would allow me to validate symbol indices?
> 
> I don't know why it is missing, but it is indeed a tricky issue. You
> really want to know the number of elements (or the size) of the symbol
> table, but it takes a little gymnastics to get that.

Thanks for confirming that this isn't available currently. Would it be 
possible to add this? What's the process for standardization here? I guess it 
would take a very long time, yet this seems to me as if it would be beneficial 
in the long term.

> Di Chen recently
> (or actually not that recently, I just still haven't reviewed, sorry!)
> posted a patch for
> https://sourceware.org/bugzilla/show_bug.cgi?id=28873 to print out the
> symbols from the dynamic segment
> https://sourceware.org/pipermail/elfutils-devel/2022q2/005086.html

Interesting. But from what I can tell, this patch has access to the full Elf 
object and thus can access segments which are not normally loaded at runtime?

> > PS: eu-elflint reports this for the broken DSOs e.g.:
> > ```
> > $ eu-elflint libQt5Qml.so.5.12
> > section [ 3] '.dynsym': symbol 1272: st_value out of bounds
> > section [ 3] '.dynsym': symbol 3684: st_value out of bounds
> > section [29] '.symtab': _GLOBAL_OFFSET_TABLE_ symbol size 0 does not
> > match
> > .got section size 18340
> > section [29] '.symtab': _DYNAMIC symbol size 0 does not match dynamic
> > segment
> > size 336
> > section [29] '.symtab': symbol 25720: st_value out of bounds
> > section [29] '.symtab': symbol 27227: st_value out of bounds
> > ```
> > 
> > Does anyone know how this can happen? Is this a bug in the toolchain?
> 
> Try with eu-elflint --gnu which suppresses some known issues.

Indeed, with `--gnu` the tool reports `No errors`.

> Also could you show those symbol values (1272, 3684, 25720, 27227) they
> might have a special type, so their st_value isn't really an address?

```
$ eu-readelf -s libQt5Qml.so.5.12.0 | grep -E "^\s*(1272|3684|25720|27227):"
 1272: 003f9974  0 NOTYPE  GLOBAL DEFAULT   25 __bss_start__@@Qt_5
 3684: 003f9974  0 NOTYPE  GLOBAL DEFAULT   25 __bss_start@@Qt_5
 1272: 003ccc4c  0 NOTYPE  LOCAL  DEFAULT   17 $d
 3684: 003cbfec  0 NOTYPE  LOCAL  DEFAULT   17 $d
25720: 003f9974  0 NOTYPE  GLOBAL DEFAULT   25 __bss_start
27227: 003f9974  0 NOTYPE  GLOBAL DEFAULT   25 __bss_start__
```

The first two matches come from the `.dynsym`, the last four come from 
`.symtab`.

Can anyone tell me how `eu-readelf` resolves these symbol names?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [RFC] sketch of an unwinder cache interface for elfutils libdwfl

2024-12-11 Thread Milian Wolff
On Mittwoch, 11. Dezember 2024 17:09:19 Mitteleuropäische Normalzeit Christian 
Hergert wrote:
> On vacation until 2025 so my replies will be sparse here :)
> 
> On 12/11/24 3:24 AM, Milian Wolff wrote:
> > For symmetry reasons, we also need an `..._end` method. And what about
> > explicitly removing processes once they have finished (PERF_RECORD_EXIT)?
> 
> One area you can run into issues with this is that there is no
> synchronization between Perf streams from different CPUs. So you could
> end up processing an EXIT from PID N on CPU 1 before a SAMPLE from PID N
> on CPU 2.

I thought that the perf events are never ordered when you obtain them, you 
always first have to sort them based on their timestamps. For optimization 
purposes thankfully we have PERF_RECORD_FINISHED_ROUND so we don't need to 
buffer all events and sort them. That said, we do sometimes see violations of 
this, with some events arriving after a PERF_RECORD_FINISHED_ROUND with a 
timestamp earlier than what was previously handled in the last round.

Is that what you have in mind, or are you thinking of something else that I am 
not aware of yet?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [RFC] sketch of an unwinder cache interface for elfutils libdwfl

2024-12-11 Thread Milian Wolff
On Mittwoch, 11. Dezember 2024 15:58:32 Mitteleuropäische Normalzeit Serhei 
Makarov wrote:
> On Wed, Dec 11, 2024, at 9:46 AM, William Cohen wrote:
> > Hi Serhei,
> > 
> > The dwfl_report_proc_map() will handle when things are added to the
> > memory map.  Is there anything to handle the inverse event when memory
> > is removed from the process memory map, such as a dlcose()?
> 
> Interestingly, linux/perf/tools/design.txt mentions an 'munmap' field in
> struct perf_event_attr, but it doesn't seem to have made it into the actual
> kernel implementation of perf_events. I'll have to do some more digging to
> find out what happened with that idea between design and implementation.
> 
> If we have reason to believe that the process mappings are changing too
> drastically to get sensible results with incremental updates,
> sysprof-live-unwinder's current quick solution (i.e. re-scan the proc
> mappings from scratch) is a feasible fallback. We just need a threshold for
> doing that which is 'sensible, but not too frequent'. Even a time threshold
> might be worth experimenting with for this. The main high-overhead case
> we're trying to avoid is when a process loads then does mmap, mmap, mmap,
> and the profiler is following this process from the start and reloading the
> Dwfl each time.

The way we handle this in perfparser is by not actually reporting anything 
until the time when we actually need it.

I.e. we store the mmap events from perf in a way that we can use for fast 
lookups. Then during unwinding we ensure that the IP for whatever we are about 
to access is reported e.g. from the `memory_read` callback or for every IP we 
found before symbolication / inline frame resolution.

When we receive more perf mmap events we check whether they invalidate old 
mappings (i.e. overlapping an existing region), we invalidate our caches,and 
unreport the modules from dwfl, and start the process anew.

I don't think it's a good idea to report all modules all the time to elfutils 
- esp. in large applications with compressed debug information it can often be 
that you only need a fraction of the loaded shared symbols e.g. Also consider 
debuginfod - you don't want to download everything (such as GDB would be 
doing), you only want to download what's needed.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.


Re: [RFC] sketch of an unwinder cache interface for elfutils libdwfl

2024-12-11 Thread Milian Wolff
 unsatisfying in terms of
> perceived performance -- extra hoops through which the data must jump
> between processes -- and maintainability -- the Sysprof binary format needs
> an extension for representing stack data, with nontrivial details to
> bikeshed there. The bikeshedding effort may as well be spent on designing a
> library interface to the eu-stacktrace functionality.
> 
> * Why do we need a Dwfl_Process_Table struct independent of Dwfl?
> 
> As mentioned above, putting it in the library gives us a place to implement
> caching of data/work that's redundant between different Dwfls. There's a
> different way of designing this that would involve having Dwfl be the
> global session struct and replacing the single Dwfl::process field with a
> table of processes, but that requires redoing the existing library
> interface to be parametrized by Dwfl_Process instead of just Dwfl, grossly
> disruptive relative to the level of functionality I'm actually adding. We
> could also have Dwfls share data through a hidden global variable, but it
> seems gross to enable such sneaky caching without the user of the library
> explicitly requesting it via Dwfl_Process_Table.

I agree with your proposal. But please take thread safety into account while 
creating your design. Right now, one could in theory at least process/unwind 
multiple processes in parallel. It would be great to keep this functionality 
even when dealing with multiple processes that share some common data behind 
the scenes!

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.