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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

F24632929: Screen Shot 2022-09-21 at 12.31.23 AM.png 
 Here is a screenshot showing this 
happening when a .o file is missing on Darwin for DWARF in .o file debugging.


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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> Sorry. I hoped that variant would make your life easier.

Np, one can dream (until the next out of reach c++ feature comes along).


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] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> I think the main reason that the RegisterInfo struct is such as it is, is 
> because it wants to be trivially constructible (or whatever the c++ term for 
> that is). I'm pretty sure that adding a std::string member will make it stop 
> being that, and I don't think we suddenly want to run global constructors for 
> all of the global RegisterInfo instances we have (and we have a lot of them).

Agreed. I think I stumbled into a solution for this for different reasons.

I might have mentioned adding a std::string somewhere in this review but I only 
did that as a way to test this patch actually would work.

> If you wanted to preserve the existing patterns, the right way to store a 
> string/array member inside a static struct would be to copy what is being 
> done in the value_regs/invalidate_regs field, i.e., store the value itself in 
> another static object, and then store the pointer to that object into 
> RegisterInfo. > For dynamic RegisterInfos (like those received from the 
> lldb-server), you'd use DynamicRegisterInfo and 
> DynamicRegisterInfo::Register, which have logic to persist/create storage for 
> these values (that logic would have to be extended to handle the new fields 
> as well).

Agreed.

> 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.

> So I might recommend adding a RegisterInfo member that is a "Fields *fields" 
> that can be easily set to NULL in POD initializers, but then stored in the 
> dynamic register info class in its own vector and then have the RegisterInfo 
> struct point to another POD definition of the Fields.

I though along the same lines but a different reason, I didn't want to store 
empty  in every register info that didn't have flags and 
balloon the total size.

