date:20220923

[Lldb-commits] [PATCH] D134570: [lldb] Skip check for conflicting filter/synth when adding a new regex.

2022-09-23 Thread Jorge Gorbe Moya via Phabricator via lldb-commits

jgorbe updated this revision to Diff 462637.
jgorbe added a comment.

Added a couple of test cases to TestDataFormatterPythonSynth.py.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134570/new/

https://reviews.llvm.org/D134570

Files:
  lldb/source/Commands/CommandObjectType.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py


Index: 
lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
===
--- 
lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
+++ 
lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
@@ -257,6 +257,22 @@
 self.expect("frame variable f00_1", matching=False,
 substrs=['fake_a = '])
 
+# check that we don't feed a regex into another regex when checking for
+# existing conflicting synth/filters. The two following expressions
+# accept different types: one will accept types that look like an array
+# of MyType, the other will accept types that contain "MyType1" or
+# "MyType2". But the second regex looks like an array of MyType, so
+# lldb used to incorrectly reject it.
+self.runCmd(r'type synth add -l fooSynthProvider -x 
"^MyType\[[0-9]+]$"')
+self.runCmd(r'type filter add --child a -x "MyType[12]"')
+
+# Same, but adding the filter first to verify the check when doing
+# `type synth add`. We need to delete the synth from the previous test
+# first.
+self.runCmd(r'type synth delete "^MyType\[[0-9]+]$"')
+self.runCmd(r'type filter add --child a -x "^MyType\[[0-9]+]$"')
+self.runCmd(r'type synth add -l fooSynthProvider -x "MyType[12]"')
+
 def rdar10960550_formatter_commands(self):
 """Test that synthetic children persist stoppoints."""
 self.runCmd("file " + self.getBuildArtifact("a.out"), 
CURRENT_EXECUTABLE_SET)
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -2335,12 +2335,17 @@
   type = eRegexSynth;
   }
 
