i#5725: Set consistent x86 vendor decoding default (#6438)

If we initialize the proc_get_vendor() to VENDOR_UNKNOWN we get contradictory decoding results for x86 opcodes that vary between VENDOR_AMD and VENDOR_INTEL (because some of our decoding code checks one and some checks the other), so we pick one (VENDOR_INTEL) for a stable self-consistent default. We set this statically and we override the underlying hardware in proc_init(). The user can use proc_set_vendor() (after an explicit init) to override if desired. Fixes #5725
DynamoRIO · Nov 10, 2023 · 8bc660c · 8bc660c
1 parent 0192e25
commit 8bc660c
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 16 deletions.
diff --git a/core/arch/proc_shared.c b/core/arch/proc_shared.c
@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2013-2022 Google, Inc.  All rights reserved.
+ * Copyright (c) 2013-2023 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2008 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -63,25 +63,36 @@
  */
 size_t cache_line_size = 32;
 static ptr_uint_t mask; /* bits that should be 0 to be cache-line-aligned */
-cpu_info_t cpu_info = { VENDOR_UNKNOWN,
+cpu_info_t cpu_info = {
+#ifdef X86
+    /* If we initialize to VENDOR_UNKNOWN we get contradictory decoding results
+     * for opcodes that vary between VENDOR_AMD and VENDOR_INTEL (because some
+     * of our decoding code checks one and some checks the other) so we pick one
+     * for a stable self-consistent default.
+     */
+    VENDOR_INTEL,
+#else
+    VENDOR_UNKNOWN,
+#endif
 #ifdef AARCHXX
-                        0,
-                        0,
+    0,
+    0,
 #endif
-                        0,
-                        0,
-                        0,
-                        0,
-                        CACHE_SIZE_UNKNOWN,
-                        CACHE_SIZE_UNKNOWN,
-                        CACHE_SIZE_UNKNOWN,
+    0,
+    0,
+    0,
+    0,
+    CACHE_SIZE_UNKNOWN,
+    CACHE_SIZE_UNKNOWN,
+    CACHE_SIZE_UNKNOWN,
 #if defined(RISCV64)
-                        /* FIXME i#3544: Not implemented */
-                        { 0 },
+    /* FIXME i#3544: Not implemented */
+    { 0 },
 #else
-                        { 0, 0, 0, 0 },
+    { 0, 0, 0, 0 },
 #endif
-                        { 0x6e6b6e75, 0x006e776f } };
+    { 0x6e6b6e75, 0x006e776f }
+};
 
 void
 proc_set_cache_size(uint val, uint *dst)

diff --git a/core/arch/x86/proc.c b/core/arch/x86/proc.c
@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2013-2022 Google, Inc.  All rights reserved.
+ * Copyright (c) 2013-2023 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2008 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -333,6 +333,21 @@ get_processor_specific_info(void)
         our_cpuid((int *)&cpu_info.brand_string[4], 0x80000003, 0);
         our_cpuid((int *)&cpu_info.brand_string[8], 0x80000004, 0);
     }
+
+    if (standalone_library) {
+        /* For separate decoding, keep a base default and do not target the
+         * underlying processor (xref i#431, i#6420, i#5725).
+         * The user can use proc_set_vendor() to override.
+         * This affects various decoding corner cases and is important to set.
+         * We leave all the other cache info, etc. pointing to the current hardware.
+         * We set this at the end here to avoid errors in getting other
+         * cpuid values with the wrong magic parameters.
+         * XXX: This is still potentially fragile; should the decoder have
+         * a separate vendor value?
+         */
+        LOG(GLOBAL, LOG_TOP, 1, "For standalone decoding, assuming Intel target.\n");
+        cpu_info.vendor = VENDOR_INTEL;
+    }
 }
 
 /* arch specific proc info */