Move some android platform functions to lldbplatformutil

[zturner]

Summary

Move some android platform functions to lldbplatformutil.

My eventual goal is to move all of the test decorators to their
own module such as `decorators.py`.  But some of the decorators
use existing functions in `lldbtest.py` and conceptually the
functions are probably more appropriately placed in lldbplatformutil.
Moreover, `lldbtest.py` is a huge file with a ton of random utility
functions scattered around, so this patch also works toward the
goal of reducing the footprint of this one module to a more
reasonable size.

So this patch starts by moveing some utility functions over to
`lldbplatformutil` with the eventual goal of moving decorators over
 to their own module as well.

[labath]

Seems to be working after applying the fixes below.

I was considering whether this shouldn't be moved to an even more platform-specific file (say androidutil.py or something), but I'll leave that up to you...

packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py

63 ↗

(On Diff #46721)

valid_api_levels=...

packages/Python/lldbsuite/test/lldbplatformutil.py

38 ↗

(On Diff #46721)

subprocess.PIPE

[tberghammer]

Looks reasonable and I agree with Pavel in the idea of moving the android related utility functions into their own file for better separation

packages/Python/lldbsuite/test/lldbplatformutil.py

73–81 ↗

(On Diff #46721)

Can we just return True/False? I don't see any usecase where we are caring about the reason why the device isn't matched.

packages/Python/lldbsuite/test/lldbtest.py

683–685 ↗

(On Diff #46721)

Can we move this function to a higher scope so we don't have to duplicate it in every android specific decorator?

[zturner]

In D16830#342842, @tberghammer wrote:

Looks reasonable and I agree with Pavel in the idea of moving the android related utility functions into their own file for better separation

I thought about creating an android specific module but I decided taht there is so little code right now it might not make sense. And then we already had this lldbplatformutil file that had only 1 function in it, so nobody was even really using it. So rather than end up with 2 different modules with 3 functions each, just have just have 1 module with 6 functions. On the other hand, if they continue to grow, then splitting apart makes a lot of sense.

It's not a big deal to me either way, so if you guys feel strongly I can move it. Since you're in a different time zone though and might not get this for another 24 hours, maybe I'll commit like this and then if you

[zturner]

In D16830#343232, @zturner wrote:

In D16830#342842, @tberghammer wrote:

Looks reasonable and I agree with Pavel in the idea of moving the android related utility functions into their own file for better separation

I thought about creating an android specific module but I decided taht there is so little code right now it might not make sense. And then we already had this lldbplatformutil file that had only 1 function in it, so nobody was even really using it. So rather than end up with 2 different modules with 3 functions each, just have just have 1 module with 6 functions. On the other hand, if they continue to grow, then splitting apart makes a lot of sense.

It's not a big deal to me either way, so if you guys feel strongly I can move it. Since you're in a different time zone though and might not get this for another 24 hours, maybe I'll commit like this and then if you

(somehow my previous message got cut off).

if you want it changed let me know and I'll move it to an android-specific file tomorrow.

[sivachandra]

This change broke Android testsuite run:
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/5147

[zturner]

I ahad 3 CLs in progress locally and I guess I messed up a merge. There's
supposed to be a subprocess.PIPE instead of just PIPE. I'll check in a fix
shortly.

[zturner]

r259724

lldb/trunk/packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py

 """Test that lldb functions correctly after the inferior has asserted."""
 
 from __future__ import print_function
 
 
 
 import os, time
 import lldb
-import lldbsuite.test.lldbutil as lldbutil
-import lldbsuite.test.lldbplatformutil as lldbplatformutil
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
 from lldbsuite.test.lldbtest import *
 
 class AssertingInferiorTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @expectedFailureWindows("llvm.org/pr21793: need to implement support for detecting assertion / abort on Windows")
     @expectedFailureLinux("llvm.org/pr25338", archs=['arm'])
def test_inferior_asserting_step(self):
         """Test that lldb functions correctly after stepping through a call to assert()."""
         self.build()
         self.inferior_asserting_step()
 
     def set_breakpoint(self, line):
         lldbutil.run_break_set_by_file_and_line (self, "main.c", line, num_expected_locations=1, loc_exact=True)
 
     def check_stop_reason(self):
-        match_result = matchAndroid(api_levels=list(range(1, 16+1)))(self)
-        if match_result is not None:
+        matched = lldbplatformutil.match_android_device(self.getArchitecture(), valid_api_levels=list(range(1, 16+1)))
+        if matched:
             # On android until API-16 the abort() call ended in a sigsegv instead of in a sigabrt
             stop_reason = 'stop reason = signal SIGSEGV'
         else:
             stop_reason = 'stop reason = signal SIGABRT'
 
         # The stop reason of the thread should be an abort signal or exception.
         self.expect("thread list", STOPPED_DUE_TO_ASSERT,
             substrs = ['stopped',

lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py

 """ This module contains functions used by the test cases to hide the
 architecture and/or the platform dependent nature of the tests. """
 
 from __future__ import absolute_import
 
 # System modules
+import re
+import subprocess
 
 # Third-party modules
+from six.moves.urllib import parse as urlparse
 
 # LLDB modules
-
-import re
+from . import configuration
+import use_lldb_suite
+import lldb
 
 def check_first_register_readable(test_case):
     arch = test_case.getArchitecture()
 
     if arch in ['x86_64', 'i386']:
         test_case.expect("register read eax", substrs = ['eax = 0x'])
     elif arch in ['arm']:
       test_case.expect("register read r0", substrs = ['r0 = 0x'])
     elif arch in ['aarch64']:
         test_case.expect("register read x0", substrs = ['x0 = 0x'])
     elif re.match("mips",arch):
         test_case.expect("register read zero", substrs = ['zero = 0x'])
     else:
         # TODO: Add check for other architectures
         test_case.fail("Unsupported architecture for test case (arch: %s)" % test_case.getArchitecture())
+
+def _run_adb_command(cmd, device_id):
+    device_id_args = []
+    if device_id:
+        device_id_args = ["-s", device_id]
+    full_cmd = ["adb"] + device_id_args + cmd
+    p = subprocess.Popen(full_cmd, stdout=PIPE, stderr=PIPE)
+    stdout, stderr = p.communicate()
+    return p.returncode, stdout, stderr
+
+def _target_is_android():
+    if not hasattr(_target_is_android, 'result'):
+        triple = lldb.DBG.GetSelectedPlatform().GetTriple()
+        match = re.match(".*-.*-.*-android", triple)
+        _target_is_android.result = match is not None
+    return _target_is_android.result
+
+def android_device_api():
+    if not hasattr(android_device_api, 'result'):
+        assert configuration.lldb_platform_url is not None
+        device_id = None
+        parsed_url = urlparse.urlparse(configuration.lldb_platform_url)
+        host_name = parsed_url.netloc.split(":")[0]
+        if host_name != 'localhost':
+            device_id = host_name
+            if device_id.startswith('[') and device_id.endswith(']'):
+                device_id = device_id[1:-1]
+        retcode, stdout, stderr = _run_adb_command(
+            ["shell", "getprop", "ro.build.version.sdk"], device_id)
+        if retcode == 0:
+            android_device_api.result = int(stdout)
+        else:
+            raise LookupError(
+                ">>> Unable to determine the API level of the Android device.\n"
+                ">>> stdout:\n%s\n"
+                ">>> stderr:\n%s\n" % (stdout, stderr))
+    return android_device_api.result
+
+def match_android_device(device_arch, valid_archs=None, valid_api_levels=None):
+    if not _target_is_android():
+        return False
+    if valid_archs is not None and device_arch not in valid_archs:
+        return False
+    if valid_api_levels is not None and android_device_api() not in valid_api_levels:
+        return False
+
+    return True
+
+def finalize_build_dictionary(dictionary):
+    if _target_is_android():
+        if dictionary is None:
+            dictionary = {}
+        dictionary["OS"] = "Android"
+        if android_device_api() >= 16:
+            dictionary["PIE"] = 1
+    return dictionary

lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py

 from subprocess import *
 import time
 import types
 
 # Third-party modules
 import unittest2
 from six import add_metaclass
 from six import StringIO as SixStringIO
-from six.moves.urllib import parse as urlparse
 import six
 
 # LLDB modules
 import use_lldb_suite
 import lldb
 from . import configuration
+from . import lldbplatformutil
 from . import lldbtest_config
 from . import lldbutil
 from . import test_categories
 from lldbsuite.support import encoded_file
 
 from .result_formatter import EventBuilder
 
 # dosep.py starts lots and lots of dotest instances

 def does_function_require_self(func):
     import inspect
     func_argc = len(inspect.getargspec(func).args)
     if func_argc == 0 or (getattr(func,'im_self',None) is not None) or (hasattr(func, '__self__')):
         return False
     else:
         return True
 
-def run_adb_command(cmd, device_id):
-    device_id_args = []
-    if device_id:
-        device_id_args = ["-s", device_id]
-    full_cmd = ["adb"] + device_id_args + cmd
-    p = Popen(full_cmd, stdout=PIPE, stderr=PIPE)
-    stdout, stderr = p.communicate()
-    return p.returncode, stdout, stderr
-
-def append_android_envs(dictionary):
-    if dictionary is None:
-        dictionary = {}
-    dictionary["OS"] = "Android"
-    if android_device_api() >= 16:
-        dictionary["PIE"] = 1
-    return dictionary
-
-def target_is_android():
-    if not hasattr(target_is_android, 'result'):
-        triple = lldb.DBG.GetSelectedPlatform().GetTriple()
-        match = re.match(".*-.*-.*-android", triple)
-        target_is_android.result = match is not None
-    return target_is_android.result
-
-def android_device_api():
-    if not hasattr(android_device_api, 'result'):
-        assert configuration.lldb_platform_url is not None
-        device_id = None
-        parsed_url = urlparse.urlparse(configuration.lldb_platform_url)
-        host_name = parsed_url.netloc.split(":")[0]
-        if host_name != 'localhost':
-            device_id = host_name
-            if device_id.startswith('[') and device_id.endswith(']'):
-                device_id = device_id[1:-1]
-        retcode, stdout, stderr = run_adb_command(
-            ["shell", "getprop", "ro.build.version.sdk"], device_id)
-        if retcode == 0:
-            android_device_api.result = int(stdout)
-        else:
-            raise LookupError(
-                ">>> Unable to determine the API level of the Android device.\n"
-                ">>> stdout:\n%s\n"
-                ">>> stderr:\n%s\n" % (stdout, stderr))
-    return android_device_api.result
-
 def check_expected_version(comparison, expected, actual):
     def fn_leq(x,y): return x <= y
     def fn_less(x,y): return x < y
     def fn_geq(x,y): return x >= y
     def fn_greater(x,y): return x > y
     def fn_eq(x,y): return x == y
     def fn_neq(x,y): return x != y
 

 #
 # Decorators for categorizing test cases.
 #
 class DecorateMode:
     Skip, Xfail = range(2)
 
 from functools import wraps
 
+def skip_for_android(reason, api_levels, archs):
+    def impl(obj):
+        result = lldbplatformutil.match_android_device(obj.getArchitecture(), valid_archs=archs, valid_api_levels=api_levels)
+        return reason if result else None
+    return impl
+
 def add_test_categories(cat):
     """Add test categories to a TestCase method"""
     cat = test_categories.validate(cat, True)
     def impl(func):
         if isinstance(func, type) and issubclass(func, unittest2.TestCase):
             raise Exception("@add_test_categories can only be used to decorate a test method")
         if hasattr(func, "categories"):
             cat.extend(func.categories)
def expectedFailureNetBSD(bugnumber=None, compilers=None, debug_info=None):
     return expectedFailureOS(['netbsd'], bugnumber, compilers, debug_info=debug_info)
 
 def expectedFailureWindows(bugnumber=None, compilers=None, debug_info=None):
     return expectedFailureOS(['windows'], bugnumber, compilers, debug_info=debug_info)
 
 def expectedFailureHostWindows(bugnumber=None, compilers=None):
     return expectedFailureHostOS(['windows'], bugnumber, compilers)
 
-def matchAndroid(api_levels=None, archs=None):
-    def match(self):
-        if not target_is_android():
-            return None     # Doesn't match, return false
-        if archs is not None and self.getArchitecture() not in archs:
-            return None     # Invalid architecture, return false
-        if api_levels is not None and android_device_api() not in api_levels:
-            return None     # API level doesn't match, return false
-
-        # This is a matching android distribution, return true
-        return "Android [arch={}] [api_level={}]".format(self.getArchitecture(), android_device_api())
-    return match
-
-
 def expectedFailureAndroid(bugnumber=None, api_levels=None, archs=None):
     """ Mark a test as xfail for Android.
 
     Arguments:
         bugnumber - The LLVM pr associated with the problem.
         api_levels - A sequence of numbers specifying the Android API levels
             for which a test is expected to fail. None means all API level.
         arch - A sequence of architecture names specifying the architectures
             for which a test is expected to fail. None means all architectures.
     """
-    return expectedFailure(matchAndroid(api_levels, archs), bugnumber)
+    return expectedFailure(skip_for_android("xfailing on android", api_levels, archs), bugnumber)
 
 # Flakey tests get two chances to run. If they fail the first time round, the result formatter
 # makes sure it is run one more time.
 def expectedFlakey(expected_fn, bugnumber=None):
     def expectedFailure_impl(func):
         @wraps(func)
         def wrapper(*args, **kwargs):
             self = args[0]

 def expectedFlakeyClang(bugnumber=None, compiler_version=None):
     return expectedFlakeyCompiler('clang', compiler_version, bugnumber)
 
 # @expectedFlakeyGcc('bugnumber', ['<=', '3.4'])
 def expectedFlakeyGcc(bugnumber=None, compiler_version=None):
     return expectedFlakeyCompiler('gcc', compiler_version, bugnumber)
 
 def expectedFlakeyAndroid(bugnumber=None, api_levels=None, archs=None):
-    return expectedFlakey(matchAndroid(api_levels, archs), bugnumber)
+    return expectedFlakey(skip_for_android("flakey on android", api_levels, archs), bugnumber)
 
 def skipIfRemote(func):
     """Decorate the item to skip tests if testing remotely."""
     def is_remote():
         return "skip on remote platform" if lldb.remote_platform else None
     return skipTestIfFn(is_remote)(func)
 
 def skipIfRemoteDueToDeadlock(func):
def skipIfTargetAndroid(api_levels=None, archs=None):
     """Decorator to skip tests when the target is Android.
 
     Arguments:
         api_levels - The API levels for which the test should be skipped. If
             it is None, then the test will be skipped for all API levels.
         arch - A sequence of architecture names specifying the architectures
             for which a test is skipped. None means all architectures.
     """
-    def is_target_android(self):
-        return matchAndroid(api_levels, archs)(self)
-    return skipTestIfFn(is_target_android)
+    return skipTestIfFn(skip_for_android("skipping for android", api_levels, archs))
 
 def skipUnlessCompilerRt(func):
     """Decorate the item to skip tests if testing remotely."""
     def is_compiler_rt_missing():
         compilerRtPath = os.path.join(os.path.dirname(__file__), "..", "..", "..", "..", "llvm","projects","compiler-rt")
         return "compiler-rt not found" if not os.path.exists(compilerRtPath) else None
     return skipTestIfFn(is_compiler_rt_missing)(func)
 
def buildProgram(self, sources, exe_name):
         """ Platform specific way to build an executable from C/C++ sources. """
         d = {'CXX_SOURCES' : sources,
              'EXE' : exe_name}
         self.buildDefault(dictionary=d)
 
     def buildDefault(self, architecture=None, compiler=None, dictionary=None, clean=True):
         """Platform specific way to build the default binaries."""
         module = builder_module()
-        if target_is_android():
-            dictionary = append_android_envs(dictionary)
+        dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDefault(self, architecture, compiler, dictionary, clean):
             raise Exception("Don't know how to build default binary")
 
     def buildDsym(self, architecture=None, compiler=None, dictionary=None, clean=True):
         """Platform specific way to build binaries with dsym info."""
         module = builder_module()
         if not module.buildDsym(self, architecture, compiler, dictionary, clean):
             raise Exception("Don't know how to build binary with dsym")
 
     def buildDwarf(self, architecture=None, compiler=None, dictionary=None, clean=True):
         """Platform specific way to build binaries with dwarf maps."""
         module = builder_module()
-        if target_is_android():
-            dictionary = append_android_envs(dictionary)
+        dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDwarf(self, architecture, compiler, dictionary, clean):
             raise Exception("Don't know how to build binary with dwarf")
 
     def buildDwo(self, architecture=None, compiler=None, dictionary=None, clean=True):
         """Platform specific way to build binaries with dwarf maps."""
         module = builder_module()
-        if target_is_android():
-            dictionary = append_android_envs(dictionary)
+        dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDwo(self, architecture, compiler, dictionary, clean):
             raise Exception("Don't know how to build binary with dwo")
 
     def buildGo(self):
         """Build the default go binary.
         """
         system([[which('go'), 'build -gcflags "-N -l" -o a.out main.go']])
 
def invoke(self, obj, name, trace=False):
         result = method()
         with recording(self, trace) as sbuf:
             print(str(method) + ":",  result, file=sbuf)
         return result
 
     def build(self, architecture=None, compiler=None, dictionary=None, clean=True):
         """Platform specific way to build the default binaries."""
         module = builder_module()
-        if target_is_android():
-            dictionary = append_android_envs(dictionary)
+        dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if self.debug_info is None:
             return self.buildDefault(architecture, compiler, dictionary, clean)
         elif self.debug_info == "dsym":
             return self.buildDsym(architecture, compiler, dictionary, clean)
         elif self.debug_info == "dwarf":
             return self.buildDwarf(architecture, compiler, dictionary, clean)
         elif self.debug_info == "dwo":
             return self.buildDwo(architecture, compiler, dictionary, clean)