[Lldb-commits] [PATCH] D70847: [lldb] Set executable module when adding modules to the Target

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:16
+        """Test adding images to the target."""
+        self.build()
+
----------------
clayborg wrote:
> Create a yaml file and put it in the same directory as this file. Change this line to be:
> ```
>         src_dir = self.getSourceDir()
>         yaml_path = os.path.join(src_dir, "a.yaml")
>         obj_path = self.getBuildArtifact("a.out")
>         self.yaml2obj(yaml_path, obj_path)
> ```
> Make an ELF file that that is for a remote platform where the triple and platform won't match typical hosts.
Do you have any specific suggestions that also have freely available toolchain? I have immediate access only to Windows/Linux x86 and Synopsys ARC, and ARC elf files don't change the platform - it remains as "host" even when connecting to a GDB-server, plus obj2yaml segfaults on ARC elf files.


================
Comment at: lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:21
+        self.assertTrue(target, VALID_TARGET)
+        self.assertEqual(len(target.GetTriple()), 0)
+
----------------
clayborg wrote:
> Don't know if I would run this assert here. I could see a target that is created with nothing possibly using the host architecture and host platform by default. So maybe remote this assert where the target triple is not set.
As far as I can understand from the `TargetList::CreateTargetInternal`, new target will not inherit architecture from the platform when exe file is not specified and neither architecture nor platform are given explicitly. So it seems that assuming that triplet will be empty is safe.


================
Comment at: lldb/packages/Python/lldbsuite/test/commands/target/set-exec/TestSetExecutable.py:26
+        target.AddModule(self.getBuildArtifact("a.out"), None, None)
+        self.assertNotEqual(len(target.GetTriple()), 0)
----------------
clayborg wrote:
> Check the exact triple of the remote executable you end up adding.
> 
> Also add a check that the platform gets updated. Platforms must be compatible with the architecture of the target's main executable, so it is good to verify that the platform gets updated as well. Something like:
> 
> ```
> platform = target.GetPlatform()
> set.assertEqual(platform.GetName(), "remote-linux")
> ```
> 
> This will make this test complete. If the platform isn't right we will need to fix this as well.
The platform doesn't change with the current code. `Target::SetExecutableModule` does direct assignment of architecture and doesn't touch the platform, so this is needed:

```
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1405,7 +1405,7 @@ void Target::SetExecutableModule(ModuleSP &executable_sp,
     // If we haven't set an architecture yet, reset our architecture based on
     // what we found in the executable module.
     if (!m_arch.GetSpec().IsValid()) {
-      m_arch = executable_sp->GetArchitecture();
+      SetArchitecture(executable_sp->GetArchitecture(), true);
       LLDB_LOG(log,
                "setting architecture to {0} ({1}) based on executable file",
                m_arch.GetSpec().GetArchitectureName(),
```

It works for me in the test case, but I wonder if `SetArchitecture` wasn't used there in the first place for a good reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70847/new/

https://reviews.llvm.org/D70847