[lld-macho] Category merger: handle addends when getting symbol at offset #91238
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
|
@llvm/pr-subscribers-lld-macho @llvm/pr-subscribers-lld Author: None (alx32) ChangesCurrently the However, it does not deal with the reloc Full diff: https://github.com/llvm/llvm-project/pull/91238.diff 1 Files Affected:
diff --git a/lld/MachO/ObjC.cpp b/lld/MachO/ObjC.cpp
index 4760fffebe3b30..919829b36f06fd 100644
--- a/lld/MachO/ObjC.cpp
+++ b/lld/MachO/ObjC.cpp
@@ -455,6 +455,7 @@ class ObjcCategoryMerger {
Defined *emitCategoryName(const std::string &name, ObjFile *objFile);
void createSymbolReference(Defined *refFrom, const Symbol *refTo,
uint32_t offset, const Reloc &relocTemplate);
+ Defined *tryFindDefinedOnIsec(const InputSection *isec, uint32_t offset);
Symbol *tryGetSymbolAtIsecOffset(const ConcatInputSection *isec,
uint32_t offset);
Defined *tryGetDefinedAtIsecOffset(const ConcatInputSection *isec,
@@ -518,12 +519,32 @@ void ObjcCategoryMerger::collectSectionWriteInfoFromIsec(
Symbol *
ObjcCategoryMerger::tryGetSymbolAtIsecOffset(const ConcatInputSection *isec,
uint32_t offset) {
+ if (!isec)
+ return nullptr;
const Reloc *reloc = isec->getRelocAt(offset);
if (!reloc)
return nullptr;
- return reloc->referent.get<Symbol *>();
+ Symbol *sym = reloc->referent.get<Symbol *>();
+
+ if (reloc->addend) {
+ assert(isa<Defined>(sym) && "Expected defined for non-zero addend");
+ Defined *definedSym = cast<Defined>(sym);
+ sym = tryFindDefinedOnIsec(definedSym->isec(),
+ definedSym->value + reloc->addend);
+ }
+
+ return sym;
+}
+
+Defined *ObjcCategoryMerger::tryFindDefinedOnIsec(const InputSection *isec,
+ uint32_t offset) {
+ for (Defined *sym : isec->symbols)
+ if ((sym->value <= offset) && (sym->value + sym->size > offset))
+ return sym;
+
+ return nullptr;
}
Defined *
|
kyulee-com
reviewed
May 8, 2024
| Defined *ObjcCategoryMerger::tryFindDefinedOnIsec(const InputSection *isec, | ||
| uint32_t offset) { | ||
| for (Defined *sym : isec->symbols) | ||
| if ((sym->value <= offset) && (sym->value + sym->size > offset)) |
There was a problem hiding this comment.
I wonder why we have an offset in the middle of symbol.
There was a problem hiding this comment.
I did not fully debug it but it does happen in very rare cases. I don't think it violates any rule, the reloc can refer to within a symbol, not necessarily to the start of one.
There was a problem hiding this comment.
I'd like to tighten the condition as possible for the expected case or value, instead of loosely accepting a valid case.
There was a problem hiding this comment.
I am not sure I understand. I can't think of a narrower case that would actually still work - we do need to find a symbol by a reloc into the middle of it (or any arbitrary position).
There was a problem hiding this comment.
Basically, I'd like to understand why we have an offset in the middle of symbol. If it's unexpected, we should fix the root-cause. If expected, we should document it.
There was a problem hiding this comment.
OK, I will look into the cases where this happens and come back to this change later.
There was a problem hiding this comment.
In the test, the addend comes from:
__CATEGORY__TtC11SimpleClass11SimpleClass_$_SimpleClass:
[...]
.quad _$s11SimpleClassAACMf+24 // <---- Addend = 24
There was a problem hiding this comment.
The function here seems similar to this. Perhaps, implementation could be shared or refactored?
There was a problem hiding this comment.
I think they might be different enough to remain separate because:
- In InputFiles it requires the offset to be to the start of the symbol:
// The offset should point at the exact address of a symbol (with no addend.)
Here the offset can be to the inside of the symbol - see here
.quad __DATA__TtC11SimpleClass11SimpleClass+2
We could remove this requirement and share them - but would have to remove the assert from the version in InputFiles.
- In InputFiles it relies on the symbols being ordered - which is not necessarily the case inside optimization passes. So here we have to do a linear scan. I don't think in practice this would be a restriction as the stage of this pass I don't think we run anything that reorders the symbols for these sections - but it's not a hard requirement to have them sorted as far as I know.
There was a problem hiding this comment.
I know the comment says that but the implementation seems to handle it for the case when it doesn't. Okay with the linear scan, but it would be a nice to make it faster if we know it's already sorted (probably worth checking). If it is sorted, it's worth adding an assert in that case as well to make sure we do do not invalidate correctness behavior.
alx32
force-pushed
the
09_symbol_addend
branch
3 times, most recently
from
4798b9c to
fb979de
Compare
thevinster
reviewed
Jun 23, 2024
| Defined *ObjcCategoryMerger::tryFindDefinedOnIsec(const InputSection *isec, | ||
| uint32_t offset) { | ||
| for (Defined *sym : isec->symbols) | ||
| if ((sym->value <= offset) && (sym->value + sym->size > offset)) |
There was a problem hiding this comment.
The function here seems similar to this. Perhaps, implementation could be shared or refactored?
thevinster
approved these changes
Jun 25, 2024
| Defined *ObjcCategoryMerger::tryFindDefinedOnIsec(const InputSection *isec, | ||
| uint32_t offset) { | ||
| for (Defined *sym : isec->symbols) | ||
| if ((sym->value <= offset) && (sym->value + sym->size > offset)) |
There was a problem hiding this comment.
I know the comment says that but the implementation seems to handle it for the case when it doesn't. Okay with the linear scan, but it would be a nice to make it faster if we know it's already sorted (probably worth checking). If it is sorted, it's worth adding an assert in that case as well to make sure we do do not invalidate correctness behavior.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/783 Here is the relevant piece of the build log for the reference: |
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…fset (llvm#91238) Currently the `tryFindDefinedOnIsec` takes in an `InputSection` and an `offset` and is supposed to return the target symbol that is referenced on that `InputSection` at the given offset. However, it does not deal with the reloc `addend` and might return the incorrect symbol. Here we add support for handling the reloc's `addend`.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…fset (llvm#91238) Currently the `tryFindDefinedOnIsec` takes in an `InputSection` and an `offset` and is supposed to return the target symbol that is referenced on that `InputSection` at the given offset. However, it does not deal with the reloc `addend` and might return the incorrect symbol. Here we add support for handling the reloc's `addend`.
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.
Currently the
tryFindDefinedOnIsectakes in anInputSectionand anoffsetand is supposed to return the target symbol that is referenced on thatInputSectionat the given offset.However, it does not deal with the reloc
addendand might return the incorrect symbol.Here we add support for handling the reloc's
addend.