[debugserver] This is a small cleanup patch to AVX support detection

[beanz]

Summary

The first Sandybridge iMacs with AVX support shipped in Spring 2011 with Snow Leopard as their OS. Unfortunately due to a kernel bug debugging AVX code was not really possible until 10.7.4.

The old code here checked the kernel build number to determine when to support AVX, but that code was incorrect. It verified that the kernel build number was greater than xnu-2020, which is the build of the kernel that had the fix for 10.8. The fix was also back ported to 10.7.4. Which means all publicly available OS builds 10.7.4 and later have working AVX support.

This new patch verifies that the host OS is greater than or equal to 10.7.4 by checking that the build number is greater than or equal to 11Exx.

The patch also removes the HasAVX assembly blob in favor of querying the kernel via sysctl for the hardware features.

Using sysctl is slower, however since the code is executed once and the result cached it is a better approach because it is possible for the kernel to disable AVX support on hardware that supports it, so listening to the kernel is a better approach for the debugger to take.

[beanz]

Removing some extra changes that accidentally came along for the ride in my initial upload.

[jasonmolenda]

Looks good, a few small suggestions from reading the code afresh. I'm not sure how many open source contributors we may have running macOS 10.6 or earlier (or macOS 10.7.[1-3]) but keeping this code is probably still a good idea at this point.

tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp

66 ↗

(On Diff #91640)

I see what you're doing -- this can either return "false, meaning the sysctlbyname failed or I got a zero value, or true meaning it succeeded and I got a nonzero value" but the use of a bitwise AND is going to look like a bug to any casual reader and make them re-read the code a few times before they've realized it is intended (at least it did me). It might be easier for future maintainers if it's written more like like

if (error != 0 && answer != 0)

return true;

else

return false;

and let the compiler come up with the shorter form when generating the instructions.

99 ↗

(On Diff #91640)

Would '@' be clearer here than 0x40? I had to run man ascii to figure out what this was. Maybe a comment saying that we expect the string from KERN_OSVERSION to be something like '11B156' & we're interested in the '11' bit would make it a little easier to read. (I had to run sysctl to look at the string you're parsing to follow along with the logic)

103 ↗

(On Diff #91640)

Not a big deal but '\0'.

[jasonmolenda]

Hah, I misread what the 'buffer[first_letter] & 0x40' line was doing still. Maybe use isupper() here instead of the bitmask?

[zturner]

tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp

66 ↗

(On Diff #91640)

Maybe I'm missing something, but why not just return !error && answer?

[beanz]

Updates based on feedback from Jason and Zachary.

[jasonmolenda]

looks good to me.

tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp

66 ↗

(On Diff #91640)

That would be fine too. The bitwise in the original is correct, but needlessly confusing IMO.

99 ↗

(On Diff #91640)

I misread this myself when writing the above. isupper() is what you're doing - I'd recommend using that instead of the bit mask.

[beanz]

Forgot a semi-colon...

lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj

/* Begin PBXBuildFile section */
     456F675E1AD46CE9002850C2 /* DNBArchImplX86_64.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 26CF99A21142EB7400011AAB /* DNBArchImplX86_64.cpp */; };
     456F675F1AD46CE9002850C2 /* DNBArchImplI386.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 26C637EA0C71334A0024798E /* DNBArchImplI386.cpp */; };
     456F67601AD46CE9002850C2 /* DNBArchImpl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2675D4220CCEB705000F49AF /* DNBArchImpl.cpp */; };
     456F67611AD46CE9002850C2 /* DNBArchImpl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 26C637FB0C71334A0024798E /* DNBArchImpl.cpp */; };
     456F67621AD46CE9002850C2 /* CFString.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2695DD9B0D3EC160007E4CA2 /* CFString.cpp */; };
     456F67641AD46CE9002850C2 /* CFBundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2695DD910D3EBFF6007E4CA2 /* CFBundle.cpp */; };
     456F67651AD46CE9002850C2 /* PseudoTerminal.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AF67ABFF0D34604D0022D128 /* PseudoTerminal.cpp */; };
     456F67671AD46CE9002850C2 /* DNBArch.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 264D5D571293835600ED4C01 /* DNBArch.cpp */; };