-  if (category->AnyMatches(type_name, eFormatCategoryItemFilter, false)) {
-if (error)
-  error->SetErrorStringWithFormat("cannot add synthetic for type %s when "
-  "filter is defined in same category!",
-  type_name.AsCString());
-return false;
+  // Only check for conflicting filters in the same category if `type_name` is
+  // an actual type name. Matching a regex string against registered regexes
+  // doesn't work.
+  if (type == eRegularSynth) {
+if (category->AnyMatches(type_name, eFormatCategoryItemFilter, false)) {
+  if (error)
+error->SetErrorStringWithFormat("cannot add synthetic for type %s when 
"
+"filter is defined in same category!",
+type_name.AsCString());
+  return false;
+}
   }
 
   if (type == eRegexSynth) {
@@ -2458,13 +2463,18 @@
 type = eRegexFilter;
 }
 
-if (category->AnyMatches(type_name, eFormatCategoryItemSynth, false)) {
-  if (error)
-error->SetErrorStringWithFormat("cannot add filter for type %s when "
-"synthetic is defined in same "
-"category!",
-type_name.AsCString());
-  return false;
+// Only check for conflicting synthetic child providers in the same 
category
+// if `type_name` is an actual type name. Matching a regex string against
+// registered regexes doesn't work.
+if (type == eRegularFilter) {
+  if (category->AnyMatches(type_name, eFormatCategoryItemSynth, false)) {
+if (error)
+  error->SetErrorStringWithFormat("cannot add filter for type %s when "
+  "synthetic is defined in same "
+  "category!",
+  type_name.AsCString());
+return false;
+  }
 }
 
 if (type == eRegexFilter) {


Index: lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-python-synth/TestDataFormatterPythonSynth.py
@@ -257,6 +257,22 @@
 self.expect("frame variable f00_1", matching=False,
 substrs=['fake_a = '])
 
+# check

[Lldb-commits] [PATCH] D134574: [lldb][test] 4 - Add gmodules test category explicitly where previously done implicitly

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462622.
Michael137 added a comment.

- Add decorator to another test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134574/new/

https://reviews.llvm.org/D134574

Files:
  lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
  lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
  lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
  lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
  lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
  lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
  lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py


Index: lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py
===
--- lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py
+++ lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py
@@ -18,8 +18,9 @@
 self.break_line = line_number(
 self.main_source, '// Set breakpoint here.')
 
-@add_test_categories(['pyapi'])
-@skipIf(debug_info=no_match(["gmodules"]), oslist=['ios', 'watchos', 
'tvos', 'bridgeos'], archs=['armv7', 'arm64'])  # this test program only builds 
for ios with -gmodules
+# this test program only builds for ios with -gmodules
+@add_test_categories(['gmodules', 'pyapi'])
+@skipIf(oslist=['ios', 'watchos', 'tvos', 'bridgeos'], archs=['armv7', 
'arm64'])
 def test_with_python_api(self):
 """Test passing structs to Objective-C methods."""
 self.build()
Index: lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
===
--- lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
+++ lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
@@ -10,7 +10,7 @@
 
 class TestClangModuleUpdate(TestBase):
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 @skipIfDarwin # rdar://76540904
 def test_expr(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
Index: 
lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
===
--- 
lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
+++ 
lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
@@ -11,7 +11,8 @@
 
 class ModulesInlineFunctionsTestCase(TestBase):
 
-@skipIf(macos_version=["<", "10.12"], debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
+@skipIf(macos_version=["<", "10.12"])
 def test_expr(self):
 self.build()
 exe = self.getBuildArtifact("a.out")
Index: lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
===
--- lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
+++ lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
@@ -15,7 +15,7 @@
 # Find the line number to break inside main().
 self.line = line_number('main.m', '// Set breakpoint 0 here.')
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 def test_expr(self):
 self.build()
 exe = self.getBuildArtifact("a.out")
Index: 
lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
===
--- 
lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
+++ 
lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
@@ -10,7 +10,7 @@
 
 class TestClangModuleHashMismatch(TestBase):
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 def test_expr(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
 f.write("""
Index: lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
===
--- lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
+++ lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
@@ -10,7 +10,7 @@
 
 class TestClangModuleAppUpdate(TestBase):
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 def test_rebuild_app_modules_untouched(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
 f.write("""
Index: lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
===
--- lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
+++ lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py

[Lldb-commits] [PATCH] D134524: [lldb][test] 1 - Don't do test-replication for gmodules debug_info variant

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462621.
Michael137 added a comment.

- Fixup


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134524/new/

https://reviews.llvm.org/D134524

Files:
  lldb/docs/resources/test.rst
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/test_categories.py


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume 
that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = 
set(test_categories.debug_info_categories)
+all_dbginfo_categories = 
set(test_categories.debug_info_categories.keys())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in 
test_categories.debug_info_categories.items() \
+  if can_replicate]
 
 for cat in categories:
 @decorators.add_test_categories([cat])
Index: lldb/docs/resources/test.rst
===
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -173,9 +173,9 @@
 similar to what you want to do.
 
 Another thing this enables is having different variants for the same test
-case. By default, we run every test for all 3 debug info formats, so once with
-DWARF from the object files, once with gmodules and finally with a dSYM on
-macOS or split DWARF (DWO) on Linux. But there are many more things we can test
+case. By default, we run every test for two debug info formats, once with
+DWARF from the object files and another with a dSYM on macOS or split
+DWARF (DWO) on Linux. But there are many more things we can test
 that are orthogonal to the test itself. On GreenDragon we have a matrix bot
 that runs the test suite under different configurations, with older host
 compilers and different DWARF versions.


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = set(test_categories.debug_info_categories)
+all_dbginfo_categories = set(test_categories.debug_info_categories.keys())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in

[Lldb-commits] [PATCH] D134572: [lldb][test] 2 - Allow multiple precompiled headers in API tests via PCH_CXX_SOURCE

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 abandoned this revision.
Michael137 added a comment.

Actually realised this isn't necessary...abandoning


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134572/new/

https://reviews.llvm.org/D134572

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134573: [lldb][test] 3 - Add gmodules XFAIL test for lang/cpp/gmodules/templates

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462618.
Michael137 added a comment.

- Fix breakpoint and move docstring


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134573/new/

https://reviews.llvm.org/D134573

Files:
  lldb/test/API/lang/cpp/gmodules/templates/Makefile
  lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
  lldb/test/API/lang/cpp/gmodules/templates/main.cpp


Index: lldb/test/API/lang/cpp/gmodules/templates/main.cpp
===
--- lldb/test/API/lang/cpp/gmodules/templates/main.cpp
+++ lldb/test/API/lang/cpp/gmodules/templates/main.cpp
@@ -1,9 +1,9 @@
 #include "b.h"
+#include 
 
 int main(int argc, const char * argv[])
 {
 Module m;
-// Test that the type Module which contains a field that is a
-// template instantiation can be fully resolved.
-return 0; //% 
self.assertTrue(self.frame().FindVariable('m').GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName()
 == 'buffer', 'find template specializations in imported modules')
+std::puts("Break here");
+return 0;
 }
Index: lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
===
--- lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
+++ lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
@@ -1,6 +1,22 @@
-import lldbsuite.test.lldbinline as lldbinline
+"""
+Test that the type Module which contains a field that is a
+template instantiation can be fully resolved.
+"""
+
+import lldb
 from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGModules(TestBase):
+
+@add_test_categories(["gmodules"])
+@expectedFailureAll
+def test_gmodules(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "Break here", 
lldb.SBFileSpec("main.cpp"))
+
+name = lldbutil.frame().FindVariable('m') \
+
.GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName()
 
-lldbinline.MakeInlineTest(__file__, globals(), [
-expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr36107",
-debug_info="gmodules")])
+self.assertEqual(name, 'buffer', 'find template specializations in 
imported modules')
Index: lldb/test/API/lang/cpp/gmodules/templates/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/gmodules/templates/Makefile
@@ -0,0 +1,4 @@
+PCH_CXX_SOURCES = a.h b.h
+CXX_SOURCES:= main.cpp
+
+include Makefile.rules


Index: lldb/test/API/lang/cpp/gmodules/templates/main.cpp
===
--- lldb/test/API/lang/cpp/gmodules/templates/main.cpp
+++ lldb/test/API/lang/cpp/gmodules/templates/main.cpp
@@ -1,9 +1,9 @@
 #include "b.h"
+#include 
 
 int main(int argc, const char * argv[])
 {
 Module m;
-// Test that the type Module which contains a field that is a
-// template instantiation can be fully resolved.
-return 0; //% self.assertTrue(self.frame().FindVariable('m').GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName() == 'buffer', 'find template specializations in imported modules')
+std::puts("Break here");
+return 0;
 }
Index: lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
===
--- lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
+++ lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
@@ -1,6 +1,22 @@
-import lldbsuite.test.lldbinline as lldbinline
+"""
+Test that the type Module which contains a field that is a
+template instantiation can be fully resolved.
+"""
+
+import lldb
 from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGModules(TestBase):
+
+@add_test_categories(["gmodules"])
+@expectedFailureAll
+def test_gmodules(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "Break here", lldb.SBFileSpec("main.cpp"))
+
+name = lldbutil.frame().FindVariable('m') \
+.GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName()
 
-lldbinline.MakeInlineTest(__file__, globals(), [
-expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr36107",
-debug_info="gmodules")])
+self.assertEqual(name, 'buffer', 'find template specializations in imported modules')
Index: lldb/test/API/lang/cpp/gmodules/templates/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/gmodules/templates/Makefile
@@ -0,0 +1,4 @@
+PCH_CXX_SOURCES = a.h b.h
+CXX_SOURCES:= main.cpp
+
+include Makefile.rules
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D134574: [lldb][test] 4 - Add gmodules test category explicitly where previously done implicitly

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 created this revision.
Michael137 added a reviewer: aprantl.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Since we don't compile with `gmodules` implicitly via
debug-info test replication, we should mark all implicit
`gmodules` tests with the appropriate category so the API
tests get actually run as intended.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134574

Files:
  lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
  lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
  lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
  lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
  lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
  lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py


Index: lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py
===
--- lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py
+++ lldb/test/API/lang/objc/objc-struct-argument/TestObjCStructArgument.py
@@ -18,8 +18,9 @@
 self.break_line = line_number(
 self.main_source, '// Set breakpoint here.')
 
-@add_test_categories(['pyapi'])
-@skipIf(debug_info=no_match(["gmodules"]), oslist=['ios', 'watchos', 
'tvos', 'bridgeos'], archs=['armv7', 'arm64'])  # this test program only builds 
for ios with -gmodules
+# this test program only builds for ios with -gmodules
+@add_test_categories(['gmodules', 'pyapi'])
+@skipIf(oslist=['ios', 'watchos', 'tvos', 'bridgeos'], archs=['armv7', 
'arm64'])
 def test_with_python_api(self):
 """Test passing structs to Objective-C methods."""
 self.build()
Index: lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
===
--- lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
+++ lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
@@ -10,7 +10,7 @@
 
 class TestClangModuleUpdate(TestBase):
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 @skipIfDarwin # rdar://76540904
 def test_expr(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
Index: 
lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
===
--- 
lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
+++ 
lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
@@ -11,7 +11,8 @@
 
 class ModulesInlineFunctionsTestCase(TestBase):
 
-@skipIf(macos_version=["<", "10.12"], debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
+@skipIf(macos_version=["<", "10.12"])
 def test_expr(self):
 self.build()
 exe = self.getBuildArtifact("a.out")
Index: lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
===
--- lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
+++ lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
@@ -15,7 +15,7 @@
 # Find the line number to break inside main().
 self.line = line_number('main.m', '// Set breakpoint 0 here.')
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 def test_expr(self):
 self.build()
 exe = self.getBuildArtifact("a.out")
Index: 
lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
===
--- 
lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
+++ 
lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
@@ -10,7 +10,7 @@
 
 class TestClangModuleHashMismatch(TestBase):
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 def test_expr(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
 f.write("""
Index: lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
===
--- lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
+++ lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
@@ -10,7 +10,7 @@
 
 class TestClangModuleAppUpdate(TestBase):
 
-@skipIf(debug_info=no_match(["gmodules"]))
+@add_test_categories(["gmodules"])
 def test_rebuild_app_modules_untouched(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
 f.write("""


Index:

[Lldb-commits] [PATCH] D134573: [lldb][test] 3 - Add gmodules XFAIL test for lang/cpp/gmodules/templates

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 created this revision.
Michael137 added a reviewer: aprantl.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Since we don't implicitly compile tests with `gmodules` now,
add a `gmodules` test category to this test explicitly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134573

Files:
  lldb/test/API/lang/cpp/gmodules/templates/Makefile
  lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
  lldb/test/API/lang/cpp/gmodules/templates/main.cpp


Index: lldb/test/API/lang/cpp/gmodules/templates/main.cpp
===
--- lldb/test/API/lang/cpp/gmodules/templates/main.cpp
+++ lldb/test/API/lang/cpp/gmodules/templates/main.cpp
@@ -5,5 +5,5 @@
 Module m;
 // Test that the type Module which contains a field that is a
 // template instantiation can be fully resolved.
-return 0; //% 
self.assertTrue(self.frame().FindVariable('m').GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName()
 == 'buffer', 'find template specializations in imported modules')
+return 0;
 }
Index: lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
===
--- lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
+++ lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
@@ -1,6 +1,17 @@
-import lldbsuite.test.lldbinline as lldbinline
+import lldb
 from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
 
-lldbinline.MakeInlineTest(__file__, globals(), [
-expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr36107",
-debug_info="gmodules")])
+class TestGModules(TestBase):
+
+@add_test_categories(["gmodules"])
+@expectedFailureAll
+def test_gmodules(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "Break here", 
lldb.SBFileSpec("main.cpp"))
+
+name = lldbutil.frame().FindVariable('m') \
+
.GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName()
+
+self.assertEqual(name, 'buffer', 'find template specializations in 
imported modules')
Index: lldb/test/API/lang/cpp/gmodules/templates/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/gmodules/templates/Makefile
@@ -0,0 +1,4 @@
+PCH_CXX_SOURCES = a.h b.h
+CXX_SOURCES:= main.cpp
+
+include Makefile.rules


Index: lldb/test/API/lang/cpp/gmodules/templates/main.cpp
===
--- lldb/test/API/lang/cpp/gmodules/templates/main.cpp
+++ lldb/test/API/lang/cpp/gmodules/templates/main.cpp
@@ -5,5 +5,5 @@
 Module m;
 // Test that the type Module which contains a field that is a
 // template instantiation can be fully resolved.
-return 0; //% self.assertTrue(self.frame().FindVariable('m').GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName() == 'buffer', 'find template specializations in imported modules')
+return 0;
 }
Index: lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
===
--- lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
+++ lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py
@@ -1,6 +1,17 @@
-import lldbsuite.test.lldbinline as lldbinline
+import lldb
 from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
 
-lldbinline.MakeInlineTest(__file__, globals(), [
-expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr36107",
-debug_info="gmodules")])
+class TestGModules(TestBase):
+
+@add_test_categories(["gmodules"])
+@expectedFailureAll
+def test_gmodules(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "Break here", lldb.SBFileSpec("main.cpp"))
+
+name = lldbutil.frame().FindVariable('m') \
+.GetChildAtIndex(0).GetChildAtIndex(0).GetChildAtIndex(0).GetName()
+
+self.assertEqual(name, 'buffer', 'find template specializations in imported modules')
Index: lldb/test/API/lang/cpp/gmodules/templates/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/gmodules/templates/Makefile
@@ -0,0 +1,4 @@
+PCH_CXX_SOURCES = a.h b.h
+CXX_SOURCES:= main.cpp
+
+include Makefile.rules
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134572: [lldb][test] 2 - Allow multiple precompiled headers in API tests via PCH_CXX_SOURCE

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 created this revision.
Michael137 added a reviewer: aprantl.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Currently we only allow a single PCH file to be
specified in an API test since Clang's `-include`
option doesn't accept multple arguments. Instead
add a `include-pch` prefix instead.

**Drive-by change**:

- Rename `PCH_CXX_SOURCE` to `PCH_CXX_SOURCES` to make the variable's usage 
clearer

**Testing**:

- Confirmed that the dwarfdump with and without the patch are identical for the 
only API test in the test-suite that uses PCH_CXX_SOURCE


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134572

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/lang/cpp/gmodules/basic/Makefile


Index: lldb/test/API/lang/cpp/gmodules/basic/Makefile
===
--- lldb/test/API/lang/cpp/gmodules/basic/Makefile
+++ lldb/test/API/lang/cpp/gmodules/basic/Makefile
@@ -1,4 +1,4 @@
-PCH_CXX_SOURCE = pch.h
+PCH_CXX_SOURCES = pch.h
 CXX_SOURCES = main.cpp
 CFLAGS_EXTRAS := $(MODULE_DEBUG_INFO_FLAGS)
 
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -458,11 +458,11 @@
 endif
 
 #--
-# Check if we have a precompiled header
+# Check if we have a precompiled headers
 #--
-ifneq "$(strip $(PCH_CXX_SOURCE))" ""
-   PCH_OUTPUT = $(PCH_CXX_SOURCE:.h=.h.pch)
-   PCHFLAGS = -include $(PCH_CXX_SOURCE)
+ifneq "$(strip $(PCH_CXX_SOURCES))" ""
+   PCH_OUTPUT = $(PCH_CXX_SOURCES:.h=.h.pch)
+   PCHFLAGS = $(addprefix -include-pch , $(PCH_OUTPUT))
 endif
 
 #--
@@ -600,11 +600,11 @@
 endif
 
 #--
-# Make the precompiled header and compile C++ sources against it
+# Make the precompiled headers and compile C++ sources against it
 #--
 
 ifneq "$(PCH_OUTPUT)" ""
-$(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
+$(PCH_OUTPUT) : $(PCH_CXX_SOURCES)
$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
 endif
 


Index: lldb/test/API/lang/cpp/gmodules/basic/Makefile
===
--- lldb/test/API/lang/cpp/gmodules/basic/Makefile
+++ lldb/test/API/lang/cpp/gmodules/basic/Makefile
@@ -1,4 +1,4 @@
-PCH_CXX_SOURCE = pch.h
+PCH_CXX_SOURCES = pch.h
 CXX_SOURCES = main.cpp
 CFLAGS_EXTRAS := $(MODULE_DEBUG_INFO_FLAGS)
 
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -458,11 +458,11 @@
 endif
 
 #--
-# Check if we have a precompiled header
+# Check if we have a precompiled headers
 #--
-ifneq "$(strip $(PCH_CXX_SOURCE))" ""
-	PCH_OUTPUT = $(PCH_CXX_SOURCE:.h=.h.pch)
-	PCHFLAGS = -include $(PCH_CXX_SOURCE)
+ifneq "$(strip $(PCH_CXX_SOURCES))" ""
+	PCH_OUTPUT = $(PCH_CXX_SOURCES:.h=.h.pch)
+	PCHFLAGS = $(addprefix -include-pch , $(PCH_OUTPUT))
 endif
 
 #--
@@ -600,11 +600,11 @@
 endif
 
 #--
-# Make the precompiled header and compile C++ sources against it
+# Make the precompiled headers and compile C++ sources against it
 #--
 
 ifneq "$(PCH_OUTPUT)" ""
-$(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
+$(PCH_OUTPUT) : $(PCH_CXX_SOURCES)
 	$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
 endif
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 added a comment.

In D134344#3811143 , @labath wrote:

> In D134344#3805953 , @Michael137 
> wrote:
>
>> that would require an audit of each API test right?
>
> Not really. I think this could be a purely mechanical change which replaces 
> `NO_DEBUG_INFO_TESTCASE = False` with something different. Ideally I'd make 
> that something a "inheriting from a different class". So we could have 
> something like `APITestCase` and a `DebugInfoTestCase` (inheriting from the 
> first), and the tests which want debug info replication (one can also imagine 
> different kinds of replication for some other tests) would inherit from the 
> latter.
>
> In D134344#3806509 , @aprantl wrote:
>
>> But I'm also missing the context as to why this would be desirable, so if 
>> there's a good reason, let me know!
>
> I have two reasons for that:
> The first is that from a simply engineering perspective, doing it the other 
> way around seems cleaner, as now we kind of have two ways to avoid 
> replication. Either you don't inherit from the replicated test base clase 
> (all lldb-server and lldb-vscode tests do that), or you do, but then mark 
> yourself as NO_DEBUG_INFO_TESTCASE.
> Secondly, it seems to be that no-replication is a better default. We have a 
> lot of features that don't (or shouldn't) depend on the kind of debug info 
> we're using, and we're probably forgetting to add this attribute to some of 
> them. It's possible those tests are adding some marginal debug info coverage, 
> but it's hard to rely on that, because noone know what that is. So I'd say 
> that an opt-in is a better default (particularly for the tests we're adding 
> nowadays), and the replication should be done when you know you're doing 
> something debug-heavy.
> I also think that having a more opt-in mechanism could enable us to do *more* 
> replication. For example, I think that running the some tests in both DWARF 
> v4 and v5 modes would be interesting, but I definitely wouldn't want to run 
> all of them, all the time.
>
> In D134344#3811091 , @Michael137 
> wrote:
>
>>> (B) Keep the `gmodules` category in the debug_info categories but add an 
>>> indicator (e.g., by making the `debug_info_categories` a dictionary) that 
>>> will skip replication if set. That would solve (1). And (2) would work as 
>>> it does today without changes.
>>
>> Uploaded alternative diff that implements this option. Seems simpler since 
>> tests in the `gmodules` category Just Work and we don't need to special-case 
>> `gmodules` in several places
>>
>> https://reviews.llvm.org/D134524
>
> It's not exactly how I was imagining this, but I like it. :)

We could perhaps move the discussion to discourse. I think the points raised 
are worth exploring further


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134344/new/

https://reviews.llvm.org/D134344

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134524: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462609.
Michael137 added a comment.

- Reword commit
- Add docs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134524/new/

https://reviews.llvm.org/D134524

Files:
  lldb/docs/resources/test.rst
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/test_categories.py


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume 
that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = 
set(test_categories.debug_info_categories)
+all_dbginfo_categories = 
set(test_categories.debug_info_categories.keys())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in 
test_categories.debug_info_categories.items() \
+  if can_replicate]
 
 for cat in categories:
 @decorators.add_test_categories([cat])
Index: lldb/docs/resources/test.rst
===
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -173,9 +173,9 @@
 similar to what you want to do.
 
 Another thing this enables is having different variants for the same test
-case. By default, we run every test for all 3 debug info formats, so once with
-DWARF from the object files, once with gmodules and finally with a dSYM on
-macOS or split DWARF (DWO) on Linux. But there are many more things we can test
+case. By default, we run every test for two debug info formats, once with
+DWARF from the object files and another with a dSYM on macOS or split
+DWARF (DWO) on Linux. But there are many more things we can test
 that are orthogonal to the test itself. On GreenDragon we have a matrix bot
 that runs the test suite under different configurations, with older host
 compilers and different DWARF versions.


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = set(test_categories.debug_info_categories)
+all_dbginfo_categories = set(test_categories.debug_info_categories.keys())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in

[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 abandoned this revision.
Michael137 added a comment.

Abandoning in favour of https://reviews.llvm.org/D134524


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134344/new/

https://reviews.llvm.org/D134344

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134570: [lldb] Skip check for conflicting filter/synth when adding a new regex.

2022-09-23 Thread Jorge Gorbe Moya via Phabricator via lldb-commits

jgorbe created this revision.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.

When adding a new synthetic child provider, we check for an existing
conflicting filter in the same category (and vice versa). This is done
by trying to match the new type name against registered formatters.

However, the new type name we're registered can also be a regex
(`type synth add -x`), and in this case the conflict check is just
wrong: it will try to match the new regex as if it was a type name,
against previously registered regexes.

See https://github.com/llvm/llvm-project/issues/57947 for a longer
explanation with concrete examples of incorrect behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134570

Files:
  lldb/source/Commands/CommandObjectType.cpp


Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -2335,12 +2335,17 @@
   type = eRegexSynth;
   }
 
-  if (category->AnyMatches(type_name, eFormatCategoryItemFilter, false)) {
-if (error)
-  error->SetErrorStringWithFormat("cannot add synthetic for type %s when "
-  "filter is defined in same category!",
-  type_name.AsCString());
-return false;
+  // Only check for conflicting filters in the same category if `type_name` is
+  // an actual type name. Matching a regex string against registered regexes
+  // doesn't work.
+  if (type == eRegularSynth) {
+if (category->AnyMatches(type_name, eFormatCategoryItemFilter, false)) {
+  if (error)
+error->SetErrorStringWithFormat("cannot add synthetic for type %s when 
"
+"filter is defined in same category!",
+type_name.AsCString());
+  return false;
+}
   }
 
   if (type == eRegexSynth) {
@@ -2458,13 +2463,18 @@
 type = eRegexFilter;
 }
 
-if (category->AnyMatches(type_name, eFormatCategoryItemSynth, false)) {
-  if (error)
-error->SetErrorStringWithFormat("cannot add filter for type %s when "
-"synthetic is defined in same "
-"category!",
-type_name.AsCString());
-  return false;
+// Only check for conflicting synthetic child providers in the same 
category
+// if `type_name` is an actual type name. Matching a regex string against
+// registered regexes doesn't work.
+if (type == eRegularFilter) {
+  if (category->AnyMatches(type_name, eFormatCategoryItemSynth, false)) {
+if (error)
+  error->SetErrorStringWithFormat("cannot add filter for type %s when "
+  "synthetic is defined in same "
+  "category!",
+  type_name.AsCString());
+return false;
+  }
 }
 
 if (type == eRegexFilter) {


Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -2335,12 +2335,17 @@
   type = eRegexSynth;
   }
 
-  if (category->AnyMatches(type_name, eFormatCategoryItemFilter, false)) {
-if (error)
-  error->SetErrorStringWithFormat("cannot add synthetic for type %s when "
-  "filter is defined in same category!",
-  type_name.AsCString());
-return false;
+  // Only check for conflicting filters in the same category if `type_name` is
+  // an actual type name. Matching a regex string against registered regexes
+  // doesn't work.
+  if (type == eRegularSynth) {
+if (category->AnyMatches(type_name, eFormatCategoryItemFilter, false)) {
+  if (error)
+error->SetErrorStringWithFormat("cannot add synthetic for type %s when "
+"filter is defined in same category!",
+type_name.AsCString());
+  return false;
+}
   }
 
   if (type == eRegexSynth) {
@@ -2458,13 +2463,18 @@
 type = eRegexFilter;
 }
 
-if (category->AnyMatches(type_name, eFormatCategoryItemSynth, false)) {
-  if (error)
-error->SetErrorStringWithFormat("cannot add filter for type %s when "
-"synthetic is defined in same "
-"category!",
-type_name.AsCString());
-  return false;
+// Only check for conflicting synthetic child providers in the same category
+// if `type_name` is an actual type name.

[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

2022-09-23 Thread Jim Ingham via Phabricator via lldb-commits

jingham added a comment.

In D134333#3812448 , @clayborg wrote:

> I just did a search through our test sources and we use 
> GetError().GetCString() 34 times in our test suites python files. So the 
> SBError::GetCString() is being misused a lot like this already and is 
> probably making some tests flaky. I would highly recommend we fix 
> SBError::GetCString() to make sure things work correctly. If people are 
> already doing this, or if they can do this with our API, then we should make 
> our API as stable as possible for users even if it costs a bit more memory in 
> our string pools.

When we return Python strings from the SWIG'ed version of 
GetError().GetCString() does the Python string we make & return copy the string 
data, or is it just holding onto the pointer.  In C/C++ if someone returns you 
a char * unless explicitly specified you assume that string is only going to 
live as long as the object that handed it out.  But a python string is 
generally self-contained, so you wouldn't expect it to lose its data.  I 
haven't checked yet, but if this is how SWIG handles making python strings from 
C strings, then this wouldn't be an error in the testsuite?  If that's now how 
it works, that might be a good way to finesse the issue.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134333/new/

https://reviews.llvm.org/D134333

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134516: [lldb] Improve display of absolute symbol lookup

2022-09-23 Thread Greg Clayton via Phabricator via lldb-commits

clayborg accepted this revision.
clayborg added a comment.

Thanks for fixing the issue!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134516/new/

https://reviews.llvm.org/D134516

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

2022-09-23 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

I just did a search through our test sources and we use GetError().GetCString() 
34 times in our test suites python files. So the SBError::GetCString() is being 
misused a lot like this already and is probably making some tests flaky. I 
would highly recommend we fix SBError::GetCString() to make sure things work 
correctly. If people are already doing this, or if they can do this with our 
API, then we should make our API as stable as possible for users even if it 
costs a bit more memory in our string pools.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134333/new/

https://reviews.llvm.org/D134333

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

2022-09-23 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

In D134041#3811289 , @labath wrote:

> In D134041#3806994 , @jasonmolenda 
> wrote:
>
>> In D134041#3805941 , @labath wrote:
>>
>>> In D134041#3805034 , 
>>> @DavidSpickett wrote:
>>>
> That said I would *love* is someone changed the RegisterInfo structs into 
> something saner, but I think that will need to be more elaborate than 
> simply stuffing a std::vector member into it. I can give you my idea of 
> how this could work, if you're interested in trying this out.

 Sure I'd be interested in that. I've just been hacking on this small part 
 of it so I don't have the full picture yet.
>>>
>>> I think that part of the problem is that nobody has a full picture of how 
>>> RegisterInfos work anymore. :)
>>>
>>> I don't claim to have this fully thought out, but the idea goes roughly 
>>> like this. For the past few years, we've been moving towards a world where 
>>> LLDB is able to fill in lots of details about the target registers. I think 
>>> we're now at a state where it is sufficient for the remote stub to specify 
>>> the register name and number, and lldb will be able to fill on most of the 
>>> details about that register: EH/DWARF/"generic" numbers, subregisters, etc. 
>>> However, this code is only invoked when communicating remote stub -- not 
>>> for core files.
>>> On one hand, this kind of makes sense -- for core files, we are the source 
>>> of the register info, so why wouldn't we provide the complete info straight 
>>> away?
>>
>> An aside, I'm working with a group inside apple that has a JTAG style 
>> debugger that can access not only the normal general purpose 
>> registers/floating point/vector registers, but control registers like 
>> AArch64's MDSCR_EL1 as one example. I haven't figured out the best format 
>> for them to express this information in a Mach-O corefile yet, but I am 
>> thinking towards a Mach-O LC_NOTE where they embed an XML register 
>> description for all the registers they can provide (and it will require that 
>> size and maybe offset are explicitly specified, at least), and a register 
>> context buffer of bytes for all of the registers.  lldb would need to 
>> augment its list of available registers with this.
>
> Thanks for jumping in Jason. I forgot about the size field. I think that we 
> need that as well. As for the offset, how do the Mach-O core files work? Are 
> all registers placed into a single note segment? (so that an "offset" is 
> well-defined). In elf, the registers are scattered in multiple notes (roughly 
> according to register sets), and we need to (arbitrarily) concatenate them so 
> that a single offset value is meaningful. But what this really confirms to me 
> that the notion of "static" register info lists is just not sufficient any 
> more.

Mach-o core files have registers in load commands (LC_THREAD) I believe. Within 
this, there is a register set flavor enum (GPR, FPU, EXC, DBG, etc), and then 
all of the registers are expected to be understood by whomever loads them and 
are given as here are N bytes for GPR. So something like:

LC_THREAD , CMDSIZE
FLAVOR, COUNT, BYTES
FLAVOR, COUNT, BYTES
ZERO

A Zero for flavor terminates the register data for a thread.

>> My vague memory is that they may have different registers available on each 
>> core ("thread") so we would need to do this on per-"thread" basis.
>
> That's interesting, but I think we should actually be in a fairly good shape 
> for that now, since in the case of ARM SVE, we can have each thread configure 
> the scalable registers with a different size.
>
>> This is all vague hand-wavy at this point, but they have the information and 
>> the debugger users want this information. At some point I'll want the 
>> corefile to be able to augment or replace lldb's register information 
>> (probably augment).
>
> Seems reasonable to me.

Typical core files seem to mostly have register data hard coded and the 
consumers are supposed to know exactly what is in register data (ELF core, 
mach-o core and minidump). Anything that gets us closer to our lldb-server 
workflow where we dynamically can figure out what the registers are is a good 
thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134041/new/

https://reviews.llvm.org/D134041

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > mstorsjo wrote:
> > > > This condition had me puzzled for a moment, until I realized that 
> > > > you're synchronizing symbol type in the other direction here. Is it 
> > > > worth clarifying this in the comment?
> > > > 
> > > > (Also, I presume the test case doesn't really trigger this case?)
> > > Hmm, I can try to improve the comment.
> > > 
> > > The `aliasInt` symbol should trigger this case. Perhaps it will be 
> > > clearer if I update the previous patch to include these symbols, so the 
> > > difference in output can be seen in this patch.
> > Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
> > expected it to return code or data. I guess this is fine then. (Changing 
> > that function, or changing the logic here to look at section types is out 
> > of scope here anyway, and I don’t know if the difference between data and 
> > invalid matters?)
> Data vs Invalid does seem to have an effect on whether a symbol can be 
> printed as data or have its address taken. I can't print or dump any 
> static/globals when the type is Invalid. I think the DWARF debug info should 
> have at least some information about these symbols, but I have no idea how 
> that works.
> 
> I wonder if I should change `MapSymbolType` to return Data for anything that 
> is not code?
I think that might be a good idea - but that's clearly a separate change from 
this one too. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134509: [LLDB][NativePDB] Let native pdb use class layout in debug info.

2022-09-23 Thread Zequan Wu via Phabricator via lldb-commits

zequanwu marked an inline comment as done.
zequanwu added a comment.

In D134509#3811203 , @labath wrote:

> It's not clear to me how much of this patch is pure refactoring, and how much 
> of it is adding new features. It would have been better to split that out 
> into two patches.
>
> I see some layout handling code in `UdtRecordCompleter` constructor, but 
> that's just two lines of code. Is that it, or should I look elsewhere?

That change in `UdtRecordCompleter` is fixing the class layout bit size. I'll 
put it in a separate patch.

Now this patch just does refactoring and hooks PdbAstBuilder to TypeSystem to 
use class layout in native pdb.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134509/new/

https://reviews.llvm.org/D134509

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134509: [LLDB][NativePDB] Let native pdb use class layout in debug info.

2022-09-23 Thread Zequan Wu via Phabricator via lldb-commits

zequanwu updated this revision to Diff 462555.
zequanwu added a comment.

Seperate refactor and the chang that fix class layout.
This just hooks PdbAstBuilder to TypeSystem to use class layout from native 
pdb, but it's still broken because the layout bitsize is missing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134509/new/

https://reviews.llvm.org/D134509

Files:
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -441,6 +441,7 @@
   // TypeSystem methods
   DWARFASTParser *GetDWARFParser() override;
   PDBASTParser *GetPDBParser() override;
+  npdb::PdbAstBuilder *GetNativePDBParser() override;
 
   // TypeSystemClang callbacks for external source lookups.
   void CompleteTagDecl(clang::TagDecl *);
@@ -1069,6 +1070,7 @@
   std::unique_ptr m_module_map_up;
   std::unique_ptr m_dwarf_ast_parser_up;
   std::unique_ptr m_pdb_ast_parser_up;
+  std::unique_ptr m_native_pdb_ast_parser_up;
   std::unique_ptr m_mangle_ctx_up;
   uint32_t m_pointer_byte_size = 0;
   bool m_ast_owned = false;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -73,6 +73,7 @@
 #include "Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h"
 #include "Plugins/SymbolFile/DWARF/DWARFASTParserClang.h"
 #include "Plugins/SymbolFile/PDB/PDBASTParser.h"
+#include "Plugins/SymbolFile/NativePDB/PdbAstBuilder.h"
 
 #include 
 
@@ -9363,6 +9364,12 @@
   return m_pdb_ast_parser_up.get();
 }
 
+npdb::PdbAstBuilder *TypeSystemClang::GetNativePDBParser() {
+  if (!m_native_pdb_ast_parser_up)
+m_native_pdb_ast_parser_up = std::make_unique(*this);
+  return m_native_pdb_ast_parser_up.get();
+}
+
 bool TypeSystemClang::LayoutRecordType(
 const clang::RecordDecl *record_decl, uint64_t _size,
 uint64_t ,
@@ -9376,6 +9383,8 @@
 importer = _dwarf_ast_parser_up->GetClangASTImporter();
   if (!importer && m_pdb_ast_parser_up)
 importer = _pdb_ast_parser_up->GetClangASTImporter();
+  if (!importer && m_native_pdb_ast_parser_up)
+importer = _native_pdb_ast_parser_up->GetClangASTImporter();
   if (!importer)
 return false;
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
@@ -312,6 +312,6 @@
   TypeSystemClang::CompleteTagDeclarationDefinition(m_derived_ct);
 
   if (auto *record_decl = llvm::dyn_cast(_tag_decl)) {
-m_ast_builder.importer().SetRecordLayout(record_decl, m_layout);
+m_ast_builder.GetClangASTImporter().SetRecordLayout(record_decl, m_layout);
   }
 }
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -19,6 +19,7 @@
 
 #include "CompileUnitIndex.h"
 #include "PdbIndex.h"
+#include "PdbAstBuilder.h"
 
 namespace clang {
 class TagDecl;
@@ -37,7 +38,6 @@
 namespace lldb_private {
 
 namespace npdb {
-class PdbAstBuilder;
 
 class SymbolFileNativePDB : public SymbolFileCommon {
   friend class UdtRecordCompleter;
@@ -137,6 +137,8 @@
   void FindFunctions(const RegularExpression , bool include_inlines,
  SymbolContextList _list) override;
 
+  llvm::Optional FindSymbolScope(PdbCompilandSymId id);
+
   void FindTypes(ConstString name, const CompilerDeclContext _decl_ctx,
  uint32_t max_matches,
  llvm::DenseSet _symbol_files,
@@ -158,8 +160,13 @@
   llvm::pdb::PDBFile () { return m_index->pdb(); }
   const llvm::pdb::PDBFile () const { return m_index->pdb(); }
 
+  PdbIndex () { return *m_index; };
+
   void DumpClangAST(Stream ) override;
 
+  llvm::Optional
+  GetParentType(llvm::codeview::TypeIndex ti);
+
 private:
   struct LineTableEntryComparator {
 bool operator()(const lldb_private::LineTable::Entry ,
@@ -180,6 +187,8 @@
 InlineSite(PdbCompilandSymId parent_id) : parent_id(parent_id){};
   };

[Lldb-commits] [PATCH] D134515: [lldb] Fix completion of 'settings set' values

2022-09-23 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added inline comments.



Comment at: lldb/test/API/functionalities/completion/TestCompletion.py:410
+"""Test that 'settins set stop-disassembly-display ' completes to [
+'never', 'always', 'no-debuginfo', 'no-source']."""
+self.complete_from_to('settings set stop-disassembly-display ',

kastiglione wrote:
> DavidSpickett wrote:
> > Could you add to the comment why this one was chosen in particular? 
> > Something like:
> > ```
> > Checks that we can complete a setting that has enum values.
> > ```
> > 
> > Since there is a `settings set` test above but clearly that wasn't enough 
> > coverage.
> done, I also renamed the test to describe why not what.
correction: to describe what, not how.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134515/new/

https://reviews.llvm.org/D134515

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

mstorsjo wrote:
> alvinhochun wrote:
> > mstorsjo wrote:
> > > This condition had me puzzled for a moment, until I realized that you're 
> > > synchronizing symbol type in the other direction here. Is it worth 
> > > clarifying this in the comment?
> > > 
> > > (Also, I presume the test case doesn't really trigger this case?)
> > Hmm, I can try to improve the comment.
> > 
> > The `aliasInt` symbol should trigger this case. Perhaps it will be clearer 
> > if I update the previous patch to include these symbols, so the difference 
> > in output can be seen in this patch.
> Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
> expected it to return code or data. I guess this is fine then. (Changing that 
> function, or changing the logic here to look at section types is out of scope 
> here anyway, and I don’t know if the difference between data and invalid 
> matters?)
Data vs Invalid does seem to have an effect on whether a symbol can be printed 
as data or have its address taken. I can't print or dump any static/globals 
when the type is Invalid. I think the DWARF debug info should have at least 
some information about these symbols, but I have no idea how that works.

I wonder if I should change `MapSymbolType` to return Data for anything that is 
not code?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134515: [lldb] Fix completion of 'settings set' values

2022-09-23 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added inline comments.



Comment at: lldb/test/API/functionalities/completion/TestCompletion.py:410
+"""Test that 'settins set stop-disassembly-display ' completes to [
+'never', 'always', 'no-debuginfo', 'no-source']."""
+self.complete_from_to('settings set stop-disassembly-display ',

DavidSpickett wrote:
> Could you add to the comment why this one was chosen in particular? Something 
> like:
> ```
> Checks that we can complete a setting that has enum values.
> ```
> 
> Since there is a `settings set` test above but clearly that wasn't enough 
> coverage.
done, I also renamed the test to describe why not what.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134515/new/

https://reviews.llvm.org/D134515

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134515: [lldb] Fix completion of 'settings set' values

2022-09-23 Thread Dave Lee via Phabricator via lldb-commits

kastiglione updated this revision to Diff 462532.
kastiglione added a comment.

Rename test and change the docstring to be meaningful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134515/new/

https://reviews.llvm.org/D134515

Files:
  lldb/source/Commands/CommandObjectSettings.cpp
  lldb/test/API/functionalities/completion/TestCompletion.py


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -405,6 +405,11 @@
   ['target.process.thread.step-avoid-regexp',
'target.process.thread.trace-thread'])
 
+def test_settings_set_can_complete_setting_enum_values(self):
+"""Checks that we can complete the values of an enum setting."""
+self.complete_from_to('settings set stop-disassembly-display ',
+  ['never', 'always', 'no-debuginfo', 'no-source'])
+
 def test_thread_plan_discard(self):
 self.build()
 (_, _, thread, _) = lldbutil.run_to_source_breakpoint(self,
Index: lldb/source/Commands/CommandObjectSettings.cpp
===
--- lldb/source/Commands/CommandObjectSettings.cpp
+++ lldb/source/Commands/CommandObjectSettings.cpp
@@ -154,7 +154,7 @@
   return;
 
 // Complete option name
-if (arg[0] != '-')
+if (arg[0] == '-')
   return;
 
 // Complete setting value


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -405,6 +405,11 @@
   ['target.process.thread.step-avoid-regexp',
'target.process.thread.trace-thread'])
 
+def test_settings_set_can_complete_setting_enum_values(self):
+"""Checks that we can complete the values of an enum setting."""
+self.complete_from_to('settings set stop-disassembly-display ',
+  ['never', 'always', 'no-debuginfo', 'no-source'])
+
 def test_thread_plan_discard(self):
 self.build()
 (_, _, thread, _) = lldbutil.run_to_source_breakpoint(self,
Index: lldb/source/Commands/CommandObjectSettings.cpp
===
--- lldb/source/Commands/CommandObjectSettings.cpp
+++ lldb/source/Commands/CommandObjectSettings.cpp
@@ -154,7 +154,7 @@
   return;
 
 // Complete option name
-if (arg[0] != '-')
+if (arg[0] == '-')
   return;
 
 // Complete setting value
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

No further objections/comments from me on this!




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

alvinhochun wrote:
> mstorsjo wrote:
> > This condition had me puzzled for a moment, until I realized that you're 
> > synchronizing symbol type in the other direction here. Is it worth 
> > clarifying this in the comment?
> > 
> > (Also, I presume the test case doesn't really trigger this case?)
> Hmm, I can try to improve the comment.
> 
> The `aliasInt` symbol should trigger this case. Perhaps it will be clearer if 
> I update the previous patch to include these symbols, so the difference in 
> output can be seen in this patch.
Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
expected it to return code or data. I guess this is fine then. (Changing that 
function, or changing the logic here to look at section types is out of scope 
here anyway, and I don’t know if the difference between data and invalid 
matters?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 8600014 - [lldb] Use Optional::{has_value, value, value_or}

2022-09-23 Thread Kazu Hirata via lldb-commits


Author: Kazu Hirata
Date: 2022-09-23T09:10:40-07:00
New Revision: 8600014b78d314e53fd941f93811492caeba2d0f

URL: 
https://github.com/llvm/llvm-project/commit/8600014b78d314e53fd941f93811492caeba2d0f
DIFF: 
https://github.com/llvm/llvm-project/commit/8600014b78d314e53fd941f93811492caeba2d0f.diff

LOG: [lldb] Use Optional::{has_value,value,value_or}

Added: 


Modified: 
lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
lldb/source/Target/PathMappingList.cpp

Removed: 




diff  --git a/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp 
b/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
index f911697f5ddc5..9e1fbcb70dc19 100644
--- a/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -240,13 +240,13 @@ void BreakpointResolverFileLine::DeduceSourceMapping(
 
 // Adding back any potentially reverse mapping stripped prefix.
 // for new_mapping_to.
-if (m_removed_prefix_opt.hasValue())
-  llvm::sys::path::append(new_mapping_to, m_removed_prefix_opt.getValue());
+if (m_removed_prefix_opt.has_value())
+  llvm::sys::path::append(new_mapping_to, m_removed_prefix_opt.value());
 
 llvm::Optional new_mapping_from_opt =
 check_suffix(sc_file_dir, request_file_dir, case_sensitive);
 if (new_mapping_from_opt) {
-  new_mapping_from = new_mapping_from_opt.getValue();
+  new_mapping_from = new_mapping_from_opt.value();
   if (new_mapping_to.empty())
 new_mapping_to = ".";
 } else {
@@ -254,7 +254,7 @@ void BreakpointResolverFileLine::DeduceSourceMapping(
   check_suffix(request_file_dir, sc_file_dir, case_sensitive);
   if (new_mapping_to_opt) {
 new_mapping_from = ".";
-llvm::sys::path::append(new_mapping_to, new_mapping_to_opt.getValue());
+llvm::sys::path::append(new_mapping_to, new_mapping_to_opt.value());
   }
 }
 

diff  --git a/lldb/source/Target/PathMappingList.cpp 
b/lldb/source/Target/PathMappingList.cpp
index d2d60822b237e..cbee5934846a0 100644
--- a/lldb/source/Target/PathMappingList.cpp
+++ b/lldb/source/Target/PathMappingList.cpp
@@ -229,7 +229,7 @@ PathMappingList::ReverseRemapPath(const FileSpec , 
FileSpec ) const {
 if (!path_ref.consume_front(it.second.GetStringRef()))
   continue;
 auto orig_file = it.first.GetStringRef();
-auto orig_style = FileSpec::GuessPathStyle(orig_file).getValueOr(
+auto orig_style = FileSpec::GuessPathStyle(orig_file).value_or(
 llvm::sys::path::Style::native);
 fixed.SetFile(orig_file, orig_style);
 AppendPathComponents(fixed, path_ref, orig_style);



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Add note to forwarder export symbols in symtab

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun updated this revision to Diff 462513.
alvinhochun retitled this revision from "[lldb][COFF] Skip forwarder export 
symbols in symtab" to "[lldb][COFF] Add note to forwarder export symbols in 
symtab".
alvinhochun edited the summary of this revision.
alvinhochun added a comment.

Changed the patch to not remove the forwarder symbol, but instead add a note to 
the symbol name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml

Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml
@@ -0,0 +1,51 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# CHECK:   UserID DSX Type File Address/Value {{.*}} SizeFlags   Name
+# CHECK-NEXT:  --
+# CHECK-NEXT:   1   X Data 0x{{[0-9a-f]+}}   0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} LoadLibrary (forwarded to kernel32.LoadLibrary)
+# CHECK-EMPTY:
+
+# Test file generated with:
+#   clang -O2 --target=x86_64-windows-msvc test.c -nostdlib -c -o test.obj
+#   lld-link -dll -out:test.dll -entry:entry -export:LoadLibrary=kernel32.LoadLibrary test.obj
+# test.c:
+#   void entry(void) {}
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:   6442450944
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:   IMAGE_SUBSYSTEM_WINDOWS_GUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+RelativeVirtualAddress: 8192
+Size:110
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  4096
+VirtualSize: 1
+SectionData: C3
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  8192
+VirtualSize: 110
+SectionData: 2820010001000100432047204B206578706F72742D666F727761726465722E632E746D702E646C6C0059204D204C6F61644C696272617279006B65726E656C33322E4C6F61644C69627261727900
+symbols: []
+...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -869,6 +869,26 @@
 llvm::cantFail(entry.getOrdinal(ordinal));
 symbol.SetID(ordinal);
 
+bool is_forwarder;
+llvm::cantFail(entry.isForwarder(is_forwarder));
+if (is_forwarder) {
+  // Forwarder exports are redirected by the loader transparently, but keep
+  // it in symtab and make a note using the symbol name.
+  llvm::StringRef forwarder_name;
+  if (auto err = entry.getForwardTo(forwarder_name)) {
+LLDB_LOG(log,
+ "ObjectFilePECOFF::AppendFromExportTable - failed to get "
+ "forwarder name of forwarder export '{0}': {1}",
+ sym_name, llvm::fmt_consume(std::move(err)));
+continue;
+  }
+  llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
+ " (forwarded to ", forwarder_name,
+ ")"};
+  symbol.GetMangled().SetDemangledName(ConstString(new_name.str()));
+  symbol.SetDemangledNameIsSynthesized(true);
+}
+
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
   LLDB_LOG(log,
@@ -886,9 +906,10 @@
 // An exported symbol may be either code or data. Guess by checking whether
 // the section containing the symbol is executable.
 symbol.SetType(lldb::eSymbolTypeData);
-if (auto section_sp = symbol.GetAddressRef().GetSection())
-  if (section_sp->GetPermissions() & ePermissionsExecutable)
-symbol.SetType(lldb::eSymbolTypeCode);
+if (!is_forwarder)
+  if (auto section_sp = symbol.GetAddressRef().GetSection())
+if (section_sp->GetPermissions() & ePermissionsExecutable)
+  symbol.SetType(lldb::eSymbolTypeCode);
 symbol.SetExternal(true);

[Lldb-commits] [PATCH] D134536: [LLDB] Add an llvm::Optional version of GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

I appreciate this is a lot of churn for code that likely "just works" (or in 
reality, "isn't used very often") but with the recent riscv changes I'd like to 
modernise this area where possible. If this is too much at least I know to look 
for smaller opportunities in future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134536/new/

https://reviews.llvm.org/D134536

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134537: [LLDB] Move MIPS64/PPC64/RISCV and misc. to optional GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett updated this revision to Diff 462494.
DavidSpickett added a comment.

Remove stray newline in riscv.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134537/new/

https://reviews.llvm.org/D134537

Files:
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
  
lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

Index: lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
===
--- lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++ lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -84,10 +84,8 @@
   const bool show_address = true;
   const bool show_bytes = true;
   const bool show_control_flow_kind = true;
-  m_inst_emulator_up->GetRegisterInfo(unwind_plan.GetRegisterKind(),
-  unwind_plan.GetInitialCFARegister(),
-  m_cfa_reg_info);
-
+  m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+  unwind_plan.GetRegisterKind(), unwind_plan.GetInitialCFARegister());
   m_fp_is_cfa = false;
   m_register_values.clear();
   m_pushed_regs.clear();
@@ -130,9 +128,8 @@
 // cache the stack pointer register number (in whatever register
 // numbering this UnwindPlan uses) for quick reference during
 // instruction parsing.
-RegisterInfo sp_reg_info;
-m_inst_emulator_up->GetRegisterInfo(
-eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, sp_reg_info);
+RegisterInfo sp_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP);
 
 // The architecture dependent condition code of the last processed
 // instruction.
@@ -172,8 +169,8 @@
 lldb::RegisterKind row_kind =
 m_unwind_plan_ptr->GetRegisterKind();
 // set m_cfa_reg_info to the row's CFA reg.
-m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
-m_cfa_reg_info);
+m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+row_kind, row_cfa_regnum);
 // set m_fp_is_cfa.
 if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
   m_fp_is_cfa = false;
@@ -219,8 +216,8 @@
   lldb::RegisterKind row_kind =
   m_unwind_plan_ptr->GetRegisterKind();
   // set m_cfa_reg_info to the row's CFA reg.
-  m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
-  m_cfa_reg_info);
+  m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+  row_kind, row_cfa_regnum);
   // set m_fp_is_cfa.
   if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
 m_fp_is_cfa = false;
@@ -623,11 +620,10 @@
   // to using SP to calculate the CFA.
   if (m_fp_is_cfa) {
 m_fp_is_cfa = false;
-RegisterInfo sp_reg_info;
 lldb::RegisterKind sp_reg_kind = eRegisterKindGeneric;
 uint32_t sp_reg_num = LLDB_REGNUM_GENERIC_SP;
-m_inst_emulator_up->GetRegisterInfo(sp_reg_kind, sp_reg_num,
-sp_reg_info);
+RegisterInfo sp_reg_info =
+*m_inst_emulator_up->GetRegisterInfo(sp_reg_kind, sp_reg_num);
 RegisterValue sp_reg_val;
 if (GetRegisterValue(sp_reg_info, sp_reg_val)) {
   m_cfa_reg_info = sp_reg_info;
Index: lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
===
--- lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
+++ lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
@@ -239,14 +239,15 @@
   Log *log = GetLog(LLDBLog::Unwind);
   LLDB_LOG(log, "EmulateLD: {0:X+8}: ld r{1}, {2}(r{3})", m_addr, rt, ids, ra);
 
-  RegisterInfo r1_info;
-  if (!GetRegisterInfo(eRegisterKindLLDB, gpr_r1_ppc64le, r1_info))
+  llvm::Optional r1_info =
+  GetRegisterInfo(eRegisterKindLLDB, gpr_r1_ppc64le);
+  if (!r1_info)
 return false;
 
   // restore SP
   Context ctx;
   ctx.type = eContextRestoreStackPointer;
-  ctx.SetRegisterToRegisterPlusOffset(r1_info, r1_info, 0);
+  ctx.SetRegisterToRegisterPlusOffset(*r1_info, *r1_info, 0);
 
   WriteRegisterUnsigned(ctx, eRegisterKindLLDB, gpr_r1_ppc64le, 0);
   LLDB_LOG(log, "EmulateLD: success!");
@@ -289,16 +290,17 @@
   }
 
   // set context
-  RegisterInfo rs_info;
-  if

[Lldb-commits] [PATCH] D134536: [LLDB] Add an llvm::Optional version of GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a reviewer: clayborg.
DavidSpickett added a subscriber: clayborg.
DavidSpickett added a comment.

@clayborg As promised on https://reviews.llvm.org/D134041.

The strategy is:

- Make the original function concrete and have it call the optional version.
- Derived classes implement the optional version.
- One by one (or rather, manageable patch by manageable patch) switch the 
callers to the optional version.
- Remove the original function.

I've stacked the changes to do that onto this one. ARM is by far the largest, 
even if it is fairly mechanical changes.

You'll see some dodgy stuff like `&(*info)` because some APIs want a 
`RegisterInfo*`, but that's another rabbit hole I do want to look at but not 
here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134536/new/

https://reviews.llvm.org/D134536

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134541: [LLDB] Remove the bool + RegisterInfo& version of GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added subscribers: frasercrmck, luismarques, apazos, sameer.abuasal, 
s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, 
atanasyan, edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook, 
johnrusso, rbar, asb, kbarton, nemanjai.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: lldb-commits, pcwang-thead, MaskRay.
Herald added a project: LLDB.
Herald added a subscriber: JDevlieghere.

All callers have been converted to the optional version.

Depends on D134540 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134541

Files:
  lldb/include/lldb/Core/EmulateInstruction.h
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.h
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h
  lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h

Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
@@ -76,8 +76,6 @@
   bool EvaluateInstruction(uint32_t options) override;
   bool TestEmulation(Stream *out_stream, ArchSpec ,
  OptionValueDictionary *test_data) override;
-  using EmulateInstruction::GetRegisterInfo;
-
   llvm::Optional GetRegisterInfo(lldb::RegisterKind reg_kind,
uint32_t reg_num) override;
 
Index: lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
===
--- lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
+++ lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
@@ -61,8 +61,6 @@
 return false;
   }
 
-  using EmulateInstruction::GetRegisterInfo;
-
   llvm::Optional GetRegisterInfo(lldb::RegisterKind reg_kind,
uint32_t reg_num) override;
 
Index: lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h
===
--- lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h
+++ lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h
@@ -72,8 +72,6 @@
 return false;
   }
 
-  using EmulateInstruction::GetRegisterInfo;
-
   llvm::Optional
   GetRegisterInfo(lldb::RegisterKind reg_kind, uint32_t reg_num) override;
 
Index: lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.h
===
--- lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.h
+++ lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.h
@@ -80,8 +80,6 @@
 return false;
   }
 
-  using EmulateInstruction::GetRegisterInfo;
-
   llvm::Optional
   GetRegisterInfo(lldb::RegisterKind reg_kind, uint32_t reg_num) override;
 
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
@@ -65,8 +65,6 @@
 return false;
   }
 
-  using EmulateInstruction::GetRegisterInfo;
-
   llvm::Optional
   GetRegisterInfo(lldb::RegisterKind reg_kind, uint32_t reg_num) override;
 
Index: lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
===
--- lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
+++ lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
@@ -135,7 +135,6 @@
   bool TestEmulation(Stream *out_stream, ArchSpec ,
  OptionValueDictionary *test_data) override;
 
-  using EmulateInstruction::GetRegisterInfo;
   llvm::Optional GetRegisterInfo(lldb::RegisterKind reg_kind,
uint32_t reg_num) override;
 
Index: lldb/source/Core/EmulateInstruction.cpp
===
--- lldb/source/Core/EmulateInstruction.cpp
+++ lldb/source/Core/EmulateInstruction.cpp
@@ -582,12 +582,3 @@
   unwind_plan.Clear();
   return false;
 }
-
-bool EmulateInstruction::GetRegisterInfo(lldb::RegisterKind reg_kind,
- uint32_t reg_num,
- RegisterInfo _info) {
-  llvm::Optional info = GetRegisterInfo(reg_kind, reg_num);
-  if (info)
-reg_info = *info;
-  return info.has_value();
-}
\ No newline at end of file
Index:

[Lldb-commits] [PATCH] D134540: [LLDB][AArch64] Move instruction emulation to optional GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Depends on D134539 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134540

Files:
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp

Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -663,9 +663,10 @@
   }
 
   Context context;
-  RegisterInfo reg_info_Rn;
-  if (GetRegisterInfo(eRegisterKindLLDB, n, reg_info_Rn))
-context.SetRegisterPlusOffset(reg_info_Rn, imm);
+  llvm::Optional reg_info_Rn =
+  GetRegisterInfo(eRegisterKindLLDB, n);
+  if (reg_info_Rn)
+context.SetRegisterPlusOffset(*reg_info_Rn, imm);
 
   if (n == GetFramePointerRegisterNumber() && d == gpr_sp_arm64 && !setflags) {
 // 'mov sp, fp' - common epilogue instruction, CFA is now in terms of the
@@ -769,24 +770,25 @@
 
   RegisterValue data_Rt;
   RegisterValue data_Rt2;
-  RegisterInfo reg_info_base;
-  RegisterInfo reg_info_Rt;
-  RegisterInfo reg_info_Rt2;
-  if (!GetRegisterInfo(eRegisterKindLLDB, gpr_x0_arm64 + n, reg_info_base))
+  llvm::Optional reg_info_base =
+  GetRegisterInfo(eRegisterKindLLDB, gpr_x0_arm64 + n);
+  if (!reg_info_base)
 return false;
 
+  llvm::Optional reg_info_Rt;
+  llvm::Optional reg_info_Rt2;
+
   if (vector) {
-if (!GetRegisterInfo(eRegisterKindLLDB, fpu_d0_arm64 + t, reg_info_Rt))
-  return false;
-if (!GetRegisterInfo(eRegisterKindLLDB, fpu_d0_arm64 + t2, reg_info_Rt2))
-  return false;
+reg_info_Rt = GetRegisterInfo(eRegisterKindLLDB, fpu_d0_arm64 + t);
+reg_info_Rt2 = GetRegisterInfo(eRegisterKindLLDB, fpu_d0_arm64 + t2);
   } else {
-if (!GetRegisterInfo(eRegisterKindLLDB, gpr_x0_arm64 + t, reg_info_Rt))
-  return false;
-if (!GetRegisterInfo(eRegisterKindLLDB, gpr_x0_arm64 + t2, reg_info_Rt2))
-  return false;
+reg_info_Rt = GetRegisterInfo(eRegisterKindLLDB, gpr_x0_arm64 + t);
+reg_info_Rt2 = GetRegisterInfo(eRegisterKindLLDB, gpr_x0_arm64 + t2);
   }
 
+  if (!reg_info_Rt || !reg_info_Rt2)
+return false;
+
   bool success = false;
   if (n == 31) {
 // CheckSPAlignment();
@@ -818,29 +820,30 @@
   context_t.type = eContextRegisterStore;
   context_t2.type = eContextRegisterStore;
 }
-context_t.SetRegisterToRegisterPlusOffset(reg_info_Rt, reg_info_base, 0);
-context_t2.SetRegisterToRegisterPlusOffset(reg_info_Rt2, reg_info_base,
+context_t.SetRegisterToRegisterPlusOffset(*reg_info_Rt, *reg_info_base, 0);
+context_t2.SetRegisterToRegisterPlusOffset(*reg_info_Rt2, *reg_info_base,
size);
 
-if (!ReadRegister(_info_Rt, data_Rt))
+if (!ReadRegister(&(*reg_info_Rt), data_Rt))
   return false;
 
-if (data_Rt.GetAsMemoryData(_info_Rt, buffer, reg_info_Rt.byte_size,
+if (data_Rt.GetAsMemoryData(&(*reg_info_Rt), buffer, reg_info_Rt->byte_size,
 eByteOrderLittle, error) == 0)
   return false;
 
-if (!WriteMemory(context_t, address + 0, buffer, reg_info_Rt.byte_size))
+if (!WriteMemory(context_t, address + 0, buffer, reg_info_Rt->byte_size))
   return false;
 
-if (!ReadRegister(_info_Rt2, data_Rt2))
+if (!ReadRegister(&(*reg_info_Rt2), data_Rt2))
   return false;
 
-if (data_Rt2.GetAsMemoryData(_info_Rt2, buffer, reg_info_Rt2.byte_size,
- eByteOrderLittle, error) == 0)
+if (data_Rt2.GetAsMemoryData(&(*reg_info_Rt2), buffer,
+ reg_info_Rt2->byte_size, eByteOrderLittle,
+ error) == 0)
   return false;
 
 if (!WriteMemory(context_t2, address + size, buffer,
- reg_info_Rt2.byte_size))
+ reg_info_Rt2->byte_size))
   return false;
   } break;
 
@@ -859,37 +862,38 @@
 context_t2.SetAddress(address + size);
 
 if (rt_unknown)
-  memset(buffer, 'U', reg_info_Rt.byte_size);
+  memset(buffer, 'U', reg_info_Rt->byte_size);
 else {
-  if (!ReadMemory(context_t, address, buffer, reg_info_Rt.byte_size))
+  if (!ReadMemory(context_t, address, buffer, reg_info_Rt->byte_size))
 return false;
 }
 
-if (data_Rt.SetFromMemoryData(_info_Rt, buffer, reg_info_Rt.byte_size,
-  eByteOrderLittle, error) == 0)
+if (data_Rt.SetFromMemoryData(&(*reg_info_Rt), buffer,
+  reg_info_Rt->byte_size, eByteOrderLittle,
+  error) == 0)
   return false;
 
 if (!vector && is_signed &&

[Lldb-commits] [PATCH] D134539: [LLDB][MIPS] Move instruction emulation to optional GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added subscribers: atanasyan, jrtc27, arichardson, sdardis.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Depends on D134538 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134539

Files:
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp

Index: lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
===
--- lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
+++ lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
@@ -1197,10 +1197,10 @@
 /* Check if this is daddiu sp, sp, imm16 */
 if (dst == dwarf_sp_mips) {
   uint64_t result = src_opd_val + imm;
-  RegisterInfo reg_info_sp;
-
-  if (GetRegisterInfo(eRegisterKindDWARF, dwarf_sp_mips, reg_info_sp))
-context.SetRegisterPlusOffset(reg_info_sp, imm);
+  llvm::Optional reg_info_sp =
+  GetRegisterInfo(eRegisterKindDWARF, dwarf_sp_mips);
+  if (reg_info_sp)
+context.SetRegisterPlusOffset(*reg_info_sp, imm);
 
   /* We are allocating bytes on stack */
   context.type = eContextAdjustStackPointer;
@@ -1229,13 +1229,12 @@
   int32_t address;
   Context bad_vaddr_context;
 
-  RegisterInfo reg_info_base;
-
   src = m_reg_info->getEncodingValue(insn.getOperand(0).getReg());
   base = m_reg_info->getEncodingValue(insn.getOperand(1).getReg());
 
-  if (!GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + base,
-   reg_info_base))
+  llvm::Optional reg_info_base =
+  GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + base);
+  if (!reg_info_base)
 return false;
 
   /* read base register */
@@ -1254,29 +1253,28 @@
 
   /* We look for sp based non-volatile register stores */
   if (nonvolatile_reg_p(src)) {
-
-RegisterInfo reg_info_src;
-
-if (!GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + src,
- reg_info_src))
+llvm::Optional reg_info_src =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + src);
+if (!reg_info_src)
   return false;
 
 Context context;
 RegisterValue data_src;
 context.type = eContextPushRegisterOnStack;
-context.SetRegisterToRegisterPlusOffset(reg_info_src, reg_info_base, 0);
+context.SetRegisterToRegisterPlusOffset(*reg_info_src, *reg_info_base, 0);
 
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(_info_base, data_src))
+if (!ReadRegister(&(*reg_info_base), data_src))
   return false;
 
-if (data_src.GetAsMemoryData(_info_src, buffer, reg_info_src.byte_size,
- eByteOrderLittle, error) == 0)
+if (data_src.GetAsMemoryData(&(*reg_info_src), buffer,
+ reg_info_src->byte_size, eByteOrderLittle,
+ error) == 0)
   return false;
 
-if (!WriteMemory(context, address, buffer, reg_info_src.byte_size))
+if (!WriteMemory(context, address, buffer, reg_info_src->byte_size))
   return false;
 
 return true;
@@ -1295,9 +1293,7 @@
   base = m_reg_info->getEncodingValue(insn.getOperand(1).getReg());
   imm = insn.getOperand(2).getImm();
 
-  RegisterInfo reg_info_base;
-  if (!GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + base,
-   reg_info_base))
+  if (GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + base))
 return false;
 
   /* read base register */
@@ -1316,17 +1312,16 @@
 
   if (nonvolatile_reg_p(src)) {
 RegisterValue data_src;
-RegisterInfo reg_info_src;
-
-if (!GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + src,
- reg_info_src))
+llvm::Optional reg_info_src =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_zero_mips + src);
+if (!reg_info_src)
   return false;
 
 Context context;
 context.type = eContextPopRegisterOffStack;
 context.SetAddress(address);
 
-return WriteRegister(context, _info_src, data_src);
+return WriteRegister(context, &(*reg_info_src), data_src);
   }
 
   return false;
@@ -1367,9 +1362,10 @@
   result = src_opd_val + rt_opd_val;
 
 Context context;
-RegisterInfo reg_info_sp;
-if (GetRegisterInfo(eRegisterKindDWARF, dwarf_sp_mips, reg_info_sp))
-  context.SetRegisterPlusOffset(reg_info_sp, rt_opd_val);
+llvm::Optional reg_info_sp =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_sp_mips);
+if (reg_info_sp)
+  context.SetRegisterPlusOffset(*reg_info_sp, rt_opd_val);
 
 /* We are allocating bytes on stack */
 context.type = eContextAdjustStackPointer;
@@ -1442,9 +1438,10 @@
   result = src_opd_val + imm9;
 
   Context context;
-  RegisterInfo reg_info_sp;
-  if (GetRegisterInfo(eRegisterKindDWARF, dwarf_sp_mips,

[Lldb-commits] [PATCH] D134538: [LLDB][ARM] Move instruction emulation to optional GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Depends on D134537 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134538

Files:
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp

Index: lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
@@ -970,13 +970,13 @@
 
 EmulateInstruction::Context context;
 context.type = EmulateInstruction::eContextPushRegisterOnStack;
-RegisterInfo reg_info;
-RegisterInfo sp_reg;
-GetRegisterInfo(eRegisterKindDWARF, dwarf_sp, sp_reg);
+llvm::Optional sp_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_sp);
 for (i = 0; i < 15; ++i) {
   if (BitIsSet(registers, i)) {
-GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + i, reg_info);
-context.SetRegisterToRegisterPlusOffset(reg_info, sp_reg, addr - sp);
+llvm::Optional reg_info =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + i);
+context.SetRegisterToRegisterPlusOffset(*reg_info, *sp_reg, addr - sp);
 uint32_t reg_value = ReadCoreReg(i, );
 if (!success)
   return false;
@@ -987,8 +987,9 @@
 }
 
 if (BitIsSet(registers, 15)) {
-  GetRegisterInfo(eRegisterKindDWARF, dwarf_pc, reg_info);
-  context.SetRegisterToRegisterPlusOffset(reg_info, sp_reg, addr - sp);
+  llvm::Optional reg_info =
+  GetRegisterInfo(eRegisterKindDWARF, dwarf_pc);
+  context.SetRegisterToRegisterPlusOffset(*reg_info, *sp_reg, addr - sp);
   const uint32_t pc = ReadCoreReg(PC_REG, );
   if (!success)
 return false;
@@ -1098,8 +1099,8 @@
 EmulateInstruction::Context context;
 context.type = EmulateInstruction::eContextPopRegisterOffStack;
 
-RegisterInfo sp_reg;
-GetRegisterInfo(eRegisterKindDWARF, dwarf_sp, sp_reg);
+llvm::Optional sp_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_sp);
 
 for (i = 0; i < 15; ++i) {
   if (BitIsSet(registers, i)) {
@@ -1115,7 +1116,7 @@
 }
 
 if (BitIsSet(registers, 15)) {
-  context.SetRegisterPlusOffset(sp_reg, addr - sp);
+  context.SetRegisterPlusOffset(*sp_reg, addr - sp);
   data = MemARead(context, addr, 4, 0, );
   if (!success)
 return false;
@@ -1185,9 +1186,9 @@
   context.type = eContextSetFramePointer;
 else
   context.type = EmulateInstruction::eContextRegisterPlusOffset;
-RegisterInfo sp_reg;
-GetRegisterInfo(eRegisterKindDWARF, dwarf_sp, sp_reg);
-context.SetRegisterPlusOffset(sp_reg, sp_offset);
+llvm::Optional sp_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_sp);
+context.SetRegisterPlusOffset(*sp_reg, sp_offset);
 
 if (!WriteRegisterUnsigned(context, eRegisterKindDWARF, dwarf_r0 + Rd,
addr))
@@ -1241,9 +1242,9 @@
   context.type = EmulateInstruction::eContextSetFramePointer;
 else
   context.type = EmulateInstruction::eContextRegisterPlusOffset;
-RegisterInfo sp_reg;
-GetRegisterInfo(eRegisterKindDWARF, dwarf_sp, sp_reg);
-context.SetRegisterPlusOffset(sp_reg, 0);
+llvm::Optional sp_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_sp);
+context.SetRegisterPlusOffset(*sp_reg, 0);
 
 if (!WriteRegisterUnsigned(context, eRegisterKindDWARF, dwarf_r0 + Rd, sp))
   return false;
@@ -1338,9 +1339,9 @@
   context.type = EmulateInstruction::eContextSetFramePointer;
 else
   context.type = EmulateInstruction::eContextRegisterPlusOffset;
-RegisterInfo dwarf_reg;
-GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + Rm, dwarf_reg);
-context.SetRegisterPlusOffset(dwarf_reg, 0);
+llvm::Optional dwarf_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + Rm);
+context.SetRegisterPlusOffset(*dwarf_reg, 0);
 
 if (!WriteCoreRegOptionalFlags(context, result, Rd, setflags))
   return false;
@@ -1557,14 +1558,14 @@
 uint64_t result = operand1 * operand2;
 
 // R[d] = result<31:0>;
-RegisterInfo op1_reg;
-RegisterInfo op2_reg;
-GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + n, op1_reg);
-GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + m, op2_reg);
+llvm::Optional op1_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + n);
+llvm::Optional op2_reg =
+GetRegisterInfo(eRegisterKindDWARF, dwarf_r0 + m);
 
 EmulateInstruction::Context context;
 context.type = eContextArithmetic;
-context.SetRegisterRegisterOperands(op1_reg, op2_reg);
+context.SetRegisterRegisterOperands(*op1_reg, *op2_reg);
 
 if (!WriteRegisterUnsigned(context,

[Lldb-commits] [PATCH] D134537: [LLDB] Move MIPS64/PPC64/RISCV and misc. to optional GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added subscribers: sunshaoce, VincentWu, vkmr, frasercrmck, evandro, 
luismarques, apazos, sameer.abuasal, s.egerton, shchenz, Jim, benna, psnobl, 
jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, atanasyan, 
edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, 
simoncook, johnrusso, rbar, asb, kbarton, arichardson, nemanjai, sdardis.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: lldb-commits, pcwang-thead, eopXD, MaskRay.
Herald added a project: LLDB.
Herald added a subscriber: JDevlieghere.

Depends on D134536 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134537

Files:
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
  
lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

Index: lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
===
--- lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++ lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -84,10 +84,8 @@
   const bool show_address = true;
   const bool show_bytes = true;
   const bool show_control_flow_kind = true;
-  m_inst_emulator_up->GetRegisterInfo(unwind_plan.GetRegisterKind(),
-  unwind_plan.GetInitialCFARegister(),
-  m_cfa_reg_info);
-
+  m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+  unwind_plan.GetRegisterKind(), unwind_plan.GetInitialCFARegister());
   m_fp_is_cfa = false;
   m_register_values.clear();
   m_pushed_regs.clear();
@@ -130,9 +128,8 @@
 // cache the stack pointer register number (in whatever register
 // numbering this UnwindPlan uses) for quick reference during
 // instruction parsing.
-RegisterInfo sp_reg_info;
-m_inst_emulator_up->GetRegisterInfo(
-eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, sp_reg_info);
+RegisterInfo sp_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP);
 
 // The architecture dependent condition code of the last processed
 // instruction.
@@ -172,8 +169,8 @@
 lldb::RegisterKind row_kind =
 m_unwind_plan_ptr->GetRegisterKind();
 // set m_cfa_reg_info to the row's CFA reg.
-m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
-m_cfa_reg_info);
+m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+row_kind, row_cfa_regnum);
 // set m_fp_is_cfa.
 if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
   m_fp_is_cfa = false;
@@ -219,8 +216,8 @@
   lldb::RegisterKind row_kind =
   m_unwind_plan_ptr->GetRegisterKind();
   // set m_cfa_reg_info to the row's CFA reg.
-  m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
-  m_cfa_reg_info);
+  m_cfa_reg_info = *m_inst_emulator_up->GetRegisterInfo(
+  row_kind, row_cfa_regnum);
   // set m_fp_is_cfa.
   if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
 m_fp_is_cfa = false;
