[PATCH] gdb, btrace: fix family and model computation

[PATCH] gdb, btrace: fix family and model computation

[Markus Metzger]

In gdb/nat/linux-btrace.c:btrace_this_cpu() we initialize the cpu
structure given to the libipt btrace decoder.

We only consider the extended model field for family 0x6 and forget about
family 0xf and we don't consider the extended family field.  Fix it.
---
 gdb/nat/linux-btrace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 4911630ba5c..a951f3b56aa 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -84,9 +84,11 @@ btrace_this_cpu (void)
 	      cpu.vendor = CV_INTEL;
 
 	      cpu.family = (cpuid >> 8) & 0xf;
-	      cpu.model = (cpuid >> 4) & 0xf;
+	      if (cpu.family == 0xf)
+		cpu.family += (cpuid >> 20) & 0xff;
 
-	      if (cpu.family == 0x6)
+	      cpu.model = (cpuid >> 4) & 0xf;
+	      if ((cpu.family == 0x6) || ((cpu.family & 0xf) == 0xf))
 		cpu.model += (cpuid >> 12) & 0xf0;
 	    }
 	}
-- 
2.37.3

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdb, btrace: fix family and model computation

[Tom Tromey]

>>>>> "Markus" == Markus Metzger via Gdb-patches <gdb-patches@sourceware.org> writes:

Markus> In gdb/nat/linux-btrace.c:btrace_this_cpu() we initialize the cpu
Markus> structure given to the libipt btrace decoder.

Markus> We only consider the extended model field for family 0x6 and forget about
Markus> family 0xf and we don't consider the extended family field.  Fix it.

You should probably just self-approve this.

Markus>  	      cpu.family = (cpuid >> 8) & 0xf;
Markus> -	      cpu.model = (cpuid >> 4) & 0xf;
Markus> +	      if (cpu.family == 0xf)
Markus> +		cpu.family += (cpuid >> 20) & 0xff;
 
Markus> -	      if (cpu.family == 0x6)
Markus> +	      cpu.model = (cpuid >> 4) & 0xf;
Markus> +	      if ((cpu.family == 0x6) || ((cpu.family & 0xf) == 0xf))
Markus>  		cpu.model += (cpuid >> 12) & 0xf0;

I wonder if these magic numbers have #defines anywhere we could use.

Tom

RE: [PATCH] gdb, btrace: fix family and model computation

[Metzger, Markus T]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

Hello Tom,

>Markus>  	      cpu.family = (cpuid >> 8) & 0xf;
>Markus> -	      cpu.model = (cpuid >> 4) & 0xf;
>Markus> +	      if (cpu.family == 0xf)
>Markus> +		cpu.family += (cpuid >> 20) & 0xff;
>
>Markus> -	      if (cpu.family == 0x6)
>Markus> +	      cpu.model = (cpuid >> 4) & 0xf;
>Markus> +	      if ((cpu.family == 0x6) || ((cpu.family & 0xf) == 0xf))
>Markus>  		cpu.model += (cpuid >> 12) & 0xf0;
>
>I wonder if these magic numbers have #defines anywhere we could use.

In fact,

    bb368aad297 gprofng: a new GNU profiler

added the same functionality, so now we have two versions of it.  I tried
to unify them (attached) but wasn't able to pass gprofng tests on my box.
They already fail without my changes.

I could replace this patch with the attached two.  What do you think?

regards,
markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[-- Attachment #2: 0001-make-gprofng-cpu-identification-available-to-others.patch --]
[-- Type: application/octet-stream, Size: 2514 bytes --]

From 606a3f58ae40d46d901703b64754a4789e9f2ae6 Mon Sep 17 00:00:00 2001
From: Markus Metzger <markus.t.metzger@intel.com>
Date: Mon, 24 Oct 2022 05:43:08 +0200
Subject: [PATCH 1/2] make gprofng cpu identification available to others

---
 gprofng/common/hwcdrv.c                      |  2 +-
 gprofng/common/cpuid.c => include/cpuident.h | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)
 rename gprofng/common/cpuid.c => include/cpuident.h (96%)

diff --git a/gprofng/common/hwcdrv.c b/gprofng/common/hwcdrv.c
index 03c3a71c8e2..115638aa623 100644
--- a/gprofng/common/hwcdrv.c
+++ b/gprofng/common/hwcdrv.c
@@ -32,7 +32,7 @@
 /* macros */
 #define IS_GLOBAL /* Mark global symbols */
 
-#include "cpuid.c" /* ftns for identifying a chip */
+#include "cpuident.h" /* ftns for identifying a chip */
 
 static hdrv_pcbe_api_t hdrv_pcbe_core_api;
 static hdrv_pcbe_api_t hdrv_pcbe_opteron_api;
diff --git a/gprofng/common/cpuid.c b/include/cpuident.h
similarity index 96%
rename from gprofng/common/cpuid.c
rename to include/cpuident.h
index 211e09aa8ac..7ab6fc1f2b1 100644
--- a/gprofng/common/cpuid.c
+++ b/include/cpuident.h
@@ -18,6 +18,11 @@
    Foundation, 51 Franklin Street - Fifth Floor, Boston,
    MA 02110-1301, USA.  */
 
+#ifndef _CPUIDENT_H
+#define _CPUIDENT_H
+
+#include <stdint.h>
+
 #if defined(__i386__) || defined(__x86_64)
 #include <cpuid.h>  /* GCC-provided */
 #elif defined(__aarch64__)
