[Rpm-maint] [PATCH] debugedit: Support String/Line table rewriting for larger/smaller paths.

[Mark Wielaard]

On Sat, 2017-03-04 at 11:28 +0200, Panu Matilainen wrote:
> I dont feel qualified to really review this, and perhaps others are 
> feeling the same way since it's been out there for a week now with no 
> comments at all.
> 
> Because this seems quite awesome (even if also a bit scary), to avoid 
> stalling forever with no-one to review it: if there are no objections 
> raised by Monday I'm going to just apply it (and the couple of other 
> recent debuginfo patches). So anybody having doubts, speak up now.

Thanks. Obviously I think the patch is awesome and should be integrated
sooner rather than later :) But being slightly worried this means "you
touched it last, you own it now", let me do at least my own review.

Questions I would ask about this patch if someone else would have
submitted it:

- Why depend on elfutils libebl/libdw for the .debug_str string table
  reconstruction? Isn't a string table just a simple concatenation of
  strings?

It is indeed a simple concatenation of strings, but to pack them
efficiently substrings can be reused/shared. e.g. if the table contains
the string "easy_and_slow" at index 16, then it also contains the string
"slow" at index 25. The elfutils strtab code already handles this
efficiently, so I wanted to depend on that instead of open coding it.

- Why introduce those string storage allocation blocks? Can't you just
  malloc whenever you need a new string?

To make sure we don't leak too much memory we need to track our mallocs
already. strings come either from the existing .debug_str table or from
newly created (replacement) strings. I felt it was a good idea to pack
those strings as closely together because they might be compared often.
So keep them close together helps data cache locality. But this was
probably premature optimisation since we end up at the strings
indirectly through the original indexes and we compare those first (if
the original indexes are equal then the (replacement) strings would be
the same too (and so we end up not having to compare that many strings
anyway). But we needed a structure to keep track of our allocated
strings anyway. So I kept it as is.

- To compare/find existing string table indexes the code uses a binary
  search tree (tsearch), but for line table indexes the code uses a
  simple (sorted) array (bsearch). Why different mechanisms for the same
  kind of lookup?

There are much, much more string table indexes than line table offsets
(e.g. libstdc++.so has 125 line tables, and 10331 full strings [ignoring
shared "tails"]). So for line tables we don't mind if we sometimes have
to do a quick linear search (in phase 0). But for the string indexes it
is important to always be able to search/insert them efficiently. So for
the string table indexes we always use the binary search tree balanced.
For line tables we simply sort the array when we collected all offsets.
The binary search tree does have a bit more memory overhead, but it was
worth it for the fast lookups [*].

- Does the switch from mmap and rawdata to simple read/write and
  elf_data cause much more memory usage?

Yes and no. When mmapping the file the memory is still allocated, but
might not really be used. We do in most cases end up using most of it
because for recalculating the build-id we need to mmap/read in all
section data. The reading in of the ELF data through elf_getdata is
slightly less efficient that elf_rawdata because it might have to do
endian conversion if the ELF file is not native. But in practice it
always is (we just build it). We can probably do better by being smarter
about when/how we update the build-ids. I didn't try to do that yet.

- The relocation rewriting seems scary.

Yes it is :{ Luckily this is only needed for object files (ET_REL) and
normally they aren't packaged. The big exceptions are kernel modules
(why those are ET_REL files and not simply ET_DYN is beyond me) and
static ar archives. If we would have to handle all possible
(architecture specific) relocations this would be really impossible.
Luckily for the indexes into .debug sections only simple relocation
types are used (basically it provides an extra offset into the section).
The line table relocations pointing into code sections are also simple
because we don't care about the actual relocation type, just where in
the section they are. So we can simply move the relocation offset.
We don't handle static ar archives at all. Which is something we might
want to look at at some later point.

- The code ends up touching allocated sections! Isn't that a no-no?

Yes. Anything allocated really shouldn't be touched. Only the
non-allocated (debug) sections should be touched/written out. Sadly a
bug in elfutils means we do end up having to mark the allocated sections
dirty to make sure the whole ELF file ends up correct on disk. This is
really unfortunate and might make things a bit slower. But no
(allocated) data is really changed at all. And once the bug is fixed (in
elfutils 0.169) this isn't not needed anymore.
https://sourceware.org/bugzilla/show_bug.cgi?id=21199

- What about the limitations (not fixed)? How often do they occur in practice?

* DW_AT_comp_dir in .debug_info using DW_FORM_string can not be made
larger. Does happen sometimes if a package contains assembler without
any debuginfo compiled with binutils gas -g. I have seen this happen in
glibc for example. There is a patch to binutils to not do this (it is
also inefficient to generate this kind of debuginfo):
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=49fced1206db40c71208c201165d65f92c69cebe
* A DW_AT_name on a DW_TAG_compile_unit is only rewritten for
DW_FORM_strp not for DW_FORM_string.
I have never seen this happen. And it would only be for an absolute
DW_AT_name path. Which is unlikely to happen (normally DW_AT_comp_dir is
the path and DW_AT_name is just the file name). In theory binutils gas
could generate this variant. But the above patch would solve that. There
could be a warning when this happens like in the above case. I didn't
implement this.
* The debug_line program isn't scanned for DW_LNE_define_file.
I don't know of any DWARF producer that generates that. It probably also
not supported by all consumers. It would be somewhat tricky to support
because we don't really want to rewrite/resize the line program itself.

- How was this tested?

valgrind is your friend! Luckily rpm now contains various testcases for
handling debuginfo packages. Those were really handy while hacking on
this. Because they will fail whenever we would produce bogus ELF data.
The following up patch "Add option to have unique debug source dirs
across version/release/arch" has an explicit testcase that verifies
paths were actually rewritten as intended. Besides those formal tests I
ran debugedit over various larger shared libraries (glibc, libstdc++,
libxul.so) and some kernel modules and compared the output of readelf -w
by hand.

Cheers,

Mark

[*] I did end up submitting a patch to glibc to reduce the size of a
tsearch node a bit.
https://sourceware.org/git/?p=glibc.git;a=commit;h=9d6861b8c3edcb74faedcebb0a6960c01bb6f34d