@@ -623,11 +620,10 @@
   // to using SP to calculate the CFA.
   if (m_fp_is_cfa) {
 m_fp_is_cfa = false;
-RegisterInfo sp_reg_info;
 lldb::RegisterKind sp_reg_kind = eRegisterKindGeneric;
 uint32_t sp_reg_num = LLDB_REGNUM_GENERIC_SP;
-m_inst_emulator_up->GetRegisterInfo(sp_reg_kind, sp_reg_num,
-sp_reg_info);
+RegisterInfo sp_reg_info =
+*m_inst_emulator_up->GetRegisterInfo(sp_reg_kind, sp_reg_num);
 RegisterValue sp_reg_val;
 if (GetRegisterValue(sp_reg_info, sp_reg_val)) {
   m_cfa_reg_info = sp_reg_info;
Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -1321,7 +1321,6 @@
 
   if (reg_index >= length || reg_kind != eRegisterKindLLDB)
 return {};
-
   return array[reg_index];
 }
 
Index:

[Lldb-commits] [PATCH] D134536: [LLDB] Add an llvm::Optional version of GetRegisterInfo

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added subscribers: frasercrmck, luismarques, apazos, sameer.abuasal, 
s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, 
atanasyan, edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook, 
johnrusso, rbar, asb, kbarton, nemanjai.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: lldb-commits, pcwang-thead, MaskRay.
Herald added a project: LLDB.
Herald added a subscriber: JDevlieghere.

We have some 500 ish uses of the bool plus ref version
so changing them all at once isn't a great idea.

This adds an overload that doesn't take a RegisterInfo&
and returns an optional.

Once I'm done switching all the existing callers I'll
remove the original function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134536

Files:
  lldb/include/lldb/Core/EmulateInstruction.h
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.h
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h
  lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
  lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h

Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
@@ -76,8 +76,10 @@
   bool EvaluateInstruction(uint32_t options) override;
   bool TestEmulation(Stream *out_stream, ArchSpec ,
  OptionValueDictionary *test_data) override;
-  bool GetRegisterInfo(lldb::RegisterKind reg_kind, uint32_t reg_num,
-   RegisterInfo _info) override;
+  using EmulateInstruction::GetRegisterInfo;
+
+  llvm::Optional GetRegisterInfo(lldb::RegisterKind reg_kind,
+   uint32_t reg_num) override;
 
   lldb::addr_t ReadPC(bool );
   bool WritePC(lldb::addr_t pc);
Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -1286,9 +1286,9 @@
LLDB_REGNUM_GENERIC_PC, pc);
 }
 