@@ -85,7 +90,7 @@ typedef struct
 
 
 #if defined(__i386__) || defined(__x86_64)
-static uint_t
+static unsigned int
 cpuid_vendorstr_to_vendorcode (char *vendorstr)
 {
   if (strcmp (vendorstr, X86_VENDORSTR_Intel) == 0)
@@ -101,8 +106,10 @@ my_cpuid (unsigned int op, cpuid_regs_t *regs)
 {
   regs->eax = regs->ebx = regs->ecx = regs->edx = 0;
   int ret = __get_cpuid (op, &regs->eax, &regs->ebx, &regs->ecx, &regs->edx);
+#ifdef DBG_LT1
   TprintfT (DBG_LT1, "my_cpuid: __get_cpuid(0x%x, 0x%x, 0x%x, 0x%x, 0x%x) returns %d\n",
 	    op, regs->eax, regs->ebx, regs->ecx, regs->edx, ret);
+#endif
   return ret;
 }
 #endif
@@ -184,20 +191,22 @@ get_cpuid_info ()
   return cpi;
 }
 
-static inline uint_t
+static inline unsigned int
 cpuid_getvendor ()
 {
   return get_cpuid_info ()->cpi_vendor;
 }
 
-static inline uint_t
+static inline unsigned int
 cpuid_getfamily ()
 {
   return get_cpuid_info ()->cpi_family;
 }
 
-static inline uint_t
+static inline unsigned int
 cpuid_getmodel ()
 {
   return get_cpuid_info ()->cpi_model;
 }
+
+#endif /* _CPUIDENT_H */
-- 
2.37.3


[-- Attachment #3: 0002-use-cpuident.h-to-implement-btrace_this_cpu.patch --]
[-- Type: application/octet-stream, Size: 1943 bytes --]

From 82f8f0a44b22d494322ce592eccf0505d0abf608 Mon Sep 17 00:00:00 2001
From: Markus Metzger <markus.t.metzger@intel.com>
Date: Mon, 24 Oct 2022 07:39:42 +0200
Subject: [PATCH 2/2] use cpuident.h to implement btrace_this_cpu

---
 gdb/nat/linux-btrace.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 4911630ba5c..108a4a10ecb 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -23,7 +23,7 @@
 #include "linux-btrace.h"
 #include "gdbsupport/common-regcache.h"
 #include "gdbsupport/gdb_wait.h"
-#include "x86-cpuid.h"
+#include "include/cpuident.h"
 #include "gdbsupport/filestuff.h"
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/scoped_mmap.h"
@@ -65,36 +65,23 @@ static struct btrace_cpu
 btrace_this_cpu (void)
 {
   struct btrace_cpu cpu;
-  unsigned int eax, ebx, ecx, edx;
-  int ok;
-
   memset (&cpu, 0, sizeof (cpu));
 
-  ok = x86_cpuid (0, &eax, &ebx, &ecx, &edx);
-  if (ok != 0)
+  unsigned int vendor = cpuid_getvendor ();
+  switch (vendor)
     {
-      if (ebx == signature_INTEL_ebx && ecx == signature_INTEL_ecx
-	  && edx == signature_INTEL_edx)
-	{
-	  unsigned int cpuid, ignore;
-
-	  ok = x86_cpuid (1, &cpuid, &ignore, &ignore, &ignore);
-	  if (ok != 0)
-	    {
-	      cpu.vendor = CV_INTEL;
-
-	      cpu.family = (cpuid >> 8) & 0xf;
-	      cpu.model = (cpuid >> 4) & 0xf;
+    case X86_VENDOR_Intel:
+      cpu.vendor = CV_INTEL;
+      break;
 
-	      if (cpu.family == 0x6)
-		cpu.model += (cpuid >> 12) & 0xf0;
-	    }
-	}
-      else if (ebx == signature_AMD_ebx && ecx == signature_AMD_ecx
-	       && edx == signature_AMD_edx)
-	cpu.vendor = CV_AMD;
+    case X86_VENDOR_AMD:
+      cpu.vendor = CV_AMD;
+      break;
     }
 
+  cpu.family = (unsigned short) cpuid_getfamily ();
+  cpu.model = (unsigned char) cpuid_getmodel ();
+
   return cpu;
 }
 
-- 
2.37.3

Re: [PATCH] gdb, btrace: fix family and model computation

[Tom Tromey]

> In fact,
>     bb368aad297 gprofng: a new GNU profiler
> added the same functionality, so now we have two versions of it.  I tried
> to unify them (attached) but wasn't able to pass gprofng tests on my box.
> They already fail without my changes.

> I could replace this patch with the attached two.  What do you think?

I wouldn't hold up your patch for this, but if you wanted to coordinate
with the gprofng folks and get this new patch in, it does seem better to
consolidate the code.

Tom

RE: [PATCH] gdb, btrace: fix family and model computation

[Metzger, Markus T]

Thanks, Tom,

>> In fact,
>>     bb368aad297 gprofng: a new GNU profiler
>> added the same functionality, so now we have two versions of it.  I tried
>> to unify them (attached) but wasn't able to pass gprofng tests on my box.
>> They already fail without my changes.
>
>> I could replace this patch with the attached two.  What do you think?
>
>I wouldn't hold up your patch for this, but if you wanted to coordinate
>with the gprofng folks and get this new patch in, it does seem better to
>consolidate the code.

I'll push this patch, then, so we get the fix.  Then, I'll send out the other
two to consolidate if there is interest from gprofng.

regards,
markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928