[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't know about debugserver, but both lldb-server and gdbserver currently 
return an error when the memory is partially accessible, even though the 
protocol explicitly allows the possibility of truncated reads. It is somewhat 
hard to reproduce, because the caching mechanism in lldb aligns memory reads, 
and the cache "line" size is usually smaller than the page size -- which is 
probably why this behavior hasn't bothered anyone so far. Nonetheless, I would 
say that this behavior (not returning partial contents) is a (QoI) bug, but the 
fact that two stubs have it makes me wonder how many other stubs do the same as 
well..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906

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

[Lldb-commits] [PATCH] D134991: [lldb] Add diagnostics

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> I think that part should be easy to test and I was planning on adding a test 
> as part of that.

Sounds good to me. Nothing needed in this change then.

A command also gives you a chance to get the diagnostics out if running until 
the actual crash fails to dump them, potentially due to the signal handler 
issues Pavel is referring to.

> Yeah, in my mind all the callbacks should be orthogonal. If it wasn't for the 
> layering, I'd make it all the responsibility of the Diagnostics class. If 
> someone has a better idea than callbacks please let me know.

I do like that they keep the Diagnostics class free of details of the others. 
Which means you just extend the existing tests for those other classes each 
time you add a dump function to them.


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

https://reviews.llvm.org/D134991

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CleanupProcess

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 465012.
labath added a comment.

Check breakpoint hit counts after termination


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

Files:
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,57 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def readRegister(self, reg):
+# Pretend we're at the breakpoint after we've been resumed.
+return "3412" if self.continued else 
"4747"
+
+def cont(self):
+self.continued = True
+return "T05thread=47;reason:breakpoint"
+
+# Connect to the first process and set our breakpoint.
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+
+bkpt = target.BreakpointCreateByAddress(0x1234)
+self.assertTrue(bkpt.IsValid())
+self.assertEqual(bkpt.GetNumLocations(), 1)
+
+# "continue" the process. It should hit our breakpoint.
+process.Continue()
+self.assertState(process.GetState(), lldb.eStateStopped)
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Now kill it. The breakpoint should still show a hit count of one.
+process.Kill()
+self.server.stop()
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Start over, and reconnect.
+self.server = MockGDBServer(self.server_socket_class())
+self.server.start()
+
+process = self.connect(target)
+
+# The hit count should be reset.
+self.assertEqual(bkpt.GetHitCount(), 0)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -177,6 +177,7 @@
   // clean up needs some help from the process.
   m_breakpoint_list.ClearAllBreakpointSites();
   m_internal_breakpoint_list.ClearAllBreakpointSites();
+  ResetBreakpointHitCounts();
   // Disable watchpoints just on the debugger side.
   std::unique_lock lock;
   this->GetWatchpointList().GetListMutex(lock);
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2761,18 +2761,15 @@
 }
 
 Status Process::WillLaunch(Module *module) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillLaunch(module);
 }
 
 Status Process::WillAttachToProcessWithID(lldb::pid_t pid) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithID(pid);
 }
 
 Status Process::WillAttachToProcessWithName(const char *process_name,
 bool wait_for_launch) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithName(process_name, wait_for_launch);
 }
 


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,57 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+ 

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CleanupProcess

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134882#3831137 , @jingham wrote:

> That seems fine.  I think it's useful to be able to see breakpoint hit counts 
> up to the point where you start a new process.  From looking at the code, it 
> looks like putting the clear in CleanupProcess will do that.  If you agree 
> this is useful, can you add to the test that the hit count is still 1 between 
> process.Kill and connecting to the new process?

That makes sense to me. I've added the extra check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

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

[Lldb-commits] [PATCH] D135029: [lldb] [gdb-remote] Isolate ClientBase from Communication

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:116-118
+  GDBRemoteCommunication &GetCommunication() {
+return m_comm;
+  }

mgorny wrote:
> labath wrote:
> > Is the plan to make this private/protected at some point, or something like 
> > that? Otherwise, I'm not really sure what's the benefit of this over the 
> > regular inheritance.
> > 
> > I like the idea of using composition instead of inheritance (I think we 
> > could do something similar with GDBRemoteCommunication and Communication), 
> > but right now this seems fairly messy, and the benefit is unclear.
> Ideally, yes. However, I don't think I'm going to pursue it that far, i.e. 
> someone else will have to take up the effort. And yes, I honestly doubt 
> anybody will.
> 
> The main goal is make ground for D135031, i.e. communication via separate 
> thread. What I've been aiming at is leaving `GetCommunication()` for stuff 
> that's unlikely to break when invoked cross-thread (or unlikely to be invoked 
> cross-thread), while abstracting away the part of the API that needs to be 
> reimplemented to communicate via the comm thread.
Ok, maybe that's fine. Let's figure out what to do with the other patch first.


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