-bool EmulateInstructionRISCV::GetRegisterInfo(lldb::RegisterKind reg_kind,
-  uint32_t reg_index,
-  RegisterInfo _info) {
+llvm::Optional
+EmulateInstructionRISCV::GetRegisterInfo(lldb::RegisterKind reg_kind,
+ uint32_t reg_index) {
   if (reg_kind == eRegisterKindGeneric) {
 switch (reg_index) {
 case LLDB_REGNUM_GENERIC_PC:
@@ -1320,10 +1320,9 @@
   RegisterInfoPOSIX_riscv64::GetRegisterInfoCount(m_arch);
 
   if (reg_index >= length || reg_kind != eRegisterKindLLDB)
-return false;
+return {};
 
-  reg_info = array[reg_index];
-  return true;
+  return array[reg_index];
 }
 
 bool EmulateInstructionRISCV::SetTargetTriple(const ArchSpec ) {
Index: lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
===
--- lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
+++ lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.h
@@ -61,8 +61,10 @@
 return false;
   }
 
-  bool GetRegisterInfo(lldb::RegisterKind reg_kind, uint32_t reg_num,
-   RegisterInfo _info) override;
+  using EmulateInstruction::GetRegisterInfo;
+
+  llvm::Optional GetRegisterInfo(lldb::RegisterKind reg_kind,
+   uint32_t reg_num) override;
 
   bool CreateFunctionEntryUnwind(UnwindPlan _plan) override;
 
Index: lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
===
--- lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
+++ lldb/source/Plugins/Instruction/PPC64/EmulateInstructionPPC64.cpp
@@ -58,16 +58,15 @@
   return arch.GetTriple().isPPC64();
 }
 
-static bool LLDBTableGetRegisterInfo(uint32_t reg_num, RegisterInfo _info) {
+static llvm::Optional LLDBTableGetRegisterInfo(uint32_t reg_num) {
   if (reg_num >=

[Lldb-commits] [PATCH] D134517: [lldb][COFF] Load absolute symbols from COFF symbol table

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun updated this revision to Diff 462479.
alvinhochun added a comment.

Rebased on parent


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134517/new/

https://reviews.llvm.org/D134517

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -4,9 +4,10 @@
 ## The .file symbol isn't checked, but is included to test that the symbol
 ## table iteration handles cases with a symbol with more than one aux symbol.
 
-# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
-# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Type File Address/Value {{.*}} SizeFlags   
Name
+# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
entry
+# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
absolute_symbol
 
 --- !COFF
 OptionalHeader:
@@ -123,4 +124,10 @@
 SimpleType:  IMAGE_SYM_TYPE_NULL
 ComplexType: IMAGE_SYM_DTYPE_NULL
 StorageClass:IMAGE_SYM_CLASS_EXTERNAL
+  - Name:absolute_symbol
+Value:   0xdeadbeef
+SectionNumber:   -1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
 ...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -830,6 +830,9 @@
 symbol.SetType(exported->GetType());
 }
   }
+} else if (section_number == llvm::COFF::IMAGE_SYM_ABSOLUTE) {
+  symbol.GetAddressRef() = Address(coff_sym_ref.getValue());
+  symbol.SetType(lldb::eSymbolTypeAbsolute);
 }
 symtab.AddSymbol(symbol);
   }


Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -4,9 +4,10 @@
 ## The .file symbol isn't checked, but is included to test that the symbol
 ## table iteration handles cases with a symbol with more than one aux symbol.
 
-# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
-# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
+# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} absolute_symbol
 
 --- !COFF
 OptionalHeader:
@@ -123,4 +124,10 @@
 SimpleType:  IMAGE_SYM_TYPE_NULL
 ComplexType: IMAGE_SYM_DTYPE_NULL
 StorageClass:IMAGE_SYM_CLASS_EXTERNAL
+  - Name:absolute_symbol
+Value:   0xdeadbeef
+SectionNumber:   -1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
 ...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -830,6 +830,9 @@
 symbol.SetType(exported->GetType());
 }
   }
