Re: [PATCH] Begin adding some docs to elfutils

[Mark Wielaard]

Hi Ben,

On Thu, 2019-08-22 at 07:47 -0700, Ben Woodard wrote:
> - Added doc/README
> - Updated doc/ChangeLog
> - Added a eu-readelf manpage based on the one from binutils.
> - Added a brand new manpage for eu-elfclassify the new utility added
> in 0.177
> - Add some new files in the doc directory and sync makefile with
> upstream.
> - Reenable the compilation of doc directory.
> - Disable sgml file building
> - Build man pages the automake way

This is great. We definitely need more (any!) documentation.

The build changes look OK.

But I do have some questions about the reuse of the binutils readelf
documentation for eu-readelf.

In theory the (re)use of GFDL documentation (with no invariants) is
fine. But then we do also need to ship a copy of the GFDL, please do
add it.

Also, the GFDL requires us shipping a "Transparent" copy of the
Document. We only distribute the nroff generated version. I guess that
is a "Transparent" since it is a "format whose specification is
available to the general public, that is suitable for revising the
document straightforwardly with generic text editors". But the original
from binutils says: "Automatically generated by Pod::Man 2.27
(Pod::Simple 3.28)". So, that does imply that at least he upstream
binutils copy was generated from another format. I am not sure that
makes out copy "Opaque", but should we generate it from the same
original source instead?

The eu-readelf.1 manual lists various arguments/flags we don't actually
support like --dwarf-depth and --dwarf-start. And is missing some which
we do support like --dwarf-skeleton and --elf-section. I would like to
see that corrected before it gets checked in.

Also we do use a Signed-off-by tag for commits to show the origins of
each patch. Please read:
https://sourceware.org/git/?p=elfutils.git;a=blob_plain;f=CONTRIBUTING;hb=HEAD
And add one if you agree.

Thanks,

Mark

Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()

[Mark Wielaard]

Hi,

On Wed, 2019-08-21 at 09:08 -0500, Jonathon Anderson wrote:
> On Wed, Aug 21, 2019 at 6:16 AM, Mark Wielaard 
> wrote:On Fri, 2019-08-16 at 14:24 -0500, Jonathon Anderson wrote:
> > >  For parallel applications that need the information in the DIEs, the
> > >  Dwarf_Abbrev hash table et al. become a massive data race. This 
> > > fixes
> > >  that by:
> > > 
> > >  1. Adding atomics & locks to the hash table to manage concurrency
> > > (lib/dynamicsizehash_concurrent.{c,h})
> > >  2. Adding a lock & array structure to the memory manager 
> > > (pseudo-TLS)
> > > (libdwP.h, libdw_alloc.c)
> > >  3. Adding extra configure options for Helgrind/DRD annotations
> > > (configure.ac)
> > >  4. Including "stdatomic.h" from FreeBSD, to support C11-style   
> > > atomics.
> > > (lib/stdatomic.h)
> > 
> > This looks like really nice work. Thanks!
> > 
> > I am splitting review in some smaller parts if you don't mind.
> > Simply because it is large and I cannot keep everything in my head at
> > once :)

BTW. I would prefer to handle this as 4 separate additions, probably in
this order:

1) configure stuff for valgrind annotations.
2) add support for stdatomic.h functions.
3) thread-safe obstack memory handling
4) concurrent dynamic hash table.

> > If the compiler provides stdatomic.h then I think it would be good
> > to
> > use that instead of our own implementation. The copyright isn't a
> > problem. But do you have a reference/URL to the upstream version? I
> > like to add that somewhere, so we can sync with it in the future. I 
> > see
> > various commented out parts. Was that already upstream? Should we just
> > remove those parts?
> 
> It would definitely be preferable to use the compiler's implementation 
> if possible, we used this in case GCC 4.7 and 4.8 (RHEL7) compatibility 
> was needed. If those versions are old enough the file can be removed 
> entirely.
> 
> The upstream is at 
> https://github.com/freebsd/freebsd/blob/master/sys/sys/stdatomic.h, 
> although the version here appears to be slightly modified (we used the 
> version that HPCToolkit ships). The components we use don't seem 
> affected, so a resync shouldn't make a difference.

OK, then we should come up with some kind of configure test to see if
we can use the standard stdatomic.h and otherwise use our own. I am
surprised I cannot find other projects doing this. Would be nice to
"steal" something standard for this.

