This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Reduce debug_str_offsets section size
ClosedPublic

Authored by labath on Jul 18 2018, 9:27 AM.

Details

Summary

The accelerator tables use the debug_str section to store their strings.
However, they do not support the indirect method of access that is
available for the debug_info section (DW_FORM_strx et al.).

Currently our code is assuming that all strings can/will be referenced
indirectly, and puts all of them into the debug_str_offsets section.
This is generally true for regular (unsplit) dwarf, but in the DWO case,
most of the strings in the debug_str section will only be used from the
accelerator tables. Therefore the contents of the debug_str_offsets
section will be largely unused and bloating the main executable.

This patch rectifies this by teaching the DwarfStringPool to
differentiate between strings accessed directly and indirectly. When a
user inserts a string into the pool it has to declare whether that
string will be referenced directly or not. If at least one user requsts
indirect access, that string will be assigned an index ID and put into
debug_str_offsets table. Otherwise, the offset table is skipped.

This approach reduces the overall binary size (when compiled with
-gdwarf-5 -gsplit-dwarf) in my tests by about 2% (debug_str_offsets is
shrunk by 99%).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jul 18 2018, 9:27 AM
mgrang added inline comments.Jul 18 2018, 10:37 AM
lib/CodeGen/AsmPrinter/DwarfStringPool.cpp
59 ↗(On Diff #156095)

A few typos.

I would like a test that did the following: add string "A" not indexed; add string "B" indexed; add string "A" indexed.
Then show that the string section has "A" followed by "B", and the offsets table is correct (entry 0 points to "B", entry 1 points to "A").
Is that feasible?

lib/CodeGen/AsmPrinter/DwarfStringPool.cpp
59 ↗(On Diff #156095)

llvm::sort

lib/CodeGen/AsmPrinter/DwarfStringPool.h
52 ↗(On Diff #156095)

s/it's/its/

test/DebugInfo/X86/string-offsets-table.ll
97 ↗(On Diff #156095)

... offsets of strings ...

JDevlieghere added inline comments.Jul 19 2018, 2:39 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2407 ↗(On Diff #156095)

Would it make sense to make false the default?

labath marked 3 inline comments as done.Jul 19 2018, 3:51 AM

I would like a test that did the following: add string "A" not indexed; add string "B" indexed; add string "A" indexed.
Then show that the string section has "A" followed by "B", and the offsets table is correct (entry 0 points to "B", entry 1 points to "A").
Is that feasible?

It's feasible, but just barely. In a non-dwo build the accel table (the only source of non-indexed strings) will not contain any new strings, as everything will be referenced (and indexed) from .debug_info. OTOH, in a dwo build the .debug_str section contains almost exclusively (non-indexed) accelerator table entries. AFAICT, the only exceptions are the DW_AT_GNU_dwo_name and DW_AT_comp_dir attributes of the skeleton units. I was able to tickle this by having three compile units in a single .ll file and having the compilation directories of some units match the variable names of others, but I fear the resulting test might be a bit brittle.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2407 ↗(On Diff #156095)

I wouldn't recommend it as I think it increases the chance of getting things wrong. (e.g. when writing this patch I definitely wanted to make the arg explicit so I could audit each usage for the correct value of the argument).

If you're worried about verbosity, one way I've considered addressing this would be to have an AccelTable::addName overload which accepts a StringPool and call's getEntry itself. This would have the extra advantage that the decision of whether to create an Indexed entry or not is moved closer to the code which does the actual emission (the part that knows whether it needs the index or not).

labath updated this revision to Diff 156228.Jul 19 2018, 3:52 AM

Fix typos and add the extra test.

JDevlieghere added inline comments.Jul 19 2018, 4:13 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2407 ↗(On Diff #156095)

That's what I figured, but I agree a strong interface is worth being a little more verbose. The overload sounds like a good idea.

labath added inline comments.Jul 19 2018, 5:53 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2407 ↗(On Diff #156095)

I tried that out, but I wasn't too happy with the result. I've had to move DwarfStringPool.h from lib/ to the include/ folder and add an extra AsmPrinter (in addition to the StringPool) argument to the addName method.

Instead I propose to do something like https://reviews.llvm.org/D49542, which is to keep the StringPool code in DwarfDebug, but collapse all of these getEntry calls into one.

I agree the ordering test looks fragile. The only other option that comes to mind is trying to exercise the APIs directly from a unittest, but then you need enough scaffolding around it to capture at least the assembler output and verify it. Probably best to leave the test as it is.

LGTM but I'll let @JDevlieghere have the last word. It looks like he still has one open question in DwarfDebug.cpp.

JDevlieghere accepted this revision.Jul 19 2018, 9:56 AM

I'm happy with this, LGTM!

This revision is now accepted and ready to land.Jul 19 2018, 9:56 AM

Anyone thought about whether it'd be worth making the indexed V unindexed
case work implicitly? If we were willing to add another pointer to
DwarfStringPoolEntryRef (doubling its size, admittedly) - back to the
string pool itself - then we could index any string where the
DwarfStringPoolEntryRef was asked for an index, and otherwise skip that?
Rather than having to declare up-front whether an index was required.

(if we wanted to get super-fancy, maybe we could emit indexes as assembly
expressions ((str_label/str_begin_label)/word_size) then you wouldn't have
to increase the size of DwarfStringPoolEntryRef, because the absolute index
wouldn't have to be known ahead of time - it'd be computed by the assembler
instead - which is just moving the work around, of course)

Couple of other possible ideas:

splitting the indexed and unindexed functions into separate, named functions for clarity (alternatively - 'named boolean' (so they don't have to be commented) by adding an enum class for legibility).

Also, possible that there could be assert/runtime checking to ensure that the caller who requests an unindexed string - could use a PointerIntPair & squirrel a boolean in the low bit to say "this DwarfStringPoolEntryRef can not provide an index"? (so that there's no chance of weird race conditions that would hide bugs - ask for an unindexed string, then later ask for that same string indexed, then the first user can request the index without a failure - until later on the second user goes away/changes/etc)

dblaikie added inline comments.Jul 19 2018, 1:26 PM
test/DebugInfo/X86/string-offsets-table-order.ll
28–31 ↗(On Diff #156228)

Rather than hardcoding these particular byte offset prefixes (honestly, probably not necessary for the dumper to dump these except in the most verbose modes maybe) - but use CHECK-NEXT instead, to ensure these are the only 4 strings (CHECK something at the end to ensure that's the end of the list - though I realize the "contribution size" validates that, but again, probably easier not to check for that specific value (easier to update the test with new strings if it's just checking for the specific elements in a list, without checking offsets, sizes, etc)

labath updated this revision to Diff 156511.Jul 20 2018, 9:12 AM
labath marked 5 inline comments as done.
  • Make the API more explicit. Of the options proposed by David, I chose to make the string pool api have two methods (getEntry/getIndexedEntry), and also I've stashed a bit into DwarfStringPoolEntryRef to store the original mode in which the reference was obtained. Now it will assert if someone obtains a non-indexed reference, but then later ask for it's index.

    Emitting the indexes does not seem feasible, as we currently have code which assumes they are known statically (so it can compute the smallest DW_FORM_strxN in which they fit).

    Making the assignment of indexes automatical would be possible, but it seemed like a pessimization that is not worth it. We currently call get(Indexed)Entry in three places and in each of them it is quite obvious how will the resulting string be used.
labath added inline comments.Jul 20 2018, 9:12 AM
test/DebugInfo/X86/string-offsets-table-order.ll
28–31 ↗(On Diff #156228)

I've updated this a bit, though I'm not sure I did exactly what you wanted. PTAL.

labath updated this revision to Diff 156795.Jul 23 2018, 8:23 AM
  • rebase the patch on top of D49670
  • add a unittest-style test of the scenario Paul wanted (so far I've kept both tests, let me know which one looks better)
  • fix a bug in dsymutil which was exposed when turing assertions on. The issue was that dsymutil is using the same string pool (it seems to me) both for emitting strings into the debug info, and as a general string pool for interning strings. It tells the difference by marking the symbol's index as -1, which is the same value I used for non-indexed strings. This caused an assertion to fire when we were sorting DwarfStringPoolEntryRef for emission. Fortunately, the fix is simple -- instead of sorting the entries which will never be emitted, I just never put them in the emission list in the first place (NonrelocatableStringPool::getEntries). To make it extra explicit that the list returned by getEntries does not contain all strings in the pool, I rename the function to getEntriesForEmission.
  • fix a bug where we would needlessly emit a .debug_str_offsets header even though the string pool contained zero indexed strings.
labath added a comment.Aug 7 2018, 2:54 AM

I nearly forgot I still have this patch pending. I think I addressed all issues raised, and it is the accepted state, so I am going to commit it today. There are two minor issues that I haven't received final feedback on, but these can be easily tweaked post-commit too:

  • testing: there are now two types of tests (unit test and lit) for the scenario of mixing indexed and unindexed entries. One of them could be removed if needed.
  • is the additional safety embedded into DwarfStringPoolEntryRef sufficient?
This revision was automatically updated to reflect the committed changes.