Skip to content
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

[lldb] Fixed the TestDebuggerAPI test on x86_64 Windows host #90580

Merged
merged 2 commits into from
May 24, 2024

Conversation

slydiman
Copy link
Contributor

@slydiman slydiman commented Apr 30, 2024

Disable the TestDebuggerAPI test in case of the remote target and Windows host. It is related to #92419.

…stDebuggerAPI.py

It is necessary to select the expected platform at the beginning.
In case of `Windows` host platform1.GetName() returned `host`. platform2.GetName() returned `remote-linux`, but platform2.GetWorkingDirectory() was None and finally
```
  File "llvm-project\lldb\test\API\python_api\debugger\TestDebuggerAPI.py", line 108, in test_CreateTarget_platform
    platform2.GetWorkingDirectory().endswith("bar"),
AttributeError: 'NoneType' object has no attribute 'endswith'
```
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 30, 2024

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

It is necessary to select the expected platform at the beginning. In case of Windows host platform1.GetName() returned host. platform2.GetName() returned remote-linux, but platform2.GetWorkingDirectory() was None and finally

  File "llvm-project\lldb\test\API\python_api\debugger\TestDebuggerAPI.py", line 108, in test_CreateTarget_platform
    platform2.GetWorkingDirectory().endswith("bar"),
AttributeError: 'NoneType' object has no attribute 'endswith'

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

1 Files Affected:

  • (modified) lldb/test/API/python_api/debugger/TestDebuggerAPI.py (+1)
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index 522de2466012ed..3d6484e5c9fbc4 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -95,6 +95,7 @@ def test_CreateTarget_platform(self):
         exe = self.getBuildArtifact("a.out")
         self.yaml2obj("elf.yaml", exe)
         error = lldb.SBError()
+        self.dbg.SetSelectedPlatform(lldb.SBPlatform("remote-linux"))
         target1 = self.dbg.CreateTarget(exe, None, "remote-linux", False, error)
         self.assertSuccess(error)
         platform1 = target1.GetPlatform()

@slydiman slydiman requested a review from labath April 30, 2024 09:48
Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

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

I don't think this is the right thing to do. SBDebugger::CreateTarget takes a platform_name argument which we're already setting to "remote-linux". If target1.GetPlatform() doesn't return the SBPlatform for remote-linux then I would think that's the actual bug.

@slydiman
Copy link
Contributor Author

slydiman commented Apr 30, 2024

@bulbazord

SBDebugger::CreateTarget takes a platform_name argument which we're already setting to "remote-linux".

Probably it works on buildbots because the host architecture is Aarch64. I'm trying to get it working on Windows x86_64.

target1 = self.dbg.CreateTarget(exe, None, "remote-linux", False, error) calls TargetList::CreateTargetInternal() in llvm-project/lldb/source/Target/TargetList.cpp, line 76

platform_options contains remote-linux, triple_str is empty, user_exe_path is a path to ELF created from elf.yaml (x86_64).

platform_arch is updated to x86_64 in TargetList.cpp, line 169:

// Only one arch and none was specified.
prefer_platform_arch = true;
platform_arch = matching_module_spec.GetArchitecture();

and platform_sp is updated to host in TargetList.cpp, line 226:

// If "arch" isn't valid, yet "platform_arch" is, it means we have an
// executable file with a single architecture which should be used.
ArchSpec fixed_platform_arch;
if (!platform_sp->IsCompatibleArchitecture(platform_arch, {}, ArchSpec::CompatibleMatch, nullptr)) {
  platform_sp = platform_list.GetOrCreate(platform_arch, {}, &fixed_platform_arch);
  if (platform_sp)
    platform_list.SetSelectedPlatform(platform_sp);
}

Next call target2 = self.dbg.CreateTarget(exe, None, "remote-linux", False, error) will create the platform remote-linux in TargetList.cpp, line 98:

// Create a new platform if a platform was specified in the platform options and doesn't match the selected platform.
if (platform_options && platform_options->PlatformWasSpecified() && !platform_options->PlatformMatches(platform_sp)) {
  const bool select_platform = true;
  platform_sp = platform_options->CreatePlatformWithOptions(debugger.GetCommandInterpreter(), 
      arch, select_platform, error, platform_arch);
  if (!platform_sp)
    return error;
}

I don't think we need to change this logic. So the fix of this test looks reasonable.

@bulbazord
Copy link
Member

I think you've laid out the events that happen nicely but I came to the opposite conclusion. I still don't think this is the right fix. We have buildbots running on x86_64 and it works there too. I don't think this test working on AArch64 machines is related. The platform architecture getting set to x86_64 makes sense to me because the ELF is built as an x86_64 binary. However, the platform_sp getting set to host during the creation of the Target. That seems like the bug to me, especially because we explicitly set the platform to remote-linux when trying to create the target. The presented solution is a small bandage to work around the underlying bug. Another way to look at it might be: If we have to set the platform to remote-linux ourselves after creating the target, what's the point of SBDebugger::CreateTarget taking a platform name as an argument?

@slydiman slydiman changed the title [lldb][Windows] Fixed unresolved test lldb-api python_api/debugger/TestDebuggerAPI.py [lldb] Fixed unresolved test lldb-api python_api/debugger/TestDebuggerAPI.py on x86_64 host May 1, 2024
@labath
Copy link
Collaborator

labath commented May 3, 2024

FWIW, I agree with @bulbazord. If the user specifies an explicit platform in the CreateTarget function, that platform should really take precedence over anything else.
(If it were up to me, I would not even attempt matching other platforms in this case (and just refuse to create the target if the provided platform does not work), but that might be going too far in terms of changing existing behavior).

@slydiman
Copy link
Contributor Author

slydiman commented May 16, 2024

I have added the issue #92419 and updated the test with @expectedFailureAll and the bugnumber for now.

@slydiman slydiman requested a review from bulbazord May 16, 2024 17:28
@slydiman slydiman changed the title [lldb] Fixed unresolved test lldb-api python_api/debugger/TestDebuggerAPI.py on x86_64 host [lldb] Fixed the TestDebuggerAPI test on x86_64 Windows host May 16, 2024
@slydiman
Copy link
Contributor Author

We are almost done with configuring the buildbot for cross lldb (Windows x86_64 host and Linux Aarch64 target). We are trying to get it green with minimal local patches.

@slydiman slydiman merged commit 77369a7 into llvm:main May 24, 2024
4 checks passed
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.

None yet

5 participants