[Lldb-commits] [PATCH] D77444: [commands] Support autorepeat in SBCommands

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/unittests/API/CMakeLists.txt:2
+add_lldb_unittest(APITests
+  TestSBCommandInterpreterTest.cpp
+

Well.. this is now definitely going to get tested well. :P

While we do have some inconsistency where some test files put "Test" as a 
prefix and others as a suffix, we definitely don't need to have both in the 
same file. The Test suffix is a prevailing convention (792 vs. 62), so I 
recommend sticking to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77444



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

[Lldb-commits] [lldb] 1d3b737 - [lldb] Fixing the bug that the "log timer" has no tab completion

[Raphael Isemann via lldb-commits]

Author: Shu Anzai
Date: 2020-04-09T08:58:52+02:00
New Revision: 1d3b7370c466eba4bf22dce4a51f885f76698053

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

LOG: [lldb] Fixing the bug that the "log timer" has no tab completion

I fixed the bug that the "log timer" has no tab command.

Original code has the only CommandObjectLogTimer class, but it is not
sufficient. Thus I divided the content of CommandObjectLog class into
CommandObjectLogEnable class, CommandObjectLogDisable class,
CommandObjectLogDump class, CommandObjectLogReset class,
CommandObjectLogIncrement class.

Reviewed by: teemperor

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectLog.cpp
lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectLog.cpp 
b/lldb/source/Commands/CommandObjectLog.cpp
index 233831f6678d..4016b07c91ed 100644
--- a/lldb/source/Commands/CommandObjectLog.cpp
+++ b/lldb/source/Commands/CommandObjectLog.cpp
@@ -298,61 +298,170 @@ class CommandObjectLogList : public CommandObjectParsed {
   }
 };
 
-class CommandObjectLogTimer : public CommandObjectParsed {
+class CommandObjectLogTimerEnable : public CommandObjectParsed {
 public:
   // Constructors and Destructors
-  CommandObjectLogTimer(CommandInterpreter &interpreter)
-  : CommandObjectParsed(interpreter, "log timers",
-"Enable, disable, dump, and reset LLDB internal "
-"performance timers.",
-"log timers < enable  | disable | dump | "
-"increment  | reset >") {}
+  CommandObjectLogTimerEnable(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "log timers enable",
+"enable LLDB internal performance timers",
+"log timers enable ") {
+CommandArgumentEntry arg;
+CommandArgumentData depth_arg;
 
-  ~CommandObjectLogTimer() override = default;
+// Define the first (and only) variant of this arg.
+depth_arg.arg_type = eArgTypeCount;
+depth_arg.arg_repetition = eArgRepeatOptional;
+
+// There is only one variant this argument could be; put it into the
+// argument entry.
+arg.push_back(depth_arg);
+
+// Push the data for the first argument into the m_arguments vector.
+m_arguments.push_back(arg);
+  }
+
+  ~CommandObjectLogTimerEnable() override = default;
+
+protected:
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+result.SetStatus(eReturnStatusFailed);
+
+if (args.GetArgumentCount() == 0) {
+  Timer::SetDisplayDepth(UINT32_MAX);
+  result.SetStatus(eReturnStatusSuccessFinishNoResult);
+} else if (args.GetArgumentCount() == 1) {
+  uint32_t depth;
+  if (args[0].ref().consumeInteger(0, depth)) {
+result.AppendError(
+"Could not convert enable depth to an unsigned integer.");
+  } else {
+Timer::SetDisplayDepth(depth);
+result.SetStatus(eReturnStatusSuccessFinishNoResult);
+  }
+}
+
+if (!result.Succeeded()) {
+  result.AppendError("Missing subcommand");
+  result.AppendErrorWithFormat("Usage: %s\n", m_cmd_syntax.c_str());
+}
+return result.Succeeded();
+  }
+};
+
+class CommandObjectLogTimerDisable : public CommandObjectParsed {
+public:
+  // Constructors and Destructors
+  CommandObjectLogTimerDisable(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "log timers disable",
+"disable LLDB internal performance timers",
+nullptr) {}
+
+  ~CommandObjectLogTimerDisable() override = default;
+
+protected:
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+Timer::DumpCategoryTimes(&result.GetOutputStream());
+Timer::SetDisplayDepth(0);
+result.SetStatus(eReturnStatusSuccessFinishResult);
+
+if (!result.Succeeded()) {
+  result.AppendError("Missing subcommand");
+  result.AppendErrorWithFormat("Usage: %s\n", m_cmd_syntax.c_str());
+}
+return result.Succeeded();
+  }
+};
+
+class CommandObjectLogTimerDump : public CommandObjectParsed {
+public:
+  // Constructors and Destructors
+  CommandObjectLogTimerDump(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "log timers dump",
+"dump LLDB internal performance timers", nullptr) 
{}
+
+  ~CommandObjectLogTimerDump() override = default;
+
+protected:
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+Timer::DumpCategoryTimes(&result.GetOutputStream());
+result.SetStatus(eReturnStatusSuccessFinishResult);
+
+if (!resu

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:477
 ifeq (1,$(USE_LIBDL))
-   ifneq ($(OS),NetBSD)
+   ifeq (,$(filter $(OS), NetBSD Windows_NT))
LDFLAGS += -ldl

amccarth wrote:
> I'm no expert in makefile syntax (especially not for Unix-y versions), but 
> this looks weird, with the leading comma and the matching parenthesis for 
> `$(filter` sitting near the end of the expression.  Is this right?
Yes, it is.

The "leading" comma introduces a empty string as the first argument to `ifeq`. 
Then the `$filter` thingy will turn `$OS` into an empty string iff it matches 
one of the strings it got as an argument (technically it will just remove any 
subwords which match those strings, but since `$OS` is just a single word, the 
effect is the same). 
So this is a very convoluted way of writing `$(OS) in [NetBSD, Windows_NT]` but:
a) it is consistent with other usages in these makefiles
b) I don't actually know of a better way to write that. If you do, let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77662



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

[Lldb-commits] [PATCH] D77771: [lldb/Docs] Document driver and inline replay.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is really great, and a lot more than I hoped for (I was expecting a couple 
of doxygen comments). I have just two remarks:

- the "Inline Replay" section is written in past tense, while the rest of the 
document uses present statement-of-fact tense. It would be good to rephrase 
that to keep the style consistent.
- there's some confusion about the terminology, this patch uses the term 
"inline" replay, whereas the SB function name is "APIReplay". Part of reason 
for my confusion was that we also have "inline" tests, and so I thought the 
other patch refers to some special logic needed to replay inline tests (I guess 
that would have been obvious if I actually opened the patch, but I was busy so 
I figured I'd just start with the first patch in the series so we make some 
progress). "API" replay also doesn't really make it clear who is it that is 
replaying the API calls. I actually very much liked the terms "active" and 
"passive", which capture that disctinction very well and also don't conflict 
with anything else in lldb. Here they are mentioned as a sort of an after 
thought, but maybe we could make it so that these are the official names for 
the two modes ?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D1



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

[Lldb-commits] [PATCH] D76906: [lldb] Fixing the bug that the "log timer" has no tab completion

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1d3b7370c466: [lldb] Fixing the bug that the "log 
timer" has no tab completion (authored by gedatsu217, committed by 
teemperor).
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D76906?vs=253683&id=256205#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76906

Files:
  lldb/source/Commands/CommandObjectLog.cpp
  lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py

Index: lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
===
--- lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
+++ lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
@@ -15,8 +15,3 @@
 def test_disable_empty(self):
 self.expect("log disable", error=True,
 substrs=["error: log disable takes a log channel and one or more log types."])
-
-@no_debug_info_test
-def test_timer_empty(self):
-self.expect("log timers", error=True,
-substrs=["error: Missing subcommand"])
Index: lldb/source/Commands/CommandObjectLog.cpp
===
--- lldb/source/Commands/CommandObjectLog.cpp
+++ lldb/source/Commands/CommandObjectLog.cpp
@@ -298,61 +298,170 @@
   }
 };
 
