[Debuginfo][DWARF][NFC] Refactor DwarfStringPoolEntryRef.

[avl]

Summary

This review is extracted from D96035.

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. Not all string map entries may be included into the result,
and then not all string entries should have DwarfStringPoolEntry
info. Currently StringMap keeps DwarfStringPoolEntry for all entries.
It leads to extra memory usage. This patch allows to keep
DwarfStringPoolEntry info only for entries which really need it.

[dblaikie]

Might be worth splitting up further - removing the indexed support in a separate patch?

Add possibility to keep not only DwarfStringPoolEntry, but also pointer to it.

Looks like this functionality is added, but untested - it'd be good to include some test coverage for it, somehow? Maybe worth adding a unit test?

(general feedback: avoid else-after-return, please check over the change for a few insntances of that)

[avl]

In D126883#3554448, @dblaikie wrote:

Might be worth splitting up further - removing the indexed support in a separate patch?

Ok.

Add possibility to keep not only DwarfStringPoolEntry, but also pointer to it.

Looks like this functionality is added, but untested - it'd be good to include some test coverage for it, somehow? Maybe worth adding a unit test?

I thought it(test) will come with D96035. Will add it for this review as requested.

(general feedback: avoid else-after-return, please check over the change for a few insntances of that)

[avl]

rebased. added unit test.

[dblaikie]

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

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

llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h

23–26

What's the motivation for this change?

90–91

Remove else-after-return

98–99

[avl]

In 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:

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

[avl]

llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h

23–26

To have fields initialized(so that they do not contain garbage).

[dblaikie]

Thanks for all the context - I think this is nuanced enough I'll have to leave it to some of the Apple folks with more context here. (@aprantl @JDevlieghere @dexonsmith )

[aprantl]

Mechanically this looks fine, but I think the documentation could be improved. Could you add something to the comment of DwarfStringPoolEntryRef that explains what the two cases in the PointerUnion and their use-cases are?

llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h

57

the formatting here is weird, it's not a doxygen comment, but uses \p...
But more importantly, it's not clear whether this describes a property of DwarfStringPoolEntryRef, a bug, or something users need to be aware of. Can you make the comment longer?

[avl]

addressed comments: make doc comments to be more detailed.

[JDevlieghere]

llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h

52

Instead of a union, would it make sense to template the DwarfStringPoolEntryRef in the type it holds? And have a typedef for an OwningDwarfStringPoolEntryRef which is a DwarfStringPoolEntryRef<DwarfStringPoolEntry> and a NonOwningDwarfStringPoolEntryRef which would be a DwarfStringPoolEntryRef<DwarfStringPoolEntry>?

[JDevlieghere]

I missed the original discussion about templating the class. It seems like that issue can be solved with the same solution we have form something like SmallVector where we pass the generic base class SmallVectorImpl around. I think we should be able to do something similar here?

[avl]

In D126883#3568729, @JDevlieghere wrote:

I missed the original discussion about templating the class. It seems like that issue can be solved with the same solution we have form something like SmallVector where we pass the generic base class SmallVectorImpl around. I think we should be able to do something similar here?

I.e. something like this:

template <typename T>
DwarfStringPoolEntryRef (
  MCSymbol *getSymbol() const;
  uint64_t getOffset() const;
  unsigned getIndex() const;
  StringRef getString() const;
  const DwarfStringPoolEntry &getEntry() const;
)


OwningDwarfStringPoolEntryRef : public DwarfStringPoolEntryRef<DwarfStringPoolEntry> (
)

NonOwningDwarfStringPoolEntryRef : public DwarfStringPoolEntryRef<DwarfStringPoolEntry*> (
)


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

template<typename T>
class DIEString {
  DwarfStringPoolEntryRef<T>& S;

}

?

[avl]

@JDevlieghere Jonas, Could you take a look at above example, please? Did I correctly understand your suggestion?

[JDevlieghere]

In D126883#3571202, @avl wrote:

In D126883#3568729, @JDevlieghere wrote:

