[PATCH] D126883: [Debuginfo][DWARF][NFC] Refactor DwarfStringPoolEntryRef.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D126883#3561946 <https://reviews.llvm.org/D126883#3561946>, @dblaikie wrote:

> The code looks mechanically OK, but I'm not quite following the patch description/motivation. Perhaps it could be rephrased?
>
>> This patch adds possibility to keep not only DwarfStringPoolEntry, but also pointer to it. The DwarfStringPoolEntryRef keeps reference to the string map entry. String map keeps string data and corresponding DwarfStringPoolEntry info.
>
> I /think/ I'm following this part, at least. Might be worth pointing to specific members/classes - which string map in particular are you referring to here?
>
>> Not all string map entries may be included into the result, and then not all string entries should have DwarfStringPoolEntry info.
>
> OK, I sort of follow - this is for strings in input .o files that, due to some DWARF dead-stripping, are no longer used in the output? If that's the case, then I'd expect that the values in the map should become unique_ptrs, perhaps? (or raw pointers, if they can be allocated by a pool allocator of some kind).

rephrased description:

This patch adds possibility to DwarfStringPoolEntryRef to keep not only DwarfStringPoolEntry, but also pointer to it. 
The DwarfStringPoolEntryRef keeps reference to the string map entry. 
String map keeps string data and corresponding DwarfStringPoolEntry info:

NonRelocatableStringpool.h:

  class NonRelocatableStringpool {
    using MapTy = StringMap<DwarfStringPoolEntry, BumpPtrAllocator>;  
  }

The DwarfStringPoolEntryRef can reference string map entries of NonRelocatableStringpool.
Not all string map entries may be included into the result, and then not all string entries
should have DwarfStringPoolEntry info. Examples:

1. NonRelocatableStringpool.h:

  class NonRelocatableStringpool {
    DwarfStringPoolEntryRef getEntry(StringRef S);  <<< add string for emission(use DwarfStringPoolEntry)
    StringRef internString(StringRef S); <<< add string not for emission(i.e. may not use DwarfStringPoolEntry)
  }



2. DWARFLinkerDeclContext.h:

  /// String pool keeping real path bodies.
  NonRelocatableStringpool StringPool;  <<< does not use DwarfStringPoolEntry



3. D96035 <https://reviews.llvm.org/D96035>:

It uses single string pool for all strings. Not all strings would be put into the final result(
some strings are internal names, some strings would be translated and then should not be put into the result
, some strings relate to garbage collected data and then should not be put into the final result).

> But I'm not sure why that would change how the *Ref class works - oh, because some users of this class don't want/need to change their map? (can you point to the different maps/use cases?)

Right. There are users of DwarfStringPoolEntryRef and this patch tries to pass newly organized data to them.

In the D96035 <https://reviews.llvm.org/D96035> I do not use NonRelocatableStringpool since it could not be used in multi-thread mode.
Instead, I use hash map from this review https://reviews.llvm.org/D125979:

llvm/include/llvm/DWARFLinkerNext/StringPool.h:

  ConcurrentHashTable<StringRef, StringMapEntry<DwarfStringPoolEntry *>, BumpPtrAllocator>

Some of the strings from ConcurrentHashTable should be passed to AccelTable:

  template <typename DataT> class AccelTable : public AccelTableBase {
  public:
    template <typename... Types>
    void addName(DwarfStringPoolEntryRef Name, Types &&... Args);  <<<<<<<
  };

So, DwarfStringPoolEntryRef should be able to point to the StringMapEntry<DwarfStringPoolEntry *>.

> Then maybe this class should become templated rather than using a union?

If we make DwarfStringPoolEntryRef to be templated then we need to add more template parameters to the 
AccelTable class(and all other users of DwarfStringPoolEntryRef):

  template <typename DataT, **typename EntryT**> class AccelTable : public AccelTableBase {
  public:
    template <typename... Types>
    void addName(DwarfStringPoolEntryRef**<EntryT>** Name, Types &&... Args);
  };

Would it be OK?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126883/new/

https://reviews.llvm.org/D126883