[MLIR][mlir-link] Additional fix for appendGlobal for single-module case, add more ConflictResolution handling #74
Conversation
|
LGTM as far as the changes go, currently waiting for the LLVM test suite to compile & pass |
| // Handle external weak linkage conflicts | ||
| if (isExternalWeakLinkage(srcLinkage)) { | ||
| if (isExternalWeakLinkage(dstLinkage)) { | ||
| // Both external weak, keep dst | ||
| return ConflictResolution::LinkFromDst; | ||
| } | ||
| if (isExternalLinkage(dstLinkage) || isWeakForLinker(dstLinkage)) { | ||
| // External weak loses to external or other weak | ||
| return ConflictResolution::LinkFromDst; | ||
| } | ||
| // External weak wins over declarations | ||
| if (dstIsDeclaration) { | ||
| return ConflictResolution::LinkFromSrc; | ||
| } | ||
| return ConflictResolution::LinkFromDst; | ||
| } | ||
|
|
||
| if (isExternalWeakLinkage(dstLinkage)) { | ||
| if (isExternalLinkage(srcLinkage) || isWeakForLinker(srcLinkage)) { | ||
| // External or weak src wins over external weak dst | ||
| return ConflictResolution::LinkFromSrc; | ||
| } | ||
| // External weak wins over declarations | ||
| if (srcIsDeclaration) { | ||
| return ConflictResolution::LinkFromDst; | ||
| } | ||
| return ConflictResolution::LinkFromSrc; | ||
| } | ||
|
|
||
| // Handle available externally + external definition conflicts | ||
| if (isAvailableExternallyLinkage(srcLinkage) && | ||
| isExternalLinkage(dstLinkage)) { | ||
| // External definition wins over available externally | ||
| return ConflictResolution::LinkFromDst; | ||
| } | ||
|
|
||
| if (isAvailableExternallyLinkage(dstLinkage) && | ||
| isExternalLinkage(srcLinkage)) { | ||
| // External definition wins over available externally | ||
| return ConflictResolution::LinkFromSrc; | ||
| } |
There was a problem hiding this comment.
This part seems a bit strange to me…
llvm-link does not have to handle this (see their implementation of conflict resolution) and it seems like quite a lot of potentially unresolved conflicts they would encounter.
Was this maybe necessary to add because of some other bug? Where/when did you encounter these unresolved conflicts?
There was a problem hiding this comment.
Yes, for safety I added conditionals to cover all other cases that didn't look to be covered, particularly for situations where it seems both symbols have definitions already from the targets. Will reinvestigate to see if its extraneous. This was for analyze-image and ros2.
For ros2 specifically:
colcon build --merge-install --base-paths src --cmake-args -DCMAKE_TOOLCHAIN_FILE=${TOOLCHAIN_FILE} --parallel-workers $(nproc) --packages-select iceoryx_hoofs
...
OBJ: Added input object CMakeFiles/iceoryx_hoofs.dir/source/log/logging.cpp.o
OBJ: Added input object CMakeFiles/iceoryx_hoofs.dir/source/log/logging_internal.cpp.o
unimplemented conflict resolution
UNREACHABLE executed at /home/alancao/instafix-llvm/llvm/out/install/linux/include/mlir/Linker/LLVMLinkerMixin.h:413!
cannot parse input module libiceoryx_hoofs.so
There was a problem hiding this comment.
I see. I will try to give it a look to see if that's a symptom of some other bug or if this is a legal case.
IIUIC, there are some invalid cases that should not pop-up (or should not allow linking to continue), so that's why I'm trying to be careful around it :)
| if (globs.empty()) { | ||
| if (auto found = summary.find(glob); found != summary.end()) | ||
| return state.clone(found->second); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Do you have a test-case for this? Unless it's too complicated.
Feels like we are chasing bugs in this part, so it would be good to have regression tests
|
@Jezurko apologies for the delay, was working heavily on the recursive dynamic linking. I've minified the PR to only handle the particular cases in The particular component in ros2, which would correspond to if (srcIsDeclaration && !dstIsDeclaration)
return ConflictResolution::LinkFromDst; |
ex0dus-0x
force-pushed
the
alan/more-symb-fixes
branch
from
4a28fb4 to
2c230e4
Compare
3 participants
No description provided.