I missed the original discussion about templating the class. It seems like that issue can be solved with the same solution we have form something like SmallVector where we pass the generic base class SmallVectorImpl around. I think we should be able to do something similar here?

I.e. something like this:

template <typename T>
DwarfStringPoolEntryRef (
  MCSymbol *getSymbol() const;
  uint64_t getOffset() const;
  unsigned getIndex() const;
  StringRef getString() const;
  const DwarfStringPoolEntry &getEntry() const;
)


OwningDwarfStringPoolEntryRef : public DwarfStringPoolEntryRef<DwarfStringPoolEntry> (
)

NonOwningDwarfStringPoolEntryRef : public DwarfStringPoolEntryRef<DwarfStringPoolEntry*> (
)


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

template<typename T>
class DIEString {
  DwarfStringPoolEntryRef<T>& S;

}

?

Almost, what I had in mind was this:

DwarfStringPoolEntry {
  ...
}

DwarfStringPoolEntryRefImpl {
  // Everything that *doesn't* depend on whether it's backed by a DwarfStringPoolEntry or DwarfStringPoolEntry*. This should be able to cover your entire public interface. 
}

template<typename T>
DwarfStringPoolEntryRef : public DwarfStringPoolEntryRefImpl {
  // Everything that *does* depend on the type of T. 
}

// The (Non)OwningDwarfStringPoolEntryRef are now just typedefs.
typedef OwningDwarfStringPoolEntryRef = DwarfStringPoolEntryRef<DwarfStringPoolEntry*>;
typedef NonOwningDwarfStringPoolEntryRef = DwarfStringPoolEntryRef<DwarfStringPoolEntry*>;

// The accelerator table can remain non-templated by only interacting with the DwarfStringPoolEntryRefImpl:

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

[avl]

In D126883#3582133, @JDevlieghere wrote:

Almost, what I had in mind was this:

DwarfStringPoolEntry {
  ...
}

DwarfStringPoolEntryRefImpl {
  // Everything that *doesn't* depend on whether it's backed by a DwarfStringPoolEntry or DwarfStringPoolEntry*. This should be able to cover your entire public interface. 
}

template<typename T>
DwarfStringPoolEntryRef : public DwarfStringPoolEntryRefImpl {
  // Everything that *does* depend on the type of T. 
}

// The (Non)OwningDwarfStringPoolEntryRef are now just typedefs.
typedef OwningDwarfStringPoolEntryRef = DwarfStringPoolEntryRef<DwarfStringPoolEntry*>;
typedef NonOwningDwarfStringPoolEntryRef = DwarfStringPoolEntryRef<DwarfStringPoolEntry*>;

// The accelerator table can remain non-templated by only interacting with the DwarfStringPoolEntryRefImpl:

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

yep, thank you.

It looks like this solution does not cover one original property of DwarfStringPoolEntryRef.
Current DwarfStringPoolEntryRef assumed to be passed by value.
That means that no neccessary to have extra storage to keep it:

CodeGen/AccelTable.h

struct HashData {
  DwarfStringPoolEntryRef Name;
  ...
}

CodeGen/DIE.h

class DIEString {
  DwarfStringPoolEntryRef S;
}

If we will pass it by reference:

CodeGen/AccelTable.h

struct HashData {
  DwarfStringPoolEntryRefImpl& Name;
  ...
}

CodeGen/DIE.h

class DIEString {
  DwarfStringPoolEntryRefImpl& S;
}

Then we need to add some containers and do more copies for OwningDwarfStringPoolEntryRef or NonOwningDwarfStringPoolEntryRef:

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

The addName should call special copy for the Name(which would copy real object referenced by DwarfStringPoolEntryRefImpl) and store it in an additional container.

class DIEString {
  DwarfStringPoolEntryRef& S;
}

The DIEString should be changed somehow like this:

class DIEString {
  std::unique_ptr<DwarfStringPoolEntryRef> S;
}

Would it be OK?

[JDevlieghere]

Ah that's a good point. Yep the DIEString approach looks good.