-    456F67681AD46CE9002850C2 /* HasAVX.s in Sources */ = {isa = PBXBuildFile; fileRef = 4971AE7113D10F4F00649E37 /* HasAVX.s */; };
     456F67691AD46CE9002850C2 /* DNBArchImplARM64.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 266B5ECF1460A68200E43F0A /* DNBArchImplARM64.cpp */; };
     456F676B1AD46CE9002850C2 /* CoreFoundation.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 26ACA3340D3E956300A2120B /* CoreFoundation.framework */; settings = {ATTRIBUTES = (Required, ); }; };
-    4971AE7213D10F4F00649E37 /* HasAVX.s in Sources */ = {isa = PBXBuildFile; fileRef = 4971AE7113D10F4F00649E37 /* HasAVX.s */; };
     49D404621E39260F00570CDC /* Foundation.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 49D404611E39260F00570CDC /* Foundation.framework */; };
     AF48558C1D75126800D19C07 /* StdStringExtractor.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AF48558B1D75126800D19C07 /* StdStringExtractor.cpp */; };
     AF48558D1D75127500D19C07 /* StdStringExtractor.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AF48558B1D75126800D19C07 /* StdStringExtractor.cpp */; };
     AFA3FCA11E39984900218D5E /* Foundation.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 49D404611E39260F00570CDC /* Foundation.framework */; };
     AFEC3364194A8B0B00FF05C6 /* Genealogy.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AFEC3363194A8B0B00FF05C6 /* Genealogy.cpp */; };
 /* End PBXBuildFile section */
 
 /* Begin PBXFileReference section */
/* Begin PBXFileReference section */
     26C638020C71334A0024798E /* SysSignal.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = SysSignal.h; sourceTree = "<group>"; };
     26C638050C71334A0024798E /* TTYState.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = TTYState.cpp; sourceTree = "<group>"; };
     26C638060C71334A0024798E /* TTYState.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = TTYState.h; sourceTree = "<group>"; };
     26CE0594115C31C20022F371 /* debugserver */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = debugserver; sourceTree = BUILT_PRODUCTS_DIR; };
     26CF99A21142EB7400011AAB /* DNBArchImplX86_64.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DNBArchImplX86_64.cpp; sourceTree = "<group>"; };
     26CF99A31142EB7400011AAB /* DNBArchImplX86_64.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DNBArchImplX86_64.h; sourceTree = "<group>"; };
     26E6B9DA0D1329010037ECDD /* RNBDefs.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RNBDefs.h; sourceTree = "<group>"; };
     456F67721AD46CE9002850C2 /* debugserver-nonui */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = "debugserver-nonui"; sourceTree = BUILT_PRODUCTS_DIR; };
