[PATCH] D49493: [DebugInfo] Reduce debug_str_offsets section size

[Pavel Labath via Phabricator via llvm-commits]

labath marked 3 inline comments as done.
labath added a comment.

In https://reviews.llvm.org/D49493#1166860, @probinson wrote:

> 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.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2407
+  AccelDebugNames.addName(
+      Holder.getStringPool().getEntry(*Asm, Name, /*Indexed*/ false), Die);
 }
----------------
JDevlieghere wrote:
> Would it make sense to make false the default?
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).


Repository:
  rL LLVM

https://reviews.llvm.org/D49493