[lldb/win] Improve check-lldb-shell with LLVM_ENABLE_DIA_SDK=NO

[thakis]

Summary

Before this change, running check-lldb-shell with LLVM_ENABLE_DIA_SDK=NO would fail ~25 tests.

This change adds a lit feature based on if the DIA SDK is available and annotates
all tests that set breakpoints with this new feature. Now these tests are skipped
(with a warning message during test startup) instead of failing. This matches how
this is handled in check-llvm.

(This doesn't touch API or Unit tests for now.)

[labath]

I fear this is going to be an endless whack-a-mole. There's no way every contributor will remember to (correctly) add this feature.

[thakis]

In D109834#3003250, @labath wrote:

I fear this is going to be an endless whack-a-mole. There's no way every contributor will remember to (correctly) add this feature.

I think that's ok. 10/250 shell tests need it and there isn't a ton of churn in these tests, so folks on windows can update it as needed.

(If you have a better idea, I'm all ears of course.)

[labath]

Hmm... is the (non native) SymbolFilePDB plugin actually capable of doing anything without the DIA SDK. If it can't set breakpoints, then I would guess the answer is: not much. Maybe we could automatically select the "Native" plugin in this case, just like we do on non-windows hosts?

It would actually be very interesting to know why the NativePDB/stack_unwinding01.cpp test fails without it, even though it forces the use of the native plugin.

[amccarth]

I'm trying to understand why I've never seen the problem that this patch is trying to address.

I wasn't aware of LLVM_ENABLE_DIA_SDK so I've never turned it off. But I've run check lldb exclusively with LLDB_USE_NATIVE_PDB_READER=TRUE for a long time, so I'm surprised to hear that some tests somehow still depend on DIA. Is check-lldb-shell not a subset of check-lldb?

How exactly would I reproduce the problem here?

I don't find much meaning in the term host-breakpoints, but maybe that's because I don't understand the bug.

[labath]

I suppose the meaning is "I can set breakpoints in host binaries", though if the entire symbol file plugin is not functional without the DIA SDK, then "I can parse host debug info" would be more correct, as the breakpoints are only the first thing you'll typically run into.

That said, if all test do pass with the Native pdb plugin, then I think that makes for an even stronger case to enable it if dia is unavailable. I might consider even enabling it by default. It's been in here for quite some time now.

[thakis]

Oh, that's a good idea. But even if I force on the native PDB reader if LLVM_ENABLE_DIA_SDK is false [1], I still get 15 failures. That's better than 27, but it's more than 0. Maybe LLDB_USE_NATIVE_PDB_READER doesn't hit all uses of DIA in lldb?

Failed Tests (15):
  lldb-shell :: Driver/TestSingleQuote.test
  lldb-shell :: Expr/nodefaultlib.cpp
  lldb-shell :: SymbolFile/NativePDB/stack_unwinding01.cpp
  lldb-shell :: SymbolFile/PDB/ast-restore.test
  lldb-shell :: SymbolFile/PDB/calling-conventions.test
  lldb-shell :: SymbolFile/PDB/class-layout.test
  lldb-shell :: SymbolFile/PDB/enums-layout.test
  lldb-shell :: SymbolFile/PDB/expressions.test
  lldb-shell :: SymbolFile/PDB/func-symbols.test
  lldb-shell :: SymbolFile/PDB/function-nested-block.test
  lldb-shell :: SymbolFile/PDB/pointers.test
  lldb-shell :: SymbolFile/PDB/type-quals.test
  lldb-shell :: SymbolFile/PDB/typedefs.test
  lldb-shell :: SymbolFile/PDB/udt-layout.test
  lldb-shell :: SymbolFile/PDB/variables.test


Testing Time: 38.91s
  Unsupported      : 1113
  Passed           :  263
  Expectedly Failed:   14
  Failed           :   15

1:

C:\src\llvm-project>git diff
diff --git a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cppindex 794fab5f7309..51c74c59b137 100644
--- a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -52,6 +52,10 @@
 #include "Plugins/Language/CPlusPlus/MSVCUndecoratedNameParser.h"
 #include "Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h"

+#if defined(_WIN32)
+#include "llvm/Config/config.h"
+#endif
+
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::pdb;
@@ -83,6 +87,7 @@ bool ShouldAddLine(uint32_t requested_line, uint32_t actual_line,

 static bool ShouldUseNativeReader() {
 #if defined(_WIN32)
+#if LLVM_ENABLE_DIA_SDK
   llvm::StringRef use_native = ::getenv("LLDB_USE_NATIVE_PDB_READER");
   return use_native.equals_insensitive("on") ||
          use_native.equals_insensitive("yes") ||
@@ -91,6 +96,9 @@ static bool ShouldUseNativeReader() {
 #else
   return true;
 #endif
+#else
+  return true;
+#endif
 }

 void SymbolFilePDB::Initialize() {

[thakis]

Well, it's a start, so let's land that part (D110172).

[labath]

The SymbolFile/PDB tests are supposed to specifically test the (non-native) pdb plugin, so disabling them in this mode makes perfect sense to me. (though someone interested in pdb support may want to take a look at them to see whether they are failing just because they are testing some specific behavior of the non-native plugin, or because they are exposing some bugs in the native version).

That leaves us with just three tests. I'm not sure what's the deal with TestSingleQuote.test, but I think it could be just some simple bug, where the code balks at a filename with quotes in it. I have no idea what's the deal with the other two. The stack unwinding failure is particularly strange, as that one already forces the use of the native reader.

Is there any chance someone could take a look at those?

lldb/test/Shell/Breakpoint/case-insensitive.test

-# REQUIRES: system-windows
+# REQUIRES: system-windows, host-breakpoints
 
 # RUN: %build %p/Inputs/case-sensitive.c --nodefaultlib -o %t
 # RUN: lldb-test breakpoints %t %s | FileCheck %s
 
 breakpoint set -f case-sensitive.c -l 3
 # CHECK-LABEL: breakpoint set -f case-sensitive.c -l 3
 # CHECK: At least one location.
 

lldb/test/Shell/Breakpoint/dummy-target.test

+# REQUIRES: host-breakpoints
+#
 # RUN: mkdir -p %t
 # RUN: cd %t
 # RUN: %build %p/Inputs/dummy-target.c -o dummy.out
 # RUN: %lldb -b -s %s dummy.out | FileCheck %s
 
 breakpoint set -D -n main
 # CHECK: Breakpoint {{[0-9]}}: no locations (pending).
 # CHECK: Breakpoint set in dummy target

lldb/test/Shell/Commands/command-thread-select.test

+# REQUIRES: host-breakpoints
+#
 # RUN: %clang_host -g %S/Inputs/main.c -o %t
 # RUN: %lldb %t -s %s -o exit | FileCheck %s
 
 b main
 # CHECK-LABEL: b main
 # CHECK: Breakpoint 1: where = {{.*}}`main
 
 run

lldb/test/Shell/Driver/TestSingleQuote.test

+# REQUIRES: host-breakpoints
+
 # Make sure lldb can handle filenames with single quotes in them.
 # RUN: %clang_host %p/Inputs/hello.c -g -o "%t-'pat"
 # RUN: %lldb -s %s "%t-'pat" | FileCheck %s
 
 br set -p return
 # CHECK: Breakpoint 1: where = TestSingleQuote.test.tmp-'pat`main

lldb/test/Shell/Expr/TestIRMemoryMapWindows.test

-# REQUIRES: system-windows
+# REQUIRES: system-windows, host-breakpoints
 
 # RUN: %clang_cl_host /Zi /GS- %p/Inputs/call-function.cpp /c /o %t.obj
 # RUN: %msvc_link /debug:full %t.obj /out:%t
 
 # RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic
 # RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-basic
 
 # RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1
 # RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-overlap1
 
 # RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-mix-malloc-free
 # RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-mix-malloc-free

lldb/test/Shell/Expr/nodefaultlib.cpp

 // Test that we're able to evaluate expressions in inferiors without the
 // standard library (and mmap-like functions in particular).
 
-// REQUIRES: native
+// REQUIRES: native, host-breakpoints
 // UNSUPPORTED: ld_preload-present
 // XFAIL: system-linux && !(target-x86 || target-x86_64)
 // XFAIL: system-netbsd || system-freebsd || system-darwin
 
 // RUN: %build %s --nodefaultlib -o %t
 // RUN: %lldb %t -o "b main" -o run -o "p call_me(5, 6)" -o exit \
 // RUN:   | FileCheck %s
 
 // CHECK: p call_me(5, 6)
 // CHECK: (int) $0 = 30
 
 int call_me(int x, long y) { return x * y; }
 
 int main() { return call_me(4, 5); }

lldb/test/Shell/Settings/TestFrameFormatColor.test

+# REQUIRES: host-breakpoints
 # RUN: %clang_host -g -O0 %S/Inputs/main.c -o %t.out
 # RUN: %lldb -x -b -s %s %t.out | FileCheck %s
 settings set use-color true
 settings set -f frame-format "frame #${frame.index}: \`${ansi.fg.green}{${function.name-with-args}${ansi.normal}\n"
 b foo
 run
 bt
 c
 q
 
 # Check the ASCII escape code
 # CHECK: ␛

lldb/test/Shell/Settings/TestFrameFormatNoColor.test

+# REQUIRES: host-breakpoints
 # RUN: %clang_host -g -O0 %S/Inputs/main.c -o %t.out
 # RUN: %lldb -x -b -s %s %t.out | FileCheck %s
 settings set use-color false
 settings set -f frame-format "frame #${frame.index}: \`${ansi.fg.green}{${function.name-with-args}${ansi.normal}\n"
 b foo
 run
 bt
 c
 q
 
 # Check the ASCII escape code
 # CHECK-NOT: ␛

lldb/test/Shell/Settings/TestLineMarkerColor.test

+# REQUIRES: host-breakpoints
 # RUN: %clang_host -g -O0 %S/Inputs/main.c -o %t.out
 # RUN: %lldb -x -b -s %s %t.out | FileCheck %s
 settings set use-color true
 settings set stop-show-line-ansi-prefix "prefix"
 settings set stop-show-line-ansi-suffix "suffix"
 b foo
 run
 c

lldb/test/Shell/SymbolFile/NativePDB/stack_unwinding01.cpp

 // clang-format off
-// REQUIRES: lld, system-windows
+// REQUIRES: lld, system-windows, host-breakpoints
 
 // RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s
 // RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
 // RUN:     %p/Inputs/stack_unwinding01.lldbinit 2>&1 | FileCheck %s
 
 
 struct Struct {
   void simple_method(int a, int b) {

lldb/test/Shell/SymbolFile/PDB/lit.local.cfg

-if 'lldb-repro' in config.available_features:
+if (not config.have_dia_sdk or
+    'lldb-repro' in config.available_features):
   config.unsupported = True

lldb/test/Shell/Watchpoint/SetErrorCases.test

+# REQUIRES: host-breakpoints
 # RUN: %clangxx_host %p/Inputs/main.cpp -g -o %t.out
 # RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -s %s %t.out 2>&1 | FileCheck %s
 
 settings show interpreter.stop-command-source-on-error
 # CHECK: interpreter.stop-command-source-on-error (boolean) = false
 
 b main.cpp:11
 run

lldb/test/Shell/lit.cfg.py

 # possible to capture a test suite run and then rerun all the test from the
 # just captured reproducer.
 lldb_repro_mode = lit_config.params.get('lldb-run-with-repro', None)
 if lldb_repro_mode:
   config.available_features.add('lldb-repro')
   lit_config.note("Running Shell tests in {} mode.".format(lldb_repro_mode))
   toolchain.use_lldb_repro_substitutions(config, lldb_repro_mode)
 
+# Do DIA check early, so that the warning shows up above the config.py notes.
+if sys.platform != 'win32':
+  config.available_features.add('host-breakpoints')
+else:
+  if config.have_dia_sdk:
+    config.available_features.add('host-breakpoints')
+  else:
+    lit_config.warning('lldb built without DIA SDK, several tests will be skipped')
+
 llvm_config.use_default_substitutions()
 toolchain.use_lldb_substitutions(config)
 toolchain.use_support_substitutions(config)
 
 if re.match(r'^arm(hf.*-linux)|(.*-linux-gnuabihf)', config.target_triple):
     config.available_features.add("armhf-linux")
 
 def calculate_arch_features(arch_string):

lldb/test/Shell/lit.site.cfg.py.in

 config.lldb_libs_dir = "@LLDB_LIBS_DIR@"
 config.lldb_tools_dir = "@LLDB_TOOLS_DIR@"
 # Since it comes from the command line, it may have backslashes which
 # should not need to be escaped.
 config.lldb_lit_tools_dir = r"@LLDB_LIT_TOOLS_DIR@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.python_executable = "@Python3_EXECUTABLE@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_dia_sdk = @LLVM_ENABLE_DIA_SDK@
 config.lldb_enable_lzma = @LLDB_ENABLE_LZMA@
 config.host_triple = "@LLVM_HOST_TRIPLE@"
 config.lldb_bitness = 64 if @LLDB_IS_64_BITS@ else 32
 config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
 config.lldb_enable_lua = @LLDB_ENABLE_LUA@
 config.lldb_build_directory = "@LLDB_TEST_BUILD_DIRECTORY@"
 config.lldb_system_debugserver = @LLDB_USE_SYSTEM_DEBUGSERVER@
 # The shell tests use their own module caches.

llvm/utils/gn/secondary/lldb/test/BUILD.gn

+import("//llvm/lib/DebugInfo/PDB/enable_dia.gni")
 import("//llvm/triples.gni")
-
-#import("//llvm/utils/gn/build/libs/xar/enable.gni")
 import("//llvm/utils/gn/build/libs/xml/enable.gni")
 import("//llvm/utils/gn/build/libs/zlib/enable.gni")
 import("//llvm/utils/gn/build/write_cmake_config.gni")
 import("lldb_lit_site_cfg_files.gni")
 
 # The bits common to writing lit.site.cfg.py.in, API/lit.site.cfg.py,
 # Shell/lit.site.cfg.py, and Unit/lit.site.cfg.py.in.
 template("write_lit_cfg") {
extra_values = [
     "LLDB_TEST_MODULE_CACHE_LLDB=" +
         rebase_path(
             "$target_gen_dir/lldb-test-build.noindex/module-cache-lldb"),
     "LLDB_TOOLS_DIR=" + rebase_path("$root_out_dir/bin"),
     "LLDB_USE_SYSTEM_DEBUGSERVER=1",  # XXX port //lldb/tools/debugserver (?)
     "LLVM_HOST_TRIPLE=$llvm_current_triple",
   ]
 
+  if (llvm_enable_dia_sdk) {
+    extra_values += [ "LLVM_ENABLE_DIA_SDK=1" ]
+  } else {
+    extra_values += [ "LLVM_ENABLE_DIA_SDK=0" ]  # Must be 0.
+  }
+
   if (llvm_enable_zlib) {
     extra_values += [ "LLVM_ENABLE_ZLIB=1" ]
   } else {
     extra_values += [ "LLVM_ENABLE_ZLIB=0" ]  # Must be 0.
   }
 
   dir = get_path_info(output, "dir")
   if (host_os == "win") {