> > >   - Currently the concurrent hash table is always enabled,
> > > performance-wise there is no known difference between it
> > > and the non-concurrent  version.
> > > This can be changed to toggle with --enable-thread-safety
> > > if preferred.
> > 
> > I would prefer it always enabled, unless there is a massive slowdown 
> > of
> > the single-threaded case. The problem with --enable-thread-safety is
> > that it is a) known broken (sigh) and b) it basically introduces two
> > separate libraries that behave subtly differently. I would very much
> > like to get rid of --enable-thread-safety by fixing the broken locking
> > and simply making it the default.
> 
> I haven't noticed any slowdown in the single-threaded case, although I 
> haven't stressed it hard enough to find out for certain. From a 
> theoretical standpoint it shouldn't, atomics (with the proper memory 
> orders) are usually (on x86 at least) as cheap as normal accesses when 
> used by a single thread, and the algorithm is otherwise effectively the 
> same as the original hash table.
> 
> How difficult would it be to fix the locking (or rather, what's 
> "broken")? We would definitely benefit from having thread-safety at 
> least for getters, which would only need locks around the internal 
> management.

To be honest I don't know how badly it is broken.
It is only implemented for libelf.
If you configure --enable-thread-safety and make check you will see
several tests fail because they abort with Unexpected error: Resource
deadlock avoided.

I think it is mainly that nobody maintained the locks and now some are
just wrongly placed. Ideally we switch --enable-thread-safety on by
default, identify which locks are wrongly placed, run all tests with
valgrind/hellgrind and fix any issues found.

It really has not been a priority. Sorry.

> > >   - Another implementation of #2 above might use dynamic TLS
> > > (pthread_key_*),
> > > we chose this implementation to reduce the overall complexity.
> > 
> > Are there any other trade-offs?
> 
> If the application spawns N threads that all use a Dwarf object (same 
> or different) enough to cause an allocation, and then joins them all, 
> any Dwarf objects allocated after will allocate N unusable slots in the 
> mem_tails array. Thus long-running applications (that don't use thread 
> pools) would start experiencing effects similar to a memory leak, of 1 
> pointer's worth (8 bytes on 64-bit) per dead thread.
> 
> The alternative is to use dynamic TLS so that only threads that

[PATCH] V2 Begin adding some docs to elfutils

[Ben Woodard]

- Added doc/README
- Updated doc/ChangeLog
- Added a eu-readelf manpage based on the one from binutils.
- Added a brand new manpage for eu-elfclassify the new utility added in 0.177
- Add some new files in the doc directory and sync makefile with upstream.
- Reenable the compilation of doc directory.
- Disable sgml file building
- Build man pages the automake way

Since V1
- Put man pages in the proper directories.
- Added copy of Gnu Free Documentation License
- Modified eu-readelf.1 man page to match the supported options.

Signed-off-by: Ben Woodard 
---
 Makefile.am  |   3 +-
 configure.ac |   3 +-
 doc/COPYING  | 455 ++
 doc/ChangeLog|  41 +++
 doc/Makefile.am  |   4 +-
 doc/README   |  20 ++
 doc/elf_begin.3  |  37 +++
 doc/elf_clone.3  |  14 +
 doc/elf_getdata.3|  28 ++
 doc/elf_update.3 |  14 +
 doc/eu-elfclassify.1 | 197 +
 doc/eu-readelf.1 | 638 +++
 12 files changed, 1449 insertions(+), 5 deletions(-)
 create mode 100644 doc/COPYING
 create mode 100644 doc/README
 create mode 100644 doc/elf_begin.3
 create mode 100644 doc/elf_clone.3
 create mode 100644 doc/elf_getdata.3
 create mode 100644 doc/elf_update.3
 create mode 100644 doc/eu-elfclassify.1
 create mode 100644 doc/eu-readelf.1

diff --git a/Makefile.am b/Makefile.am
index 2ff444e7..9f2ece49 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -26,9 +26,8 @@ AM_MAKEFLAGS = --no-print-directory
 
 pkginclude_HEADERS = version.h
 
-# Add doc back when we have some real content.
 SUBDIRS = config m4 lib libelf libebl libdwelf libdwfl libdw libcpu libasm \
- backends src po tests
+ backends src po doc tests
 
 EXTRA_DIST = elfutils.spec GPG-KEY NOTES CONTRIBUTING \
 COPYING COPYING-GPLV2 COPYING-LGPLV3