+} else if (section_number == llvm::COFF::IMAGE_SYM_ABSOLUTE) {
+  symbol.GetAddressRef() = Address(coff_sym_ref.getValue());
+  symbol.SetType(lldb::eSymbolTypeAbsolute);
 }
 symtab.AddSymbol(symbol);
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun updated this revision to Diff 462476.
alvinhochun added a comment.

Updated test based on parent and addressed review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml

Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -4,18 +4,18 @@
 # Checks that the symtab contains both symbols from the export table and the
 # COFF symbol table.
 
-# CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
+# CHECK:  UserID DSX Type   File Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
-# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT:  1   X Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code   0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data   0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code   0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295   X Additional 0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
+# CHECK-NEXT: 4294967295   X Additional 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
+# CHECK-NEXT: 4294967295 Invalid0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -268,10 +268,12 @@
 
 private:
   bool CreateBinary();
+  typedef std::vector> rva_symbol_list_t;
   void AppendFromCOFFSymbolTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
-  void AppendFromExportTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
+ lldb_private::Symtab ,
+ const rva_symbol_list_t _exports);
+  rva_symbol_list_t AppendFromExportTable(lldb_private::SectionList *sect_list,
+  lldb_private::Symtab );
 
   dos_header_t m_dos_header;
   coff_header_t m_coff_header;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -761,12 +761,18 @@
 
 void ObjectFilePECOFF::ParseSymtab(Symtab ) {
   SectionList *sect_list = GetSectionList();
-  AppendFromExportTable(sect_list, symtab);
-  AppendFromCOFFSymbolTable(sect_list, symtab);
+  rva_symbol_list_t sorted_exports = AppendFromExportTable(sect_list, symtab);
+  AppendFromCOFFSymbolTable(sect_list, symtab, sorted_exports);
 }
 
-void ObjectFilePECOFF::AppendFromCOFFSymbolTable(SectionList *sect_list,
- Symtab )

[Lldb-commits] [PATCH] D134265: [lldb][COFF] Improve info of symbols from export table

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun updated this revision to Diff 462472.
alvinhochun added a comment.

Updated to new test from parent


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134265/new/

https://reviews.llvm.org/D134265

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -6,10 +6,10 @@
 
 # CHECK:  UserID DSX TypeFile Address/Value {{.*}} Size
Flags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT: 4294967295 D   Code0x0001800010200x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT: 4294967295 D   Code0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 D   Code0x0001800030000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 D   Code0x0001800030040x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportIntAlias
 # CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} entry
 # CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFunc
 # CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} aliasFunc
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -826,6 +826,10 @@
 // Note: symbol name may be empty if it is only exported by ordinal.
 symbol.GetMangled().SetValue(ConstString(sym_name));
 
+uint32_t ordinal;
+llvm::cantFail(entry.getOrdinal(ordinal));
+symbol.SetID(ordinal);
+
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
   LLDB_LOG(log,
@@ -834,10 +838,19 @@
sym_name, llvm::fmt_consume(std::move(err)));
   continue;
 }
+// Skip the symbol if it doesn't look valid.
+if (function_rva == 0 && sym_name.empty())
+  continue;
 symbol.GetAddressRef() =
 Address(m_coff_header_opt.image_base + function_rva, sect_list);
-symbol.SetType(lldb::eSymbolTypeCode);
-symbol.SetDebug(true);
+
+// An exported symbol may be either code or data. Guess by checking whether
+// the section containing the symbol is executable.
+symbol.SetType(lldb::eSymbolTypeData);
+if (auto section_sp = symbol.GetAddressRef().GetSection())
+  if (section_sp->GetPermissions() & ePermissionsExecutable)
+symbol.SetType(lldb::eSymbolTypeCode);
+symbol.SetExternal(true);
 symtab.AddSymbol(symbol);
   }
 }


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -6,10 +6,10 @@
 
 # CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT: 4294967295 D   Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT: 4294967295 D   Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 D   Code0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 D   Code0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
 # CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
 # CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
 # CHECK-NEXT: 4294967295 Code0x000180001020

[Lldb-commits] [PATCH] D134196: [lldb][COFF] Rewrite ParseSymtab to list both export and symbol tables

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun updated this revision to Diff 462471.
alvinhochun added a comment.

Updated the test to include more symbols used in later changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134196/new/

https://reviews.llvm.org/D134196

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
  llvm/include/llvm/Object/COFF.h

Index: llvm/include/llvm/Object/COFF.h
===
--- llvm/include/llvm/Object/COFF.h
+++ llvm/include/llvm/Object/COFF.h
@@ -914,6 +914,10 @@
 
   uint32_t getStringTableSize() const { return StringTableSize; }
 
