[Lldb-commits] [PATCH] D66327: [dotest] Make it possible to forward CCC_OVERRIDE_OPTIONS to Make

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

How do you envision this to be used? Via something like 
`CCC_OVERRIDE_OPTIONS=foo ninja check-lldb` ?

Overall, I think it would be nice to avoid the tests being affected by the 
inherited environment variables because that opens the door for tests to 
unexpectedly fail or simply behave differently just because someone happens to 
have some weird environment variable set (and he may not even know about it). I 
think this is also consistent with the aproach taken in llvm (see e.g. 
`possibly_dangerous_env_vars` in `llvm/utils/lit/lit/llvm/config.py`). Overall, 
being able to run the (dotest) test suite with different options would be nice, 
but I think it would be better if there was a more explicit way to do that 
(e.g., a command line argument). Incidentally, `dotest` already has a command 
line argument (`--env`), which seems to be exactly intended for its purpose. 
Couldn't you just use that?

(Also, instead of CCC_OVERRIDE_OPTIONS, I think I'd try to use CFLAGS_EXTRAS, 
because that explicitly handled by makefiles, and so it gives an easier way for 
tests to opt-out of this when they want very specific flags for something)




Comment at: lldb/packages/Python/lldbsuite/test/plugins/builder_base.py:39
 
+def getEnv():
+"""Returns the build environment."""

I am confused. How is this function used and what's its relation the the second 
change below?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66327



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

[Lldb-commits] [PATCH] D66331: Save / restore selected platform in tests that may change it

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

> I'm not sure if fixing this on a test-by-test basis is the right place for 
> it. It might be better to have dotest.py's run_suite() save the remote 
> platform and lldbtest.py's class Base setUpClass() method re-set the platform 
> before the test is invoked, in case it was mutated during a test run.

Yeah, I was thinking the same thing. I think that, at this point, we have 
enough tests needs things like this that a more general solution is justified.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66331



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

[Lldb-commits] [PATCH] D66250: [JIT][Unwinder] Add Trampoline ObjectFile and UnwindPlan support for FCB

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D66250#1632187 , @jasonmolenda 
wrote:

> In D66250#1631200 , @labath wrote:
>
> > I have one high-level question. Who is generating the code inside the 
> > "trampoline" object file? Couldn't we have him generate proper unwind 
> > information so that the regular unwind machinery "just works"? I imagine 
> > all it would take is a couple of well-placed `.cfi` assembler directives...
>
>
> The ABI plugin creates the trampoline by writing bytes directly IIRC.  The 
> trampoline has to save the entire register context to the stack before it 
> calls the jitted function that does the comparison and possibly breaks.  Then 
> the trampoline restores all the register context, executes any replaced 
> functions, and then jumps directly back into the original function.
>
> The trampoline has to be expressed in terms of assembly (or hardcoded bytes) 
> because we're not using the normal calling conventions - we have to spill all 
> registers so the jitted function doesn't overwrite a caller-spilled register 
> and corrupt the original user function that we're patching in to.  Also, it 
> is not CALLed from the original user function, it is JMPed to, so there's no 
> way for the unwinder to get from the trampoline function back to the original 
> user function by looking at the stack.
>
> The question is then, why do we have to JMP to the trampoline?  This 
> simplifies running the replaced instructions in the trampoline, because the 
> stack pointer and frame pointer have the original user function's values when 
> the replaced instructions are executed at the end of the trampoline.  Using a 
> CALL to get from the original function to the trampoline solves the unwinder 
> problem, but we're still going to need to express the trampoline in assembly 
> terms to save & restore all the registers, and have the replaced instructions 
> execute.
>
> On non-x86 architectures, like Aarch64, doing a function call to the 
> trampoline would mean we need to spill the $lr value to the stack in the 
> original user function before calling the trampoline so we don't overwrite 
> the value; that's a larger number of instructions that need to be shifted 
> into the trampoline to execute.
>
> Of course the only reason we have this ObjectFile plugin is for the 
> unwinder's benefit.  If we used a CALL instruction to get to the trampoline, 
> and we sent the assembly through the clang FE as a series of asm()s to jit 
> it, instead of writing the bytes down, the unwinder would get the unwind 
> information as we do a normal jitted function -- you're right, cfi directives 
> to point to the saved register context would be sufficient.
>
> But because we're jumping directly to the trampoline, and jumping back from 
> the trampoline, we need to have a hand-written unwind plan that has the 
> return address hard coded in it for the unwinder to work.
>
> Is having a small ObjectFile plugin for this purpose a bad idea?  It seems 
> like the right approach, but maybe I'm not seeing the issue.  The 
> CreateInstance method returns false always, so when we iterate looking for an 
> ObjectFile plugin, we will call this method but it will return false right 
> away.


I wouldn't say creating a new ObjectFile class (*) is necessarily a "bad" idea. 
I am just wondering whether it is possible to implement this feature without 
introducing special knowledge about it to other parts of the code, as that has 
a lot of benefits (increasing coverage for the existing code paths that you do 
end up using, keeping the feature self-contained, etc.).

I understand the reason for doing jumps, and I totally agree with that. And I 
agree that in this case the unwinder would need some additional help to figure 
out the return address. However, we already have a mechanism for telling the 
unwinder how to unwind from tricky frames -- the eh_frame section. And this 
kind of unwind is easily expressible using eh_frame data -- you could say that 
via something like `DW_CFA_val_expression(%rip, DW_OP_const8u $WHATEVER)`. What 
you're doing here is extremely similar to the signal hander trampolines. These 
are also written in assembly and have hand-coded eh_frame data which precisely 
describes where the signal hander will return, and where are all the registers 
saved. Its unwind rule is something like

  row[0]:0: CFA=DW_OP_breg7 +160, DW_OP_deref  => rax=[DW_OP_breg7 +144] 
rdx=[DW_OP_breg7 +136] rcx=[DW_OP_breg7 +152] rbx=[DW_OP_breg7 +128] 
rsi=[DW_OP_breg7 +112] rdi=[DW_OP_breg7 +104] rbp=[DW_OP_breg7 +120] 
rsp=[DW_OP_breg7 +160] r8=[DW_OP_breg7 +40] r9=[DW_OP_breg7 +48] 
r10=[DW_OP_breg7 +56] r11=[DW_OP_breg7 +64] r12=[DW_OP_breg7 +72] 
r13=[DW_OP_breg7 +80] r14=[DW_OP_breg7 +88] r15=[DW_OP_breg7 +96] 
rip=[DW_OP_breg7 +168] 

These kinds of rules can be "easily" generated by an appropriate `.cfi_escape` 
directive, similar to how he it's done in some

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor created this revision.
teemperor added a reviewer: LLDB.
Herald added subscribers: lldb-commits, abidh.
Herald added a project: LLDB.

Added support for for-range iterating over StringList. There is no 
reverse-iterator support because it seems
no one is doing that in LLDB. Also added some tests and migrated LLDB code over 
to for-range loops
where possible.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66345

Files:
  lldb/include/lldb/Utility/CompletionRequest.h
  lldb/include/lldb/Utility/StringList.h
  lldb/source/Breakpoint/WatchpointOptions.cpp
  lldb/source/Commands/CommandObjectApropos.cpp
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/CommandObjectMultiword.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Utility/Args.cpp
  lldb/source/Utility/StringList.cpp
  lldb/unittests/Editline/EditlineTest.cpp
  lldb/unittests/Utility/StringListTest.cpp

