[llvm] [nfc]Make InstrProfSymtab non-copyable and non-movable (PR #86882)
Mingming Liu via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 27 15:34:53 PDT 2024
https://github.com/minglotus-6 updated https://github.com/llvm/llvm-project/pull/86882
>From bc67452fa23392a761dc9c2721f9ea1dba993432 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 27 Mar 2024 15:08:46 -0700
Subject: [PATCH 1/2] [nfc]Make InstrProfSymtab non-copyable and non-movable -
The direct use case (in pr 66825) is to add llvm::IntervalMap and allocator
to InstrProfSymtab as class members. The allocator class doesn't have a
move-assignment operator; and it's going to take much effort to implement
move-assignment operator for the allocator class such that its enclosing
class is movable. - There is only one use of compiler-generated
move-assignment operator in the repo, which is in CoverageMappingReader.cpp.
Luckily it's possible to use std::unique_ptr<InstrProfSymtab> instead in its
surrounding code, so did the change.
---
.../Coverage/CoverageMappingReader.h | 4 +--
llvm/include/llvm/ProfileData/InstrProf.h | 9 +++++++
.../Coverage/CoverageMappingReader.cpp | 27 ++++++++++---------
3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
index 346ca4ad2eb314..5bb5eb582bbfef 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
@@ -184,7 +184,7 @@ class BinaryCoverageReader : public CoverageMappingReader {
private:
std::vector<std::string> Filenames;
std::vector<ProfileMappingRecord> MappingRecords;
- InstrProfSymtab ProfileNames;
+ std::unique_ptr<InstrProfSymtab> ProfileNames;
size_t CurrentRecord = 0;
std::vector<StringRef> FunctionsFilenames;
std::vector<CounterExpression> Expressions;
@@ -211,7 +211,7 @@ class BinaryCoverageReader : public CoverageMappingReader {
static Expected<std::unique_ptr<BinaryCoverageReader>>
createCoverageReaderFromBuffer(StringRef Coverage,
FuncRecordsStorage &&FuncRecords,
- InstrProfSymtab &&ProfileNames,
+ std::unique_ptr<InstrProfSymtab> ProfileNames,
uint8_t BytesInAddress,
llvm::endianness Endian,
StringRef CompilationDir = "");
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 25ec06a7392027..3877a5a546e422 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -471,6 +471,15 @@ class InstrProfSymtab {
public:
InstrProfSymtab() = default;
+ // Not copyable or movable.
+ // Consider std::unique_ptr for move.
+ // InstrProfSymtab has a few containers as class members, so consider
+ // std::shared_ptr for read-only copy.
+ InstrProfSymtab(const InstrProfSymtab &) = delete;
+ InstrProfSymtab &operator=(const InstrProfSymtab &) = delete;
+ InstrProfSymtab(InstrProfSymtab &&) = delete;
+ InstrProfSymtab &operator=(InstrProfSymtab &&) = delete;
+
/// Create InstrProfSymtab from an object file section which
/// contains function PGO names. When section may contain raw
/// string data or string data in compressed form. This method
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index d328460510830a..1e3c70e3b6a2e4 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -894,31 +894,32 @@ static Error readCoverageMappingData(
Expected<std::unique_ptr<BinaryCoverageReader>>
BinaryCoverageReader::createCoverageReaderFromBuffer(
StringRef Coverage, FuncRecordsStorage &&FuncRecords,
- InstrProfSymtab &&ProfileNames, uint8_t BytesInAddress,
+ std::unique_ptr<InstrProfSymtab> ProfileNames, uint8_t BytesInAddress,
llvm::endianness Endian, StringRef CompilationDir) {
std::unique_ptr<BinaryCoverageReader> Reader(
new BinaryCoverageReader(std::move(FuncRecords)));
Reader->ProfileNames = std::move(ProfileNames);
+ InstrProfSymtab &ProfileNamesRef = *Reader->ProfileNames;
StringRef FuncRecordsRef = Reader->FuncRecords->getBuffer();
if (BytesInAddress == 4 && Endian == llvm::endianness::little) {
if (Error E = readCoverageMappingData<uint32_t, llvm::endianness::little>(
- Reader->ProfileNames, Coverage, FuncRecordsRef,
- Reader->MappingRecords, CompilationDir, Reader->Filenames))
+ ProfileNamesRef, Coverage, FuncRecordsRef, Reader->MappingRecords,
+ CompilationDir, Reader->Filenames))
return std::move(E);
} else if (BytesInAddress == 4 && Endian == llvm::endianness::big) {
if (Error E = readCoverageMappingData<uint32_t, llvm::endianness::big>(
- Reader->ProfileNames, Coverage, FuncRecordsRef,
- Reader->MappingRecords, CompilationDir, Reader->Filenames))
+ ProfileNamesRef, Coverage, FuncRecordsRef, Reader->MappingRecords,
+ CompilationDir, Reader->Filenames))
return std::move(E);
} else if (BytesInAddress == 8 && Endian == llvm::endianness::little) {
if (Error E = readCoverageMappingData<uint64_t, llvm::endianness::little>(
- Reader->ProfileNames, Coverage, FuncRecordsRef,
- Reader->MappingRecords, CompilationDir, Reader->Filenames))
+ ProfileNamesRef, Coverage, FuncRecordsRef, Reader->MappingRecords,
+ CompilationDir, Reader->Filenames))
return std::move(E);
} else if (BytesInAddress == 8 && Endian == llvm::endianness::big) {
if (Error E = readCoverageMappingData<uint64_t, llvm::endianness::big>(
- Reader->ProfileNames, Coverage, FuncRecordsRef,
- Reader->MappingRecords, CompilationDir, Reader->Filenames))
+ ProfileNamesRef, Coverage, FuncRecordsRef, Reader->MappingRecords,
+ CompilationDir, Reader->Filenames))
return std::move(E);
} else
return make_error<CoverageMapError>(
@@ -963,8 +964,8 @@ loadTestingFormat(StringRef Data, StringRef CompilationDir) {
if (Data.size() < ProfileNamesSize)
return make_error<CoverageMapError>(coveragemap_error::malformed,
"the size of ProfileNames is too big");
- InstrProfSymtab ProfileNames;
- if (Error E = ProfileNames.create(Data.substr(0, ProfileNamesSize), Address))
+ auto ProfileNames = std::make_unique<InstrProfSymtab>();
+ if (Error E = ProfileNames->create(Data.substr(0, ProfileNamesSize), Address))
return std::move(E);
Data = Data.substr(ProfileNamesSize);
@@ -1099,7 +1100,7 @@ loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
OF->isLittleEndian() ? llvm::endianness::little : llvm::endianness::big;
// Look for the sections that we are interested in.
- InstrProfSymtab ProfileNames;
+ auto ProfileNames = std::make_unique<InstrProfSymtab>();
std::vector<SectionRef> NamesSectionRefs;
// If IPSK_name is not found, fallback to search for IPK_covname, which is
// used when binary correlation is enabled.
@@ -1116,7 +1117,7 @@ loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
return make_error<CoverageMapError>(
coveragemap_error::malformed,
"the size of coverage mapping section is not one");
- if (Error E = ProfileNames.create(NamesSectionRefs.back()))
+ if (Error E = ProfileNames->create(NamesSectionRefs.back()))
return std::move(E);
auto CoverageSection = lookupSections(*OF, IPSK_covmap);
>From 74e0feb02e4bc6a0fc48544e44f09cdded5f36c6 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 27 Mar 2024 15:32:57 -0700
Subject: [PATCH 2/2] Changes: 1. In create method, return error upon nullptr.
2. Update private constructor of BinaryCoverageReader to take a
unique_ptr, rather than setting unique_ptr immediately after class
constructor runs.
---
.../llvm/ProfileData/Coverage/CoverageMappingReader.h | 5 +++--
llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp | 8 +++++---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
index 5bb5eb582bbfef..7698d03e305756 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
@@ -195,8 +195,9 @@ class BinaryCoverageReader : public CoverageMappingReader {
// D69471, which can split up function records into multiple sections on ELF.
FuncRecordsStorage FuncRecords;
- BinaryCoverageReader(FuncRecordsStorage &&FuncRecords)
- : FuncRecords(std::move(FuncRecords)) {}
+ BinaryCoverageReader(std::unique_ptr<InstrProfSymtab> Symtab,
+ FuncRecordsStorage &&FuncRecords)
+ : ProfileNames(std::move(Symtab)), FuncRecords(std::move(FuncRecords)) {}
public:
BinaryCoverageReader(const BinaryCoverageReader &) = delete;
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index 1e3c70e3b6a2e4..cb75ce979791cc 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -896,9 +896,11 @@ BinaryCoverageReader::createCoverageReaderFromBuffer(
StringRef Coverage, FuncRecordsStorage &&FuncRecords,
std::unique_ptr<InstrProfSymtab> ProfileNames, uint8_t BytesInAddress,
llvm::endianness Endian, StringRef CompilationDir) {
- std::unique_ptr<BinaryCoverageReader> Reader(
- new BinaryCoverageReader(std::move(FuncRecords)));
- Reader->ProfileNames = std::move(ProfileNames);
+ if (ProfileNames == nullptr)
+ return make_error<CoverageMapError>(coveragemap_error::malformed,
+ "Caller must provide ProfileNames");
+ std::unique_ptr<BinaryCoverageReader> Reader(new BinaryCoverageReader(
+ std::move(ProfileNames), std::move(FuncRecords)));
InstrProfSymtab &ProfileNamesRef = *Reader->ProfileNames;
StringRef FuncRecordsRef = Reader->FuncRecords->getBuffer();
if (BytesInAddress == 4 && Endian == llvm::endianness::little) {
More information about the llvm-commits
mailing list