+  const export_directory_table_entry *getExportTable() const {
+return ExportDirectory;
+  }
+
   const coff_load_configuration32 *getLoadConfig32() const {
 assert(!is64());
 return reinterpret_cast(LoadConfig);
Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -0,0 +1,109 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# Checks that the symtab contains both symbols from the export table and the
+# COFF symbol table.
+
+# CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
+# CHECK-NEXT: --
+# CHECK-NEXT: 4294967295 D   Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT: 4294967295 D   Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 D   Code0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 D   Code0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
+# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 Invalid 0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
+# CHECK-NEXT: 4294967295 Invalid 0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-EMPTY:
+
+# Test file generated with:
+#   clang -O2 --target=x86_64-windows-msvc test.c -nostdlib -c -o test.obj
+#   lld-link -debug:symtab -dll -out:test.dll -entry:entry -export:exportFnAlias=aliasFunc -export:exportIntAlias=aliasInt test.obj
+# test.c:
+#   __declspec(dllexport) int exportInt;
+#   int aliasInt;
+#   int internalInt;
+#   void entry(void) {}
+#   __declspec(dllexport) void exportFunc(void) {}
+#   void aliasFunc(void) {}
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:   6442450944
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:   IMAGE_SUBSYSTEM_WINDOWS_GUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+RelativeVirtualAddress: 8192
+Size:156
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  4096
+VirtualSize: 33
+SectionData: C32E0F1F8400C32E0F1F8400C3
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  8192
+VirtualSize: 156
+SectionData: 282001000400040042205220622073796D626F6C732D6578706F7274732E632E746D702E646C6C0020101010003004306A20782083208D200100020003006578706F7274466E416C696173006578706F727446756E63006578706F7274496E74006578706F7274496E74416C69617300
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+VirtualAddress:  12288
+VirtualSize: 12
+SectionData: ''
+symbols:
+  - Name:entry
+Value:   0
+SectionNumber:   1
+SimpleType:

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+if (symbol_type != lldb::eSymbolTypeInvalid)
+  exported->SetType(symbol_type);
+if (exported->GetMangled() == symbol.GetMangled()) {

mstorsjo wrote:
> As a curious question - since D134265 we get better quality for the symbol 
> types set from the get go - what are the other cases you foresee where this 
> will make a difference? I don't mind if there isn't any difference though, 
> syncing the types from the symbol table which is more expressible probably is 
> good anyway. Just wondering about the actual utility of it.
Given what limited info the existing implementations (LLD and GNU ld) write to 
the symbol table, we probably won't get any better type info from just the 
symbol table alone. Though I was thinking if we can use a bit of additional 
heuristics we can assign more specific symbol types.

For example, perhaps if we can guess that a particular code symbol may be an 
import thunk from the jump instruction it contains, then we can set the symbol 
as 'eSymbolTypeTrampoline'. It seems the breakpoint command will skip 
trampoline symbols, so if you, say, set a breakpoint on `memcpy` it will only 
break on the real function but not the import thunk. Not sure how feasible it 
is to implement though.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

mstorsjo wrote:
> This condition had me puzzled for a moment, until I realized that you're 
> synchronizing symbol type in the other direction here. Is it worth clarifying 
> this in the comment?
> 
> (Also, I presume the test case doesn't really trigger this case?)
Hmm, I can try to improve the comment.

The `aliasInt` symbol should trigger this case. Perhaps it will be clearer if I 
update the previous patch to include these symbols, so the difference in output 
can be seen in this patch.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

labath wrote:
> Can you have multiple symbols pointing to the same address? Make this should 
> use `stable_sort` instead?
Yes, it can happen. The ordering shouldn't affect what 
AppendFromCOFFSymbolTable does but I suppose stable_sort does make it more 
deterministic to avoid future issues down the line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun added a comment.

In D134518#3811218 , @labath wrote:

> Ok, so is there any harm in keeping them this way?
>
> The symbols may not be very useful, but I would not say that they are 
> useless. If, for whatever reason, the user finds himself inspecting the part 
> of the memory covered by the forwarder string, then with this symbol, that 
> memory would symbolicate as `forwarded_symbol+X`, which seems nice.

I guess you're right, we can keep these symbols. Though do you think it make 
sense to synthesize a demangled name for the symbol, let's say `ExportName 
(forwarded to kernel32.LoadLibrary)`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test:1
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)

mstorsjo wrote:
> DavidSpickett wrote:
> > Isn't this fixing an issue that you saw on Windows as well?
> Yes, that was the place where I observed the issue (as Windows apparently, 
> under some circumstances, can have a couple of extra threads running that 
> haven't been spawned by the code of the process itself), but it's easily 
> reproducible on any platform as long as you make sure there's more than one 
> thread running.
> 
> I copied the basis for the test from 
> `Shell/Register/x86-multithreaded-read.test` - most of the tests there under 
> `Shell/Register` seem to have such an XFAIL, not sure exactly why. (In this 
> particular test, at least the `-pthread` linker flag might be a trivial 
> platform specific difference that might break it, which would need to be 
> abstracted somewhere.)
Yeah I didn't notice the pthread flag, that could be it. Makes sense to me.



Comment at: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp:5
+  asm volatile(
+"int3\n\t"
+  );

mstorsjo wrote:
> DavidSpickett wrote:
> > This is an x86 breakpoint, right? Shame there is no `__builtin_break` 
> > (well, msvc has one apparently but no help for this).
> > 
> > I might add the AArch64 and Arm equivalent if I have some spare time.
> Yes, afaik this is a regular x86 programmatic breakpoint.
> 
> About the portability of breakpoints on ARM btw; the exact `udf #xx` code for 
> a programmatic breakpoint is OS dependent, and does differ between Linux and 
> Windows at least - see e.g. 
> https://github.com/mingw-w64/mingw-w64/commit/38c9f0318c6509110706afdc52adc12d8cc34ead.
Good point, definitely would have tripped over that. I'll double check arm64 as 
well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134037/new/

https://reviews.llvm.org/D134037

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo updated this revision to Diff 462454.
mstorsjo added a comment.

Add a code comment about the code that is changed in the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134037/new/

https://reviews.llvm.org/D134037

Files:
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
  lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,6 @@
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | 
FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2471,8 +2471,12 @@
 
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
-if (!stop_info)
-  return false;
+if (!stop_info) {
+  // If there's no stop_info, keep iterating through the other threads;
+  // it's enough that any thread has got a stop_info that indicates
+  // an abnormal stop, to consider the process to be stopped abnormally.
+  continue;
+}
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,6 @@
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2471,8 +2471,12 @@
 
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
-if (!stop_info)
-  return false;
+if (!stop_info) {
+  // If there's no stop_info, keep iterating through the other threads;
+  // it's enough that any thread has got a stop_info that indicates
+  // an abnormal stop, to consider the process to be stopped abnormally.
+  continue;
+}
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D134041#3806994 , @jasonmolenda 
wrote:

> In D134041#3805941 , @labath wrote:
>
>> In D134041#3805034 , 
>> @DavidSpickett wrote:
>>
 That said I would *love* is someone changed the RegisterInfo structs into 
 something saner, but I think that will need to be more elaborate than 
 simply stuffing a std::vector member into it. I can give you my idea of 
 how this could work, if you're interested in trying this out.
>>>
>>> Sure I'd be interested in that. I've just been hacking on this small part 
>>> of it so I don't have the full picture yet.
>>
>> I think that part of the problem is that nobody has a full picture of how 
>> RegisterInfos work anymore. :)
>>
>> I don't claim to have this fully thought out, but the idea goes roughly like 
>> this. For the past few years, we've been moving towards a world where LLDB 
>> is able to fill in lots of details about the target registers. I think we're 
>> now at a state where it is sufficient for the remote stub to specify the 
>> register name and number, and lldb will be able to fill on most of the 
>> details about that register: EH/DWARF/"generic" numbers, subregisters, etc. 
>> However, this code is only invoked when communicating remote stub -- not for 
>> core files.
>> On one hand, this kind of makes sense -- for core files, we are the source 
>> of the register info, so why wouldn't we provide the complete info straight 
>> away?
>
> An aside, I'm working with a group inside apple that has a JTAG style 
> debugger that can access not only the normal general purpose 
> registers/floating point/vector registers, but control registers like 
> AArch64's MDSCR_EL1 as one example. I haven't figured out the best format for 
> them to express this information in a Mach-O corefile yet, but I am thinking 
> towards a Mach-O LC_NOTE where they embed an XML register description for all 
> the registers they can provide (and it will require that size and maybe 
> offset are explicitly specified, at least), and a register context buffer of 
> bytes for all of the registers.  lldb would need to augment its list of 
> available registers with this.

Thanks for jumping in Jason. I forgot about the size field. I think that we 
need that as well. As for the offset, how do the Mach-O core files work? Are 
all registers placed into a single note segment? (so that an "offset" is 
well-defined). In elf, the registers are scattered in multiple notes (roughly 
according to register sets), and we need to (arbitrarily) concatenate them so 
that a single offset value is meaningful. But what this really confirms to me 
that the notion of "static" register info lists is just not sufficient any more.

> My vague memory is that they may have different registers available on each 
> core ("thread") so we would need to do this on per-"thread" basis.

That's interesting, but I think we should actually be in a fairly good shape 
for that now, since in the case of ARM SVE, we can have each thread configure 
the scalable registers with a different size.

> This is all vague hand-wavy at this point, but they have the information and 
> the debugger users want this information. At some point I'll want the 
> corefile to be able to augment or replace lldb's register information 
> (probably augment).

Seems reasonable to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134041/new/

https://reviews.llvm.org/D134041

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.

Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2443
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
+  continue;

DavidSpickett wrote:
> Please add a comment explaining why we walk all the threads.
> 
> (though in hindsight it does seem the obvious thing to do)
Sure, can do

Comment at: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test:1
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)

DavidSpickett wrote:
> Isn't this fixing an issue that you saw on Windows as well?
Yes, that was the place where I observed the issue (as Windows apparently, 
under some circumstances, can have a couple of extra threads running that 
haven't been spawned by the code of the process itself), but it's easily 
reproducible on any platform as long as you make sure there's more than one 
thread running.

I copied the basis for the test from 
`Shell/Register/x86-multithreaded-read.test` - most of the tests there under 
`Shell/Register` seem to have such an XFAIL, not sure exactly why. (In this 
particular test, at least the `-pthread` linker flag might be a trivial 
platform specific difference that might break it, which would need to be 
abstracted somewhere.)

Comment at: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp:5
+  asm volatile(
+"int3\n\t"
+  );

DavidSpickett wrote:
> This is an x86 breakpoint, right? Shame there is no `__builtin_break` (well, 
> msvc has one apparently but no help for this).
> 
> I might add the AArch64 and Arm equivalent if I have some spare time.
Yes, afaik this is a regular x86 programmatic breakpoint.

About the portability of breakpoints on ARM btw; the exact `udf #xx` code for a 
programmatic breakpoint is OS dependent, and does differ between Linux and 
Windows at least - see e.g. 
https://github.com/mingw-w64/mingw-w64/commit/38c9f0318c6509110706afdc52adc12d8cc34ead.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134037/new/

https://reviews.llvm.org/D134037

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] c831cea - [LLDB] Fix "memory region --all" when there is no ABI plugin

2022-09-23 Thread David Spickett via lldb-commits


Author: David Spickett
Date: 2022-09-23T12:32:38Z
New Revision: c831cea5efaaafaa97e3faf3119da3362868f41a

URL: 
https://github.com/llvm/llvm-project/commit/c831cea5efaaafaa97e3faf3119da3362868f41a
DIFF: 
https://github.com/llvm/llvm-project/commit/c831cea5efaaafaa97e3faf3119da3362868f41a.diff

LOG: [LLDB] Fix "memory region --all" when there is no ABI plugin

There are two conditions for the loop exit. Either we hit LLDB_INVALID_ADDRESS
or the ABI tells us we are beyond mappable memory.

I made a mistake in that second part that meant if you had no ABI plugin
--all would stop on the first loop and return nothing.

If there's no ABI plugin we should only check for LLDB_INVALID_ADDRESS.

Depends on D134029

Reviewed By: labath

Differential Revision: https://reviews.llvm.org/D134030

Added: 


Modified: 
lldb/source/Commands/CommandObjectMemory.cpp
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectMemory.cpp 
b/lldb/source/Commands/CommandObjectMemory.cpp
index 33c40a9ed8902..e42665b974156 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1821,7 +1821,7 @@ class CommandObjectMemoryRegion : public 
CommandObjectParsed {
  // When there are non-address bits the last range will not extend
  // to LLDB_INVALID_ADDRESS but to the max virtual address.
  // This prevents us looping forever if that is the case.
- (abi && (abi->FixAnyAddress(addr) == addr))) {
+ (!abi || (abi->FixAnyAddress(addr) == addr))) {
 lldb_private::MemoryRegionInfo region_info;
 error = process_sp->GetMemoryRegionInfo(addr, region_info);
 

diff  --git a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py 
b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
index 5e4c80bbb5767..2bced341d2429 100644
--- a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -160,4 +160,38 @@ def qMemoryRegionInfo(self, addr):
 interp.HandleCommand("memory region --all ", result)
 self.assertFalse(result.Succeeded())
 self.assertEqual(result.GetError(),
-"error: qMemoryRegionInfo is not supported\n")
\ No newline at end of file
+"error: qMemoryRegionInfo is not supported\n")
+
+@skipIfAsan
+def test_all_no_abi_plugin(self):
+# There are two conditions for breaking the all loop. Either we get to
+# LLDB_INVALID_ADDRESS, or the ABI plugin tells us we have got beyond
+# the mappable range. If we don't have an ABI plugin, the option 
should still
+# work and only check the first condition.
+
+class MyResponder(MockGDBServerResponder):
+def qMemoryRegionInfo(self, addr):
+if addr == 0:
+return "start:0;size:1;"
+# Goes until the end of memory.
+if addr == 0x1:
+return "start:1;size:fffe;"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+  [lldb.eStateStopped])
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+interp.HandleCommand("memory region --all ", result)
+self.assertTrue(result.Succeeded())
+self.assertEqual(result.GetOutput(),
+"[0x-0x0001) ---\n"
+"[0x0001-0x) ---\n")



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134030: [LLDB] Fix "memory region --all" when there is no ABI plugin

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc831cea5efaa: [LLDB] Fix memory region --all 
when there is no ABI plugin (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134030/new/

https://reviews.llvm.org/D134030

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/test/API/functionalities/memory-region/TestMemoryRegion.py


Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -160,4 +160,38 @@
 interp.HandleCommand("memory region --all ", result)
 self.assertFalse(result.Succeeded())
 self.assertEqual(result.GetError(),
-"error: qMemoryRegionInfo is not supported\n")
\ No newline at end of file
+"error: qMemoryRegionInfo is not supported\n")
+
+@skipIfAsan
+def test_all_no_abi_plugin(self):
+# There are two conditions for breaking the all loop. Either we get to
+# LLDB_INVALID_ADDRESS, or the ABI plugin tells us we have got beyond
+# the mappable range. If we don't have an ABI plugin, the option 
should still
+# work and only check the first condition.
+
+class MyResponder(MockGDBServerResponder):
+def qMemoryRegionInfo(self, addr):
+if addr == 0:
+return "start:0;size:1;"
+# Goes until the end of memory.
+if addr == 0x1:
+return "start:1;size:fffe;"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+  [lldb.eStateStopped])
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+interp.HandleCommand("memory region --all ", result)
+self.assertTrue(result.Succeeded())
+self.assertEqual(result.GetOutput(),
+"[0x-0x0001) ---\n"
+"[0x0001-0x) ---\n")
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1821,7 +1821,7 @@
  // When there are non-address bits the last range will not extend
  // to LLDB_INVALID_ADDRESS but to the max virtual address.
  // This prevents us looping forever if that is the case.
- (abi && (abi->FixAnyAddress(addr) == addr))) {
+ (!abi || (abi->FixAnyAddress(addr) == addr))) {
 lldb_private::MemoryRegionInfo region_info;
 error = process_sp->GetMemoryRegionInfo(addr, region_info);
 


Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -160,4 +160,38 @@
 interp.HandleCommand("memory region --all ", result)
 self.assertFalse(result.Succeeded())
 self.assertEqual(result.GetError(),
-"error: qMemoryRegionInfo is not supported\n")
\ No newline at end of file
+"error: qMemoryRegionInfo is not supported\n")
+
+@skipIfAsan
+def test_all_no_abi_plugin(self):
+# There are two conditions for breaking the all loop. Either we get to
+# LLDB_INVALID_ADDRESS, or the ABI plugin tells us we have got beyond
+# the mappable range. If we don't have an ABI plugin, the option should still
+# work and only check the first condition.
+
+class MyResponder(MockGDBServerResponder):
+def qMemoryRegionInfo(self, addr):
+if addr == 0:
+return "start:0;size:1;"
+# Goes until the end of memory.
+if addr == 0x1:
+return "start:1;size:fffe;"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+
+process = self.connect(target)
+

[Lldb-commits] [lldb] ee58200 - [LLDB] Properly return errors from "memory region --all"

2022-09-23 Thread David Spickett via lldb-commits


Author: David Spickett
Date: 2022-09-23T12:32:12Z
New Revision: ee582001bf19be8611257df7c5fc5a5e7e7da586

URL: 
https://github.com/llvm/llvm-project/commit/ee582001bf19be8611257df7c5fc5a5e7e7da586
DIFF: 
https://github.com/llvm/llvm-project/commit/ee582001bf19be8611257df7c5fc5a5e7e7da586.diff

LOG: [LLDB] Properly return errors from "memory region --all"

When I wrote the initial version I forgot that a region being
unmapped is not an error. There are real errors that we don't
want to hide, such as the remote not supporting the
qMemoryRegionInfo packet (gdbserver does not).

Reviewed By: labath

Differential Revision: https://reviews.llvm.org/D134029

Added: 


