[lldb] Extend builder to pass the TRIPLE spec to Make

[JDevlieghere]

Summary

This is needed to build the test binaries for iOS. This is part of upstreaming the last missing pieces to run the on-device test suite with the upstream repository.

[friss]

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

[JDevlieghere]

In D85539#2203247, @friss wrote:

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

[JDevlieghere]

• Simplify things a bit more
• Add a comment explaining that the triple spec is only relevant with an Apple SDK

[labath]

The part which bugs me about this is that the first thing that Makefile.rules will do with the triple we've painstakingly constructed here is it will decompose it into individual components, and then do some more matching on them, including running some xcrun commands, which are already being run here. I do think that doing this in python is a good idea (I believe the main reason a lot of this stuff is done in make is so that running "make" would just work, but we are way past that point now), but if we're going to do that in python, how about we move all of that to python? It seems the only effect of the TRIPLE argument is to add -target $(TRIPLE) -m$(OS)-version-min=$(VERSION) to CFLAGS. That should be easy to achieve using the information that construct_triple has available.

In D85539#2203251, @JDevlieghere wrote:

In D85539#2203247, @friss wrote:

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

How about making an abstract/dummy getTripleSpec (or whatever) method in the base builder class. Then the darwin builder could override it do to its thing, and all of this code could reside in builder_darwin.py. If any other platform decides it needs to set the triple argument, then it's likely it will do that via some completely different method.

lldb/packages/Python/lldbsuite/test/lldbremote.py

37–39 ↗