So I have:

  struct RegisterInfo {
  <...>
/// If not nullptr, a custom type defined by XML descriptions.
/// Using shared_ptr here means that the majority of registers that don't 
have
/// custom types do not have to waste sizeof(RegisterFlags) bytes.
std::shared_ptr flags_type = nullptr;

But shared_ptr still has a destructor. It could be `RegisterFlags*`, I just 
have to work out when all that data needs to be dealocated. Probably when the 
target itself destructs but I need to figure that out, in general i don't know 
yet where the best place for these flags/fields to live is.

If it is a plain pointer then this patch is redundant but hey I learned how to 
use placement new :)


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] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196
+  struct RegisterPlusOffsetStruct {
 RegisterInfo reg;  // base register
 int64_t signed_offset; // signed offset added to base register

labath wrote:
> I actually think that the simplest solution here would be to store the 
> RegisterInfos as pointers. Copying them around doesn't make sense, 
> particularly if their size is about to grow.
True, but sounds like we're going toward adding a pointer to the info. So that 
should keep the size constant.



Comment at: lldb/include/lldb/Core/EmulateInstruction.h:342-345
+  info.~ContextUnion();
+  info.info_type = eInfoTypeRegisterRegisterOperands;
+  new (&info.RegisterRegisterOperands.operand1) RegisterInfo(op1_reg);
+  new (&info.RegisterRegisterOperands.operand2) RegisterInfo(op2_reg);

labath wrote:
> I am not a C++ expert, but I have a strong suspicion that this is not 
> correct. You're destroyed the whole object, but then are only recreating its 
> individual submembers. I get that these are the only members which have 
> non-trivial constructors, but I don't think that's how c++ works. I can 
> confirm this with some c++ experts if you want, but I am pretty sure that you 
> need to recreate the ContextUnion object as a whole here to avoid straying 
> into UB territory.
I will check the rules here, it's not too hard to change anyway and it would 
mean I don't forget one of the members.



Comment at: lldb/include/lldb/lldb-private-types.h:67-77
+  RegisterInfo() { kinds.fill(LLDB_INVALID_REGNUM); }
+
+  RegisterInfo(const char *name, const char *alt_name, uint32_t byte_size,
+   uint32_t byte_offset, lldb::Encoding encoding,
+   lldb::Format format,
+   std::array kinds,
+   uint32_t *value_regs, uint32_t *invalidate_regs)

labath wrote:
> Is this class still trivially constructible (not requiring dynamic 
> initialization)?
Current state, no. Due to the constructor setting `kinds`. Which is my attempt 
to remove the `memset` that was used elsewhere to do this, which sort of 
implies it was never trivially constructable really. Unless you knew you 
weren't going to read certain fields.

Given that:
```
#define LLDB_INVALID_REGNUM UINT32_MAX
```

And the memsets all used 0. At the very least it's opaque what the state of 
RegisterInfo is supposed to be.

That said I looked again at kNumRegisterKinds and it only goes up to 5. So I 
could just write this as:
```
uint32_t kinds[lldb::kNumRegisterKinds] = {LLDB_INVALID_REGNUM ... };
```

To avoid all this (I think I mixed it up with another enum with 100s of 
members).



Comment at: 
lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp:785-787
 bool EmulateInstructionARM::GetRegisterInfo(lldb::RegisterKind reg_kind,
 uint32_t reg_num,
 RegisterInfo ®_info) {

clayborg wrote:
> Do we want to switch this over to returning a llvm::Optional as 
> well in the base class?
I will try this as a separate change, unconnected to this. In general any time 
we can change bool+ref to optional, I'll take it.


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] D134265: [lldb][COFF] Improve info of symbols from export table

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

Looks good to me in general. Do any of the changes make a functional 
difference, or is it just improved (and less misleading) display to the user of 
the debugger?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134265

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

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

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

This looks good to me, nothing to add from my PoV. Nice to have this condensed 
to a small enough rewrite, with few enough functional changes to wrap one's 
head around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134196

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

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

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

I think marking the symbol as Data instead of Code does have an effect on 
commands that look for functions (e.g. "disassemble" will not disassemble a 
Code symbol, which makes sense). For the rest of the changes I think they are 
only visible in the symtab dump.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134265

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

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

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

I think marking the symbol as Data instead of Code does have an effect on 
commands that look for functions (e.g. "disassemble" will not disassemble a 
Data symbol, which makes sense). For the rest of the changes I think they are 
only visible in the symtab dump.

(Sorry, I got confused and made a mistake in my previous comment.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134265

___
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

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added reviewers: labath, aprantl.
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 the `@gmodules_test` decorator which test cases
will be decorated with to specify that we're running a `gmodules`
test. The decorator serves following purposes:

1. Will skip the test on unsupported platforms
2. Add a single debug-info variant to the test-category such that we don't run 
a `gmodules` test multiple times

To enable compilation with `-gmodules`, the `MAKE_GMODULES`
flag will be needed in the test's `Makefile`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134344

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py


Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -21,10 +21,10 @@
 from . import test_categories
 from . import lldbtest_config
 from lldbsuite.support import funcutils
+from lldbsuite.support import gmodules
 from lldbsuite.test import lldbplatform
 from lldbsuite.test import lldbplatformutil
 
-
 class DecorateMode:
 Skip, Xfail = range(2)
 
@@ -390,6 +390,25 @@
 
 return skipTestIfFn(should_skip_simulator_test)
 
+def gmodules_test(func):
+"""
+Use this decorator for 'gmodules'-specific tests. This decorator
+makes sure we skip the test if 'gmodules' is unsupported on the
+host platforma and adds a single debug_info category to the test
+so we don't run it multiple times for each debug_info variant.
+"""
+def should_skip_gmodules_test(self):
+supported_platforms = ["darwin", "macosx", "ios", "watchos", "tvos", 
"bridgeos"]
+platform = lldbplatformutil.getHostPlatform()
+compiler_path = self.getCompiler()
+if platform not in supported_platforms:
+return "Tests not supported on " % 
lldbplatformutil.getHostPlatform()
+elif not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+return "-gmodules option is not supported on " % compiler_path
+else:
+return None
+
+return 
skipTestIfFn(should_skip_gmodules_test)(add_test_categories(["dwarf"])(func))
 
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""


Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -21,10 +21,10 @@
 from . import test_categories
 from . import lldbtest_config
 from lldbsuite.support import funcutils
+from lldbsuite.support import gmodules
 from lldbsuite.test import lldbplatform
 from lldbsuite.test import lldbplatformutil
 
-
 class DecorateMode:
 Skip, Xfail = range(2)
 
@@ -390,6 +390,25 @@
 
 return skipTestIfFn(should_skip_simulator_test)
 
+def gmodules_test(func):
+"""
+Use this decorator for 'gmodules'-specific tests. This decorator
+makes sure we skip the test if 'gmodules' is unsupported on the
+host platforma and adds a single debug_info category to the test
+so we don't run it multiple times for each debug_info variant.
+"""
+def should_skip_gmodules_test(self):
+supported_platforms = ["darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]
+platform = lldbplatformutil.getHostPlatform()
+compiler_path = self.getCompiler()
+if platform not in supported_platforms:
+return "Tests not supported on " % lldbplatformutil.getHostPlatform()
+elif not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+return "-gmodules option is not supported on " % compiler_path
+else:
+return None
+
+return skipTestIfFn(should_skip_gmodules_test)(add_test_categories(["dwarf"])(func))
 
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134345: [WIP][lldb][test] 2 - Remove gmodules debug_info variant from lldbsuite

[Michael Buch via Phabricator via lldb-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134345

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  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
@@ -10,12 +10,8 @@
 
 # Third-party modules
 
-# LLDB modules
-from lldbsuite.support import gmodules
-
-
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
+'dwarf', 'dwo', 'dsym'
 ]
 
 all_categories = {
@@ -30,7 +26,6 @@
 'expression': 'Tests related to the expression parser',
 'flakey': 'Flakey test cases, i.e. tests that do not reliably pass at each 
execution',
 'fork': 'Tests requiring the process plugin fork/vfork event support',
-'gmodules': 'Tests that can be run with -gmodules debug information',
 'instrumentation-runtime': 'Tests for the instrumentation runtime plugins',
 'libc++': 'Test for libc++ data formatters',
 'libstdcxx': 'Test for libstdcxx data formatters',
@@ -62,11 +57,6 @@
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios", "watchos", "tvos", 
"bridgeos"]
-elif category == "gmodules":
-# First, check to see if the platform can even support gmodules.
-if platform not in ["darwin", "macosx", "ios", "watchos", "tvos", 
"bridgeos"]:
-return False
-return gmodules.is_compiler_clang_with_gmodules(compiler_path)
 return True
 
 
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
@@ -544,7 +544,9 @@
 $(DSYM) : $(EXE)
 ifeq "$(OS)" "Darwin"
 ifneq "$(MAKE_DSYM)" "NO"
+ifneq "$(MAKE_GMODULES)" "YES"
"$(DS)" $(DSFLAGS) -o "$(DSYM)" "$(EXE)"
+endif
 else
 endif
 else
@@ -587,10 +589,12 @@
$(CODESIGN) -s - "$(DYLIB_FILENAME)"
 endif
 ifneq "$(MAKE_DSYM)" "NO"
+ifneq "$(MAKE_GMODULES)" "YES"
 ifneq "$(DS)" ""
"$(DS)" $(DSFLAGS) "$(DYLIB_FILENAME)"
 endif
 endif
+endif
 else
$(LD) $(DYLIB_OBJECTS) $(LDFLAGS) -shared -o "$(DYLIB_FILENAME)"
 ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
Index: lldb/packages/Python/lldbsuite/test/builders/builder.py
===
--- lldb/packages/Python/lldbsuite/test/builders/builder.py
+++ lldb/packages/Python/lldbsuite/test/builders/builder.py
@@ -133,8 +133,6 @@
 return ["MAKE_DSYM=NO"]
 if debug_info == "dwo":
 return ["MAKE_DSYM=NO", "MAKE_DWO=YES"]
-if debug_info == "gmodules":
-return ["MAKE_DSYM=NO", "MAKE_GMODULES=YES"]
 return None
 
 def getBuildCommand(self, debug_info, architecture=None, compiler=None,


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
@@ -10,12 +10,8 @@
 
 # Third-party modules
 
-# LLDB modules
-from lldbsuite.support import gmodules
-
-
 debug_info_categories = [
-'dwarf', 'dwo', 'dsym', 'gmodules'
+'dwarf', 'dwo', 'dsym'
 ]
 
 all_categories = {
@@ -30,7 +26,6 @@
 'expression': 'Tests related to the expression parser',
 'flakey': 'Flakey test cases, i.e. tests that do not reliably pass at each execution',
 'fork': 'Tests requiring the process plugin fork/vfork event support',
-'gmodules': 'Tests that can be run with -gmodules debug information',
 'instrumentation-runtime': 'Tests for the instrumentation runtime plugins',
 'libc++': 'Test for libc++ data formatters',
 'libstdcxx': 'Test for libstdcxx data formatters',
@@ -62,11 +57,6 @@
 return platform in ["linux", "freebsd"]
 elif category == "dsym":
 return platform in ["darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]
-elif category == "gmodules":
-# First, check to see if the platform can even support gmodules.
-if platform not in ["darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]:
-return False
-return gmodules.is_compiler_clang_with_gmodules(compiler_path)
 return True
 
 
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbs

[Lldb-commits] [PATCH] D134346: [WIP][lldb][test] 3 - Add gmodules decorator to API tests

[Michael Buch via Phabricator via lldb-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134346

Files:
  lldb/test/API/commands/expression/import_builtin_fileid/Makefile
  
lldb/test/API/commands/expression/import_builtin_fileid/TestImportBuiltinFileID.py
  lldb/test/API/commands/expression/namespace_local_var_same_name_obj_c/Makefile
  
lldb/test/API/commands/expression/namespace_local_var_same_name_obj_c/TestNamespaceLocalVarSameNameObjC.py
  
lldb/test/API/commands/expression/persist_objc_pointeetype/TestPersistObjCPointeeType.py
  lldb/test/API/commands/expression/top-level/TestTopLevelExprs.py
  lldb/test/API/functionalities/target_var/TestTargetVar.py
  lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
  
lldb/test/API/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/Makefile
  
lldb/test/API/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/TestBreakpointInMemberFuncWNonPrimitiveParams.py
  lldb/test/API/lang/cpp/gmodules/basic/Makefile
  lldb/test/API/lang/cpp/gmodules/basic/TestWithModuleDebugging.py
  lldb/test/API/lang/cpp/gmodules/template-with-same-arg/Makefile
  
lldb/test/API/lang/cpp/gmodules/template-with-same-arg/TestTemplateWithSameArg.py
  lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
  lldb/test/API/lang/objc/modules-app-update/Makefile
  lldb/test/API/lang/objc/modules-app-update/TestClangModulesAppUpdate.py
  lldb/test/API/lang/objc/modules-hash-mismatch/Makefile
  lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
  lldb/test/API/lang/objc/modules-incomplete/Makefile
  lldb/test/API/lang/objc/modules-incomplete/TestIncompleteModules.py
  lldb/test/API/lang/objc/modules-inline-functions/Makefile
  lldb/test/API/lang/objc/modules-inline-functions/TestModulesInlineFunctions.py
  lldb/test/API/lang/objc/modules-update/Makefile
  lldb/test/API/lang/objc/modules-update/TestClangModulesUpdate.py
  lldb/test/API/lang/objc/objc-struct-argument/Makefile
  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
@@ -19,7 +19,7 @@
 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
+@gmodules_test
 def test_with_python_api(self):
 """Test passing structs to Objective-C methods."""
 self.build()
Index: lldb/test/API/lang/objc/objc-struct-argument/Makefile
===
--- lldb/test/API/lang/objc/objc-struct-argument/Makefile
+++ lldb/test/API/lang/objc/objc-struct-argument/Makefile
@@ -1,3 +1,4 @@
+MAKE_GMODULES = YES
 OBJC_SOURCES := test.m
 LD_EXTRAS := -lobjc -framework Foundation
 
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"]))
+@gmodules_test
 @skipIfDarwin # rdar://76540904
 def test_expr(self):
 with open(self.getBuildArtifact("module.modulemap"), "w") as f:
Index: lldb/test/API/lang/objc/modules-update/Makefile
===
--- lldb/test/API/lang/objc/modules-update/Makefile
+++ lldb/test/API/lang/objc/modules-update/Makefile
@@ -1,3 +1,4 @@
+MAKE_GMODULES = YES
 CFLAGS_EXTRAS = -I$(BUILDDIR)
 USE_PRIVATE_MODULE_CACHE = YES
 include Makefile.rules
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"]))
+@skipIf(macos_version=["<", "10.12"])
+@gmodules_test
 def test_expr(self):
 self.build()
 exe = self.getBuildArtifact("a.out")
Index: lldb/test/API/lang/objc/modules-inline-functions/Makefile
===
--- lldb/test/API/lang/objc/modules-inline-functions/Makefile
+

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

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

An alternative would be to keep the `debug_info` category and simply flag it as 
not to be run by default unless specified explicitly in the test. That way we 
wouldn't need the `MAKE_GMODULES` in the Makefile and the special `MAKE_DSYM` 
rule in https://reviews.llvm.org/D134345


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] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Michael Buch via Phabricator via lldb-commits]

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

- Fix typo in comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134344

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py


Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -21,10 +21,10 @@
 from . import test_categories
 from . import lldbtest_config
 from lldbsuite.support import funcutils
+from lldbsuite.support import gmodules
 from lldbsuite.test import lldbplatform
 from lldbsuite.test import lldbplatformutil
 
-
 class DecorateMode:
 Skip, Xfail = range(2)
 
@@ -390,6 +390,25 @@
 
 return skipTestIfFn(should_skip_simulator_test)
 
+def gmodules_test(func):
+"""
+Use this decorator for 'gmodules'-specific tests. This decorator
+makes sure we skip the test if 'gmodules' is unsupported on the
+host platform and adds a single debug_info category to the test
+so we don't run it multiple times (i.e., once for each debug_info variant).
+"""
+def should_skip_gmodules_test(self):
+supported_platforms = ["darwin", "macosx", "ios", "watchos", "tvos", 
"bridgeos"]
+platform = lldbplatformutil.getHostPlatform()
+compiler_path = self.getCompiler()
+if platform not in supported_platforms:
+return "Tests not supported on " % 
lldbplatformutil.getHostPlatform()
+elif not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+return "-gmodules option is not supported on " % compiler_path
+else:
+return None
+
+return 
skipTestIfFn(should_skip_gmodules_test)(add_test_categories(["dwarf"])(func))
 
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""


Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -21,10 +21,10 @@
 from . import test_categories
 from . import lldbtest_config
 from lldbsuite.support import funcutils
+from lldbsuite.support import gmodules
 from lldbsuite.test import lldbplatform
 from lldbsuite.test import lldbplatformutil
 
-
 class DecorateMode:
 Skip, Xfail = range(2)
 
@@ -390,6 +390,25 @@
 
 return skipTestIfFn(should_skip_simulator_test)
 
+def gmodules_test(func):
+"""
+Use this decorator for 'gmodules'-specific tests. This decorator
+makes sure we skip the test if 'gmodules' is unsupported on the
+host platform and adds a single debug_info category to the test
+so we don't run it multiple times (i.e., once for each debug_info variant).
+"""
+def should_skip_gmodules_test(self):
+supported_platforms = ["darwin", "macosx", "ios", "watchos", "tvos", "bridgeos"]
+platform = lldbplatformutil.getHostPlatform()
+compiler_path = self.getCompiler()
+if platform not in supported_platforms:
+return "Tests not supported on " % lldbplatformutil.getHostPlatform()
+elif not gmodules.is_compiler_clang_with_gmodules(compiler_path):
+return "-gmodules option is not supported on " % compiler_path
+else:
+return None
+
+return skipTestIfFn(should_skip_gmodules_test)(add_test_categories(["dwarf"])(func))
 
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am afraid that this patch misses one method of initiating a debug session -- 
connecting to a running debug server (`process connect`, 
`SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in case 
of a reconnect. This isn't a particularly common use case (and the only reason 
I've noticed it is that for `PlatformQemuUser`, all "launches" are actually 
"connects" under the hood 
),
 but I've verified that this problem can be reproduced by issuing connect 
commands manually (on the regular host platform). I'm pretty sure that was not 
intentional.

Fixing this by adding another callout to `ResetBreakpointHitCounts` would be 
easy enough, but I'm also thinking if there isn't a better place from which to 
call this function, one that would capture all three scenarios in a single 
statement. I think that one such place could be `Target::CreateProcess`. This 
function is called by all three code paths, and it's a very good indicator that 
we will be starting a new debug session.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133858

___
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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

celebration_balloons 


I like this a lot. Incidentally, I believe the main reason these tests don't 
work on non-darwin platforms is because their system libraries are not 
modularized. If you make the gmodules tests self-contained (which I would 
recommend, for the sake of reproducibility, anyway), then I think a lot of 
these tests could run elsewhere as well.

In D134344#3805270 , @Michael137 
wrote:

> An alternative would be to keep the `debug_info` category and simply flag it 
> as not to be run by default unless specified explicitly in the test. That way 
> we wouldn't need the `MAKE_GMODULES` in the Makefile and the special 
> `MAKE_DSYM` rule in https://reviews.llvm.org/D134345

Avoiding the MAKE_GMODULES repetition would definitely be nice, but I might 
try(*) do it slightly differently:

- keep `gmodules` as a category, but not a *debug info* category. Among other 
things this enables running all gmodules tests with the `--category gmodules` 
flag.
- teach the debug info replication to ignore tests with the gmodules category 
(just like it does for `@no_debug_info_test_case` tests). This step wouldn't be 
necessary if we made debug info replication opt-in instead of opt-out, as 
discussed on one of the previous patches (@JDevlieghere might remember which 
one it was)
- teach `buildDefault` to build with gmodules enabled in case the test is 
annotated with the gmodules category.

(*) I'm not entirely sure how this would work out, but I think it should be 
fine.


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] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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? 
However, it means that the information that "`ah` is a one byte sub-register of 
`rax` at offset 1" is repeated multiple times (we currently have three core 
file plugins, times the number of architectures they support). If we made core 
file register infos go through this "augmentation" process, then we could unify 
our core file/live process flow more, and relieve the core file plugins of the 
burden of dealing with the subregisters -- all they'd need to know is how to 
read/write whole registers, and the generic code would take care of all the 
subregisters.
This would also mean that *all* register infos being handled by generic code 
would be DynamicRegisterInfos, which means we could drop the avoid this POD 
business, and just replace that class with something modern and easy to use. 
The only place where we would need to store static arrays would be in the 
bowels of individual plugins, but these would be simpler than the current 
RegisterInfo struct, as a lot of this info would be deduced (maybe including 
the register type information that you're trying to introduce), and we might 
even have each plugin store the info in whichever format it sees fit -- the 
only requirement would be that a DynamicRegisterInfo comes out at the end. Some 
plugins may choose not to store static info at all, as we're already running 
into the limits of what can be stored statically -- if an architecture has 
multiple optional registers sets (whose presence is only known at runtime), 
then its impossible to stuff those registers into a static array -- I believe 
all our AArch64 registers are currently dynamic for this reason.

I know this is somewhat vague, but that's why this is just an idea. Someone 
would have to try it out to find all the issues and figure them out. I can try 
to help if you want to take it on.




Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196
+  struct RegisterPlusOffsetStruct {
 RegisterInfo reg;  // base register
 int64_t signed_offset; // signed offset added to base register

DavidSpickett wrote:
> labath wrote:
> > I actually think that the simplest solution here would be to store the 
> > RegisterInfos as pointers. Copying them around doesn't make sense, 
> > particularly if their size is about to grow.
> True, but sounds like we're going toward adding a pointer to the info. So 
> that should keep the size constant.
It should, but regardless of that, I'm surprised to see the structs being 
stored here, I'm not aware of any other place which stores RegisterInfos be 
value.


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] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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...


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] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

Thanks for reviewing

> - keep `gmodules` as a category, but not a *debug info* category. Among other 
> things this enables running all gmodules tests with the `--category gmodules` 
> flag.

This should be easy to arrange and was the other alternative I considered. Just 
removing `gmodules` from the `debug_info_categories` list in 
`test_categories.py` should suffice

> teach buildDefault to build with gmodules enabled in case the test is 
> annotated with the gmodules category.

I assume you're referring to `getBuildCommand` in `builder.py`. Currently we're 
passing it the `debug_info` category and infer the Makefile flags from it. Will 
have to see if we can extract the `categories` attribute there too.

> teach the debug info replication to ignore tests with the gmodules category 
> (just like it does for @no_debug_info_test_case tests). This step wouldn't be 
> necessary if we made debug info replication opt-in instead of opt-out, as 
> discussed on one of the previous patches (@JDevlieghere might remember which 
> one it was)

That's an interesting idea. @JDevlieghere @aprantl How much appetite is there 
for changing the replication to be opt-in (that would require an audit of each 
API test right?). Otherwise, an alternative that comes to mind without 
hard-coding a `category == gmodules` into the replication logic would be to 
make `debug_info_categories` a `dictionary` 
and keep `gmodules` in there. Then we wouldn't need to make changes to 
`getBuildCommand` either.


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] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Thanks! As the one who introduced this feature back in the day (and against 
Pavel's resistance) I've come around to seeing more value in targeted tests 
than a spray-paint approach of running the entire testsuite. It doesn't catch 
many interesting issues, since most tests try to avoid external dependencies 
(that could be built as modules) anyway, and we can write dedicated tests for 
libcxx and Objective-C modules like Foundation.


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] D133670: [LLDB][RISCV] Add RVM and RVA instruction support for EmulateInstructionRISCV

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Awesome, bot is green again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133670

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

[Lldb-commits] [PATCH] D134252: Track .dwo/.dwp loading errors and notify user when viewing variables.

[jeffrey tan via Phabricator via lldb-commits]

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

Technical wise this patch looks good. One concern is that these error messages 
are user facing but we are making it favoring debugging, like skeleton DIE, 
showing the DIE offset etc... which is not very useful/actionable for end 
users. 
Ideally, the user facing messages should focus on two things: 1. what is wrong 
in plain english. 2. any suggestion to fix it (e.g. ensure running lldb from 
build repo root). The more technical messages (with DIE offset) can be printed 
to log channel instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134252

___
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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

In D134344#3805953 , @Michael137 
wrote:

>> teach the debug info replication to ignore tests with the gmodules category 
>> (just like it does for @no_debug_info_test_case tests). This step wouldn't 
>> be necessary if we made debug info replication opt-in instead of opt-out, as 
>> discussed on one of the previous patches (@JDevlieghere might remember which 
>> one it was)
>
> That's an interesting idea. @JDevlieghere @aprantl How much appetite is there 
> for changing the replication to be opt-in (that would require an audit of 
> each API test right?). Otherwise, an alternative that comes to mind without 
> hard-coding a `category == gmodules` into the replication logic would be to 
> make `debug_info_categories` a `dictionary bool>` and keep `gmodules` in there. Then we wouldn't need to make changes to 
> `getBuildCommand` either.

That's such a big change (we also need to make the change in all downstream 
branches like swift-lldb) that I probably wouldn't want to roll it into this 
patch series right now, but I'm open to having a separate discussion about it. 
But I'm also missing the context as to why this would be desirable, so if 
there's a good reason, let me know!


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] D134378: [lldb] Support simplified template names

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks created this revision.
Herald added a reviewer: shafik.
Herald added a project: All.
aeubanks updated this revision to Diff 461963.
aeubanks added a comment.
aeubanks added a reviewer: dblaikie.
aeubanks published this revision for review.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

comments




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:801
 
-const char *DWARFDebugInfoEntry::GetQualifiedName(DWARFUnit *cu,
-  std::string &storage) const {

moved since now we need the clang AST to rebuild the fully qualified name
the only other caller is some logging


See https://discourse.llvm.org/t/dwarf-using-simplified-template-names/58417 
for background on simplified template names.

lldb doesn't work with simplified template names because it uses DW_AT_name 
which doesn't contain template parameters under simplified template names.

Two major changes are required to make lldb work with simplified template names.

1. When building clang ASTs for struct-like dies, we use the name as a cache 
key. To distinguish between different instantiations of a template class, we 
need to add in the template parameters.

2. When looking up types, if the requested type name contains '<' and we didn't 
initially find any types from the index searching the name, strip the template 
parameters and search the index, then filter out results with non-matching 
template parameters. This takes advantage of the clang AST's ability to print 
full names rather than doing it by ourself.

An alternative is to fix up the names in the index to contain the fully 
qualified name, but that doesn't respect .debug_names.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134378

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/lang/cpp/complete-type-check/TestCppIsTypeComplete.py
  lldb/test/API/lang/cpp/unique-types2/Makefile
  lldb/test/API/lang/cpp/unique-types2/TestUniqueTypes.py
  lldb/test/API/lang/cpp/unique-types2/main.cpp

Index: lldb/test/API/lang/cpp/unique-types2/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/unique-types2/main.cpp
@@ -0,0 +1,11 @@
+
+template  class Foo {
+  T t;
+};
+
+int main() {
+  Foo t1;
+  Foo t2;
+  Foo> t3;
+  // Set breakpoint here
+}
Index: lldb/test/API/lang/cpp/unique-types2/TestUniqueTypes.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/unique-types2/TestUniqueTypes.py
@@ -0,0 +1,18 @@
+"""
+Test that we return only the requested template instantiation.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+class UniqueTypesTestCase(TestBase):
+def test(self):
+"""Test that we only display the requested Foo instantiation, not all Foo instantiations."""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// Set breakpoint here", lldb.SBFileSpec("main.cpp"))
+
+self.expect("image lookup -A -t Foo", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
+self.expect("image lookup -A -t Foo", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
+self.expect("image lookup -A -t 'Foo >'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
+self.expect("image lookup -A -t Foo", DATA_TYPES_DISPLAYED_CORRECTLY, error=True)
Index: lldb/test/API/lang/cpp/unique-types2/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/unique-types2/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/test/API/lang/cpp/complete-type-check/TestCppIsTypeComplete.py
===
--- lldb/test/API/lang/cpp/complete-type-check/TestCppIsTypeComplete.py
+++ lldb/test/API/lang/cpp/complete-type-check/TestCppIsTypeComplete.py
@@ -51,7 +51,7 @@
 # Record types without a defining declaration are not complete.
 self.assertPointeeIncomplete("FwdClass *", "fwd_class")
 self.assertPointeeIncomplete("FwdClassTypedef *", "fwd_class_typedef")
-self.assertPointeeIncomplete("FwdTemplateClass<> *", "fwd_template_class")
+self.assertPointeeIncomplete("FwdTemplateClass *", "fwd_template_class")
 
 # A pointer type is complete even when it points to an incomplete type.
 fwd_class_ptr = self.expec

[Lldb-commits] [PATCH] D130534: loading a binary at a slide multiple times leaves old entries in the SectionLoadList

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 462000.
jasonmolenda added a comment.

Getting back to this old patch.  When I lost track of it, @DavidSpickett was 
saying that the test case was still not clear -- I was loading a section at an 
address, checking that I could read values out of arrays, then setting the 
section to a different address and seeing that I could still read the value out 
of the arrays.  I added comments to these two key parts of the test file,

  # View the first element of `first` and `second` with
  # no slide applied, but with load address set.
  #
  # In memory, we have something like
  #0x1000 - 0x17ff  first[]
  #0x1800 - 0x1fff  second[]
  target.SetModuleLoadAddress(module, 0)
  self.expect("p/d ((int*)&first)[0]", substrs=['= 5'])
  self.expect("p/d ((int*)&second)[0]", substrs=['= 6'])
  self.assertEqual(first_sym.GetStartAddress().GetLoadAddress(target), 
   first_sym.GetStartAddress().GetFileAddress())
  self.assertEqual(second_sym.GetStartAddress().GetLoadAddress(target),
   second_sym.GetStartAddress().GetFileAddress())
  
  # Slide it a little bit less than the size of the first array.
  #
  # In memory, we have something like
  #0xfc0 - 0x17bf  first[]
  #0x17c0 - 0x1fbf second[]
  #
  # but if the original entries are still present in lldb, 
  # the beginning address of second[] will get a load address
  # of 0x1800, instead of 0x17c0 (0x1800-64) as we need to get.
  target.SetModuleLoadAddress(module, first_size - 64)
  self.expect("p/d ((int*)&first)[0]", substrs=['= 5'])
  self.expect("p/d ((int*)&second)[0]", substrs=['= 6'])
  self.assertNotEqual(first_sym.GetStartAddress().GetLoadAddress(target), 
   first_sym.GetStartAddress().GetFileAddress())
  self.assertNotEqual(second_sym.GetStartAddress().GetLoadAddress(target),
   second_sym.GetStartAddress().GetFileAddress())

The inferior is never actually run, so I can change the load addresses of the 
sections around and it ultimately should read the data out of the file on disk 
every time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130534

Files:
  lldb/source/Target/SectionLoadList.cpp
  lldb/test/API/functionalities/multiple-slides/Makefile
  lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py
  lldb/test/API/functionalities/multiple-slides/main.c

Index: lldb/test/API/functionalities/multiple-slides/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/multiple-slides/main.c
@@ -0,0 +1,5 @@
+int first[2048] = { 5 };
+int second[2048] = { 6 };
+int main()  {
+  return first[0] + second[0];
+}
Index: lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py
===
--- /dev/null
+++ lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py
@@ -0,0 +1,79 @@
+"""
+Test that a binary can be slid to different load addresses correctly
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class MultipleSlidesTestCase(TestBase):
+
+NO_DEBUG_INFO_TESTCASE = True
+def test_mulitple_slides(self):
+"""Test that a binary can be slid multiple times correctly."""
+self.build()
+exe = self.getBuildArtifact("a.out")
+err = lldb.SBError()
+load_dependent_modules = False
+target = self.dbg.CreateTarget(exe, '', '', load_dependent_modules, err)
+self.assertTrue(target.IsValid())
+module = target.GetModuleAtIndex(0)
+self.assertTrue(module.IsValid())
+
+first_sym = target.FindSymbols("first").GetContextAtIndex(0).GetSymbol()
+second_sym = target.FindSymbols("second").GetContextAtIndex(0).GetSymbol()
+first_size = first_sym.GetEndAddress().GetOffset() - first_sym.GetStartAddress().GetOffset()
+second_size = second_sym.GetEndAddress().GetOffset() - second_sym.GetStartAddress().GetOffset()
+
+# View the first element of `first` and `second` while
+# they have no load address set.
+self.expect("p/d ((int*)&first)[0]", substrs=['= 5'])
+self.expect("p/d ((int*)&second)[0]", substrs=['= 6'])
+self.assertEqual(first_sym.GetStartAddress().GetLoadAddress(target), lldb.LLDB_INVALID_ADDRESS)
+self.assertEqual(second_sym.GetStartAddress().GetLoadAddress(target), lldb.LLDB_INVALID_ADDRESS)
+
+
+# View the first element of `first` and `second` with
+# no slide applied, but with load address set.
+#
+# In memory, we have something like
+#0x1000 - 0x17ff  first[]
+#0x1800 - 0x1fff  second[]
+target.SetModuleLoadAddress(module, 0)
+self.expect("p/d ((int*)&first)[0]", substrs=['= 5'])
+self.exp

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

D134385  should fix the problem:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133525

___
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

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

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.  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.

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).


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] D130534: loading a binary at a slide multiple times leaves old entries in the SectionLoadList

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

And to be clear, the problem is that we start out with an expression like 
`first[0]` which we find the Section + address for `first`, then resolve to a 
load address (addr_t) via the Target SectionLoadList, and then try to read 
memory from that address - at which point we go the other direction, converting 
the addr_t into a Section+offset via the Target SectionLoadList.  When we have 
the old section entries in SectionLoadList, we don't end up with the same 
Section+offset as we started on -- one of the old entries is picked up (if you 
slide it the right way - like the test does) and you read data from the wrong 
address in the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130534

___
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

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D134344#3806509 , @aprantl wrote:

> In D134344#3805953 , @Michael137 
> wrote:
>
>>> teach the debug info replication to ignore tests with the gmodules category 
>>> (just like it does for @no_debug_info_test_case tests). This step wouldn't 
>>> be necessary if we made debug info replication opt-in instead of opt-out, 
>>> as discussed on one of the previous patches (@JDevlieghere might remember 
>>> which one it was)
>>
>> That's an interesting idea. @JDevlieghere @aprantl How much appetite is 
>> there for changing the replication to be opt-in (that would require an audit 
>> of each API test right?). Otherwise, an alternative that comes to mind 
>> without hard-coding a `category == gmodules` into the replication logic 
>> would be to make `debug_info_categories` a `dictionary> replicable: bool>` and keep `gmodules` in there. Then we wouldn't need to 
>> make changes to `getBuildCommand` either.
>
> That's such a big change (we also need to make the change in all downstream 
> branches like swift-lldb) that I probably wouldn't want to roll it into this 
> patch series right now, but I'm open to having a separate discussion about 
> it. But I'm also missing the context as to why this would be desirable, so if 
> there's a good reason, let me know!

Just to clarify, any solution will have to support the following points (which 
Pavel mentioned in his suggestion):
(1) Make sure we *don't* run a test with all debug-info variants if we only 
want `gmodules`
(2) Be able to explicitly compile with `gmodules` support when we add the 
`gmodules` category to a test

Currently the options are:
(A) Currently the test-replication works by checking all the categories on a 
test-case and if it finds no debug_info categories it will replicate the test 
once for each variant. If we made `gmodules` not a debug_info category this 
means tagging a test with `add_test_categories(["gmodules"])` would not work as 
expected. Pavel suggests making it opt-in, in which case this loop would never 
do the unexpected thing. We would still have to solve (2), but we would get (1) 
as a consequence of the opt-in (Correct me if I misunderstood @labath).

(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.

(C) The approach in this patch. Remove the `gmodules` category entirely and add 
a decorator that is responsible for skipping the test and adding a debug_info 
category to the decorated test-case (to prevent replication from kicking in). 
Unfortunately this also requires the user to add `MAKE_GMODULES`/`MAKE_DSYM` to 
the test's Makefile; that's how this patch addresses the issue of (2), i.e., it 
bypasses it entirely


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] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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. We don't want anyone thinking 
they should free the pointer. 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:

  value_list.GetError().GetCString()

the SBValueList::GetError() would return a SBError instance and it would 
immediately destruct itself and the "GetCString()" which was returning a 
"Status::m_string.c_str()" is now invalid. This patch in fact didn't work 
without fixing this issue in our public API. It would get an empty string as an 
error. And made our APIs less useful. So seeing as we have the 
SBError::GetCString() API, and we want it to work as expected for people, we 
really should be fixing this. Or we should add a new API like:

  size_t SBError::GetErrorString(char *buffer, size_t buffer_size);

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.


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] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Our other option is to add more headerdoc to the SBError.h file and deal with 
this API issue by stating that the SBError object must be kept alive long 
enough to copy the error string returned by "const char 
*SBError::GetCString()". I am ok with either, I just think that it is currently 
safer and more consistent with all of our other APIs that return "const char *" 
since they all currently return a ConstString copy that will never go away,


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