Modified: 
lldb/source/Commands/CommandObjectMemory.cpp
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectMemory.cpp 
b/lldb/source/Commands/CommandObjectMemory.cpp
index 22483e7c597c7..33c40a9ed8902 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1830,9 +1830,6 @@ class CommandObjectMemoryRegion : public 
CommandObjectParsed {
   addr = region_info.GetRange().GetRangeEnd();
 }
   }
-
-  // Even if we read nothing, don't error for --all
-  error.Clear();
 } else {
   lldb_private::MemoryRegionInfo region_info;
   error = process_sp->GetMemoryRegionInfo(load_addr, region_info);

diff  --git a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py 
b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
index 66e75e0905aed..5e4c80bbb5767 100644
--- a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -8,6 +8,8 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
+from lldbsuite.test.gdbclientutils import MockGDBServerResponder
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
 
 
 class MemoryCommandRegion(TestBase):
@@ -126,3 +128,36 @@ def test_no_overlapping_regions(self):
 
 previous_base = region_base
 previous_end = region_end
+
+class MemoryCommandRegionAll(GDBRemoteTestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_all_error(self):
+# The --all option should keep looping until the end of the memory 
range.
+# If there is an error it should be reported as if you were just asking
+# for one region. In this case the error is the remote not supporting
+# qMemoryRegionInfo.
+# (a region being unmapped is not an error, we just get a result
+# describing an unmapped range)
+class MyResponder(MockGDBServerResponder):
+def qMemoryRegionInfo(self, addr):
+# Empty string means unsupported.
+return ""
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+self.runCmd("log enable gdb-remote packets")
+self.addTearDownHook(
+  lambda: self.runCmd("log disable gdb-remote packets"))
+
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+  [lldb.eStateStopped])
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+interp.HandleCommand("memory region --all ", result)
+self.assertFalse(result.Succeeded())
+self.assertEqual(result.GetError(),
+"error: qMemoryRegionInfo is not supported\n")
\ No newline at end of file



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134029: [LLDB] Properly return errors from "memory region --all"

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGee582001bf19: [LLDB] Properly return errors from 
memory region --all (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134029/new/

https://reviews.llvm.org/D134029

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/test/API/functionalities/memory-region/TestMemoryRegion.py


Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -8,6 +8,8 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
+from lldbsuite.test.gdbclientutils import MockGDBServerResponder
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
 
 
 class MemoryCommandRegion(TestBase):
@@ -126,3 +128,36 @@
 
 previous_base = region_base
 previous_end = region_end
+
+class MemoryCommandRegionAll(GDBRemoteTestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_all_error(self):
+# The --all option should keep looping until the end of the memory 
range.
+# If there is an error it should be reported as if you were just asking
+# for one region. In this case the error is the remote not supporting
+# qMemoryRegionInfo.
+# (a region being unmapped is not an error, we just get a result
+# describing an unmapped range)
+class MyResponder(MockGDBServerResponder):
+def qMemoryRegionInfo(self, addr):
+# Empty string means unsupported.
+return ""
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+self.runCmd("log enable gdb-remote packets")
+self.addTearDownHook(
+  lambda: self.runCmd("log disable gdb-remote packets"))
+
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+  [lldb.eStateStopped])
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+interp.HandleCommand("memory region --all ", result)
+self.assertFalse(result.Succeeded())
+self.assertEqual(result.GetError(),
+"error: qMemoryRegionInfo is not supported\n")
\ No newline at end of file
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1830,9 +1830,6 @@
   addr = region_info.GetRange().GetRangeEnd();
 }
   }
