obj2yaml: Introduce CovMap dump #127432
Open
obj2yaml: Introduce CovMap dump #127432
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chapuni
requested review from
rupprecht,
keith and
aaronmondal
as code owners
llvmbot
added
PGO
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-objectyaml Author: NAKAMURA Takumi (chapuni) Changes
For now, this is dedicated to ELF and disabled by default. Affected sections are as below:
There are options for enabling CovMap in
There are a few namespaces. Other implementations are sunk into anonymous namespace.
Test input files in Patch is 129.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127432.diff 20 Files Affected:
diff --git a/llvm/include/llvm/ObjectYAML/CovMap.h b/llvm/include/llvm/ObjectYAML/CovMap.h
new file mode 100644
index 0000000000000..fa9492fc1ee72
--- /dev/null
+++ b/llvm/include/llvm/ObjectYAML/CovMap.h
@@ -0,0 +1,385 @@
+//===- CovMap.h - ObjectYAML Interface for coverage map ---------*- 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
+//
+//===----------------------------------------------------------------------===//
+//
+// - llvm::coverage::yaml
+//
+// Describes binary file formats and YAML structures of coverage map.
+//
+// - llvm::yaml
+//
+// Attachments for YAMLTraits.
+//
+// - llvm::covmap
+//
+// Provides YAML encoder and decoder for coverage map.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_OBJECTYAML_COVMAP_H
+#define LLVM_OBJECTYAML_COVMAP_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ObjectYAML/ELFYAML.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/YAMLTraits.h"
+#include <cstdint>
+#include <memory>
+#include <optional>
+#include <variant>
+
+namespace llvm {
+class InstrProfSymtab;
+class raw_ostream;
+} // namespace llvm
+
+namespace llvm::coverage::yaml {
+
+/// This works like vector container but can be replaced with
+/// MutableArrayRef. See also SequenceTraits<VectorOrRef>.
+template <typename T, typename Vec = std::vector<T>> class VectorOrRef {
+ using Ref = MutableArrayRef<T>;
+
+ /// Holds vector type initially.
+ std::variant<Vec, Ref> Array;
+
+public:
+ // FIXME: Iterator impl is minimal easy.
+ using iterator = T *;
+
+ iterator begin() {
+ if (auto *V = std::get_if<Vec>(&Array))
+ return &V->front();
+ else
+ return &std::get<Ref>(Array).front();
+ }
+
+ iterator end() {
+ if (auto *V = std::get_if<Vec>(&Array))
+ return &V->back() + 1;
+ else
+ return &std::get<Ref>(Array).back() + 1;
+ }
+
+ size_t size() const {
+ if (const auto *V = std::get_if<Vec>(&Array))
+ return V->size();
+ else
+ return std::get<Ref>(Array).size();
+ }
+
+ T &operator[](int Idx) {
+ if (auto *V = std::get_if<Vec>(&Array))
+ return (*V)[Idx];
+ else
+ return std::get<Ref>(Array)[Idx];
+ }
+
+ void resize(size_t Size) { std::get<Vec>(Array).resize(Size); }
+
+ VectorOrRef() = default;
+
+ /// Initialize with MutableArrayRef.
+ VectorOrRef(Ref &&Tmp) : Array(std::move(Tmp)) {}
+};
+
+/// Options for Decoder.
+struct DecoderParam {
+ bool Detailed; ///< Generate and show processed records.
+ bool Raw; ///< Show raw data oriented records.
+ bool dLoc; ///< Show raw dLoc (differential Loc).
+};
+
+struct DecoderContext;
+
+/// Base Counter, corresponding to coverage::Counter.
+struct CounterTy {
+ enum TagTy : uint8_t {
+ Zero = 0,
+ Ref,
+ Sub,
+ Add,
+ };
+
+ /// Optional in detailed view, since most Tag can be determined from
+ /// other optional fields.
+ std::optional<TagTy> Tag;
+
+ /// Internal use.
+ std::optional<uint64_t> Val;
+
+ std::optional<uint64_t> RefOpt;
+ std::optional<uint64_t> SubOpt;
+ std::optional<uint64_t> AddOpt;
+
+ virtual ~CounterTy() {}
+
+ virtual void mapping(llvm::yaml::IO &IO);
+
+ /// Holds Val for extensions.
+ Error decodeOrTag(DecoderContext &Data);
+
+ /// Raise Error if Val isn't empty.
+ Error decode(DecoderContext &Data);
+
+ void encode(raw_ostream &OS) const;
+};
+
+/// Holds a pair of both hands but doesn't hold ops(add or sub).
+/// Ops is stored in CounterTy::Tag.
+using ExpressionTy = std::array<CounterTy, 2>;
+
+/// {True, False}
+using BranchTy = std::array<CounterTy, 2>;
+
+/// {ID, TrueID, FalseID}
+/// Note: This has +1 offset unlike mcdc::ConditionID.
+using MCDCBranchTy = std::array<uint16_t, 3>;
+
+struct DecisionTy {
+ uint64_t BIdx; ///< Bitmap index
+ uint64_t NC; ///< NumConds
+
+ void mapping(llvm::yaml::IO &IO);
+
+ Error decode(DecoderContext &Data);
+
+ void encode(raw_ostream &OS) const;
+};
+
+/// {LineStart, ColumnStart, LineEnd, ColumnEnd}
+using LocTy = std::array<uint64_t, 4>;
+
+///
+struct RecTy : CounterTy {
+ enum ExtTagTy : uint8_t {
+ Skip = 2,
+ Branch = 4,
+ Decision = 5,
+ MCDCBranch = 6,
+ };
+
+ /// This is optional in detailed view.
+ std::optional<ExtTagTy> ExtTag;
+
+ // Options for extensions.
+ std::optional<uint64_t> Expansion; ///< Doesn't have ExtTag.
+ std::optional<BranchTy> BranchOpt; ///< Optionally has MCDC.
+ std::optional<MCDCBranchTy> MCDC;
+ std::optional<DecisionTy> DecisionOpt;
+
+ /// True or None.
+ /// Stored in ColumnEnd:31.
+ std::optional<bool> isGap;
+
+ std::optional<LocTy> Loc; ///< Absolute line numbers.
+ std::optional<LocTy> dLoc; ///< Differential line numbers.
+
+ void mapping(llvm::yaml::IO &IO) override;
+
+ Error decode(DecoderContext &Data);
+
+ void encode(uint64_t &StartLoc, raw_ostream &OS) const;
+};
+
+/// {NumRecs, Recs...}
+struct FileRecsTy {
+ std::optional<unsigned> Index; ///< Shown in detailed view.
+ std::optional<std::string> Filename; ///< Resolved by FileIDs.
+ std::vector<RecTy> Recs;
+
+ void mapping(llvm::yaml::IO &IO);
+};
+
+/// Key is FilenamesRef.
+using CovMapByRefTy = llvm::DenseMap<uint64_t, struct CovMapTy *>;
+
+/// An element of CovFun array.
+struct CovFunTy {
+ std::optional<llvm::yaml::Hex64> NameRef; ///< Hash value of the symbol.
+ std::optional<std::string> FuncName; ///< Resolved by symtab.
+ llvm::yaml::Hex64 FuncHash; ///< Signature of this function.
+ llvm::yaml::Hex64 FilenamesRef; ///< Pointer to CovMap
+ std::optional<std::vector<unsigned>> FileIDs; ///< Resolved by CovMap
+ std::vector<ExpressionTy> Expressions;
+ std::vector<FileRecsTy> Files; ///< 2-dimension array of Recs.
+
+ void mapping(llvm::yaml::IO &IO);
+
+ /// Depends on CovMap and SymTab(IPSK_names)
+ Expected<uint64_t> decode(CovMapByRefTy &CovMapByRef, InstrProfSymtab *SymTab,
+ const ArrayRef<uint8_t> Content, uint64_t Offset,
+ const DecoderParam &Param, bool IsLE = true);
+
+ void encode(raw_ostream &OS) const;
+};
+
+/// An element of CovMap array.
+struct CovMapTy {
+ /// This is the key of CovMap but not present in the file
+ /// format. Calculate and store with Filenames.
+ llvm::yaml::Hex64 FilenamesRef;
+
+ std::optional<uint32_t> Version;
+
+ /// Raw Filenames (and storage of Files)
+ std::optional<std::vector<std::string>> Filenames;
+
+ /// Since Version5: Filenames[0] is the working directory (or
+ /// zero-length string). Note that indices in CovFun::FileIDs is
+ /// base on Filenames. (Then, +0, as WD, is not expected to appear)
+ std::optional<std::string> WD;
+ /// This may be ArrayRef in Decoder since Filenames has been
+ /// filled. On the other hand in Encoder, this should be a vector
+ /// since YAML parser doesn't endorse references.
+ std::optional<VectorOrRef<std::string>> Files;
+
+ void mapping(llvm::yaml::IO &IO);
+
+ bool useWD() const { return (!Version || *Version >= 4); }
+ StringRef getWD() const { return (WD ? *WD : StringRef()); }
+
+ Expected<uint64_t> decode(const ArrayRef<uint8_t> Content, uint64_t Offset,
+ const DecoderParam &Param, bool IsLE = true);
+
+ /// Generate Accumulated list with WD.
+ /// Returns a single element {WD} if AccFiles is not given.
+ std::vector<std::string>
+ generateAccFilenames(const std::optional<ArrayRef<StringRef>> &AccFilesOpt =
+ std::nullopt) const;
+ /// Regenerate Filenames with WD.
+ /// Use Files if it is not None. Or given AccFiles is used.
+ void
+ regenerateFilenames(const std::optional<ArrayRef<StringRef>> &AccFilesOpt);
+
+ /// Encode Filenames. This is mostly used just to obtain FilenamesRef.
+ std::pair<uint64_t, std::string> encodeFilenames(
+ const std::optional<ArrayRef<StringRef>> &AccFilesOpt = std::nullopt,
+ bool Compress = false) const;
+
+ void encode(raw_ostream &OS) const;
+};
+
+} // namespace llvm::coverage::yaml
+
+namespace llvm::yaml {
+template <typename T>
+struct SequenceTraits<llvm::coverage::yaml::VectorOrRef<T>> {
+ static size_t size(IO &io, llvm::coverage::yaml::VectorOrRef<T> &seq) {
+ return seq.size();
+ }
+ static T &element(IO &, llvm::coverage::yaml::VectorOrRef<T> &seq,
+ size_t index) {
+ if (index >= seq.size())
+ seq.resize(index + 1);
+ return seq[index];
+ }
+};
+} // namespace llvm::yaml
+
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::coverage::yaml::CovMapTy)
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::coverage::yaml::CovFunTy)
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::coverage::yaml::ExpressionTy)
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::coverage::yaml::RecTy)
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::coverage::yaml::FileRecsTy)
+LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(llvm::coverage::yaml::CounterTy)
+
+#define LLVM_COVERAGE_YAML_ELEM_MAPPING(Ty) \
+ namespace llvm::yaml { \
+ template <> struct MappingTraits<llvm::coverage::yaml::Ty> { \
+ static void mapping(IO &IO, llvm::coverage::yaml::Ty &Obj) { \
+ Obj.mapping(IO); \
+ } \
+ }; \
+ }
+
+/// `Flow` is used for emission of a compact oneliner for RecTy.
+#define LLVM_COVERAGE_YAML_ELEM_MAPPING_FLOW(Ty) \
+ namespace llvm::yaml { \
+ template <> struct MappingTraits<llvm::coverage::yaml::Ty> { \
+ static void mapping(IO &IO, llvm::coverage::yaml::Ty &Obj) { \
+ Obj.mapping(IO); \
+ (void)flow; \
+ } \
+ static const bool flow = true; \
+ }; \
+ }
+
+#define LLVM_COVERAGE_YAML_ENUM(Ty) \
+ namespace llvm::yaml { \
+ template <> struct ScalarEnumerationTraits<llvm::coverage::yaml::Ty> { \
+ static void enumeration(IO &IO, llvm::coverage::yaml::Ty &Value); \
+ }; \
+ }
+
+LLVM_COVERAGE_YAML_ENUM(CounterTy::TagTy)
+LLVM_COVERAGE_YAML_ENUM(RecTy::ExtTagTy)
+LLVM_COVERAGE_YAML_ELEM_MAPPING_FLOW(CounterTy)
+LLVM_COVERAGE_YAML_ELEM_MAPPING_FLOW(DecisionTy)
+LLVM_COVERAGE_YAML_ELEM_MAPPING_FLOW(RecTy)
+LLVM_COVERAGE_YAML_ELEM_MAPPING(FileRecsTy)
+LLVM_COVERAGE_YAML_ELEM_MAPPING(CovFunTy)
+LLVM_COVERAGE_YAML_ELEM_MAPPING(CovMapTy)
+
+namespace llvm::covmap {
+
+class Decoder {
+public:
+ virtual ~Decoder() {}
+
+ /// Returns DecoderImpl.
+ static std::unique_ptr<Decoder>
+ get(const coverage::yaml::DecoderParam &Param);
+
+ /// Called from the Sections loop in advance of the final dump.
+ /// Decoder predecodes Names and CovMap, and captures Contents of
+ /// CovFuns.
+ virtual Error
+ acquire(uint64_t Offset, unsigned AddressAlign, StringRef Name,
+ std::function<Expected<ArrayRef<uint8_t>>()> getSectionContents) = 0;
+
+ /// Called before the final dump after `acquire`.
+ /// Decode contents partially and resolve names.
+ virtual Error fixup() = 0;
+
+ /// Create an ELFYAML object. This just puts predecoded data in
+ /// `fixup`.
+ virtual Expected<ELFYAML::Section *>
+ make(uint64_t Offset, StringRef Name,
+ std::function<Error(ELFYAML::Section &S)> dumpCommonSection) = 0;
+
+ /// Suppress emission of CovMap unless enabled.
+ static bool enabled;
+};
+
+class Encoder {
+public:
+ virtual ~Encoder() {}
+
+ /// Returns EncoderImpl.
+ static std::unique_ptr<Encoder> get();
+
+ /// Called from the Sections loop.
+ virtual void collect(ELFYAML::Chunk *Chunk) = 0;
+
+ /// Resolves names along DecoderParam in advance of Emitter. It'd be
+ /// too late to resolve sections in Emitter since they are immutable
+ /// then.
+ virtual void fixup() = 0;
+};
+
+/// Returns whether Name is interested.
+bool nameMatches(StringRef Name);
+
+/// Returns a new ELFYAML Object.
+std::unique_ptr<ELFYAML::Section> make_unique(StringRef Name);
+
+} // namespace llvm::covmap
+
+#endif // LLVM_OBJECTYAML_COVMAP_H
diff --git a/llvm/include/llvm/ObjectYAML/ELFYAML.h b/llvm/include/llvm/ObjectYAML/ELFYAML.h
index dfdfa055d65fa..2dc1a861077c3 100644
--- a/llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -229,6 +229,7 @@ struct Chunk {
DependentLibraries,
CallGraphProfile,
BBAddrMap,
+ CovMapBase,
// Special chunks.
SpecialChunksStart,
@@ -398,6 +399,19 @@ struct RawContentSection : Section {
std::optional<std::vector<uint8_t>> ContentBuf;
};
+struct CovMapSectionBase : Section {
+ std::optional<llvm::yaml::Hex64> Info;
+
+ CovMapSectionBase() : Section(ChunkKind::CovMapBase) {}
+
+ virtual void mapping(yaml::IO &IO) = 0;
+ virtual Error encode(raw_ostream &OS) const = 0;
+
+ static bool classof(const Chunk *S) {
+ return S->Kind == ChunkKind::CovMapBase;
+ }
+};
+
struct NoBitsSection : Section {
NoBitsSection() : Section(ChunkKind::NoBits) {}
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 7133c0c6a302c..e20424da3cac2 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -545,6 +545,12 @@ class InstrProfSymtab {
/// This method is a wrapper to \c readAndDecodeStrings method.
Error create(StringRef NameStrings);
+ // PrfNames is nested array.
+ using PrfNamesTy = SmallVector<std::string>;
+ using PrfNamesChunksTy = SmallVector<PrfNamesTy, 1>;
+
+ Expected<PrfNamesChunksTy> createAndGetList(ArrayRef<uint8_t> Content);
+
/// Initialize symtab states with function names and vtable names. \c
/// FuncNameStrings is a string composed of one or more encoded function name
/// strings, and \c VTableNameStrings composes of one or more encoded vtable
diff --git a/llvm/lib/ObjectYAML/CMakeLists.txt b/llvm/lib/ObjectYAML/CMakeLists.txt
index b36974d47d9f8..11054a1e91388 100644
--- a/llvm/lib/ObjectYAML/CMakeLists.txt
+++ b/llvm/lib/ObjectYAML/CMakeLists.txt
@@ -7,6 +7,7 @@ add_llvm_component_library(LLVMObjectYAML
CodeViewYAMLTypes.cpp
COFFEmitter.cpp
COFFYAML.cpp
+ CovMap.cpp
DWARFEmitter.cpp
DWARFYAML.cpp
DXContainerEmitter.cpp
@@ -34,7 +35,9 @@ add_llvm_component_library(LLVMObjectYAML
LINK_COMPONENTS
BinaryFormat
+ Coverage
Object
+ ProfileData
Support
TargetParser
DebugInfoCodeView
diff --git a/llvm/lib/ObjectYAML/CovMap.cpp b/llvm/lib/ObjectYAML/CovMap.cpp
new file mode 100644
index 0000000000000..12c03a651668c
--- /dev/null
+++ b/llvm/lib/ObjectYAML/CovMap.cpp
@@ -0,0 +1,977 @@
+//===- CovMap.cpp - ObjectYAML Interface for coverage map -----------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// Implementations of CovMap, encoder, decoder.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ObjectYAML/CovMap.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ObjectYAML/ELFYAML.h"
+#include "llvm/ProfileData/Coverage/CoverageMapping.h"
+#include "llvm/ProfileData/Coverage/CoverageMappingReader.h"
+#include "llvm/ProfileData/Coverage/CoverageMappingWriter.h"
+#include "llvm/ProfileData/InstrProf.h"
+#include "llvm/Support/Alignment.h"
+#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/LEB128.h"
+#include "llvm/Support/MD5.h"
+#include "llvm/Support/YAMLTraits.h"
+#include <cstdint>
+
+#define COVMAP_V3
+
+using namespace llvm;
+using namespace llvm::coverage::yaml;
+using namespace llvm::covmap;
+
+bool Decoder::enabled;
+
+// DataExtractor w/ single Cursor
+struct coverage::yaml::DecoderContext : DataExtractor,
+ DataExtractor::Cursor,
+ DecoderParam {
+ uint64_t LineStart = 0;
+
+ DecoderContext(const ArrayRef<uint8_t> Content, const DecoderParam &Param,
+ bool IsLE)
+ : DataExtractor(Content, IsLE, /*AddressSize=*/0),
+ DataExtractor::Cursor(0), DecoderParam(Param) {}
+
+ bool eof() { return DataExtractor::eof(*this); }
+ uint32_t getU32() { return DataExtractor::getU32(*this); }
+ uint64_t getU64() { return DataExtractor::getU64(*this); }
+ Expected<uint64_t> getULEB128() {
+ uint64_t Result = DataExtractor::getULEB128(*this);
+ if (!*this)
+ return takeError();
+ return Result;
+ }
+ StringRef getBytes(size_t sz) { return DataExtractor::getBytes(*this, sz); }
+};
+
+void CounterTy::encode(raw_ostream &OS) const {
+ std::pair<unsigned, uint64_t> C;
+ if (RefOpt)
+ C = {Ref, *RefOpt};
+ else if (SubOpt)
+ C = {Sub, *SubOpt};
+ else if (AddOpt)
+ C = {Add, *AddOpt};
+ else if (Tag && *Tag == Zero)
+ C = {Zero, 0u};
+ else if (Tag && Val)
+ C = {*Tag, *Val};
+ else
+ llvm_unreachable("Null value cannot be met");
+
+ encodeULEB128(C.first | (C.second << 2), OS);
+}
+
+Error CounterTy::decodeOrTag(DecoderContext &Data) {
+ auto COrErr = Data.getULEB128();
+ if (!COrErr)
+ return COrErr.takeError();
+ auto T = static_cast<TagTy>(*COrErr & 0x03);
+ auto V = (*COrErr >> 2);
+ if (T == Zero) {
+ if (V == 0)
+ Tag = Zero; // w/o Val
+ else
+ Val = V; // w/o Tag
+ } else {
+ if (Data.Raw) {
+ Tag = T;
+ Val = V;
+ } else {
+ switch (T) {
+ case Zero:
+ llvm_unreachable("Zero should be handled in advance");
+ case Ref:
+ RefOpt = V;
+ break;
+ case Sub:
+ SubOpt = V;
+ break;
+ case Add:
+ AddOpt = V;
+ break;
+ }
+ }
+ }
+
+ return Error::success();
+}
+
+Error CounterTy::decode(DecoderContext &Data) {
+ if (auto E = decodeOrTag(Data))
+ return E;
+ if (!this->Tag && this->Val)
+ return make_error<CoverageMapError>(
+ coveragemap_error::malformed,
+ "Counter::Zero shouldn't have the Val: 0x" +
+ Twine::utohexstr(*this->Val));
+ return Error::success();
+}
+
+void DecisionTy::encode(raw_ostream &OS) const {
+ encodeULEB128(BIdx, OS);
+ encodeULEB128(NC, OS);
+}
+
+Error DecisionTy::decode(DecoderContext &Data) {
+ auto BIdxOrErr = Data.getULEB128();
+ if (!BIdxOrErr)
+ return BIdxOrErr.takeError();
+ BIdx = *BIdxOrErr;
+
+ auto NCOrErr = Data.getULEB128();
+ if (!NCOrErr)
+ return NCOrErr.takeError();
+ NC = *NCOrErr;
+
+ return Error::success();
+}
+
+void RecTy::encode(uint64_t &StartLoc, raw_ostream &OS) const {
+ if (Expansion) {
+ encodeULEB128(4 + (*Expansion << 3), OS);
+ } else if (ExtTag && *ExtTag == Skip) {
+ encodeULEB128(2 << 3, OS);
+ } else if (DecisionOpt) {
+ assert(!ExtTag || *ExtTag == Decision);
+ encodeULEB128(5 << 3, OS);
+ DecisionOpt->encode(OS);
+ } else if (MCDC) {
+ assert(!ExtTag || *ExtTag == MCDCBranch);
+ assert(BranchOpt);
+ encodeULEB128(6 << 3, OS);
+ (*BranchOpt)[0].encode(OS);
+ (*BranchOpt)[1].encode(OS);
+ encodeULEB128((*MCDC)[0], OS);
+ encodeULEB128((*MCDC)[1], OS);
+ encodeULEB128((*MCDC)[2], OS);
+ } else if (BranchOpt) {
+ assert(!ExtTag || *ExtTag == Branch);
+ encodeULEB128(4 << 3, OS);
+ (*BranchOpt)[0].encode(OS);
+ (*BranchOpt)[1].encode(OS);
+ } else {
+ // Non-tag CounterTy
+ CounterTy::encode(OS);
+ }
+
+ assert((!isGap || *isGap) && "Don't set isGap=false");
+ uint32_t Gap = (isGap ? (1u << 31) : 0u);
+ if (Loc) {
+ encodeULEB128((*Loc)[0] - St...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ornata
reviewed
Feb 17, 2025
| iterator begin() { | ||
| if (auto *V = std::get_if<Vec>(&Array)) | ||
| return &V->front(); | ||
| else |
There was a problem hiding this comment.
There was a problem hiding this comment.
I prefer symmetry here but I'll follow.
| iterator end() { | ||
| if (auto *V = std::get_if<Vec>(&Array)) | ||
| return &V->back() + 1; | ||
| else |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
and so on in other places throughout the code
| iterator begin() { | ||
| if (auto *V = std::get_if<Vec>(&Array)) | ||
| return &V->front(); | ||
| else |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
prefer
if ()
return ...
return ...
| iterator end() { | ||
| if (auto *V = std::get_if<Vec>(&Array)) | ||
| return &V->back() + 1; | ||
| else |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
prefer
if ()
return ...
return ...
| size_t size() const { | ||
| if (const auto *V = std::get_if<Vec>(&Array)) | ||
| return V->size(); | ||
| else |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
prefer
if ()
return ...
return ...
| T &operator[](int Idx) { | ||
| if (auto *V = std::get_if<Vec>(&Array)) | ||
| return (*V)[Idx]; | ||
| else |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
prefer
if ()
return ...
return ...
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| cl::desc("Dump raw YAML in Coverage Map."), | ||
| cl::cat(ELFCat)); | ||
| static cl::opt<bool> CovMapDLoc("covmap-dloc", | ||
| cl::desc("Prefer dLoc thatn absolute Loc."), |
There was a problem hiding this comment.
s/thatn/over
Recommend rephrasing to
Prefer differential line numbers over absolute line numbers in dump.
There was a problem hiding this comment.
Thanks. I'd like to assume users are developers and they can understand Loc and dLoc. I suppose they are developers' options.
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| @@ -580,6 +596,31 @@ ELFDumper<ELFT>::dumpSections() { | |||
| return Error::success(); | |||
| }; | |||
|
|
|||
| coverage::yaml::DecoderParam Param; | |||
There was a problem hiding this comment.
coverage::yaml::DecoderParam is only relevant when covmap::Decoder::enabled is true, right?
Would it make sense to sink all of this into the if (covmap::Decoder::enabled) part?
There was a problem hiding this comment.
Exposure of coverage::yaml::DecoderParam is not good but it is a minimal exposure to suppress covmap w/o explicit DecoderImpl context. (See ELFYAML.cpp)
There was a problem hiding this comment.
Correct me if I'm wrong, but I think @ornata is suggesting just moving the code down a few lines into the if block starting at line 604 that makes use of it.
chapuni
commented
Feb 17, 2025
| iterator begin() { | ||
| if (auto *V = std::get_if<Vec>(&Array)) | ||
| return &V->front(); | ||
| else |
There was a problem hiding this comment.
I prefer symmetry here but I'll follow.
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| cl::desc("Dump raw YAML in Coverage Map."), | ||
| cl::cat(ELFCat)); | ||
| static cl::opt<bool> CovMapDLoc("covmap-dloc", | ||
| cl::desc("Prefer dLoc thatn absolute Loc."), |
There was a problem hiding this comment.
Thanks. I'd like to assume users are developers and they can understand Loc and dLoc. I suppose they are developers' options.
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| @@ -580,6 +596,31 @@ ELFDumper<ELFT>::dumpSections() { | |||
| return Error::success(); | |||
| }; | |||
|
|
|||
| coverage::yaml::DecoderParam Param; | |||
There was a problem hiding this comment.
Exposure of coverage::yaml::DecoderParam is not good but it is a minimal exposure to suppress covmap w/o explicit DecoderImpl context. (See ELFYAML.cpp)
|
Apologies for not looking at this so far this week - I've got a lot on between my regular workload and multiple other reviews in flight. I'd like to review this before it gets merged and will aim to do so Monday or Tuesday next week. |
jh7370
reviewed
Feb 24, 2025
There was a problem hiding this comment.
There's an awful lot of code in this PR now and I wouldn't be confident in anybody's ability to reliably review it all at once. Is there a way you could split it down into multiple pieces?
For example, could you have support for each relevant section in a separate PR, or are there parts of the section format that are not required to have a "minimal" section that can be generated. You could even have the section not technically correct, but have some of the fields or similar not emitted initially, even though they're required for the proper behaviour. This allows for reviewing to focus on individual bits at a time. Keeping this PR as-is would still be useful, in that it could act as a reference point so that reviewers can look at the "big picture" too.
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| @@ -21,6 +23,18 @@ | |||
|
|
|||
| using namespace llvm; | |||
|
|
|||
| cl::OptionCategory ELFCat("ELF Options"); | |||
|
|
|||
| static cl::opt<bool> | |||
There was a problem hiding this comment.
Why do you feel the need to guard covmap decoding behind an option?
No other special section type has this sort of option, so it feels unnecessary.
llvm/lib/ObjectYAML/CovMap.cpp
Outdated
| default: | ||
| return make_error<CoverageMapError>( | ||
| coveragemap_error::malformed, | ||
| "Record doesn't have an valid Tag: 0x" + Twine::utohexstr(Tag)); |
There was a problem hiding this comment.
Suggested change
| "Record doesn't have an valid Tag: 0x" + Twine::utohexstr(Tag)); | |
| "Record doesn't have a valid Tag: 0x" + Twine::utohexstr(Tag)); |
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| @@ -580,6 +596,31 @@ ELFDumper<ELFT>::dumpSections() { | |||
| return Error::success(); | |||
| }; | |||
|
|
|||
| coverage::yaml::DecoderParam Param; | |||
There was a problem hiding this comment.
Correct me if I'm wrong, but I think @ornata is suggesting just moving the code down a few lines into the if block starting at line 604 that makes use of it.
ornata
reviewed
Feb 28, 2025
ornata
reviewed
Feb 28, 2025
llvm/tools/obj2yaml/elf2yaml.cpp
Outdated
| static cl::opt<bool> CovMapRaw("covmap-raw", | ||
| cl::desc("Dump raw YAML in Coverage Map."), | ||
| cl::cat(ELFCat)); | ||
| static cl::opt<bool> CovMapDLoc("covmap-dloc", |
There was a problem hiding this comment.
can the code for covmap-dloc be in a separate patch?
There was a problem hiding this comment.
No, it doesn't make sense. It means me to split out small amount of lines.
ornata
reviewed
Feb 28, 2025
| static cl::opt<bool> | ||
| CovMapDetailed("covmap", cl::desc("Dump detailed YAML in Coverage Map."), | ||
| cl::cat(ELFCat)); | ||
| static cl::opt<bool> CovMapRaw("covmap-raw", |
There was a problem hiding this comment.
can the code for covmap-raw be in a separate patch?
it would be easiest to review if the patch introduces minimal functionality the first time through.
There was a problem hiding this comment.
No. "Raw" is more basic impl. I split out "detailed".
ornata
reviewed
Feb 28, 2025
| // | ||
| // - llvm::covmap | ||
| // | ||
| // Provides YAML encoder and decoder for coverage map. |
There was a problem hiding this comment.
would it be possible to do only the encoder or decoder first in this patch, and then in another patch, do the other one?
e.g.
- patch 1: add ability to encode
- patch 2: add ability to decode
- patch 3: add support for covmap-raw
- patch 4: add support for covmap-dloc
There was a problem hiding this comment.
I split out "encoder" and "decoder detailed(--covmap)".
chapuni
changed the title
|
I have my doubts about using options to control how obj2yaml dumps things, since we don't do that for other bits. What is your use-case for this? Please don't mark my comments as resolved when you believe you have addressed them. It makes things harder to review. For more details, please see https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178. |
Conflicts: llvm/include/llvm/ObjectYAML/CovMap.h llvm/lib/ObjectYAML/CovMap.cpp
Conflicts: llvm/include/llvm/ObjectYAML/CovMap.h llvm/lib/ObjectYAML/CovMap.cpp
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
obj2yamlis able to understand and dump Coverage Mapping related sections with--covmap-raw.For now, this is dedicated to ELF and disabled by default. Affected sections are as below:
__llvm_covmap__llvm_covfun__llvm_prf_namesDepends on #129472