-    4971AE7013D10F4F00649E37 /* HasAVX.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = HasAVX.h; sourceTree = "<group>"; };
-    4971AE7113D10F4F00649E37 /* HasAVX.s */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.asm; path = HasAVX.s; sourceTree = "<group>"; };
     49D404611E39260F00570CDC /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = System/Library/Frameworks/Foundation.framework; sourceTree = SDKROOT; };
     49F530111331519C008956F6 /* MachRegisterStatesI386.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MachRegisterStatesI386.h; sourceTree = "<group>"; };
     49F5301213316D7F008956F6 /* MachRegisterStatesX86_64.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MachRegisterStatesX86_64.h; sourceTree = "<group>"; };
     9457ECF61419864100DFE7D8 /* stack_logging.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = stack_logging.h; sourceTree = "<group>"; };
     AF0934BA18E12B92005A11FD /* Genealogy.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = Genealogy.h; sourceTree = "<group>"; };
     AF0934BB18E12B92005A11FD /* GenealogySPI.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = GenealogySPI.h; sourceTree = "<group>"; };
     AF48558B1D75126800D19C07 /* StdStringExtractor.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = StdStringExtractor.cpp; sourceTree = "<group>"; };
     AF61C60418F75ABC00B48D9D /* debugserver-macosx-entitlements.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = "debugserver-macosx-entitlements.plist"; sourceTree = "<group>"; };
26C637E60C71334A0024798E /* MacOSX */ = {
         2695DD9B0D3EC160007E4CA2 /* CFString.cpp */,
         26C637E70C71334A0024798E /* CFUtils.h */,
         2307CCCC1D4A5DAE0016ABC0 /* CMakeLists.txt */,
         2675D41C0CCEB6CF000F49AF /* arm */,
         266B5ECE1460A68200E43F0A /* arm64 */,
         26C637E90C71334A0024798E /* i386 */,
         26C637FA0C71334A0024798E /* ppc */,
         26CF99A11142EB7400011AAB /* x86_64 */,
-        4971AE7013D10F4F00649E37 /* HasAVX.h */,
-        4971AE7113D10F4F00649E37 /* HasAVX.s */,
         26C637E80C71334A0024798E /* dbgnub-mig.defs */,
         AFEC3363194A8B0B00FF05C6 /* Genealogy.cpp */,
         AF0934BA18E12B92005A11FD /* Genealogy.h */,
         AF0934BB18E12B92005A11FD /* GenealogySPI.h */,
         26C637EF0C71334A0024798E /* MachException.h */,
         26C637EE0C71334A0024798E /* MachException.cpp */,
         26C637F10C71334A0024798E /* MachProcess.h */,
         26C637F00C71334A0024798E /* MachProcess.mm */,
26CE0591115C31C20022F371 /* Sources */ = {
         26CE05C0115C364F0022F371 /* DNBArchImplI386.cpp in Sources */,
         26CE05C1115C36510022F371 /* DNBArchImpl.cpp in Sources */,
         26CE05C2115C36550022F371 /* DNBArchImpl.cpp in Sources */,
         26CE05C5115C36590022F371 /* CFBundle.cpp in Sources */,
         26CE05C3115C36580022F371 /* CFString.cpp in Sources */,
         23562ED91D342B0000AB2BD4 /* LogMessage.cpp in Sources */,
         26CE05F1115C387C0022F371 /* PseudoTerminal.cpp in Sources */,
         264D5D581293835600ED4C01 /* DNBArch.cpp in Sources */,
-        4971AE7213D10F4F00649E37 /* HasAVX.s in Sources */,
         237821B01D4917D20028B7A1 /* LogFilterExactMatch.cpp in Sources */,
         266B5ED11460A68200E43F0A /* DNBArchImplARM64.cpp in Sources */,
       );
       runOnlyForDeploymentPostprocessing = 0;
     };
     456F67451AD46CE9002850C2 /* Sources */ = {
       isa = PBXSourcesBuildPhase;
       buildActionMask = 2147483647;
456F67451AD46CE9002850C2 /* Sources */ = {
         23043C9E1D35DBFA00FC25CA /* StringConvert.cpp in Sources */,
         456F67611AD46CE9002850C2 /* DNBArchImpl.cpp in Sources */,
         456F67621AD46CE9002850C2 /* CFString.cpp in Sources */,
         23AC04CB1D2F42250072351D /* LogFilterChain.cpp in Sources */,
         AF48558D1D75127500D19C07 /* StdStringExtractor.cpp in Sources */,
         456F67641AD46CE9002850C2 /* CFBundle.cpp in Sources */,
         456F67651AD46CE9002850C2 /* PseudoTerminal.cpp in Sources */,
         456F67671AD46CE9002850C2 /* DNBArch.cpp in Sources */,
-        456F67681AD46CE9002850C2 /* HasAVX.s in Sources */,
         23AC04D01D2F58AF0072351D /* LogFilterRegex.cpp in Sources */,
         456F67691AD46CE9002850C2 /* DNBArchImplARM64.cpp in Sources */,
       );
       runOnlyForDeploymentPostprocessing = 0;
     };
 /* End PBXSourcesBuildPhase section */
 
 /* Begin XCBuildConfiguration section */

lldb/trunk/tools/debugserver/source/MacOSX/CMakeLists.txt

