Fix for dotest.py ERRORs on OSX caused by svn rev.237053.

[dawn]

Summary

This fixes dotest.py on OSX after svn rev.237053. After that commit, running dotest.py on OSX would return nothing but ERRORs after running test test_disassemble_invalid_vst_1_64_raw_data (TestDisassemble_VST1_64.Disassemble_VST1_64), and the total tests dropped from 1324 to 850. See my comments in http://reviews.llvm.org/rL237053.

[ted]

This is essentially reverting http://reviews.llvm.org/rL237053 , so we'd go back to the problem with it - LLDB won't be able to set the platform based on the architecture in the target binary, but will use the currently selected platform, even if it's not compatible.

Line 131 has this:

platform_sp = debugger.GetPlatformList().GetSelectedPlatform();

So platform_sp will always be true, unless LLDB doesn't have a current (or default host) platform. The code to check to see if the target arch and the platform are compatible will never be run.

What error is causing the tests to fail?

[dawn]

In D12303#231759, @ted wrote:

This is essentially reverting http://reviews.llvm.org/rL237053 , so we'd go back to the problem with it - LLDB won't be able to set the platform based on the architecture in the target binary, but will use the currently selected platform, even if it's not compatible.

Hmm. Looking at test test_disassemble_invalid_vst_1_64_raw_data, this appeared to be intentional since it does:

target = self.dbg.CreateTargetWithFileAndTargetTriple ("", "thumbv7")

and expects the target arch to be set from that. There are a couple other tests which are setting the arch manually like this also. It's certainly possible that the test cases are wrong. Should I replace this patch with skips for those tests?

So platform_sp will always be true, unless LLDB doesn't have a current (or default host) platform.

Exactly. Again, this looked intentional. Hmm.

What error is causing the tests to fail?

Please see my comment at http://reviews.llvm.org/rL237053. On OSX, all you see is ERROR and nothing else.

[clayborg]

Unless you really know what you are doing, I don't want anyone playing with this code just to fix some random test case.

What must remain true: we first try the currently selected platform, but we must make sure it is compatible with the architecture of the binary specified and the arch that was specified. If the platform and arch are compatible with the current platform, we use it. Else we find a platform that will work with the current binary and specified architecture (either of which can be missing).

[dawn]

Thank you both for your explanation and reviews. After further investigation in light of this, it appears the problem most likely lies in the python API. That's an area I'm unfamiliar with however. I'll open a bug and submit a patch to disable the offending test.

[ted]

My guess is the target create is failing. This line in the test:

target = self.dbg.CreateTargetWithFileAndTargetTriple ("", "thumbv7")

Seems to be the same as:

target create -a thumbv7 ""

On my Linux box, I get this:

(lldb) target create -a thumbv7 ""
Current executable set to '' (arm).
(lldb) target list
Current targets:
* target #0: <none> ( arch=arm--linux, platform=remote-linux )

My guess is LLDB on OSX doesn't have a platform that would handle a "thumbv7" target, so the target create fails.

Perhaps LLDB should stick with the selected platform if it's valid and it can't find a platform that is compatible with the target. Something like:

if (!prefer_platform_arch && arch.IsValid())
{
    if (!platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch))
    {
        platform_sp = Platform::GetPlatformForArchitecture(arch, &platform_arch);
        if (!is_dummy_target && platform_sp)
            debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
    }
}
else if (platform_arch.IsValid())
{
    // 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, false, &fixed_platform_arch))
    {
        platform_sp = Platform::GetPlatformForArchitecture(platform_arch, &fixed_platform_arch);
        if (!is_dummy_target && platform_sp)
            debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
    }
}
if (!platform_sp)
    platform_sp = debugger.GetPlatformList().GetSelectedPlatform();

What do you think, Greg - return an error, or use the current (incompatible) platform?

[dawn]

My guess is LLDB on OSX doesn't have a platform that would handle a "thumbv7" target, so the target create fails.

It looks OK to me. It does:

(lldb) target create -a thumbv7 ""
Current executable set to '' (thumbv7).
(lldb) target list
Current targets:
* target #0: <none> ( arch=thumbv7-apple-ios, platform=remote-ios )

I put printfs in lldb and ran the tests and I can see that the platform_sp is getting set, so this change won't make a difference:

if (!platform_sp)
    platform_sp = debugger.GetPlatformList().GetSelectedPlatform();

Your change in r237053 caused it to get *reset* (which you guys confirmed is as intended), and that somehow caused the python API to be left in a bad state to where no other tests can run. The test test_disassemble_invalid_vst_1_64_raw_data passes - it's all the ones that follow it that fail. I opened llvm.org/pr24575 for this and have a new patch on review http://reviews.llvm.org/D12329 to skip the test.

[dawn]

Hi Ted,
Your patch is causing it to get set to the dummy target in the 1st call to CreateTargetInternal, so in the recursive call, is_dummy_target is true, so we get the new platform but don't set it. Removing the tests for "!is_dummy_target &&" in your patch fixes the problem. Would you go along with that as a fix?

[dawn]

I'm submitting this revised patch to show the alternate fix described in my previous comment. Consider this patch as a proposal; if nothing else I'd like to understand why the checks for is_dummy_target were added in svn rev.237053.

• what's new in this diff -

This patch keeps the resetting behavior in rev.237053 while still fixing the mix-matched platform and dummy target problem that is leaving the python API in a bad state. An additional change was made to check for user_exe_path[0] since this is common practice elsewhere in the code.

[dawn]

This patch works on a small set of tests but fails in the same way that r237053 does when dotest is run on the entire set of lldb tests. Please accept http://reviews.llvm.org/D12329 until someone can investigate this further and provide a proper fix. Thanks!

[clayborg]

Isn't fixing this as simple as having the TestDisassemble_VST1_64.py restore the platform correctly?

[ted]

I tried deleting the target and setting the platform to the host platform at the end of the test in TestDisassemble_VST1_64.py, but that didn't solve the problem.

source/Target/TargetList.cpp

         }
     }
 
-    // If we have a valid architecture, make sure the current platform is
-    // compatible with that architecture
-    if (!prefer_platform_arch && arch.IsValid())
+    if (!platform_sp)
     {
-        if (!platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch))
+        // Get the current platform.
+        platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+        assert(platform_sp);
+        
+        // If we have a valid architecture, make sure the current platform is
+        // compatible with that architecture
+        if (!prefer_platform_arch && arch.IsValid())
         {
-            platform_sp = Platform::GetPlatformForArchitecture(arch, &platform_arch);
-            if (!is_dummy_target && platform_sp)
-                debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
+            if (!platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch))
+            {
+                platform_sp = Platform::GetPlatformForArchitecture(arch, &platform_arch);
+                if (!is_dummy_target && platform_sp)
+                    debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
+            }
         }
-    }
-    else if (platform_arch.IsValid())
-    {
-        // 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, false, &fixed_platform_arch))
+        else if (platform_arch.IsValid())
         {
-            platform_sp = Platform::GetPlatformForArchitecture(platform_arch, &fixed_platform_arch);
-            if (!is_dummy_target && platform_sp)
-                debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
+            // 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, false, &fixed_platform_arch))
+            {
+                platform_sp = Platform::GetPlatformForArchitecture(platform_arch, &fixed_platform_arch);
+                if (!is_dummy_target && platform_sp)
+                    debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
+            }
         }
     }