[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

Thanks for the prompt review!

> This change is wrong for ValueObjectConstResult's. The original point of 
> ValueObjectConstResult's was to store the results of expressions so that, 
> even if the process is not at the original stop point, you could still check 
> the value. It should have the value it had at the time the expression was 
> evaluated. Updating the value is exactly what you don't want to do. So I 
> think you need to make sure that we can't change their value after they are 
> created.

Looking at the current state of things and the available APIs, I don't think 
this principle holds anymore. `ValueObjectConstResult` is also used for values 
created via `SBTarget::CreateValueFromData`. As a user I don't see any reason 
why I shouldn't be allowed to modify the value of an object I created myself 
earlier. I would argue the same for the results of the expression evaluation. 
Why are some values different from others?

> Note, ExpressionResult variables are different from 
> ExpressionPersistentVariables. The former should not be updated, and after 
> the process moves on any children that weren't gathered become "unknown". But 
> ExpressionPersistentVariables should be able to be assigned to, and when the 
> reference target object, it should update them live. .

Please, correct me if I'm wrong. In my example `ExpressionResult` is a value 
returned by `EvaluateExpression` (i.e. `b`) and `ExpressionPersistentVariable` 
is variable created by the expression evaluator (i.e. `$b_0`), right? As far as 
I can tell, they're both `ValueObjectConstResult` and both have the problem 
outlined in this patch:

  frame0.EvaluateExpression("auto $b_0 = b1")
  b = frame0.FindValue("$b_0", lldb.eValueTypeConstResult)
  ...
  # Same problem, updating the value of `b` doesn't invalidate children.



> The other thing to check about this patch is whether it defeats detecting 
> changed values in child elements.

This patch invalidates children only in `ValueObject::SetNeedsUpdate()` which 
is called only from `SetData/SetValueFromCString` as far as I understand. If 
you have a `ValueObjectVariable` which tracks some variable that can be 
modified by the process, then it will continue to work fine. 
`TestValueVarUpdate.py` passes successfully, but I didn't look to0 closely yet 
whether it actually tests the scenario you described.

---

Looking at this from the user perspective, I would prefer to be able to update 
any value, regardless of which API it came from. In my use case I rely on this 
heavily -- 
https://werat.dev/blog/blazing-fast-expression-evaluation-for-c-in-lldb/#maintaning-state.
I could potentially live with the results of `EvaluateExpression` being 
immutable, but being able to modify values created by `CreateValueFromData` or 
persistent values like `$b_0` is necessary for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470

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

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

[Andy Yankovsky via Phabricator via lldb-commits]

werat updated this revision to Diff 357477.
werat added a comment.

Add a test for a value created via `SBTarget::CreateValueFromData`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470

Files:
  lldb/source/Core/ValueObject.cpp
  lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
  lldb/test/API/python_api/value/change_values/main.c


Index: lldb/test/API/python_api/value/change_values/main.c
===
--- lldb/test/API/python_api/value/change_values/main.c
+++ lldb/test/API/python_api/value/change_values/main.c
@@ -8,7 +8,12 @@
   uint32_t  second_val;
   uint64_t  third_val;
 };