[avl]

In D126883#3583613, @JDevlieghere wrote:

Ah that's a good point. Yep the DIEString approach looks good.

I would suggest to use solution from this patch instead of suggestion with DwarfStringPoolEntryRefImpl.

The drawbacks of suggestion with DwarfStringPoolEntryRefImpl:

1. It leads to extra memory usage and fragmentation since many additional objects would be allocated.
2. It requires to change many places where DwarfStringPoolEntryRef is currently used.

class NonRelocatableStringpool {
  std::unique_ptr<DwarfStringPoolEntryRefImpl> getEntry(StringRef S);
}

class DIEString {
  std::unique_ptr<DwarfStringPoolEntryRefImpl> S;
}

class AccelTableBase {
public:
  /// Represents a group of entries with identical name (and hence, hash value).
  struct HashData {
    std::unique_ptr<DwarfStringPoolEntryRefImpl> Name;
 }
}
...

3. It introduces new performance penalties, because std::unique_ptr de-referencing and virtual function call would be added while accessing DwarfStringPoolEntryRef methods.

The solution from this patch does not introduce new memory allocations, It does not change usage points of DwarfStringPoolEntryRef, It adds only "MapEntry.is<ByValStringEntryPtr>" check while accessing DwarfStringPoolEntryRef methods.

What do you think?

[avl]

@JDevlieghere friendly ping.

[JDevlieghere]

Thanks for the ping. Fair enough. This LGTM.

[avl]

@JDevlieghere Thank you for the review!

[vitalybuka]

Msan complains about this patch https://lab.llvm.org/buildbot/#/builders/74/builds/11664/steps/17/logs/stdio

[ RUN      ] DwarfStringPoolEntryRefTest.TestShortEntry
Uninitialized bytes in MemcmpInterceptorCommon at offset 20 inside [0x7ffc5d135098, 24)
==2198==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x562f6d41b1fc in __interceptor_bcmp /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:978:33
    #1 0x562f6d41b1fc in bcmp /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:973:1
    #2 0x562f6d55d54b in DwarfStringPoolEntryRefTest_TestShortEntry_Test::TestBody() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/unittests/CodeGen/DwarfStringPoolEntryRefTest.cpp:97:3
    #3 0x562f75fec7e4 in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #4 0x562f75fec7e4 in testing::Test::Run() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2508:5
    #5 0x562f75ff14c4 in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
    #6 0x562f75ff3070 in testing::TestSuite::Run() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
    #7 0x562f76025691 in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
    #8 0x562f76023dff in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #9 0x562f76023dff in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
    #10 0x562f6d7918cf in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2472:46
    #11 0x562f6d7918cf in main /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/unittests/CodeGen/TargetOptionsTest.cpp:76:10
    #12 0x7f6d31eb409a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: eb6a5dd378d22b1e695984462a799cd4c81cdc22)
    #13 0x562f6d3c3d69 in _start (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/unittests/CodeGen/CodeGenTests+0x2f80d69)
  Uninitialized value was created by an allocation of 'DwarfEntry2' in the stack frame of function '_ZN47DwarfStringPoolEntryRefTest_TestShortEntry_Test8TestBodyEv'
    #0 0x562f6d558fc0 in DwarfStringPoolEntryRefTest_TestShortEntry_Test::TestBody() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/unittests/CodeGen/DwarfStringPoolEntryRefTest.cpp:57
SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:978:33 in __interceptor_bcmp
Exiting

llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h

 //===- llvm/CodeGen/DwarfStringPoolEntry.h - String pool entry --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_DWARFSTRINGPOOLENTRY_H
 #define LLVM_CODEGEN_DWARFSTRINGPOOLENTRY_H
 