https://reviews.llvm.org/D135029

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

[Lldb-commits] [PATCH] D135031: [lldb] [llgs] Move client-server communication into a separate thread (WIP)

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think we should get some measurements (e.g. from the `process plugin packet 
speed-test` cmd) of the overhead of this approach. The latency/RTT of the 
connection is very important for some users, and I fear that all of this ping 
pong could significantly regress that.


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

https://reviews.llvm.org/D135031

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

[Lldb-commits] [PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a reviewer: shafik.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

Fixes #58135

Somehow lldb was able to print the member on its own but when we try
to print the whole type found by "image lookup -t" lldb would crash.

This is because we'd encoded the initial value of the member as an integer.
Which isn't the end of the world because bool is integral for C++.
However, clang has a special AST node to handle literal bool and it
expected us to use that instead.

This adds a new codepath to handle static bool which uses cxxBoolLiteralExpr
and we get the member printed as you'd expect.

For testing I added a struct with just the bool because trying to print
all of "A" crashes as well. Presumably because one of the other member's
types isn't handled properly either.

So for now I just added the bool case, we can merge it with A later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135169

Files:
  clang/include/clang/AST/ExprCXX.h
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp

Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,8 +79,13 @@
   ScopedEnum::scoped_enum_case1;
 };
 
+struct StaticBoolStruct {
+  static const bool value = false;
+};
+
 int main() {
   A a;
+  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,6 +32,11 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
+# Test a bool member when printing the struct it is a member of.
+# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
+self.expect("image lookup -t StaticBoolStruct",
+substrs=["static const bool value = false;"])
+
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
Index: lldb/source/Symbol/CompilerType.cpp
===
--- lldb/source/Symbol/CompilerType.cpp
+++ lldb/source/Symbol/CompilerType.cpp
@@ -154,6 +154,12 @@
   return IsIntegerType(is_signed) || IsEnumerationType(is_signed);
 }
 