Index: lldb/unittests/Utility/StringListTest.cpp
===
--- lldb/unittests/Utility/StringListTest.cpp
+++ lldb/unittests/Utility/StringListTest.cpp
@@ -504,3 +504,23 @@
   StringList s;
   EXPECT_EQ(0U, s.GetMaxStringLength());
 }
+
+TEST(StringListTest, ForRangeEmpty) {
+  StringList s;
+  for (const std::string &e : s)
+FAIL() << "Shouldn't have hit an element in for range";
+}
+
+TEST(StringListTest, ForRangeSingle) {
+  StringList s;
+  s.AppendString("a");
+  s.AppendString("b");
+  s.AppendString("c");
+  std::vector recorded;
+  for (const std::string &e : s)
+recorded.push_back(e);
+  EXPECT_EQ(3, recorded.size());
+  EXPECT_EQ("a", recorded.at(0));
+  EXPECT_EQ("b", recorded.at(1));
+  EXPECT_EQ("c", recorded.at(2));
+}
Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -196,8 +196,8 @@
   int start_block_count = 0;
   int brace_balance = 0;
 
-  for (size_t i = 0; i < lines.GetSize(); ++i) {
-for (auto ch : lines[i]) {
+  for (const std::string &line : lines) {
+for (auto ch : line) {
   if (ch == '{') {
 ++start_block_count;
 ++brace_balance;
@@ -312,8 +312,8 @@
   // Without any auto indentation support, our output should directly match our
   // input.
   std::vector reported_lines;
-  for (size_t i = 0; i < el_reported_lines.GetSize(); ++i)
-reported_lines.push_back(el_reported_lines[i]);
+  for (const std::string &line : el_reported_lines)
+reported_lines.push_back(line);
 
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
Index: lldb/source/Utility/StringList.cpp
===
--- lldb/source/Utility/StringList.cpp
+++ lldb/source/Utility/StringList.cpp
@@ -61,10 +61,7 @@
 }
 
 void StringList::AppendList(StringList strings) {
-  size_t len = strings.GetSize();
-
-  for (size_t i = 0; i < len; ++i)
-m_strings.push_back(strings.GetStringAtIndex(i));
+  m_strings.insert(m_strings.end(), strings.begin(), strings.end());
 }
 
 size_t StringList::GetSize() const { return m_strings.size(); }
Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -172,8 +172,8 @@
 Args::Args(const Args &rhs) { *this = rhs; }
 
 Args::Args(const StringList &list) : Args() {
-  for (size_t i = 0; i < list.GetSize(); ++i)
-AppendArgument(list[i]);
+  for (const std::string &arg : list)
+AppendArgument(arg);
 }
 
 Args &Args::operator=(const Args &rhs) {
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -195,9 +195,7 @@
 
 Status error;
 
-for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-  const char *type_name =
-  options->m_target_types.GetStringAtIndex(i);
+for (const std::string &type_name : options->m_target_types) {
   CommandObjectTypeSummaryAdd::AddSummary(
   ConstString(type_name), script_format,
   (options->m_regex
@@ -437,9 +435,7 @@
 
 Status error;
 
-for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-  const char *type_name =
-  options->m_target_types.GetStringAtIndex(i);
+for (const std::string &type_name : options->m_target_types) {
   ConstString const_type_name(type_name);
   if (const_type_name) {
 if (!CommandObjectTypeSynthAdd::AddSynth(
Index: lldb/source/Commands/CommandObjectMultiword.cpp
===
--- lldb

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor updated this revision to Diff 215586.
teemperor added a comment.

- Fixed some minor issues.


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

https://reviews.llvm.org/D66345

Files:
  lldb/include/lldb/Utility/CompletionRequest.h
  lldb/include/lldb/Utility/StringList.h
  lldb/source/Breakpoint/WatchpointOptions.cpp
  lldb/source/Commands/CommandObjectApropos.cpp
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/CommandObjectMultiword.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Utility/Args.cpp
  lldb/source/Utility/StringList.cpp
  lldb/unittests/Editline/EditlineTest.cpp
  lldb/unittests/Utility/StringListTest.cpp

Index: lldb/unittests/Utility/StringListTest.cpp
===
--- lldb/unittests/Utility/StringListTest.cpp
+++ lldb/unittests/Utility/StringListTest.cpp
@@ -504,3 +504,23 @@
   StringList s;
   EXPECT_EQ(0U, s.GetMaxStringLength());
 }
+
+TEST(StringListTest, ForRangeEmpty) {
+  StringList s;
+  for (const std::string &e : s)
+FAIL() << "Shouldn't have hit an element in for range" << e;
+}
+
+TEST(StringListTest, ForRangeSingle) {
+  StringList s;
+  s.AppendString("a");
+  s.AppendString("b");
+  s.AppendString("c");
+  std::vector recorded;
+  for (const std::string &e : s)
+recorded.push_back(e);
+  EXPECT_EQ(3U, recorded.size());
+  EXPECT_EQ("a", recorded.at(0));
+  EXPECT_EQ("b", recorded.at(1));
+  EXPECT_EQ("c", recorded.at(2));
+}
Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -196,8 +196,8 @@
   int start_block_count = 0;
   int brace_balance = 0;
 
-  for (size_t i = 0; i < lines.GetSize(); ++i) {
-for (auto ch : lines[i]) {
+  for (const std::string &line : lines) {
+for (auto ch : line) {
   if (ch == '{') {
 ++start_block_count;
 ++brace_balance;
@@ -312,8 +312,8 @@
   // Without any auto indentation support, our output should directly match our
   // input.
   std::vector reported_lines;
-  for (size_t i = 0; i < el_reported_lines.GetSize(); ++i)
-reported_lines.push_back(el_reported_lines[i]);
+  for (const std::string &line : el_reported_lines)
+reported_lines.push_back(line);
 
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
Index: lldb/source/Utility/StringList.cpp
===
--- lldb/source/Utility/StringList.cpp
+++ lldb/source/Utility/StringList.cpp
@@ -61,10 +61,7 @@
 }
 
 void StringList::AppendList(StringList strings) {
-  size_t len = strings.GetSize();
-
-  for (size_t i = 0; i < len; ++i)
-m_strings.push_back(strings.GetStringAtIndex(i));
+  m_strings.insert(m_strings.end(), strings.begin(), strings.end());
 }
 
 size_t StringList::GetSize() const { return m_strings.size(); }
Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -172,8 +172,8 @@
 Args::Args(const Args &rhs) { *this = rhs; }
 
 Args::Args(const StringList &list) : Args() {
-  for (size_t i = 0; i < list.GetSize(); ++i)
-AppendArgument(list[i]);
+  for (const std::string &arg : list)
+AppendArgument(arg);
 }
 
 Args &Args::operator=(const Args &rhs) {
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -195,9 +195,7 @@
 
 Status error;
 
-for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-  const char *type_name =
-  options->m_target_types.GetStringAtIndex(i);
+for (const std::string &type_name : options->m_target_types) {
   CommandObjectTypeSummaryAdd::AddSummary(
   ConstString(type_name), script_format,
   (options->m_regex
@@ -437,9 +435,7 @@
 
 Status error;
 
-for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-  const char *type_name =
-  options->m_target_types.GetStringAtIndex(i);
+for (const std::string &type_name : options->m_target_types) {
   ConstString const_type_name(type_name);
   if (const_type_name) {
 if (!CommandObjectTypeSynthAdd::AddSynth(
Index: lldb/source/Commands/CommandObjectMultiword.cpp
===
--- lldb/source/Commands/CommandObjectMultiword.cpp
+++ lldb/source/Commands/CommandObjectMultiword.cpp
@@ -136,9 +136,9 @@
 
   if (num_subcmd_matches > 0) {
 error_msg.append(" Possible completions:");
-for (size_t i = 0

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Definitely an improvement, though one day, i'd like to get rid of the 
StringList class (and various other XXXList classes) completely :)




Comment at: lldb/unittests/Utility/StringListTest.cpp:522-525
+  EXPECT_EQ(3U, recorded.size());
+  EXPECT_EQ("a", recorded.at(0));
+  EXPECT_EQ("b", recorded.at(1));
+  EXPECT_EQ("c", recorded.at(2));

maybe `EXPECT_THAT(recorded, testing::ElementsAre("a", "b", "c"))` ?
(The advantages are that it's shorter, produces better error messages, and 
doesn't cause the subsequent checks to invoke UB if recorded.size happens to be 
less than 3).


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

https://reviews.llvm.org/D66345



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

[Lldb-commits] [lldb] r369113 - [lldb][NFC] Allow for-ranges on StringList

2019-08-16 Thread Raphael Isemann via lldb-commits 
Author: teemperor
Date: Fri Aug 16 07:27:35 2019
New Revision: 369113

URL: http://llvm.org/viewvc/llvm-project?rev=369113&view=rev
Log:
[lldb][NFC] Allow for-ranges on StringList

Modified:
lldb/trunk/include/lldb/Utility/CompletionRequest.h
lldb/trunk/include/lldb/Utility/StringList.h
lldb/trunk/source/Breakpoint/WatchpointOptions.cpp
lldb/trunk/source/Commands/CommandObjectApropos.cpp
lldb/trunk/source/Commands/CommandObjectCommands.cpp
lldb/trunk/source/Commands/CommandObjectMultiword.cpp
lldb/trunk/source/Commands/CommandObjectType.cpp
lldb/trunk/source/Utility/Args.cpp
lldb/trunk/source/Utility/StringList.cpp
lldb/trunk/unittests/Editline/EditlineTest.cpp
lldb/trunk/unittests/Utility/StringListTest.cpp

Modified: lldb/trunk/include/lldb/Utility/CompletionRequest.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/CompletionRequest.h?rev=369113&r1=369112&r2=369113&view=diff
==
--- lldb/trunk/include/lldb/Utility/CompletionRequest.h (original)
+++ lldb/trunk/include/lldb/Utility/CompletionRequest.h Fri Aug 16 07:27:35 2019
@@ -116,8 +116,8 @@ public:
   ///
   /// \see AddCompletion
   void AddCompletions(const StringList &completions) {
-for (std::size_t i = 0; i < completions.GetSize(); ++i)
-  AddCompletion(completions.GetStringAtIndex(i));
+for (const std::string &completion : completions)
+  AddCompletion(completion);
   }
 
   /// Adds multiple possible completion strings alongside their descriptions.

Modified: lldb/trunk/include/lldb/Utility/StringList.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/StringList.h?rev=369113&r1=369112&r2=369113&view=diff
==
--- lldb/trunk/include/lldb/Utility/StringList.h (original)
+++ lldb/trunk/include/lldb/Utility/StringList.h Fri Aug 16 07:27:35 2019
@@ -23,6 +23,8 @@ class Stream;
 namespace lldb_private {
 
 class StringList {
+  typedef std::vector StorageType;
+
 public:
   StringList();
 
@@ -52,6 +54,14 @@ public:
 
   size_t GetMaxStringLength() const;
 
+  typedef StorageType::iterator iterator;
+  typedef StorageType::const_iterator const_iterator;
+
+  iterator begin() { return m_strings.begin(); }
+  iterator end() { return m_strings.end(); }
+  const_iterator begin() const { return m_strings.begin(); }
+  const_iterator end() const { return m_strings.end(); }
+
   std::string &operator[](size_t idx) {
 // No bounds checking, verify "idx" is good prior to calling this function
 return m_strings[idx];
@@ -125,7 +135,7 @@ public:
   }
 
 private:
-  std::vector m_strings;
+  StorageType m_strings;
 };
 
 } // namespace lldb_private

Modified: lldb/trunk/source/Breakpoint/WatchpointOptions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/WatchpointOptions.cpp?rev=369113&r1=369112&r2=369113&view=diff
==
--- lldb/trunk/source/Breakpoint/WatchpointOptions.cpp (original)
+++ lldb/trunk/source/Breakpoint/WatchpointOptions.cpp Fri Aug 16 07:27:35 2019
@@ -170,9 +170,8 @@ void WatchpointOptions::CommandBaton::Ge
 
   s->IndentMore();
   if (data && data->user_source.GetSize() > 0) {
-const size_t num_strings = data->user_source.GetSize();
-for (size_t i = 0; i < num_strings; ++i) {
-  s->Indent(data->user_source.GetStringAtIndex(i));
+for (const std::string &line : data->user_source) {
+  s->Indent(line);
   s->EOL();
 }
   } else {

Modified: lldb/trunk/source/Commands/CommandObjectApropos.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectApropos.cpp?rev=369113&r1=369112&r2=369113&view=diff
==
--- lldb/trunk/source/Commands/CommandObjectApropos.cpp (original)
+++ lldb/trunk/source/Commands/CommandObjectApropos.cpp Fri Aug 16 07:27:35 2019
@@ -65,10 +65,8 @@ bool CommandObjectApropos::DoExecute(Arg
   "The following commands may relate to '%s':\n", args[0].c_str());
   size_t max_len = 0;
 
-  for (size_t i = 0; i < commands_found.GetSize(); ++i) {
-size_t len = strlen(commands_found.GetStringAtIndex(i));
-if (len > max_len)
-  max_len = len;
+  for (const std::string &command : commands_found) {
+max_len = std::max(max_len, command.size());
   }
 
   for (size_t i = 0; i < commands_found.GetSize(); ++i)

Modified: lldb/trunk/source/Commands/CommandObjectCommands.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectCommands.cpp?rev=369113&r1=369112&r2=369113&view=diff
==
--- lldb/trunk/source/Commands/CommandObjectCommands.cpp (original)
+++ lldb/t

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor updated this revision to Diff 215595.
teemperor added a comment.

Thanks!

Not sure if we can get rid of StringList so easily as we still have 
SBStringList.


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

https://reviews.llvm.org/D66345

Files:
  lldb/include/lldb/Utility/CompletionRequest.h
  lldb/include/lldb/Utility/StringList.h
  lldb/source/Breakpoint/WatchpointOptions.cpp
  lldb/source/Commands/CommandObjectApropos.cpp
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/CommandObjectMultiword.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Utility/Args.cpp
  lldb/source/Utility/StringList.cpp
  lldb/unittests/Editline/EditlineTest.cpp
  lldb/unittests/Utility/StringListTest.cpp

Index: lldb/unittests/Utility/StringListTest.cpp
===
--- lldb/unittests/Utility/StringListTest.cpp
+++ lldb/unittests/Utility/StringListTest.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Utility/StringList.h"
 #include "lldb/Utility/StreamString.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
@@ -504,3 +505,20 @@
   StringList s;
   EXPECT_EQ(0U, s.GetMaxStringLength());
 }
+
+TEST(StringListTest, ForRangeEmpty) {
+  StringList s;
+  for (const std::string &e : s)
+FAIL() << "Shouldn't have hit an element in for range" << e;
+}
+
+TEST(StringListTest, ForRangeSingle) {
+  StringList s;
+  s.AppendString("a");
+  s.AppendString("b");
+  s.AppendString("c");
+  std::vector recorded;
+  for (const std::string &e : s)
+recorded.push_back(e);
+  EXPECT_THAT(recorded, testing::ElementsAre("a", "b", "c"));
+}
Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -196,8 +196,8 @@
   int start_block_count = 0;
   int brace_balance = 0;
 
-  for (size_t i = 0; i < lines.GetSize(); ++i) {
-for (auto ch : lines[i]) {
+  for (const std::string &line : lines) {
+for (auto ch : line) {
   if (ch == '{') {
 ++start_block_count;
 ++brace_balance;
@@ -312,8 +312,8 @@
   // Without any auto indentation support, our output should directly match our
   // input.
   std::vector reported_lines;
-  for (size_t i = 0; i < el_reported_lines.GetSize(); ++i)
-reported_lines.push_back(el_reported_lines[i]);
+  for (const std::string &line : el_reported_lines)
+reported_lines.push_back(line);
 
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
Index: lldb/source/Utility/StringList.cpp
===
--- lldb/source/Utility/StringList.cpp
+++ lldb/source/Utility/StringList.cpp
@@ -61,10 +61,7 @@
 }
 
 void StringList::AppendList(StringList strings) {
-  size_t len = strings.GetSize();
-
-  for (size_t i = 0; i < len; ++i)
-m_strings.push_back(strings.GetStringAtIndex(i));
+  m_strings.insert(m_strings.end(), strings.begin(), strings.end());
 }
 
 size_t StringList::GetSize() const { return m_strings.size(); }
Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -172,8 +172,8 @@
 Args::Args(const Args &rhs) { *this = rhs; }
 
 Args::Args(const StringList &list) : Args() {
-  for (size_t i = 0; i < list.GetSize(); ++i)
-AppendArgument(list[i]);
+  for (const std::string &arg : list)
+AppendArgument(arg);
 }
 
 Args &Args::operator=(const Args &rhs) {
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -195,9 +195,7 @@
 
 Status error;
 
-for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-  const char *type_name =
-  options->m_target_types.GetStringAtIndex(i);
+for (const std::string &type_name : options->m_target_types) {
   CommandObjectTypeSummaryAdd::AddSummary(
   ConstString(type_name), script_format,
   (options->m_regex
@@ -437,9 +435,7 @@
 
 Status error;
 
-for (size_t i = 0; i < options->m_target_types.GetSize(); i++) {
-  const char *type_name =
-  options->m_target_types.GetStringAtIndex(i);
+for (const std::string &type_name : options->m_target_types) {
   ConstString const_type_name(type_name);
   if (const_type_name) {
 if (!CommandObjectTypeSynthAdd::AddSynth(
Index: lldb/source/Commands/CommandObjectMultiword.cpp
===
--- lldb/source/Commands/CommandObjectMultiword.cpp
+++ lldb/

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D66345#1633118 , @teemperor wrote:

> Not sure if we can get rid of StringList so easily as we still have 
> SBStringList.


We can keep the SBStringList. We can just have it be backed by a 
`vector` instead of the StringList thingy...


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

https://reviews.llvm.org/D66345



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

[Lldb-commits] [PATCH] D66249: [JIT][Breakpoint] Add "BreakpointInjectedSite" and FCB Trampoline

2019-08-16 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added inline comments.



Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:35
+/// "lldb/Breakpoint/BreakpointInjectedSite.h" Class that manages the injected
+/// conditional breakpoints.
+//--

This comment assumes that the reader knows what injected breakpoints are; can 
you either link to the file where this is explained or add a one-paragraph 
description of what this means here?



Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:37
+//--
+
+struct ArgumentMetadata {

Is this still recognized by doxygen if there is an empty line before the 
declaration?



Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:38
+
+struct ArgumentMetadata {
+

can this be an inner class of BreakpointInjectedSite?



Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:58
+
+  BreakpointInjectedSite(BreakpointSiteList *list,
+   const lldb::BreakpointLocationSP &owner,

Doxygen comment explaining the \param eters?



Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:64
+
+  bool BuildConditionExpression(void);
+

Please add comments to all non-obvious functions.



Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:90
+  lldb::TargetSP m_target_sp;
+  Address m_real_addr;
+  Address m_trap_addr;

... and members :-)



Comment at: lldb/include/lldb/Target/ABI.h:143
+
+  virtual uint64_t GetJumpOpcode(void) { return 0; }
+

Comment explaining what this is supposed to do?

Is there always *exactly* one opcode that fits this requirement, and is an 
Opcode always < 64 bits on all ABIs?



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:181
+# FCB falls back to regular conditional breakpoint that get hit once.
+self.assertTrue(breakpoint.GetHitCount() == 1)

how does this test ensure that the breakpoint was actually injected and that it 
didn't fall back to a regular breakpoint?



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:30
+
+bool BreakpointInjectedSite::BuildConditionExpression(void) {
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_JIT_LOADER);

we usually write this just as `BuildConditionExpression()`



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:47
+   loc_sp->GetConditionText());
+  return false;
+}

Should this return an error instead of logging and returning false?



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:341
+  "\n"
+  "   typedef unsigned intuint32_t;\n"
+  "   typedef unsigned long long  uint64_t ;\n"

This seems awfully specific. Shouldn't this be target-dependent, and is there 
no pre-existing table in LLDB that we could derive this from?



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:382
+  "   intptr_t rcx;\n"
+  "   intptr_t rax;\n"
+  "} register_context;\n\n";

This should be in an x86_64-specific subclass, I think?



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:443
+  int64_t operand = op.getRawOperand(0);
+  expr += "   src_addr = " + std::to_string(operand) +
+  ";\n"

This seems unnecessarily slow. Perhaps use an llvm::Twine?



Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:225
+
+  // Copy saved instruction to inferior memory buffer
+  Status error;

`.` at the end :-)



Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp:273
+  mov_buffer[1] = X64_MOV_OPCODE;
+  mov_buffer[2] = 0xE7; // %rsp SIB Bytes
+  std::memcpy(&trampoline_buffer[trampoline_size], &mov_buffer, X64_MOV_SIZE);

We don't typically use end-of-line comments in LLVM and prefer full sentences.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249



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

[Lldb-commits] [PATCH] D66327: [dotest] Make it possible to forward CCC_OVERRIDE_OPTIONS to Make

2019-08-16 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

In D66327#1632684 , @labath wrote:

> How do you envision this to be used? Via something like 
> `CCC_OVERRIDE_OPTIONS=foo ninja check-lldb` ?
>
> Overall, I think it would be nice to avoid the tests being affected by the 
> inherited environment variables because that opens the door for tests to 
> unexpectedly fail or simply behave differently just because someone happens 
> to have some weird environment variable set (and he may not even know about 
> it). I think this is also consistent with the aproach taken in llvm (see e.g. 
> `possibly_dangerous_env_vars` in `llvm/utils/lit/lit/llvm/config.py`). 
> Overall, being able to run the (dotest) test suite with different options 
> would be nice, but I think it would be better if there was a more explicit 
> way to do that (e.g., a command line argument). Incidentally, `dotest` 
> already has a command line argument (`--env`), which seems to be exactly 
> intended for its purpose. Couldn't you just use that?


I considered this, and the reason I didn't go with that approach is because of 
how the builder gets it arguments: they're already passed through the 
environment. Given that `CCC_OVERRIDE_OPTIONS` is special in that it is 
consumed through the environment, I felt like this warranted just forwarding it.

I think passing the `--env` flags to the builder would be good, but it's going 
to be messy unless we move away from passing everything to the builder in the 
environment.

> (Also, instead of CCC_OVERRIDE_OPTIONS, I think I'd try to use CFLAGS_EXTRAS, 
> because that explicitly handled by makefiles, and so it gives an easier way 
> for tests to opt-out of this when they want very specific flags for something)

They're not exactly the same, `CCC_OVERRIDE_OPTIONS` allows you to do cool 
stuff like `'s/-g(lldb)?$/-gdwarf-5/'`. I want to use it on our matrix bot to 
check against different DWARF versions. The old bot was creating a wrapper 
around clang that set the `CCC_OVERRIDE_OPTIONS` and changes out the test 
compiler, but this would be a lot easier.




Comment at: lldb/packages/Python/lldbsuite/test/plugins/builder_base.py:39
 
+def getEnv():
+"""Returns the build environment."""

labath wrote:
> I am confused. How is this function used and what's its relation the the 
> second change below?
I uploaded a slightly outdated patch, the function below should call `getEnv`. 


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66327



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

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision as: JDevlieghere.
JDevlieghere added a comment.

LGTM with the inline comment.




Comment at: lldb/include/lldb/Utility/StringList.h:26
 class StringList {
+  typedef std::vector StorageType;
+

This typedef is commonly `collection` in LLDB [1]. I think we should do the 
same here for consistency. 

[1] See `TypeMap.h`, `Value.h`, `QueueList.h` etc


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

https://reviews.llvm.org/D66345



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

[Lldb-commits] [PATCH] D66327: [dotest] Make it possible to forward CCC_OVERRIDE_OPTIONS to Make

2019-08-16 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added a comment.

You know, the first rule about CCC_OVERRIDE_OPTIONS is that you don't talk 
about CCC_OVERRIDE_OPTIONS ;-)

I suppose this is to implement the new lldb-matrix buildbot that tests various 
DWARF versions?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66327



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

[Lldb-commits] [PATCH] D66327: [dotest] Make it possible to forward CCC_OVERRIDE_OPTIONS to Make

2019-08-16 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D66327#1633285 , @aprantl wrote:

> You know, the first rule about CCC_OVERRIDE_OPTIONS is that you don't talk 
> about CCC_OVERRIDE_OPTIONS ;-)


🤐

> I suppose this is to implement the new lldb-matrix buildbot that tests 
> various DWARF versions?

Yep


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66327



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

[Lldb-commits] [PATCH] D66327: [dotest] Make it possible to forward CCC_OVERRIDE_OPTIONS to Make

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D66327#1633251 , @JDevlieghere 
wrote:

> They're not exactly the same, `CCC_OVERRIDE_OPTIONS` allows you to do cool 
> stuff like `'s/-g(lldb)?$/-gdwarf-5/'`.


That is cool, and, at the same time, scary...

I think it would be nicer if this is passed in a slightly more explicit way 
than via the environment, but if that's too complicated for some reason, then I 
don't think doing this is such a big deal.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66327



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

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

2019-08-16 Thread Guilherme Andrade via Phabricator via lldb-commits 
guiandrade added a comment.

This is a follow up of the investigation I mentioned in 
http://lists.llvm.org/pipermail/lldb-dev/2019-August/015324.html.
Please let me know if you guys think this makes sense. Thanks!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66357



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

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

2019-08-16 Thread Guilherme Andrade via Phabricator via lldb-commits 
guiandrade created this revision.
guiandrade added reviewers: clayborg, labath.
guiandrade added projects: LLDB, clang.
Herald added subscribers: lldb-commits, teemperor.
guiandrade added a comment.

This is a follow up of the investigation I mentioned in 
http://lists.llvm.org/pipermail/lldb-dev/2019-August/015324.html.
Please let me know if you guys think this makes sense. Thanks!


Currently, we return all the entries such that their decl_ctx pointer >= 
decl_ctx provided.
Instead, we should return only the ones that decl_ctx pointer == decl_ctx 
provided.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66357

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2183,9 +2183,10 @@
 std::vector DWARFASTParserClang::GetDIEForDeclContext(
 lldb_private::CompilerDeclContext decl_context) {
   std::vector result;
-  for (auto it = m_decl_ctx_to_die.find(
-   (clang::DeclContext *)decl_context.GetOpaqueDeclContext());
-   it != m_decl_ctx_to_die.end(); it++)
+  auto opaque_decl_ctx =
+  (clang::DeclContext *)decl_context.GetOpaqueDeclContext();
+  for (auto it = m_decl_ctx_to_die.find(opaque_decl_ctx);
+   it != m_decl_ctx_to_die.end() && it->first == opaque_decl_ctx; it++)
 result.push_back(it->second);
   return result;
 }


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2183,9 +2183,10 @@
 std::vector DWARFASTParserClang::GetDIEForDeclContext(
 lldb_private::CompilerDeclContext decl_context) {
   std::vector result;
-  for (auto it = m_decl_ctx_to_die.find(
-   (clang::DeclContext *)decl_context.GetOpaqueDeclContext());
-   it != m_decl_ctx_to_die.end(); it++)
+  auto opaque_decl_ctx =
+  (clang::DeclContext *)decl_context.GetOpaqueDeclContext();
+  for (auto it = m_decl_ctx_to_die.find(opaque_decl_ctx);
+   it != m_decl_ctx_to_die.end() && it->first == opaque_decl_ctx; it++)
 result.push_back(it->second);
   return result;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D66250: [JIT][Unwinder] Add Trampoline ObjectFile and UnwindPlan support for FCB

2019-08-16 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Why are we not just using ObjectFileJIT? I am guessing these breakpoint 
expressions create one of these by compiling the breakpoint expression and 
JIT'ing it just like any other expression. If this is the case, then why do we 
need to create a ObjectFileTrampoline? Seems like we could add .cfi directives 
to the assembly we use for the breakpoint condition function so that we can 
unwind out of it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66250



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

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

2019-08-16 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Needs a test, but looks good.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66357



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

[Lldb-commits] [PATCH] D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex

2019-08-16 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 215645.
JDevlieghere added a comment.

Make RegularExpression POSIX compliant.


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

https://reviews.llvm.org/D66174

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverFileRegex.h
  lldb/include/lldb/Breakpoint/BreakpointResolverName.h
  lldb/include/lldb/Core/AddressResolverName.h
  lldb/include/lldb/Interpreter/OptionValueRegex.h
  lldb/include/lldb/Utility/RegularExpression.h
  lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
  lldb/source/Breakpoint/BreakpointResolverName.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Core/AddressResolverName.cpp
  lldb/source/Core/Disassembler.cpp
  lldb/source/Host/common/Socket.cpp
  lldb/source/Interpreter/CommandObjectRegexCommand.cpp
  lldb/source/Interpreter/OptionArgParser.cpp
  lldb/source/Interpreter/OptionValueRegex.cpp
  lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
  lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Symbol/ObjectFile.cpp
  lldb/source/Symbol/Variable.cpp
  lldb/source/Target/ThreadPlanStepInRange.cpp
  lldb/source/Utility/RegularExpression.cpp
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/NameMatchesTest.cpp
  lldb/unittests/Utility/RegularExpressionTest.cpp

Index: lldb/unittests/Utility/RegularExpressionTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/RegularExpressionTest.cpp
@@ -0,0 +1,65 @@
+//===-- RegularExpressionTest.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/Utility/RegularExpression.h"
+#include "llvm/ADT/SmallVector.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace llvm;
+
+TEST(RegularExpression, Valid) {
+  RegularExpression r1("^[0-9]+$");
+  cantFail(r1.GetError());
+  EXPECT_TRUE(r1.IsValid());
+  EXPECT_EQ("^[0-9]+$", r1.GetText());
+  EXPECT_TRUE(r1.Execute("916"));
+}
+
+TEST(RegularExpression, CopyAssignment) {
+  RegularExpression r1("^[0-9]+$");
+  RegularExpression r2 = r1;
+  cantFail(r2.GetError());
+  EXPECT_TRUE(r2.IsValid());
+  EXPECT_EQ("^[0-9]+$", r2.GetText());
+  EXPECT_TRUE(r2.Execute("916"));
+}
+
+TEST(RegularExpression, Empty) {
+  RegularExpression r1("");
+  Error err = r1.GetError();
+  EXPECT_TRUE(static_cast(err));
+  consumeError(std::move(err));
+  EXPECT_FALSE(r1.IsValid());
+  EXPECT_EQ("", r1.GetText());
+  EXPECT_FALSE(r1.Execute("916"));
+}
+
+TEST(RegularExpression, Invalid) {
+  RegularExpression r1("a[b-");
+  Error err = r1.GetError();
+  EXPECT_TRUE(static_cast(err));
+  consumeError(std::move(err));
+  EXPECT_FALSE(r1.IsValid());
+  EXPECT_EQ("a[b-", r1.GetText());
+  EXPECT_FALSE(r1.Execute("ab"));
+}
+
+TEST(RegularExpression, Match) {
+  RegularExpression r1("[0-9]+([a-f])?:([0-9]+)");
+  cantFail(r1.GetError());
+  EXPECT_TRUE(r1.IsValid());
+  EXPECT_EQ("[0-9]+([a-f])?:([0-9]+)", r1.GetText());
+
+  SmallVector matches;
+  EXPECT_TRUE(r1.Execute("9a:513b", &matches));
+  EXPECT_EQ(3u, matches.size());
+  EXPECT_EQ("9a:513", matches[0].str());
+  EXPECT_EQ("a", matches[1].str());
+  EXPECT_EQ("513", matches[2].str());
+}
Index: lldb/unittests/Utility/NameMatchesTest.cpp
===
--- lldb/unittests/Utility/NameMatchesTest.cpp
+++ lldb/unittests/Utility/NameMatchesTest.cpp
@@ -49,8 +49,8 @@
 TEST(NameMatchesTest, RegularExpression) {
   EXPECT_TRUE(NameMatches("foobar", NameMatch::RegularExpression, "foo"));
   EXPECT_TRUE(NameMatches("foobar", NameMatch::RegularExpression, "f[oa]o"));
-  EXPECT_TRUE(NameMatches("foo", NameMatch::RegularExpression, ""));
-  EXPECT_TRUE(NameMatches("", NameMatch::RegularExpression, ""));
+  EXPECT_FALSE(NameMatches("foo", NameMatch::RegularExpression, ""));
+  EXPECT_FALSE(NameMatches("", NameMatch::RegularExpression, ""));
   EXPECT_FALSE(NameMatches("foo", NameMatch::RegularExpression, "b"));
   EXPECT_FALSE(NameMatches("", NameMatch::RegularExpression, "b"));
   EXPECT_FALSE(NameMatches("^a", NameMatch::RegularExpression, "^a"));
Index: lldb/unittests/Utility/CMakeLists.txt
===
--- lldb/unittests/Utility/CMakeLists.txt
+++ lldb/unittests/Utility/CMakeLists.txt
@@ -21,8 +21,9 @@
   RangeMapTest.cpp
   RangeTest.cpp
   RegisterValueTest.cpp
-  ReproducerTest.cpp

[Lldb-commits] [PATCH] D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex

2019-08-16 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

lgtm. Thanks for doing this.


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

https://reviews.llvm.org/D66174



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

[Lldb-commits] [PATCH] D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex

2019-08-16 Thread Jim Ingham via Phabricator via lldb-commits 
jingham accepted this revision.
jingham added a comment.

Pavel said "we'll just have to bite the bullet and say that our expressions are 
now (more or less) POSIX conformant"...   We should say this somewhere users 
are likely to find.

All the options that take regular expressions look like:

(lldb) help break set
...

  -p  ( --source-pattern-regexp  )

And then for any option value type, you can get help:

(lldb) help regular-expression

   -- A regular expression.

Maybe it would be good to say a little more here?

With that addition I'm fine with this.


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

https://reviews.llvm.org/D66174



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

[Lldb-commits] [lldb] r369153 - [Utility] Reimplement RegularExpression on top of llvm::Regex

2019-08-16 Thread Jonas Devlieghere via lldb-commits 
Author: jdevlieghere
Date: Fri Aug 16 14:25:36 2019
New Revision: 369153

URL: http://llvm.org/viewvc/llvm-project?rev=369153&view=rev
Log:
[Utility] Reimplement RegularExpression on top of llvm::Regex

Originally I wanted to remove the RegularExpression class in Utility and
replace it with llvm::Regex. However, during that transition I noticed
that there are several places where need the regular expression string.
So instead I propose to keep the RegularExpression class and make it a
thin wrapper around llvm::Regex.

This patch also removes the workaround for empty regular expressions.
The result is that we are now (more or less) POSIX conformant.

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

Added:
lldb/trunk/unittests/Utility/RegularExpressionTest.cpp
Modified:
lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileRegex.h
lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h
lldb/trunk/include/lldb/Core/AddressResolverName.h
lldb/trunk/include/lldb/Interpreter/OptionValueRegex.h
lldb/trunk/include/lldb/Utility/RegularExpression.h
lldb/trunk/source/Breakpoint/BreakpointResolverFileRegex.cpp
lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
lldb/trunk/source/Commands/CommandObjectBreakpoint.cpp
lldb/trunk/source/Commands/CommandObjectFrame.cpp
lldb/trunk/source/Core/AddressResolverName.cpp
lldb/trunk/source/Core/Disassembler.cpp
lldb/trunk/source/Host/common/Socket.cpp
lldb/trunk/source/Interpreter/CommandObject.cpp
lldb/trunk/source/Interpreter/CommandObjectRegexCommand.cpp
lldb/trunk/source/Interpreter/OptionArgParser.cpp
lldb/trunk/source/Interpreter/OptionValueRegex.cpp
lldb/trunk/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
lldb/trunk/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

lldb/trunk/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/trunk/source/Symbol/ObjectFile.cpp
lldb/trunk/source/Symbol/Variable.cpp
lldb/trunk/source/Target/ThreadPlanStepInRange.cpp
lldb/trunk/source/Utility/RegularExpression.cpp
lldb/trunk/unittests/Utility/CMakeLists.txt
lldb/trunk/unittests/Utility/NameMatchesTest.cpp

Modified: lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileRegex.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileRegex.h?rev=369153&r1=369152&r2=369153&view=diff
==
--- lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileRegex.h (original)
+++ lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileRegex.h Fri Aug 16 
14:25:36 2019
@@ -24,7 +24,7 @@ namespace lldb_private {
 class BreakpointResolverFileRegex : public BreakpointResolver {
 public:
   BreakpointResolverFileRegex(
-  Breakpoint *bkpt, RegularExpression ®ex,
+  Breakpoint *bkpt, RegularExpression regex,
   const std::unordered_set &func_name_set, bool exact_match);
 
   static BreakpointResolver *

Modified: lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h?rev=369153&r1=369152&r2=369153&view=diff
==
--- lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h (original)
+++ lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h Fri Aug 16 
14:25:36 2019
@@ -44,7 +44,7 @@ public:
 
   // Creates a function breakpoint by regular expression.  Takes over control
   // of the lifespan of func_regex.
-  BreakpointResolverName(Breakpoint *bkpt, RegularExpression &func_regex,
+  BreakpointResolverName(Breakpoint *bkpt, RegularExpression func_regex,
  lldb::LanguageType language, lldb::addr_t offset,
  bool skip_prologue);
 

Modified: lldb/trunk/include/lldb/Core/AddressResolverName.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/AddressResolverName.h?rev=369153&r1=369152&r2=369153&view=diff
==
--- lldb/trunk/include/lldb/Core/AddressResolverName.h (original)
+++ lldb/trunk/include/lldb/Core/AddressResolverName.h Fri Aug 16 14:25:36 2019
@@ -31,7 +31,7 @@ public:
 
   // Creates a function breakpoint by regular expression.  Takes over control
   // of the lifespan of func_regex.
-  AddressResolverName(RegularExpression &func_regex);
+  AddressResolverName(RegularExpression func_regex);
 
   AddressResolverName(const char *class_name, const char *method,
   AddressResolver::MatchType type);

Modified: lldb/trunk/include/lldb/Interpreter/OptionValueRegex.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/

[Lldb-commits] [PATCH] D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex

2019-08-16 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL369153: [Utility] Reimplement RegularExpression on top of 
llvm::Regex (authored by JDevlieghere, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66174?vs=215645&id=215676#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66174

Files:
  lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileRegex.h
  lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h
  lldb/trunk/include/lldb/Core/AddressResolverName.h
  lldb/trunk/include/lldb/Interpreter/OptionValueRegex.h
  lldb/trunk/include/lldb/Utility/RegularExpression.h
  lldb/trunk/source/Breakpoint/BreakpointResolverFileRegex.cpp
  lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
  lldb/trunk/source/Commands/CommandObjectBreakpoint.cpp
  lldb/trunk/source/Commands/CommandObjectFrame.cpp
  lldb/trunk/source/Core/AddressResolverName.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/Socket.cpp
  lldb/trunk/source/Interpreter/CommandObject.cpp
  lldb/trunk/source/Interpreter/CommandObjectRegexCommand.cpp
  lldb/trunk/source/Interpreter/OptionArgParser.cpp
  lldb/trunk/source/Interpreter/OptionValueRegex.cpp
  lldb/trunk/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  
lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/trunk/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
  lldb/trunk/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/trunk/source/Symbol/ObjectFile.cpp
  lldb/trunk/source/Symbol/Variable.cpp
  lldb/trunk/source/Target/ThreadPlanStepInRange.cpp
  lldb/trunk/source/Utility/RegularExpression.cpp
  lldb/trunk/unittests/Utility/CMakeLists.txt
  lldb/trunk/unittests/Utility/NameMatchesTest.cpp
  lldb/trunk/unittests/Utility/RegularExpressionTest.cpp

Index: lldb/trunk/unittests/Utility/RegularExpressionTest.cpp
===
--- lldb/trunk/unittests/Utility/RegularExpressionTest.cpp
+++ lldb/trunk/unittests/Utility/RegularExpressionTest.cpp
@@ -0,0 +1,65 @@
+//===-- RegularExpressionTest.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/Utility/RegularExpression.h"
+#include "llvm/ADT/SmallVector.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace llvm;
+
+TEST(RegularExpression, Valid) {
+  RegularExpression r1("^[0-9]+$");
+  cantFail(r1.GetError());
+  EXPECT_TRUE(r1.IsValid());
+  EXPECT_EQ("^[0-9]+$", r1.GetText());
+  EXPECT_TRUE(r1.Execute("916"));
+}
+
+TEST(RegularExpression, CopyAssignment) {
+  RegularExpression r1("^[0-9]+$");
+  RegularExpression r2 = r1;
+  cantFail(r2.GetError());
+  EXPECT_TRUE(r2.IsValid());
+  EXPECT_EQ("^[0-9]+$", r2.GetText());
+  EXPECT_TRUE(r2.Execute("916"));
+}
+
+TEST(RegularExpression, Empty) {
+  RegularExpression r1("");
+  Error err = r1.GetError();
+  EXPECT_TRUE(static_cast(err));
+  consumeError(std::move(err));
+  EXPECT_FALSE(r1.IsValid());
+  EXPECT_EQ("", r1.GetText());
+  EXPECT_FALSE(r1.Execute("916"));
+}
+
+TEST(RegularExpression, Invalid) {
+  RegularExpression r1("a[b-");
+  Error err = r1.GetError();
+  EXPECT_TRUE(static_cast(err));
+  consumeError(std::move(err));
+  EXPECT_FALSE(r1.IsValid());
+  EXPECT_EQ("a[b-", r1.GetText());
+  EXPECT_FALSE(r1.Execute("ab"));
+}
+
+TEST(RegularExpression, Match) {
+  RegularExpression r1("[0-9]+([a-f])?:([0-9]+)");
+  cantFail(r1.GetError());
+  EXPECT_TRUE(r1.IsValid());
+  EXPECT_EQ("[0-9]+([a-f])?:([0-9]+)", r1.GetText());
+
+  SmallVector matches;
+  EXPECT_TRUE(r1.Execute("9a:513b", &matches));
+  EXPECT_EQ(3u, matches.size());
+  EXPECT_EQ("9a:513", matches[0].str());
+  EXPECT_EQ("a", matches[1].str());
+  EXPECT_EQ("513", matches[2].str());
+}
Index: lldb/trunk/unittests/Utility/CMakeLists.txt
===
--- lldb/trunk/unittests/Utility/CMakeLists.txt
+++ lldb/trunk/unittests/Utility/CMakeLists.txt
@@ -21,8 +21,9 @@
   RangeMapTest.cpp
   RangeTest.cpp
   RegisterValueTest.cpp
-  ReproducerTest.cpp
+  RegularExpressionTest.cpp
   ReproducerInstrumentationTest.cpp
+  ReproducerTest.cpp
   ScalarTest.cpp
   StateTest.cpp
   StatusTest.cpp
Index: lldb/trunk/unittests/Utility/NameMatchesTest.cpp
===
--- lldb/trunk/unittests/Utility/NameMatchesTest.cpp
+++ lldb/trunk/unittests/Utility/NameMatchesTest.c

[Lldb-commits] [PATCH] D66248: [JIT][Command] Add "inject-condition" flag to conditional breakpoints

2019-08-16 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib marked 3 inline comments as done.
mib added inline comments.



Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:120
+  ///
   BreakpointOptions(const char *condition, bool enabled = true,
 int32_t ignore = 0, bool one_shot = false,

shafik wrote:
> You have a lot of `bool` parameters, these are hard to distinguish when 
> calling the function and easy to get mixed up during refactors and subsequent 
> merge conflicts. It would probably be better to combine these `bool` options 
> into a `struct` and then each option has an explicit name that that will be 
> assigned to which makes it explicit which options are being chosen at the 
> call site.
I only added the `bool inject_condition` parameter to the `BreakpointOptions` 
constructor. I also added documentation that was missing for the other 
parameters. I don't think having a struct for those options is a necessary 
since that's what the `BreakpointOptions` class is for. 



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/main.c:30
+printf("local_count = %d\n", local_count++);  // Find the line number of 
condition breakpoint for local_count
+asm("nop");
+asm("nop");

shafik wrote:
> Can you explain why we need the `nop`s injected?
I'm still working on dynamically patching the copied instructions, so I use a 
`nop` sled for now to test my feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66248



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

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

2019-08-16 Thread Adrian McCarthy via Phabricator via lldb-commits 
amccarth added inline comments.



Comment at: lldb/source/Commands/CommandObjectApropos.cpp:70
+max_len = std::max(max_len, command.size());
   }
 

Or

```
const size_t max_len =
std::max_element(commands_found.begin(), commands_found.end());
```



Comment at: lldb/source/Commands/CommandObjectType.cpp:439
+for (const std::string &type_name : options->m_target_types) {
   ConstString const_type_name(type_name);
   if (const_type_name) {

Constructing the const_type_name isn't necessary anymore.  Just change the if 
to:

```
if (!type_name.empty())
```




Comment at: lldb/source/Utility/StringList.cpp:67
-  for (size_t i = 0; i < len; ++i)
-m_strings.push_back(strings.GetStringAtIndex(i));
 }

Optionally add:

```
m_strings.reserve(m_strings.size() + strings.GetSize());
```



Comment at: lldb/unittests/Utility/StringListTest.cpp:515
+
+TEST(StringListTest, ForRangeSingle) {
+  StringList s;

What does "ForRangeSingle" mean here?  The name led me to believe you were 
going to test a 1-element StringList, but I see three elements.


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

https://reviews.llvm.org/D66345



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

[Lldb-commits] [PATCH] D66248: [JIT][Command] Add "inject-condition" flag to conditional breakpoints

2019-08-16 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib marked 2 inline comments as done.
mib added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:45
+
+def enable_injected_breakpoint_condition(self, cli):
+exe = self.getBuildArtifact(self.binary)

JDevlieghere wrote:
> What does `cli` stand for?
CLI = Command Line Interface. In this case, it means running the command 
through the "REPL" instead of the SBAPI, to test if it works properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66248



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

[Lldb-commits] [PATCH] D66370: [dotest] Add a dotest flag `--force-dwarf` to override the tested DWARF version.

2019-08-16 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere created this revision.
JDevlieghere added reviewers: labath, aprantl, jankratochvil.
Herald added a project: LLDB.

On the matrix bot on GreenDragon [1] we want to run the test suite against 
different DWARF versions. The idea here is not to replace targeted tests for 
certain DWARF features, but rather to provide an easy way to support this 
configuration.

[1] http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66370

Files:
  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/packages/Python/lldbsuite/test/lldbtest.py


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1290,6 +1290,8 @@
 
 def getDwarfVersion(self):
 """ Returns the dwarf version generated by clang or '0'. """
+if configuration.dwarf_version:
+return str(configuration.dwarf_version)
 if 'clang' in self.getCompiler():
 try:
 driver_output = check_output(
Index: lldb/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/packages/Python/lldbsuite/test/dotest_args.py
@@ -125,6 +125,12 @@
 dest='out_of_tree_debugserver',
 action='store_true',
 help='A flag to indicate an out-of-tree debug server is being used')
+group.add_argument(
+'--force-dwarf',
+metavar='dwarf_version',
+dest='dwarf_version',
+type=int,
+help='Override the DWARF version.')
 group.add_argument(
 '-s',
 metavar='name',
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -342,9 +342,15 @@
 configuration.skipCategories += test_categories.validate(
 args.skipCategories, False)
 
+cflags_extras = ""
 if args.E:
-cflags_extras = args.E
-os.environ['CFLAGS_EXTRAS'] = cflags_extras
+cflags_extras += args.E
+
+if args.dwarf_version:
+configuration.dwarf_version = args.dwarf_version
+cflags_extras += '-gdwarf-{}'.format(args.dwarf_version)
+
+os.environ['CFLAGS_EXTRAS'] = cflags_extras
 
 if args.d:
 sys.stdout.write(
@@ -364,6 +370,7 @@
 if args.framework:
 configuration.lldbFrameworkPath = args.framework
 
+
 if args.executable:
 # lldb executable is passed explicitly
 lldbtest_config.lldbExec = os.path.realpath(args.executable)
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -194,11 +194,9 @@
 macos_version[0],
 macos_version[1],
 platform.mac_ver()[0])))
-skip_for_dwarf_version = (
- dwarf_version is None) or (
- (self.getDebugInfo() is 'dwarf') and
- _check_expected_version(
- dwarf_version[0], dwarf_version[1], 
self.getDwarfVersion()))
+skip_for_dwarf_version = (dwarf_version is None) or (
+_check_expected_version(dwarf_version[0], dwarf_version[1],
+self.getDwarfVersion()))
 
 # For the test to be skipped, all specified (e.g. not None) parameters 
must be True.
 # An unspecified parameter means "any", so those are marked skip by 
default.  And we skip
Index: lldb/packages/Python/lldbsuite/test/configuration.py
===
--- lldb/packages/Python/lldbsuite/test/configuration.py
+++ lldb/packages/Python/lldbsuite/test/configuration.py
@@ -43,6 +43,9 @@
 arch = None# Must be initialized after option parsing
 compiler = None# Must be initialized after option parsing
 
+# The overriden dwarf verison.
+dwarf_version = 0
+
 # Path to the FileCheck testing tool. Not optional.
 filecheck = None
 


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1290,6 +1290,8 @@
 
 def getDwarfVersion(self):
 """ Returns the dwarf version generated by clang or '0'. """
+if configuration.dwarf_version:
+return str(configuration.dwarf_version)
 if 'clang' in self.g

[Lldb-commits] [PATCH] D66370: [dotest] Add a dotest flag `--force-dwarf` to override the tested DWARF version.

2019-08-16 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added a comment.

This is definitely more elegant than the current solution!




Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:353
+
+os.environ['CFLAGS_EXTRAS'] = cflags_extras
 

Does this actually work with tests whose Makefiles manually set CFLAGS_EXTRAS? 
I.e., do we need to distinguish between extra CFLAGS that come in from the 
layer above and ones that are meant to be customized in the individual test 
Makefiles?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66370



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

[Lldb-commits] [lldb] r369179 - Simplify code (NFC).

2019-08-16 Thread Adrian Prantl via lldb-commits 
Author: adrian
Date: Fri Aug 16 17:38:58 2019
New Revision: 369179

URL: http://llvm.org/viewvc/llvm-project?rev=369179&view=rev
Log:
Simplify code (NFC).

Modified:
lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=369179&r1=369178&r2=369179&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Fri Aug 16 
17:38:58 2019
@@ -2502,32 +2502,29 @@ size_t SymbolFileDWARF::FindTypes(const
   m_index->GetTypes(name, die_offsets);
   const size_t num_die_matches = die_offsets.size();
 
-  if (num_die_matches) {
-size_t num_matches = 0;
-for (size_t i = 0; i < num_die_matches; ++i) {
-  const DIERef &die_ref = die_offsets[i];
-  DWARFDIE die = GetDIE(die_ref);
+  size_t num_matches = 0;
+  for (size_t i = 0; i < num_die_matches; ++i) {
+const DIERef &die_ref = die_offsets[i];
+DWARFDIE die = GetDIE(die_ref);
 
-  if (die) {
-std::vector die_context;
-die.GetDeclContext(die_context);
-if (die_context != context)
-  continue;
+if (die) {
+  std::vector die_context;
+  die.GetDeclContext(die_context);
+  if (die_context != context)
+continue;
 
-Type *matching_type = ResolveType(die, true, true);
-if (matching_type) {
-  // We found a type pointer, now find the shared pointer form our type
-  // list
-  types.InsertUnique(matching_type->shared_from_this());
-  ++num_matches;
-}
-  } else {
-m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
+  Type *matching_type = ResolveType(die, true, true);
+  if (matching_type) {
+// We found a type pointer, now find the shared pointer form our type
+// list
+types.InsertUnique(matching_type->shared_from_this());
+++num_matches;
   }
+} else {
+  m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
 }
-return num_matches;
   }
-  return 0;
+  return num_matches;
 }
 
 CompilerDeclContext


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