-#include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/StringMap.h"
 
 namespace llvm {
 
 class MCSymbol;
 
 /// Data for a string pool entry.
 struct DwarfStringPoolEntry {
   static constexpr unsigned NotIndexed = -1;
 
-  MCSymbol *Symbol;
-  uint64_t Offset;
-  unsigned Index;
+  MCSymbol *Symbol = nullptr;
+  uint64_t Offset = 0;
+  unsigned Index = 0;
 
   bool isIndexed() const { return Index != NotIndexed; }
 };
 
-/// String pool entry reference.
+/// DwarfStringPoolEntryRef: Dwarf string pool entry reference.
+///
+/// Dwarf string pool entry keeps string value and its data.
+/// There are two variants how data are represented:
+///
+///   1. By value - StringMapEntry<DwarfStringPoolEntry>.
+///   2. By pointer - StringMapEntry<DwarfStringPoolEntry *>.
+///
+/// The "By pointer" variant allows for reducing memory usage for the case
+/// when string pool entry does not have data: it keeps the null pointer
+/// and so no need to waste space for the full DwarfStringPoolEntry.
+/// It is recommended to use "By pointer" variant if not all entries
+/// of dwarf string pool have corresponding DwarfStringPoolEntry.
+
 class DwarfStringPoolEntryRef {
-  const StringMapEntry<DwarfStringPoolEntry> *MapEntry = nullptr;
+  /// Pointer type for "By value" string entry.
+  using ByValStringEntryPtr = const StringMapEntry<DwarfStringPoolEntry> *;
 
-  const StringMapEntry<DwarfStringPoolEntry> *getMapEntry() const {
-    return MapEntry;
-  }
+  /// Pointer type for "By pointer" string entry.
+  using ByPtrStringEntryPtr = const StringMapEntry<DwarfStringPoolEntry *> *;
+
+  /// Pointer to the dwarf string pool Entry.
+  PointerUnion<ByValStringEntryPtr, ByPtrStringEntryPtr> MapEntry = nullptr;
 
 public:
   DwarfStringPoolEntryRef() = default;
+
+  /// ASSUMPTION: DwarfStringPoolEntryRef keeps pointer to \p Entry,
+  /// thus specified entry mustn`t be reallocated.
   DwarfStringPoolEntryRef(const StringMapEntry<DwarfStringPoolEntry> &Entry)
       : MapEntry(&Entry) {}
 
-  explicit operator bool() const { return getMapEntry(); }
+  /// ASSUMPTION: DwarfStringPoolEntryRef keeps pointer to \p Entry,
+  /// thus specified entry mustn`t be reallocated.
+  DwarfStringPoolEntryRef(const StringMapEntry<DwarfStringPoolEntry *> &Entry)
+      : MapEntry(&Entry) {
+    assert(MapEntry.get<ByPtrStringEntryPtr>()->second != nullptr);
+  }
+
+  explicit operator bool() const { return !MapEntry.isNull(); }
+
+  /// \returns symbol for the dwarf string.
   MCSymbol *getSymbol() const {
-    assert(getMapEntry()->second.Symbol && "No symbol available!");
-    return getMapEntry()->second.Symbol;
+    assert(getEntry().Symbol && "No symbol available!");
+    return getEntry().Symbol;
   }
-  uint64_t getOffset() const { return getMapEntry()->second.Offset; }
+
+  /// \returns offset for the dwarf string.
+  uint64_t getOffset() const { return getEntry().Offset; }
+
+  /// \returns index for the dwarf string.
   unsigned getIndex() const {
-    assert(getMapEntry()->getValue().isIndexed());
-    return getMapEntry()->second.Index;
+    assert(getEntry().isIndexed() && "Index is not set!");
+    return getEntry().Index;
+  }
+
+  /// \returns string.
+  StringRef getString() const {
+    if (MapEntry.is<ByValStringEntryPtr>())
+      return MapEntry.get<ByValStringEntryPtr>()->first();
+
+    return MapEntry.get<ByPtrStringEntryPtr>()->first();
+  }
+
+  /// \returns the entire string pool entry for convenience.
+  const DwarfStringPoolEntry &getEntry() const {
+    if (MapEntry.is<ByValStringEntryPtr>())
+      return MapEntry.get<ByValStringEntryPtr>()->second;
+
+    return *MapEntry.get<ByPtrStringEntryPtr>()->second;
   }
-  StringRef getString() const { return getMapEntry()->first(); }
-  /// Return the entire string pool entry for convenience.
-  DwarfStringPoolEntry getEntry() const { return getMapEntry()->getValue(); }
 
   bool operator==(const DwarfStringPoolEntryRef &X) const {
-    return getMapEntry() == X.getMapEntry();
+    return MapEntry.getOpaqueValue() == X.MapEntry.getOpaqueValue();
   }
+
   bool operator!=(const DwarfStringPoolEntryRef &X) const {
-    return getMapEntry() != X.getMapEntry();
+    return MapEntry.getOpaqueValue() != X.MapEntry.getOpaqueValue();
   }
 };
 
 } // end namespace llvm
 
 #endif

