-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SHT_LLVM_BB_ADDR_MAP] Add assertion and clarify docstring #77374
[SHT_LLVM_BB_ADDR_MAP] Add assertion and clarify docstring #77374
Conversation
This patch adds an assertion to readBBAddrMapImpl to confirm that PGOAnalyses and BBAddrMaps are of the same size when PGO information is requested (part of the API contract). This patch also updates the docstring for readBBAddrMap to better clarify what is guaranteed.
@llvm/pr-subscribers-llvm-binary-utilities Author: Aiden Grossman (boomanaiden154) ChangesThis patch adds an assertion to readBBAddrMapImpl to confirm that PGOAnalyses and BBAddrMaps are of the same size when PGO information is requested (part of the API contract). This patch also updates the docstring for readBBAddrMap to better clarify what is guaranteed. Full diff: https://github.com/llvm/llvm-project/pull/77374.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Object/ELFObjectFile.h b/llvm/include/llvm/Object/ELFObjectFile.h
index 86f42654f8af8d..5182c6cf62c2c5 100644
--- a/llvm/include/llvm/Object/ELFObjectFile.h
+++ b/llvm/include/llvm/Object/ELFObjectFile.h
@@ -110,11 +110,12 @@ class ELFObjectFileBase : public ObjectFile {
Expected<std::vector<VersionEntry>> readDynsymVersions() const;
/// Returns a vector of all BB address maps in the object file. When
- // `TextSectionIndex` is specified, only returns the BB address maps
- // corresponding to the section with that index. When `PGOAnalyses`is
- // specified, the vector is cleared then filled with extra PGO data.
- // `PGOAnalyses` will always be the same length as the return value on
- // success, otherwise it is empty.
+ /// `TextSectionIndex` is specified, only returns the BB address maps
+ /// corresponding to the section with that index. When `PGOAnalyses`is
+ /// specified, the vector is cleared then filled with extra PGO data.
+ /// When PGO information is requested (`PGOAnalyses` is not nullptr),
+ /// `PGOAnalyses` will always be the same length as the return value,
+ /// assuming no error occurs. Upon failure, `PGOAnalyses` will be empty.
Expected<std::vector<BBAddrMap>>
readBBAddrMap(std::optional<unsigned> TextSectionIndex = std::nullopt,
std::vector<PGOAnalysisMap> *PGOAnalyses = nullptr) const;
diff --git a/llvm/lib/Object/ELFObjectFile.cpp b/llvm/lib/Object/ELFObjectFile.cpp
index 95c4f9f8545db2..f43fdf8be58541 100644
--- a/llvm/lib/Object/ELFObjectFile.cpp
+++ b/llvm/lib/Object/ELFObjectFile.cpp
@@ -830,6 +830,10 @@ Expected<std::vector<BBAddrMap>> static readBBAddrMapImpl(
std::move(BBAddrMapOrErr->begin(), BBAddrMapOrErr->end(),
std::back_inserter(BBAddrMaps));
}
+ if (PGOAnalyses)
+ assert(PGOAnalyses->size() == BBAddrMaps.size() &&
+ "The same number of BBAddrMaps and PGOAnalysisMaps should be "
+ "returned when PGO information is requested");
return BBAddrMaps;
}
|
Bump on this patch when reviewers have a chance. Thanks! |
LGTM with a minor nit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one nit about the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This patch adds an assertion to readBBAddrMapImpl to confirm that PGOAnalyses and BBAddrMaps are of the same size when PGO information is requested (part of the API contract). This patch also updates the docstring for readBBAddrMap to better clarify what is guaranteed.