Skip to content

Conversation

@GeorgeHuyubo
Copy link
Contributor

@GeorgeHuyubo GeorgeHuyubo commented Jun 11, 2025

Since this PR: #141670 We started to override the Platform/Arch for a target if needed. However we may have already registered locate module callback with the old platform.

This PR will move the locate module callback to the new Platform whenever Target changes architecture.

@llvmbot
Copy link
Member

llvmbot commented Jun 11, 2025

@llvm/pr-subscribers-lldb

Author: None (GeorgeHuyubo)

Changes

Since this PR: #141670

We start to override the Platform/Arch for a target if needed. However we may have already registered locate module callback with the old platform. So this fix will just ensure to set the callback to the new platform object.


Full diff: https://github.com/llvm/llvm-project/pull/143793.diff

1 Files Affected:

  • (modified) lldb/source/Target/Target.cpp (+2)
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 9660fc97970b0..45a9e1196a049 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1706,6 +1706,8 @@ bool Target::SetArchitecture(const ArchSpec &arch_spec, bool set_platform,
         if (PlatformSP arch_platform_sp =
                 GetDebugger().GetPlatformList().GetOrCreate(other, {},
                                                             &platform_arch)) {
+          arch_platform_sp->SetLocateModuleCallback(
+              platform_sp->GetLocateModuleCallback());
           SetPlatform(arch_platform_sp);
           if (platform_arch.IsValid())
             other = platform_arch;

Copy link
Contributor

@Jlalond Jlalond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Let's update the PR description to explain whenever Target changes architecture we mvoe any corresponding platform Module callback, because that wasn't really apparent to me from reading the summary.

@GeorgeHuyubo GeorgeHuyubo changed the title Properly handle locate module callback while overriding Properly handle locate module callback while Target change arch Jun 12, 2025
@Jlalond Jlalond changed the title Properly handle locate module callback while Target change arch Properly handle locate module callback when Target change arch Jun 12, 2025
@Jlalond Jlalond changed the title Properly handle locate module callback when Target change arch [lldb] Properly handle locate module callback when Target change arch Jun 12, 2025
@GeorgeHuyubo GeorgeHuyubo merged commit b1f5e26 into llvm:main Jun 12, 2025
9 checks passed
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
…llvm#143793)

Since this PR: llvm#141670 We
started to override the Platform/Arch for a target if needed. However we
may have already registered locate module callback with the old
platform.

This PR will move the locate module callback to the new Platform
whenever Target changes architecture.

Co-authored-by: George Hu <[email protected]>
Comment on lines +1709 to +1710
arch_platform_sp->SetLocateModuleCallback(
platform_sp->GetLocateModuleCallback());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This almost seems like the SetLocateModuleCallback should be on the target instead of on the platform. But this will work around the issue for sure.

akuhlens pushed a commit to akuhlens/llvm-project that referenced this pull request Jun 24, 2025
…llvm#143793)

Since this PR: llvm#141670 We
started to override the Platform/Arch for a target if needed. However we
may have already registered locate module callback with the old
platform.

This PR will move the locate module callback to the new Platform
whenever Target changes architecture.

Co-authored-by: George Hu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants