i#2924 VS2017: Switch Appveyor to VS2017 (#3999)

Changes Appveyor to use VS2017 instead of VS2013.  We no longer officially
maintain VS2013 support.

Includes quite a few small fixes to get all builds and as many tests as possible passing on VS2017 and Win10-1607 on Appveyor:
+ Add VS2017 support to x64 LIB env var swapping
+ Add pseudo-dll mapping for Ext-MS-Win-NtUser-* and API-MS-Win-RtCore-NtUser-*
+ Increase drmarker cmd size
+ Avoid crashing in debug-only code if PEB.KernelCallbackTable is NULL
+ Expand win32.fpe allowed output to include 'inf' for DBL_MAX+DBL_MAX
+ Pass /arch:IA32 /MD to tool.cpuid.exe to keep drcpusim tests passing w/o any new opcodes
+#4056 drsyms: allow either order of args in output; allow an extra leading underscore; allow a too-long name_available_size; give up on types matching given all the weirdness in recent dbghelps
+ client.fibers fixes for VS2017:
  + Delete the main fiber on exit, since the deletion callback is not called natively
    with VS2017 otherwise.
  + Increase the max FLS slot limit to match pre-Win10.
  + Allow either order of deletion callbacks.
+ client.winxfer test:
  + Allow variations in event counts
  + Remove #4053 assert on un-suspendable un-scheduled attach thread
    since it happens on win10 at exit and is not fatal
+ Fix unit_tests problems:
  + Set -msgbox_mask to 0 to avoid hangs
  + Skip static TLS from #4030 since it leads to asserts in the
    LoadLibrary unit tests b/c we don't support TLS in later-loaded libs
  + Fix exit-time crashes by always swapping the PEB and swapping back at
    the end of the drwinapi tests.
* Add an AUTOMATED_TESTING cmake and cpp define, set it for arg_travis
in runsuite.cmake, and use it to make -msgbox_mask 0 by default, as a
failsafe to avoid hangs in CI.
+ Use a better dbghelp:
  + Update search paths: use ProgFiles(x86) instead of PROGFILES.
  + Have drsyms make a local copy in ext/lib64/debug.
+ i#4057: Copy CTest xml files manually
  CMake 3.14 dropped support for copying xml results locally.
  We solve that by manually copying them ourselves.
+ Fix VPS build warnings with VS2017 and from #4050 and #4053
+ Switch to regular FindMFC; fix DRstats build warnings on VS2017
+ Disable MSVC popups in drcachesim's launcher to avoid hanging Appveyor
+ Ignore the remaining VS2017 test failures, leaving fixing those to #4058 

Issue: #4030, #4053, #4001, #4058
Fixes #2924
Fixes #4056
Fixes #4057

.appveyor.yml

@@ -1,5 +1,5 @@
 # **********************************************************
-# Copyright (c) 2017-2018 Google, Inc.  All rights reserved.
+# Copyright (c) 2017-2019 Google, Inc.  All rights reserved.
 # **********************************************************
 
 # Redistribution and use in source and binary forms, with or without
@@ -53,16 +53,15 @@ branches:
 
 platform: x64
 
-image: Visual Studio 2013
+image: Visual Studio 2017
 
 build:
   verbosity: detailed
 
 # i#2406: Appveyor's global serialization makes it painful to use more than
-# one configuration.  We no longer build packages with VS2010 and are
-# dropping official support for it, meaning we only need to test VS2013 here.
+# one configuration.  We thus officially only support a single VS version.
 configuration:
-  - 2013
+  - 2017
 
 install:
   ##################################################
@@ -92,7 +91,7 @@ install:
   # XXX i#2145: point at Qt5 for testing drgui build.
 
 before_build:
-  - if "%configuration%"=="2013" call "c:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat" x86
+  - if "%configuration%"=="2017" call "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Auxiliary\Build\vcvars32.bat"
   - cd c:\projects\dynamorio
 
 build_script:

CMakeLists.txt

@@ -1366,14 +1366,18 @@ endmacro(DR_export_target)
 # We locate it here for use in both subdirs.
 if (X64)
   set(PROGFILES "$ENV{PROGRAMW6432}") # cmake is 32-bit
-  set(PROGFILES32 "$ENV{PROGRAMFILES}")
+  set(PROGFILES32 "$ENV{ProgramFiles\(x86\)}")
+  if ("${PROGFILES32}" STREQUAL "")
+    set(PROGFILES32 "$ENV{PROGRAMFILES}")
+  endif ()
   set(ARCH_SFX "x64")
 else (X64)
   set(PROGFILES "$ENV{PROGRAMFILES}")
   set(PROGFILES32 "$ENV{PROGRAMFILES}")
   set(ARCH_SFX "x86")
 endif (X64)
 set(dbghelp_paths
+  "${PROGFILES32}/Microsoft Visual Studio/*/Professional/Common7/IDE/Remote Debugger/${ARCH_SFX}/dbghelp.dll"
   "${PROGFILES}/Microsoft Visual Studio */Common7/IDE/Remote Debugger/${ARCH_SFX}/dbghelp.dll"
   "${PROGFILES32}/Windows Kits/*/Debuggers/${ARCH_SFX}/dbghelp.dll")
 if (X64)
@@ -1471,6 +1475,8 @@ endif (NOT CLIENT_INTERFACE)
 # An alternative is to keep them in separate projects and only build
 # when we run them via --build-and-test.
 option(BUILD_TESTS "build tests" OFF)
+# This is to disable the default -msgbox_mask.
+option(AUTOMATED_TESTING "build for automated testing" OFF)
 if (BUILD_TESTS)
   # Tests require tools
   set(BUILD_TOOLS ON)
@@ -1502,7 +1508,11 @@ endif (BUILD_TOOLS)
 if (BUILD_DRSTATS)
   # i#1376: we have our own variant of the standard FindMFC.cmake
   # so we can pass -D_UNICODE, as MBCS MFC support is not part of VS2013.
-  find_package(MFCUnicode)
+  # Update: To get things going with VS2017 I'm switching to the regular one
+  # to get some version of DRstats building.  We don't really use it much
+  # anymore; only if someone notices any unicode issues is it worth more time
+  # updating make/modules/FindMFCUnicode.cmake.
+  find_package(MFC)
   if (NOT MFC_FOUND)
     message(STATUS "MFC not found: disabling DRstats")
     set(BUILD_DRSTATS OFF)

CTestConfig.cmake

@@ -1,4 +1,5 @@
 # **********************************************************
+# Copyright (c) 2020 Google, Inc.    All rights reserved.
 # Copyright (c) 2009-2010 VMware, Inc.    All rights reserved.
 # **********************************************************
 
@@ -32,25 +33,16 @@
 set(CTEST_PROJECT_NAME "DynamoRIO")
 
 if (SUBMIT_LOCAL)
-  set(CTEST_DROP_METHOD "scp")
-  if (WIN32)
-    # Cygwin scp won't take : later in path (and interprets drive as host like
-    # unix scp would) so we use a batch file.
-    # Note that CTEST_SCP_COMMAND must be a single executable so we can't
-    # use "cmake -E copy".  We could use cygwin "cp" but that requires cygwin.
-    set(CTEST_SCP_COMMAND "${CTEST_SCRIPT_DIRECTORY}/../make/copy.bat")
-  else (WIN32)
-    find_program(CTEST_SCP_COMMAND scp DOC "scp command for local copy of results")
-  endif (WIN32)
+  # There is no longer support for "cp" or "scp methods in cmake 3.14+: there is no
+  # way to copy locally using ctest_submit().  We copy ourselves manually.
+  set(CTEST_SUBMIT_URL "none")
+  set(CTEST_DROP_METHOD "none")
   set(CTEST_TRIGGER_SITE "")
   set(CTEST_DROP_SITE_USER "")
-  # CTest does "scp file ${CTEST_DROP_SITE}:${CTEST_DROP_LOCATION}" so for
-  # local copy w/o needing sshd on localhost we arrange to have : in the
-  # absolute filepath (when absolute, scp interprets as local even if : later)
   if (NOT EXISTS "${CTEST_DROP_SITE}:${CTEST_DROP_LOCATION}")
     message(FATAL_ERROR
       "must set ${CTEST_DROP_SITE}:${CTEST_DROP_LOCATION} to an existing directory")
-  endif (NOT EXISTS "${CTEST_DROP_SITE}:${CTEST_DROP_LOCATION}")
+  endif ()
 else (SUBMIT_LOCAL)
   # Nightly runs will use sources as of this time
   set(CTEST_NIGHTLY_START_TIME "04:00:00 EST")

clients/drcachesim/launcher.cpp

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2015-2019 Google, Inc.  All rights reserved.
+ * Copyright (c) 2015-2020 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -200,6 +200,13 @@ _tmain(int argc, const TCHAR *targv[])
     // one, there's no problem to solve like the UNIX fifo file left
     // behind.  Two, the ^c handler in a new thread is more work to
     // deal with as it races w/ the main thread.
+#    ifdef DEBUG
+    // Avoid pop-up messageboxes in tests.
+    if (!IsDebuggerPresent()) {
+        _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
+    }
+#    endif
 #endif
 
 #if defined(WINDOWS) && !defined(_UNICODE)

core/hotpatch.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2019 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2020 Google, Inc.  All rights reserved.
  * Copyright (c) 2005-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -604,7 +604,7 @@ hotp_dump_reg_state(const hotp_context_t *reg_state, const app_pc eip,
 #    endif
 static void
 hotp_only_inject_patch(const hotp_offset_match_t *ppoint_desc,
-                       const thread_record_t **all_threads, const int num_threads);
+                       const thread_record_t **thread_table, const int num_threads);
 static void
 hotp_only_remove_patch(dcontext_t *dcontext, const hotp_module_t *module,
                        hotp_patch_point_t *cur_ppoint);
@@ -1333,15 +1333,15 @@ hotp_read_policy_modes(hotp_policy_mode_t **old_modes)
      */
     for (i = 0; i < num_mode_update_entries; i++) {
         bool matched = false;
-        char *temp, *policy_id;
+        char *split, *policy_id;
 
         SET_STR_PTR(policy_id, start);
-        temp = strchr(policy_id, ':');
-        if (temp == NULL)
+        split = strchr(policy_id, ':');
+        if (split == NULL)
             goto error_reading_policy;
-        *temp++ = '\0'; /* TODO: during file mapping, this won't work */
+        *split++ = '\0'; /* TODO: during file mapping, this won't work */
 
-        SET_NUM(mode, uint, MODE, temp);
+        SET_NUM(mode, uint, MODE, split);
 
         /* Must set mode for all vulnerabilities with a matching policy_id, not
          * just the first one.
@@ -2043,16 +2043,16 @@ hotp_compute_hash(app_pc base, hotp_patch_point_hash_t *hash)
 static void
 hotp_process_image_helper(const app_pc base, const bool loaded,
                           const bool own_hot_patch_lock, const bool just_check,
-                          bool *needs_processing, const thread_record_t **all_threads,
+                          bool *needs_processing, const thread_record_t **thread_table,
                           const int num_threads, const bool ldr_safety,
                           vm_area_vector_t *toflush);
 void
 hotp_process_image(const app_pc base, const bool loaded, const bool own_hot_patch_lock,
                    const bool just_check, bool *needs_processing,
-                   const thread_record_t **all_threads, const int num_threads)
+                   const thread_record_t **thread_table, const int num_threads)
 {
     hotp_process_image_helper(base, loaded, own_hot_patch_lock, just_check,
-                              needs_processing, all_threads, num_threads, false, NULL);
+                              needs_processing, thread_table, num_threads, false, NULL);
 }
 
 /* Helper routine for seeing if point is in hotp_ppoint_vec */
@@ -2138,7 +2138,7 @@ hotp_perscache_overlap(uint vul, uint set, uint module, app_pc base, size_t imag
 static void
 hotp_process_image_helper(const app_pc base, const bool loaded,
                           const bool own_hot_patch_lock, const bool just_check,
-                          bool *needs_processing, const thread_record_t **all_threads,
+                          bool *needs_processing, const thread_record_t **thread_table,
                           const int num_threads_arg, const bool ldr_safety,
                           vm_area_vector_t *toflush)
 {
@@ -2488,7 +2488,7 @@ hotp_process_image_helper(const app_pc base, const bool loaded,
                                     hotp_ppoint_areas_add(&ppoint_desc);
 
                                 if (DYNAMO_OPTION(hotp_only)) {
-                                    hotp_only_inject_patch(&ppoint_desc, all_threads,
+                                    hotp_only_inject_patch(&ppoint_desc, thread_table,
                                                            num_threads);
                                 }
                             }
@@ -2727,7 +2727,7 @@ hotp_point_not_on_list(const app_pc start, const app_pc end, bool own_hot_patch_
  *
  */
 static void
-hotp_walk_loader_list(thread_record_t **all_threads, const int num_threads,
+hotp_walk_loader_list(thread_record_t **thread_table, const int num_threads,
                       vm_area_vector_t *toflush, bool probe_init)
 {
     /* This routine will go away; till then need to compile on linux.  Not walking
@@ -2741,7 +2741,7 @@ hotp_walk_loader_list(thread_record_t **all_threads, const int num_threads,
     LIST_ENTRY *e, *start;
     LDR_MODULE *mod;
 
-    /* For hotp_only, all_threads can be valid, i.e., all known threads may be
+    /* For hotp_only, thread_table can be valid, i.e., all known threads may be
      * suspended.  Even if they are not, all synch locks will be held, so that
      * module processing can happen without races.  Check for that.
      * Note: for -probe_api, this routine can be called during dr init time,
@@ -2775,7 +2775,7 @@ hotp_walk_loader_list(thread_record_t **all_threads, const int num_threads,
 
         /* TODO: ASSERT that the module is loaded? */
         hotp_process_image_helper(mod->BaseAddress, true, probe_init ? false : true,
-                                  false, NULL, all_threads, num_threads, false /*!ldr*/,
+                                  false, NULL, thread_table, num_threads, false /*!ldr*/,
                                   toflush);
 
         /* TODO: make hotp_process_image() emit different log messages
@@ -2965,7 +2965,7 @@ nudge_action_read_policies(void)
     hotp_vul_t *old_vul_table = NULL, *new_vul_table = NULL;
     uint num_old_vuls = 0, num_new_vuls;
     int num_threads = 0;
-    thread_record_t **all_threads = NULL;
+    thread_record_t **thread_table = NULL;
 
     STATS_INC(hotp_num_policy_nudge);
     /* Fix for case 6090;  TODO: remove when -hotp_policy_size is removed */
@@ -3041,7 +3041,7 @@ nudge_action_read_policies(void)
         if (DYNAMO_OPTION(hotp_only)) {
 #    ifdef WINDOWS
             DEBUG_DECLARE(bool ok =)
-            synch_with_all_threads(THREAD_SYNCH_SUSPENDED, &all_threads,
+            synch_with_all_threads(THREAD_SYNCH_SUSPENDED, &thread_table,
                                    /* Case 6821: other synch-all-thread uses that
                                     * only care about threads carrying fcache
                                     * state can ignore us
@@ -3086,7 +3086,7 @@ nudge_action_read_policies(void)
         hotp_init_policy_status_table();
         if (!DYNAMO_OPTION(hotp_only))
             vmvector_init_vector(&toflush, 0); /* no lock init needed since not used */
-        hotp_walk_loader_list(all_threads, num_threads,
+        hotp_walk_loader_list(thread_table, num_threads,
                               DYNAMO_OPTION(hotp_only) ? NULL : &toflush,
                               false /* !probe_init */);
         SELF_PROTECT_DATASEC(DATASEC_RARELY_PROT);
@@ -3095,7 +3095,7 @@ nudge_action_read_policies(void)
         d_r_write_unlock(&hotp_vul_table_lock);
 #    ifdef WINDOWS
         if (DYNAMO_OPTION(hotp_only)) {
-            end_synch_with_all_threads(all_threads, num_threads, true /*resume*/);
+            end_synch_with_all_threads(thread_table, num_threads, true /*resume*/);
         }
 #    endif
 
@@ -3193,7 +3193,7 @@ hotp_nudge_handler(uint nudge_action_mask)
     }
 
     if (TEST(NUDGE_GENERIC(mode), nudge_action_mask)) {
-        thread_record_t **all_threads = NULL;
+        thread_record_t **thread_table = NULL;
         int num_threads = 0;
         hotp_policy_mode_t *old_modes = NULL;
         vm_area_vector_t toflush; /* never initialized for hotp_only */
@@ -3209,7 +3209,7 @@ hotp_nudge_handler(uint nudge_action_mask)
         /* Suspend all threads (for hotp_only) and grab locks. */
         if (DYNAMO_OPTION(hotp_only)) {
             DEBUG_DECLARE(bool ok =)
-            synch_with_all_threads(THREAD_SYNCH_SUSPENDED, &all_threads,
+            synch_with_all_threads(THREAD_SYNCH_SUSPENDED, &thread_table,
                                    /* Case 6821: other synch-all-thread uses that
                                     * only care about threads carrying fcache
                                     * state can ignore us
@@ -3242,14 +3242,14 @@ hotp_nudge_handler(uint nudge_action_mask)
 
         if (!DYNAMO_OPTION(hotp_only))
             vmvector_init_vector(&toflush, 0); /* no lock init needed since not used */
-        hotp_walk_loader_list(all_threads, num_threads,
+        hotp_walk_loader_list(thread_table, num_threads,
                               DYNAMO_OPTION(hotp_only) ? NULL : &toflush,
                               false /* !probe_init */);
 
         /* Release all locks. */
         d_r_write_unlock(&hotp_vul_table_lock);
         if (DYNAMO_OPTION(hotp_only)) {
-            end_synch_with_all_threads(all_threads, num_threads, true /*resume*/);
+            end_synch_with_all_threads(thread_table, num_threads, true /*resume*/);
         }
 
         /* If modes did change, then we need to flush out patches that were
@@ -3971,7 +3971,7 @@ patch_cti_tgt(byte *tgt_loc, byte *new_val, bool hot_patch)
  */
 static void
 hotp_only_inject_patch(const hotp_offset_match_t *ppoint_desc,
-                       const thread_record_t **all_threads, const int num_threads)
+                       const thread_record_t **thread_table, const int num_threads)
 {
     hotp_vul_t *vul;
     hotp_set_t *set;
@@ -3984,13 +3984,13 @@ hotp_only_inject_patch(const hotp_offset_match_t *ppoint_desc,
 
     ASSERT(DYNAMO_OPTION(hotp_only));
 
-    /* At startup there should be no other thread than this, so all_threads
+    /* At startup there should be no other thread than this, so thread_table
      * won't be valid.
      */
     if (num_threads != HOTP_ONLY_NUM_THREADS_AT_INIT) {
-        ASSERT(ppoint_desc != NULL && all_threads != NULL);
+        ASSERT(ppoint_desc != NULL && thread_table != NULL);
     } else {
-        ASSERT(ppoint_desc != NULL && all_threads == NULL);
+        ASSERT(ppoint_desc != NULL && thread_table == NULL);
     }
 
     /* Check if it is safe to patch, i.e., no known threads should be running
@@ -4210,14 +4210,14 @@ hotp_only_inject_patch(const hotp_offset_match_t *ppoint_desc,
 
         for (i = 0; i < num_threads; i++) {
             /* Skip the current thread; nudge thread's Eip isn't relevant. */
-            if (my_tid == all_threads[i]->id)
+            if (my_tid == thread_table[i]->id)
                 continue;
 
             /* App thread can't be in the core holding a lock when suspended. */
-            ASSERT(thread_owns_no_locks(all_threads[i]->dcontext));
+            ASSERT(thread_owns_no_locks(thread_table[i]->dcontext));
 
             cxt.ContextFlags = CONTEXT_FULL; /* PR 264138: don't need xmm regs */
-            res = thread_get_context((thread_record_t *)all_threads[i], &cxt);
+            res = thread_get_context((thread_record_t *)thread_table[i], &cxt);
             ASSERT(res);
             eip = (app_pc)cxt.CXT_XIP;
 
@@ -4248,7 +4248,7 @@ hotp_only_inject_patch(const hotp_offset_match_t *ppoint_desc,
                 cxt.CXT_XIP =
                     (ptr_uint_t)(cur_ppoint->app_code_copy +
                                  (eip - (module->base_address + cur_ppoint->offset)));
-                res = thread_set_context((thread_record_t *)all_threads[i], &cxt);
+                res = thread_set_context((thread_record_t *)thread_table[i], &cxt);
                 ASSERT(res);
             }
         }
@@ -4426,7 +4426,7 @@ hotp_only_mem_prot_change(const app_pc start, const size_t size, const bool remo
     app_pc base;
     bool needs_processing = false;
     int num_threads = 0;
-    thread_record_t **all_threads = NULL;
+    thread_record_t **thread_table = NULL;
 #    ifdef WINDOWS
     DEBUG_DECLARE(bool ok;)
 #    endif
@@ -4478,7 +4478,7 @@ hotp_only_mem_prot_change(const app_pc start, const size_t size, const bool remo
 #    ifdef WINDOWS
     /* Ok, let's suspend all threads and do the injection/removal. */
     DEBUG_DECLARE(ok =)
-    synch_with_all_threads(THREAD_SYNCH_SUSPENDED, &all_threads, &num_threads,
+    synch_with_all_threads(THREAD_SYNCH_SUSPENDED, &thread_table, &num_threads,
                            /* Case 6821: other synch-all-thread uses that
                             * only care about threads carrying fcache
                             * state can ignore us
@@ -4502,7 +4502,7 @@ hotp_only_mem_prot_change(const app_pc start, const size_t size, const bool remo
         DODEBUG(hotp_globals->ldr_safe_hook_injection = true;); /* Case 7998. */
         DODEBUG(hotp_globals->ldr_safe_hook_removal = false;);  /* Case 7832. */
         hotp_process_image_helper(base, true, true, false, NULL,
-                                  (const thread_record_t **)all_threads, num_threads,
+                                  (const thread_record_t **)thread_table, num_threads,
                                   true, NULL);
         DODEBUG(hotp_globals->ldr_safe_hook_injection = false;);
         /* Similarly, hotp_remove_patches_from_module() is inefficient too.
@@ -4518,7 +4518,7 @@ hotp_only_mem_prot_change(const app_pc start, const size_t size, const bool remo
     }
     d_r_write_unlock(&hotp_vul_table_lock);
 #    ifdef WINDOWS
-    end_synch_with_all_threads(all_threads, num_threads, true /*resume*/);
+    end_synch_with_all_threads(thread_table, num_threads, true /*resume*/);
 #    endif
 }