(On Diff #284010)

when dealing with triples, I believe this field is usually called the "os", not "platform".

[JDevlieghere]

In D85539#2215387, @labath wrote:

The part which bugs me about this is that the first thing that Makefile.rules will do with the triple we've painstakingly constructed here is it will decompose it into individual components, and then do some more matching on them, including running some xcrun commands, which are already being run here. I do think that doing this in python is a good idea (I believe the main reason a lot of this stuff is done in make is so that running "make" would just work, but we are way past that point now), but if we're going to do that in python, how about we move all of that to python? It seems the only effect of the TRIPLE argument is to add -target $(TRIPLE) -m$(OS)-version-min=$(VERSION) to CFLAGS. That should be easy to achieve using the information that construct_triple has available.

Sounds reasonable.

In D85539#2203251, @JDevlieghere wrote:

In D85539#2203247, @friss wrote:

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

It's not an issue because it checks for the Apple SDK, but I could hoist that out of the helper to make it more obvious.

How about making an abstract/dummy getTripleSpec (or whatever) method in the base builder class. Then the darwin builder could override it do to its thing, and all of this code could reside in builder_darwin.py. If any other platform decides it needs to set the triple argument, then it's likely it will do that via some completely different method.

Even though this file is called "base", there's no inheritance going on here, it's just methods and lldbtest.py will import the appropriate module. I considered this too, but didn't feel like rewriting those files as a classes for this small change. But if we're going to pass the TRIPLE, OS and VERSION separately, we might as well.

[labath]

In D85539#2216197, @JDevlieghere wrote:

Even though this file is called "base", there's no inheritance going on here, it's just methods and lldbtest.py will import the appropriate module. I considered this too, but didn't feel like rewriting those files as a classes for this small change. But if we're going to pass the TRIPLE, OS and VERSION separately, we might as well.

I'd just send the final ARCH_CFLAGS value. Then the Android.rules makefile could be ported to python as well.

[JDevlieghere]

Reimplement based on passing the ARCH_CFLAGS to Make

[JDevlieghere]

lldb/packages/Python/lldbsuite/builders/builder.py

224 ↗

(On Diff #286431)

This should've been part of D86174, I'll fix that before updating the patch/landing.

[jasonmolenda]

This looks good to me, getting any logic out of the Makefile templates that we can, that's a good improvement for maintainability.

[labath]

lldb/packages/Python/lldbsuite/builders/builder.py

201–202 ↗

(On Diff #286455)

Instead of a new method for each variable any subclass might want to set it would probably be better to just have an getExtraMakeArgs method that each subclass can return whatever it wants. ARCH_CFLAGS is fairly generic, so it may make sense to keep that separate, but DSYMUTIL and CODESIGN (and maybe SDKROOT) sound like they could go into the "extra" category.

[jankratochvil]

It broke the Debian buildbot: http://lab.llvm.org:8011/builders/lldb-x86_64-debian/builds/16095
And also Fedora (although there the commit looks as a different one): http://lab.llvm.org:8014/builders/lldb-x86_64-fedora?numbuilds=10000

lldb/packages/Python/lldbsuite/test/builders/builder.py

def getCompiler(self):
         compiler = lldbutil.which(compiler)
         return os.path.abspath(compiler)
 
     def getExtraMakeArgs(self):
         """
         Helper function to return extra argumentsfor the make system. This
         method is meant to be overridden by platform specific builders.
         """
+
+    def getArchCFlags(self, architecture):
+        """Returns the ARCH_CFLAGS for the make system."""
         return ""
 
     def getMake(self, test_subdir, test_name):
         """Returns the invocation for GNU make.
         The first argument is a tuple of the relative path to the testcase
         and its filename stem."""
         if platform.system() == "FreeBSD" or platform.system() == "NetBSD":
             make = "gmake"
def buildDefault(self,
                      dictionary=None,
                      testdir=None,
                      testname=None):
         """Build the binaries the default way."""
         commands = []
         commands.append(
             self.getMake(testdir, testname) + [
                 "all",
+                self.getArchCFlags(architecture),
                 self.getArchSpec(architecture),
                 self.getCCSpec(compiler),
                 self.getExtraMakeArgs(),
                 self.getSDKRootSpec(),
                 self.getModuleCacheSpec(),
                 self.getCmdLine(dictionary)
             ])
 
def buildDwarf(self,
                    dictionary=None,
                    testdir=None,
                    testname=None):
         """Build the binaries with dwarf debug info."""
         commands = []
         commands.append(
             self.getMake(testdir, testname) + [
                 "MAKE_DSYM=NO",
+                self.getArchCFlags(architecture),
                 self.getArchSpec(architecture),
                 self.getCCSpec(compiler),
                 self.getExtraMakeArgs(),
                 self.getSDKRootSpec(),
                 self.getModuleCacheSpec(),
                 self.getCmdLine(dictionary)
             ])
 
         self.runBuildCommands(commands, sender=sender)
         # True signifies that we can handle building dwarf.
         return True
 
     def buildDwo(self,
                  sender=None,
                  architecture=None,
                  compiler=None,
                  dictionary=None,
                  testdir=None,
                  testname=None):
         """Build the binaries with dwarf debug info."""
         commands = []
         commands.append(
             self.getMake(testdir, testname) + [
                 "MAKE_DSYM=NO", "MAKE_DWO=YES",
+                self.getArchCFlags(architecture),
                 self.getArchSpec(architecture),
                 self.getCCSpec(compiler),
                 self.getExtraMakeArgs(),
                 self.getSDKRootSpec(),
                 self.getModuleCacheSpec(),
                 self.getCmdLine(dictionary)
             ])
 
         self.runBuildCommands(commands, sender=sender)
         # True signifies that we can handle building dwo.
         return True
 
     def buildGModules(self,
                       sender=None,
                       architecture=None,
                       compiler=None,
                       dictionary=None,
                       testdir=None,
                       testname=None):
         """Build the binaries with dwarf debug info."""
         commands = []
         commands.append(
             self.getMake(testdir, testname) + [
                 "MAKE_DSYM=NO", "MAKE_GMODULES=YES",
+                self.getArchCFlags(architecture),
                 self.getArchSpec(architecture),
                 self.getCCSpec(compiler),
                 self.getExtraMakeArgs(),
                 self.getSDKRootSpec(),
                 self.getModuleCacheSpec(),
                 self.getCmdLine(dictionary)
             ])
 

lldb/packages/Python/lldbsuite/test/builders/darwin.py

+import re
+import subprocess
+
 from .builder import Builder
+from lldbsuite.test import configuration
+
+REMOTE_PLATFORM_NAME_RE = re.compile(r"^remote-(.+)$")
+SIMULATOR_PLATFORM_RE = re.compile(r"^(.+)-simulator$")
+
+
+def get_sdk(os, env):
+    if os == "ios":
+        if env == "simulator":
+            return "iphonesimulator"
+        if env == "macabi":
+            return "macosx"
+        return "iphoneos"
+    elif os == "tvos":
+        if env == "simulator":
+            return "appletvsimulator"
+        return "appletvos"
+    elif os == "watchos":
+        if env == "simulator":
+            return "watchsimulator"
+        return "watchos"
+    return os
+
+
+def get_os_env_from_platform(platform):
+    match = REMOTE_PLATFORM_NAME_RE.match(platform)
+    if match:
+        return match.group(1), ""
+    match = SIMULATOR_PLATFORM_RE.match(platform)
+    if match:
+        return match.group(1), "simulator"
+    return None, None
+
+
+def get_os_from_sdk(sdk):
+    return sdk[:sdk.find('.')], ""
 
 from lldbsuite.test import configuration
 
 
 class BuilderDarwin(Builder):
     def getExtraMakeArgs(self):
         """
         Helper function to return extra argumentsfor the make system. This
         method is meant to be overridden by platform specific builders.
         """
         args = dict()
 
         if configuration.dsymutil:
             args['DSYMUTIL'] = configuration.dsymutil
 
+        os, _ = self.getOsAndEnv()
+        if os and os != "macosx":
+            args['CODESIGN'] = 'codesign'
+
         # Return extra args as a formatted string.
         return ' '.join(
             {'{}="{}"'.format(key, value)
              for key, value in args.items()})
+    def getOsAndEnv(self):
+        if configuration.lldb_platform_name:
+            return get_os_env_from_platform(configuration.lldb_platform_name)
+        elif configuration.apple_sdk:
+            return get_os_from_sdk(configuration.apple_sdk)
+        return None, None
+
+    def getArchCFlags(self, architecture):
+        """Returns the ARCH_CFLAGS for the make system."""
+
+        # Construct the arch component.
+        arch = architecture if architecture else configuration.arch
+        if not arch:
+            arch = subprocess.check_output(['machine'
+                                            ]).rstrip().decode('utf-8')
+        if not arch:
+            return ""
+
+        # Construct the vendor component.
+        vendor = "apple"
+
+        # Construct the os component.
+        os, env = self.getOsAndEnv()
+        if os is None or env is None:
+            return ""
+
+        # Get the SDK from the os and env.
+        sdk = get_sdk(os, env)
+        if not sdk:
+            return ""
+
+        version = subprocess.check_output(
+            ["xcrun", "--sdk", sdk,
+             "--show-sdk-version"]).rstrip().decode('utf-8')
+        if not version:
+            return ""
+
+        # Construct the triple from its components.
+        triple = "{}-{}-{}-{}".format(vendor, os, version, env)
+
+        # Construct min version argument
+        version_min = ""
+        if env == "simulator":
+            version_min = "-m{}-simulator-version-min={}".format(os, version)
+        elif os == "macosx":
+            version_min = "-m{}-version-min={}".format(os, version)
+
+        return "ARCH_CFLAGS=\"-target {} {}\"".format(triple, version_min)
 
     def buildDsym(self,
                   sender=None,
                   architecture=None,
                   compiler=None,
                   dictionary=None,
                   testdir=None,
                   testname=None):
         """Build the binaries with dsym debug info."""
         commands = []
         commands.append(
             self.getMake(testdir, testname) + [
                 "MAKE_DSYM=YES",
+                self.getArchCFlags(architecture),
                 self.getArchSpec(architecture),
                 self.getCCSpec(compiler),
                 self.getExtraMakeArgs(),
                 self.getSDKRootSpec(),
                 self.getModuleCacheSpec(), "all",
                 self.getCmdLine(dictionary)
             ])
 
         self.runBuildCommands(commands, sender=sender)
 
         # True signifies that we can handle building dsym.
         return True

lldb/packages/Python/lldbsuite/test/dotest.py

def getVersionForSDK(sdk):
     full_path = seven.get_command_output('xcrun -sdk %s --show-sdk-path' % sdk)
     basename = os.path.basename(full_path)
     basename = os.path.splitext(basename)[0]
     basename = str.lower(basename)
     ver = basename.replace(sdk, '')
     return ver
 
 
-def setDefaultTripleForPlatform():
-    if configuration.lldb_platform_name == 'ios-simulator':
-        triple_str = 'x86_64-apple-ios%s' % (
-            getVersionForSDK('iphonesimulator'))
-        os.environ['TRIPLE'] = triple_str
-        return {'TRIPLE': triple_str}
-    return {}
-
-
 def checkCompiler():
     # Add some intervention here to sanity check that the compiler requested is sane.
     # If found not to be an executable program, we abort.
     c = configuration.compiler
     if which(c):
         return
 
     if not sys.platform.startswith("darwin"):
if configuration.lldb_platform_name:
                 print("Connected.")
             else:
                 print("error: failed to connect to remote platform using URL '%s': %s" % (
                     configuration.lldb_platform_url, err))
                 exitTestSuite(1)
         else:
             configuration.lldb_platform_url = None
 
-    platform_changes = setDefaultTripleForPlatform()
-    first = True
-    for key in platform_changes:
-        if first:
-            print("Environment variables setup for platform support:")
-            first = False
-        print("%s = %s" % (key, platform_changes[key]))
-
     if configuration.lldb_platform_working_dir:
         print("Setting remote platform working directory to '%s'..." %
               (configuration.lldb_platform_working_dir))
         error = lldb.remote_platform.MakeDirectory(
             configuration.lldb_platform_working_dir, 448)  # 448 = 0o700
         if error.Fail():
             raise Exception("making remote directory '%s': %s" % (
                 configuration.lldb_platform_working_dir, error))

lldb/packages/Python/lldbsuite/test/make/Makefile.rules

else
   FIXUP_SYNTAX_HIGHLIGHTING_IN_MY_EDITOR = '
 endif
 
 #----------------------------------------------------------------------
 # If TRIPLE is not defined try to set the ARCH, CC, CFLAGS, and more
 # from the triple alone
 #----------------------------------------------------------------------
 ARCH_CFLAGS :=
-ifneq "$(TRIPLE)" ""
-  triple_space = $(subst -, ,$(TRIPLE))
-  ARCH =$(word 1, $(triple_space))
-  TRIPLE_VENDOR =$(word 2, $(triple_space))
-  triple_os_and_version =$(shell echo $(word 3, $(triple_space)) | sed 's/\([a-z]*\)\(.*\)/\1 \2/')
-  TRIPLE_OS =$(word 1, $(triple_os_and_version))
-  TRIPLE_VERSION =$(word 2, $(triple_os_and_version))
-  TRIPLE_ENV =$(word 4, $(triple_space))
-  ifeq "$(TRIPLE_VENDOR)" "apple"
-    ifeq "$(TRIPLE_OS)" "ios"
-      ifeq "$(TRIPLE_ENV)" "simulator"
-        SDK_NAME := iphonesimulator
-      else
-      ifeq "$(TRIPLE_ENV)" "macabi"
-        SDK_NAME := macosx
-      else
-        SDK_NAME := iphoneos
-      endif
-      endif
-    endif
-    ifeq "$(TRIPLE_OS)" "tvos"
-      ifeq "$(TRIPLE_ENV)" "simulator"
-        SDK_NAME := appletvsimulator
-      else
-        SDK_NAME := appletvos
-      endif
-    endif
-    ifeq "$(TRIPLE_OS)" "watchos"
-      ifeq "$(TRIPLE_ENV)" "simulator"
-        SDK_NAME := watchsimulator
-      else
-        SDK_NAME := watchos
-      endif
-    endif
-    ifneq "$(TRIPLE_OS)" "macosx"
-      ifeq "$(TRIPLE_ENV)" ""
-        CODESIGN := codesign
-      endif
-    endif
-
-    ifeq "$(SDKROOT)" ""
-      SDKROOT := $(shell xcrun --sdk $(SDK_NAME) --show-sdk-path)
-    endif
-    ifeq "$(TRIPLE_VERSION)" ""
-      ifeq "$(SDK_NAME)" ""
-                               $(error "SDK_NAME is empty")
-      endif
-      TRIPLE_VERSION := $(shell xcrun --sdk $(SDK_NAME) --show-sdk-version)
-    endif
-    ifeq "$(TRIPLE_ENV)" "simulator"
-      ARCH_CFLAGS := -m$(TRIPLE_OS)-simulator-version-min=$(TRIPLE_VERSION)
-    else
-    ifneq "$(TRIPLE_OS)" "macosx"
-      ARCH_CFLAGS := -m$(TRIPLE_OS)-version-min=$(TRIPLE_VERSION)
-    endif
-    endif
-  endif
-  ARCH_CFLAGS += -target $(TRIPLE)
-endif
 ifeq "$(OS)" "Android"
   include $(THIS_FILE_DIR)/Android.rules
 endif
 
 #----------------------------------------------------------------------
 # If ARCH is not defined, default to x86_64.
 #----------------------------------------------------------------------
 ifeq "$(ARCH)" ""