[ASTWriter] Do not write ObjCCategories if empty. #141841
Merged
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
davemgreen
requested review from
Bigcheese,
ostannard,
jansvoboda11 and
tmatheson-arm
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: David Green (davemgreen) ChangesThis is a fix for a completely unrelated patch, that started to cause failures in the explicit-build.cpp test because the size of the b.pcm and b-not-a.pcm files became the same. The alignment added by empty ObjCCategory blobs being written to the file causes them to become the same size, and the error 'module file has a different size than expected' will not be emitted as the pcms only track module size, not content, for whether they are valid. This prevents that issue by not saving the ObjCCategories if it is empty. The change in clang/lib/Serialization/ASTReaderDecl.cpp is just a format, but shows that the only use of ObjCCategoriesMap loaded from the file will be OK with null (never loaded) data. It is a bit of a weird fix, but should help decrease the size of the modules for objects that are not used. Full diff: https://github.com/llvm/llvm-project/pull/141841.diff 2 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index e84932c765663..237b8b9d6ef6a 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -4634,11 +4634,10 @@ namespace {
// Perform a binary search to find the local redeclarations for this
// declaration (if any).
- const ObjCCategoriesInfo Compare = { LocalID, 0 };
- const ObjCCategoriesInfo *Result
- = std::lower_bound(M.ObjCCategoriesMap,
- M.ObjCCategoriesMap + M.LocalNumObjCCategoriesInMap,
- Compare);
+ const ObjCCategoriesInfo Compare = {LocalID, 0};
+ const ObjCCategoriesInfo *Result = std::lower_bound(
+ M.ObjCCategoriesMap,
+ M.ObjCCategoriesMap + M.LocalNumObjCCategoriesInMap, Compare);
if (Result == M.ObjCCategoriesMap + M.LocalNumObjCCategoriesInMap ||
LocalID != Result->getDefinitionID()) {
// We didn't find anything. If the class definition is in this module
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index cc9916a75d4b4..a92e9c37f6a7c 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4978,6 +4978,9 @@ void ASTWriter::WriteCUDAPragmas(Sema &SemaRef) {
}
void ASTWriter::WriteObjCCategories() {
+ if (ObjCClassesWithCategories.empty())
+ return;
+
SmallVector<ObjCCategoriesInfo, 2> CategoriesMap;
RecordData Categories;
|
|
This change makes sense to me. I have two questions:
|
Thanks - For 1 I can do. I'll see about adding a test. For 2 I'm not sure. I was thinking of adding a triple to make sure it more reliable and less likely to go wrong on different architectures, but excluding the objc info was enough to get it working again for me. As for adding a import signature I'm not an expert, it certainly sounds sensible to have something more precise than the timestamp if what the test is testing is an important thing to get right. |
davemgreen
force-pushed
the
gh-clang-donotwriteobjc
branch
from
8e7522b to
a811e8b
Compare
This is a fix for a completely unrelated patch, that started to fail the explicit-build.cpp test because the size of the b.pcm and b-not-a.pcm files became the same. The alignment added by empty ObjCCategory blobs being written to the file causes them to be the same size, and the error 'module file has a different size than expected' will not be emitted. This prevents that issue by not saving the ObjCCategories if it is empty. The change in clang/lib/Serialization/ASTReaderDecl.cpp is just a format, but shows that the only use of ObjCCategoriesMap loaded from the file will be OK with null (never loaded) data.
davemgreen
force-pushed
the
gh-clang-donotwriteobjc
branch
from
a811e8b to
696c8b3
Compare
jansvoboda11
approved these changes
Jun 2, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/17220 Here is the relevant piece of the build log for the reference |
sallto
pushed a commit
to sallto/llvm-project
that referenced
this pull request
Jun 3, 2025
This is a fix for a completely unrelated patch, that started to cause failures in the explicit-build.cpp test because the size of the b.pcm and b-not-a.pcm files became the same. The alignment added by empty ObjCCategory blobs being written to the file causes them to become the same size, and the error 'module file has a different size than expected' will not be emitted as the pcms only track module size, not content, for whether they are valid. This prevents that issue by not saving the ObjCCategories if it is empty. The change in clang/lib/Serialization/ASTReaderDecl.cpp is just formatting, but shows that the only use of ObjCCategoriesMap loaded from the file will be OK with null (never loaded) data. It is a bit of a weird fix, but should help decrease the size of the modules for objects that are not used.
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.
This is a fix for a completely unrelated patch, that started to cause failures in the explicit-build.cpp test because the size of the b.pcm and b-not-a.pcm files became the same. The alignment added by empty ObjCCategory blobs being written to the file causes them to become the same size, and the error 'module file has a different size than expected' will not be emitted as the pcms only track module size, not content, for whether they are valid.
This prevents that issue by not saving the ObjCCategories if it is empty. The change in clang/lib/Serialization/ASTReaderDecl.cpp is just a format, but shows that the only use of ObjCCategoriesMap loaded from the file will be OK with null (never loaded) data. It is a bit of a weird fix, but should help decrease the size of the modules for objects that are not used.