diff --git a/configure.ac b/configure.ac
index c443fa3b..30429e29 100644
--- a/configure.ac
+++ b/configure.ac
@@ -497,8 +497,7 @@ AC_SUBST([argp_LDADD])
 dnl The directories with content.
 
 dnl Documentation.
-dnl Commented out for now.
-dnl AC_CONFIG_FILES([doc/Makefile])
+AC_CONFIG_FILES([doc/Makefile])
 
 dnl Support library.
 AC_CONFIG_FILES([lib/Makefile])
diff --git a/doc/COPYING b/doc/COPYING
new file mode 100644
index ..98310abe
--- /dev/null
+++ b/doc/COPYING
@@ -0,0 +1,455 @@
+This license applies to the eu-readelf.1 man page which was forked
+from the binutils readelf version of the man page. The rest of the
+documentation is provided under the license found in the top level
+directory.
+
+GNU Free Documentation License
+ Version 1.3, 3 November 2008
+
+
+ Copyright (C) 2000, 2001, 2002, 2007, 2008 Free Software Foundation, Inc.
+ 
+ Everyone is permitted to copy and distribute verbatim copies
+ of this license document, but changing it is not allowed.
+
+0. PREAMBLE
+
+The purpose of this License is to make a manual, textbook, or other
+functional and useful document "free" in the sense of freedom: to
+assure everyone the effective freedom to copy and redistribute it,
+with or without modifying it, either commercially or noncommercially.
+Secondarily, this License preserves for the author and publisher a way
+to get credit for their work, while not being considered responsible
+for modifications made by others.
+
+This License is a kind of "copyleft", which means that derivative
+works of the document must themselves be free in the same sense.  It
+complements the GNU General Public License, which is a copyleft
+license designed for free software.
+
+We have designed this License in order to use it for manuals for free
+software, because free software needs free documentation: a free
+program should come with manuals providing the same freedoms that the
+software does.  But this License is not limited to software manuals;
+it can be used for any textual work, regardless of subject matter or
+whether it is published as a printed book.  We recommend this License
+principally for works whose purpose is instruction or reference.
+
+
+1. APPLICABILITY AND DEFINITIONS
+
+This License applies to any manual or other work, in any medium, that
+contains a notice placed by the copyright holder saying it can be
+distributed under the terms of this License.  Such a notice grants a
+world-wide, royalty-free license, unlimited in duration, to use that
+work under the conditions stated herein.  The "Document", below,
+refers to any such manual or work.  Any member of the public is a
+licensee, and is addressed as "you".  You accept the license if you
+copy, modify or distribute the work in a way requiring permission
+under copyright law.
+
+A "Modified Version" of the Document means any work containing the
+Document or a portion of it, either copied verbatim, or with
+modifications and/or translated into another language.
+
+A "Secondary Section" is a named appendix or a front-matter section of
+the Document that deals exclu

Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()

[Mark Wielaard]

On Wed, 2019-08-21 at 09:20 -0500, Jonathon Anderson wrote:
> First message failed to send, hopefully this one works...

Just for the record, the mailinglist did reject HTML
messages/attachments. It should have been changed now to simply strip
the HTML.

Cheers,

Mark

Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()

[Mark Wielaard]

On Wed, 2019-08-21 at 17:21 -0500, Jonathon Anderson wrote:
> > P.S. It looks like something decided to add some line breaks in the
> > patch so that it doesn't easily apply. It isn't hard to fixup, but you
> > might want to consider submitting using git send-email or attaching 
> > the result of git format-patch instead of putting the patch in the
> > message body.
> 
> Originally I had some issues with git send-mail, I usually do PRs 
> purely in git so the email side is still a little new. I've attached 
> the original patch from git format-patch, sorry for the mess.

No worries. The project is a bit old-fashioned. We still do everything
through email. It does make (free form) discussions of patches simpler.
But occasionally an email transport might mangle the actual code.

If it is easier please feel free to include a reference to a
repo/branch from which to pull a patch (or use a git request-pull
message).

Thanks,

Mark

Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()

[Jonathon Anderson]

On Fri, Aug 23, 2019 at 1:25 PM, Mark Wielaard  wrote:

Hi,

On Wed, 2019-08-21 at 09:08 -0500, Jonathon Anderson wrote:

 On Wed, Aug 21, 2019 at 6:16 AM, Mark Wielaard 
 wrote:On Fri, 2019-08-16 at 14:24 -0500, Jonathon Anderson wrote:
 > >  For parallel applications that need the information in the 
