Skip to content

Commit 2c7610c

Browse files
[nfc]Make InstrProfSymtab non-copyable and non-movable (#86882)
- The direct use case (in [1]) is to add `llvm::IntervalMap` [2] and the allocator required by IntervalMap ctor [3] to class `InstrProfSymtab` as owned 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 the 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, so did the change. [1] #66825 [2] https://github.com/llvm/llvm-project/blob/4c2f68840e984b0f111779c46845ac00e3a7547d/llvm/include/llvm/ADT/IntervalMap.h#L936 [3] https://github.com/llvm/llvm-project/blob/4c2f68840e984b0f111779c46845ac00e3a7547d/llvm/include/llvm/ADT/IntervalMap.h#L1041
1 parent 070d7af commit 2c7610c

File tree

3 files changed

+34
-25
lines changed

3 files changed

+34
-25
lines changed

llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ class BinaryCoverageReader : public CoverageMappingReader {
184184
private:
185185
std::vector<std::string> Filenames;
186186
std::vector<ProfileMappingRecord> MappingRecords;
187-
InstrProfSymtab ProfileNames;
187+
std::unique_ptr<InstrProfSymtab> ProfileNames;
188188
size_t CurrentRecord = 0;
189189
std::vector<StringRef> FunctionsFilenames;
190190
std::vector<CounterExpression> Expressions;
@@ -195,8 +195,9 @@ class BinaryCoverageReader : public CoverageMappingReader {
195195
// D69471, which can split up function records into multiple sections on ELF.
196196
FuncRecordsStorage FuncRecords;
197197

198-
BinaryCoverageReader(FuncRecordsStorage &&FuncRecords)
199-
: FuncRecords(std::move(FuncRecords)) {}
198+
BinaryCoverageReader(std::unique_ptr<InstrProfSymtab> Symtab,
199+
FuncRecordsStorage &&FuncRecords)
200+
: ProfileNames(std::move(Symtab)), FuncRecords(std::move(FuncRecords)) {}
200201

201202
public:
202203
BinaryCoverageReader(const BinaryCoverageReader &) = delete;
@@ -209,12 +210,10 @@ class BinaryCoverageReader : public CoverageMappingReader {
209210
SmallVectorImpl<object::BuildIDRef> *BinaryIDs = nullptr);
210211

211212
static Expected<std::unique_ptr<BinaryCoverageReader>>
212-
createCoverageReaderFromBuffer(StringRef Coverage,
213-
FuncRecordsStorage &&FuncRecords,
214-
InstrProfSymtab &&ProfileNames,
215-
uint8_t BytesInAddress,
216-
llvm::endianness Endian,
217-
StringRef CompilationDir = "");
213+
createCoverageReaderFromBuffer(
214+
StringRef Coverage, FuncRecordsStorage &&FuncRecords,
215+
std::unique_ptr<InstrProfSymtab> ProfileNamesPtr, uint8_t BytesInAddress,
216+
llvm::endianness Endian, StringRef CompilationDir = "");
218217

219218
Error readNextRecord(CoverageMappingRecord &Record) override;
220219
};

llvm/include/llvm/ProfileData/InstrProf.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,13 @@ class InstrProfSymtab {
471471
public:
472472
InstrProfSymtab() = default;
473473

474+
// Not copyable or movable.
475+
// Consider std::unique_ptr for move.
476+
InstrProfSymtab(const InstrProfSymtab &) = delete;
477+
InstrProfSymtab &operator=(const InstrProfSymtab &) = delete;
478+
InstrProfSymtab(InstrProfSymtab &&) = delete;
479+
InstrProfSymtab &operator=(InstrProfSymtab &&) = delete;
480+
474481
/// Create InstrProfSymtab from an object file section which
475482
/// contains function PGO names. When section may contain raw
476483
/// string data or string data in compressed form. This method

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -894,31 +894,34 @@ static Error readCoverageMappingData(
894894
Expected<std::unique_ptr<BinaryCoverageReader>>
895895
BinaryCoverageReader::createCoverageReaderFromBuffer(
896896
StringRef Coverage, FuncRecordsStorage &&FuncRecords,
897-
InstrProfSymtab &&ProfileNames, uint8_t BytesInAddress,
897+
std::unique_ptr<InstrProfSymtab> ProfileNamesPtr, uint8_t BytesInAddress,
898898
llvm::endianness Endian, StringRef CompilationDir) {
899-
std::unique_ptr<BinaryCoverageReader> Reader(
900-
new BinaryCoverageReader(std::move(FuncRecords)));
901-
Reader->ProfileNames = std::move(ProfileNames);
899+
if (ProfileNamesPtr == nullptr)
900+
return make_error<CoverageMapError>(coveragemap_error::malformed,
901+
"Caller must provide ProfileNames");
902+
std::unique_ptr<BinaryCoverageReader> Reader(new BinaryCoverageReader(
903+
std::move(ProfileNamesPtr), std::move(FuncRecords)));
904+
InstrProfSymtab &ProfileNames = *Reader->ProfileNames;
902905
StringRef FuncRecordsRef = Reader->FuncRecords->getBuffer();
903906
if (BytesInAddress == 4 && Endian == llvm::endianness::little) {
904907
if (Error E = readCoverageMappingData<uint32_t, llvm::endianness::little>(
905-
Reader->ProfileNames, Coverage, FuncRecordsRef,
906-
Reader->MappingRecords, CompilationDir, Reader->Filenames))
908+
ProfileNames, Coverage, FuncRecordsRef, Reader->MappingRecords,
909+
CompilationDir, Reader->Filenames))
907910
return std::move(E);
908911
} else if (BytesInAddress == 4 && Endian == llvm::endianness::big) {
909912
if (Error E = readCoverageMappingData<uint32_t, llvm::endianness::big>(
910-
Reader->ProfileNames, Coverage, FuncRecordsRef,
911-
Reader->MappingRecords, CompilationDir, Reader->Filenames))
913+
ProfileNames, Coverage, FuncRecordsRef, Reader->MappingRecords,
914+
CompilationDir, Reader->Filenames))
912915
return std::move(E);
913916
} else if (BytesInAddress == 8 && Endian == llvm::endianness::little) {
914917
if (Error E = readCoverageMappingData<uint64_t, llvm::endianness::little>(
915-
Reader->ProfileNames, Coverage, FuncRecordsRef,
916-
Reader->MappingRecords, CompilationDir, Reader->Filenames))
918+
ProfileNames, Coverage, FuncRecordsRef, Reader->MappingRecords,
919+
CompilationDir, Reader->Filenames))
917920
return std::move(E);
918921
} else if (BytesInAddress == 8 && Endian == llvm::endianness::big) {
919922
if (Error E = readCoverageMappingData<uint64_t, llvm::endianness::big>(
920-
Reader->ProfileNames, Coverage, FuncRecordsRef,
921-
Reader->MappingRecords, CompilationDir, Reader->Filenames))
923+
ProfileNames, Coverage, FuncRecordsRef, Reader->MappingRecords,
924+
CompilationDir, Reader->Filenames))
922925
return std::move(E);
923926
} else
924927
return make_error<CoverageMapError>(
@@ -963,8 +966,8 @@ loadTestingFormat(StringRef Data, StringRef CompilationDir) {
963966
if (Data.size() < ProfileNamesSize)
964967
return make_error<CoverageMapError>(coveragemap_error::malformed,
965968
"the size of ProfileNames is too big");
966-
InstrProfSymtab ProfileNames;
967-
if (Error E = ProfileNames.create(Data.substr(0, ProfileNamesSize), Address))
969+
auto ProfileNames = std::make_unique<InstrProfSymtab>();
970+
if (Error E = ProfileNames->create(Data.substr(0, ProfileNamesSize), Address))
968971
return std::move(E);
969972
Data = Data.substr(ProfileNamesSize);
970973

@@ -1099,7 +1102,7 @@ loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
10991102
OF->isLittleEndian() ? llvm::endianness::little : llvm::endianness::big;
11001103

11011104
// Look for the sections that we are interested in.
1102-
InstrProfSymtab ProfileNames;
1105+
auto ProfileNames = std::make_unique<InstrProfSymtab>();
11031106
std::vector<SectionRef> NamesSectionRefs;
11041107
// If IPSK_name is not found, fallback to search for IPK_covname, which is
11051108
// used when binary correlation is enabled.
@@ -1116,7 +1119,7 @@ loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
11161119
return make_error<CoverageMapError>(
11171120
coveragemap_error::malformed,
11181121
"the size of coverage mapping section is not one");
1119-
if (Error E = ProfileNames.create(NamesSectionRefs.back()))
1122+
if (Error E = ProfileNames->create(NamesSectionRefs.back()))
11201123
return std::move(E);
11211124

11221125
auto CoverageSection = lookupSections(*OF, IPSK_covmap);

0 commit comments

Comments
 (0)