-class CommandObjectLogTimer : public CommandObjectParsed {
+class CommandObjectLogTimerEnable : public CommandObjectParsed {
 public:
   // Constructors and Destructors
-  CommandObjectLogTimer(CommandInterpreter &interpreter)
-  : CommandObjectParsed(interpreter, "log timers",
-"Enable, disable, dump, and reset LLDB internal "
-"performance timers.",
-"log timers < enable  | disable | dump | "
-"increment  | reset >") {}
+  CommandObjectLogTimerEnable(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "log timers enable",
+"enable LLDB internal performance timers",
+"log timers enable ") {
+CommandArgumentEntry arg;
+CommandArgumentData depth_arg;
 
-  ~CommandObjectLogTimer() override = default;
+// Define the first (and only) variant of this arg.
+depth_arg.arg_type = eArgTypeCount;
+depth_arg.arg_repetition = eArgRepeatOptional;
+
+// There is only one variant this argument could be; put it into the
+// argument entry.
+arg.push_back(depth_arg);
+
+// Push the data for the first argument into the m_arguments vector.
+m_arguments.push_back(arg);
+  }
+
+  ~CommandObjectLogTimerEnable() override = default;
+
+protected:
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+result.SetStatus(eReturnStatusFailed);
+
+if (args.GetArgumentCount() == 0) {
+  Timer::SetDisplayDepth(UINT32_MAX);
+  result.SetStatus(eReturnStatusSuccessFinishNoResult);
+} else if (args.GetArgumentCount() == 1) {
+  uint32_t depth;
+  if (args[0].ref().consumeInteger(0, depth)) {
+result.AppendError(
+"Could not convert enable depth to an unsigned integer.");
+  } else {
+Timer::SetDisplayDepth(depth);
+result.SetStatus(eReturnStatusSuccessFinishNoResult);
+  }
+}
+
+if (!result.Succeeded()) {
+  result.AppendError("Missing subcommand");
+  result.AppendErrorWithFormat("Usage: %s\n", m_cmd_syntax.c_str());
+}
+return result.Succeeded();
+  }
+};
+
+class CommandObjectLogTimerDisable : public CommandObjectParsed {
+public:
+  // Constructors and Destructors
+  CommandObjectLogTimerDisable(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "log timers disable",
+"disable LLDB internal performance timers",
+nullptr) {}
+
+  ~CommandObjectLogTimerDisable() override = default;
+
+protected:
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+Timer::DumpCategoryTimes(&result.GetOutputStream());
+Timer::SetDisplayDepth(0);
+result.SetStatus(eReturnStatusSuccessFinishResult);
+
+if (!result.Succeeded()) {
+  result.AppendError("Missing subcommand");
+  result.AppendErrorWithFormat("Usage: %s\n", m_cmd_syntax.c_str());
+}
+return result.Succeeded();
+  }
+};
+
+class CommandObjectLogTimerDump : public CommandObjectParsed {
+public:
+  // Constructors and Destructors
+  CommandObjectLogTimerDump(CommandInterpreter &interpreter)
+  : CommandObjectParsed(interpreter, "log timers dump",
+"dump LLDB internal performance timers", nullptr) {}
+
+  ~CommandObjectLogTimerDump() override = default;
+
+protected:
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+Timer

[Lldb-commits] [PATCH] D77588: [lldb/Reproducers] Make it possible to capture reproducers for the Python test suite.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, I'm starting to understand what's going on here. This patch looks mostly 
fine, but IIUC, it won't really do anything until the subsequent patch lands as 
that is what implements the inline/api/passive replay. I'm going to start 
looking at that other patch now.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D77588



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

[Lldb-commits] [PATCH] D77765: Fix incorrect L1 inferior memory cache flushing

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 256207.
jarin marked 10 inline comments as done.
jarin added a comment.

Addressed some of the reviewer comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77765

Files:
  lldb/include/lldb/Target/Memory.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Memory.cpp
  lldb/source/Target/Process.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/MemoryCacheTest.cpp

Index: lldb/unittests/Target/MemoryCacheTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/MemoryCacheTest.cpp
@@ -0,0 +1,128 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Target/Memory.h"
+#include "lldb/Utility/Status.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+
+class MemoryCacheTest : public MemoryFromInferiorReader, public testing::Test {
+  static const size_t k_memory_size = 256;
+  static const uint64_t k_cache_line_size = 16;
+
+  size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
+Status &error) override {
+m_inferior_read_count++;
+
+if (vm_addr >= m_memory.size())
+  return 0;
+
+size = std::min(size, m_memory.size() - vm_addr);
+memcpy(buf, m_memory.data() + vm_addr, size);
+return size;
+  }
+
+public:
+  MemoryCacheTest()
+  : m_cache(*this, k_cache_line_size), m_inferior_read_count(0) {
+// Fill memory from [0x0 - 0x256) with byte values that match the index. We
+// will use this memory in each test and each test will start with a fresh
+// copy.
+for (size_t i = 0; i < k_memory_size; i++)
+  m_memory.push_back(static_cast(i));
+  }
+
+  void AddRangeToL1Cache(lldb::addr_t addr, size_t len) {
+m_cache.AddL1CacheData(addr, m_memory.data() + addr, len);
+  }
+
+  void IncrementAndFlushRange(lldb::addr_t addr, size_t len) {
+for (size_t i = 0; i < len; i++)
+  m_memory[addr + i]++;
+m_cache.Flush(addr, len);
+  }
+
+  uint8_t ReadByte(lldb::addr_t addr) {
+uint8_t result;
+Status error;
+m_cache.Read(addr, &result, 1, error);
+EXPECT_TRUE(error.Success());
+return result;
+  }
+
+protected:
+  MemoryCache m_cache;
+  std::vector m_memory;
+  size_t m_inferior_read_count;
+};
+
+TEST_F(MemoryCacheTest, L1FlushSimple) {
+  // Add a byte to the cache.
+  AddRangeToL1Cache(10, 1);
+
+  // Sanity check - memory should agree with what ReadByte returns.
+  EXPECT_EQ(10, m_memory[10]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+  // Check that we have hit the cache and not the inferior's memory.
+  EXPECT_EQ(0u, m_inferior_read_count);
+
+  IncrementAndFlushRange(10, 1);
+
+  // Check the memory is incremented and the real memory agrees with ReadByte's
+  // result.
+  EXPECT_EQ(11, m_memory[11]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+}
+
+// Let us do a parametrized test that caches two ranges. In the test, we then
+// try to modify (and flush) various regions of memory and check that the cache
+// still returns the correct values from reads.
+using AddrRange = Range;
+class MemoryCacheParametrizedTest
+: public MemoryCacheTest,
+  public testing::WithParamInterface {};
+
+TEST_P(MemoryCacheParametrizedTest, L1FlushWithTwoRegions) {
+  // Add two ranges to the cache.
+  std::vector cached = {{10, 10}, {30, 10}};
+  for (AddrRange cached_range : cached)
+AddRangeToL1Cache(cached_range.base, cached_range.size);
+
+  // Flush the range given by the parameter.
+  AddrRange flush_range = GetParam();
+  IncrementAndFlushRange(flush_range.base, flush_range.size);
+
+  // Check that reads from the cached ranges read the inferior's memory
+  // if and only if the flush range intersects with one of the cached ranges.
+  bool has_intersection = false;
+  for (AddrRange cached_range : cached) {
+if (flush_range.DoesIntersect(cached_range))
+  has_intersection = true;
+
+for (size_t i = 0; i < cached_range.size; i++)
+  ReadByte(cached_range.base + i);
+  }
+  EXPECT_EQ(has_intersection, m_inferior_read_count > 0);
+
+  // Sanity check: check the memory contents.
+  for (size_t i = 0; i < 50; i++) {
+EXPECT_EQ(m_memory[i], ReadByte(i)) << " (element at addr=" << i << ")";
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(
+AllMemoryCacheParametrizedTest, MemoryCacheParametrizedTest,
+::testing::Values(AddrRange(5, 6), AddrRange(5, 10), AddrRange(5, 20),
+  AddrRange(5, 30), AddrRange(5, 40), AddrRange(10, 1),
+  AddrRange(10, 21), AddrRange(19, 1), AddrRange(19, 11),
+  AddrRange(19, 12), AddrRange(20, 11), AddrRange(20, 25),
+  AddrRange(29, 2), AddrRange(29, 12), AddrRange(30, 1),
+  AddrRange(39, 1), AddrRange(39, 5), AddrRange(5, 1),
+  AddrRange(5, 5), AddrRange(9, 1), AddrRange(20, 1),
+

[Lldb-commits] [PATCH] D77765: Fix incorrect L1 inferior memory cache flushing

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

I rewrote parts of the test, hopefully making it a bit clearer. Please let me 
know if this made it better.




Comment at: lldb/source/Target/Memory.cpp:23-24
 // MemoryCache constructor
-MemoryCache::MemoryCache(Process &process)
+MemoryCache::MemoryCache(MemoryFromInferiorReader &reader,
+ uint64_t cache_line_size)
 : m_mutex(), m_L1_cache(), m_L2_cache(), m_invalid_ranges(),

clayborg wrote:
> Might be cleaner to add getting the cache line size from the 
> MemoryFromInferiorReader as a second virtual function. This would avoid 
> having to add the extra parameter here and to the clear method.
That's what I thought originally, but it does not logically belong there - it 
does not feel like inferior's memory should control cache line sizes. I also 
like that the extra parameter makes it clear when we reflect changes in the 
cache line size setting in the cache itself rather than having to guess when 
the cache might do a callback. I had trouble understanding that part before. 

Also, note that Process::GetMemoryCacheLineSize would have to be some ugly 
forwarder method because the existing GetMemoryCacheLineSize method is in 
ProcessProperties.

I am happy to put the method there if you insist, but I do not think it is a 
good idea. I would like to hear Pavel's opinion, too.



Comment at: lldb/source/Target/Memory.cpp:65-66
+  // intersecting.
+  BlockMap::iterator previous = pos;
+  previous--;
+  AddrRange previous_range(previous->first,

clayborg wrote:
> This can just be:
> 
> ```
> BlockMap::iterator previous = pos - 1;
> ```
It cannot because `map::iterator` does not have `operator-`.



Comment at: lldb/source/Target/Process.cpp:488
   m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
-  m_memory_cache(*this), m_allocated_memory_cache(*this),
-  m_should_detach(false), m_next_event_action_up(), m_public_run_lock(),
-  m_private_run_lock(), m_finalizing(false), m_finalize_called(false),
+  m_memory_cache(*this, GetMemoryCacheLineSize()),
+  m_allocated_memory_cache(*this), m_should_detach(false),

clayborg wrote:
> remove GetMemoryCacheLineSize() if we add virtual method to 
> MemoryFromInferiorReader
Is not calling virtual functions in constructors dangerous? Note that this is 
going to be called during the construction of process.



Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:75-76
+
+  m_cache.Read(10, result.data(), result.size(), error);
+  ASSERT_EQ(10, result[0]);
+

clayborg wrote:
> So we are reading 10 bytes, but only verifying the first byte in the 10 bytes 
> we read? Why not verify all of them to be sure it worked with a for loop?
> 
> ```
> for (int i=0; i   ASSERT_EQ(i+10, result[i]);
> ```
> 
> or just do manual asserts:
> ```
> ASSERT_EQ(10, result[0]);
> ASSERT_EQ(11, result[1]);
> ASSERT_EQ(12, result[2]);
> ...
> ```
> 
Ouch, this test does not make sense because we do not put anything into L1. 
Fixed now.

Changed this to test just one byte.



Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:81
+  m_cache.Read(10, result.data(), result.size(), error);
+  ASSERT_EQ(m_memory[10], result[0]);
+}

clayborg wrote:
> So with "IncrementAndFlushRange(10, 1);" we are modifying one byte at index 
> 10 by incrementing it right? It isn't clear from the:
> ```
> ASSERT_EQ(m_memory[10], result[0]);
> ```
> What we are verifying here. Wouldn't it be simpler to do:
> ```
> ASSERT_EQ(11, result[0]);
> ```
> And also verify that all of the other 9 bytes we read were unchanged?
> 
> ```
> ASSERT_EQ(11, result[1]);
> ASSERT_EQ(12, result[2]);
> ...
> ```
> 
What we really care about is that the value that Read returns is the same as 
the real one in memory, no? I actually do not really care how the 
IncrementAndFlushRange method modifies the memory as long as Read returns the 
correct value.

Nevertheless, I rewrote the code to read just one byte and to assert both the 
value and the invariant.



Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:92
+  for (std::pair p : cached)
+AddRangeToL1Cache(p.first, p.second);
+

clayborg wrote:
> seems weird to use AddRangeToL1Cache instead of just reading memory from 
> these locations and it makes it less of a real world kind a test if we are 
> mucking with the L1 cache directly. Makes the test a bit harder to follow. 
> Maybe just reading from memory is easier to follow?:
> 
> ```
> // Read ranges from memory to populate the L1 cache
> std::vector result(10);
> for (std::pair p : cached)
>   ReadByBytes(p.first, p.second);
> ```
> 
I am quite confused by this comment. What the test does is precisely what we do 
in [the real 
world](https://github.com/llvm/llvm-project/blob/1d3b7370c466eba4bf22dce4a51f885f76698053/lldb/source/Plugi

[Lldb-commits] [PATCH] D77602: [lldb/Reproducers] Support inline replay

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I haven't looked at the code in details, but some of my high-level concerns are:

- the `LLDB_RECORD_***` macros are getting rather long and repetitive. We 
should try to find a way to move some of that stuff into a regular function and 
keep the macros for the stuff that cannot be done elsewhere. I'm thinking that 
instead of:

  lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,\
   stringify_args(__VA_ARGS__));
\
   if (lldb_private::repro::InstrumentationData _data = 
\
   LLDB_GET_INSTRUMENTATION_DATA()) {   
\
 if (lldb_private::repro::Serializer *_serializer = 
\
 _data.GetSerializer()) {   
\
   _recorder.Record(*_serializer, _data.GetRegistry(),  
\
&lldb_private::repro::construct::doit, 
\
__VA_ARGS__);   
\
   _recorder.RecordResult(this, false); 
\
 } else if (lldb_private::repro::Deserializer *_deserializer =  
\
_data.GetDeserializer()) {  
\
   if (_recorder.ShouldCapture()) { 
\
 lldb_private::repro::replay_construct::doit(  
\
 _recorder, *_deserializer, _data.GetRegistry(),
\
 LLVM_PRETTY_FUNCTION); 
\
   }
\
 }  
\
   }

we could have something like:

  lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,\
stringify_args(__VA_ARGS__));   
 \
  if (lldb_private::repro::InstrumentationData _data = 
LLDB_GET_INSTRUMENTATION_DATA() /* is this needed? could the Recorder object 
tell us when replay is enabled? */) {
lldb_private::repro::Record/*?*/>(_recorder, _data/*?*/, anything_else_you_need, __VA_ARGS__)
  }

and have the `Record` ( `HandleAPI` ?) function handle the details of what 
should happen in different reproducer modes. The above assumes that 
`lldb_private::repro::construct` becomes a class which has not one but two 
member functions -- basically the merge of the `construct` and 
`replay_construct` functions from the current patch, so that the `Record` 
function can select the right operation based on the current mode.

- the chopping of LLVM_PRETTY_FUNCTION in `Registry::CheckSignature` sounds 
sub-optimal. It feels like we should be able to obtain the "ids" of both 
"expected" and "actual" methods and compare those (and have the PRETTY_FUNCTION 
just be there for error reporting purposes).
- this functionality should have a unit test just like the existing 
ReproducerInstrumentation.cpp functionality. At that point it should be 
possible/reasonable to move this patch to the front of the patch set and have 
the SB wiring come after the underlying infrastructure is set up.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D77602



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

[Lldb-commits] [PATCH] D77765: Fix incorrect L1 inferior memory cache flushing

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for tackling this. I believe this should be split into two patches: an 
NFC patch which makes it possible to unit test this code, and then a follow-up 
patch to fix the actual bug. The first patch can include the general testing 
infrastructure, and a some tests for the functionality which *is* working at 
the moment.




Comment at: lldb/source/Target/Memory.cpp:31
 
-void MemoryCache::Clear(bool clear_invalid_ranges) {
+void MemoryCache::Clear(uint64_t cache_line_size, bool clear_invalid_ranges) {
   std::lock_guard guard(m_mutex);

clayborg wrote:
> remove "cache_line_size" if we add a new virtual function to 
> MemoryFromInferiorReader
I agree with @jarin that `cache_line_size` looks out of place on the 
`MemoryFromInferiorReader` inferface. Maybe this would look better if `Clear` 
is renamed to `Reset` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77765



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

[Lldb-commits] [PATCH] D77327: [nfc] [lldb] 2/2: Introduce DWARF callbacks

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I just had one more idea. The `bool` return value on all of these methods, is 
that here just to implement the `fallback` mechanism in DebugNamesDWARFIndex? 
Could it be avoided if debug_names index calls the "fallback" *after* it has 
done its own processing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77327



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

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1967019 , @fche2 wrote:

> >> So it might be good to have the SymbolVendors use one or more SymbolServer 
> >> plug-ins.
> > 
> > I don't believe we have anything that would require all modules in a given 
> > target (or whatever) to use the same symbol vendor type.  [...]
>
> Just for clarity, is someone proposing to undertake such a rework of that 
> infrastructure?  It sounds like this is becoming a prerequisite for Konrad's 
> patch, but if no one's actually doing it, that means Konrad's work is on hold 
> indefinitely.  Is that the intent?


Yes, I believe that is becoming a prerequisite. I believe Konrad is willing to 
try to implement that, but I have advised him to hold on a bit until the exact 
details are hashed out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750



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

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and ptrace support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77047#1966539 , @omjavaid wrote:

> @labath Core file FP register access is not supported on AArch64. I am 
> working on a follow up patch to fix it. This patch only has linux changes so 
> can we get this through?


This patch is a bit big and basically untested (an unconditionally skipped test 
hardly counts), so I am looking for ways to split it up and add some test 
coverage. I'm also worried about copying what appear to be chunks of linux 
header files...




Comment at: lldb/source/Plugins/Process/Linux/NativeThreadLinux.h:42-43
   NativeRegisterContextLinux &GetRegisterContext() override {
+if (m_reg_context_up && IsStopped(nullptr))
+  m_reg_context_up->ConfigureRegisterContext();
+

Why can't this work be done in the register context constructor? I believe we 
are always creating a Thread (and its reg context) while it is stopped...



Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:38-40
+  uint32_t SetRegisterInfoMode(uint32_t mode) {
+return m_register_info_interface_up->SetRegisterInfoMode(mode);
+  }

I can't find any callers of this function. Who is calling it?


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

https://reviews.llvm.org/D77047



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

[Lldb-commits] [PATCH] D77043: Fix process gdb-remote usage of value_regs/invalidate_regs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Register infos in lldb are a mess. However lldb seems to be able to communicate 
(more or less successfully) with stub which know nothing about lldb or numbers 
it assigns to various registers. So it does not seem like there needs to be (or 
even _can_ be) one universal constant for a register in all situations.

IIUC, the problem is that on the server side the "invalidate" numbers are given 
in the form of the "lldb" register numbers, but on we don't send that number 
over -- instead lldb client makes it up (from the index).

Your patch fixes that by sending that number over explicitly. The thing I don't 
like about that is that it: a) increases the chance of things going wrong with 
non-lldb stubs; b) forks the code paths depending on whether one is talking to 
the old or new stub.

It seems to me like this could be solved in another way -- changing the meaning 
of the "invalidate" numbers to mean register indexes -- basically blessing the 
thing that the client is doing now. Since for the current targets the two 
numbers are the same, that should not make any functional difference, but it 
would make things work for SVE without forking anything or introducing new 
concepts. The translation could be done at the protocol level, just before 
sending the data over.

What do you think of that. Are there any downsides there?


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

https://reviews.llvm.org/D77043



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

[Lldb-commits] [lldb] ebb0713 - [lldb/Core] Fix a race in the Communication class

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-04-09T12:06:47+02:00
New Revision: ebb071345cdae2509c55f9eec76090926fee86a2

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

LOG: [lldb/Core] Fix a race in the Communication class

Summary:
Communication::SynchronizeWithReadThread is called whenever a process
stops to ensure that we process all of its stdout before we report the
stop. If the process exits, we first call this method, and then close
the connection.

However, when the child process exits, the thread reading its stdout
will usually (but not always) read an EOF because the other end of the
pty has been closed. In response to an EOF, the Communication read
thread closes it's end of the connection too.

This can result in a race where the read thread is closing the
connection while the synchronizing thread is attempting to get its
attention via Connection::InterruptRead.

The fix is to hold the synchronization mutex while closing the
connection.

I've found this issue while tracking down a rare flake in some of the
vscode tests. I am not sure this is the cause of those failures (as I
would have expected this issue to manifest itself differently), but it
is an issue nonetheless.

The attached test demonstrates the steps needed to reproduce the race.
It will fail under tsan without this patch.

Reviewers: clayborg, JDevlieghere

Subscribers: mgorny, lldb-commits

Tags: #lldb

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

Added: 
lldb/unittests/Core/CommunicationTest.cpp

Modified: 
lldb/source/Core/Communication.cpp
lldb/unittests/Core/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index f4163847e4bb..c3e421191b01 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -315,16 +315,12 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   Status error;
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
+  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
 size_t bytes_read = comm->ReadFromConnection(
 buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-if (bytes_read > 0)
+if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
   comm->AppendBytesToCache(buf, bytes_read, true, status);
-else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
-  if (comm->GetCloseOnEOF())
-comm->Disconnect();
-  comm->AppendBytesToCache(buf, bytes_read, true, status);
-}
 
 switch (status) {
 case eConnectionStatusSuccess:
@@ -332,11 +328,12 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
 
 case eConnectionStatusEndOfFile:
   done = true;
+  disconnect = comm->GetCloseOnEOF();
   break;
 case eConnectionStatusError: // Check GetError() for details
   if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
 // EIO on a pipe is usually caused by remote shutdown
-comm->Disconnect();
+disconnect = comm->GetCloseOnEOF();
 done = true;
   }
   if (error.Fail())
@@ -365,9 +362,15 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   if (log)
 LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->m_read_thread_did_exit = true;
-  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  {
+std::lock_guard guard(comm->m_synchronize_mutex);
+comm->m_read_thread_did_exit = true;
+if (disconnect)
+  comm->Disconnect();
+  }
+
+  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }

diff  --git a/lldb/unittests/Core/CMakeLists.txt 
b/lldb/unittests/Core/CMakeLists.txt
index 688a39d9a10f..6393c6fe38c2 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_lldb_unittest(LLDBCoreTests
+  CommunicationTest.cpp
   MangledTest.cpp
   RichManglingContextTest.cpp
   StreamCallbackTest.cpp

diff  --git a/lldb/unittests/Core/CommunicationTest.cpp 
b/lldb/unittests/Core/CommunicationTest.cpp
new file mode 100644
index ..6ad0bc720d93
--- /dev/null
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -0,0 +1,35 @@
+//===-- CommunicationTest.cpp 
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "l

[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

I am afraid that the last patch in this series introduces a bunch of failures 
for me. The failures appear to be more-or-less consistent, but the impacted 
tests seem pretty random:

  lldb-api :: 
commands/expression/persistent_ptr_update/TestPersistentPtrUpdate.py
  lldb-api :: 
commands/expression/persistent_variables/TestPersistentVariables.py
  lldb-api :: commands/watchpoints/hello_watchlocation/TestWatchLocation.py
  lldb-api :: commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py
  lldb-api :: commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
  lldb-api :: 
commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py
  lldb-api :: 
commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
  lldb-api :: 
commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
  lldb-api :: 
commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py
  lldb-api :: 
functionalities/breakpoint/auto_continue/TestBreakpointAutoContinue.py
  lldb-api :: 
functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/atomic/TestLibCxxAtomic.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/function/TestLibCxxFunction.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/map/TestDataFormatterStdMap.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/tuple/TestDataFormatterStdTuple.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/vector/TestDataFormatterStdVector.py
  lldb-api :: functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
  lldb-api :: functionalities/signal/handle-abrt/TestHandleAbort.py
  lldb-api :: functionalities/type_completion/TestTypeCompletion.py
  lldb-api :: lang/c/register_variables/TestRegisterVariables.py
  lldb-api :: lang/cpp/class_types/TestClassTypes.py
  lldb-api :: python_api/sbvalue_persist/TestSBValuePersist.py
  lldb-shell :: Driver/TestSingleQuote.test
  lldb-shell :: Settings/TestFrameFormatMangling.test

Reverting the last patch makes the problem go away, but I am not sure the 
problem is really there. Given that this series exposes a bunch of previously 
completely unused code, it could be that the problem is completely elsewhere.

Anyway, this needs to be redone somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76806



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

[Lldb-commits] [PATCH] D77295: [lldb/Core] Fix a race in the Communication class

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGebb071345cda: [lldb/Core] Fix a race in the Communication 
class (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77295

Files:
  lldb/source/Core/Communication.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/CommunicationTest.cpp

Index: lldb/unittests/Core/CommunicationTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/CommunicationTest.cpp
@@ -0,0 +1,35 @@
+//===-- CommunicationTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/Communication.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
+#include "lldb/Host/Pipe.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+TEST(CommunicationTest, SynchronizeWhileClosing) {
+  // Set up a communication object reading from a pipe.
+  Pipe pipe;
+  ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
+llvm::Succeeded());
+
+  Communication comm("test");
+  comm.SetConnection(std::make_unique(
+  pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
+  comm.SetCloseOnEOF(true);
+  ASSERT_TRUE(comm.StartReadThread());
+
+  // Ensure that we can safely synchronize with the read thread while it is
+  // closing the read end (in response to us closing the write end).
+  pipe.CloseWriteFileDescriptor();
+  comm.SynchronizeWithReadThread();
+
+  ASSERT_TRUE(comm.StopReadThread());
+}
Index: lldb/unittests/Core/CMakeLists.txt
===
--- lldb/unittests/Core/CMakeLists.txt
+++ lldb/unittests/Core/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_lldb_unittest(LLDBCoreTests
+  CommunicationTest.cpp
   MangledTest.cpp
   RichManglingContextTest.cpp
   StreamCallbackTest.cpp
Index: lldb/source/Core/Communication.cpp
===
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -315,16 +315,12 @@
   Status error;
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
+  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
 size_t bytes_read = comm->ReadFromConnection(
 buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-if (bytes_read > 0)
+if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
   comm->AppendBytesToCache(buf, bytes_read, true, status);
-else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
-  if (comm->GetCloseOnEOF())
-comm->Disconnect();
-  comm->AppendBytesToCache(buf, bytes_read, true, status);
-}
 
 switch (status) {
 case eConnectionStatusSuccess:
@@ -332,11 +328,12 @@
 
 case eConnectionStatusEndOfFile:
   done = true;
+  disconnect = comm->GetCloseOnEOF();
   break;
 case eConnectionStatusError: // Check GetError() for details
   if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
 // EIO on a pipe is usually caused by remote shutdown
-comm->Disconnect();
+disconnect = comm->GetCloseOnEOF();
 done = true;
   }
   if (error.Fail())
@@ -365,9 +362,15 @@
   if (log)
 LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->m_read_thread_did_exit = true;
-  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  {
+std::lock_guard guard(comm->m_synchronize_mutex);
+comm->m_read_thread_did_exit = true;
+if (disconnect)
+  comm->Disconnect();
+  }
+
+  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 76975c7 - Revert "[lldb/Core] Fix a race in the Communication class"

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-04-09T12:49:56+02:00
New Revision: 76975c744da1e82ca6bdbe6883c799340acaf4f0

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

LOG: Revert "[lldb/Core] Fix a race in the Communication class"

This reverts commit ebb071345cdae2509c55f9eec76090926fee86a2 -- it seems
to introduce a deadlock in some circumstances.

Added: 


Modified: 
lldb/source/Core/Communication.cpp
lldb/unittests/Core/CMakeLists.txt

Removed: 
lldb/unittests/Core/CommunicationTest.cpp



diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index c3e421191b01..f4163847e4bb 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -315,12 +315,16 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   Status error;
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
-  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
 size_t bytes_read = comm->ReadFromConnection(
 buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
+if (bytes_read > 0)
   comm->AppendBytesToCache(buf, bytes_read, true, status);
+else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
+  if (comm->GetCloseOnEOF())
+comm->Disconnect();
+  comm->AppendBytesToCache(buf, bytes_read, true, status);
+}
 
 switch (status) {
 case eConnectionStatusSuccess:
@@ -328,12 +332,11 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
 
 case eConnectionStatusEndOfFile:
   done = true;
-  disconnect = comm->GetCloseOnEOF();
   break;
 case eConnectionStatusError: // Check GetError() for details
   if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
 // EIO on a pipe is usually caused by remote shutdown
-disconnect = comm->GetCloseOnEOF();
+comm->Disconnect();
 done = true;
   }
   if (error.Fail())
@@ -362,15 +365,9 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   if (log)
 LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
-  {
-std::lock_guard guard(comm->m_synchronize_mutex);
-comm->m_read_thread_did_exit = true;
-if (disconnect)
-  comm->Disconnect();
-  }
-
+  comm->m_read_thread_did_exit = true;
   // Let clients know that this thread is exiting
+  comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
   comm->BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }

diff  --git a/lldb/unittests/Core/CMakeLists.txt 
b/lldb/unittests/Core/CMakeLists.txt
index 6393c6fe38c2..688a39d9a10f 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -1,5 +1,4 @@
 add_lldb_unittest(LLDBCoreTests
-  CommunicationTest.cpp
   MangledTest.cpp
   RichManglingContextTest.cpp
   StreamCallbackTest.cpp

diff  --git a/lldb/unittests/Core/CommunicationTest.cpp 
b/lldb/unittests/Core/CommunicationTest.cpp
deleted file mode 100644
index 6ad0bc720d93..
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-//===-- CommunicationTest.cpp 
-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "lldb/Core/Communication.h"
-#include "lldb/Host/ConnectionFileDescriptor.h"
-#include "lldb/Host/Pipe.h"
-#include "llvm/Testing/Support/Error.h"
-#include "gtest/gtest.h"
-
-using namespace lldb_private;
-
-TEST(CommunicationTest, SynchronizeWhileClosing) {
-  // Set up a communication object reading from a pipe.
-  Pipe pipe;
-  ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
-llvm::Succeeded());
-
-  Communication comm("test");
-  comm.SetConnection(std::make_unique(
-  pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
-  comm.SetCloseOnEOF(true);
-  ASSERT_TRUE(comm.StartReadThread());
-
-  // Ensure that we can safely synchronize with the read thread while it is
-  // closing the read end (in response to us closing the write end).
-  pipe.CloseWriteFileDescriptor();
-  comm.SynchronizeWithReadThread();
-
-  ASSERT_TRUE(comm.StopReadThread());
-}



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

[Lldb-commits] [lldb] 769d704 - Recommit "[lldb/Core] Fix a race in the Communication class"

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-04-09T13:39:00+02:00
New Revision: 769d7041cc1457dc2c5bcd040f24d530af48b76b

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

LOG: Recommit "[lldb/Core] Fix a race in the Communication class"

The synchronization logic in the previous had a subtle bug. Moving of
the "m_read_thread_did_exit = true" into the critical section made it
possible for some threads calling SynchronizeWithReadThread call to get
stuck. This could happen if there were already past the point where they
checked this variable. In that case, they would block on waiting for the
eBroadcastBitNoMorePendingInput event, which would never come as the
read thread was blocked on getting the synchronization mutex.

The new version moves that line out of the critical section and before
the sending of the eBroadcastBitNoMorePendingInput event, and also adds
some comments to explain why the things need to be in this sequence:
- m_read_thread_did_exit = true: prevents new threads for waiting on
  events
- eBroadcastBitNoMorePendingInput: unblock any current thread waiting
  for the event
- Disconnect(): close the connection. This is the only bit that needs to
  be in the critical section, and this is to ensure that we don't close
  the connection while the synchronizing thread is mucking with it.

Original commit message follows:

Communication::SynchronizeWithReadThread is called whenever a process
stops to ensure that we process all of its stdout before we report the
stop. If the process exits, we first call this method, and then close
the connection.

However, when the child process exits, the thread reading its stdout
will usually (but not always) read an EOF because the other end of the
pty has been closed. In response to an EOF, the Communication read
thread closes it's end of the connection too.

This can result in a race where the read thread is closing the
connection while the synchronizing thread is attempting to get its
attention via Connection::InterruptRead.

The fix is to hold the synchronization mutex while closing the
connection.

I've found this issue while tracking down a rare flake in some of the
vscode tests. I am not sure this is the cause of those failures (as I
would have expected this issue to manifest itself differently), but it
is an issue nonetheless.

The attached test demonstrates the steps needed to reproduce the race.
It will fail under tsan without this patch.

Reviewers: clayborg, JDevlieghere

Subscribers: mgorny, lldb-commits

Tags: #lldb

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

Added: 
lldb/unittests/Core/CommunicationTest.cpp

Modified: 
lldb/source/Core/Communication.cpp
lldb/unittests/Core/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index f4163847e4bb..c3e421191b01 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -315,16 +315,12 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   Status error;
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
+  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
 size_t bytes_read = comm->ReadFromConnection(
 buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-if (bytes_read > 0)
+if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
   comm->AppendBytesToCache(buf, bytes_read, true, status);
-else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
-  if (comm->GetCloseOnEOF())
-comm->Disconnect();
-  comm->AppendBytesToCache(buf, bytes_read, true, status);
-}
 
 switch (status) {
 case eConnectionStatusSuccess:
@@ -332,11 +328,12 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
 
 case eConnectionStatusEndOfFile:
   done = true;
+  disconnect = comm->GetCloseOnEOF();
   break;
 case eConnectionStatusError: // Check GetError() for details
   if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
 // EIO on a pipe is usually caused by remote shutdown
-comm->Disconnect();
+disconnect = comm->GetCloseOnEOF();
 done = true;
   }
   if (error.Fail())
@@ -365,9 +362,15 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   if (log)
 LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->m_read_thread_did_exit = true;
-  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  {
+std::lock_guard guard(comm->m_synchronize_mutex);
+comm->m_read_thread_did_exit = true;
+if (disconnect)
+  comm->Disconnect();
+  

[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin created this revision.
jarin added reviewers: labath, clayborg.
jarin added a project: LLDB.
Herald added subscribers: lldb-commits, mgorny.

This patch adds a test for L1  of the inferior's 
memory cache and makes the cache testable. This is mostly in preparation for an 
L1  flushing bug fix and its regression test 
(context: https://reviews.llvm.org/D77765).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77790

Files:
  lldb/include/lldb/Target/Memory.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Memory.cpp
  lldb/source/Target/Process.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/MemoryCacheTest.cpp

Index: lldb/unittests/Target/MemoryCacheTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/MemoryCacheTest.cpp
@@ -0,0 +1,126 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Target/Memory.h"
+#include "lldb/Utility/Status.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+
+class MemoryCacheTest : public MemoryFromInferiorReader, public testing::Test {
+  static const size_t k_memory_size = 256;
+  static const uint64_t k_cache_line_size = 16;
+
+  size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
+Status &error) override {
+m_inferior_read_count++;
+
+if (vm_addr >= m_memory.size())
+  return 0;
+
+size = std::min(size, m_memory.size() - vm_addr);
+memcpy(buf, m_memory.data() + vm_addr, size);
+return size;
+  }
+
+public:
+  MemoryCacheTest()
+  : m_cache(*this, k_cache_line_size), m_inferior_read_count(0) {
+// Fill memory from [0x0 - 0x256) with byte values that match the index. We
+// will use this memory in each test and each test will start with a fresh
+// copy.
+for (size_t i = 0; i < k_memory_size; i++)
+  m_memory.push_back(static_cast(i));
+  }
+
+  void AddRangeToL1Cache(lldb::addr_t addr, size_t len) {
+m_cache.AddL1CacheData(addr, m_memory.data() + addr, len);
+  }
+
+  void IncrementAndFlushRange(lldb::addr_t addr, size_t len) {
+for (size_t i = 0; i < len; i++)
+  m_memory[addr + i]++;
+m_cache.Flush(addr, len);
+  }
+
+  uint8_t ReadByte(lldb::addr_t addr) {
+uint8_t result;
+Status error;
+m_cache.Read(addr, &result, 1, error);
+EXPECT_TRUE(error.Success());
+return result;
+  }
+
+protected:
+  MemoryCache m_cache;
+  std::vector m_memory;
+  size_t m_inferior_read_count;
+};
+
+TEST_F(MemoryCacheTest, L1FlushSimple) {
+  // Add a byte to the cache.
+  AddRangeToL1Cache(10, 1);
+
+  // Sanity check - memory should agree with what ReadByte returns.
+  EXPECT_EQ(10, m_memory[10]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+  // Check that we have hit the cache and not the inferior's memory.
+  EXPECT_EQ(0u, m_inferior_read_count);
+
+  IncrementAndFlushRange(10, 1);
+
+  // Check the memory is incremented and the real memory agrees with ReadByte's
+  // result.
+  EXPECT_EQ(11, m_memory[11]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+}
+
+// Let us do a parametrized test that caches two ranges. In the test, we then
+// try to modify (and flush) various regions of memory and check that the cache
+// still returns the correct values from reads.
+using AddrRange = Range;
+class MemoryCacheParametrizedTest
+: public MemoryCacheTest,
+  public testing::WithParamInterface {};
+
+TEST_P(MemoryCacheParametrizedTest, L1FlushWithTwoRegions) {
+  // Add two ranges to the cache.
+  std::vector cached = {{10, 10}, {30, 10}};
+  for (AddrRange cached_range : cached)
+AddRangeToL1Cache(cached_range.base, cached_range.size);
+
+  // Flush the range given by the parameter.
+  AddrRange flush_range = GetParam();
+  IncrementAndFlushRange(flush_range.base, flush_range.size);
+
+  // Check that reads from the cached ranges read the inferior's memory
+  // if and only if the flush range intersects with one of the cached ranges.
+  bool has_intersection = false;
+  for (AddrRange cached_range : cached) {
+if (flush_range.DoesIntersect(cached_range))
+  has_intersection = true;
+
+for (size_t i = 0; i < cached_range.size; i++)
+  ReadByte(cached_range.base + i);
+  }
+  EXPECT_EQ(has_intersection, m_inferior_read_count > 0);
+
+  // Sanity check: check the memory contents.
+  for (size_t i = 0; i < 50; i++) {
+EXPECT_EQ(m_memory[i], ReadByte(i)) << " (element at addr=" << i << ")";
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(
+AllMemoryCacheParametrizedTest, MemoryCacheParametrizedTest,
+::testing::Values(AddrRange(5, 6), AddrRange(5, 10), AddrRange(5, 20),
+  AddrRange(5, 30), AddrRange(5, 40), AddrRange(10, 1),
+  AddrRange(10, 21), AddrRange(19, 1), AddrRange(19, 11),
+  AddrRange(19, 12), AddrRange(30, 1), AddrRange(39, 1),
+  Ad

[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for splitting this up. This looks fine to me, modulo some nits, but lets 
wait  @clayborg has to say.




Comment at: lldb/source/Target/Memory.cpp:63
 if (pos != m_L1_cache.begin()) {
-  --pos;
+  pos--;
 }

I guess this is a leftover from splitting the patches?

Speaking of post-increment the [[ 
http://llvm.org/docs/CodingStandards.html#prefer-preincrement | llvm rule ]] is 
to use pre-increment whereever possible. I see the test uses post-increment 
exclusively for no good reason.



Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:30
+  MemoryCacheTest()
+  : m_cache(*this, k_cache_line_size), m_inferior_read_count(0) {
+// Fill memory from [0x0 - 0x256) with byte values that match the index. We

You can move setting of `m_inferior_read_count` into the initializer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77790



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

[Lldb-commits] [lldb] a9406da - [lldb] Add parts accidentally left out of 769d704: Recommit "[lldb/Core] Fix a race in the Communication class"

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-04-09T14:45:23+02:00
New Revision: a9406daaa60f9899955a59c27f39db5519a9

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

LOG: [lldb] Add parts accidentally left out of 769d704: Recommit "[lldb/Core] 
Fix a race in the Communication class"

I went to a great length to explain the reason why these changes were
needed, but I did not actually ammend the patch to include them. :(

Added: 


Modified: 
lldb/source/Core/Communication.cpp

Removed: 




diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index c3e421191b01..2990f4157584 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -362,10 +362,17 @@ lldb::thread_result_t 
Communication::ReadThread(lldb::thread_arg_t p) {
   if (log)
 LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  // Handle threads wishing to synchronize with us.
   {
-std::lock_guard guard(comm->m_synchronize_mutex);
+// Prevent new ones from showing up.
 comm->m_read_thread_did_exit = true;
+
+// Unblock any existing thread waiting for the synchronization signal.
+comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
+
+// Wait for the thread to finish...
+std::lock_guard guard(comm->m_synchronize_mutex);
+// ... and disconnect.
 if (disconnect)
   comm->Disconnect();
   }



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

[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 256257.
jarin marked 2 inline comments as done.
jarin added a comment.

Addressed reviewer comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77790

Files:
  lldb/include/lldb/Target/Memory.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Memory.cpp
  lldb/source/Target/Process.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/MemoryCacheTest.cpp

Index: lldb/unittests/Target/MemoryCacheTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/MemoryCacheTest.cpp
@@ -0,0 +1,125 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Target/Memory.h"
+#include "lldb/Utility/Status.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+
+class MemoryCacheTest : public MemoryFromInferiorReader, public testing::Test {
+  static const size_t k_memory_size = 256;
+  static const uint64_t k_cache_line_size = 16;
+
+  size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
+Status &error) override {
+++m_inferior_read_count;
+
+if (vm_addr >= m_memory.size())
+  return 0;
+
+size = std::min(size, m_memory.size() - vm_addr);
+memcpy(buf, m_memory.data() + vm_addr, size);
+return size;
+  }
+
+public:
+  MemoryCacheTest() : m_cache(*this, k_cache_line_size) {
+// Fill memory from [0x0 - 0x256) with byte values that match the index. We
+// will use this memory in each test and each test will start with a fresh
+// copy.
+for (size_t i = 0; i < k_memory_size; ++i)
+  m_memory.push_back(static_cast(i));
+  }
+
+  void AddRangeToL1Cache(lldb::addr_t addr, size_t len) {
+m_cache.AddL1CacheData(addr, m_memory.data() + addr, len);
+  }
+
+  void IncrementAndFlushRange(lldb::addr_t addr, size_t len) {
+for (size_t i = 0; i < len; ++i)
+  ++m_memory[addr + i];
+m_cache.Flush(addr, len);
+  }
+
+  uint8_t ReadByte(lldb::addr_t addr) {
+uint8_t result;
+Status error;
+m_cache.Read(addr, &result, 1, error);
+EXPECT_TRUE(error.Success());
+return result;
+  }
+
+protected:
+  MemoryCache m_cache;
+  std::vector m_memory;
+  size_t m_inferior_read_count = 0;
+};
+
+TEST_F(MemoryCacheTest, L1FlushSimple) {
+  // Add a byte to the cache.
+  AddRangeToL1Cache(10, 1);
+
+  // Sanity check - memory should agree with what ReadByte returns.
+  EXPECT_EQ(10, m_memory[10]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+  // Check that we have hit the cache and not the inferior's memory.
+  EXPECT_EQ(0u, m_inferior_read_count);
+
+  IncrementAndFlushRange(10, 1);
+
+  // Check the memory is incremented and the real memory agrees with ReadByte's
+  // result.
+  EXPECT_EQ(11, m_memory[11]);
+  EXPECT_EQ(m_memory[10], ReadByte(10));
+}
+
+// Let us do a parametrized test that caches two ranges. In the test, we then
+// try to modify (and flush) various regions of memory and check that the cache
+// still returns the correct values from reads.
+using AddrRange = Range;
+class MemoryCacheParametrizedTest
+: public MemoryCacheTest,
+  public testing::WithParamInterface {};
+
+TEST_P(MemoryCacheParametrizedTest, L1FlushWithTwoRegions) {
+  // Add two ranges to the cache.
+  std::vector cached = {{10, 10}, {30, 10}};
+  for (AddrRange cached_range : cached)
+AddRangeToL1Cache(cached_range.base, cached_range.size);
+
+  // Flush the range given by the parameter.
+  AddrRange flush_range = GetParam();
+  IncrementAndFlushRange(flush_range.base, flush_range.size);
+
+  // Check that reads from the cached ranges read the inferior's memory
+  // if and only if the flush range intersects with one of the cached ranges.
+  bool has_intersection = false;
+  for (AddrRange cached_range : cached) {
+if (flush_range.DoesIntersect(cached_range))
+  has_intersection = true;
+
+for (size_t i = 0; i < cached_range.size; ++i)
+  ReadByte(cached_range.base + i);
+  }
+  EXPECT_EQ(has_intersection, m_inferior_read_count > 0);
+
+  // Sanity check: check the memory contents.
+  for (size_t i = 0; i < 50; ++i) {
+EXPECT_EQ(m_memory[i], ReadByte(i)) << " (element at addr=" << i << ")";
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(
+AllMemoryCacheParametrizedTest, MemoryCacheParametrizedTest,
+::testing::Values(AddrRange(5, 6), AddrRange(5, 10), AddrRange(5, 20),
+  AddrRange(5, 30), AddrRange(5, 40), AddrRange(10, 1),
+  AddrRange(10, 21), AddrRange(19, 1), AddrRange(19, 11),
+  AddrRange(19, 12), AddrRange(30, 1), AddrRange(39, 1),
+  AddrRange(39, 5), AddrRange(5, 1), AddrRange(5, 5),
+  AddrRange(9, 1), AddrRange(20, 1), AddrRange(20, 10),
+  AddrRange(29, 1), AddrRange(40, 1), AddrRange(40, 10)));
+
+} // namespace
Index: lldb/unittests/Target

[Lldb-commits] [PATCH] D77793: Fix LLDB elf core dump register access for ARM/AArch64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid created this revision.
omjavaid added a reviewer: labath.
Herald added subscribers: danielkiss, kristof.beyls.

This patch adds support to access AArch64 FP SIMD core dump registers and adds 
a test case to verify registers.

This patches fixes a bug where doing "register read --all" causes lldb to crash.


https://reviews.llvm.org/D77793

Files:
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64.out

Index: lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
@@ -0,0 +1,57 @@
+// This is test program for generating core dump test file for aarch64-linux.
+// Use following steps to generate a core dump that populates various registers:
+
+// Prerequisite (Linux): Make sure you can generate coredumps on your system.
+// Step 0.1 - set ulimit) ulimit -c unlimited
+// Step 0.2 - Set coredump target dir) sudo sysctl -w kernel.core_pattern=core
+// Step 1 - Build exe) clang -O3 aarch64-neon.c -o linux-aarch64-neon.out
+// Step 2 - Run exec and force a coredump) exec ./linux-aarch64-neon.out
+// mv ./core linux-aarch64-neon.core
+
+#include 
+#include 
+
+#define MATRIX_ROWS 8
+#define MATRIX_COLS 8
+
+void matrix_multiply(float *matrix_a, float *matrix_b, float *matrix_r,
+ unsigned int rows, unsigned int cols) {
+  if (rows != cols)
+return;
+
+  for (int i = 0; i < rows; i++) {
+for (int j = 0; j < cols; j++) {
+  matrix_r[cols * i + j] = 0.0;
+  for (int k = 0; k < cols; k++) {
+matrix_r[cols * i + j] +=
+matrix_a[cols * i + k] * matrix_b[cols * k + j];
+  }
+}
+  }
+}
+
+void matrix_print(float *matrix, unsigned int cols, unsigned int rows) {
+  for (int i = 0; i < rows; i++) {
+for (int j = 0; j < cols; j++) {
+  printf("%f ", matrix[j * rows + i]);
+}
+printf("\n");
+  }
+  printf("\n");
+}
+
+int main() {
+  float matrix_a[MATRIX_ROWS * MATRIX_COLS];
+  float matrix_b[MATRIX_ROWS * MATRIX_COLS];
+  float matrix_r[MATRIX_ROWS * MATRIX_COLS];
+
+  for (int i = 0; i < (MATRIX_ROWS * MATRIX_COLS); i++) {
+matrix_a[i] = (float)rand() / (float)(RAND_MAX);
+matrix_b[i] = (float)rand() / (float)(RAND_MAX);
+matrix_r[i] = 0.0;
+  }
+
+  matrix_multiply(matrix_a, matrix_b, matrix_r, MATRIX_ROWS, MATRIX_COLS);
+  matrix_print(matrix_r, MATRIX_COLS, MATRIX_ROWS);
+  asm volatile("brk  #0\n\t");
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -19,6 +19,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
+_aarch64_pid = 37688
 _i386_pid = 32306
 _x86_64_pid = 32259
 _s390x_pid = 1045
@@ -27,12 +28,20 @@
 _mips_o32_pid = 3532
 _ppc64le_pid = 28147
 
+_aarch64_regions = 4
 _i386_regions = 4
 _x86_64_regions = 5
 _s390x_regions = 2
 _mips_regions = 5
 _ppc64le_regions = 2
 
+
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64(self):
+"""Test that lldb can read the process information from an aarch64 linux core file."""
+self.do_test("linux-aarch64", self._aarch64_pid, self._aarch64_regions, "a.out")
+
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("X86")
 def test_i386(self):
@@ -209,6 +218,182 @@
 
 self.dbg.DeleteTarget(target)
 
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-neon.core")
+
+values = {}
+values["x0"] = "0x000a"
+values["x1"] = "0x"
+values["x2"] = "0x0001"
+values["x3"] = "0x"
+values["x4"] = "0x000a"
+values["x5"] = "0x41154400"
+values["x6"] = "0x00413057"
+values["x7"] = "0x7f7f7f7f7f7f7f7f"
+values["x8"] = "0x0040"
+values["x9"] = "0x0001"
+values["x10"] = "0x000a"
+values["x11"] = "0x0001"
+   

[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added inline comments.



Comment at: lldb/source/Target/Memory.cpp:63
 if (pos != m_L1_cache.begin()) {
-  --pos;
+  pos--;
 }

labath wrote:
> I guess this is a leftover from splitting the patches?
> 
> Speaking of post-increment the [[ 
> http://llvm.org/docs/CodingStandards.html#prefer-preincrement | llvm rule ]] 
> is to use pre-increment whereever possible. I see the test uses 
> post-increment exclusively for no good reason.
But, but,... I only increment/decrement in statement position!

Just kidding, I am sorry, fixed now...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77790



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

[Lldb-commits] [PATCH] D77327: [nfc] [lldb] 2/2: Introduce DWARF callbacks

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 256259.
jankratochvil marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77327

Files:
  lldb/include/lldb/Core/UniqueCStringMap.h
  lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
@@ -31,8 +31,8 @@
 
   DWARFCompileUnit *GetDWOCompileUnitForHash(uint64_t hash);
 
-  size_t GetObjCMethodDIEOffsets(lldb_private::ConstString class_name,
- DIEArray &method_die_offsets) override;
+  void GetObjCMethods(lldb_private::ConstString class_name,
+  llvm::function_ref callback) override;
 
   llvm::Expected
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -95,10 +95,10 @@
   return GetBaseSymbolFile().GetForwardDeclClangTypeToDie();
 }
 
-size_t SymbolFileDWARFDwo::GetObjCMethodDIEOffsets(
-lldb_private::ConstString class_name, DIEArray &method_die_offsets) {
-  return GetBaseSymbolFile().GetObjCMethodDIEOffsets(class_name,
- method_die_offsets);
+void SymbolFileDWARFDwo::GetObjCMethods(
+lldb_private::ConstString class_name,
+llvm::function_ref callback) {
+  GetBaseSymbolFile().GetObjCMethods(class_name, callback);
 }
 
 UniqueDWARFASTTypeMap &SymbolFileDWARFDwo::GetUniqueDWARFASTTypeMap() {
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -237,8 +237,8 @@
   lldb_private::CompileUnit *
   GetCompUnitForDWARFCompUnit(DWARFCompileUnit &dwarf_cu);
 
-  virtual size_t GetObjCMethodDIEOffsets(lldb_private::ConstString class_name,
- DIEArray &method_die_offsets);
+  virtual void GetObjCMethods(lldb_private::ConstString class_name,
+  llvm::function_ref callback);
 
   bool Supports_DW_AT_APPLE_objc_complete_type(DWARFUnit *cu);
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1463,11 +1463,9 @@
   return static_cast(non_dwo_cu->GetUserData());
 }
 
-size_t SymbolFileDWARF::GetObjCMethodDIEOffsets(ConstString class_name,
-DIEArray &method_die_offsets) {
-  method_die_offsets.clear();
-  m_index->GetObjCMethods(class_name, method_die_offsets);
-  return method_die_offsets.size();
+void SymbolFileDWARF::GetObjCMethods(
+ConstString class_name, llvm::function_ref callback) {
+  m_index->GetObjCMethods(class_name, callback);
 }
 
 bool SymbolFileDWARF::GetFunction(const DWARFDIE &die, SymbolContext &sc) {
@@ -2048,32 +2046,28 @@
   context, basename))
 basename = name.GetStringRef();
 
-  DIEArray die_offsets;
-  m_index->GetGlobalVariables(ConstString(basename), die_offsets);
-  const size_t num_die_matches = die_offsets.size();
-
-  SymbolContext sc;
-  sc.module_sp = m_objfile_sp->GetModule();
-  assert(sc.module_sp);
-
   // Loop invariant: Variables up to this index have been checked for context
   // matches.
   uint32_t pruned_idx = original_size;
 
-  for (size_t i = 0; i < num_die_matches; ++i) {
-const DIERef &die

[Lldb-commits] [PATCH] D77327: [nfc] [lldb] 2/2: Introduce DWARF callbacks

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D77327#1971435 , @labath wrote:

> The `bool` return value on all of these methods, is that here just to 
> implement the `fallback` mechanism in DebugNamesDWARFIndex?


Most of them yes. There are still some functions needing to return `bool` to 
benefit from the early returns from callbacks. I hope I found the minimal set 
of such `bool` functions (none of those are in `DWARFIndex`).

> Could it be avoided if debug_names index calls the "fallback" *after* it has 
> done its own processing?

Yes. Done so now. I still do not understand what is `DebugNamesDWARFIndex` good 
for when it always calls `ManualDWARFIndex` anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77327



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

[Lldb-commits] [lldb] 9aa5fbb - [lldb] Disable the new Communication test on windows

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-04-09T15:19:24+02:00
New Revision: 9aa5fbb3afed00deb2faad4ac7963c5d3247815d

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

LOG: [lldb] Disable the new Communication test on windows

The ConnectionFileDescriptor class on windows does not support
interruption (see the BytesAvailable method). Therefore this test makes
no sense there.

Added: 


Modified: 
lldb/unittests/Core/CommunicationTest.cpp

Removed: 




diff  --git a/lldb/unittests/Core/CommunicationTest.cpp 
b/lldb/unittests/Core/CommunicationTest.cpp
index 6ad0bc720d93..3ddc78d4a5af 100644
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -14,6 +14,7 @@
 
 using namespace lldb_private;
 
+#ifndef _WIN32
 TEST(CommunicationTest, SynchronizeWhileClosing) {
   // Set up a communication object reading from a pipe.
   Pipe pipe;
@@ -33,3 +34,4 @@ TEST(CommunicationTest, SynchronizeWhileClosing) {
 
   ASSERT_TRUE(comm.StopReadThread());
 }
+#endif



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

[Lldb-commits] [PATCH] D77765: Fix incorrect L1 inferior memory cache flushing

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

As labath@ suggested, I have teased apart the test and the testability 
refactoring into a separate patch. The patch lives at 
https://reviews.llvm.org/D77790, could you please take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77765



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

[Lldb-commits] [PATCH] D77587: [SVE] Add new VectorType subclasses

[Christopher Tetreault via Phabricator via lldb-commits]

ctetreau updated this revision to Diff 256129.
ctetreau added a comment.

Move getMinNumElements to ScalableVectorType. There's no reason for it to be in 
base VectorType


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77587

Files:
  lldb/source/Expression/IRInterpreter.cpp
  llvm/include/llvm-c/Core.h
  llvm/include/llvm/IR/DataLayout.h
  llvm/include/llvm/IR/DerivedTypes.h
  llvm/include/llvm/IR/Type.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/ValueTypes.cpp
  llvm/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
  llvm/lib/IR/AsmWriter.cpp
  llvm/lib/IR/Constants.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/DataLayout.cpp
  llvm/lib/IR/Type.cpp
  llvm/lib/Linker/IRMover.cpp
  llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
  llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
  llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
  llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
  llvm/lib/Transforms/IPO/GlobalOpt.cpp
  llvm/lib/Transforms/Utils/FunctionComparator.cpp
  mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

Index: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
===
--- mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
+++ mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
@@ -168,12 +168,12 @@
   return nullptr;
 return LLVMType::getArrayTy(elementType, type->getArrayNumElements());
   }
-  case llvm::Type::VectorTyID: {
-auto *typeVTy = llvm::cast(type);
-if (typeVTy->isScalable()) {
-  emitError(unknownLoc) << "scalable vector types not supported";
-  return nullptr;
-}
+  case llvm::Type::ScalableVectorTyID: {
+emitError(unknownLoc) << "scalable vector types not supported";
+return nullptr;
+  }
+  case llvm::Type::FixedVectorTyID: {
+auto *typeVTy = llvm::cast(type);
 LLVMType elementType = processType(typeVTy->getElementType());
 if (!elementType)
   return nullptr;
Index: llvm/lib/Transforms/Utils/FunctionComparator.cpp
===
--- llvm/lib/Transforms/Utils/FunctionComparator.cpp
+++ llvm/lib/Transforms/Utils/FunctionComparator.cpp
@@ -483,7 +483,8 @@
   return cmpNumbers(STyL->getNumElements(), STyR->getNumElements());
 return cmpTypes(STyL->getElementType(), STyR->getElementType());
   }
-  case Type::VectorTyID: {
+  case Type::FixedVectorTyID:
+  case Type::ScalableVectorTyID: {
 auto *STyL = cast(TyL);
 auto *STyR = cast(TyR);
 if (STyL->getElementCount().Scalable != STyR->getElementCount().Scalable)
Index: llvm/lib/Transforms/IPO/GlobalOpt.cpp
===
--- llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -130,7 +130,8 @@
   default: break;
   case Type::PointerTyID:
 return true;
-  case Type::VectorTyID:
+  case Type::FixedVectorTyID:
+  case Type::ScalableVectorTyID:
 if (cast(Ty)->getElementType()->isPointerTy())
   return true;
 break;
Index: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
===
--- llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
+++ llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
@@ -1184,7 +1184,7 @@
 case Type::IntegerTyID: // Integers larger than 64 bits
 case Type::StructTyID:
 case Type::ArrayTyID:
-case Type::VectorTyID:
+case Type::FixedVectorTyID:
   ElementSize = DL.getTypeStoreSize(ETy);
   // Ptx allows variable initilization only for constant and
   // global state spaces.
@@ -1358,7 +1358,7 @@
   switch (ETy->getTypeID()) {
   case Type::StructTyID:
   case Type::ArrayTyID:
-  case Type::VectorTyID:
+  case Type::FixedVectorTyID:
 ElementSize = DL.getTypeStoreSize(ETy);
 O << " .b8 ";
 getSymbol(GVar)->print(O, MAI);
@@ -1892,7 +1892,7 @@
   }
 
   case Type::ArrayTyID:
-  case Type::VectorTyID:
+  case Type::FixedVectorTyID:
   case Type::StructTyID: {
 if (isa(CPV) || isa(CPV)) {
   int ElementSize = DL.getTypeAllocSize(CPV->getType());
Index: llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
===
--- llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
+++ llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
@@ -307,7 +307,7 @@
 const ArrayType *ATy = cast(Ty);
 return getSmallestAddressableSize(ATy->getElementType(), GV, TM);
   }
-  case Type::VectorTyID: {
+  case Type::FixedVectorTyID: {
 const VectorType *PTy = cast(Ty);
 return getSmallestAddressableSize(PTy->getElementType(), GV, TM);
   }
Index: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
===
--- llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
+++ llvm/lib/Target/AMDGPU/AMD

[Lldb-commits] [PATCH] D77587: [SVE] Add new VectorType subclasses

[Eli Friedman via Phabricator via lldb-commits]

efriedma added a comment.

Looks right, generally.




Comment at: llvm/include/llvm-c/Core.h:163
+   * value of enum variants after the removal of LLVMVectorTypeKind
+   */
+  LLVMMetadataTypeKind = LLVMPointerTypeKind + 2, /**< Metadata */

The C API is officially not ABI-stable anymore across versions, No reason to 
have a gap like this.



Comment at: llvm/lib/ExecutionEngine/ExecutionEngine.cpp:1011
 llvm_unreachable("Unknown constant pointer type!");
-  }
-  break;
+  } break;
 

?



Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:481
   }
-} else if (ArgType->getTypeID() == Type::VectorTyID) {
+} else if (ArgType->getTypeID() == Type::FixedVectorTyID) {
   Type *IType = NULL;

Please fix this to use isa<> while you're here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77587



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

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

[Adrian McCarthy via Phabricator via lldb-commits]

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

Well, it looks to go _me_.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77662



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

[Lldb-commits] [PATCH] D77771: [lldb/Docs] Document driver and inline replay.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 256330.
JDevlieghere added a comment.

Thanks for the feedback!


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

https://reviews.llvm.org/D1

Files:
  lldb/docs/resources/reproducers.rst


Index: lldb/docs/resources/reproducers.rst
===
--- lldb/docs/resources/reproducers.rst
+++ lldb/docs/resources/reproducers.rst
@@ -91,7 +91,72 @@
 Design
 --
 
-Coming soon.
+
+Replay
+``
+
+Reproducers support two replay modes. The main and most common mode is active
+replay. It's called active, because it's LLDB that is driving replay by calling
+the captured SB API functions one after each other. The second mode is passive
+replay. In this mode, LLDB sits idle until an SB API function is called, for
+example from Python, and then replays just this individual call.
+
+Active Replay
+^
+
+No matter how a reproducer was captured, they can always be replayed with the
+command line driver. When a reproducer is passed with the `--replay` flag, the
+driver short-circuits and passes off control to the reproducer infrastructure,
+effectively bypassing its normal operation. This works because the driver is
+implemented using the SB API and is therefore nothing more than a sequence of
+SB API calls.
+
+Replay is driven by the ``Registry::Replay``. As long as there's data in the
+buffer holding the API data, the next SB API function call is deserialized.
+Once the function is known, the registry can retrieve its signature, and use
+that to deserialize its arguments. The function can then be invoked, most
+commonly through the synthesized default replayer, or potentially using a
+custom defined replay function. This process continues, until more data is
+available or a replay error is encountered.
+
+During replay only a function's side effects matter. The result returned by the
+replayed function is ignored because it cannot be observed beyond the driver.
+This is sound, because anything that is passed into a subsequent API call will
+have been serialized as an input argument. This also works for SB API objects
+because the reproducers know about every object that has crossed the API
+boundary, which is true by definition for object return values.
+
+
+Passive Replay
+^^
+
+Passive replay exists to support running the API test suite against a
+reproducer. The API test suite is written in Python and tests the debugger by
+calling into its API from Python. To make this work, the API must transparently
+replay itself when called. This is what makes passive replay different from
+driver replay, where it is lldb itself that's driving replay. For passive
+replay, the driving factor is external.
+
+In order to replay API calls, the reproducers need a way to intercept them.
+Every API call is already instrumented with an ``LLDB_RECORD_*`` macro that
+captures its input arguments. Furthermore, it also contains the necessary logic
+to detect which calls cross the API boundary and should be intercepted. We were
+able to reuse all of this to implement passive replay.
+
+During passive replay is enabled, nothing happens until an SB API is called.
+Inside that API function, the macro detects whether this call should be
+replayed (i.e. crossed the API boundary). If the answer is yes, the next
+function is deserialized from the SB API data and compared to the current
+function. If the signature matches, we deserialize its input arguments and
+reinvoke the current function with the deserialized arguments. We don't need to
+do anything special to prevent us from recursively calling the replayed version
+again, as the API boundary crossing logic knows that we're still behind the API
+boundary when we re-invoked the current function.
+
+Another big difference with driver replay is the return value. While this
+didn't matter for driver replay, it's key for passive replay, because that's
+what gets checked by the test suite. Luckily, the ``LLDB_RECORD_*`` macros
+contained sufficient type information to derive the result type.
 
 Testing
 ---


Index: lldb/docs/resources/reproducers.rst
===
--- lldb/docs/resources/reproducers.rst
+++ lldb/docs/resources/reproducers.rst
@@ -91,7 +91,72 @@
 Design
 --
 
-Coming soon.
+
+Replay
+``
+
+Reproducers support two replay modes. The main and most common mode is active
+replay. It's called active, because it's LLDB that is driving replay by calling
+the captured SB API functions one after each other. The second mode is passive
+replay. In this mode, LLDB sits idle until an SB API function is called, for
+example from Python, and then replays just this individual call.
+
+Active Replay
+^
+
+No matter how a reproducer was captured, they can always be replayed with the
+command line driver. When a reproducer is passed with the `--replay` flag, the
+driver short-circuit

[Lldb-commits] [lldb] 680082a - [lldb/Reproducers] Add a small artificial delay before exiting

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-04-09T11:03:24-07:00
New Revision: 680082a408dd2df7410d77696100eac8ce9d5530

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

LOG: [lldb/Reproducers] Add a small artificial delay before exiting

Add a small artificial delay in replay mode before exiting to ensure
that all asynchronous events have completed. This should reduce the
level of replay flakiness on some of the slower bots.

Added: 


Modified: 
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index 4c32d9407830..3bf81286bbe9 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Utility/ReproducerInstrumentation.h"
 #include "lldb/Utility/Reproducer.h"
+#include 
 #include 
 #include 
 
@@ -94,6 +95,10 @@ bool Registry::Replay(llvm::StringRef buffer) {
 GetReplayer(id)->operator()(deserializer);
   }
 
+  // Add a small artificial delay to ensure that all asynchronous events have
+  // completed before we exit.
+  std::this_thread::sleep_for (std::chrono::milliseconds(100));
+
   return true;
 }
 



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

[Lldb-commits] [PATCH] D76906: [lldb] Fixing the bug that the "log timer" has no tab completion

[Shu Anzai via Phabricator via lldb-commits]

gedatsu217 added a comment.

  [39/575] Linking CXX shared library lib/libc++abi.1.0.dylib
  FAILED: lib/libc++abi.1.0.dylib 
  : && /Library/Developer/CommandLineTools/usr/bin/c++ -fPIC 
-fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color  -g  -dynamiclib 
-Wl,-headerpad_max_install_names -nodefaultlibs  -compatibility_version 1.0.0 
-current_version 1.0.0 -o lib/libc++abi.1.0.dylib -install_name 
@rpath/libc++abi.1.dylib 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_aux_runtime.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_default_handlers.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_demangle.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_guard.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_handlers.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_unexpected.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_vector.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_virtual.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_exception.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_stdexcept.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_typeinfo.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/abort_message.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/fallback_malloc.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/private_typeinfo.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_new_delete.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception.cpp.o 
projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_personality.cpp.o  
-Wl,-rpath,@loader_path/../lib  -lSystem  
-Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/itanium-base.exp
  
-Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/new-delete.exp
  
-Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/personality-v0.exp
 && :
  Undefined symbols for architecture x86_64:
"__ZTIDu", referenced from:
   -exported_symbol[s_list] command line option
"__ZTIPDu", referenced from:
   -exported_symbol[s_list] command line option
"__ZTIPKDu", referenced from:
   -exported_symbol[s_list] command line option
"__ZTSDu", referenced from:
   -exported_symbol[s_list] command line option
"__ZTSPDu", referenced from:
   -exported_symbol[s_list] command line option
"__ZTSPKDu", referenced from:
   -exported_symbol[s_list] command line option
  ld: symbol(s) not found for architecture x86_64
  clang: error: linker command failed with exit code 1 (use -v to see 
invocation)

This is an error message. I know it is caused around libc++, but I don't know 
where parts should I fix. 
I did below to solve it.

- delete build directory and make build directory again (cmake -G Ninja 
-DLLVM_ENABLE_PROJECTS="clang;lldb;libcxx;libcxxabi" ../llvm)
- build each project(ninja lldb, ninja clang, ninja cxx, ninja cxxabi)
  - but when I executed "ninja cxx" and "ninja cxxabi", the same error occured.

I sometimes tried to solve this problem this week, but I could not solve it.
(I could not go to my laboratory because of coronavirus, and I was busy dealing 
with that, so I could not take much time.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76906



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

[Lldb-commits] [lldb] 143d507 - Preserve the owning module information from DWARF in the synthesized AST

[Adrian Prantl via lldb-commits]

Author: Adrian Prantl
Date: 2020-04-09T11:09:44-07:00
New Revision: 143d507c9ff90db93e547f0e1131e92db06e2675

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

LOG: Preserve the owning module information from DWARF in the synthesized AST

Types that came from a Clang module are nested in DW_TAG_module tags
in DWARF. This patch recreates the Clang module hierarchy in LLDB and
1;95;0csets the owning module information accordingly. My primary motivation
is to facilitate looking up per-module APINotes for individual
declarations, but this likely also has other applications.

This reapplies the previously reverted commit, but without support for
ClassTemplateSpecializations, which I'm going to look into separately.

rdar://problem/59634380

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

Added: 
lldb/test/Shell/SymbolFile/DWARF/Inputs/ModuleOwnership/A.h
lldb/test/Shell/SymbolFile/DWARF/Inputs/ModuleOwnership/B.h
lldb/test/Shell/SymbolFile/DWARF/Inputs/ModuleOwnership/module.modulemap
lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm

Modified: 
lldb/include/lldb/Symbol/CompilerType.h
lldb/include/lldb/Symbol/TypeSystem.h
lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.h
lldb/source/Plugins/Language/ObjC/NSDictionary.cpp

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
lldb/source/Symbol/CompilerType.cpp
lldb/source/Symbol/Type.cpp
lldb/source/Symbol/TypeSystem.cpp
lldb/test/Shell/SymbolFile/DWARF/lit.local.cfg
lldb/unittests/Symbol/TestTypeSystemClang.cpp
lldb/unittests/TestingSupport/Symbol/ClangTestUtils.h

Removed: 




diff  --git a/lldb/include/lldb/Symbol/CompilerType.h 
b/lldb/include/lldb/Symbol/CompilerType.h
index 0d1b00a1b26c..b0a7953190f8 100644
--- a/lldb/include/lldb/Symbol/CompilerType.h
+++ b/lldb/include/lldb/Symbol/CompilerType.h
@@ -242,8 +242,10 @@ class CompilerType {
   /// Create a typedef to this type using "name" as the name of the typedef 
this
   /// type is valid and the type system supports typedefs, else return an
   /// invalid type.
+  /// \param payload   The typesystem-specific \p lldb::Type payload.
   CompilerType CreateTypedef(const char *name,
- const CompilerDeclContext &decl_ctx) const;
+ const CompilerDeclContext &decl_ctx,
+ uint32_t payload) const;
 
   /// If the current object represents a typedef type, get the underlying type
   CompilerType GetTypedefedType() const;

diff  --git a/lldb/include/lldb/Symbol/TypeSystem.h 
b/lldb/include/lldb/Symbol/TypeSystem.h
index a84b9a1c441c..ba2bbfaf4650 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -259,9 +259,12 @@ class TypeSystem : public PluginInterface {
 
   virtual CompilerType AddRestrictModifier(lldb::opaque_compiler_type_t type);
 
+  /// \param opaque_payload  The m_payload field of Type, which may
+  /// carry TypeSystem-specific extra information.
   virtual CompilerType CreateTypedef(lldb::opaque_compiler_type_t type,
  const char *name,
- const CompilerDeclContext &decl_ctx);
+ const CompilerDeclContext &decl_ctx,
+ uint32_t opaque_payload);
 
   // Exploring the type
 

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 4b3e237dc62c..2a21e5e0d7b9 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -18,6 +18,7 @@
 
 #include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/ExpressionParser/Clang/ClangASTMetadata.h"
+#include "Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.h"
 #include "Plugins/ExpressionParser/Clang/ClangUtil.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 
@@ -1003,6 

[Lldb-commits] [PATCH] D77588: [lldb/Reproducers] Make it possible to capture reproducers for the Python test suite.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D77588#1971345 , @labath wrote:

> Ok, I'm starting to understand what's going on here. This patch looks mostly 
> fine, but IIUC, it won't really do anything until the subsequent patch lands 
> as that is what implements the inline/api/passive replay. I'm going to start 
> looking at that other patch now.


It's sufficient for stage one, i.e. capture the test suite and use the driver 
(active replay), but it also contains the necessary changes for stage 2 
(passive replay). I could've split it up in two patches but given that the 
changes are so similar that didn't seem worth it.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D77588



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

[Lldb-commits] [PATCH] D77588: [lldb/Reproducers] Make it possible to capture reproducers for the Python test suite.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 256341.
JDevlieghere added a comment.

Update naming to passive replay.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D77588

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBReproducer.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/SBReproducer.h
  lldb/packages/Python/lldbsuite/test/configuration.py
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/dotest_args.py
  lldb/source/API/SBReproducer.cpp
  lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
  lldb/test/API/lit.cfg.py
  lldb/test/API/lldbtest.py

Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -1,6 +1,6 @@
 from __future__ import absolute_import
 import os
-
+import tempfile
 import subprocess
 import sys
 
@@ -67,14 +67,15 @@
 # python exe as the first parameter of the command.
 cmd = [sys.executable] + self.dotest_cmd + [testPath, '-p', testFile]
 
+builddir = getBuildDir(cmd)
+mkdir_p(builddir)
+
 # The macOS system integrity protection (SIP) doesn't allow injecting
 # libraries into system binaries, but this can be worked around by
 # copying the binary into a different location.
 if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
 (sys.executable.startswith('/System/') or \
 sys.executable.startswith('/usr/bin/')):
-builddir = getBuildDir(cmd)
-mkdir_p(builddir)
 copied_python = os.path.join(builddir, 'copied-system-python')
 if not os.path.isfile(copied_python):
 import shutil, subprocess
@@ -86,6 +87,16 @@
 shutil.copy(python, copied_python)
 cmd[0] = copied_python
 
+if 'lldb-repro-capture' in test.config.available_features or \
+   'lldb-repro-replay' in test.config.available_features:
+reproducer_root = os.path.join(builddir, 'reproducers')
+mkdir_p(reproducer_root)
+reproducer_path = os.path.join(reproducer_root, testFile)
+if 'lldb-repro-capture' in test.config.available_features:
+cmd.extend(['--capture-path', reproducer_path])
+else:
+cmd.extend(['--replay-path', reproducer_path])
+
 timeoutInfo = None
 try:
 out, err, exitCode = lit.util.executeCommand(
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -60,6 +60,17 @@
   config.environment['LLDB_CAPTURE_REPRODUCER'] = os.environ[
   'LLDB_CAPTURE_REPRODUCER']
 
+# Support running the test suite under the lldb-repro wrapper. This makes it
+# possible to capture a test suite run and then rerun all the test from the
+# just captured reproducer.
+lldb_repro_mode = lit_config.params.get('lldb-run-with-repro', None)
+if lldb_repro_mode:
+  lit_config.note("Running API tests in {} mode.".format(lldb_repro_mode))
+  if lldb_repro_mode == 'capture':
+config.available_features.add('lldb-repro-capture')
+  elif lldb_repro_mode == 'replay':
+config.available_features.add('lldb-repro-replay')
+
 # Clean the module caches in the test build directory. This is necessary in an
 # incremental build whenever clang changes underneath, so doing it once per
 # lit.py invocation is close enough.
Index: lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
===
--- lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
+++ lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
@@ -20,6 +20,7 @@
 @skipIfWindows
 @skipIfRemote
 @skipIfiOSSimulator
+@skipIfReproducer
 def test_reproducer_attach(self):
 """Test thread creation after process attach."""
 exe = '%s_%d' % (self.testMethodName, os.getpid())
Index: lldb/source/API/SBReproducer.cpp
===
--- lldb/source/API/SBReproducer.cpp
+++ lldb/source/API/SBReproducer.cpp
@@ -124,6 +124,15 @@
   return nullptr;
 }
 
+const char *SBReproducer::PassiveReplay(const char *path) {
+  static std::string error;
+  if (auto e = Reproducer::Initialize(ReproducerMode::Replay, FileSpec(path))) {
+error = llvm::toString(std::move(e));
+return error.c_str();
+  }
+  return nullptr;
+}
+
 const char *SBReproducer::Replay(const char *path) {
   return SBReproducer::Replay(path, false);
 }
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/t

Re: [Lldb-commits] [lldb] 680082a - [lldb/Reproducers] Add a small artificial delay before exiting

[Davide Italiano via lldb-commits]

> On Apr 9, 2020, at 11:05, Jonas Devlieghere via lldb-commits 
>  wrote:
> 
> 
> Author: Jonas Devlieghere
> Date: 2020-04-09T11:03:24-07:00
> New Revision: 680082a408dd2df7410d77696100eac8ce9d5530
> 
> URL: 
> https://github.com/llvm/llvm-project/commit/680082a408dd2df7410d77696100eac8ce9d5530
> DIFF: 
> https://github.com/llvm/llvm-project/commit/680082a408dd2df7410d77696100eac8ce9d5530.diff
> 
> LOG: [lldb/Reproducers] Add a small artificial delay before exiting
> 
> Add a small artificial delay in replay mode before exiting to ensure
> that all asynchronous events have completed. This should reduce the
> level of replay flakiness on some of the slower bots.
> 
> Added: 
> 
> 
> Modified: 
>lldb/source/Utility/ReproducerInstrumentation.cpp
> 
> Removed: 
> 
> 
> 
> 
> diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
> b/lldb/source/Utility/ReproducerInstrumentation.cpp
> index 4c32d9407830..3bf81286bbe9 100644
> --- a/lldb/source/Utility/ReproducerInstrumentation.cpp
> +++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
> @@ -8,6 +8,7 @@
> 
> #include "lldb/Utility/ReproducerInstrumentation.h"
> #include "lldb/Utility/Reproducer.h"
> +#include 
> #include 
> #include 
> 
> @@ -94,6 +95,10 @@ bool Registry::Replay(llvm::StringRef buffer) {
> GetReplayer(id)->operator()(deserializer);
>   }
> 
> +  // Add a small artificial delay to ensure that all asynchronous events have
> +  // completed before we exit.
> +  std::this_thread::sleep_for (std::chrono::milliseconds(100));
> +
>   return true;
> }
> 

I understand this is (unfortunately) sorely needed, but I’m somehow skeptical 
of random delays. 
You probably thought about this and dropped the idea on the floor — but, is 
there a way to make the sync more explicit?
If you can’t, at least you can detect not everything is in order and sleep, 
for, e.g. another 100 milliseconds in a retry-loop?

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

[Lldb-commits] [lldb] 8fbac4e - [nfc] [lldb] Unindent code

[Jan Kratochvil via lldb-commits]

Author: Jan Kratochvil
Date: 2020-04-09T20:43:00+02:00
New Revision: 8fbac4e1a2f2506671ca06226e4513a5965bdbf5

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

LOG: [nfc] [lldb] Unindent code

It removes some needless deep indentation and some redundant statements.
It prepares the code for a more clean next patch - DWARF index callbacks
D77327.

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 62aba37c6fe7..56f2f07674cc 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2016,17 +2016,14 @@ bool DWARFASTParserClang::CompleteRecordType(const 
DWARFDIE &die,
 DIEArray method_die_offsets;
 dwarf->GetObjCMethodDIEOffsets(class_name, method_die_offsets);
 
-if (!method_die_offsets.empty()) {
+const size_t num_matches = method_die_offsets.size();
+for (size_t i = 0; i < num_matches; ++i) {
+  const DIERef &die_ref = method_die_offsets[i];
   DWARFDebugInfo &debug_info = dwarf->DebugInfo();
+  DWARFDIE method_die = debug_info.GetDIE(die_ref);
 
-  const size_t num_matches = method_die_offsets.size();
-  for (size_t i = 0; i < num_matches; ++i) {
-const DIERef &die_ref = method_die_offsets[i];
-DWARFDIE method_die = debug_info.GetDIE(die_ref);
-
-if (method_die)
-  method_die.ResolveType();
-  }
+  if (method_die)
+method_die.ResolveType();
 }
 
 for (DelayedPropertyList::iterator pi = delayed_properties.begin(),

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
index aa66c920994b..9072b2dc0115 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
@@ -48,6 +48,7 @@ class DWARFDeclContext {
   }
 
   bool operator==(const DWARFDeclContext &rhs) const;
+  bool operator!=(const DWARFDeclContext &rhs) const { return !(*this == rhs); 
}
 
   uint32_t GetSize() const { return m_entries.size(); }
 

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
index 5f01b8176b98..c82a8682ca49 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
@@ -67,8 +67,8 @@ void DWARFMappedHash::ExtractClassOrStructDIEArray(
   const size_t count = die_info_array.size();
   for (size_t i = 0; i < count; ++i) {
 const dw_tag_t die_tag = die_info_array[i].tag;
-if (die_tag != 0 && die_tag != DW_TAG_class_type &&
-die_tag != DW_TAG_structure_type)
+if (!(die_tag == 0 || die_tag == DW_TAG_class_type ||
+  die_tag == DW_TAG_structure_type))
   continue;
 if (die_info_array[i].type_flags & eTypeFlagClassIsImplementation) {
   if (return_implementation_only_if_available) {

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 5c14d3b52ac5..fb449edb90ca 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2051,64 +2051,53 @@ void SymbolFileDWARF::FindGlobalVariables(
   DIEArray die_offsets;
   m_index->GetGlobalVariables(ConstString(basename), die_offsets);
   const size_t num_die_matches = die_offsets.size();
-  if (num_die_matches) {
-SymbolContext sc;
-sc.module_sp = m_objfile_sp->GetModule();
-assert(sc.module_sp);
 
-// Loop invariant: Variables up to this index have been checked for context
-// matches.
-uint32_t pruned_idx = original_size;
+  SymbolContext sc;
+  sc.module_sp = m_objfile_sp->GetModule();
+  assert(sc.module_sp);
 
-bool done = false;
-for (size_t i = 0; i < num_die_matches && !done; ++i) {
-  const DIERef &die_ref = die_offsets[i];
-  DWARFDIE die = GetDIE(die_ref);
+  // Loop invariant: Variables up to this index have been checked for context
+  // matches.
+  uint32_t pruned_idx = original_size;
 
-  if (die) {
-switch (die.Tag()) {
-default:
-case DW_TAG_subprogram:
-case DW_TAG_inlined_subroutine:
-case DW_TAG_try_

Re: [Lldb-commits] [lldb] 680082a - [lldb/Reproducers] Add a small artificial delay before exiting

[Jonas Devlieghere via lldb-commits]

On Thu, Apr 9, 2020 at 11:39 AM Davide Italiano  wrote:

>
>
> > On Apr 9, 2020, at 11:05, Jonas Devlieghere via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> >
> > Author: Jonas Devlieghere
> > Date: 2020-04-09T11:03:24-07:00
> > New Revision: 680082a408dd2df7410d77696100eac8ce9d5530
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/680082a408dd2df7410d77696100eac8ce9d5530
> > DIFF:
> https://github.com/llvm/llvm-project/commit/680082a408dd2df7410d77696100eac8ce9d5530.diff
> >
> > LOG: [lldb/Reproducers] Add a small artificial delay before exiting
> >
> > Add a small artificial delay in replay mode before exiting to ensure
> > that all asynchronous events have completed. This should reduce the
> > level of replay flakiness on some of the slower bots.
> >
> > Added:
> >
> >
> > Modified:
> >lldb/source/Utility/ReproducerInstrumentation.cpp
> >
> > Removed:
> >
> >
> >
> >
> 
> > diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp
> b/lldb/source/Utility/ReproducerInstrumentation.cpp
> > index 4c32d9407830..3bf81286bbe9 100644
> > --- a/lldb/source/Utility/ReproducerInstrumentation.cpp
> > +++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
> > @@ -8,6 +8,7 @@
> >
> > #include "lldb/Utility/ReproducerInstrumentation.h"
> > #include "lldb/Utility/Reproducer.h"
> > +#include 
> > #include 
> > #include 
> >
> > @@ -94,6 +95,10 @@ bool Registry::Replay(llvm::StringRef buffer) {
> > GetReplayer(id)->operator()(deserializer);
> >   }
> >
> > +  // Add a small artificial delay to ensure that all asynchronous
> events have
> > +  // completed before we exit.
> > +  std::this_thread::sleep_for (std::chrono::milliseconds(100));
> > +
> >   return true;
> > }
> >
>
> I understand this is (unfortunately) sorely needed, but I’m somehow
> skeptical of random delays.
> You probably thought about this and dropped the idea on the floor — but,
> is there a way to make the sync more explicit?
> If you can’t, at least you can detect not everything is in order and
> sleep, for, e.g. another 100 milliseconds in a retry-loop?
>

Yeah, it's a hack, and I'm not really happy with it. The last replayed API
call can be literaly anything, so you don't know what you might have to
wait for. I can't think of a good way to do the synchronization without
breaking layering/abstractions. The issue on GreenDragon was that we exited
before the stop event came in, and while we could probably synchronize
that, it would be even more ad-hoc than this...


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

[Lldb-commits] [lldb] 68c04a4 - [lldb] Disable failing register tests for reproducers

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-04-09T21:05:17+02:00
New Revision: 68c04a4f73ac10b0881db31c99282955765e4c8f

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

LOG: [lldb] Disable failing register tests for reproducers

Added: 


Modified: 
lldb/test/Shell/Register/x86-64-read.test
lldb/test/Shell/Register/x86-64-ymm-read.test

Removed: 




diff  --git a/lldb/test/Shell/Register/x86-64-read.test 
b/lldb/test/Shell/Register/x86-64-read.test
index fc093190c256..bd770ecc24b8 100644
--- a/lldb/test/Shell/Register/x86-64-read.test
+++ b/lldb/test/Shell/Register/x86-64-read.test
@@ -1,4 +1,4 @@
-# XFAIL: system-windows
+# XFAIL: system-window, lldb-repros
 # REQUIRES: native && target-x86_64
 # RUN: %clangxx_host %p/Inputs/x86-64-read.cpp -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s

diff  --git a/lldb/test/Shell/Register/x86-64-ymm-read.test 
b/lldb/test/Shell/Register/x86-64-ymm-read.test
index 0d01b0937f1d..155a00ee6856 100644
--- a/lldb/test/Shell/Register/x86-64-ymm-read.test
+++ b/lldb/test/Shell/Register/x86-64-ymm-read.test
@@ -1,4 +1,4 @@
-# XFAIL: system-windows
+# XFAIL: system-windows, lldb-repro
 # REQUIRES: native && target-x86_64 && native-cpu-avx
 # RUN: %clangxx_host %p/Inputs/x86-ymm-read.cpp -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s



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

[Lldb-commits] [lldb] 98b47f4 - [lldb/test] Fix typo to disable reproducer's test phase

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-04-09T21:54:45+02:00
New Revision: 98b47f447c93e531db8cc70841bad8c867d76134

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

LOG: [lldb/test] Fix typo to disable reproducer's test phase

Added: 


Modified: 
lldb/test/Shell/Register/x86-64-read.test

Removed: 




diff  --git a/lldb/test/Shell/Register/x86-64-read.test 
b/lldb/test/Shell/Register/x86-64-read.test
index bd770ecc24b8..99937071add8 100644
--- a/lldb/test/Shell/Register/x86-64-read.test
+++ b/lldb/test/Shell/Register/x86-64-read.test
@@ -1,4 +1,4 @@
-# XFAIL: system-window, lldb-repros
+# XFAIL: system-window, lldb-repro
 # REQUIRES: native && target-x86_64
 # RUN: %clangxx_host %p/Inputs/x86-64-read.cpp -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s



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

[Lldb-commits] [PATCH] D77326: 1/2: [nfc] [lldb] Unindent code

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fbac4e1a2f2: [nfc] [lldb] Unindent code (authored by 
jankratochvil).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77326

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2051,64 +2051,53 @@
   DIEArray die_offsets;
   m_index->GetGlobalVariables(ConstString(basename), die_offsets);
   const size_t num_die_matches = die_offsets.size();
-  if (num_die_matches) {
-SymbolContext sc;
-sc.module_sp = m_objfile_sp->GetModule();
-assert(sc.module_sp);
 
-// Loop invariant: Variables up to this index have been checked for context
-// matches.
-uint32_t pruned_idx = original_size;
+  SymbolContext sc;
+  sc.module_sp = m_objfile_sp->GetModule();
+  assert(sc.module_sp);
 
-bool done = false;
-for (size_t i = 0; i < num_die_matches && !done; ++i) {
-  const DIERef &die_ref = die_offsets[i];
-  DWARFDIE die = GetDIE(die_ref);
+  // Loop invariant: Variables up to this index have been checked for context
+  // matches.
+  uint32_t pruned_idx = original_size;
 
-  if (die) {
-switch (die.Tag()) {
-default:
-case DW_TAG_subprogram:
-case DW_TAG_inlined_subroutine:
-case DW_TAG_try_block:
-case DW_TAG_catch_block:
-  break;
+  for (size_t i = 0; i < num_die_matches; ++i) {
+const DIERef &die_ref = die_offsets[i];
+DWARFDIE die = GetDIE(die_ref);
+if (!die) {
+  m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
+  continue;
+}
 
-case DW_TAG_variable: {
-  auto *dwarf_cu = llvm::dyn_cast(die.GetCU());
-  if (!dwarf_cu)
-continue;
-  sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
-
-  if (parent_decl_ctx) {
-if (DWARFASTParser *dwarf_ast = GetDWARFParser(*die.GetCU())) {
-  CompilerDeclContext actual_parent_decl_ctx =
-  dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
-  if (!actual_parent_decl_ctx ||
-  actual_parent_decl_ctx != parent_decl_ctx)
-continue;
-}
-  }
+if (die.Tag() != DW_TAG_variable)
+  continue;
 
-  ParseVariables(sc, die, LLDB_INVALID_ADDRESS, false, false,
- &variables);
-  while (pruned_idx < variables.GetSize()) {
-VariableSP var_sp = variables.GetVariableAtIndex(pruned_idx);
-if (name_is_mangled ||
-var_sp->GetName().GetStringRef().contains(name.GetStringRef()))
-  ++pruned_idx;
-else
-  variables.RemoveVariableAtIndex(pruned_idx);
-  }
+auto *dwarf_cu = llvm::dyn_cast(die.GetCU());
+if (!dwarf_cu)
+  continue;
+sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
 
-  if (variables.GetSize() - original_size >= max_matches)
-done = true;
-} break;
-}
-  } else {
-m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
+if (parent_decl_ctx) {
+  if (DWARFASTParser *dwarf_ast = GetDWARFParser(*die.GetCU())) {
+CompilerDeclContext actual_parent_decl_ctx =
+dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
+if (!actual_parent_decl_ctx ||
+actual_parent_decl_ctx != parent_decl_ctx)
+  continue;
   }
 }
+
+ParseVariables(sc, die, LLDB_INVALID_ADDRESS, false, false, &variables);
+while (pruned_idx < variables.GetSize()) {
+  VariableSP var_sp = variables.GetVariableAtIndex(pruned_idx);
+  if (name_is_mangled ||
+  var_sp->GetName().GetStringRef().contains(name.GetStringRef()))
+++pruned_idx;
+  else
+variables.RemoveVariableAtIndex(pruned_idx);
+}
+
+if (variables.GetSize() - original_size >= max_matches)
+  break;
   }
 
   // Return the number of variable that were appended to the list
@@ -2693,54 +2682,53 @@
 
   const size_t num_matches = die_offsets.size();
 
-  if (num_matches) {
-for (size_t i = 0; i < num_matches; ++i) {
-  const DIERef &die_ref = die_offsets[i];
-  DWARFDIE type_die = GetDIE(die_ref);
-
-  if (type_die) {
-bool try_resolving_type = false;
+  for (size_t i = 0; i < num_matches; ++i) {
+const DIERef &die_ref = die_offsets[i];
+DWARFDIE type_die = GetDIE(die_ref);
+if (!type_die) {
+  m_index->ReportInv

[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay updated this revision to Diff 256416.
emrekultursay added a comment.

Fix broken tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76806

Files:
  lldb/include/lldb/Core/SourceManager.h
  lldb/source/Core/SourceManager.cpp
  lldb/test/API/commands/settings/use_source_cache/Makefile
  lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py
  lldb/test/API/commands/settings/use_source_cache/main.cpp

Index: lldb/test/API/commands/settings/use_source_cache/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/settings/use_source_cache/main.cpp
@@ -0,0 +1,619 @@
+// This file should be large enough that LLDB decides to load it
+// using memory mapping. See:
+//   ⁠llvm/lib/Support/MemoryBuffer.cp:shouldUseMmap()
+
+#include 
+
+int calc(int x0, int x1, int x2);
+
+int
+main(int argc, char const *argv[])
+{
+  fprintf(stderr, "Hello, world => %d\n", calc(0, 1, 2));
+  return 0;
+}
+
+
+// The name of this function is used in tests to set breakpoints by name.
+int calc(int x0, int x1, int x2) {
+  int x3 = x2 * x1 + x0;
+  int x4 = x3 * x2 + x1;
+  int x5 = x4 * x3 + x2;
+  int x6 = x5 * x4 + x3;
+  int x7 = x6 * x5 + x4;
+  int x8 = x7 * x6 + x5;
+  int x9 = x8 * x7 + x6;
+  int x10 = x9 * x8 + x7;
+  int x11 = x10 * x9 + x8;
+  int x12 = x11 * x10 + x9;
+  int x13 = x12 * x11 + x10;
+  int x14 = x13 * x12 + x11;
+  int x15 = x14 * x13 + x12;
+  int x16 = x15 * x14 + x13;
+  int x17 = x16 * x15 + x14;
+  int x18 = x17 * x16 + x15;
+  int x19 = x18 * x17 + x16;
+  int x20 = x19 * x18 + x17;  
+  int x21 = x20 * x19 + x18;
+  int x22 = x21 * x20 + x19;
+  int x23 = x22 * x21 + x20;
+  int x24 = x23 * x22 + x21;
+  int x25 = x24 * x23 + x22;
+  int x26 = x25 * x24 + x23;
+  int x27 = x26 * x25 + x24;
+  int x28 = x27 * x26 + x25;
+  int x29 = x28 * x27 + x26;
+  int x30 = x29 * x28 + x27;
+  int x31 = x30 * x29 + x28;
+  int x32 = x31 * x30 + x29;
+  int x33 = x32 * x31 + x30;
+  int x34 = x33 * x32 + x31;
+  int x35 = x34 * x33 + x32;
+  int x36 = x35 * x34 + x33;
+  int x37 = x36 * x35 + x34;
+  int x38 = x37 * x36 + x35;
+  int x39 = x38 * x37 + x36;
+  int x40 = x39 * x38 + x37;
+  int x41 = x40 * x39 + x38;
+  int x42 = x41 * x40 + x39;
+  int x43 = x42 * x41 + x40;
+  int x44 = x43 * x42 + x41;
+  int x45 = x44 * x43 + x42;
+  int x46 = x45 * x44 + x43;
+  int x47 = x46 * x45 + x44;
+  int x48 = x47 * x46 + x45;
+  int x49 = x48 * x47 + x46;
+  int x50 = x49 * x48 + x47;
+  int x51 = x50 * x49 + x48;
+  int x52 = x51 * x50 + x49;
+  int x53 = x52 * x51 + x50;
+  int x54 = x53 * x52 + x51;
+  int x55 = x54 * x53 + x52;
+  int x56 = x55 * x54 + x53;
+  int x57 = x56 * x55 + x54;
+  int x58 = x57 * x56 + x55;
+  int x59 = x58 * x57 + x56;
+  int x60 = x59 * x58 + x57;
+  int x61 = x60 * x59 + x58;
+  int x62 = x61 * x60 + x59;
+  int x63 = x62 * x61 + x60;
+  int x64 = x63 * x62 + x61;
+  int x65 = x64 * x63 + x62;
+  int x66 = x65 * x64 + x63;
+  int x67 = x66 * x65 + x64;
+  int x68 = x67 * x66 + x65;
+  int x69 = x68 * x67 + x66;
+  int x70 = x69 * x68 + x67;
+  int x71 = x70 * x69 + x68;
+  int x72 = x71 * x70 + x69;
+  int x73 = x72 * x71 + x70;
+  int x74 = x73 * x72 + x71;
+  int x75 = x74 * x73 + x72;
+  int x76 = x75 * x74 + x73;
+  int x77 = x76 * x75 + x74;
+  int x78 = x77 * x76 + x75;
+  int x79 = x78 * x77 + x76;
+  int x80 = x79 * x78 + x77;
+  int x81 = x80 * x79 + x78;
+  int x82 = x81 * x80 + x79;
+  int x83 = x82 * x81 + x80;
+  int x84 = x83 * x82 + x81;
+  int x85 = x84 * x83 + x82;
+  int x86 = x85 * x84 + x83;
+  int x87 = x86 * x85 + x84;
+  int x88 = x87 * x86 + x85;
+  int x89 = x88 * x87 + x86;
+  int x90 = x89 * x88 + x87;
+  int x91 = x90 * x89 + x88;
+  int x92 = x91 * x90 + x89;
+  int x93 = x92 * x91 + x90;
+  int x94 = x93 * x92 + x91;
+  int x95 = x94 * x93 + x92;
+  int x96 = x95 * x94 + x93;
+  int x97 = x96 * x95 + x94;
+  int x98 = x97 * x96 + x95;
+  int x99 = x98 * x97 + x96;
+  int x100 = x99 * x98 + x97;
+  int x101 = x100 * x99 + x98;
+  int x102 = x101 * x100 + x99;
+  int x103 = x102 * x101 + x100;
+  int x104 = x103 * x102 + x101;
+  int x105 = x104 * x103 + x102;
+  int x106 = x105 * x104 + x103;
+  int x107 = x106 * x105 + x104;
+  int x108 = x107 * x106 + x105;
+  int x109 = x108 * x107 + x106;
+  int x110 = x109 * x108 + x107;
+  int x111 = x110 * x109 + x108;
+  int x112 = x111 * x110 + x109;
+  int x113 = x112 * x111 + x110;
+  int x114 = x113 * x112 + x111;
+  int x115 = x114 * x113 + x112;
+  int x116 = x115 * x114 + x113;
+  int x117 = x116 * x115 + x114;
+  int x118 = x117 * x116 + x115;
+  int x119 = x118 * x117 + x116;
+  int x120 = x119 * x118 + x117;
+  int x121 = x120 * x119 + x118;
+  int x122 = x121 * x120 + x119;
+  int x123 = x122 * x121 + x120;
+  int x124 = x123 * x122 + x121;
+  int x125 = x124 * x123 + x122;
+  int x126 = x125 * x124 + x123;
+  int x127 = x126 * x

[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay added a comment.

Thanks for noticing the test breakages Pavel.  I fixed the bug, and verified 
that the tests you mentioned pass. PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76806



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

[Lldb-commits] [lldb] 0d525ce - [lldb] Fix breakeage introduced by llvm::LoadInst api change

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-04-10T00:11:00+02:00
New Revision: 0d525ce068c95a73558ed88ec3a1272c0ffe0e96

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

LOG: [lldb] Fix breakeage introduced by llvm::LoadInst api change

Added: 


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

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
index b9442872c785..2e16ba38bdde 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
@@ -1120,7 +1120,7 @@ bool 
IRForTarget::RewritePersistentAlloc(llvm::Instruction *persistent_alloc) {
   // Now, since the variable is a pointer variable, we will drop in a load of
   // that pointer variable.
 
-  LoadInst *persistent_load = new LoadInst(persistent_global, "", alloc);
+  LoadInst *persistent_load = new 
LoadInst(persistent_global->getType()->getPointerElementType(), 
persistent_global, "", alloc);
 
   LLDB_LOG(log, "Replacing \"{0}\" with \"{1}\"", PrintValue(alloc),
PrintValue(persistent_load));
@@ -1792,7 +1792,7 @@ bool IRForTarget::ReplaceVariables(Function 
&llvm_function) {
   get_element_ptr, value->getType()->getPointerTo(), "",
   entry_instruction);
 
-  LoadInst *load = new LoadInst(bit_cast, "", entry_instruction);
+  LoadInst *load = new 
LoadInst(bit_cast->getType()->getPointerElementType(), bit_cast, "", 
entry_instruction);
 
   return load;
 } else {

diff  --git 
a/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
 
b/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
index 34e362fa489c..73203d163665 100644
--- 
a/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ 
b/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -189,7 +189,7 @@ bool fixupX86StructRetCalls(llvm::Module &module) {
 ->setName("new_func_ptr_load_cast");
 // load the new function address ready for a jump
 llvm::LoadInst *new_func_addr_load =
-new llvm::LoadInst(new_func_ptr, "load_func_pointer", call_inst);
+new llvm::LoadInst(new_func_ptr->getType()->getPointerElementType(), 
new_func_ptr, "load_func_pointer", call_inst);
 // and create a callinstruction from it
 llvm::CallInst *new_call_inst =
 llvm::CallInst::Create(new_func_type, new_func_addr_load, 
new_call_args,
@@ -197,7 +197,7 @@ bool fixupX86StructRetCalls(llvm::Module &module) {
 new_call_inst->setCallingConv(call_inst->getCallingConv());
 new_call_inst->setTailCall(call_inst->isTailCall());
 llvm::LoadInst *lldb_save_result_address =
-new llvm::LoadInst(return_value_alloc, "save_return_val", call_inst);
+new 
llvm::LoadInst(return_value_alloc->getType()->getPointerElementType(), 
return_value_alloc, "save_return_val", call_inst);
 
 // Now remove the old broken call
 call_inst->replaceAllUsesWith(lldb_save_result_address);



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

[Lldb-commits] [PATCH] D77698: [DWARF] Assign the correct scope to constant variables

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1028
+case DW_TAG_inlined_subroutine:
+  return false;
+

[Sorry for showing up late.]

This seems unintuitive. How is 

```
void f() {
static int g_i = 0;
}
```

represented in DWARF?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1029
+  return false;
+
+case DW_TAG_compile_unit:

To save everyone the trouble:
```
0x002a:   DW_TAG_subprogram
DW_AT_low_pc(0x)
DW_AT_high_pc   (0x0006)
DW_AT_frame_base(DW_OP_reg6 RBP)
DW_AT_name  ("f")
DW_AT_decl_file ("/tmp/t.c")
DW_AT_decl_line (1)
DW_AT_external  (true)

0x003f: DW_TAG_variable
  DW_AT_name("g_i")
  DW_AT_type(0x0055 "int")
  DW_AT_decl_file   ("/tmp/t.c")
  DW_AT_decl_line   (2)
  DW_AT_location(DW_OP_addr 0x280)

0x0054: NULL
```


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

https://reviews.llvm.org/D77698



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

[Lldb-commits] [PATCH] D77698: [DWARF] Assign the correct scope to constant variables

[Davide Italiano via Phabricator via lldb-commits]

davide marked an inline comment as done.
davide added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1029
+  return false;
+
+case DW_TAG_compile_unit:

aprantl wrote:
> To save everyone the trouble:
> ```
> 0x002a:   DW_TAG_subprogram
> DW_AT_low_pc  (0x)
> DW_AT_high_pc (0x0006)
> DW_AT_frame_base  (DW_OP_reg6 RBP)
> DW_AT_name("f")
> DW_AT_decl_file   ("/tmp/t.c")
> DW_AT_decl_line   (1)
> DW_AT_external(true)
> 
> 0x003f: DW_TAG_variable
>   DW_AT_name  ("g_i")
>   DW_AT_type  (0x0055 "int")
>   DW_AT_decl_file ("/tmp/t.c")
>   DW_AT_decl_line (2)
>   DW_AT_location  (DW_OP_addr 0x280)
> 
> 0x0054: NULL
> ```
hmm, so, what's the DWARF'y way to find out whether a variable is static? 
This code is just moved from another place which did this.


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

https://reviews.llvm.org/D77698



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

[Lldb-commits] [lldb] 6612b82 - [lldb] Reformat 'Fix breakage introduced by llvm::LoadInst api change' (NFC)

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-04-10T00:37:08+02:00
New Revision: 6612b826d05cc9fd1beb47a6a93f9e2b093b48af

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

LOG: [lldb] Reformat 'Fix breakage introduced by llvm::LoadInst api change' 
(NFC)

Added: 


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

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
index 2e16ba38bdde..92294ff9c727 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
@@ -1120,7 +1120,9 @@ bool 
IRForTarget::RewritePersistentAlloc(llvm::Instruction *persistent_alloc) {
   // Now, since the variable is a pointer variable, we will drop in a load of
   // that pointer variable.
 
-  LoadInst *persistent_load = new 
LoadInst(persistent_global->getType()->getPointerElementType(), 
persistent_global, "", alloc);
+  LoadInst *persistent_load =
+  new LoadInst(persistent_global->getType()->getPointerElementType(),
+   persistent_global, "", alloc);
 
   LLDB_LOG(log, "Replacing \"{0}\" with \"{1}\"", PrintValue(alloc),
PrintValue(persistent_load));
@@ -1792,7 +1794,9 @@ bool IRForTarget::ReplaceVariables(Function 
&llvm_function) {
   get_element_ptr, value->getType()->getPointerTo(), "",
   entry_instruction);
 
-  LoadInst *load = new 
LoadInst(bit_cast->getType()->getPointerElementType(), bit_cast, "", 
entry_instruction);
+  LoadInst *load =
+  new LoadInst(bit_cast->getType()->getPointerElementType(),
+   bit_cast, "", entry_instruction);
 
   return load;
 } else {

diff  --git 
a/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
 
b/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
index 73203d163665..51bf5655ed12 100644
--- 
a/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ 
b/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -189,15 +189,17 @@ bool fixupX86StructRetCalls(llvm::Module &module) {
 ->setName("new_func_ptr_load_cast");
 // load the new function address ready for a jump
 llvm::LoadInst *new_func_addr_load =
-new llvm::LoadInst(new_func_ptr->getType()->getPointerElementType(), 
new_func_ptr, "load_func_pointer", call_inst);
+new llvm::LoadInst(new_func_ptr->getType()->getPointerElementType(),
+   new_func_ptr, "load_func_pointer", call_inst);
 // and create a callinstruction from it
 llvm::CallInst *new_call_inst =
 llvm::CallInst::Create(new_func_type, new_func_addr_load, 
new_call_args,
"new_func_call", call_inst);
 new_call_inst->setCallingConv(call_inst->getCallingConv());
 new_call_inst->setTailCall(call_inst->isTailCall());
-llvm::LoadInst *lldb_save_result_address =
-new 
llvm::LoadInst(return_value_alloc->getType()->getPointerElementType(), 
return_value_alloc, "save_return_val", call_inst);
+llvm::LoadInst *lldb_save_result_address = new llvm::LoadInst(
+return_value_alloc->getType()->getPointerElementType(),
+return_value_alloc, "save_return_val", call_inst);
 
 // Now remove the old broken call
 call_inst->replaceAllUsesWith(lldb_save_result_address);



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

[Lldb-commits] [PATCH] D77698: [DWARF] Assign the correct scope to constant variables

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1029
+  return false;
+
+case DW_TAG_compile_unit:

davide wrote:
> aprantl wrote:
> > To save everyone the trouble:
> > ```
> > 0x002a:   DW_TAG_subprogram
> > DW_AT_low_pc(0x)
> > DW_AT_high_pc   (0x0006)
> > DW_AT_frame_base(DW_OP_reg6 RBP)
> > DW_AT_name  ("f")
> > DW_AT_decl_file ("/tmp/t.c")
> > DW_AT_decl_line (1)
> > DW_AT_external  (true)
> > 
> > 0x003f: DW_TAG_variable
> >   DW_AT_name("g_i")
> >   DW_AT_type(0x0055 "int")
> >   DW_AT_decl_file   ("/tmp/t.c")
> >   DW_AT_decl_line   (2)
> >   DW_AT_location(DW_OP_addr 0x280)
> > 
> > 0x0054: NULL
> > ```
> hmm, so, what's the DWARF'y way to find out whether a variable is static? 
> This code is just moved from another place which did this.
Looks like the function's name is just confusing. We should add a doxgygen 
comment for it,


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

https://reviews.llvm.org/D77698



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

[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Mostly good, but it seems weird to supply the cache line size when calling the 
MemoryCache::Clear() function. We also don't seem to be catching updates to the 
cache line size property and invalidating the cache when and only when the 
property is modified, but we seem to be relying on calls to Clear() to do this 
when we stop? It would be nice to clean this up before submitting just because 
this change forces this now out of place looking setting to be passed along. 
ProcessProperties::ProcessProperties() has a place where you can hook in and 
call a function when the property changes:

  ProcessProperties::ProcessProperties(lldb_private::Process *process)
  : Properties(),
m_process(process) // Can be nullptr for global ProcessProperties
  {
if (process == nullptr) {
  // Global process properties, set them up one time
  m_collection_sp =
  
std::make_shared(ConstString("process"));
  m_collection_sp->Initialize(g_process_properties);
  m_collection_sp->AppendProperty(
  ConstString("thread"), ConstString("Settings specific to threads."),
  true, Thread::GetGlobalProperties()->GetValueProperties());
} else {
  m_collection_sp = std::make_shared(
  Process::GetGlobalProperties().get());
  m_collection_sp->SetValueChangedCallback(
  ePropertyPythonOSPluginPath,
  [this] { m_process->LoadOperatingSystemPlugin(true); });
}
  }

So we could add:

  m_collection_sp->SetValueChangedCallback(
  ePropertyMemCacheLineSize,
  [this] { m_process->UpdateCacheLineSize(); });

This function would then update the cache line size in the m_memory_cache 
variable.




Comment at: lldb/source/Target/Memory.cpp:31-38
+void MemoryCache::Clear(uint64_t cache_line_size, bool clear_invalid_ranges) {
   std::lock_guard guard(m_mutex);
   m_L1_cache.clear();
   m_L2_cache.clear();
   if (clear_invalid_ranges)
 m_invalid_ranges.Clear();
+  m_L2_cache_line_byte_size = cache_line_size;

Do we call clear when the cache line size changes? Maybe we should just have a 
function that does this and only this?:

```
void MemoryCache::SetCacheLineSize(uint64_t size);
```




Comment at: lldb/source/Target/Process.cpp:614
   m_image_tokens.clear();
-  m_memory_cache.Clear();
+  m_memory_cache.Clear(GetMemoryCacheLineSize());
   m_allocated_memory_cache.Clear();

Seems weird to be passing the cache line size to the clear method. Here we 
clearly just want to clear out the cache and don't care about the cache line 
size.



Comment at: lldb/source/Target/Process.cpp:1497
 m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
-  m_memory_cache.Clear();
+  m_memory_cache.Clear(GetMemoryCacheLineSize());
   LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",

Seems weird to be passing the cache line size to the clear method. Here we 
clearly just want to clear out the cache and don't care about the cache line 
size.



Comment at: lldb/source/Target/Process.cpp:5615
   m_thread_list.DiscardThreadPlans();
-  m_memory_cache.Clear(true);
+  m_memory_cache.Clear(GetMemoryCacheLineSize(), true);
   DoDidExec();

Seems weird to be passing the cache line size to the clear method. Here we 
clearly just want to clear out the cache and don't care about the cache line 
size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77790



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

[Lldb-commits] [PATCH] D77842: Fix setting Python3_ROOT_DIR on Windows

[Isuru Fernando via Phabricator via lldb-commits]

isuruf created this revision.
Herald added subscribers: lldb-commits, mgorny.
Herald added a project: LLDB.
isuruf added a reviewer: LLDB.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77842

Files:
  lldb/cmake/modules/FindPythonInterpAndLibs.cmake


Index: lldb/cmake/modules/FindPythonInterpAndLibs.cmake
===
--- lldb/cmake/modules/FindPythonInterpAndLibs.cmake
+++ lldb/cmake/modules/FindPythonInterpAndLibs.cmake
@@ -11,7 +11,7 @@
   if (SWIG_FOUND)
 if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
   # Use PYTHON_HOME as a hint to find Python 3.
-  set(Python3_ROOT_DIR PYTHON_HOME)
+  set(Python3_ROOT_DIR "${PYTHON_HOME}")
   find_package(Python3 COMPONENTS Interpreter Development)
   if (Python3_FOUND AND Python3_Interpreter_FOUND)
 set(PYTHON_LIBRARIES ${Python3_LIBRARIES})


Index: lldb/cmake/modules/FindPythonInterpAndLibs.cmake
===
--- lldb/cmake/modules/FindPythonInterpAndLibs.cmake
+++ lldb/cmake/modules/FindPythonInterpAndLibs.cmake
@@ -11,7 +11,7 @@
   if (SWIG_FOUND)
 if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
   # Use PYTHON_HOME as a hint to find Python 3.
-  set(Python3_ROOT_DIR PYTHON_HOME)
+  set(Python3_ROOT_DIR "${PYTHON_HOME}")
   find_package(Python3 COMPONENTS Interpreter Development)
   if (Python3_FOUND AND Python3_Interpreter_FOUND)
 set(PYTHON_LIBRARIES ${Python3_LIBRARIES})
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

[Vedant Kumar via Phabricator via lldb-commits]

vsk created this revision.
vsk added reviewers: JDevlieghere, davide.
Herald added a subscriber: mgorny.
Herald added a project: LLDB.

Languages can have different ways of formatting special characters.
E.g. when debugging C++ code a string might look like "\b", but when
debugging Swift code the same string would look like "\u{8}".

To make this work, plugins override GetStringPrinterEscapingHelper.
However, because there's a large amount of subtly divergent work done in
each override, we end up with large amounts of duplicated code. And all
the memory smashers fixed in one copy of the logic (see D73860 
) don't
get fixed in the others.

IMO the GetStringPrinterEscapingHelper is overly general and hard to
use. I propose deleting it and replacing it with an EscapeStyle enum,
which can be set as needed by each plugin.

A fix for some swift-lldb memory smashers falls out fairly naturally
from this deletion (https://github.com/apple/llvm-project/pull/1046). As
the swift logic becomes really tiny, I propose moving it upstream as
part of this change. I've added unit tests to cover it.

rdar://61419673


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77843

Files:
  lldb/include/lldb/DataFormatters/StringPrinter.h
  lldb/include/lldb/Target/Language.h
  lldb/source/DataFormatters/StringPrinter.cpp
  lldb/source/Plugins/Language/ObjC/NSString.cpp
  lldb/source/Target/Language.cpp
  lldb/unittests/DataFormatter/CMakeLists.txt
  lldb/unittests/DataFormatter/StringPrinterTests.cpp

Index: lldb/unittests/DataFormatter/StringPrinterTests.cpp
===
--- /dev/null
+++ lldb/unittests/DataFormatter/StringPrinterTests.cpp
@@ -0,0 +1,135 @@
+//===-- StringPrinterTests.cpp --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/DataFormatters/StringPrinter.h"
+#include "lldb/Utility/DataExtractor.h"
+#include "lldb/Utility/Endian.h"
+#include "lldb/Utility/StreamString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace lldb;
+using namespace lldb_private;
+using lldb_private::formatters::StringPrinter;
+using llvm::StringRef;
+
+#define QUOTE(x) "\"" x "\""
+
+/// Dump out "name: {  }".
+static void dumpStr(StringRef name, StringRef str) {
+  llvm::errs() << name << ": { ";
+  for (char c : str)
+llvm::errs() << c << " ";
+  llvm::errs() << "}\n";
+}
+
+/// Format \p input according to the options specified in the template params,
+/// then check whether the result is equal to \p reference. If not, dump the
+/// expeted vs. actual results.
+template 
+static bool isFormatCorrect(StringRef input, StringRef reference) {
+  StreamString out;
+  StringPrinter::ReadBufferAndDumpToStreamOptions opts;
+  opts.SetStream(&out);
+  opts.SetSourceSize(input.size());
+  opts.SetNeedsZeroTermination(true);
+  opts.SetEscapeNonPrintables(true);
+  opts.SetIgnoreMaxLength(false);
+  opts.SetEscapeStyle(escape_style);
+  DataExtractor extractor(input.data(), input.size(),
+  endian::InlHostByteOrder(), sizeof(void *));
+  opts.SetData(extractor);
+  const bool success = StringPrinter::ReadBufferAndDumpToStream(opts);
+  const bool matches = out.GetString() == reference;
+  if (!success || !matches) {
+dumpStr("expected", reference);
+dumpStr(" but got", out.GetString());
+  }
+  return matches;
+}
+
+// The "StringElementType::ASCII + EscapeStyle::CXX" combination is not tested
+// because it probably should not be supported (see FIXME in StringPrinter.cpp),
+// and because it's implemented by calling into the UTF8 logic anyway.
+
+// Test UTF8 formatting for C++.
+TEST(StringPrinterTests, CxxUTF8) {
+  auto matches = [](StringRef str, StringRef reference) {
+return isFormatCorrect(str, reference);
+  };
+
+  // Special escapes.
+  EXPECT_TRUE(matches({"\0", 1}, QUOTE("")));
+  EXPECT_TRUE(matches("\a", QUOTE("\\a")));
+  EXPECT_TRUE(matches("\b", QUOTE("\\b")));
+  EXPECT_TRUE(matches("\f", QUOTE("\\f")));
+  EXPECT_TRUE(matches("\n", QUOTE("\\n")));
+  EXPECT_TRUE(matches("\r", QUOTE("\\r")));
+  EXPECT_TRUE(matches("\t", QUOTE("\\t")));
+  EXPECT_TRUE(matches("\v", QUOTE("\\v")));
+  EXPECT_TRUE(matches("\"", QUOTE("\\\"")));
+  EXPECT_TRUE(matches("\'", QUOTE("'")));
+  EXPECT_TRUE(matches("\\", QUOTE("")));
+
+  // Printable characters.
+  EXPECT_TRUE(matches("'", QUOTE("'")));
+  EXPECT_TRUE(matches("a", QUOTE("a")));
+  EXPECT_TRUE(matches("Z", QUOTE("Z")));
+  EXPECT_TRUE(matches("🥑", QUOTE("🥑")));
+
+  // Octal (\nnn), hex (\xnn), extended octal (\u or \U).
+  EXPECT_TRUE(matches("\uD55C"

[Lldb-commits] [PATCH] D77842: Fix setting Python3_ROOT_DIR on Windows

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Can you explain why this is necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77842



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

[Lldb-commits] [PATCH] D77842: Fix setting Python3_ROOT_DIR on Windows

[Isuru Fernando via Phabricator via lldb-commits]

isuruf added a comment.

The intention of the code is to set the variable `Python3_ROOT_DIR` to the 
value of the variable `PYTHON_HOME`, but it was using just the string 
`"PYTHON_HOME"` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77842



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

[Lldb-commits] [PATCH] D77602: [lldb/Reproducers] Support inline replay

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 256505.
JDevlieghere added a comment.

- Simplify `LLDB_RECORD_CONSTRUCTOR` macro. The other macros have `return` 
statements that need to be inlined in the caller for replay, and the boundary 
tracking needs to be updated right, so I'm not convinced the extra complexity 
is worth the deduplication.
- Use the replayer ID to check the signature.

I didn't get around to adding the unit tests yet. I noticed that this change 
breaks something and I'm still debugging it. I think it's specific to the unit 
test though, as the test suite isn't affected.


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

https://reviews.llvm.org/D77602

Files:
  lldb/include/lldb/Utility/Reproducer.h
  lldb/include/lldb/Utility/ReproducerInstrumentation.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/API/SBReproducer.cpp
  lldb/source/API/SBReproducerPrivate.h
  lldb/source/Utility/Reproducer.cpp
  lldb/source/Utility/ReproducerInstrumentation.cpp

Index: lldb/source/Utility/ReproducerInstrumentation.cpp
===
--- lldb/source/Utility/ReproducerInstrumentation.cpp
+++ lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -8,9 +8,9 @@
 
 #include "lldb/Utility/ReproducerInstrumentation.h"
 #include "lldb/Utility/Reproducer.h"
-#include 
 #include 
 #include 
+#include 
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
@@ -97,7 +97,7 @@
 
   // Add a small artificial delay to ensure that all asynchronous events have
   // completed before we exit.
-  std::this_thread::sleep_for (std::chrono::milliseconds(100));
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
 
   return true;
 }
@@ -122,6 +122,22 @@
   return m_ids[id].second.ToString();
 }
 
+void Registry::CheckID(unsigned expected, unsigned actual) {
+  if (expected != actual) {
+llvm::errs() << "Reproducer expected signature " << expected << ": '"
+ << GetSignature(expected) << "'\n";
+llvm::errs() << "Reproducer actual signature " << actual << ": '"
+ << GetSignature(actual) << "'\n";
+llvm::report_fatal_error(
+"Detected reproducer replay divergence. Refusing to continue.");
+  }
+
+#ifdef LLDB_REPRO_INSTR_TRACE
+  llvm::errs() << "Replaying " << actual << ": " << GetSignature(actual)
+   << "\n";
+#endif
+}
+
 Replayer *Registry::GetReplayer(unsigned id) {
   assert(m_ids.count(id) != 0 && "ID not in registry");
   return m_ids[id].first;
Index: lldb/source/Utility/Reproducer.cpp
===
--- lldb/source/Utility/Reproducer.cpp
+++ lldb/source/Utility/Reproducer.cpp
@@ -228,7 +228,8 @@
 }
 
 Loader::Loader(FileSpec root)
-: m_root(MakeAbsolute(std::move(root))), m_loaded(false) {}
+: m_root(MakeAbsolute(std::move(root))), m_loaded(false),
+  m_passive_replay(false) {}
 
 llvm::Error Loader::LoadIndex() {
   if (m_loaded)
Index: lldb/source/API/SBReproducerPrivate.h
===
--- lldb/source/API/SBReproducerPrivate.h
+++ lldb/source/API/SBReproducerPrivate.h
@@ -55,6 +55,17 @@
   SBRegistry m_registry;
 };
 
+class ReplayData {
+public:
+  ReplayData(llvm::StringRef buffer) : m_registry(), m_deserializer(buffer) {}
+  Deserializer &GetDeserializer() { return m_deserializer; }
+  Registry &GetRegistry() { return m_registry; }
+
+private:
+  SBRegistry m_registry;
+  Deserializer m_deserializer;
+};
+
 inline InstrumentationData GetInstrumentationData() {
   if (!lldb_private::repro::Reproducer::Initialized())
 return {};
@@ -64,6 +75,17 @@
 return {p.GetSerializer(), p.GetRegistry()};
   }
 
+  if (auto *l = lldb_private::repro::Reproducer::Instance().GetLoader()) {
+if (l->IsPassiveReplay()) {
+  FileSpec file = l->GetFile();
+  static auto error_or_file = llvm::MemoryBuffer::getFile(file.GetPath());
+  if (!error_or_file)
+return {};
+  static ReplayData r((*error_or_file)->getBuffer());
+  return {r.GetDeserializer(), r.GetRegistry()};
+}
+  }
+
   return {};
 }
 
Index: lldb/source/API/SBReproducer.cpp
===
--- lldb/source/API/SBReproducer.cpp
+++ lldb/source/API/SBReproducer.cpp
@@ -130,6 +130,8 @@
 error = llvm::toString(std::move(e));
 return error.c_str();
   }
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  loader->SetAPIReplay(true);
   return nullptr;
 }
 
Index: lldb/source/API/SBDebugger.cpp
===
--- lldb/source/API/SBDebugger.cpp
+++ lldb/source/API/SBDebugger.cpp
@@ -1629,31 +1629,31 @@
 
 template <> void RegisterMethods(Registry &R) {
   // Custom implementation.
-  R.Register(&invoke::method<&SBDebugger::SetErrorFileHandle>::doit,
+  R.Register(&invoke::method<
+ &SBDebugger::SetErrorFileHandle>::recor

[Lldb-commits] [lldb] a4da4e3 - [lldb/Reproducers] Fix typo introduced when disabling register failing tests

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-04-10T08:37:06+02:00
New Revision: a4da4e3292834b5f93f54440f825dd90876d6615

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

LOG: [lldb/Reproducers] Fix typo introduced when disabling register failing 
tests

Added: 


Modified: 
lldb/test/Shell/Register/x86-64-read.test
lldb/test/Shell/Register/x86-64-ymm-read.test

Removed: 




diff  --git a/lldb/test/Shell/Register/x86-64-read.test 
b/lldb/test/Shell/Register/x86-64-read.test
index 99937071add8..ac3d4d1e2724 100644
--- a/lldb/test/Shell/Register/x86-64-read.test
+++ b/lldb/test/Shell/Register/x86-64-read.test
@@ -1,4 +1,5 @@
-# XFAIL: system-window, lldb-repro
+# UNSUPPORTED: lldb-repro
+# XFAIL: system-windows
 # REQUIRES: native && target-x86_64
 # RUN: %clangxx_host %p/Inputs/x86-64-read.cpp -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s

diff  --git a/lldb/test/Shell/Register/x86-64-ymm-read.test 
b/lldb/test/Shell/Register/x86-64-ymm-read.test
index 155a00ee6856..be9133b77d42 100644
--- a/lldb/test/Shell/Register/x86-64-ymm-read.test
+++ b/lldb/test/Shell/Register/x86-64-ymm-read.test
@@ -1,4 +1,5 @@
-# XFAIL: system-windows, lldb-repro
+# UNSUPPORTED: lldb-repro
+# XFAIL: system-windows
 # REQUIRES: native && target-x86_64 && native-cpu-avx
 # RUN: %clangxx_host %p/Inputs/x86-ymm-read.cpp -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s



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