DIEs, the

 > >  Dwarf_Abbrev hash table et al. become a massive data race. This
 > > fixes
 > >  that by:
 > >
 > >  1. Adding atomics & locks to the hash table to manage 
concurrency

 > > (lib/dynamicsizehash_concurrent.{c,h})
 > >  2. Adding a lock & array structure to the memory manager
 > > (pseudo-TLS)
 > > (libdwP.h, libdw_alloc.c)
 > >  3. Adding extra configure options for Helgrind/DRD annotations
 > > (configure.ac)
 > >  4. Including "stdatomic.h" from FreeBSD, to support C11-style
 > > atomics.
 > > (lib/stdatomic.h)
 >
 > This looks like really nice work. Thanks!
 >
 > I am splitting review in some smaller parts if you don't mind.
 > Simply because it is large and I cannot keep everything in my 
head at

 > once :)


BTW. I would prefer to handle this as 4 separate additions, probably 
in

this order:

1) configure stuff for valgrind annotations.
2) add support for stdatomic.h functions.
3) thread-safe obstack memory handling
4) concurrent dynamic hash table.


Sure thing, I can split the patch into bits over the weekend. I may 
take your advice and just use git request-pull though.





 > If the compiler provides stdatomic.h then I think it would be good
 > to
 > use that instead of our own implementation. The copyright isn't a
 > problem. But do you have a reference/URL to the upstream version? 
I
 > like to add that somewhere, so we can sync with it in the future. 
I

 > see
 > various commented out parts. Was that already upstream? Should we 
just

 > remove those parts?

 It would definitely be preferable to use the compiler's 
implementation
 if possible, we used this in case GCC 4.7 and 4.8 (RHEL7) 
compatibility

 was needed. If those versions are old enough the file can be removed
 entirely.

 The upstream is at
 https://github.com/freebsd/freebsd/blob/master/sys/sys/stdatomic.h,
 although the version here appears to be slightly modified (we used 
the

 version that HPCToolkit ships). The components we use don't seem
 affected, so a resync shouldn't make a difference.


OK, then we should come up with some kind of configure test to see if
we can use the standard stdatomic.h and otherwise use our own. I am
surprised I cannot find other projects doing this. Would be nice to
"steal" something standard for this.


At least OpenSSL does it: 
https://github.com/openssl/openssl/blob/master/include/internal/refcount.h, 
the interesting note being that it has a series of fallbacks (various 
compiler builtins and then locks). The other projects I skimmed just 
have the fallbacks and don't check for C11, given that Elfutils only 
supports GCC that might be a valid (and more compact) approach.





 > >   - Currently the concurrent hash table is always enabled,
 > > performance-wise there is no known difference between it
 > > and the non-concurrent  version.
 > > This can be changed to toggle with --enable-thread-safety
 > > if preferred.
 >
 > I would prefer it always enabled, unless there is a massive 
slowdown

 > of
 > the single-threaded case. The problem with --enable-thread-safety 
is
 > that it is a) known broken (sigh) and b) it basically introduces 
two
 > separate libraries that behave subtly differently. I would very 
much
 > like to get rid of --enable-thread-safety by fixing the broken 
locking

 > and simply making it the default.

 I haven't noticed any slowdown in the single-threaded case, 
although I

 haven't stressed it hard enough to find out for certain. From a
 theoretical standpoint it shouldn't, atomics (with the proper memory
 orders) are usually (on x86 at least) as cheap as normal accesses 
when
 used by a single thread, and the algorithm is otherwise effectively 
the

 same as the original hash table.

 How difficult would it be to fix the locking (or rather, what's
 "broken")? We would definitely benefit from having thread-safety at
 least for getters, which would only need locks around the internal
 management.


To be honest I don't know how badly it is broken.
It is only implemented for libelf.
If you configure --enable-thread-safety and make check you will see
several tests fail because they abort with Unexpected error: Resource
deadlock avoided.

I think it is mainly that nobody maintained the locks and now some are
just wrongly placed. Ideally we switch --enable-thread-safety on by
default, identify which locks are wrongly placed, run all tests with
valgrind/hellgrind and fix any issues found.

It really has not been a priority. Sorry.


No worries, its not a priority on our end either. Elfutils' codebase is 
significantly simpler (IMHO) than ours, so if it ever comes up we'll 
just submit another patch.





 > >   - Anothe