[llvm] Reduce llvm-gsymutil memory usage (PR #91023)
Kevin Frei via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 26 10:03:44 PDT 2024
https://github.com/kevinfrei updated https://github.com/llvm/llvm-project/pull/91023
>From 3817bfd16ccdbbdab2bab498d381b8e12bcddef1 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Tue, 30 Apr 2024 09:15:12 -0700
Subject: [PATCH 01/11] Release line tables when we're done with them
---
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
index 601686fdd3dd5..9376515e3712f 100644
--- a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
+++ b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
@@ -587,6 +587,8 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
DWARFDie Die = getDie(*CU);
CUInfo CUI(DICtx, dyn_cast<DWARFCompileUnit>(CU.get()));
handleDie(Out, CUI, Die);
+ // Release the line table, once we're done.
+ DICtx.clearLineTableForUnit(CU.get());
}
} else {
// LLVM Dwarf parser is not thread-safe and we need to parse all DWARF up
@@ -612,7 +614,7 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
DWARFDie Die = getDie(*CU);
if (Die) {
CUInfo CUI(DICtx, dyn_cast<DWARFCompileUnit>(CU.get()));
- pool.async([this, CUI, &LogMutex, &Out, Die]() mutable {
+ pool.async([this, CUI, &CU, &LogMutex, &Out, Die]() mutable {
std::string storage;
raw_string_ostream StrStream(storage);
OutputAggregator ThreadOut(Out.GetOS() ? &StrStream : nullptr);
@@ -623,6 +625,9 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
StrStream.flush();
Out << storage;
}
+ // Release the line table, once we're done.
+ DICtx.clearLineTableForUnit(CU.get());
+
Out.Merge(ThreadOut);
});
}
>From 5fa6827d21a8b157293827d86aa7106fb152d213 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Tue, 30 Apr 2024 14:54:53 -0700
Subject: [PATCH 02/11] Finished without crashing
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 2 ++
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 5 +++++
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp | 5 ++++-
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 80c27aea89312..52a8730a46c2c 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -566,6 +566,8 @@ class DWARFUnit {
Error tryExtractDIEsIfNeeded(bool CUDieOnly);
+ void freeDIEs();
+
private:
/// Size in bytes of the .debug_info data associated with this compile unit.
size_t getDebugInfoSize() const {
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index bdd04b00f557b..4786208c79fc2 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -495,6 +495,11 @@ void DWARFUnit::extractDIEsIfNeeded(bool CUDieOnly) {
Context.getRecoverableErrorHandler()(std::move(e));
}
+void DWARFUnit::freeDIEs() {
+ if (DieArray.capacity())
+ std::vector<DWARFDebugInfoEntry>().swap(DieArray);
+}
+
Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
if ((CUDieOnly && !DieArray.empty()) ||
DieArray.size() > 1)
diff --git a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
index 9376515e3712f..7204cefae1b84 100644
--- a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
+++ b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
@@ -625,8 +625,9 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
StrStream.flush();
Out << storage;
}
- // Release the line table, once we're done.
+ // Release the line table and DIEs once we're done.
DICtx.clearLineTableForUnit(CU.get());
+ CU->freeDIEs();
Out.Merge(ThreadOut);
});
@@ -634,6 +635,8 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
}
pool.wait();
}
+ for (const auto &CU : DICtx.compile_units())
+ CU->freeDIEs();
size_t FunctionsAddedCount = Gsym.getNumFunctionInfos() - NumBefore;
Out << "Loaded " << FunctionsAddedCount << " functions from DWARF.\n";
return Error::success();
>From 0f6694cd3f735819e5b099759b95d01bc7363d00 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Wed, 1 May 2024 14:58:26 -0700
Subject: [PATCH 03/11] Added locking
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 10 +++--
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 41 ++++++++++++++-----
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp | 13 +++---
3 files changed, 44 insertions(+), 20 deletions(-)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 52a8730a46c2c..9614aab8bb9b5 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -22,6 +22,7 @@
#include "llvm/DebugInfo/DWARF/DWARFLocationExpression.h"
#include "llvm/DebugInfo/DWARF/DWARFUnitIndex.h"
#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/RWMutex.h"
#include <cassert>
#include <cstddef>
#include <cstdint>
@@ -257,6 +258,9 @@ class DWARFUnit {
std::shared_ptr<DWARFUnit> DWO;
+ mutable llvm::sys::RWMutex m_cu_die_array_mutex;
+ mutable llvm::sys::RWMutex m_all_die_array_mutex;
+
protected:
friend dwarf_linker::parallel::CompileUnit;
@@ -566,7 +570,8 @@ class DWARFUnit {
Error tryExtractDIEsIfNeeded(bool CUDieOnly);
- void freeDIEs();
+ /// clearDIEs - Clear parsed DIEs to keep memory usage low.
+ void clearDIEs(bool KeepCUDie);
private:
/// Size in bytes of the .debug_info data associated with this compile unit.
@@ -583,9 +588,6 @@ class DWARFUnit {
void extractDIEsToVector(bool AppendCUDie, bool AppendNonCUDIEs,
std::vector<DWARFDebugInfoEntry> &DIEs) const;
- /// clearDIEs - Clear parsed DIEs to keep memory usage low.
- void clearDIEs(bool KeepCUDie);
-
/// parseDWO - Parses .dwo file for current compile unit. Returns true if
/// it was actually constructed.
/// The \p AlternativeLocation specifies an alternative location to get
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index 4786208c79fc2..b3fdb4e017803 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -495,26 +495,43 @@ void DWARFUnit::extractDIEsIfNeeded(bool CUDieOnly) {
Context.getRecoverableErrorHandler()(std::move(e));
}
-void DWARFUnit::freeDIEs() {
- if (DieArray.capacity())
- std::vector<DWARFDebugInfoEntry>().swap(DieArray);
-}
-
Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
- if ((CUDieOnly && !DieArray.empty()) ||
- DieArray.size() > 1)
- return Error::success(); // Already parsed.
-
- bool HasCUDie = !DieArray.empty();
- extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
+ // read-lock:
+ {
+ llvm::sys::ScopedReader Lock(m_cu_die_array_mutex);
+ if ((CUDieOnly && !DieArray.empty()) ||
+ DieArray.size() > 1)
+ return Error::success(); // Already parsed.
+ }
+ bool HasCUDie = false;
+ // write-lock:
+ {
+ llvm::sys::ScopedWriter Lock(m_cu_die_array_mutex);
+ if ((CUDieOnly && !DieArray.empty()) ||
+ DieArray.size() > 1)
+ return Error::success(); // Already parsed.
+ HasCUDie = !DieArray.empty();
+ extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
+ }
+ // read-lock:
+{
+ llvm::sys::ScopedReader Lock(m_all_die_array_mutex);
if (DieArray.empty())
return Error::success();
// If CU DIE was just parsed, copy several attribute values from it.
if (HasCUDie)
return Error::success();
+}
+ // write-lock:
+llvm::sys::ScopedWriter Lock(m_all_die_array_mutex);
+ if (DieArray.empty())
+ return Error::success();
+ // If CU DIE was just parsed, copy several attribute values from it.
+ if (HasCUDie)
+ return Error::success();
DWARFDie UnitDie(this, &DieArray[0]);
if (std::optional<uint64_t> DWOId =
toUnsigned(UnitDie.find(DW_AT_GNU_dwo_id)))
@@ -663,6 +680,8 @@ void DWARFUnit::clearDIEs(bool KeepCUDie) {
// It depends on the implementation whether the request is fulfilled.
// Create a new vector with a small capacity and assign it to the DieArray to
// have previous contents freed.
+ llvm::sys::ScopedWriter CULock(m_cu_die_array_mutex);
+ llvm::sys::ScopedWriter AllLock(m_all_die_array_mutex);
DieArray = (KeepCUDie && !DieArray.empty())
? std::vector<DWARFDebugInfoEntry>({DieArray[0]})
: std::vector<DWARFDebugInfoEntry>();
diff --git a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
index 7204cefae1b84..bd4c24f3ee28e 100644
--- a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
+++ b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
@@ -619,24 +619,27 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
raw_string_ostream StrStream(storage);
OutputAggregator ThreadOut(Out.GetOS() ? &StrStream : nullptr);
handleDie(ThreadOut, CUI, Die);
+ // Release the line table once we're done.
+ DICtx.clearLineTableForUnit(CU.get());
+ // Free any DIEs that were allocated by the DWARF parser.
+ // If/when they're needed by other CU's, they'll be recreated.
+ CU->clearDIEs(false);
// Print ThreadLogStorage lines into an actual stream under a lock
std::lock_guard<std::mutex> guard(LogMutex);
if (Out.GetOS()) {
StrStream.flush();
Out << storage;
}
- // Release the line table and DIEs once we're done.
- DICtx.clearLineTableForUnit(CU.get());
- CU->freeDIEs();
-
Out.Merge(ThreadOut);
+
});
}
}
pool.wait();
}
+ // Now get rid of all the DIEs that may have been recreated
for (const auto &CU : DICtx.compile_units())
- CU->freeDIEs();
+ CU->clearDIEs(false);
size_t FunctionsAddedCount = Gsym.getNumFunctionInfos() - NumBefore;
Out << "Loaded " << FunctionsAddedCount << " functions from DWARF.\n";
return Error::success();
>From ed0135851f7ad00a3e8d6da8d5ecd9a19fb1b5cb Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Fri, 3 May 2024 09:26:24 -0700
Subject: [PATCH 04/11] Code formatting
---
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 28 ++++++++------------
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp | 4 ++-
2 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index b3fdb4e017803..cc79d9ec7130c 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -496,42 +496,36 @@ void DWARFUnit::extractDIEsIfNeeded(bool CUDieOnly) {
}
Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
- // read-lock:
{
llvm::sys::ScopedReader Lock(m_cu_die_array_mutex);
- if ((CUDieOnly && !DieArray.empty()) ||
- DieArray.size() > 1)
+ if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
return Error::success(); // Already parsed.
}
bool HasCUDie = false;
- // write-lock:
{
llvm::sys::ScopedWriter Lock(m_cu_die_array_mutex);
- if ((CUDieOnly && !DieArray.empty()) ||
- DieArray.size() > 1)
+ if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
return Error::success(); // Already parsed.
HasCUDie = !DieArray.empty();
extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
}
+ {
+ llvm::sys::ScopedReader Lock(m_all_die_array_mutex);
+ if (DieArray.empty())
+ return Error::success();
- // read-lock:
-{
- llvm::sys::ScopedReader Lock(m_all_die_array_mutex);
+ // If CU DIE was just parsed, copy several attribute values from it.
+ if (HasCUDie)
+ return Error::success();
+ }
+ llvm::sys::ScopedWriter Lock(m_all_die_array_mutex);
if (DieArray.empty())
return Error::success();
// If CU DIE was just parsed, copy several attribute values from it.
if (HasCUDie)
return Error::success();
-}
- // write-lock:
-llvm::sys::ScopedWriter Lock(m_all_die_array_mutex);
- if (DieArray.empty())
- return Error::success();
- // If CU DIE was just parsed, copy several attribute values from it.
- if (HasCUDie)
- return Error::success();
DWARFDie UnitDie(this, &DieArray[0]);
if (std::optional<uint64_t> DWOId =
toUnsigned(UnitDie.find(DW_AT_GNU_dwo_id)))
diff --git a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
index bd4c24f3ee28e..4a1ed91a7244f 100644
--- a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
+++ b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
@@ -589,6 +589,9 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
handleDie(Out, CUI, Die);
// Release the line table, once we're done.
DICtx.clearLineTableForUnit(CU.get());
+ // Free any DIEs that were allocated by the DWARF parser.
+ // If/when they're needed by other CU's, they'll be recreated.
+ CU->clearDIEs(false);
}
} else {
// LLVM Dwarf parser is not thread-safe and we need to parse all DWARF up
@@ -631,7 +634,6 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
Out << storage;
}
Out.Merge(ThreadOut);
-
});
}
}
>From 987cda64b2640badb1d939037732416f04788efe Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Mon, 13 May 2024 11:53:08 -0700
Subject: [PATCH 05/11] Updated from code review feedback
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 4 ++--
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 15 +++++++--------
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp | 6 +++---
3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 9614aab8bb9b5..4e0601d1e042e 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -258,8 +258,8 @@ class DWARFUnit {
std::shared_ptr<DWARFUnit> DWO;
- mutable llvm::sys::RWMutex m_cu_die_array_mutex;
- mutable llvm::sys::RWMutex m_all_die_array_mutex;
+ mutable llvm::sys::RWMutex CUDieArrayMutex;
+ mutable llvm::sys::RWMutex AllDieArrayMutex;
protected:
friend dwarf_linker::parallel::CompileUnit;
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index cc79d9ec7130c..bd9c49bcd6b90 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -497,35 +497,34 @@ void DWARFUnit::extractDIEsIfNeeded(bool CUDieOnly) {
Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
{
- llvm::sys::ScopedReader Lock(m_cu_die_array_mutex);
+ llvm::sys::ScopedReader Lock(CUDieArrayMutex);
if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
return Error::success(); // Already parsed.
}
bool HasCUDie = false;
{
- llvm::sys::ScopedWriter Lock(m_cu_die_array_mutex);
+ llvm::sys::ScopedWriter Lock(CUDieArrayMutex);
if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
return Error::success(); // Already parsed.
HasCUDie = !DieArray.empty();
extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
}
{
- llvm::sys::ScopedReader Lock(m_all_die_array_mutex);
+ llvm::sys::ScopedReader Lock(AllDieArrayMutex);
if (DieArray.empty())
return Error::success();
- // If CU DIE was just parsed, copy several attribute values from it.
if (HasCUDie)
return Error::success();
}
- llvm::sys::ScopedWriter Lock(m_all_die_array_mutex);
+ llvm::sys::ScopedWriter Lock(AllDieArrayMutex);
if (DieArray.empty())
return Error::success();
- // If CU DIE was just parsed, copy several attribute values from it.
if (HasCUDie)
return Error::success();
+ // If CU DIE was just parsed, copy several attribute values from it.
DWARFDie UnitDie(this, &DieArray[0]);
if (std::optional<uint64_t> DWOId =
toUnsigned(UnitDie.find(DW_AT_GNU_dwo_id)))
@@ -674,8 +673,8 @@ void DWARFUnit::clearDIEs(bool KeepCUDie) {
// It depends on the implementation whether the request is fulfilled.
// Create a new vector with a small capacity and assign it to the DieArray to
// have previous contents freed.
- llvm::sys::ScopedWriter CULock(m_cu_die_array_mutex);
- llvm::sys::ScopedWriter AllLock(m_all_die_array_mutex);
+ llvm::sys::ScopedWriter CULock(CUDieArrayMutex);
+ llvm::sys::ScopedWriter AllLock(AllDieArrayMutex);
DieArray = (KeepCUDie && !DieArray.empty())
? std::vector<DWARFDebugInfoEntry>({DieArray[0]})
: std::vector<DWARFDebugInfoEntry>();
diff --git a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
index 4a1ed91a7244f..e1b30648b6a77 100644
--- a/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
+++ b/llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
@@ -591,7 +591,7 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
DICtx.clearLineTableForUnit(CU.get());
// Free any DIEs that were allocated by the DWARF parser.
// If/when they're needed by other CU's, they'll be recreated.
- CU->clearDIEs(false);
+ CU->clearDIEs(/*KeepCUDie=*/false);
}
} else {
// LLVM Dwarf parser is not thread-safe and we need to parse all DWARF up
@@ -626,7 +626,7 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
DICtx.clearLineTableForUnit(CU.get());
// Free any DIEs that were allocated by the DWARF parser.
// If/when they're needed by other CU's, they'll be recreated.
- CU->clearDIEs(false);
+ CU->clearDIEs(/*KeepCUDie=*/false);
// Print ThreadLogStorage lines into an actual stream under a lock
std::lock_guard<std::mutex> guard(LogMutex);
if (Out.GetOS()) {
@@ -641,7 +641,7 @@ Error DwarfTransformer::convert(uint32_t NumThreads, OutputAggregator &Out) {
}
// Now get rid of all the DIEs that may have been recreated
for (const auto &CU : DICtx.compile_units())
- CU->clearDIEs(false);
+ CU->clearDIEs(/*KeepCUDie=*/false);
size_t FunctionsAddedCount = Gsym.getNumFunctionInfos() - NumBefore;
Out << "Loaded " << FunctionsAddedCount << " functions from DWARF.\n";
return Error::success();
>From feaf17d7ea2a0c21a27fd4cb8dbc2f480bbe35d9 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Tue, 21 May 2024 08:40:17 -0700
Subject: [PATCH 06/11] Added the top level lock to prevent clearDies from
causing problems
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 1 +
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 2 ++
2 files changed, 3 insertions(+)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 4e0601d1e042e..5315d10891ed1 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -258,6 +258,7 @@ class DWARFUnit {
std::shared_ptr<DWARFUnit> DWO;
+ mutable llvm::sys::RWMutex CUDieFreeMutex;
mutable llvm::sys::RWMutex CUDieArrayMutex;
mutable llvm::sys::RWMutex AllDieArrayMutex;
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index bd9c49bcd6b90..1c4866cbbb41d 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -496,6 +496,7 @@ void DWARFUnit::extractDIEsIfNeeded(bool CUDieOnly) {
}
Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
+ llvm::sys::ScopedReader FreeLock(CUDieFreeMutex);
{
llvm::sys::ScopedReader Lock(CUDieArrayMutex);
if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
@@ -673,6 +674,7 @@ void DWARFUnit::clearDIEs(bool KeepCUDie) {
// It depends on the implementation whether the request is fulfilled.
// Create a new vector with a small capacity and assign it to the DieArray to
// have previous contents freed.
+ llvm::sys::ScopedWriter FreeLock(CUDieFreeMutex);
llvm::sys::ScopedWriter CULock(CUDieArrayMutex);
llvm::sys::ScopedWriter AllLock(AllDieArrayMutex);
DieArray = (KeepCUDie && !DieArray.empty())
>From 1c6a0fdf8c642691c8168f0625cbfd80e4d53560 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Tue, 21 May 2024 08:40:17 -0700
Subject: [PATCH 07/11] Using a helper to make the double checked locking more
legible
Summary:
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: https://phabricator.intern.facebook.com/D57863981
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 4 +
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 78 +++++++++++++------
2 files changed, 58 insertions(+), 24 deletions(-)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 5315d10891ed1..c83bcf96f295c 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -585,6 +585,10 @@ class DWARFUnit {
/// hasn't already been done
void extractDIEsIfNeeded(bool CUDieOnly);
+ /// extractAllDIEsHelper - helper to be invoked *only* from inside
+ /// tryExtractDIEsIfNeeded, which holds correct locks.
+ Error extractAllDIEsHelper();
+
/// extractDIEsToVector - Appends all parsed DIEs to a vector.
void extractDIEsToVector(bool AppendCUDie, bool AppendNonCUDIEs,
std::vector<DWARFDebugInfoEntry> &DIEs) const;
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index 1c4866cbbb41d..7fc1e3233df04 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -495,36 +495,66 @@ void DWARFUnit::extractDIEsIfNeeded(bool CUDieOnly) {
Context.getRecoverableErrorHandler()(std::move(e));
}
-Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
- llvm::sys::ScopedReader FreeLock(CUDieFreeMutex);
+static bool DoubleCheckedRWLocker(llvm::sys::RWMutex &Mutex,
+ const std::function<bool()> &reader,
+ const std::function<void()> &writer) {
{
- llvm::sys::ScopedReader Lock(CUDieArrayMutex);
- if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
- return Error::success(); // Already parsed.
+ llvm::sys::ScopedReader Lock(Mutex);
+ if (reader())
+ return true;
}
+ llvm::sys::ScopedWriter Lock(Mutex);
+ if (reader())
+ return true;
+ // If we get here, then the reader function returned false. This means that
+ // no one else is currently writing to this data structure and it's safe for
+ // us to write to it now. We can use a scoped writer lock since there are no
+ // other readers or writers at this point.
+ writer();
+ return false;
+}
+
+Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
+ llvm::sys::ScopedReader FreeLock(CUDieFreeMutex);
bool HasCUDie = false;
- {
- llvm::sys::ScopedWriter Lock(CUDieArrayMutex);
- if ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1)
- return Error::success(); // Already parsed.
- HasCUDie = !DieArray.empty();
- extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
- }
- {
- llvm::sys::ScopedReader Lock(AllDieArrayMutex);
- if (DieArray.empty())
- return Error::success();
+ Error Result = Error::success();
- if (HasCUDie)
- return Error::success();
- }
- llvm::sys::ScopedWriter Lock(AllDieArrayMutex);
- if (DieArray.empty())
- return Error::success();
+ // Lambda to check if the CU DIE has been extracted already.
+ auto CheckIfCUDieExtracted = [this, CUDieOnly]() {
+ // True means already parsed.
+ return ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1);
+ };
- if (HasCUDie)
- return Error::success();
+ // Lambda to extract the CU DIE.
+ auto ExtractCUDie = [this, CUDieOnly, &HasCUDie]() {
+ HasCUDie = !DieArray.empty();
+ extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
+ };
+
+ // Safely check if the CU DIE has been extract already. If not, then extract
+ // it.
+ if (DoubleCheckedRWLocker(CUDieArrayMutex, CheckIfCUDieExtracted,
+ ExtractCUDie))
+ return Result;
+
+ // Lambda to check if all DIEs have been extract already.
+ auto CheckIfAllDiesExtracted = [this, HasCUDie]() {
+ return (DieArray.empty() || HasCUDie);
+ };
+
+ // Lambda to extract all the DIEs using the helper function
+ auto ExtractAllDies = [this, &Result]() { Result = extractAllDIEsHelper(); };
+
+ // Safely check if *all* the DIEs have been parsed already. If not, then parse
+ // them.
+ DoubleCheckedRWLocker(AllDieArrayMutex, CheckIfAllDiesExtracted,
+ ExtractAllDies);
+ return Result;
+}
+// This should only be used from the tryExtractDIEsIfNeeded function:
+// it must already have all the proper locks acquired.
+Error DWARFUnit::extractAllDIEsHelper() {
// If CU DIE was just parsed, copy several attribute values from it.
DWARFDie UnitDie(this, &DieArray[0]);
if (std::optional<uint64_t> DWOId =
>From bdcb8d22ca60312f7e126e40eefd8b0c0123b813 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 6 Jun 2024 10:55:00 -0700
Subject: [PATCH 08/11] Fixed the error handling issues; fully tested
---
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index 7fc1e3233df04..e71b794d92c27 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -543,7 +543,13 @@ Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
};
// Lambda to extract all the DIEs using the helper function
- auto ExtractAllDies = [this, &Result]() { Result = extractAllDIEsHelper(); };
+ auto ExtractAllDies = [this, &Result]() {
+ if (Error E = extractAllDIEsHelper()) {
+ // Consume the success placeholder and save the actual error
+ consumeError(std::move(Result));
+ Result = std::move(E);
+ }
+ };
// Safely check if *all* the DIEs have been parsed already. If not, then parse
// them.
>From 51d944b46460a92252efafecf41922628f0fe005 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 6 Jun 2024 16:51:24 -0700
Subject: [PATCH 09/11] Refactored to make the overall code read more cleanly
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 10 ++-
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 73 ++++++++++---------
2 files changed, 46 insertions(+), 37 deletions(-)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index c83bcf96f295c..27bd2ea0f6827 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -585,8 +585,16 @@ class DWARFUnit {
/// hasn't already been done
void extractDIEsIfNeeded(bool CUDieOnly);
+ /// extracCUDieIfNeeded - Parse CU DIE if it hasn't already been done.
+ /// Only to be used from extractDIEsIfNeeded, which holds the correct locks.
+ bool extractCUDieIfNeeded(bool CUDieOnly, bool &HasCUDie);
+
+ /// extractAllDIEsIfNeeded - Parses non-CU DIE's for a given CU if needed.
+ /// Only to be used from extractDIEsIfNeeded, which holds the correct locks.
+ Error extractAllDIEsIfNeeded(bool HasCUDie);
+
/// extractAllDIEsHelper - helper to be invoked *only* from inside
- /// tryExtractDIEsIfNeeded, which holds correct locks.
+ /// tryExtractDIEsIfNeeded, which holds the correct locks.
Error extractAllDIEsHelper();
/// extractDIEsToVector - Appends all parsed DIEs to a vector.
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index e71b794d92c27..a7ead8d072fe0 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -514,50 +514,51 @@ static bool DoubleCheckedRWLocker(llvm::sys::RWMutex &Mutex,
return false;
}
-Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
- llvm::sys::ScopedReader FreeLock(CUDieFreeMutex);
- bool HasCUDie = false;
- Error Result = Error::success();
-
- // Lambda to check if the CU DIE has been extracted already.
- auto CheckIfCUDieExtracted = [this, CUDieOnly]() {
- // True means already parsed.
- return ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1);
- };
-
- // Lambda to extract the CU DIE.
- auto ExtractCUDie = [this, CUDieOnly, &HasCUDie]() {
- HasCUDie = !DieArray.empty();
- extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
- };
-
+bool DWARFUnit::extractCUDieIfNeeded(bool CUDieOnly, bool &HasCUDie) {
// Safely check if the CU DIE has been extract already. If not, then extract
// it.
- if (DoubleCheckedRWLocker(CUDieArrayMutex, CheckIfCUDieExtracted,
- ExtractCUDie))
- return Result;
-
- // Lambda to check if all DIEs have been extract already.
- auto CheckIfAllDiesExtracted = [this, HasCUDie]() {
- return (DieArray.empty() || HasCUDie);
- };
-
- // Lambda to extract all the DIEs using the helper function
- auto ExtractAllDies = [this, &Result]() {
- if (Error E = extractAllDIEsHelper()) {
- // Consume the success placeholder and save the actual error
- consumeError(std::move(Result));
- Result = std::move(E);
- }
- };
+ return DoubleCheckedRWLocker(
+ CUDieArrayMutex,
+ // Calculate if the CU DIE has been extracted already.
+ [&]() {
+ return ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1);
+ },
+ // Lambda to extract the CU DIE.
+ [&]() {
+ HasCUDie = !DieArray.empty();
+ extractDIEsToVector(!HasCUDie, !CUDieOnly, DieArray);
+ });
+}
+Error DWARFUnit::extractAllDIEsIfNeeded(bool HasCUDie) {
// Safely check if *all* the DIEs have been parsed already. If not, then parse
// them.
- DoubleCheckedRWLocker(AllDieArrayMutex, CheckIfAllDiesExtracted,
- ExtractAllDies);
+ Error Result = Error::success();
+ DoubleCheckedRWLocker(
+ AllDieArrayMutex,
+ // Lambda to check if all DIEs have been extracted already.
+ [=]() { return (DieArray.empty() || HasCUDie); },
+ // Lambda to extract all the DIEs using the helper function
+ [&]() {
+ if (Error E = extractAllDIEsHelper()) {
+ // Consume the success placeholder and save the actual error
+ consumeError(std::move(Result));
+ Result = std::move(E);
+ }
+ });
return Result;
}
+Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
+ // Take the CUDieFreeMutex lock to prevent the CU DIE from being freed
+ // after it was just parsed, but before the rest of the DIES are parsed.
+ llvm::sys::ScopedReader FreeLock(CUDieFreeMutex);
+ bool HasCUDie = false;
+ if (extractCUDieIfNeeded(CUDieOnly, HasCUDie))
+ return Error::success();
+ return extractAllDIEsIfNeeded(HasCUDie);
+}
+
// This should only be used from the tryExtractDIEsIfNeeded function:
// it must already have all the proper locks acquired.
Error DWARFUnit::extractAllDIEsHelper() {
>From cf823efca5c6d86d6b9064896a9cbc7e1dd144b0 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Fri, 7 Jun 2024 14:12:49 -0700
Subject: [PATCH 10/11] Removed the acquisition of extra, unecessary locks
---
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 2 --
1 file changed, 2 deletions(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index a7ead8d072fe0..be2bbf19de67a 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -712,8 +712,6 @@ void DWARFUnit::clearDIEs(bool KeepCUDie) {
// Create a new vector with a small capacity and assign it to the DieArray to
// have previous contents freed.
llvm::sys::ScopedWriter FreeLock(CUDieFreeMutex);
- llvm::sys::ScopedWriter CULock(CUDieArrayMutex);
- llvm::sys::ScopedWriter AllLock(AllDieArrayMutex);
DieArray = (KeepCUDie && !DieArray.empty())
? std::vector<DWARFDebugInfoEntry>({DieArray[0]})
: std::vector<DWARFDebugInfoEntry>();
>From b8c0704adc00c156b71ef03d8fcb83214effa5cd Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Mon, 17 Jun 2024 13:15:20 -0700
Subject: [PATCH 11/11] Renamed locks and helper methods, improved comments
---
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h | 14 +++----
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | 41 +++++++++++--------
2 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
index 27bd2ea0f6827..26ef7db718dd5 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -258,9 +258,9 @@ class DWARFUnit {
std::shared_ptr<DWARFUnit> DWO;
- mutable llvm::sys::RWMutex CUDieFreeMutex;
- mutable llvm::sys::RWMutex CUDieArrayMutex;
- mutable llvm::sys::RWMutex AllDieArrayMutex;
+ mutable llvm::sys::RWMutex FreeDIEsMutex;
+ mutable llvm::sys::RWMutex ExtractCUDieMutex;
+ mutable llvm::sys::RWMutex ExtractNonCUDIEsMutex;
protected:
friend dwarf_linker::parallel::CompileUnit;
@@ -589,13 +589,13 @@ class DWARFUnit {
/// Only to be used from extractDIEsIfNeeded, which holds the correct locks.
bool extractCUDieIfNeeded(bool CUDieOnly, bool &HasCUDie);
- /// extractAllDIEsIfNeeded - Parses non-CU DIE's for a given CU if needed.
+ /// extractNonCUDIEsIfNeeded - Parses non-CU DIE's for a given CU if needed.
/// Only to be used from extractDIEsIfNeeded, which holds the correct locks.
- Error extractAllDIEsIfNeeded(bool HasCUDie);
+ Error extractNonCUDIEsIfNeeded(bool HasCUDie);
- /// extractAllDIEsHelper - helper to be invoked *only* from inside
+ /// extractNonCUDIEsHelper - helper to be invoked *only* from inside
/// tryExtractDIEsIfNeeded, which holds the correct locks.
- Error extractAllDIEsHelper();
+ Error extractNonCUDIEsHelper();
/// extractDIEsToVector - Appends all parsed DIEs to a vector.
void extractDIEsToVector(bool AppendCUDie, bool AppendNonCUDIEs,
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
index be2bbf19de67a..2760cef7edfdb 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -508,17 +508,18 @@ static bool DoubleCheckedRWLocker(llvm::sys::RWMutex &Mutex,
return true;
// If we get here, then the reader function returned false. This means that
// no one else is currently writing to this data structure and it's safe for
- // us to write to it now. We can use a scoped writer lock since there are no
+ // us to write to it now. The scoped writer lock guarantees there are no
// other readers or writers at this point.
writer();
return false;
}
+// Helper to safely check if the Compile-Unit DIE has been extracted already.
+// If not, then extract it, and return false, indicating that it was *not*
+// already extracted.
bool DWARFUnit::extractCUDieIfNeeded(bool CUDieOnly, bool &HasCUDie) {
- // Safely check if the CU DIE has been extract already. If not, then extract
- // it.
return DoubleCheckedRWLocker(
- CUDieArrayMutex,
+ ExtractCUDieMutex,
// Calculate if the CU DIE has been extracted already.
[&]() {
return ((CUDieOnly && !DieArray.empty()) || DieArray.size() > 1);
@@ -530,17 +531,17 @@ bool DWARFUnit::extractCUDieIfNeeded(bool CUDieOnly, bool &HasCUDie) {
});
}
-Error DWARFUnit::extractAllDIEsIfNeeded(bool HasCUDie) {
- // Safely check if *all* the DIEs have been parsed already. If not, then parse
- // them.
+// Helper to safely check if the non-Compile-Unit DIEs have been parsed
+// already. If they haven't been parsed, go ahead and parse them.
+Error DWARFUnit::extractNonCUDIEsIfNeeded(bool HasCUDie) {
Error Result = Error::success();
DoubleCheckedRWLocker(
- AllDieArrayMutex,
+ ExtractNonCUDIEsMutex,
// Lambda to check if all DIEs have been extracted already.
[=]() { return (DieArray.empty() || HasCUDie); },
// Lambda to extract all the DIEs using the helper function
[&]() {
- if (Error E = extractAllDIEsHelper()) {
+ if (Error E = extractNonCUDIEsHelper()) {
// Consume the success placeholder and save the actual error
consumeError(std::move(Result));
Result = std::move(E);
@@ -550,18 +551,21 @@ Error DWARFUnit::extractAllDIEsIfNeeded(bool HasCUDie) {
}
Error DWARFUnit::tryExtractDIEsIfNeeded(bool CUDieOnly) {
- // Take the CUDieFreeMutex lock to prevent the CU DIE from being freed
- // after it was just parsed, but before the rest of the DIES are parsed.
- llvm::sys::ScopedReader FreeLock(CUDieFreeMutex);
+ // Acquire the FreeDIEsMutex lock (in read-mode) to prevent the Compile Unit
+ // DIE from being freed by a thread calling clearDIEs() after the CU DIE was
+ // parsed, but before the rest of the DIEs are parsed, as there are no other
+ // locks held during that brief period.
+ llvm::sys::ScopedReader FreeLock(FreeDIEsMutex);
bool HasCUDie = false;
if (extractCUDieIfNeeded(CUDieOnly, HasCUDie))
return Error::success();
- return extractAllDIEsIfNeeded(HasCUDie);
+ // Right here is where the above-mentioned race condition exists.
+ return extractNonCUDIEsIfNeeded(HasCUDie);
}
-// This should only be used from the tryExtractDIEsIfNeeded function:
-// it must already have all the proper locks acquired.
-Error DWARFUnit::extractAllDIEsHelper() {
+// Helper used from the tryExtractDIEsIfNeeded function: it must already have
+// acquired the ExtractNonCUDIEsMutex for writing.
+Error DWARFUnit::extractNonCUDIEsHelper() {
// If CU DIE was just parsed, copy several attribute values from it.
DWARFDie UnitDie(this, &DieArray[0]);
if (std::optional<uint64_t> DWOId =
@@ -706,12 +710,15 @@ bool DWARFUnit::parseDWO(StringRef DWOAlternativeLocation) {
}
void DWARFUnit::clearDIEs(bool KeepCUDie) {
+ // We need to acquire the FreeDIEsMutex lock in write-mode, because we are
+ // going to free the DIEs, when other threads might be trying to create them.
+ llvm::sys::ScopedWriter FreeLock(FreeDIEsMutex);
+
// Do not use resize() + shrink_to_fit() to free memory occupied by dies.
// shrink_to_fit() is a *non-binding* request to reduce capacity() to size().
// It depends on the implementation whether the request is fulfilled.
// Create a new vector with a small capacity and assign it to the DieArray to
// have previous contents freed.
- llvm::sys::ScopedWriter FreeLock(CUDieFreeMutex);
DieArray = (KeepCUDie && !DieArray.empty())
? std::vector<DWARFDebugInfoEntry>({DieArray[0]})
: std::vector<DWARFDebugInfoEntry>();
More information about the llvm-commits
mailing list