-
-  // Even if we read nothing, don't error for --all
-  error.Clear();
 } else {
   lldb_private::MemoryRegionInfo region_info;
   error = process_sp->GetMemoryRegionInfo(load_addr, region_info);


Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -8,6 +8,8 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
+from lldbsuite.test.gdbclientutils import MockGDBServerResponder
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
 
 
 class MemoryCommandRegion(TestBase):
@@ -126,3 +128,36 @@
 
 previous_base = region_base
 previous_end = region_end
+
+class MemoryCommandRegionAll(GDBRemoteTestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_all_error(self):
+# The --all option should keep looping until the end of the memory range.
+# If there is an error it should be reported as if you were just asking
+# for one region. In this case the error is the remote not supporting
+# qMemoryRegionInfo.
+# (a region being unmapped is not an error, we just get a result
+# describing an unmapped range)
+class MyResponder(MockGDBServerResponder):
+def qMemoryRegionInfo(self, addr):
+# Empty string means unsupported.
+return ""
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+self.runCmd("log enable gdb-remote packets")
+self.addTearDownHook(
+  lambda: self.runCmd("log disable gdb-remote packets"))
+
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.



Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2443
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
+  continue;

Please add a comment explaining why we walk all the threads.

(though in hindsight it does seem the obvious thing to do)



Comment at: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test:1
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)

Isn't this fixing an issue that you saw on Windows as well?



Comment at: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp:5
+  asm volatile(
+"int3\n\t"
+  );

This is an x86 breakpoint, right? Shame there is no `__builtin_break` (well, 
msvc has one apparently but no help for this).

I might add the AArch64 and Arm equivalent if I have some spare time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134037/new/

https://reviews.llvm.org/D134037

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

lol, I knew about the last two parts (not that I agree with them, but I think 
we have about an equal amount of code which relies on it, and that which tries 
to work around it), but the first one is really weird. I think we have invented 
a middle ground between sign- and zero-extension.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D134333#3807511 , @clayborg wrote:

> In D134333#3805945 , @labath wrote:
>
>> Do we actually promise that strings returned by the SB API will live 
>> forever? That's not something *I* would want to promote. I always though 
>> that we're ConstStringifying the return values only when we don't know any 
>> better...
>
> Anything that does return a "const char *" should currently live forever, at 
> least that is what we have been doing so far.

I know there have been patches recently which changed some functions to return 
constified strings, but my impression is that things did not start out that 
way. From a cursory search I can find a number of (fairly old) APIs that return 
strings whose lifetime is less than "forever": SBBreakpoint::GetCondition, 
SBBreakpoint::GetThreadName, SBData::GetString, SBFrame::Disassemble, ... The 
constifying changes I remember were usually tied to us changing some `const 
char *`s into StringRefs internally, which mean we could no longer forward the 
string at the SB level (as it might not be null terminated). Constructing a 
temporary string was not possible because it would destruct before the function 
returns.

However, that's not the case here. The strings lifetime is the same as the 
enclosing SBError object, and I don't think it's unusual for c++ objects to be 
handing out pointers to parts of themselves. In this case, one could write the 
code in question as something like:

  if (SBError err = top_scope->GetError()) {
do_stuff(err.GetCString());
  }

> We don't want anyone thinking they should free the pointer.

I agree with that, and I am not aware of any situation where we do that.

The issue here is we were previously handing out a "Status::m_string.c_str()" 
pointer whose lifespan used to be tied to the SBError's lifespan, but we have a 
lot of APIs that return a SBError instance. So if you did code like:

> So that the temp objects can be use to safely grab the string. But seeing as 
> we already have the GEtCString() API, I am guessing that are probably latent 
> bugs right now due to the unexpectedly short lifespan of the string.

One can use the a temporary object to grab the string, though it is somewhat 
complicated by the fact that the result can be null. You need some wrapper like:

  optional to_optional_string(const char *s) { if (s) return string(s); 
else return nullopt; }

  ...
  auto s = to_optional_string(get_temporary_error().GetCString());

Without the null values, a plain string assignment would work fine.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134333/new/

https://reviews.llvm.org/D134333

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Andy Yankovsky via Phabricator via lldb-commits

werat added a comment.

On a somewhat related note, is it expected that `GetValueAsUnsigned()` performs 
an overflow as `int32` if the type is smaller that `int64`?

  ❯ cat overflow.cc 
  #include 
  int main() {
int8_t i8 = -1;
int32_t i32 = -1;
int64_t i64 = -1;
return 0;
  }
  
  (lldb) script
  
  >>> lldb.frame.FindVariable("i8").GetValueAsUnsigned()
  4294967295
  >>> lldb.frame.FindVariable("i32").GetValueAsUnsigned()
  4294967295
  >>> lldb.frame.FindVariable("i64").GetValueAsUnsigned()
  18446744073709551615
  >>>




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D134518#3811206 , @alvinhochun 
wrote:

> In D134518#3811160 , @labath wrote:
>
>> Where is the "dll description string" that they point to? Could they be made 
>> to point to that?
>
> The current symbol address is already pointing to it.

Ok, so is there any harm in keeping them this way?

The symbols may not be very useful, but I would not say that they are useless. 
If, for whatever reason, the user finds himself inspecting the part of the 
memory covered by the forwarder string, then with this symbol, that memory 
would symbolicate as `forwarded_symbol+X`, which seems nice.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath accepted this revision.
labath added a comment.

Seems fine (with the caveat that all I know about coroutines was learned in 
this review).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

Can you have multiple symbols pointing to the same address? Make this should 
use `stable_sort` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134037/new/

https://reviews.llvm.org/D134037

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun added a comment.

In D134518#3811155 , @mstorsjo wrote:

> In D134518#3811153 , @labath wrote:
>
>> They may not be useful (at the moment), but if they're not actively causing 
>> harm (e.g. stopping some other feature from functioning, or if we're badly 
>> misrepresenting them in the symtab output), then I think we should keep 
>> them. You never know -- maybe someone will find a use for them.
>
> Hm, maybe, but they're just plain names even without an associated address? 
> @alvinhochun If we keep them, do we need to adjust the code below to handle 
> the fact that they're addressless?

Hmm I'm not sure how. They do have an address pointing to the forwarder string, 
but they are useless as it is now.

Mind that the DLL containing the forwarder export does not actually export the 
symbol. When another EXE or DLL imports the forwarder symbol, Windows sees that 
it is a forwarder and loads the forwarded target instead. Unless a real 
exported function is imported from this DLL, it isn't even loaded into the 
process (at least that's what I gather from Process Explorer's module search 
with "api-ms" -- DLLs using this as a prefix contains only forwarder symbols to 
ucrtbase.dll or other system DLLs).

If anyone wants to list these symbols they can always do it with 
`llvm-objdump`. (`llvm-readobj` isn't handling this right now but I will make a 
patch.)

In D134518#3811160 , @labath wrote:

> Where is the "dll description string" that they point to? Could they be made 
> to point to that?

The current symbol address is already pointing to it.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134509: [LLDB][NativePDB] Let native pdb use class layout in debug info.

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

It's not clear to me how much of this patch is pure refactoring, and how much 
of it is adding new features. It would have been better to split that out into 
two patches.

I see some layout handling code in `UdtRecordCompleter` constructor, but that's 
just two lines of code. Is that it, or should I look elsewhere?




Comment at: lldb/include/lldb/Symbol/TypeSystem.h:31-32
 class DWARFASTParser;
 class PDBASTParser;
+class PdbAstBuilder;
 

Uh-oh. This is definitely not an intuitive naming scheme. How about we keep 
this class in the `lldb_private::npdb` namespace? You can forward-declare it 
there just as well.

Layering-wise, this code here is pretty bad, but moving the parser declaration 
into the global namespace is not going to make that better. And then you can 
undo all of the namespacing churn in the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134509/new/

https://reviews.llvm.org/D134509

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

This looks mostly reasonable to me, a couple discussion points only.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+if (symbol_type != lldb::eSymbolTypeInvalid)
+  exported->SetType(symbol_type);
+if (exported->GetMangled() == symbol.GetMangled()) {

As a curious question - since D134265 we get better quality for the symbol 
types set from the get go - what are the other cases you foresee where this 
will make a difference? I don't mind if there isn't any difference though, 
syncing the types from the symbol table which is more expressible probably is 
good anyway. Just wondering about the actual utility of it.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

This condition had me puzzled for a moment, until I realized that you're 
synchronizing symbol type in the other direction here. Is it worth clarifying 
this in the comment?

(Also, I presume the test case doesn't really trigger this case?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

Where is the "dll description string" that they point to? Could they be made to 
point to that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D134518#3811153 , @labath wrote:

> They may not be useful (at the moment), but if they're not actively causing 
> harm (e.g. stopping some other feature from functioning, or if we're badly 
> misrepresenting them in the symtab output), then I think we should keep them. 
> You never know -- maybe someone will find a use for them.

Hm, maybe, but they're just plain symbols even without an associated address? 
@alvinhochun If we keep them, do we need to adjust the code below to handle the 
fact that they're addressless?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

They may not be useful (at the moment), but if they're not actively causing 
harm (e.g. stopping some other feature from functioning, or if we're badly 
misrepresenting them in the symtab output), then I think we should keep them. 
You never know -- maybe someone will find a use for them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134518/new/

https://reviews.llvm.org/D134518

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd5d904285008: [lldb][TypeSystemClang] Deduce 
lldb::eEncodingUint for unsigned enum types (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:

[Lldb-commits] [lldb] d5d9042 - [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via lldb-commits


Author: Michael Buch
Date: 2022-09-23T12:27:08+02:00
New Revision: d5d90428500870107909fb8f90023ff608cd1ec2

URL: 
https://github.com/llvm/llvm-project/commit/d5d90428500870107909fb8f90023ff608cd1ec2
DIFF: 
https://github.com/llvm/llvm-project/commit/d5d90428500870107909fb8f90023ff608cd1ec2.diff

LOG: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

The motivating issue was the following:
```
$ cat main.cpp
enum class EnumVals : uint16_t {
VAL1 = 0
};

struct Foo {
EnumVals b1 : 4;
};

int main() {
// Assign value out-of-range if
// bit-field were signed
Foo f{.b1 = (EnumVals)8};

return 0; // Break here
}

(lldb) script
>>> lldb.frame.FindVariable("f").GetChildMemberWithName("b1").GetValueAsUnsigned()
4294967288
```

In the above example we observe a unsigned integer wrap-around
because we sign-extended the bit-fields underlying Scalar value
before casting it to an unsigned. The sign extension occurs because
we don't mark APSInt::IsUnsigned == true correctly when extracting
the value from memory (in Value::ResolveValue). The reason why sign
extension causes the wraparound is that the value we're assigning
to the bit-field is out-of-range (if it were a signed bit-field),
which causes `Scalar::sext` to left-fill the Scalar with 1s.

This patch corrects GetEncoding to account for unsigned enum types.
With this change the Scalar would be zero-extended instead.

This is mainly a convenience fix which well-formed code wouldn't
encounter.

rdar://99785324

Differential Revision: https://reviews.llvm.org/D134493

Added: 

lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp

Modified: 
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

Removed: 




diff  --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index e298d7fee4214..6164b978f2f24 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@ lldb::Encoding 
TypeSystemClang::GetEncoding(lldb::opaque_compiler_type_t type,
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:

diff  --git 
a/lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
 
b/lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
new file mode 100644
index 0..eb01fd6ee63a7
--- /dev/null
+++ 
b/lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())

diff  --git 
a/lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp 
b/lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
new file mode 100644
index 0..9859754d4973f
--- /dev/null
+++ b/lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Ok, sounds good then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D134344#3805953 , @Michael137 
wrote:

> that would require an audit of each API test right?

Not really. I think this could be a purely mechanical change which replaces 
`NO_DEBUG_INFO_TESTCASE = False` with something different. Ideally I'd make 
that something a "inheriting from a different class". So we could have 
something like `APITestCase` and a `DebugInfoTestCase` (inheriting from the 
first), and the tests which want debug info replication (one can also imagine 
different kinds of replication for some other tests) would inherit from the 
latter.

In D134344#3806509 , @aprantl wrote:

> But I'm also missing the context as to why this would be desirable, so if 
> there's a good reason, let me know!

I have two reasons for that:
The first is that from a simply engineering perspective, doing it the other way 
around seems cleaner, as now we kind of have two ways to avoid replication. 
Either you don't inherit from the replicated test base clase (all lldb-server 
and lldb-vscode tests do that), or you do, but then mark yourself as 
NO_DEBUG_INFO_TESTCASE.
Secondly, it seems to be that no-replication is a better default. We have a lot 
of features that don't (or shouldn't) depend on the kind of debug info we're 
using, and we're probably forgetting to add this attribute to some of them. 
It's possible those tests are adding some marginal debug info coverage, but 
it's hard to rely on that, because noone know what that is. So I'd say that an 
opt-in is a better default (particularly for the tests we're adding nowadays), 
and the replication should be done when you know you're doing something 
debug-heavy.
I also think that having a more opt-in mechanism could enable us to do *more* 
replication. For example, I think that running the some tests in both DWARF v4 
and v5 modes would be interesting, but I definitely wouldn't want to run all of 
them, all the time.

In D134344#3811091 , @Michael137 
wrote:

>> (B) Keep the `gmodules` category in the debug_info categories but add an 
>> indicator (e.g., by making the `debug_info_categories` a dictionary) that 
>> will skip replication if set. That would solve (1). And (2) would work as it 
>> does today without changes.
>
> Uploaded alternative diff that implements this option. Seems simpler since 
> tests in the `gmodules` category Just Work and we don't need to special-case 
> `gmodules` in several places
>
> https://reviews.llvm.org/D134524

It's not exactly how I was imagining this, but I like it. :)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134344/new/

https://reviews.llvm.org/D134344

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462435.
Michael137 added a comment.

- Reword commit message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462434.
Michael137 added a comment.

- Fix test description


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+We test this by assigning to a bit-field a value
+that is out-of-range of it's signed counterpart.
+I.e., with a bit-field of width 4, assigning
+8 to it would be out-of-range if we treated it
+as a signed. If LLDB were to sign-extend the Scalar
+(which shouldn't happen for unsigned bit-fields)
+it would left-fill the result with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 added a comment.

In D134493#3811097 , @labath wrote:

> In D134493#3810290 , @Michael137 
> wrote:
>
>> Wasn't sure how to properly test this since the original reproducer 
>> technically relies on implementation-defined behaviour (i.e., initialising a 
>> bitfield with an out-of-range value). Suggestions are welcome
>
> I'm probably missing something, but what exactly is undefined about that test 
> program? The number eight fits comfortably in four bits, and afaik it is a 
> valid value for the `EnumVals` type because it has a fixed underlying type.
>
> Other than that, this seems fine to me.

Ah yes, absolutely true! Misinterpreted it from some of the other reproducers.
It's enough for the value we assign (here `8`) to be out of range of the 
bit-field (if it were signed). Since we previously treated
all bit-fields of enum type as signed, that would incorrectly left-fill with 1s.
I.e., if the bit-field were of width 16, then 32768 would sign-extend 
incorrectly. I'll update the test description accordingly

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134516: [lldb] Improve display of absolute symbol lookup

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1552
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());

alvinhochun wrote:
> DavidSpickett wrote:
> > Could you use the stream's indent here instead of `" Foo:"`? You can 
> > pass a number of spaces to pad `void IndentMore(unsigned amount = 2);`.
> I was just following the pre-existing code below and also in `DumpAddress`. I 
> do have some issues with the output of the lookup command when there are 
> multiple results -- there is no separation between different results which 
> makes the output a bit hard to parse (and is why I made a semi-related patch 
> in D134111), perhaps we can try to improve it more in a separate change.
Fair enough, I'd like it to be all `stream.Indent` but the result is the same 
so that's fine.

It mostly helps if you come back later and add another level within this, but 
that doesn't seem likely here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134516/new/

https://reviews.llvm.org/D134516

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D134493#3810290 , @Michael137 
wrote:

> Wasn't sure how to properly test this since the original reproducer 
> technically relies on implementation-defined behaviour (i.e., initialising a 
> bitfield with an out-of-range value). Suggestions are welcome

I'm probably missing something, but what exactly is undefined about that test 
program? The number eight fits comfortably in four bits, and afaik it is a 
valid value for the `EnumVals` type because it has a fixed underlying type.

Other than that, this seems fine to me.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134524: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462432.
Michael137 added a comment.

- Fixup prototype


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134524/new/

https://reviews.llvm.org/D134524

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/test_categories.py


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume 
that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = 
set(test_categories.debug_info_categories)
+all_dbginfo_categories = 
set(test_categories.debug_info_categories.keys())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in 
test_categories.debug_info_categories.items() \
+  if can_replicate]
 
 for cat in categories:
 @decorators.add_test_categories([cat])


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = set(test_categories.debug_info_categories)
+all_dbginfo_categories = set(test_categories.debug_info_categories.keys())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in test_categories.debug_info_categories.items() \
+  if can_replicate]
 
 for cat in categories:
 @decorators.add_test_categories([cat])
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 added a comment.

> (B) Keep the `gmodules` category in the debug_info categories but add an 
> indicator (e.g., by making the `debug_info_categories` a dictionary) that 
> will skip replication if set. That would solve (1). And (2) would work as it 
> does today without changes.

Uploaded alternative diff that implements this option. Seems simpler since 
tests in the `gmodules` category Just Work and we don't need to special-case 
`gmodules` in several places

https://reviews.llvm.org/D134524


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134344/new/

https://reviews.llvm.org/D134344

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134524: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 created this revision.
Michael137 added reviewers: aprantl, labath.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Currently, by default LLDB runs an API test with 3 variants,
one of which, depending on platform, is `gmodules`. However,
most tests don't actually make use of any specific `gmodules`
feature since they compile a single `main.cpp` file without
imporint types from other modules.

Instead of running all tests an extra time with `-gmodules`
enabled, we plan to test `gmodules` features with dedicated
API tests that explicitly opt-into compiling with `-gmodules`.
One of the main benefits of this will be a reduction in total
API test-suite run-time (by around 1/3).

This patch adds a flag to `debug_info_categories` that indicates
whether a category is eligible to be replicated by `lldbtest`.

Keeping `gmodules` a debug-info category is desirable because
`builder.py` knows how to inject the appropriate Makefile flags
into the build command for debug-info categories already. Whereas
for non-debug-info categories we'd have to teach it how to. The
category is inferred from the test-name debug-info suffix currently.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134524

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/test_categories.py


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume 
that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = 
set(test_categories.debug_info_categories)
+all_dbginfo_categories = 
set(test_categories.debug_info_categories.values())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+categories = [category for category, can_replicate \
+  in 
test_categories.debug_info_categories.items() \
+  if can_replicate]
 
 for cat in categories:
 @decorators.add_test_categories([cat])


Index: lldb/packages/Python/lldbsuite/test/test_categories.py
===
--- lldb/packages/Python/lldbsuite/test/test_categories.py
+++ lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -13,10 +13,14 @@
 # LLDB modules
 from lldbsuite.support import gmodules
 
-
-debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
-]
+# Key: Category name
+# Value: should be used in lldbtest's debug-info replication
+debug_info_categories = {
+'dwarf': True,
+'dwo'  : True,
+'dsym' : True,
+'gmodules' : False
+}
 
 all_categories = {
 'basic_process': 'Basic process execution sniff tests.',
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1664,14 +1664,16 @@
 # If any debug info categories were explicitly tagged, assume that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = set(test_categories.debug_info_categories)
+all_dbginfo_categories = set(test_categories.debug_info_categories.values())
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
-categories = all_dbginfo_categories
+

[Lldb-commits] [PATCH] D134516: [lldb] Improve display of absolute symbol lookup

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1552
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());

DavidSpickett wrote:
> Could you use the stream's indent here instead of `" Foo:"`? You can pass 
> a number of spaces to pad `void IndentMore(unsigned amount = 2);`.
I was just following the pre-existing code below and also in `DumpAddress`. I 
do have some issues with the output of the lookup command when there are 
multiple results -- there is no separation between different results which 
makes the output a bit hard to parse (and is why I made a semi-related patch in 
D134111), perhaps we can try to improve it more in a separate change.



Comment at: lldb/test/Shell/SymbolFile/absolute-symbol.test:4
 # CHECK: 1 symbols match 'absolute_symbol'
+# CHECK:   Name: absolute_symbol
 # CHECK:   Value: 0x12345678

DavidSpickett wrote:
> This is my ignorance talking, but isn't it redundant to print the name given 
> that it's shown in the `match` line?
> 
> Perhaps that lookup isn't exact and mangling or partial matches can lead to 
> multiple matches in which case printing the line is very useful.
The lookup command can use regex filters which does usually return multiple 
matches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134516/new/

https://reviews.llvm.org/D134516

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 added a comment.

In D134493#3810991 , @DavidSpickett 
wrote:

> It wouldn't need to be same across platforms either really. Can always 
> `@skipifnotarchwhatever` and as long as it's tested somewhere that's fine. Ok 
> clang could change its mind so we mitigate that by making sure the test would 
> fail if it does, then update it as needed.

Fair enough, I added a test for this. Can skip/fix if compilers ever change 
their mind


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 462427.
Michael137 added a comment.

- Add API test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
  lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ 
lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+This test relies on implementation-defined behaviour
+of initializing a bit-field with an out-of-range value.
+On clang this will do the expected thing of writing
+the full value into the bit-field and the trailing
+padding bits. Then during sign-extension (which
+shouldn't occur for unsigned bit-fields) it would
+left-fill the resulting Scalar with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:


Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum class EnumVals : uint16_t { VAL0 = 0 };
+
+struct Foo {
+  EnumVals b : 4;
+};
+
+int main(int argc, char const *argv[], char const *envp[]) {
+  Foo f{.b = static_cast(8)};
+  return 0; //% b = self.frame().FindVariable("f").GetChildMemberWithName("b")
+//% val = b.GetValueAsUnsigned()
+//% self.assertEqual(val, 8, "Bit-field not correctly extracted")
+}
Index: lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbvalue_unsigned_enum_bitfield_value/TestSBValueUnsignedEnumBitField.py
@@ -0,0 +1,18 @@
+"""
+Test that SBValue doesn't incorrectly sign-extend
+the Scalar value of a bitfield that has an unsigned
+enum type.
+
+This test relies on implementation-defined behaviour
+of initializing a bit-field with an out-of-range value.
+On clang this will do the expected thing of writing
+the full value into the bit-field and the trailing
+padding bits. Then during sign-extension (which
+shouldn't occur for unsigned bit-fields) it would
+left-fill the resulting Scalar with 1s; we test
+for this not to happen.
+"""
+
+import lldbsuite.test.lldbinline as lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5097,7 +5097,9 @@
   case clang::Type::Record:
 break;
   case clang::Type::Enum:
-return lldb::eEncodingSint;
+return qual_type->isUnsignedIntegerOrEnumerationType()
+   ? lldb::eEncodingUint
+   : lldb::eEncodingSint;
   case clang::Type::DependentSizedArray:
   case clang::Type::DependentSizedExtVector:
   case clang::Type::UnresolvedUsing:
___
lldb-commits mailing list

[Lldb-commits] [PATCH] D134515: [lldb] Fix completion of 'settings set' values

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.



Comment at: lldb/test/API/functionalities/completion/TestCompletion.py:409
+def test_settings_set_stop_disassembly_display_value(self):
+"""Test that 'settins set stop-disassembly-display ' completes to [
+'never', 'always', 'no-debuginfo', 'no-source']."""

DavidSpickett wrote:
> settings
And these comments are rather redundant but it's the local style so it's fine 
to stick with it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134515/new/

https://reviews.llvm.org/D134515

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134516: [lldb] Improve display of absolute symbol lookup

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1552
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());

Could you use the stream's indent here instead of `" Foo:"`? You can pass a 
number of spaces to pad `void IndentMore(unsigned amount = 2);`.



Comment at: lldb/test/Shell/SymbolFile/absolute-symbol.test:4
 # CHECK: 1 symbols match 'absolute_symbol'
+# CHECK:   Name: absolute_symbol
 # CHECK:   Value: 0x12345678

This is my ignorance talking, but isn't it redundant to print the name given 
that it's shown in the `match` line?

Perhaps that lookup isn't exact and mangling or partial matches can lead to 
multiple matches in which case printing the line is very useful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134516/new/

https://reviews.llvm.org/D134516

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134515: [lldb] Fix completion of 'settings set' values

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added inline comments.



Comment at: lldb/test/API/functionalities/completion/TestCompletion.py:409
+def test_settings_set_stop_disassembly_display_value(self):
+"""Test that 'settins set stop-disassembly-display ' completes to [
+'never', 'always', 'no-debuginfo', 'no-source']."""

settings



Comment at: lldb/test/API/functionalities/completion/TestCompletion.py:410
+"""Test that 'settins set stop-disassembly-display ' completes to [
+'never', 'always', 'no-debuginfo', 'no-source']."""
+self.complete_from_to('settings set stop-disassembly-display ',

Could you add to the comment why this one was chosen in particular? Something 
like:
```
Checks that we can complete a setting that has enum values.
```

Since there is a `settings set` test above but clearly that wasn't enough 
coverage.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134515/new/

https://reviews.llvm.org/D134515

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

It wouldn't need to be same across platforms either really. Can always 
`@skipifnotarchwhatever` and as long as it's tested somewhere that's fine. Ok 
clang could change its mind so we mitigate that by making sure the test would 
fail if it does, then update it as needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

2022-09-23 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

> Wasn't sure how to properly test this since the original reproducer 
> technically relies on implementation-defined behaviour (i.e., initialising a 
> bitfield with an out-of-range value). Suggestions are welcome

Is this undefined behaviour defined at least for clang only across platforms? 
I'm guessing most of the time the test suite programs are built with clang, 
though you can change that with a cmake var. Perhaps you could do something 
like `@skipifnotclang` in that case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134493/new/

https://reviews.llvm.org/D134493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun created this revision.
alvinhochun added reviewers: labath, DavidSpickett, mstorsjo.
Herald added a project: All.
alvinhochun requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Forwarder exports do not point to a real function or variable. Instead
they point to a string describing which DLL and symbol to forward to.
Any imports which uses them will be redirected by the loader
transparently, therefore it is not necessary for LLDB to know about them
when debugging.

Depends on D134426 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134518

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -868,6 +868,18 @@
 llvm::cantFail(entry.getOrdinal(ordinal));
 symbol.SetID(ordinal);
 
+bool is_forwarder;
+llvm::cantFail(entry.isForwarder(is_forwarder));
+if (is_forwarder) {
+  // Forwarder exports are redirected by the loader transparently, so LLDB
+  // has no use for them.
+  LLDB_LOG(log,
+   "ObjectFilePECOFF::AppendFromExportTable - skipping forwarder "
+   "symbol '{0}'",
+   sym_name);
+  continue;
+}
+
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
   LLDB_LOG(log,


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -868,6 +868,18 @@
 llvm::cantFail(entry.getOrdinal(ordinal));
 symbol.SetID(ordinal);
 
+bool is_forwarder;
+llvm::cantFail(entry.isForwarder(is_forwarder));
+if (is_forwarder) {
+  // Forwarder exports are redirected by the loader transparently, so LLDB
+  // has no use for them.
+  LLDB_LOG(log,
+   "ObjectFilePECOFF::AppendFromExportTable - skipping forwarder "
+   "symbol '{0}'",
+   sym_name);
+  continue;
+}
+
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
   LLDB_LOG(log,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134517: [lldb][COFF] Load absolute symbols from COFF symbol table

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun created this revision.
alvinhochun added reviewers: labath, DavidSpickett, mstorsjo.
Herald added a project: All.
alvinhochun requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Depends on D134426 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134517

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -4,9 +4,10 @@
 ## The .file symbol isn't checked, but is included to test that the symbol
 ## table iteration handles cases with a symbol with more than one aux symbol.
 
-# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
-# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Type File Address/Value {{.*}} SizeFlags   
Name
+# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
entry
+# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
absolute_symbol
 
 --- !COFF
 OptionalHeader:
@@ -123,4 +124,10 @@
 SimpleType:  IMAGE_SYM_TYPE_NULL
 ComplexType: IMAGE_SYM_DTYPE_NULL
 StorageClass:IMAGE_SYM_CLASS_EXTERNAL
+  - Name:absolute_symbol
+Value:   0xdeadbeef
+SectionNumber:   -1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
 ...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -829,6 +829,9 @@
 symbol.SetType(exported->GetType());
 }
   }
+} else if (section_number == llvm::COFF::IMAGE_SYM_ABSOLUTE) {
+  symbol.GetAddressRef() = Address(coff_sym_ref.getValue());
+  symbol.SetType(lldb::eSymbolTypeAbsolute);
 }
 symtab.AddSymbol(symbol);
   }


Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -4,9 +4,10 @@
 ## The .file symbol isn't checked, but is included to test that the symbol
 ## table iteration handles cases with a symbol with more than one aux symbol.
 
-# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
-# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
+# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} absolute_symbol
 
 --- !COFF
 OptionalHeader:
@@ -123,4 +124,10 @@
 SimpleType:  IMAGE_SYM_TYPE_NULL
 ComplexType: IMAGE_SYM_DTYPE_NULL
 StorageClass:IMAGE_SYM_CLASS_EXTERNAL
+  - Name:absolute_symbol
+Value:   0xdeadbeef
+SectionNumber:   -1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
 ...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -829,6 +829,9 @@
 symbol.SetType(exported->GetType());
 }
   }
+} else if (section_number == llvm::COFF::IMAGE_SYM_ABSOLUTE) {
+  symbol.GetAddressRef() = Address(coff_sym_ref.getValue());
+  symbol.SetType(lldb::eSymbolTypeAbsolute);
 }
 symtab.AddSymbol(symbol);
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134516: [lldb] Improve display of absolute symbol lookup

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits

alvinhochun created this revision.
alvinhochun added reviewers: labath, clayborg.
Herald added a project: All.
alvinhochun requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When running `target module lookup` command, show the name of absolute
symbols. Also fix indentation issue after printing an absolute symbol.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134516

Files:
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/test/Shell/SymbolFile/absolute-symbol.test


Index: lldb/test/Shell/SymbolFile/absolute-symbol.test
===
--- lldb/test/Shell/SymbolFile/absolute-symbol.test
+++ lldb/test/Shell/SymbolFile/absolute-symbol.test
@@ -1,6 +1,7 @@
 # RUN: yaml2obj %s -o %t.o
 # RUN: %lldb -b -o 'target modules lookup -s absolute_symbol' %t.o | FileCheck 
%s
 # CHECK: 1 symbols match 'absolute_symbol'
+# CHECK:   Name: absolute_symbol
 # CHECK:   Value: 0x12345678
 # Created from:
 #   .globl absolute_symbol
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -1549,12 +1549,16 @@
   strm.EOL();
 } else {
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());
+  strm.EOL();
   strm.Indent("Value: ");
   strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetRawValue());
   if (symbol->GetByteSizeIsValid()) {
 strm.Indent("Size: ");
 strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetByteSize());
   }
+  strm.IndentLess();
 }
   }
 }


Index: lldb/test/Shell/SymbolFile/absolute-symbol.test
===
--- lldb/test/Shell/SymbolFile/absolute-symbol.test
+++ lldb/test/Shell/SymbolFile/absolute-symbol.test
@@ -1,6 +1,7 @@
 # RUN: yaml2obj %s -o %t.o
 # RUN: %lldb -b -o 'target modules lookup -s absolute_symbol' %t.o | FileCheck %s
 # CHECK: 1 symbols match 'absolute_symbol'
+# CHECK:   Name: absolute_symbol
 # CHECK:   Value: 0x12345678
 # Created from:
 #   .globl absolute_symbol
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -1549,12 +1549,16 @@
   strm.EOL();
 } else {
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());
+  strm.EOL();
   strm.Indent("Value: ");
   strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetRawValue());
   if (symbol->GetByteSizeIsValid()) {
 strm.Indent("Size: ");
 strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetByteSize());
   }
+  strm.IndentLess();
 }
   }
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

86 matches

Mail list logo