add_custom_command(OUTPUT ${DEBUGSERVER_VERS_GENERATED_FILE}
   COMMAND ${LLDB_SOURCE_DIR}/scripts/generate-vers.pl
           ${LLDB_SOURCE_DIR}/lldb.xcodeproj/project.pbxproj debugserver
           > ${DEBUGSERVER_VERS_GENERATED_FILE}
   DEPENDS ${LLDB_SOURCE_DIR}/scripts/generate-vers.pl
           ${LLDB_SOURCE_DIR}/lldb.xcodeproj/project.pbxproj
   )
 
 add_lldb_tool(debugserver INCLUDE_IN_FRAMEWORK
-  HasAVX.s
   CFBundle.cpp
   CFString.cpp
   Genealogy.cpp
   MachException.cpp
   MachProcess.mm
   MachTask.mm
   MachThread.cpp
   MachThreadList.cpp

lldb/trunk/tools/debugserver/source/MacOSX/HasAVX.h

-//===-- HasAVX.h ------------------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef HasAVX_h
-#define HasAVX_h
-
-#if defined(__i386__) || defined(__x86_64__)
-
-#ifdef __cplusplus
-extern "C" {
-#endif
-
-int HasAVX();
-
-#ifdef __cplusplus
-}
-#endif
-
-#endif
-
-#endif

lldb/trunk/tools/debugserver/source/MacOSX/HasAVX.s

-//===-- HasAVX.s ---------------------------------------*- x86 Assembly -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#if defined (__i386__) || defined (__x86_64__)
-
-.globl _HasAVX
-
-_HasAVX:
-#if defined (__x86_64__)
-    pushq %rbp
-    movq %rsp, %rbp
-    pushq %rbx
-#else
-    pushl %ebp
-    movl %esp, %ebp
-    pushl %ebx
-#endif
-    mov $1, %eax
-    cpuid                                                                       // clobbers ebx
-    and $0x018000000, %ecx
-    cmp $0x018000000, %ecx
-    jne not_supported
-    mov $0, %ecx
-.byte 0x0f, 0x01, 0xd0    // xgetbv, for those assemblers that don't know it
-    and $0x06, %eax
-    cmp $0x06, %eax
-    jne not_supported
-    mov $1, %eax
-    jmp done
-not_supported:
-    mov $0, %eax
-done:
-#if defined (__x86_64__)
-    popq %rbx
-    movq %rbp, %rsp
-    popq %rbp
-#else
-    popl %ebx
-    movl %ebp, %esp
-    popl %ebp
-#endif
-    ret                                                                         // return
-
-#endif

lldb/trunk/tools/debugserver/source/MacOSX/i386/DNBArchImplI386.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef __DNBArchImplI386_h__
 #define __DNBArchImplI386_h__
 
 #if defined(__i386__) || defined(__x86_64__)
 
-#include "../HasAVX.h"
 #include "DNBArch.h"
 #include "MachRegisterStatesI386.h"
 
 #include <map>
 
 class MachThread;
 
 class DNBArchImplI386 : public DNBArchProtocol {

lldb/trunk/tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp

 //===----------------------------------------------------------------------===//
 
 #if defined(__i386__) || defined(__x86_64__)
 
 #include <sys/cdefs.h>
 #include <sys/sysctl.h>
 #include <sys/types.h>
 
-#include "../HasAVX.h"
 #include "DNBLog.h"
 #include "MacOSX/x86_64/DNBArchImplX86_64.h"
 #include "MachProcess.h"
 #include "MachThread.h"
 #include <mach/mach.h>
 #include <stdlib.h>
 
 #if defined(LLDB_DEBUGSERVER_RELEASE) || defined(LLDB_DEBUGSERVER_DEBUG)

 
 #define DEBUG_FPU_REGS (DebugFPURegs())
 #define FORCE_AVX_REGS (ForceAVXRegs())
 #else
 #define DEBUG_FPU_REGS (0)
 #define FORCE_AVX_REGS (0)
 #endif
 
-extern "C" bool CPUHasAVX() {
-  enum AVXPresence { eAVXUnknown = -1, eAVXNotPresent = 0, eAVXPresent = 1 };
+bool DetectHardwareFeature(const char *feature) {
+  int answer = 0;
+  size_t answer_size = sizeof(answer);
+  int error = ::sysctlbyname(feature, &answer, &answer_size, NULL, 0);
+  return error != 0 && answer != 0;
+}
+
 
+bool LogAVXAndReturn(AVXPresence has_avx, int err, const char * os_ver) {
+  DNBLogThreadedIf(LOG_THREAD,
+                   "CPUHasAVX(): g_has_avx = %i (err = %i, os_ver = %s)",
+                   has_avx, err, os_ver);
+  return (has_avx == eAVXPresent);
+}
+
+extern "C" bool CPUHasAVX() {
   static AVXPresence g_has_avx = eAVXUnknown;
-  if (g_has_avx == eAVXUnknown) {
-    g_has_avx = eAVXNotPresent;
+  if (g_has_avx != eAVXUnknown)
+    return LogAVXAndReturn(g_has_avx, 0, "");
+
 
-    // Only xnu-2020 or later has AVX support, any versions before
-    // this have a busted thread_get_state RPC where it would truncate
-    // the thread state buffer (<rdar://problem/10122874>). So we need to
-    // verify the kernel version number manually or disable AVX support.
-    int mib[2];
-    char buffer[1024];
-    size_t length = sizeof(buffer);
-    uint64_t xnu_version = 0;
-    mib[0] = CTL_KERN;
-    mib[1] = KERN_VERSION;
-    int err = ::sysctl(mib, 2, &buffer, &length, NULL, 0);
-    if (err == 0) {
-      const char *xnu = strstr(buffer, "xnu-");
-      if (xnu) {
-        const char *xnu_version_cstr = xnu + 4;
-        xnu_version = strtoull(xnu_version_cstr, NULL, 0);
-        if (xnu_version >= 2020 && xnu_version != ULLONG_MAX) {
-          if (::HasAVX()) {
-            g_has_avx = eAVXPresent;
-          }
-        }
-      }
-    }
-    DNBLogThreadedIf(LOG_THREAD, "CPUHasAVX(): g_has_avx = %i (err = %i, errno "
-                                 "= %i, xnu_version = %llu)",
-                     g_has_avx, err, errno, xnu_version);
-  }
+  // OS X 10.7.3 and earlier have a bug in thread_get_state that truncated the
+  // size of the return. To work around this we have to disable AVX debugging
+  // on hosts prior to 10.7.3 (<rdar://problem/10122874>).
+  mib[1] = KERN_OSVERSION;
+
+  // KERN_OSVERSION returns the build number which is a number signifying the
+  // major version, a capitol letter signifying the minor version, and numbers
+  // signifying the build (ex: on 10.12.3, the returned value is 16D32).
+  if (err != 0)
+    return LogAVXAndReturn(g_has_avx, err, "");
+
+  size_t first_letter = 0;
+  for (; first_letter < length; ++first_letter) {
+    // This is looking for the first uppercase letter
+    if (isupper(buffer[first_letter]))
+      break;
+  }
+  char letter = buffer[first_letter];
+  buffer[first_letter] = '\0';
+  auto major_ver = strtoull(buffer, NULL, 0);
+  buffer[first_letter] = letter;
+
+  // In this check we're looking to see that our major and minor version numer
+  // was >= 11E, which is the 10.7.4 release.
+  if (major_ver < 11 || (major_ver == 11 && letter < 'E'))
+    return LogAVXAndReturn(g_has_avx, err, buffer);
+  if (DetectHardwareFeature("hw.optional.avx1_0"))
 
-  return (g_has_avx == eAVXPresent);
+  return LogAVXAndReturn(g_has_avx, err, buffer);
 }
 
 uint64_t DNBArchImplX86_64::GetPC(uint64_t failValue) {
   // Get program counter
   if (GetGPRState(false) == KERN_SUCCESS)
     return m_state.context.gpr.__rip;
   return failValue;
 }