llvm/unittests/CodeGen/CMakeLists.txt

 
 add_llvm_unittest(CodeGenTests
   AArch64SelectionDAGTest.cpp
   AllocationOrderTest.cpp
   AMDGPUMetadataTest.cpp
   AsmPrinterDwarfTest.cpp
   DIEHashTest.cpp
   DIETest.cpp
+  DwarfStringPoolEntryRefTest.cpp
   InstrRefLDVTest.cpp
   LowLevelTypeTest.cpp
   LexicalScopesTest.cpp
   MachineInstrBundleIteratorTest.cpp
   MachineInstrTest.cpp
   MachineOperandTest.cpp
   RegAllocScoreTest.cpp
   PassManagerTest.cpp

llvm/unittests/CodeGen/DwarfStringPoolEntryRefTest.cpp

+//===- llvm/unittest/CodeGen/DwarfStringPoolEntryRefTest.cpp --------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/DwarfStringPoolEntry.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <string>
+
+using namespace llvm;
+
+TEST(DwarfStringPoolEntryRefTest, TestFullEntry) {
+  BumpPtrAllocator Allocator;
+  StringMapEntry<DwarfStringPoolEntry> *StringEntry1 =
+      StringMapEntry<DwarfStringPoolEntry>::Create(
+          "Key1", Allocator, DwarfStringPoolEntry{nullptr, 0, 0});
+
+  EXPECT_TRUE(StringEntry1->getKey() == "Key1");
+  EXPECT_TRUE(StringEntry1->second.Symbol == nullptr);
+  EXPECT_TRUE(StringEntry1->second.Offset == 0);
+  EXPECT_TRUE(StringEntry1->second.Index == 0);
+
+  DwarfStringPoolEntryRef Ref1(*StringEntry1);
+  EXPECT_TRUE(Ref1.getString() == "Key1");
+  EXPECT_TRUE(Ref1.getOffset() == 0);
+  EXPECT_TRUE(Ref1.getIndex() == 0);
+
+  DwarfStringPoolEntryRef Ref2(*StringEntry1);
+  EXPECT_TRUE(Ref2.getString() == "Key1");
+  EXPECT_TRUE(Ref2.getOffset() == 0);
+  EXPECT_TRUE(Ref2.getIndex() == 0);
+  EXPECT_TRUE(Ref1 == Ref2);
+  EXPECT_FALSE(Ref1 != Ref2);
+
+  StringMapEntry<DwarfStringPoolEntry> *StringEntry2 =
+      StringMapEntry<DwarfStringPoolEntry>::Create(
+          "Key2", Allocator, DwarfStringPoolEntry{nullptr, 0x1000, 1});
+  EXPECT_TRUE(StringEntry2->getKey() == "Key2");
+  EXPECT_TRUE(StringEntry2->second.Symbol == nullptr);
+  EXPECT_TRUE(StringEntry2->second.Offset == 0x1000);
+  EXPECT_TRUE(StringEntry2->second.Index == 1);
+
+  DwarfStringPoolEntryRef Ref3(*StringEntry2);
+  EXPECT_TRUE(Ref3.getString() == "Key2");
+  EXPECT_TRUE(Ref3.getOffset() == 0x1000);
+  EXPECT_TRUE(Ref3.getIndex() == 1);
+  EXPECT_TRUE(Ref1 != Ref3);
+}
+
+TEST(DwarfStringPoolEntryRefTest, TestShortEntry) {
+  BumpPtrAllocator Allocator;
+  DwarfStringPoolEntry DwarfEntry1 = {nullptr, 0, 0};
+  StringMapEntry<DwarfStringPoolEntry *> *StringEntry1 =
+      StringMapEntry<DwarfStringPoolEntry *>::Create("Key1", Allocator,
+                                                     &DwarfEntry1);
+
+  EXPECT_TRUE(StringEntry1->getKey() == "Key1");
+  EXPECT_TRUE(StringEntry1->second->Symbol == nullptr);
+  EXPECT_TRUE(StringEntry1->second->Offset == 0);
+  EXPECT_TRUE(StringEntry1->second->Index == 0);
+
+  DwarfStringPoolEntryRef Ref1(*StringEntry1);
+  EXPECT_TRUE(Ref1.getString() == "Key1");
+  EXPECT_TRUE(Ref1.getOffset() == 0);
+  EXPECT_TRUE(Ref1.getIndex() == 0);
+  EXPECT_TRUE(memcmp(&Ref1.getEntry(), &DwarfEntry1,
+                     sizeof(DwarfStringPoolEntry)) == 0);
+
+  DwarfStringPoolEntryRef Ref2(*StringEntry1);
+  EXPECT_TRUE(Ref2.getString() == "Key1");
+  EXPECT_TRUE(Ref2.getOffset() == 0);
+  EXPECT_TRUE(memcmp(&Ref2.getEntry(), &DwarfEntry1,
+                     sizeof(DwarfStringPoolEntry)) == 0);
+  EXPECT_TRUE(Ref1 == Ref2);
+  EXPECT_FALSE(Ref1 != Ref2);
+
+  DwarfStringPoolEntry DwarfEntry2 = {nullptr, 0x1000, 1};
+  StringMapEntry<DwarfStringPoolEntry *> *StringEntry2 =
+      StringMapEntry<DwarfStringPoolEntry *>::Create("Key2", Allocator,
+                                                     &DwarfEntry2);
+  EXPECT_TRUE(StringEntry2->getKey() == "Key2");
+  EXPECT_TRUE(StringEntry2->second->Symbol == nullptr);
+  EXPECT_TRUE(StringEntry2->second->Offset == 0x1000);
+  EXPECT_TRUE(StringEntry2->second->Index == 1);
+
+  DwarfStringPoolEntryRef Ref3(*StringEntry2);
+  EXPECT_TRUE(Ref3.getString() == "Key2");
+  EXPECT_TRUE(Ref3.getOffset() == 0x1000);
+  EXPECT_TRUE(Ref3.getIndex() == 1);
+  EXPECT_TRUE(memcmp(&Ref3.getEntry(), &DwarfEntry2,
+                     sizeof(DwarfStringPoolEntry)) == 0);
+  EXPECT_TRUE(Ref1 != Ref3);
+}
+
+TEST(DwarfStringPoolEntryRefTest, CompareFullAndShort) {
+  BumpPtrAllocator Allocator;
+
+  DwarfStringPoolEntry DwarfEntry1 = {nullptr, 0, 0};
+  StringMapEntry<DwarfStringPoolEntry *> *StringEntry1 =
+      StringMapEntry<DwarfStringPoolEntry *>::Create("Key1", Allocator,
+                                                     &DwarfEntry1);
+  DwarfStringPoolEntryRef Ref1(*StringEntry1);
+
+  StringMapEntry<DwarfStringPoolEntry> *StringEntry2 =
+      StringMapEntry<DwarfStringPoolEntry>::Create(
+          "Key1", Allocator, DwarfStringPoolEntry{nullptr, 0, 0});
+  DwarfStringPoolEntryRef Ref2(*StringEntry2);
+
+  EXPECT_FALSE(Ref1 == Ref2);
+}