+bool CompilerType::IsBooleanType() const {
+  if (IsValid())
+return m_type_system->IsBooleanType(m_type);
+  return false;
+}
+
 bool CompilerType::IsPointerType(CompilerType *pointee_type) const {
   if (IsValid()) {
 return m_type_system->IsPointerType(m_type, pointee_type);
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -592,6 +592,8 @@
   bool IsEnumerationType(lldb::opaque_compiler_type_t type,
  bool &is_signed) override;
 
+  bool IsBooleanType(lldb::opaque_compiler_type_t type) override;
+
   bool IsScopedEnumerationType(lldb::opaque_compiler_type_t type) override;
 
   static bool IsObjCClassType(const CompilerType &type);
@@ -860,6 +862,8 @@
   static void SetIntegerInitializerForVariable(clang::VarDecl *var,
const llvm::APInt &init_value);
 
+  static void SetBoolInitializerForVariable(clang::VarDecl *var, bool value);
+
   /// Initializes a variable with a floating point value.
   /// \param var The variable to initialize. Must not already have an
   ///initializer and must have a floating point type.
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3224,6 +3224,20 @@
   return false;
 }
 
+bool TypeSystemClang::Is

[Lldb-commits] [PATCH] D135170: [LLDB] Fix crash when printing a struct with a static signed char member

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

As with static bool for whatever reason printing them on their own
worked fine but wasn't handled when you printed the whole type.

I don't see a good way to test this from clang's side so our existing
tests will have to do.

We can now print all of the struct "A", so there's no need for a separate
one for static bool testing. I've not checked the output, just that it
succeeds. This saves us having to handle different min/max between systems.

Depends on D135169 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135170

Files:
  clang/lib/AST/StmtPrinter.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't 
crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_expr("const int *i = &A::int_val_with_address; *i",
  result_value="2")
 
+# Printing the whole type takes a slightly different code path. Check 
that
+# it does not crash.
+self.expect("image lookup -t A")
+
 # dsymutil strips the debug info for classes that only have const static
 # data members without a definition namespace scope.
 @expectedFailureAll(debug_info=["dsym"])
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -1280,6 +1280,7 @@
   case BuiltinType::Char_S:
   case BuiltinType::Char_U:OS << "i8"; break;
   case BuiltinType::UChar: OS << "Ui8"; break;
+  case BuiltinType::SChar: OS << "i8"; break;
   case BuiltinType::Short: OS << "i16"; break;
   case BuiltinType::UShort:OS << "Ui16"; break;
   case BuiltinType::Int:   break; // no suffix.


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_ex

[Lldb-commits] [PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via lldb-commits]

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

For https://github.com/llvm/llvm-project/issues/58135.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135169

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

[Lldb-commits] [PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:733
 
+  static CXXBoolLiteralExpr *Create(const ASTContext &C, bool Val, QualType Ty,
+SourceLocation Loc) {

I think this makes sense but if we are going to do this refactor then we should 
clean up all the locations we are currently doing `new (Context) 
CXXBoolLiteralExpr()` as well.

Maybe that should be the initial PR and then this change would be a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135169

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

[Lldb-commits] [PATCH] D135178: [lldb][test] Skip import-std-module/vector tests

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

These tests have begun failing starting with commit
`69a6417406a1b0316a1fa6aeb63339d0e1d2abbd`, which
added a new `import` to `ASTNodeImporter::VisitTypedefType`.
This trips an assertion in following way:

1. When creating a persistent variable for the result we call `CopyType` (in 
`DeportType`) under a `CompleteTagDeclsScope` (which is supposed to complete 
all decls newly imported in the `CopyType` call).
2. During `CopyType` we call `ASTNodeImporter::VisitTypedefType`
3. This now has a second import call on the desugared type
4. In `ASTImporterDelegate::ImportImpl` we will now try to import a decl that 
we originally got from the `std` module (which means it has no valid origin). 
But since we’re doing this under a CompleteTagDeclsScope, the 
`NewDeclListener::NewDeclImported` adds the decl to the list of decls to 
complete after the `CopyType` call. But this list shouldn’t contain decls with 
invalid origins because we assert this in `~CompleteTagDeclsScope`, which is 
where the tests crash.

We suspect that we previously didn’t see this assert trigger because by the time
we create the result variable we are using an AST whose decls all have
a valid debug-info origin (constructed with the help of the std module).
So we never expected decls from modules to be imported under
`CompleteTagDeclsScope` without a m_sema available (which is the case by
the time we get to `DeportType`). Since there is no `m_sema` available,
`CxxModuleHandler::Import` trivially returns and the decls don’t get added
to the `m_decls_to_ignore` list and count as "newly imported decls".

Skip this test for now until we have a fix or the origin tracking gets
refactored (see https://reviews.llvm.org/D101950).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135178

Files:
  
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py


Index: 
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
===
--- 
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ 
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 
Index: 
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
===
--- 
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
+++ 
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 


Index: lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 
Index: lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 
___
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.

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe added a comment.

Ping? If anyone has other suggestions for reviewers I'd appreciate them.


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

https://reviews.llvm.org/D134570

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

[Lldb-commits] [lldb] 9abeb0c - [lldb][test] Skip import-std-module/vector tests

[Michael Buch via lldb-commits]

Author: Michael Buch
Date: 2022-10-04T18:38:47+01:00
New Revision: 9abeb0cbd983c7e32a729e333b033673546e5485

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

LOG: [lldb][test] Skip import-std-module/vector tests

These tests have begun failing starting with commit
`69a6417406a1b0316a1fa6aeb63339d0e1d2abbd`, which
added a new `import` to `ASTNodeImporter::VisitTypedefType`.
This trips an assertion in following way:
1. When creating a persistent variable for the result we call `CopyType`
   (in `DeportType`) under a `CompleteTagDeclsScope` (which is supposed to 
complete all
   decls newly imported in the `CopyType` call).
2. During `CopyType` we call `ASTNodeImporter::VisitTypedefType`
3. This now has a second import call on the desugared type
4. In `ASTImporterDelegate::ImportImpl` we will now try to import a decl
   that we originally got from the `std` module (which means it has no valid 
origin).
   But since we’re doing this under a CompleteTagDeclsScope, the
   `NewDeclListener::NewDeclImported` adds the decl to the list of decls to
   complete after the `CopyType` call. But this list shouldn’t contain decls
   with invalid origins because we assert this in `~CompleteTagDeclsScope`, 
which
   is where the tests crash.

We suspect that we previously didn’t see this assert trigger because by the time
we create the result variable we are using an AST whose decls all have
a valid debug-info origin (constructed with the help of the std module).
So we never expected decls from modules to be imported under
`CompleteTagDeclsScope` without a m_sema available (which is the case by
the time we get to `DeportType`). Since there is no `m_sema` available,
`CxxModuleHandler::Import` trivially returns and the decls don’t get added
to the `m_decls_to_ignore` list and count as "newly imported decls".

Skip this test for now until we have a fix or the origin tracking gets
refactored (see https://reviews.llvm.org/D101950).

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

Added: 


Modified: 

lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py

lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py

Removed: 




diff  --git 
a/lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
 
b/lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
index 659e329efd76b..39ad475ab64d2 100644
--- 
a/lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
+++ 
b/lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
@@ -11,6 +11,7 @@ class TestBoolVector(TestBase):
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 

diff  --git 
a/lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
 
b/lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
index 143c5216c4d3f..f05d3dcd0c1da 100644
--- 
a/lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ 
b/lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -11,6 +11,7 @@ class TestBasicVector(TestBase):
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 



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

[Lldb-commits] [PATCH] D135178: [lldb][test] Skip import-std-module/vector tests

[Michael Buch via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9abeb0cbd983: [lldb][test] Skip import-std-module/vector 
tests (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135178

Files:
  
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py


Index: 
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
===
--- 
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ 
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 
Index: 
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
===
--- 
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
+++ 
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 


Index: lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 
Index: lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
@@ -11,6 +11,7 @@
 
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIf(bugnumber="rdar://100741983")
 def test(self):
 self.build()
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 2c43cd8 - Turn off the warning that the undefined behavior I am using in

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2022-10-04T11:38:32-07:00
New Revision: 2c43cd883c20b603bc7d9461bfa2591e80a36a3b

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

LOG: Turn off the warning that the undefined behavior I am using in
a test generates.  The green dragon bot compiler is treating this
warning as an error for some reason, hopefully this will calm its
worries.

Added: 


Modified: 
lldb/test/API/functionalities/ubsan/basic/Makefile

Removed: 




diff  --git a/lldb/test/API/functionalities/ubsan/basic/Makefile 
b/lldb/test/API/functionalities/ubsan/basic/Makefile
index b27db90a40de2..caa15bff7b816 100644
--- a/lldb/test/API/functionalities/ubsan/basic/Makefile
+++ b/lldb/test/API/functionalities/ubsan/basic/Makefile
@@ -1,4 +1,4 @@
 C_SOURCES := main.c
-CFLAGS_EXTRAS := -fsanitize=undefined -g
+CFLAGS_EXTRAS := -fsanitize=undefined -g -Wno-int-conversion
 
 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] D132300: [clang][lldb][cmake] Use new `*_INSTALL_LIBDIR_BASENAME` CPP macro

[John Ericson via Phabricator via lldb-commits]

Ericson2314 added a comment.

Thanks @MaskRay that's good to know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132300

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

[Lldb-commits] [lldb] 40501f1 - buildbot-based-debugging a Microsoft lldb test XPASS

[David Blaikie via lldb-commits]

Author: David Blaikie
Date: 2022-10-04T21:38:33Z
New Revision: 40501f1b4139e472094ac0d4102923d2730515a1

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

LOG: buildbot-based-debugging a Microsoft lldb test XPASS

Let's see if it's been fixed by my recent ABI fixes...

Added: 


Modified: 
lldb/test/API/commands/expression/xvalue/TestXValuePrinting.py

Removed: 




diff  --git a/lldb/test/API/commands/expression/xvalue/TestXValuePrinting.py 
b/lldb/test/API/commands/expression/xvalue/TestXValuePrinting.py
index 8ef0654b6444..e89ba6b7b62f 100644
--- a/lldb/test/API/commands/expression/xvalue/TestXValuePrinting.py
+++ b/lldb/test/API/commands/expression/xvalue/TestXValuePrinting.py
@@ -5,7 +5,6 @@
 
 class ExprXValuePrintingTestCase(TestBase):
 
-@expectedFailureAll(oslist=["windows"], archs=["i[3-6]86", "x86_64"], 
bugnumber="llvm.org/pr21765")
 def test(self):
 """Printing an xvalue should work."""
 self.build()



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

[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Jim & David, thanks so much for the comments.  The most important question of 
correctness that David asks is whether we might try to read 32k from an address 
and mark that address as inaccessible because a page 24k into the memory read 
failed (for instance).  I tried to be conservative and only mark an address as 
inaccessible when we were able to read 0 bytes (ReadMemory returns partial 
reads).

I think adding a little hint in the error message, "(cached)" would make it 
clearer to an lldb maintainer what happened, and not distract the user with too 
much.

In D134906#3830290 , @DavidSpickett 
wrote:

>> If we wanted to track these decisions it would be more appropriate to log 
>> them, but I'm not sure even that is necessary.
>
> Yeah this is a better idea, if we do it at all.
>
> Let me rephrase the question. If I had a memory read failing and I suspected 
> that the cache was marking it as unreadable how would I confirm that? If 
> there's a way to do that when it's really needed, then we're good.

When I was testing/debugging this, I did it by turning the packet log on :)

> Perhaps log only when a new address is added to the unreadable list? Then 
> with the memory log plus the packet log you could figure out the issue, even 
> if you didn't know that the cache had this unreadable address feature before 
> you started investigating.

Yes, I can add a log message to Process when I add an address, good idea.

Let me go through the patch again carefully to make sure I'm correctly handling 
partially-successful reads (not putting anything in the inaccessible memory 
cache) and tweak that error message a tad & add a bit of logging.

In D134906#3832642 , @labath wrote:

> I don't know about debugserver, but both lldb-server and gdbserver currently 
> return an error when the memory is partially accessible, even though the 
> protocol explicitly allows the possibility of truncated reads. It is somewhat 
> hard to reproduce, because the caching mechanism in lldb aligns memory reads, 
> and the cache "line" size is usually smaller than the page size -- which is 
> probably why this behavior hasn't bothered anyone so far. Nonetheless, I 
> would say that this behavior (not returning partial contents) is a (QoI) bug, 
> but the fact that two stubs have it makes me wonder how many other stubs do 
> the same as well..



In D134906#3832642 , @labath wrote:

> I don't know about debugserver, but both lldb-server and gdbserver currently 
> return an error when the memory is partially accessible, even though the 
> protocol explicitly allows the possibility of truncated reads. It is somewhat 
> hard to reproduce, because the caching mechanism in lldb aligns memory reads, 
> and the cache "line" size is usually smaller than the page size -- which is 
> probably why this behavior hasn't bothered anyone so far. Nonetheless, I 
> would say that this behavior (not returning partial contents) is a (QoI) bug, 
> but the fact that two stubs have it makes me wonder how many other stubs do 
> the same as well..



In D134906#3832642 , @labath wrote:

> I don't know about debugserver, but both lldb-server and gdbserver currently 
> return an error when the memory is partially accessible, even though the 
> protocol explicitly allows the possibility of truncated reads. It is somewhat 
> hard to reproduce, because the caching mechanism in lldb aligns memory reads, 
> and the cache "line" size is usually smaller than the page size -- which is 
> probably why this behavior hasn't bothered anyone so far. Nonetheless, I 
> would say that this behavior (not returning partial contents) is a (QoI) bug, 
> but the fact that two stubs have it makes me wonder how many other stubs do 
> the same as well..

Hi Pavel, thanks for pointing this out.  I did a quick check with debugserver 
on darwin, using `memory region` to find the end of an accessible region in 
memory, and starting a read request a little earlier, in readable memory:

  (lldb) sett set target.process.disable-memory-cache true
  
  (lldb) mem region 0x00010180
   <  31> send packet: $qMemoryRegionInfo:10180#ce
   <  34> read packet: $start:10180;size:6a60;#00
  [0x00010180-0x00016be0) ---
  
  (lldb) mem region 0x00010180-4
   <  31> send packet: $qMemoryRegionInfo:1017c#d8
   < 122> read packet: 
$start:10100;size:80;permissions:rw;dirty-pages:10100,101008000,10100c000,1017fc000;type:heap,malloc-small;#00
  [0x00010100-0x00010180) rw-
  
  (lldb) x/8wx 0x00010180-4
  
   <  17> send packet: $x1017c,20#ca
   <   8> read packet: $#00
  
   <  17> send packet: $x10180,1c#f2
   <   7> read packet: $E80#00
  
  0x1017c: 0x 0x 0x 0x
  0x1018c: 

[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D134906#3835260 , @jasonmolenda 
wrote:

> We ask for 32 bytes starting at 0x1017c, get back 4 bytes.  Then we try 
> to read the remaining 28 bytes starting at 0x10180, and get an error. So 
> this is different behavior from other stubs where you might simply get an 
> error for the request to read more bytes than are readable.  This does 
> complicate the approach I'm doing -- because I'll never know which address 
> was inaccessible beyond "something within this address range".  I don't think 
> I can do anything here if that's the case.

to be clear, I think I'll need to abandon this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906

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

[Lldb-commits] [PATCH] D126260: [lldb/crashlog] Add support for Application Specific Backtraces & Information

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 465232.
mib marked 2 inline comments as done.
mib retitled this revision from "[lldb/crashlog] Add support for Application 
Specific Backtraces" to "[lldb/crashlog] Add support for Application Specific 
Backtraces & Information".
mib edited the summary of this revision.
mib added a reviewer: jingham.
mib added a comment.

- Add test (wip)
- Add Application Specific Information to process extended crash info
- Add {,Scripted}Process::GetMetadata method


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

https://reviews.llvm.org/D126260

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/asi.ips
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/main.m
  
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/app_specific_backtrace_crashlog.test

Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/app_specific_backtrace_crashlog.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/app_specific_backtrace_crashlog.test
@@ -0,0 +1,57 @@
+# REQUIRES: python, native && target-aarch64 && system-darwin
+
+# RUN: %clangxx_host -framework Foundation -g %S/Inputs/main.m -o %t.out
+
+# RUN: cp %S/Inputs/asi.ips %t.crash
+# RUN: %python %S/patch-crashlog.py --binary %t.out --crashlog %t.crash --offsets '{"main":160, "bar":20, "foo":24}' --json
+# RUN: %lldb %t.out -o 'command script import lldb.macosx.crashlog' -o 'crashlog -a -i %t.crash' 2>&1 -o "thread list" -o "bt all" | FileCheck %s
+
+# CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" options on these commands
+
+# CHECK: (lldb) process status
+# CHECK-NEXT: Process 24991 stopped
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT: frame #0: 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar
+
+# CHECK: (lldb) thread backtrace
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT:   * frame #0: 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar
+# CHECK-NEXT: frame #1: 0x0001047f5998 scripted_crashlog_json.test.tmp.out`foo
+# CHECK-NEXT: frame #2: 0x0001047f5b04 scripted_crashlog_json.test.tmp.out`compute_pow
+# CHECK-NEXT: frame #3: 0x0001047f7690 scripted_crashlog_json.test.tmp.out`decltype
+# CHECK-NEXT: frame #4: 0x0001047f7614 scripted_crashlog_json.test.tmp.out`void std::__1::__thread_execute
+# CHECK-NEXT: frame #5: 0x0001047f6d58 scripted_crashlog_json.test.tmp.out`void* std::__1::__thread_proxy
+# CHECK-NEXT: frame #6: 0x00018bf5326c libsystem_pthread.dylib`_pthread_start
+# CHECK-NEXT: frame #7: 0x00018bf4e08c libsystem_pthread.dylib`thread_start
+
+# CHECK: (lldb) thread list
+# CHECK-NEXT: Process 24991 stopped
+# CHECK-NEXT:  thread #1: tid = 0x4ea840, 0x00018bf17854 libsystem_kernel.dylib`__ulock_wait{{.*}}, queue = 'com.apple.main-thread'
+# CHECK-NEXT:  thread #2: tid = 0x4ea850, 0x0001047f59e8 scripted_crashlog_json.test.tmp.out`call_and_wait
+# CHECK-NEXT: * thread #3: tid = 0x4ea851, 0x0001047f5970 scripted_crashlog_json.test.tmp.out`bar{{.*}}, stop reason = EXC_BAD_ACCESS
+
+
+# CHECK: (lldb) bt all
+# CHECK-NEXT:   thread #1
+# CHECK-NEXT: frame #0: 0x00018bf17854 libsystem_kernel.dylib`__ulock_wait
+# CHECK-NEXT: frame #1: 0x00018bf555a0 libsystem_pthread.dylib`_pthread_join
+# CHECK-NEXT: frame #2: 0x00018beae9c0 libc++.1.dylib`std::__1::thread::join
+# CHECK-NEXT: frame #3: 0x0001047f5bb8 scripted_crashlog_json.test.tmp.out`main
+# CHECK-NEXT: frame #4: 0x000104ae5088 dyld`start
+# CHECK-NEXT:   thread #2
+# CHECK-NEXT: frame #0: 0x0001047f59e8 scripted_crashlog_json.test.tmp.out`call_and_wait
+# CHECK-NEXT: frame #1: 0x0001047f59d4 scripted_crashlog_json.test.tmp.out`call_and_wait
+# CHECK-NEXT: frame #2: 0x0001047f7690 scripted_crashlog_json.test.tmp.out`decltype
+# CHECK-NEXT: frame #3: 0x0001047f7614 scripted_crashlog_json.test.tmp.out`void std::__1::__thread_execute
+# CHECK-NEXT: frame #4: 0x0001047f6d58 scripted_crashlog_json.test.tmp.out`void* std::__1::__thread_proxy
+# CHECK-NEXT: frame #5: 0x00018bf5326c libsystem_pthread.dylib`_pthread_start
+# CHECK-NEXT: frame #6: 0x00018bf4e08c libsystem_pt

[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134906#3835260 , @jasonmolenda 
wrote:

> In D134906#3832642 , @labath wrote:
>
>> I don't know about debugserver, but both lldb-server and gdbserver currently 
>> return an error when the memory is partially accessible, even though the 
>> protocol explicitly allows the possibility of truncated reads. It is 
>> somewhat hard to reproduce, because the caching mechanism in lldb aligns 
>> memory reads, and the cache "line" size is usually smaller than the page 
>> size -- which is probably why this behavior hasn't bothered anyone so far. 
>> Nonetheless, I would say that this behavior (not returning partial contents) 
>> is a (QoI) bug, but the fact that two stubs have it makes me wonder how many 
>> other stubs do the same as well..
>
> Hi Pavel, thanks for pointing this out.  I did a quick check with debugserver 
> on darwin, using `memory region` to find the end of an accessible region in 
> memory, and starting a read request a little earlier, in readable memory:
>
>   (lldb) sett set target.process.disable-memory-cache true
>   
>   (lldb) mem region 0x00010180
><  31> send packet: $qMemoryRegionInfo:10180#ce
><  34> read packet: $start:10180;size:6a60;#00
>   [0x00010180-0x00016be0) ---
>   
>   (lldb) mem region 0x00010180-4
><  31> send packet: $qMemoryRegionInfo:1017c#d8
>< 122> read packet: 
> $start:10100;size:80;permissions:rw;dirty-pages:10100,101008000,10100c000,1017fc000;type:heap,malloc-small;#00
>   [0x00010100-0x00010180) rw-
>   
>   (lldb) x/8wx 0x00010180-4
>   
><  17> send packet: $x1017c,20#ca
><   8> read packet: $#00
>   
><  17> send packet: $x10180,1c#f2
><   7> read packet: $E80#00
>   
>   0x1017c: 0x 0x 0x 0x
>   0x1018c: 0x 0x 0x 0x
>   warning: Not all bytes (4/32) were able to be read from 0x1017c.
>   (lldb) 
>
> We ask for 32 bytes starting at 0x1017c, get back 4 bytes.  Then we try 
> to read the remaining 28 bytes starting at 0x10180, and get an error. So 
> this is different behavior from other stubs where you might simply get an 
> error for the request to read more bytes than are readable.

Yes, that's pretty much what I did, except that I was not able to read any data 
with the caching turned off.

In D134906#3835291 , @jasonmolenda 
wrote:

> to be clear, I think I'll need to abandon this.

I don't think this is necessarily a lost cause. I mean, the debugserver 
behavior (truncated reads) is definitely better here, and the caching of 
negative acesses makes sense. And, as the example above shows, the current 
behavior with the non-truncating stubs is already kind of broken, because you 
cannot read the areas near the end of mapped space without doing some kind of a 
bisection on the size of the read (we could implement the bisection in lldb, 
but... ewww).

The safest way to pursue this would be to have the stub indicate (maybe via 
qSupported) its intention to return truncated reads, and then key the behavior 
off of that. However, if that's not possible (it seems you have some hardware 
stub here), I could imagine just enabling this behavior by default. We can 
definitely change the lldb-server behavior, and for the rest, we can tell them 
to fix their stubs.

That is, if they even notice this. The memory read alignment hides this problem 
fairly well. To demonstrate this, we've had to turn the caching off -- but that 
would also turn off the negative cache, and avoid this problem. So, if someone 
can't fix their stub, we can always tell them to turn the cache off as a 
workaround.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906

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