-  
+
+struct bar
+{
+  int value;
+};
+
 int main ()
 {
   int val = 100;
@@ -18,6 +23,11 @@
   ptr->second_val = ;
   ptr->third_val = ;
 
+  struct bar _b1 = {.value = 1};
+  struct bar _b2 = {.value = 2};
+  struct bar *b1 = &_b1;
+  struct bar *b2 = &_b2;
+
   // Stop here and set values
   printf ("Val - %d Mine - %d, %d, %llu. Ptr - %d, %d, %llu\n", val, 
   mine.first_val, mine.second_val, mine.third_val,
Index: lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
===
--- lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
+++ lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
@@ -130,6 +130,20 @@
 self.assertEquals(actual_value, 98765,
 "Got the right changed value from ptr->second_val")
 
+# Test updating the children after updating the parent value.
+b = frame0.EvaluateExpression("auto $b_0 = b1; $b_0")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b1").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "1")
+
+result = b.SetValueFromCString(frame0.FindVariable("b2").GetValue())
+self.assertTrue(result, "Success setting $b_0")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b2").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "2")
+
 # gcc may set multiple locations for breakpoint
 breakpoint.SetEnabled(False)
 
Index: lldb/source/Core/ValueObject.cpp
===
--- lldb/source/Core/ValueObject.cpp
+++ lldb/source/Core/ValueObject.cpp
@@ -231,6 +231,10 @@
   // We have to clear the value string here so ConstResult children will notice
   // if their values are changed by hand (i.e. with SetValueAsCString).
   ClearUserVisibleData(eClearUserVisibleDataItemsValue);
+  // Children have to be re-computed after updating the parent value.
+  m_flags.m_children_count_valid = false;
+  m_children.Clear();
+  SetSyntheticChildren(lldb::SyntheticChildrenSP());
 }
 
 void ValueObject::ClearDynamicTypeInformation() {


Index: lldb/test/API/python_api/value/change_values/main.c
===
--- lldb/test/API/python_api/value/change_values/main.c
+++ lldb/test/API/python_api/value/change_values/main.c
@@ -8,7 +8,12 @@
   uint32_t  second_val;
   uint64_t  third_val;
 };
-  
+
+struct bar
+{
+  int value;
+};
+
 int main ()
 {
   int val = 100;
@@ -18,6 +23,11 @@
   ptr->second_val = ;
   ptr->third_val = ;
 
+  struct bar _b1 = {.value = 1};
+  struct bar _b2 = {.value = 2};
+  struct bar *b1 = &_b1;
+  struct bar *b2 = &_b2;
+
   // Stop here and set values
   printf ("Val - %d Mine - %d, %d, %llu. Ptr - %d, %d, %llu\n", val, 
   mine.first_val, mine.second_val, mine.third_val,
Index: lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
===
--- lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
+++ lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
@@ -130,6 +130,20 @@
 self.assertEquals(actual_value, 98765,
 "Got the right changed value from ptr->second_val")
 
+# Test updating the children after updating the parent value.
+b = frame0.EvaluateExpression("auto $b_0 = b1; $b_0")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b1").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "1")
+
+result = b.SetValueFromCString(frame0.FindVariable("b2").GetValue())
+self.assertTrue(result, "Success setting $b_0")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b2").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "2")
+
 # gcc may set multiple locations for breakpoint
 breakpoint.SetEnabled(False)
 
Index: lldb/source/Core/ValueObject.cpp
===
--

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

[Andy Yankovsky via Phabricator via lldb-commits]

werat updated this revision to Diff 357478.
werat added a comment.

Add a test for a value created via `SBTarget::CreateValueFromData`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470

Files:
  lldb/source/Core/ValueObject.cpp
  lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
  lldb/test/API/python_api/value/change_values/main.c


Index: lldb/test/API/python_api/value/change_values/main.c
===
--- lldb/test/API/python_api/value/change_values/main.c
+++ lldb/test/API/python_api/value/change_values/main.c
@@ -8,7 +8,12 @@
   uint32_t  second_val;
   uint64_t  third_val;
 };
-  
+
+struct bar
+{
+  int value;
+};
+
 int main ()
 {
   int val = 100;
@@ -18,6 +23,11 @@
   ptr->second_val = ;
   ptr->third_val = ;
 
+  struct bar _b1 = {.value = 1};
+  struct bar _b2 = {.value = 2};
+  struct bar *b1 = &_b1;
+  struct bar *b2 = &_b2;
+
   // Stop here and set values
   printf ("Val - %d Mine - %d, %d, %llu. Ptr - %d, %d, %llu\n", val, 
   mine.first_val, mine.second_val, mine.third_val,
Index: lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
===
--- lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
+++ lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
@@ -130,6 +130,44 @@
 self.assertEquals(actual_value, 98765,
 "Got the right changed value from ptr->second_val")
 
+# Test updating the children after updating the parent value.
+
+# Test for 'b' -- value created by the expression evaluator.
+b = frame0.EvaluateExpression("auto $b_0 = b1; $b_0")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b1").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "1")
+
+result = b.SetValueFromCString(frame0.FindVariable("b2").GetValue())
+self.assertTrue(result, "Success setting $b_0")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b2").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "2")
+
+# Test for 'b' -- value created by SBTarget::CreateValueFromData.
+b1 = frame0.FindVariable("b1")
+b1_addr_bytes = b1.GetValueAsUnsigned().to_bytes(
+b1.GetByteSize(),
+'little' if process.GetByteOrder() == lldb.eByteOrderLittle else 
'big')
+error = lldb.SBError()
+data = lldb.SBData()
+data.SetData(
+error, b1_addr_bytes, process.GetByteOrder(), b1.GetByteSize())
+b = target.CreateValueFromData("b", data, b1.GetType())
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b1").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "1")
+
+result = b.SetValueFromCString(frame0.FindVariable("b2").GetValue())
+self.assertTrue(result, "Success setting b")
+self.assertEquals(
+b.GetValue(),
+frame0.FindVariable("b2").GetValue())
+self.assertEquals(b.GetChildAtIndex(0).GetValue(), "2")
+
 # gcc may set multiple locations for breakpoint
 breakpoint.SetEnabled(False)
 
Index: lldb/source/Core/ValueObject.cpp
===
--- lldb/source/Core/ValueObject.cpp
+++ lldb/source/Core/ValueObject.cpp
@@ -231,6 +231,10 @@
   // We have to clear the value string here so ConstResult children will notice
   // if their values are changed by hand (i.e. with SetValueAsCString).
   ClearUserVisibleData(eClearUserVisibleDataItemsValue);
+  // Children have to be re-computed after updating the parent value.
+  m_flags.m_children_count_valid = false;
+  m_children.Clear();
+  SetSyntheticChildren(lldb::SyntheticChildrenSP());
 }
 
 void ValueObject::ClearDynamicTypeInformation() {


Index: lldb/test/API/python_api/value/change_values/main.c
===
--- lldb/test/API/python_api/value/change_values/main.c
+++ lldb/test/API/python_api/value/change_values/main.c
@@ -8,7 +8,12 @@
   uint32_t  second_val;
   uint64_t  third_val;
 };
-  
+
+struct bar
+{
+  int value;
+};
+
 int main ()
 {
   int val = 100;
@@ -18,6 +23,11 @@
   ptr->second_val = ;
   ptr->third_val = ;
 
+  struct bar _b1 = {.value = 1};
+  struct bar _b2 = {.value = 2};
+  struct bar *b1 = &_b1;
+  struct bar *b2 = &_b2;
+
   // Stop here and set values
   printf ("Val - %d Mine - %d, %d, %llu. Ptr - %d, %d, %llu\n", val, 
   mine.first_val, mine.second_val, mine.third_val,
Index: lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
===
--- lldb/test/API/python_api/value/change_va

[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[David Spickett via Phabricator via lldb-commits]

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

Update various comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

Files:
  lldb/include/lldb/Target/MemoryTagManager.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Index: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
===
--- lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -94,6 +94,121 @@
   manager.ExpandToGranule(MemoryTagManagerAArch64MTE::TagRange(18, 4)));
 }
 
+static MemoryRegionInfo MakeRegionInfo(lldb::addr_t base, lldb::addr_t size,
+   bool tagged) {
+  return MemoryRegionInfo(
+  MemoryRegionInfo::RangeType(base, size), MemoryRegionInfo::eYes,
+  MemoryRegionInfo::eYes, MemoryRegionInfo::eYes, MemoryRegionInfo::eYes,
+  ConstString(), MemoryRegionInfo::eNo, 0,
+  /*memory_tagged=*/
+  tagged ? MemoryRegionInfo::eYes : MemoryRegionInfo::eNo);
+}
+
+TEST(MemoryTagManagerAArch64MTETest, MakeTaggedRange) {
+  MemoryTagManagerAArch64MTE manager;
+  MemoryRegionInfos memory_regions;
+
+  // No regions means no tagged regions, error
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0, 0x10, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x0:0x10 is not in a memory tagged region"));
+
+  // Alignment is done before checking regions.
+  // Here 1 is rounded up to the granule size of 0x10.
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0, 1, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x0:0x10 is not in a memory tagged region"));
+
+  // Range must not be inverted
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(1, 0, memory_regions),
+  llvm::FailedWithMessage(
+  "End address (0x0) must be greater than the start address (0x1)"));
+
+  // Adding a single region to cover the whole range
+  memory_regions.push_back(MakeRegionInfo(0, 0x1000, true));
+
+  // Range can have different tags for begin and end
+  // (which would make it look inverted if we didn't remove them)
+  // Note that range comes back with an untagged base and alginment
+  // applied.
+  MemoryTagManagerAArch64MTE::TagRange expected_range(0x0, 0x10);
+  llvm::Expected got =
+  manager.MakeTaggedRange(0x0f00, 0x0e01,
+  memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+  ASSERT_EQ(*got, expected_range);
+
+  // Error if the range isn't within any region
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0x1000, 0x1010, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x1000:0x1010 is not in a memory tagged region"));
+
+  // Error if the first part of a range isn't tagged
+  memory_regions.clear();
+  const char *err_msg =
+  "Address range 0x0:0x1000 is not in a memory tagged region";
+
+  // First because it has no region entry
+  memory_regions.push_back(MakeRegionInfo(0x10, 0x1000, true));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // Then because the first region is untagged
+  memory_regions.push_back(MakeRegionInfo(0, 0x10, false));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // If we tag that first part it succeeds
+  memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+  expected_range = MemoryTagManagerAArch64MTE::TagRange(0x0, 0x1000);
+  got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+  ASSERT_EQ(*got, expected_range);
+
+  // Error if the end of a range is untagged
+  memory_regions.clear();
+
+  // First because it has no region entry
+  memory_regions.push_back(MakeRegionInfo(0, 0xF00, true));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // Then because the last region is untagged
+  memory_regions.push_back(MakeRegionInfo(0xF00, 0x100, false));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // If we tag the last part it succeeds
+  memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+  got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded

[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> It will become a bit more complicated for a reader who have no knowledge 
> about memory tag range and disjoint region.

I've added/updated some of the comments in `MakeTaggedRange`. Maybe you can 
highlight any bits that aren't clear and I'll make sure they get comments too.




Comment at: lldb/include/lldb/Target/MemoryTagManager.h:66
+  // If so, return a modified range. This will be expanded to be
+  // granule aligned and have an untagged base address.
+  virtual llvm::Expected MakeTaggedRange(

omjavaid wrote:
> I couldnt understand this point in comment  "have an untagged base address" 
This is noted for 2 reasons:
* Callers don't need to remove tags before displaying the range (memory tag 
read).
* There's potentially 2 different tags, begin/end, so removing them is the 
easiest way to make (or rather not make) that choice.

You could keep the base tagged the same as `addr` but the code works out 
simpler if you don't and all it means is that `memory tag read` has to remove 
the tags itself. If it turns out that most callers want it tagged then sure 
I'll change the behaviour.



Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:70
 Process *process = m_exe_ctx.GetProcessPtr();
 llvm::Expected tag_manager_or_err =
+process->GetMemoryTagManager();

omjavaid wrote:
> If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we 
> can directly return a tag manager pointer here.
> 
> Also SupportsMemoryTagging may also run once for the life time of a process? 
> We can store this information is a flag or something.
> 
> 
> If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we 
> can directly return a tag manager pointer here.

Sure, that way `GetMemoryTagManager` does exactly what it says on the tin. I 
can just make a utility function in `CommandObjectMemoryTag.cpp` to reduce the 
duplication of checking both, when adding the write command.

Will do that in the next update.

> Also SupportsMemoryTagging may also run once for the life time of a process? 
> We can store this information is a flag or something.

It already is in that for the GDB remote it'll only send a `qSupported` the 
first time it (or any other property) is asked for. So we go through a few 
function calls but that's it.



Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:71
 llvm::Expected tag_manager_or_err =
-process->GetMemoryTagManager(start_addr, end_addr);
+process->GetMemoryTagManager();
 

omjavaid wrote:
> should we process pointer validity check before this call?
Covered by `eCommandRequiresProcess` in the constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

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

[Lldb-commits] [PATCH] D105698: [lldb/Target] Fix event handling during process launch

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added a reviewer: jingham.
mib requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch fixes process event handling when the events are broadcasted
at launch. To do so, the patch introduces a new listener to ensure the
event ordering.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105698

Files:
  lldb/source/Target/Process.cpp


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -10,6 +10,7 @@
 #include 
 #include 
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 
@@ -2470,6 +2471,11 @@
 if (error.Fail())
   return error;
 
+// Listen and queue events that are broadcasted during the process launch.
+ListenerSP listener_sp(Listener::MakeListener("LaunchEventHijack"));
+HijackProcessEvents(listener_sp);
+auto on_exit = llvm::make_scope_exit([this]() { RestoreProcessEvents(); });
+
 if (PrivateStateThreadIsValid())
   PausePrivateStateThread();
 


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -10,6 +10,7 @@
 #include 
 #include 
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 
@@ -2470,6 +2471,11 @@
 if (error.Fail())
   return error;
 
+// Listen and queue events that are broadcasted during the process launch.
+ListenerSP listener_sp(Listener::MakeListener("LaunchEventHijack"));
+HijackProcessEvents(listener_sp);
+auto on_exit = llvm::make_scope_exit([this]() { RestoreProcessEvents(); });
+
 if (PrivateStateThreadIsValid())
   PausePrivateStateThread();
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:70
 Process *process = m_exe_ctx.GetProcessPtr();
 llvm::Expected tag_manager_or_err =
+process->GetMemoryTagManager();

DavidSpickett wrote:
> omjavaid wrote:
> > If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we 
> > can directly return a tag manager pointer here.
> > 
> > Also SupportsMemoryTagging may also run once for the life time of a 
> > process? We can store this information is a flag or something.
> > 
> > 
> > If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we 
> > can directly return a tag manager pointer here.
> 
> Sure, that way `GetMemoryTagManager` does exactly what it says on the tin. I 
> can just make a utility function in `CommandObjectMemoryTag.cpp` to reduce 
> the duplication of checking both, when adding the write command.
> 
> Will do that in the next update.
> 
> > Also SupportsMemoryTagging may also run once for the life time of a 
> > process? We can store this information is a flag or something.
> 
> It already is in that for the GDB remote it'll only send a `qSupported` the 
> first time it (or any other property) is asked for. So we go through a few 
> function calls but that's it.
On second thought I'd rather leave `GetMemoryTagManager` as it is.

If we split the checks up we'll have the same 2 checks repeated in:
* `memory tag read`, `memory tag write` (same file so you could make a helper 
function)
* `Process::ReadMemoryTags`, `Process::WriteMemoryTags` (again you could make a 
helper but...)

That helper would be what `GetMemoryTagManager` is now. Leaving it as is saves 
duplicating it.

I thought of simply only checking `SupportsMemoryTagging` in 
Process::ReadMemoryTags. Problem with that is that you would never get this 
error if your remote wasn't able to show you MTE memory region flags. And you'd 
hope the most obvious checks would come first.

The other reason to split the 2 checks is so `GetMemoryTagManager` could be 
called from the API to get a manager even if the remote didn't support MTE. 
Which seems like a very niche use case, perhaps you've got some very old remote 
but are somehow running an MTE enabled program on it? Seems unlikely.

If we find a use case later when I work on the API then it'll be easy to switch 
it around anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

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

[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[David Spickett via Phabricator via lldb-commits]

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

Correct `//` to `///` in comment block for ReadMemoryTags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

Files:
  lldb/include/lldb/Target/MemoryTagManager.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Index: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
===
--- lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -94,6 +94,121 @@
   manager.ExpandToGranule(MemoryTagManagerAArch64MTE::TagRange(18, 4)));
 }
 
+static MemoryRegionInfo MakeRegionInfo(lldb::addr_t base, lldb::addr_t size,
+   bool tagged) {
+  return MemoryRegionInfo(
+  MemoryRegionInfo::RangeType(base, size), MemoryRegionInfo::eYes,
+  MemoryRegionInfo::eYes, MemoryRegionInfo::eYes, MemoryRegionInfo::eYes,
+  ConstString(), MemoryRegionInfo::eNo, 0,
+  /*memory_tagged=*/
+  tagged ? MemoryRegionInfo::eYes : MemoryRegionInfo::eNo);
+}
+
+TEST(MemoryTagManagerAArch64MTETest, MakeTaggedRange) {
+  MemoryTagManagerAArch64MTE manager;
+  MemoryRegionInfos memory_regions;
+
+  // No regions means no tagged regions, error
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0, 0x10, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x0:0x10 is not in a memory tagged region"));
+
+  // Alignment is done before checking regions.
+  // Here 1 is rounded up to the granule size of 0x10.
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0, 1, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x0:0x10 is not in a memory tagged region"));
+
+  // Range must not be inverted
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(1, 0, memory_regions),
+  llvm::FailedWithMessage(
+  "End address (0x0) must be greater than the start address (0x1)"));
+
+  // Adding a single region to cover the whole range
+  memory_regions.push_back(MakeRegionInfo(0, 0x1000, true));
+
+  // Range can have different tags for begin and end
+  // (which would make it look inverted if we didn't remove them)
+  // Note that range comes back with an untagged base and alginment
+  // applied.
+  MemoryTagManagerAArch64MTE::TagRange expected_range(0x0, 0x10);
+  llvm::Expected got =
+  manager.MakeTaggedRange(0x0f00, 0x0e01,
+  memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+  ASSERT_EQ(*got, expected_range);
+
+  // Error if the range isn't within any region
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0x1000, 0x1010, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x1000:0x1010 is not in a memory tagged region"));
+
+  // Error if the first part of a range isn't tagged
+  memory_regions.clear();
+  const char *err_msg =
+  "Address range 0x0:0x1000 is not in a memory tagged region";
+
+  // First because it has no region entry
+  memory_regions.push_back(MakeRegionInfo(0x10, 0x1000, true));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // Then because the first region is untagged
+  memory_regions.push_back(MakeRegionInfo(0, 0x10, false));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // If we tag that first part it succeeds
+  memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+  expected_range = MemoryTagManagerAArch64MTE::TagRange(0x0, 0x1000);
+  got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+  ASSERT_EQ(*got, expected_range);
+
+  // Error if the end of a range is untagged
+  memory_regions.clear();
+
+  // First because it has no region entry
+  memory_regions.push_back(MakeRegionInfo(0, 0xF00, true));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // Then because the last region is untagged
+  memory_regions.push_back(MakeRegionInfo(0xF00, 0x100, false));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // If we tag the last part it succeeds
+  memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+  got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+  ASSERT_

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D105470#2866731 , @werat wrote:

> Thanks for the prompt review!
>
>> This change is wrong for ValueObjectConstResult's. The original point of 
>> ValueObjectConstResult's was to store the results of expressions so that, 
>> even if the process is not at the original stop point, you could still check 
>> the value. It should have the value it had at the time the expression was 
>> evaluated. Updating the value is exactly what you don't want to do. So I 
>> think you need to make sure that we can't change their value after they are 
>> created.
>
> Looking at the current state of things and the available APIs, I don't think 
> this principle holds anymore. `ValueObjectConstResult` is also used for 
> values created via `SBTarget::CreateValueFromData`. As a user I don't see any 
> reason why I shouldn't be allowed to modify the value of an object I created 
> myself earlier. I would argue the same for the results of the expression 
> evaluation. Why are some values different from others?

Expression results store the results of an expression evaluation.  As such, it 
doesn't really make sense to change their values.  Moreover, I'd argue it is 
confusing to update them.  If I get the value of an object at time A and the 
try to fetch a child of it at time B, the value I would get is incoherent with 
the state of the object when I fetched it.  So I don't think those operations 
make much sense for ExpressionResults, and allowing them to change when you 
look at them again defeats their purpose, which is to store the results of the 
expression at the time it was evaluated.

Convenience variables made in the expression parser have a whole different 
meaning.  They are made by and their values controlled by the user of the 
expression parser.  You should be able to change them at will.  And since you 
might have handed them off to the program (for instance storing a convenience 
variable held object in some collection managed by the program, they need to be 
updated when program state changes.

>> Note, ExpressionResult variables are different from 
>> ExpressionPersistentVariables. The former should not be updated, and after 
>> the process moves on any children that weren't gathered become "unknown". 
>> But ExpressionPersistentVariables should be able to be assigned to, and when 
>> the reference target object, it should update them live. .
>
> Please, correct me if I'm wrong. In my example `ExpressionResult` is a value 
> returned by `EvaluateExpression` (i.e. `b`) and 
> `ExpressionPersistentVariable` is variable created by the expression 
> evaluator (i.e. `$b_0`), right? As far as I can tell, they're both 
> `ValueObjectConstResult` and both have the problem outlined in this patch:
>
>   frame0.EvaluateExpression("auto $b_0 = b1")
>   b = frame0.FindValue("$b_0", lldb.eValueTypeConstResult)
>   ...
>   # Same problem, updating the value of `b` doesn't invalidate children.

You are right in everything bug that ExpressionResult variables should not be 
updated when program state changes.

>   > The other thing to check about this patch is whether it defeats detecting 
> changed values in child elements. 
>   
>   This patch invalidates children only in `ValueObject::SetNeedsUpdate()` 
> which is called only from `SetData/SetValueFromCString` as far as I 
> understand. If you have a `ValueObjectVariable` which tracks some variable 
> that can be modified by the process, then it will continue to work fine. 
> `TestValueVarUpdate.py` passes successfully, but I didn't look to0 closely 
> yet whether it actually tests the scenario you described.

The fact that SetNeedsUpdate only gets called from SetData/SetValueFromCString 
seems to me to be just an accident, nothing enforces not using these when the 
program state changes, for instance.

> ---
>
> Looking at this from the user perspective, I would prefer to be able to 
> update any value, regardless of which API it came from. In my use case I rely 
> on this heavily -- 
> https://werat.dev/blog/blazing-fast-expression-evaluation-for-c-in-lldb/#maintaning-state.
> I could potentially live with the results of `EvaluateExpression` being 
> immutable, but being able to modify values created by `CreateValueFromData` 
> or persistent values like `$b_0` is necessary for me.

I'm less certain about updating "CreateValueFromData" variables.  Those tend to 
be used by Synthetic Child Providers, for instance when you've found an element 
of an NSDictionary, you use CreateValueFromData to produce the ValueObject that 
represents it.  But in these cases, the data formatters generally recreate 
these objects rather than update them.  If they were going to update them using 
SetValueFromCString and the like, you would then need to preserve "IsChanged" 
information in that process.

But for certain expression result variables should be changeable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST 

[Lldb-commits] [PATCH] D105327: [lldb] Add the ability to silently import scripted commands

[Jonas Devlieghere via Phabricator via lldb-commits]

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

I kept the existing behavior everywhere except when `--silent` is passed. I 
also don't think "being interactive" is a good enough discriminator, I can 
imagine some people might want to see the work done by the script. Happy to 
revisit all of that in a separate patch though.


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

https://reviews.llvm.org/D105327

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

[Lldb-commits] [lldb] f951735 - [lldb] Add the ability to silently import scripted commands

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-07-09T10:05:39-07:00
New Revision: f9517353959b4ef5c706e4356b56aeb9e0087584

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

LOG: [lldb] Add the ability to silently import scripted commands

Add the ability to silence command script import. The motivation for
this change is being able to add command script import -s
lldb.macosx.crashlog to your ~/.lldbinit without it printing the
following message at the beginning of every debug session.

  "malloc_info", "ptr_refs", "cstr_refs", "find_variable", and
  "objc_refs" commands have been installed, use the "--help" options on
  these commands for detailed help.

In addition to forwarding the silent option to LoadScriptingModule, this
also changes ScriptInterpreterPythonImpl::ExecuteOneLineWithReturn and
ScriptInterpreterPythonImpl::ExecuteMultipleLines to honor the enable IO
option in ExecuteScriptOptions, which until now was ignored.

Note that IO is only enabled (or disabled) at the start of a session,
and for this particular use case, that's done when taking the Python
lock in LoadScriptingModule, which means that the changes to these two
functions are not strictly necessary, but (IMO) desirable nonetheless.

Differential revision: https://reviews.llvm.org/D105327

Added: 
lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test

Modified: 
lldb/include/lldb/Interpreter/ScriptInterpreter.h
lldb/source/Commands/CommandObjectCommands.cpp
lldb/source/Commands/Options.td
lldb/source/Core/Module.cpp
lldb/source/Interpreter/ScriptInterpreter.cpp
lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

Removed: 




diff  --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h 
b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
index 8d1c38abffee1..e98e1be049f98 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -71,6 +71,28 @@ class ExecuteScriptOptions {
   bool m_maskout_errors = true;
 };
 
+class LoadScriptOptions {
+public:
+  LoadScriptOptions() = default;
+
+  bool GetInitSession() const { return m_init_session; }
+  bool GetSilent() const { return m_silent; }
+
+  LoadScriptOptions &SetInitSession(bool b) {
+m_init_session = b;
+return *this;
+  }
+
+  LoadScriptOptions &SetSilent(bool b) {
+m_silent = b;
+return *this;
+  }
+
+private:
+  bool m_init_session = false;
+  bool m_silent = false;
+};
+
 class ScriptInterpreterIORedirect {
 public:
   /// Create an IO redirect. If IO is enabled, this will redirects the output
@@ -509,7 +531,7 @@ class ScriptInterpreter : public PluginInterface {
   virtual bool CheckObjectExists(const char *name) { return false; }
 
   virtual bool
-  LoadScriptingModule(const char *filename, bool init_session,
+  LoadScriptingModule(const char *filename, const LoadScriptOptions &options,
   lldb_private::Status &error,
   StructuredData::ObjectSP *module_sp = nullptr,
   FileSpec extra_search_dir = {});

diff  --git a/lldb/source/Commands/CommandObjectCommands.cpp 
b/lldb/source/Commands/CommandObjectCommands.cpp
index c6b0f7fdf3b19..9a8b81c007ad8 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1242,6 +1242,9 @@ class CommandObjectCommandsScriptImport : public 
CommandObjectParsed {
   case 'c':
 relative_to_command_file = true;
 break;
+  case 's':
+silent = true;
+break;
   default:
 llvm_unreachable("Unimplemented option");
   }
@@ -1257,6 +1260,7 @@ class CommandObjectCommandsScriptImport : public 
CommandObjectParsed {
   return llvm::makeArrayRef(g_script_import_options);
 }
 bool relative_to_command_file = false;
+bool silent = false;
   };
 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
@@ -1278,7 +1282,10 @@ class CommandObjectCommandsScriptImport : public 
CommandObjectParsed {
 for (auto &entry : command.entries()) {
   Status error;
 
-  const bool init_session = true;
+  LoadScriptOptions options;
+  options.SetInitSession(true);
+  options.SetSilent(m_options.silent);
+
   // FIXME: this is necessary because CommandObject::CheckRequirements()
   // assumes that commands won't ever be recursively invoked, but it's
   // actually possible to craft a Python scri

[Lldb-commits] [PATCH] D105327: [lldb] Add the ability to silently import scripted commands

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf9517353959b: [lldb] Add the ability to silently import 
scripted commands (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D105327?vs=357037&id=357554#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105327

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Module.cpp
  lldb/source/Interpreter/ScriptInterpreter.cpp
  lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
  lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test

Index: lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test
@@ -0,0 +1,8 @@
+# REQUIRES: python
+
+# RUN: %lldb --script-language python -o 'command script import lldb.macosx.crashlog' 2>&1 | FileCheck %s
+# RUN: %lldb --script-language python -o 'command script import -s lldb.macosx.crashlog' 2>&1 | FileCheck %s --check-prefix SILENT
+# RUN: %lldb --script-language python -o 'command script import --silent lldb.macosx.crashlog' 2>&1 | FileCheck %s --check-prefix SILENT
+
+CHECK: commands have been installed
+SILENT-NOT: commands have been installed
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -234,7 +234,8 @@
   bool RunScriptFormatKeyword(const char *impl_function, ValueObject *value,
   std::string &output, Status &error) override;
 
-  bool LoadScriptingModule(const char *filename, bool init_session,
+  bool LoadScriptingModule(const char *filename,
+   const LoadScriptOptions &options,
lldb_private::Status &error,
StructuredData::ObjectSP *module_sp = nullptr,
FileSpec extra_search_dir = {}) override;
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -1077,11 +1077,24 @@
 llvm::StringRef in_string, ScriptInterpreter::ScriptReturnType return_type,
 void *ret_value, const ExecuteScriptOptions &options) {
 
+  llvm::Expected>
+  io_redirect_or_error = ScriptInterpreterIORedirect::Create(
+  options.GetEnableIO(), m_debugger, /*result=*/nullptr);
+
+  if (!io_redirect_or_error) {
+llvm::consumeError(io_redirect_or_error.takeError());
+return false;
+  }
+
+  ScriptInterpreterIORedirect &io_redirect = **io_redirect_or_error;
+
   Locker locker(this,
 Locker::AcquireLock | Locker::InitSession |
 (options.GetSetLLDBGlobals() ? Locker::InitGlobals : 0) |
 Locker::NoSTDIN,
-Locker::FreeAcquiredLock | Locker::TearDownSession);
+Locker::FreeAcquiredLock | Locker::TearDownSession,
+io_redirect.GetInputFile(), io_redirect.GetOutputFile(),
+io_redirect.GetErrorFile());
 
   PythonModule &main_module = GetMainModule();
   PythonDictionary globals = main_module.GetDictionary();
@@ -1190,11 +1203,22 @@
   if (in_string == nullptr)
 return Status();
 
+  llvm::Expected>
+  io_redirect_or_error = ScriptInterpreterIORedirect::Create(
+  options.GetEnableIO(), m_debugger, /*result=*/nullptr);
+
+  if (!io_redirect_or_error)
+return Status(io_redirect_or_error.takeError());
+
+  ScriptInterpreterIORedirect &io_redirect = **io_redirect_or_error;
+
   Locker locker(this,
 Locker::AcquireLock | Locker::InitSession |
 (options.GetSetLLDBGlobals() ? Locker::InitGlobals : 0) |
 Locker::NoSTDIN,
-Locker::FreeAcquiredLock | Locker::TearDownSession);
+Locker::FreeAcquiredLock | Locker::TearDownSession,
+io_redirect.GetInputFile(), io_redirect.GetOutputFile(),
+io_redirect.GetErrorFil

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 357578.
shafik added reviewers: jingham, jasonmolenda.
shafik added a comment.

Changing approach based on Adrian's comments.


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

https://reviews.llvm.org/D105564

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/Shell/SymbolFile/DWARF/x86/auto_return_symtab.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/auto_return_symtab.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/auto_return_symtab.s
@@ -0,0 +1,257 @@
+# This tests that lldb when using symbol table we are able to find the definition
+# for an auto return function.
+
+# RUN: llvm-mc -triple x86_64-apple-macosx10.15.0 %s -filetype=obj > %t.o
+# RUN: lldb-test symbols --dump-clang-ast %t.o | FileCheck %s
+
+# CHECK: CXXMethodDecl {{.*}} <>  f 'int ()'
+
+# This was compiled from the following code:
+#
+# struct A {
+# auto f();
+# };
+#
+# auto A::f() {
+# return 0;
+# }
+#
+# Compiled using:
+#
+#  -target x86_64-apple-macosx10.15.0
+#
+# and edited to remove some uneeded sections.
+
+	.section	__TEXT,__text,regular,pure_instructions
+	.globl	__ZN1A1fEv  ## -- Begin function _ZN1A1fEv
+	.p2align	4, 0x90
+__ZN1A1fEv: ## @_ZN1A1fEv
+Lfunc_begin0:
+	.cfi_startproc
+## %bb.0:   ## %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	movq	%rdi, -8(%rbp)
+Ltmp0:
+	xorl	%eax, %eax
+	popq	%rbp
+	retq
+Ltmp1:
+Lfunc_end0:
+	.cfi_endproc
+## -- End function
+	.section	__DWARF,__debug_abbrev,regular,debug
+Lsection_abbrev:
+	.byte	1   ## Abbreviation Code
+	.byte	17  ## DW_TAG_compile_unit
+	.byte	1   ## DW_CHILDREN_yes
+	.byte	37  ## DW_AT_producer
+	.byte	14  ## DW_FORM_strp
+	.byte	19  ## DW_AT_language
+	.byte	5   ## DW_FORM_data2
+	.byte	3   ## DW_AT_name
+	.byte	14  ## DW_FORM_strp
+	.ascii	"\202|" ## DW_AT_LLVM_sysroot
+	.byte	14  ## DW_FORM_strp
+	.ascii	"\357\177"  ## DW_AT_APPLE_sdk
+	.byte	14  ## DW_FORM_strp
+	.byte	16  ## DW_AT_stmt_list
+	.byte	23  ## DW_FORM_sec_offset
+	.byte	27  ## DW_AT_comp_dir
+	.byte	14  ## DW_FORM_strp
+	.byte	17  ## DW_AT_low_pc
+	.byte	1   ## DW_FORM_addr
+	.byte	18  ## DW_AT_high_pc
+	.byte	6   ## DW_FORM_data4
+	.byte	0   ## EOM(1)
+	.byte	0   ## EOM(2)
+	.byte	2   ## Abbreviation Code
+	.byte	19  ## DW_TAG_structure_type
+	.byte	1   ## DW_CHILDREN_yes
+	.byte	54  ## DW_AT_calling_convention
+	.byte	11  ## DW_FORM_data1
+	.byte	3   ## DW_AT_name
+	.byte	14  ## DW_FORM_strp
+	.byte	11  ## DW_AT_byte_size
+	.byte	11  ## DW_FORM_data1
+	.byte	58  ## DW_AT_decl_file
+	.byte	11  ## DW_FORM_data1
+	.byte	59  ## DW_AT_decl_line
+	.byte	11  ## DW_FORM_data1
+	.byte	0   ## EOM(1)
+	.byte	0   ## EOM(2)
+	.byte	3   ## Abbreviation Code
+	.byte	46  ## DW_TAG_subprogram
+	.byte	1   ## DW_CHILDREN_yes
+	.byte	110 ## DW_AT_linkage_name
+	.byte	14  ## DW_FORM_strp
+	.byte	3   ## DW_AT_name
+	.byte	14  ## DW_FORM_strp
+	.byte	58  ## DW_AT_decl_file
+	.byte	11  ## DW_FORM_data1
+	.byte	59  ## DW_AT_decl_line
+	.byte	11  ## DW_FORM_data1
+	.byte	73  ## DW_AT_type
+	.byte	19  ## DW_FORM_ref4
+	.byte	60  ## DW_AT_declaration
+	.byte	25  

[Lldb-commits] [PATCH] D105717: Create "thread trace dump stats" command

[hanbing wang via Phabricator via lldb-commits]

hanbingwang created this revision.
hanbingwang added reviewers: wallace, clayborg.
Herald added a subscriber: dang.
hanbingwang requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When the user types that command 'thread trace dump stats' and there's a 
running Trace session in LLDB, a raw trace in bytes should be printed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105717

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/test/API/commands/trace/TestTraceDumpStats.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -33,7 +33,8 @@
 # check that the Process and Thread objects were created correctly
 self.expect("thread info", substrs=["tid = 3842849"])
 self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
-
+self.expect("thread trace dump stats", substrs=['''thread #1: tid = 3842849
+raw trace size 4096'''])
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpStats.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpStats.py
@@ -0,0 +1,39 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpStats(TraceIntelPTTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump stats",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump stats",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("thread trace dump stats",
+substrs=["error: Process is not being traced"],
+error=True)
+
+def testDumpRawTraceSize(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump stats",
+substrs=['''thread #1: tid = 3842849
+raw trace size 4096'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
@@ -67,6 +67,10 @@
 
   lldb::TraceCursorUP GetCursor(Thread &thread) override;
 
+  void DumpTraceStats(Thread &thread, Stream &s, bool verbose) override;
+
+  llvm::Optional GetRawTraceSize(Thread &thread);
+
   void DoRefreshLiveProcessState(
   llvm::Expected state) override;
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -108,6 +108,24 @@
   return Decode(thread)->GetCursor();
 }
 
+void TraceIntelPT::DumpTraceStats(Thread &thread, Stream &s, bool verbose) {
+  Optional raw_size = GetRawTraceSize(thread);
+  s.Printf("thread #%u: tid = %" PRIu64, thread.GetIndexID(), thread.GetID());
+  if (!raw_size) {
+s.Printf(", not traced\n");
+return;
+  }
+  s.Printf("\nraw trace size %zu\n", *raw_size);
+  return;
+}
+
+Optional TraceIntelPT::GetRawTraceSize(Thread &thread) {
+  if (IsTraced(thread))
+return Decode(thread)->GetRawTraceSize();
+  else
+return None;
+}
+
 Expected TraceIntelPT::GetCPUInfoForLiveProcess() {
   Expected> cpu_info = GetLiveProcessBinaryData("cpuInfo");
   if (!cpu_info)
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
@@ -194,7 +194,7 @@
 
 static Expected>
 De

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@aprantl after your comments and discussion offline I changed my approach to do 
this lookup using the symbol table and it worked out.

The main issue with the first approach was that gcc would also have to be 
updated in order for them to change their approach to generating debug info as 
well and that felt a lot harder to justify.

This is also a simpler approach, it requires fewer changes.


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

https://reviews.llvm.org/D105564

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

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> Currently when we have a member function that has an auto return type and the 
> definition is out of line we generate two DWARF DIE.
> One for the declaration and a second one for the definition, the definition 
> holds the deduced type but does not contain a DW_AT_name nor
> a DW_AT_linkage_name so there was not way to look up the definition DIE.

Regarding the DWARF, the definition DIE should have a DW_AT_specification that 
points back to the declaration; is that missing here?
I'm not familiar with LLDB so it's likely I'm misunderstanding the problem.


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

https://reviews.llvm.org/D105564

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

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D105564#2867717 , @probinson wrote:

>> Currently when we have a member function that has an auto return type and 
>> the definition is out of line we generate two DWARF DIE.
>> One for the declaration and a second one for the definition, the definition 
>> holds the deduced type but does not contain a DW_AT_name nor
>> a DW_AT_linkage_name so there was not way to look up the definition DIE.
>
> Regarding the DWARF, the definition DIE should have a DW_AT_specification 
> that points back to the declaration; is that missing here?
> I'm not familiar with LLDB so it's likely I'm misunderstanding the problem.

Apologies, I should have updated the description as well, portions of that 
description are out of date with the new approach I just updated to.


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

https://reviews.llvm.org/D105564

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

[Lldb-commits] [lldb] c476566 - [IRForTarget] Don't pass nullptr to GetElementPtrInst::Create() (NFC)

[Nikita Popov via lldb-commits]

Author: Nikita Popov
Date: 2021-07-09T21:14:41+02:00
New Revision: c476566be5d025a3f122392af481a74f887911e6

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

LOG: [IRForTarget] Don't pass nullptr to GetElementPtrInst::Create() (NFC)

In one case use the source element type of the original GEP. In the
other the correct type isn't obvious to me, so use
getPointerElementType() for now.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
index 15d160787ea4..b3188ec56fdf 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
@@ -14,6 +14,7 @@
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/Operator.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
@@ -1574,7 +1575,8 @@ bool IRForTarget::UnfoldConstant(Constant *old_constant,
   FunctionValueCache get_element_pointer_maker(
   [&value_maker, &entry_instruction_finder, old_constant,
constant_expr](llvm::Function *function) -> llvm::Value * {
-Value *ptr = constant_expr->getOperand(0);
+auto *gep = cast(constant_expr);
+Value *ptr = gep->getPointerOperand();
 
 if (ptr == old_constant)
   ptr = value_maker.GetValue(function);
@@ -1597,7 +1599,7 @@ bool IRForTarget::UnfoldConstant(Constant *old_constant,
 ArrayRef indices(index_vector);
 
 return GetElementPtrInst::Create(
-nullptr, ptr, indices, "",
+gep->getSourceElementType(), ptr, indices, "",
 llvm::cast(
 entry_instruction_finder.GetValue(function)));
   });
@@ -1780,7 +1782,8 @@ bool IRForTarget::ReplaceVariables(Function 
&llvm_function) {
 ConstantInt *offset_int(
 ConstantInt::get(offset_type, offset, true));
 GetElementPtrInst *get_element_ptr = GetElementPtrInst::Create(
-nullptr, argument, offset_int, "", entry_instruction);
+argument->getType()->getPointerElementType(), argument,
+offset_int, "", entry_instruction);
 
 if (name == m_result_name && !m_result_is_pointer) {
   BitCastInst *bit_cast = new BitCastInst(



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

[Lldb-commits] [lldb] 2e3f469 - [IR] Add GEPOperator::indices() (NFC)

[Nikita Popov via lldb-commits]

Author: Nikita Popov
Date: 2021-07-09T21:41:20+02:00
New Revision: 2e3f4694d61dd50c8ec1278331edee84a60555f8

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

LOG: [IR] Add GEPOperator::indices() (NFC)

In order to mirror the GetElementPtrInst::indices() API.

Wanted to use this in the IRForTarget code, and was surprised to
find that it didn't exist yet.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
llvm/include/llvm/IR/Operator.h
llvm/lib/Analysis/ScalarEvolution.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
index b3188ec56fdf..5655d548ee34 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
@@ -1582,14 +1582,7 @@ bool IRForTarget::UnfoldConstant(Constant *old_constant,
   ptr = value_maker.GetValue(function);
 
 std::vector index_vector;
-
-unsigned operand_index;
-unsigned num_operands = constant_expr->getNumOperands();
-
-for (operand_index = 1; operand_index < num_operands;
- ++operand_index) {
-  Value *operand = constant_expr->getOperand(operand_index);
-
+for (Value *operand : gep->indices()) {
   if (operand == old_constant)
 operand = value_maker.GetValue(function);
 

diff  --git a/llvm/include/llvm/IR/Operator.h b/llvm/include/llvm/IR/Operator.h
index 46c1dfc3e735..d0bce742cc96 100644
--- a/llvm/include/llvm/IR/Operator.h
+++ b/llvm/include/llvm/IR/Operator.h
@@ -488,6 +488,14 @@ class GEPOperator
   inline op_iterator   idx_end() { return op_end(); }
   inline const_op_iterator idx_end()   const { return op_end(); }
 
+  inline iterator_range indices() {
+return make_range(idx_begin(), idx_end());
+  }
+
+  inline iterator_range indices() const {
+return make_range(idx_begin(), idx_end());
+  }
+
   Value *getPointerOperand() {
 return getOperand(0);
   }
@@ -544,7 +552,7 @@ class GEPOperator
   }
 
   unsigned countNonConstantIndices() const {
-return count_if(make_range(idx_begin(), idx_end()), [](const Use& use) {
+return count_if(indices(), [](const Use& use) {
 return !isa(*use);
   });
   }

diff  --git a/llvm/lib/Analysis/ScalarEvolution.cpp 
b/llvm/lib/Analysis/ScalarEvolution.cpp
index 843c04855bf7..221b116d4371 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -5732,8 +5732,8 @@ const SCEV *ScalarEvolution::createNodeForGEP(GEPOperator 
*GEP) {
 return getUnknown(GEP);
 
   SmallVector IndexExprs;
-  for (auto Index = GEP->idx_begin(); Index != GEP->idx_end(); ++Index)
-IndexExprs.push_back(getSCEV(*Index));
+  for (Value *Index : GEP->indices())
+IndexExprs.push_back(getSCEV(Index));
   return getGEPExpr(GEP, IndexExprs);
 }
 



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

[Lldb-commits] [lldb] 488fcea - [lldb] Use custom script instead of lldb.macosx.crashlog in test

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-07-09T12:42:11-07:00
New Revision: 488fcea3b552e6cbc96eb767abdfb11f4184b812

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

LOG: [lldb] Use custom script instead of lldb.macosx.crashlog in test

I'm not entirely sure this is the problem, but the Windows bot doesn't
seem to like this test. Let's do something similar to
command_import.test which doesn't have that issue.

Added: 


Modified: 
lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test

Removed: 




diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test 
b/lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test
index 58e15439a008..d6a42a4f7aff 100644
--- a/lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test
+++ b/lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test
@@ -1,8 +1,11 @@
 # REQUIRES: python
 
-# RUN: %lldb --script-language python -o 'command script import 
lldb.macosx.crashlog' 2>&1 | FileCheck %s
-# RUN: %lldb --script-language python -o 'command script import -s 
lldb.macosx.crashlog' 2>&1 | FileCheck %s --check-prefix SILENT
-# RUN: %lldb --script-language python -o 'command script import --silent 
lldb.macosx.crashlog' 2>&1 | FileCheck %s --check-prefix SILENT
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: echo "print('Rene Magritte')" >> %t/foo.py
 
-CHECK: commands have been installed
-SILENT-NOT: commands have been installed
+# RUN: %lldb --script-language python -o 'command script import %t/foo.py' 
2>&1 | FileCheck %s
+# RUN: %lldb --script-language python -o 'command script import -s %t/foo.py' 
2>&1 | FileCheck %s --check-prefix SILENT
+# RUN: %lldb --script-language python -o 'command script import --silent 
%t/foo.py' 2>&1 | FileCheck %s --check-prefix SILENT
+
+CHECK: Rene Magritte
+SILENT-NOT: Rene Magritte



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

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105649

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

[Lldb-commits] [lldb] 3338819 - [lldb] Drop REQUIRES where redundant because of lit.local.cfg

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-07-09T13:25:10-07:00
New Revision: 3338819b08faa7f23f65fb4e67154583984ebf5c

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

LOG: [lldb] Drop REQUIRES where redundant because of lit.local.cfg

Added: 


Modified: 
lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_callback.test
lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_function_callback.test
lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test
lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
lldb/test/Shell/ScriptInterpreter/Lua/fail_breakpoint_oneline.test
lldb/test/Shell/ScriptInterpreter/Lua/independent_state.test
lldb/test/Shell/ScriptInterpreter/Lua/io.test
lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
lldb/test/Shell/ScriptInterpreter/Lua/lua.test
lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test
lldb/test/Shell/ScriptInterpreter/Lua/partial_statements.test
lldb/test/Shell/ScriptInterpreter/Lua/persistent_state.test
lldb/test/Shell/ScriptInterpreter/Lua/print.test
lldb/test/Shell/ScriptInterpreter/Lua/quit.test
lldb/test/Shell/ScriptInterpreter/Lua/watchpoint_callback.test
lldb/test/Shell/ScriptInterpreter/Python/command_import.test
lldb/test/Shell/ScriptInterpreter/Python/command_relative_import.test
lldb/test/Shell/ScriptInterpreter/Python/fail_breakpoint_oneline.test
lldb/test/Shell/ScriptInterpreter/Python/python.test
lldb/test/Shell/ScriptInterpreter/Python/sb_address_exception.test
lldb/test/Shell/ScriptInterpreter/Python/scripted_breakpoint.test
lldb/test/Shell/ScriptInterpreter/Python/silent_command_script_import.test

Removed: 




diff  --git a/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
index db0df1694e9e4..d453f11f1ec76 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
@@ -1,4 +1,3 @@
-# REQUIRES: lua
 # UNSUPPORTED: lldb-repro
 #
 # RUN: cat %s | %lldb --script-language lua 2>&1 | FileCheck %s

diff  --git a/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_callback.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_callback.test
index 574873d2c0b63..f2ad440227af1 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_callback.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_callback.test
@@ -1,4 +1,3 @@
-# REQUIRES: lua
 # RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t
 # RUN: %lldb -s %s --script-language lua %t 2>&1 | FileCheck %s
 b main

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_function_callback.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_function_callback.test
index 69dd6818140a1..125913f7e9bfc 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_function_callback.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_function_callback.test
@@ -1,4 +1,3 @@
-# REQUIRES: lua
 # RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t
 # RUN: %lldb -s %s --script-language lua %t 2>&1 | FileCheck %s
 b main

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
index ef07a8af4d1f0..8424cecf37f72 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
@@ -1,4 +1,3 @@
-# REQUIRES: lua
 # RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t
 # RUN: %lldb -s %s --script-language lua %t 2>&1 | FileCheck %s
 b main

diff  --git a/lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test
index 6a0692d33efbc..719e71191fa44 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test
@@ -1,4 +1,3 @@
-# REQUIRES: lua
 # RUN: %lldb --script-language lua -o 'command script import 
%S/Inputs/testmodule.lua' -o 'script testmodule.foo()' 2>&1 | FileCheck %s
 # CHECK: Hello World!
 

diff  --git a/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
index 1b26bd0e2eade..6ebcb953a96e0 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
@@ -1,4 +1,3 @@
-# REQUIRES: lua
 # UNSUPPORTED: lldb-repro
 #
 # This tests that the convenience variables are n

[Lldb-commits] [lldb] d124133 - Add scoped timers to ReadMemoryFromInferior and ReadMemoryFromFileCache.

[Adrian Prantl via lldb-commits]

Author: Adrian Prantl
Date: 2021-07-09T13:37:04-07:00
New Revision: d124133f17357536c4034cb2b2c0bec537cd8fd5

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

LOG: Add scoped timers to ReadMemoryFromInferior and ReadMemoryFromFileCache.

Added: 


Modified: 
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp

Removed: 




diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f139479db13bd..c6411f6c2244b 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -68,6 +68,7 @@
 #include "lldb/Utility/ProcessInfo.h"
 #include "lldb/Utility/SelectHelper.h"
 #include "lldb/Utility/State.h"
+#include "lldb/Utility/Timer.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -2044,6 +2045,8 @@ size_t Process::ReadCStringFromMemory(addr_t addr, char 
*dst,
 
 size_t Process::ReadMemoryFromInferior(addr_t addr, void *buf, size_t size,
Status &error) {
+  LLDB_SCOPED_TIMER();
+
   if (buf == nullptr || size == 0)
 return 0;
 

diff  --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index e73de43ef78df..839eb58878933 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1691,6 +1691,7 @@ bool Target::ModuleIsExcludedForUnconstrainedSearches(
 
 size_t Target::ReadMemoryFromFileCache(const Address &addr, void *dst,
size_t dst_len, Status &error) {
+  LLDB_SCOPED_TIMER();
   SectionSP section_sp(addr.GetSection());
   if (section_sp) {
 // If the contents of this section are encrypted, the on-disk file is



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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace dump ctf -f ` command

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, clayborg.
Herald added subscribers: dang, mgorny.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This diff enables visualization of Intel PT traces by converting a summarized 
trace representation (HTR) to Chrome Trace Format (CTF) via the `thread trace 
dump ctf -f ` command - see attached video for an example of Intel PT 
trace visualization.

**Context**
To efficiently store and operate on traces we introduce HTR (Hierarchical Trace 
Representation). HTR is designed to be extensible and agnostic to trace type. 
In HTR, a trace is transformed in a way akin to compiler passes. Each pass 
modifies the trace and outputs a new HTR-based representation of the trace. A 
**layer** is each instance of trace data between passes. 
A layer is composed of **blocks** - a block in //layer n// refers to a sequence 
of blocks in //layer n - 1//. The blocks in the first layer correspond to a 
dynamic instruction in the trace.

A **pass** is applied to a layer to extract useful information (summarization) 
and compress the trace representation into a new layer. The idea is to have a 
series of passes where each pass specializes in extracting certain information 
about the trace. Some examples of potential passes include: identifying 
functions, identifying loops, or a more general purpose such as identifying 
long sequences of instructions that are repeated. This diff contains one such 
pass - //Basic Super Block Reduction//.

**Overview of Changes**

- Add basic HTR structures (layer, block, block metadata)
- Implement Super Basic Block Reduction Pass (HeadsAndTailsMerge in the code) 
to identify and merge patterns of repeated instructions
- Add 'thread trace dump ctf' command to export the HTR of an Intel PT trace to 
Chrome Trace Format (CTF)

F17851042: lldbdemo.mov 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105741

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceHTR.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceHTR.cpp
  lldb/test/API/commands/trace/TestTraceDumpCTF.py

Index: lldb/test/API/commands/trace/TestTraceDumpCTF.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpCTF.py
@@ -0,0 +1,68 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+import os
+
+class TestTraceDumpInstructions(TraceIntelPTTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump ctf",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump ctf",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("thread trace dump ctf",
+substrs=["error: Process is not being traced"],
+error=True)
+
+def testDumpCTF(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+ctf_test_file = self.getBuildArtifact("ctf-test.json")
+
+# file name, no count
+if os.path.exists(ctf_test_file):
+remove_file(ctf_test_file)
+self.expect(f"thread trace dump ctf --file {ctf_test_file}",
+substrs=["Success", f"{ctf_test_file}", "21 Total Blocks"])
+self.assertTrue(os.path.exists(ctf_test_file))
+
+# file name, "normal" count
+if os.path.exists(ctf_test_file):
+remove_file(ctf_test_file)
+self.expect(f"thread trace dump ctf --file {ctf_test_file} --count 10",
+substrs=["Success", f"{ctf_test_file}", "10 Total Blocks"])
+self.assertTrue(os.path.exists(ctf_test_file))
+
+# file name, count exceeding size of trace (21 instructions)
+if os.path.exists(ctf_test_file):
+remove_file(ctf_test_file)
+self.expect(f"thread trace dump ctf --file {ctf_test_file} --count 34",
+substrs=["Success", f"{ctf_test_file}", "21 Total Blocks"])
+self.assertTrue(os.path.exists(ctf_test_file))
+

[Lldb-commits] [PATCH] D105698: [lldb/Target] Fix event handling during process launch

[Jim Ingham via Phabricator via lldb-commits]

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

This LGTM.  This code is fetching events by hand off the event queue and then 
resending them, so it makes sense that it should stop the ones it is handling 
from leaking out to the Process listener.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105698

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