[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

2018-07-11 Thread Venkata Ramanaiah via Phabricator via lldb-commits 
ramana-nvr updated this revision to Diff 155114.
ramana-nvr added a comment.

The error messages now refer to the thread ID instead of the thread index.


https://reviews.llvm.org/D48865

Files:
  source/Commands/CommandObjectThread.cpp


Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -1183,8 +1183,8 @@
   thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
   if (frame == nullptr) {
 result.AppendErrorWithFormat(
-"Frame index %u is out of range for thread %u.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+"Frame index %u is out of range for thread id %" PRIu64 ".\n",
+m_options.m_frame_idx, thread->GetID());
 result.SetStatus(eReturnStatusFailed);
 return false;
   }
@@ -1201,9 +1201,8 @@
 
 if (line_table == nullptr) {
   result.AppendErrorWithFormat("Failed to resolve the line table for "
-   "frame %u of thread index %u.\n",
-   m_options.m_frame_idx,
-   m_options.m_thread_idx);
+   "frame %u of thread id %" PRIu64 ".\n",
+   m_options.m_frame_idx, thread->GetID());
   result.SetStatus(eReturnStatusFailed);
   return false;
 }
@@ -1279,13 +1278,13 @@
 new_plan_sp->SetOkayToDiscard(false);
   } else {
 result.AppendErrorWithFormat(
-"Frame index %u of thread %u has no debug information.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+  "Frame index %u of thread id %" PRIu64 " has no debug 
information.\n",
+  m_options.m_frame_idx, thread->GetID());
 result.SetStatus(eReturnStatusFailed);
 return false;
   }
 
-  process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
+  process->GetThreadList().SetSelectedThreadByID(thread->GetID());
 
   StreamString stream;
   Status error;


Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -1183,8 +1183,8 @@
   thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
   if (frame == nullptr) {
 result.AppendErrorWithFormat(
-"Frame index %u is out of range for thread %u.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+"Frame index %u is out of range for thread id %" PRIu64 ".\n",
+m_options.m_frame_idx, thread->GetID());
 result.SetStatus(eReturnStatusFailed);
 return false;
   }
@@ -1201,9 +1201,8 @@
 
 if (line_table == nullptr) {
   result.AppendErrorWithFormat("Failed to resolve the line table for "
-   "frame %u of thread index %u.\n",
-   m_options.m_frame_idx,
-   m_options.m_thread_idx);
+   "frame %u of thread id %" PRIu64 ".\n",
+   m_options.m_frame_idx, thread->GetID());
   result.SetStatus(eReturnStatusFailed);
   return false;
 }
@@ -1279,13 +1278,13 @@
 new_plan_sp->SetOkayToDiscard(false);
   } else {
 result.AppendErrorWithFormat(
-"Frame index %u of thread %u has no debug information.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+  "Frame index %u of thread id %" PRIu64 " has no debug information.\n",
+  m_options.m_frame_idx, thread->GetID());
 result.SetStatus(eReturnStatusFailed);
 return false;
   }
 
-  process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
+  process->GetThreadList().SetSelectedThreadByID(thread->GetID());
 
   StreamString stream;
   Status error;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r336884 - Remove unused variable m_header as it hasn't been used since it was

2018-07-11 Thread Eric Christopher via lldb-commits 
Author: echristo
Date: Wed Jul 11 20:52:45 2018
New Revision: 336884

URL: http://llvm.org/viewvc/llvm-project?rev=336884=rev
Log:
Remove unused variable m_header as it hasn't been used since it was
added in 2016.

Modified:
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=336884=336883=336884=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Wed Jul 11 
20:52:45 2018
@@ -60,14 +60,13 @@ MinidumpParser::Create(const lldb::DataB
 directory->location;
   }
 
-  return MinidumpParser(data_buf_sp, header, std::move(directory_map));
+  return MinidumpParser(data_buf_sp, std::move(directory_map));
 }
 
 MinidumpParser::MinidumpParser(
-const lldb::DataBufferSP _buf_sp, const MinidumpHeader *header,
+const lldb::DataBufferSP _buf_sp,
 llvm::DenseMap &_map)
-: m_data_sp(data_buf_sp), m_header(header), m_directory_map(directory_map) 
{
-}
+: m_data_sp(data_buf_sp), m_directory_map(directory_map) {}
 
 llvm::ArrayRef MinidumpParser::GetData() {
   return llvm::ArrayRef(m_data_sp->GetBytes(),

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h?rev=336884=336883=336884=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h Wed Jul 11 
20:52:45 2018
@@ -91,11 +91,10 @@ public:
 
 private:
   lldb::DataBufferSP m_data_sp;
-  const MinidumpHeader *m_header;
   llvm::DenseMap m_directory_map;
 
   MinidumpParser(
-  const lldb::DataBufferSP _buf_sp, const MinidumpHeader *header,
+  const lldb::DataBufferSP _buf_sp,
   llvm::DenseMap &_map);
 };
 


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

[Lldb-commits] [lldb] r336885 - Remove the unused m_signal member variable, but leave the code that gets it out of the json.

2018-07-11 Thread Eric Christopher via lldb-commits 
Author: echristo
Date: Wed Jul 11 20:52:46 2018
New Revision: 336885

URL: http://llvm.org/viewvc/llvm-project?rev=336885=rev
Log:
Remove the unused m_signal member variable, but leave the code that gets it out 
of the json.

Modified:
lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.cpp
lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.h

Modified: lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.cpp?rev=336885=336884=336885=diff
==
--- lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.cpp (original)
+++ lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.cpp Wed Jul 11 
20:52:46 2018
@@ -58,9 +58,9 @@ support::endianness ProcessInfo::GetEndi
 
 //== ThreadInfo 

 ThreadInfo::ThreadInfo(StringRef name, StringRef reason, RegisterMap registers,
-   unsigned int signal)
+   unsigned int)
 : m_name(name.str()), m_reason(reason.str()),
-  m_registers(std::move(registers)), m_signal(signal) {}
+  m_registers(std::move(registers)) {}
 
 const RegisterValue *ThreadInfo::ReadRegister(unsigned int Id) const {
   auto Iter = m_registers.find(Id);

Modified: lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.h?rev=336885=336884=336885=diff
==
--- lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.h (original)
+++ lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.h Wed Jul 11 
20:52:46 2018
@@ -60,7 +60,6 @@ private:
   std::string m_name;
   std::string m_reason;
   RegisterMap m_registers;
-  unsigned int m_signal;
 };
 
 class JThreadsInfo : public Parser {


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

Re: [Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

2018-07-11 Thread Ramana via lldb-commits 
I do not have commit access to LLDB. Could someone please commit the below
changes?

On Thu, Jul 12, 2018 at 2:32 AM, Jason Molenda via Phabricator <
revi...@reviews.llvm.org> wrote:

> jasonmolenda accepted this revision.
> jasonmolenda added a comment.
> This revision is now accepted and ready to land.
>
> looks good.
>
>
> https://reviews.llvm.org/D48868
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49155: [IRInterpreter] Fix misevaluation of interpretation expressions with `urem`

2018-07-11 Thread Davide Italiano via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336872: [IRInterpreter] Fix misevaluation of interpretation 
expressions with `urem`. (authored by davide, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49155?vs=154884=155096#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49155

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
  lldb/trunk/source/Core/Scalar.cpp


Index: lldb/trunk/source/Core/Scalar.cpp
===
--- lldb/trunk/source/Core/Scalar.cpp
+++ lldb/trunk/source/Core/Scalar.cpp
@@ -1184,38 +1184,38 @@
   case e_void:
 break;
   case e_sint:
+m_type = e_uint;
 success = true;
 break;
   case e_uint:
-m_type = e_uint;
 success = true;
 break;
   case e_slong:
+m_type = e_ulong;
 success = true;
 break;
   case e_ulong:
-m_type = e_ulong;
 success = true;
 break;
   case e_slonglong:
+m_type = e_ulonglong;
 success = true;
 break;
   case e_ulonglong:
-m_type = e_ulonglong;
 success = true;
 break;
   case e_sint128:
+m_type = e_uint128;
 success = true;
 break;
   case e_uint128:
-m_type = e_uint128;
 success = true;
 break;
   case e_sint256:
+m_type = e_uint256;
 success = true;
 break;
   case e_uint256:
-m_type = e_uint256;
 success = true;
 break;
   case e_float:
Index: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
@@ -0,0 +1,3 @@
+LEVEL = ../../make
+C_SOURCES := main.c
+include $(LEVEL)/Makefile.rules
Index: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
@@ -0,0 +1,19 @@
+// Make sure we IR-interpret the expression correctly.
+
+typedef unsigned int uint32_t;
+struct S0 {
+  signed f2;
+};
+static g_463 = 0x1561983AL;
+void func_1(void)
+{
+  struct S0 l_19;
+  l_19.f2 = 419;
+  uint32_t l_4037 = 4294967295UL;
+  l_19.f2 = g_463; //%self.expect("expr ((l_4037 % (-(g_463))) | l_19.f2)", 
substrs=['(unsigned int) $0 = 358717883'])
+}
+int main()
+{
+  func_1();
+  return 0;
+}
Index: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(), None)


Index: lldb/trunk/source/Core/Scalar.cpp
===
--- lldb/trunk/source/Core/Scalar.cpp
+++ lldb/trunk/source/Core/Scalar.cpp
@@ -1184,38 +1184,38 @@
   case e_void:
 break;
   case e_sint:
+m_type = e_uint;
 success = true;
 break;
   case e_uint:
-m_type = e_uint;
 success = true;
 break;
   case e_slong:
+m_type = e_ulong;
 success = true;
 break;
   case e_ulong:
-m_type = e_ulong;
 success = true;
 break;
   case e_slonglong:
+m_type = e_ulonglong;
 success = true;
 break;
   case e_ulonglong:
-m_type = e_ulonglong;
 success = true;
 break;
   case e_sint128:
+m_type = e_uint128;
 success = true;
 break;
   case e_uint128:
-m_type = e_uint128;
 success = true;
 break;
   case e_sint256:
+m_type = e_uint256;
 success = true;
 break;
   case e_uint256:
-m_type = e_uint256;
 success = true;
 break;
   case e_float:
Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
===
--- lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
+++ lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
@@ -0,0 +1,3 @@
+LEVEL = ../../make
+C_SOURCES := main.c
+include $(LEVEL)/Makefile.rules
Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c

[Lldb-commits] [lldb] r336872 - [IRInterpreter] Fix misevaluation of interpretation expressions with `urem`.

2018-07-11 Thread Davide Italiano via lldb-commits 
Author: davide
Date: Wed Jul 11 17:31:04 2018
New Revision: 336872

URL: http://llvm.org/viewvc/llvm-project?rev=336872=rev
Log:
[IRInterpreter] Fix misevaluation of interpretation expressions with `urem`.

Scalar::MakeUnsigned was implemented incorrectly so it didn't
really change the sign of the type (leaving signed types signed).
This showed up as a misevaluation when IR-interpreting urem but
it's likely to arise in other contexts.

This commit fixes the definition, and adds a test to make
sure this won't regress in future (hopefully).

Fixes rdar://problem/42038760 and LLVM PR38076

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

Added:
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/

lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile

lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py

lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
Modified:
lldb/trunk/source/Core/Scalar.cpp

Added: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile?rev=336872=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/Makefile
 Wed Jul 11 17:31:04 2018
@@ -0,0 +1,3 @@
+LEVEL = ../../make
+C_SOURCES := main.c
+include $(LEVEL)/Makefile.rules

Added: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py?rev=336872=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/TestScalarURem.py
 Wed Jul 11 17:31:04 2018
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(), None)

Added: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c?rev=336872=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/rdar42038760/main.c
 Wed Jul 11 17:31:04 2018
@@ -0,0 +1,19 @@
+// Make sure we IR-interpret the expression correctly.
+
+typedef unsigned int uint32_t;
+struct S0 {
+  signed f2;
+};
+static g_463 = 0x1561983AL;
+void func_1(void)
+{
+  struct S0 l_19;
+  l_19.f2 = 419;
+  uint32_t l_4037 = 4294967295UL;
+  l_19.f2 = g_463; //%self.expect("expr ((l_4037 % (-(g_463))) | l_19.f2)", 
substrs=['(unsigned int) $0 = 358717883'])
+}
+int main()
+{
+  func_1();
+  return 0;
+}

Modified: lldb/trunk/source/Core/Scalar.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Scalar.cpp?rev=336872=336871=336872=diff
==
--- lldb/trunk/source/Core/Scalar.cpp (original)
+++ lldb/trunk/source/Core/Scalar.cpp Wed Jul 11 17:31:04 2018
@@ -1184,38 +1184,38 @@ bool Scalar::MakeUnsigned() {
   case e_void:
 break;
   case e_sint:
+m_type = e_uint;
 success = true;
 break;
   case e_uint:
-m_type = e_uint;
 success = true;
 break;
   case e_slong:
+m_type = e_ulong;
 success = true;
 break;
   case e_ulong:
-m_type = e_ulong;
 success = true;
 break;
   case e_slonglong:
+m_type = e_ulonglong;
 success = true;
 break;
   case e_ulonglong:
-m_type = e_ulonglong;
 success = true;
 break;
   case e_sint128:
+m_type = e_uint128;
 success = true;
 break;
   case e_uint128:
-m_type = e_uint128;
 success = true;
 break;
   case e_sint256:
+m_type = e_uint256;
 success = true;
 break;
   case e_uint256:
-m_type = e_uint256;
 success = true;
 break;
   case e_float:


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

[Lldb-commits] [PATCH] D49155: [IRInterpreter] Fix misevaluation of interpretation expressions with `urem`

2018-07-11 Thread Davide Italiano via Phabricator via lldb-commits 
davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

Ack'ed by Jim offline.


https://reviews.llvm.org/D49155



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

[Lldb-commits] [PATCH] D49207: Get rid of the C-string parameter in DoExecute

2018-07-11 Thread Jim Ingham via Phabricator via lldb-commits 
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

I think the CommandObjectRegex check that you took out wasn't something devious 
or clever, but was just an incomplete (and so buggy) version of the check:

if (command && command[0])

that you've been replacing with:

if (command.empty())

everywhere else in the patch.  After all, the error message if you take the 
failing branch was:

  result.AppendError("empty command passed to regular expression command");

So I think instead of taking this line out, you should also convert it to an 
if(command.empty()) and early return it with the error message you removed.

Other than that, this change looks fine.


https://reviews.llvm.org/D49207



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

[Lldb-commits] [PATCH] D49207: Get rid of the C-string parameter in DoExecute

2018-07-11 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor created this revision.
teemperor added a reviewer: davide.

This patch gets rid of the C-string parameter in the 
RawCommandObject::DoExecute function,
making the code simpler and less memory unsafe.

There seems to be a assumption in some command objects that this parameter 
could be a nullptr,
but from what I can see the rest of the API doesn't actually allow this (and 
other command
objects and related code pieces dereference this parameter without any checks).

Especially CommandObjectRegexCommand has error handling code for a nullptr that 
is now gone.


https://reviews.llvm.org/D49207

Files:
  include/lldb/Interpreter/CommandObject.h
  include/lldb/Interpreter/CommandObjectRegexCommand.h
  include/lldb/Interpreter/ScriptInterpreter.h
  source/Commands/CommandObjectCommands.cpp
  source/Commands/CommandObjectExpression.cpp
  source/Commands/CommandObjectExpression.h
  source/Commands/CommandObjectPlatform.cpp
  source/Commands/CommandObjectSettings.cpp
  source/Commands/CommandObjectThread.cpp
  source/Commands/CommandObjectType.cpp
  source/Commands/CommandObjectWatchpoint.cpp
  source/Interpreter/CommandObjectRegexCommand.cpp
  source/Interpreter/CommandObjectScript.cpp
  source/Interpreter/CommandObjectScript.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/ScriptInterpreter/None/ScriptInterpreterNone.cpp
  source/Plugins/ScriptInterpreter/None/ScriptInterpreterNone.h
  source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

Index: source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -151,14 +151,14 @@
   bool Interrupt() override;
 
   bool ExecuteOneLine(
-  const char *command, CommandReturnObject *result,
+  llvm::StringRef command, CommandReturnObject *result,
   const ExecuteScriptOptions  = ExecuteScriptOptions()) override;
 
   void ExecuteInterpreterLoop() override;
 
   bool ExecuteOneLineWithReturn(
-  const char *in_string, ScriptInterpreter::ScriptReturnType return_type,
-  void *ret_value,
+  llvm::StringRef in_string,
+  ScriptInterpreter::ScriptReturnType return_type, void *ret_value,
   const ExecuteScriptOptions  = ExecuteScriptOptions()) override;
 
   lldb_private::Status ExecuteMultipleLines(
@@ -259,18 +259,17 @@
   GetSyntheticTypeName(const StructuredData::ObjectSP ) override;
 
   bool
-  RunScriptBasedCommand(const char *impl_function, const char *args,
+  RunScriptBasedCommand(const char *impl_function, llvm::StringRef args,
 ScriptedCommandSynchronicity synchronicity,
 lldb_private::CommandReturnObject _retobj,
 Status ,
 const lldb_private::ExecutionContext _ctx) override;
 
-  bool
-  RunScriptBasedCommand(StructuredData::GenericSP impl_obj_sp, const char *args,
-ScriptedCommandSynchronicity synchronicity,
-lldb_private::CommandReturnObject _retobj,
-Status ,
-const lldb_private::ExecutionContext _ctx) override;
+  bool RunScriptBasedCommand(
+  StructuredData::GenericSP impl_obj_sp, llvm::StringRef args,
+  ScriptedCommandSynchronicity synchronicity,
+  lldb_private::CommandReturnObject _retobj, Status ,
+  const lldb_private::ExecutionContext _ctx) override;
 
   Status GenerateFunction(const char *signature,
   const StringList ) override;
Index: source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -751,12 +751,12 @@
 }
 
 bool ScriptInterpreterPython::ExecuteOneLine(
-const char *command, CommandReturnObject *result,
+llvm::StringRef command, CommandReturnObject *result,
 const ExecuteScriptOptions ) {
   if (!m_valid_session)
 return false;
 
-  if (command && command[0]) {
+  if (!command.empty()) {
 // We want to call run_one_line, passing in the dictionary and the command
 // string.  We cannot do this through PyRun_SimpleString here because the
 // command string may contain escaped characters, and putting it inside
@@ -894,9 +894,11 @@
   return true;
 
 // The one-liner failed.  Append the error message.
-if (result)
+if (result) {
+  std::string command_str = command.str();
   result->AppendErrorWithFormat(
-  "python failed attempting to evaluate '%s'\n", command);
+  "python failed attempting to evaluate '%s'\n", command_str.c_str());
+}
 return false;
   }

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo updated this revision to Diff 155080.
Herald added a subscriber: mgorny.

https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/CMakeLists.txt
  unittests/Process/minidump/Inputs/bad_duplicate_streams.dmp
  unittests/Process/minidump/Inputs/bad_overlapping_streams.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,32 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+ASSERT_NE(BufferPtr, nullptr);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void InvalidMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+ASSERT_NE(BufferPtr, nullptr);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +84,15 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  InvalidMinidump("linux-x86_64.dmp", 32);
+  InvalidMinidump("linux-x86_64.dmp", 100);
+  InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
+}
 
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, IllFormedMinidumps) {
+  InvalidMinidump("bad_duplicate_streams.dmp", -1);
+  InvalidMinidump("bad_overlapping_streams.dmp", -1);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: unittests/Process/minidump/CMakeLists.txt
===
--- unittests/Process/minidump/CMakeLists.txt
+++ unittests/Process/minidump/CMakeLists.txt
@@ -17,6 +17,8 @@
linux-x86_64.dmp
linux-x86_64_not_crashed.dmp
fizzbuzz_no_heap.dmp
-   fizzbuzz_wow64.dmp)
+   fizzbuzz_wow64.dmp
+   bad_duplicate_streams.dmp
+   bad_overlapping_streams.dmp)
 
 add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,10 +164,29 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
---

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo updated this revision to Diff 155076.
lemo added a comment.

Adding a few ill-formed minidumps for testing the new checks


https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/Inputs/bad_duplicate_streams.dmp
  unittests/Process/minidump/Inputs/bad_overlapping_streams.dmp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,30 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void InvalidMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +82,15 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  InvalidMinidump("linux-x86_64.dmp", 32);
+  InvalidMinidump("linux-x86_64.dmp", 100);
+  InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
+}
 
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, IllFormedMinidumps) {
+  InvalidMinidump("bad_duplicate_streams.dmp", -1);
+  InvalidMinidump("bad_overlapping_streams.dmp", -1);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,10 +164,29 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,14 +89,16 @@
 
   llvm::Optional

[Lldb-commits] [lldb] r336865 - [windows] Fix out-of-memory failure in some of the tests

2018-07-11 Thread Stella Stamenova via lldb-commits 
Author: stella.stamenova
Date: Wed Jul 11 15:47:35 2018
New Revision: 336865

URL: http://llvm.org/viewvc/llvm-project?rev=336865=rev
Log:
[windows] Fix out-of-memory failure in some of the tests

Summary: When ReadProcessMemory fails, bytes_read is sometimes set to a large 
garbage value. In that case, we need to set it back to zero before returning or 
the garbage value will be used to allocate memory later causing LLDB to crash 
with an out of memory error.

Reviewers: asmith, zturner

Reviewed By: zturner

Subscribers: zturner, asmith, stella.stamenova, llvm-commits

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

Modified:
lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

Modified: lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp?rev=336865=336864=336865=diff
==
--- lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp 
(original)
+++ lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp Wed Jul 
11 15:47:35 2018
@@ -240,7 +240,7 @@ Status ProcessWindows::DoLaunch(Module *
   if (working_dir && (!working_dir.ResolvePath() ||
   !fs::is_directory(working_dir.GetPath( {
 result.SetErrorStringWithFormat("No such file or directory: %s",
-   working_dir.GetCString());
+working_dir.GetCString());
 return result;
   }
 
@@ -365,7 +365,10 @@ Status ProcessWindows::DoResume() {
   Status result = thread->DoResume();
   if (result.Fail()) {
 failed = true;
-LLDB_LOG(log, "Trying to resume thread at index {0}, but failed with 
error {1}.", i, result);
+LLDB_LOG(
+log,
+"Trying to resume thread at index {0}, but failed with error {1}.",
+i, result);
   }
 }
 
@@ -473,8 +476,9 @@ void ProcessWindows::RefreshStateAfterSt
   m_session_data->m_debugger->GetActiveException();
   ExceptionRecordSP active_exception = exception_record.lock();
   if (!active_exception) {
-LLDB_LOG(log, "there is no active exception in process {0}.  Why is the "
-  "process stopped?",
+LLDB_LOG(log,
+ "there is no active exception in process {0}.  Why is the "
+ "process stopped?",
  m_session_data->m_debugger->GetProcess().GetProcessId());
 return;
   }
@@ -491,8 +495,9 @@ void ProcessWindows::RefreshStateAfterSt
 const uint64_t pc = register_context->GetPC();
 BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
 if (site && site->ValidForThisThread(stop_thread.get())) {
-  LLDB_LOG(log, "Single-stepped onto a breakpoint in process {0} at "
-"address {1:x} with breakpoint site {2}",
+  LLDB_LOG(log,
+   "Single-stepped onto a breakpoint in process {0} at "
+   "address {1:x} with breakpoint site {2}",
m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
site->GetID());
   stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(*stop_thread,
@@ -514,22 +519,25 @@ void ProcessWindows::RefreshStateAfterSt
 
 BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
 if (site) {
-  LLDB_LOG(log, "detected breakpoint in process {0} at address {1:x} with "
-"breakpoint site {2}",
+  LLDB_LOG(log,
+   "detected breakpoint in process {0} at address {1:x} with "
+   "breakpoint site {2}",
m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
site->GetID());
 
   if (site->ValidForThisThread(stop_thread.get())) {
-LLDB_LOG(log, "Breakpoint site {0} is valid for this thread ({1:x}), "
-  "creating stop info.",
+LLDB_LOG(log,
+ "Breakpoint site {0} is valid for this thread ({1:x}), "
+ "creating stop info.",
  site->GetID(), stop_thread->GetID());
 
 stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(
 *stop_thread, site->GetID());
 register_context->SetPC(pc);
   } else {
-LLDB_LOG(log, "Breakpoint site {0} is not valid for this thread, "
-  "creating empty stop info.",
+LLDB_LOG(log,
+ "Breakpoint site {0} is not valid for this thread, "
+ "creating empty stop info.",
  site->GetID());
   }
   stop_thread->SetStopInfo(stop_info);
@@ -651,8 +659,13 @@ size_t ProcessWindows::DoReadMemory(lldb
   SIZE_T bytes_read = 0;
   if (!ReadProcessMemory(process.GetNativeProcess().GetSystemHandle(), addr,
  buf, size, _read)) {
+// Reading from the process can fail for a number of reasons -

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo added a comment.

Regarding test for the other checks, I'll try to fabricate a few invalid 
minidumps (although it would obviously provide limited coverage)




Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

amccarth wrote:
> lemo wrote:
> > amccarth wrote:
> > > I would rather see this function return the result of the Initialize and 
> > > let the individual tests check the expectation explicitly.
> > > 
> > > I know that will lead to a little bit of duplication in the tests, but it 
> > > will make the individual tests easier to understand in isolation.  It 
> > > also makes it possible for each test to decide whether to ASSERT or 
> > > EXPECT.  And it eliminates the need for the bool parameter which is hard 
> > > to decipher at the call sites.
> > Good idea, although gunit doesn't let us ASSERT in non-void returning 
> > functions.
> > 
> > I agree that the bool parameter is ugly, so I created a separate 
> > TruncateMinidump() helper (which cleans up the SetUpData since the 
> > load_size was only used for truncation)
> This isn't quit what I meant.  I'd rather not have the ASSERTs in helper 
> functions at all (regardless of return type).  The helpers should return a 
> status and the actual test code should do whatever ASSERT or EXPECT is 
> appropriate.
So how would we handle the existing checks in SetupData()?


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Adrian McCarthy via Phabricator via lldb-commits 
amccarth added inline comments.



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();

lemo wrote:
> amccarth wrote:
> > Should the architecture check be in the MinidumpParser::Initialize with the 
> > other checks?
> > 
> > I don't know the answer; I'm just asking for your thinking about this.
> Good question, here's my take: the checks are for consistency and a minidump 
> with a currently unsupported architecture is a valid minidump. 
> 
> So I think it's better to have this check external since the architecture 
> support is not a minidump parser concern. WDYT?
Yes, this makes sense to me.


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Adrian McCarthy via Phabricator via lldb-commits 
amccarth added a comment.

Also, I'm not seeing tests for the other consistency checks you're adding (like 
whether there are any streams or whether the streams overlap).  Is it difficult 
to create such malformed minidumps?




Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

lemo wrote:
> amccarth wrote:
> > I would rather see this function return the result of the Initialize and 
> > let the individual tests check the expectation explicitly.
> > 
> > I know that will lead to a little bit of duplication in the tests, but it 
> > will make the individual tests easier to understand in isolation.  It also 
> > makes it possible for each test to decide whether to ASSERT or EXPECT.  And 
> > it eliminates the need for the bool parameter which is hard to decipher at 
> > the call sites.
> Good idea, although gunit doesn't let us ASSERT in non-void returning 
> functions.
> 
> I agree that the bool parameter is ugly, so I created a separate 
> TruncateMinidump() helper (which cleans up the SetUpData since the load_size 
> was only used for truncation)
This isn't quit what I meant.  I'd rather not have the ASSERTs in helper 
functions at all (regardless of return type).  The helpers should return a 
status and the actual test code should do whatever ASSERT or EXPECT is 
appropriate.


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo updated this revision to Diff 155069.
lemo marked 9 inline comments as done.
lemo added a comment.

Incorporating CR feedback


https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -38,16 +38,30 @@
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
 std::string filename = GetInputFilePath(minidump_filename);
-auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+llvm::Optional optional_parser =
+MinidumpParser::Create(BufferPtr);
+ASSERT_TRUE(optional_parser.hasValue());
+parser.reset(new MinidumpParser(optional_parser.getValue()));
+ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void TruncateMinidump(const char *minidump_filename, uint64_t load_size) {
+std::string filename = GetInputFilePath(minidump_filename);
+auto BufferPtr =
+DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr parser;
@@ -68,12 +82,10 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
-
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  TruncateMinidump("linux-x86_64.dmp", 32);
+  TruncateMinidump("linux-x86_64.dmp", 100);
+  TruncateMinidump("linux-x86_64.dmp", 20 * 1024);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,10 +164,29 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,14 +89,16 @@
 
   llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
 
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
+private:
+  MinidumpParser(const lldb::DataBufferSP _buf_sp);
+
 private:
   lldb::DataBufferSP m_data_sp;
-  const MinidumpHeader *m_header;
+  const

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo added inline comments.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
 #include 
+#include 
+#include 

amccarth wrote:
> Why add ``?  It looks like your new map is just a vector.
Good catch



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:558
+  return result;
+}

amccarth wrote:
> This is a long function (which, unfortunately, is not unusual in LLVM/LLDB), 
> but the comments here are really good, so I'm OK with it.
I agree size-wise it's borderline. It's also a bit smelly in that it does two 
things (checks + initialization).

I originally had a separate "ConsistencyCheck", but there was significant 
duplication (the directory traversal) and the combined version won by a tiny 
margin :) If this grows with more checks I'd be happy to revisit and refactor 
it. 




Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+

amccarth wrote:
> I'm not a big fan of 2-step initialization, but that seems to be the way of 
> LLDB. :-(
Agreed. I don't see how to avoid this w/o much bigger changes though (we don't 
have a chance to return meaningful errors during process creation).



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();

amccarth wrote:
> Should the architecture check be in the MinidumpParser::Initialize with the 
> other checks?
> 
> I don't know the answer; I'm just asking for your thinking about this.
Good question, here's my take: the checks are for consistency and a minidump 
with a currently unsupported architecture is a valid minidump. 

So I think it's better to have this check external since the architecture 
support is not a minidump parser concern. WDYT?



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:193
   if (!pid) {
-error.SetErrorString("failed to parse PID");
+error.SetErrorString("Failed to parse PID");
 return error;

amccarth wrote:
> I realize nothing is perfectly consistent, but I think LLVM tends to start 
> error strings with a lowercase letter (except for proper nouns).  Can you 
> revert this case change and make sure your new error strings follow that 
> pattern?
Sigh, sure. I was hoping it's the other way around.



Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

amccarth wrote:
> I would rather see this function return the result of the Initialize and let 
> the individual tests check the expectation explicitly.
> 
> I know that will lead to a little bit of duplication in the tests, but it 
> will make the individual tests easier to understand in isolation.  It also 
> makes it possible for each test to decide whether to ASSERT or EXPECT.  And 
> it eliminates the need for the bool parameter which is hard to decipher at 
> the call sites.
Good idea, although gunit doesn't let us ASSERT in non-void returning functions.

I agree that the bool parameter is ugly, so I created a separate 
TruncateMinidump() helper (which cleans up the SetUpData since the load_size 
was only used for truncation)


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Adrian McCarthy via Phabricator via lldb-commits 
amccarth added inline comments.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
 #include 
+#include 
+#include 

Why add ``?  It looks like your new map is just a vector.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:367
   // appropriate range linearly each time is stupid.  Perhaps we should build
-  // an index for faster lookups.
+  // an i for faster lookups.
   llvm::Optional range = FindMemoryRange(addr);

This looks like an inadvertent change.  Please change "i" back to "index".



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:454
+Status MinidumpParser::Initialize() {
+  Status result;
+

For consistency, please consider renaming `result` to `error`.



Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:558
+  return result;
+}

This is a long function (which, unfortunately, is not unusual in LLVM/LLDB), 
but the comments here are really good, so I'm OK with it.



Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+

I'm not a big fan of 2-step initialization, but that seems to be the way of 
LLDB. :-(



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();

Should the architecture check be in the MinidumpParser::Initialize with the 
other checks?

I don't know the answer; I'm just asking for your thinking about this.



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:193
   if (!pid) {
-error.SetErrorString("failed to parse PID");
+error.SetErrorString("Failed to parse PID");
 return error;

I realize nothing is perfectly consistent, but I think LLVM tends to start 
error strings with a lowercase letter (except for proper nouns).  Can you 
revert this case change and make sure your new error strings follow that 
pattern?



Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)

I would rather see this function return the result of the Initialize and let 
the individual tests check the expectation explicitly.

I know that will lead to a little bit of duplication in the tests, but it will 
make the individual tests easier to understand in isolation.  It also makes it 
possible for each test to decide whether to ASSERT or EXPECT.  And it 
eliminates the need for the bool parameter which is hard to decipher at the 
call sites.



Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:80
+  SetUpData("linux-x86_64.dmp", 100, true);
+  SetUpData("linux-x86_64.dmp", 20 * 1024, true);
 }

See comment on SetUpData.  As much as practical, I'd rather have the EXPECTs 
and ASSERTs directly in the tests rather than hiding them in a helper function. 
 Also note that, while the first two arguments are pretty intuitive, there's no 
way to understand the true/false in the third argument without going to look up 
to see what SetUpData does.


https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

2018-07-11 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

looks good.


https://reviews.llvm.org/D48868



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

[Lldb-commits] [PATCH] D49192: [FileCheck] Add -allow-deprecated-dag-overlap to failing lldb tests

2018-07-11 Thread Joel E. Denny via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336846: [FileCheck] Add -allow-deprecated-dag-overlap to 
failing lldb tests (authored by jdenny, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49192?vs=155008=155056#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49192

Files:
  lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp


Index: lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp
@@ -7,7 +7,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method 
%t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t 
| \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full 
%t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function 
--function-flags=base %t | \
@@ -21,7 +21,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method 
%t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t 
| \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full 
%t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function 
--function-flags=base %t | \
@@ -36,7 +36,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method 
%t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t 
| \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full 
%t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function 
--function-flags=base %t | \


Index: lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp
@@ -7,7 +7,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
@@ -21,7 +21,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
@@ -36,7 +36,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-11 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo created this revision.
lemo added reviewers: amccarth, labath.
lemo added a project: LLDB.
Herald added a subscriber: mgrang.

Corrupted minidumps was leading to unpredictable behavior.

This change adds explicit consistency checks for the minidump early on. The 
checks are not comprehensive but they should catch obvious structural 
violations:

- streams with type == 0
- duplicate streams (same type)
- overlapping streams
- truncated minidumps

Another early check is to make sure we actually support the minidump 
architecture instead of crashing at a random place deep inside LLDB.


https://reviews.llvm.org/D49202

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/MinidumpTypes.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -39,15 +39,21 @@
 class MinidumpParserTest : public testing::Test {
 public:
   void SetUpData(const char *minidump_filename,
- uint64_t load_size = UINT64_MAX) {
+ uint64_t load_size = UINT64_MAX,
+ bool expect_failure = false) {
 std::string filename = GetInputFilePath(minidump_filename);
 auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
 
 llvm::Optional optional_parser =
 MinidumpParser::Create(BufferPtr);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
 ASSERT_GT(parser->GetData().size(), 0UL);
+auto result = parser->Initialize();
+if (expect_failure)
+  ASSERT_TRUE(result.Fail());
+else
+  ASSERT_TRUE(result.Success()) << result.AsCString();
   }
 
   std::unique_ptr parser;
@@ -68,12 +74,10 @@
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef thread_list;
-
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  SetUpData("linux-x86_64.dmp", 32, true);
+  SetUpData("linux-x86_64.dmp", 100, true);
+  SetUpData("linux-x86_64.dmp", 20 * 1024, true);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -105,7 +105,7 @@
   if (!DataPtr)
 return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef HeaderBytes = DataPtr->GetData();
@@ -164,14 +164,33 @@
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+// supported
+break;
+
+  default:
+error.SetErrorStringWithFormat("Unsupported minidump architecture: %s",
+   arch.GetArchitectureName());
+return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional pid = m_minidump_parser.GetPid();
   if (!pid) {
-error.SetErrorString("failed to parse PID");
+error.SetErrorString("Failed to parse PID");
 return error;
   }
   SetID(pid.getValue());
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -33,9 +33,6 @@
   version != MinidumpHeaderConstants::Version)
 return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
Index: source/Plugins/Process/minidump/MinidumpParser.h
===
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -89,14 +89,16 @@
 
   llvm::Optional GetMemoryRegionInfo(lldb::addr_t);
 
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
+private:
+  MinidumpParser(const lldb::DataBufferSP

[Lldb-commits] [PATCH] D49192: [FileCheck] Add -allow-deprecated-dag-overlap to failing lldb tests

2018-07-11 Thread Paul Robinson via Phabricator via lldb-commits 
probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D49192



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

[Lldb-commits] [PATCH] D48659: Allow specifying an exit code for the 'quit' command

2018-07-11 Thread Raphael Isemann via Phabricator via lldb-commits 
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336824: Allow specifying an exit code for the 
quit command (authored by teemperor, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48659?vs=154318=155030#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48659

Files:
  lldb/trunk/include/lldb/API/SBCommandInterpreter.h
  lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
  lldb/trunk/lit/Quit/TestQuitExitCode-30.test
  lldb/trunk/lit/Quit/TestQuitExitCode0.test
  lldb/trunk/lit/Quit/TestQuitExitCode30.test
  lldb/trunk/lit/Quit/TestQuitExitCodeHex0.test
  lldb/trunk/lit/Quit/TestQuitExitCodeHexA.test
  lldb/trunk/lit/Quit/TestQuitExitCodeImplicit0.test
  lldb/trunk/lit/Quit/TestQuitExitCodeNonInt.test
  lldb/trunk/lit/Quit/TestQuitExitCodeTooManyArgs.test
  lldb/trunk/lit/Quit/expect_exit_code.py
  lldb/trunk/lit/Quit/lit.local.cfg
  lldb/trunk/packages/Python/lldbsuite/test/quit/TestQuit.py
  lldb/trunk/scripts/interface/SBCommandInterpreter.i
  lldb/trunk/source/API/SBCommandInterpreter.cpp
  lldb/trunk/source/Commands/CommandObjectQuit.cpp
  lldb/trunk/source/Interpreter/CommandInterpreter.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Driver.h

Index: lldb/trunk/source/API/SBCommandInterpreter.cpp
===
--- lldb/trunk/source/API/SBCommandInterpreter.cpp
+++ lldb/trunk/source/API/SBCommandInterpreter.cpp
@@ -379,6 +379,23 @@
 m_opaque_ptr->SetPromptOnQuit(b);
 }
 
+void SBCommandInterpreter::AllowExitCodeOnQuit(bool allow) {
+  if (m_opaque_ptr)
+m_opaque_ptr->AllowExitCodeOnQuit(allow);
+}
+
+bool SBCommandInterpreter::HasCustomQuitExitCode() {
+  bool exited = false;
+  if (m_opaque_ptr)
+m_opaque_ptr->GetQuitExitCode(exited);
+  return exited;
+}
+
+int SBCommandInterpreter::GetQuitStatus() {
+  bool exited = false;
+  return (m_opaque_ptr ? m_opaque_ptr->GetQuitExitCode(exited) : 0);
+}
+
 void SBCommandInterpreter::ResolveCommand(const char *command_line,
   SBCommandReturnObject ) {
   result.Clear();
Index: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp
@@ -144,6 +144,26 @@
   m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
 }
 
+void CommandInterpreter::AllowExitCodeOnQuit(bool allow) {
+  m_allow_exit_code = allow;
+  if (!allow)
+m_quit_exit_code.reset();
+}
+
+bool CommandInterpreter::SetQuitExitCode(int exit_code) {
+  if (!m_allow_exit_code)
+return false;
+  m_quit_exit_code = exit_code;
+  return true;
+}
+
+int CommandInterpreter::GetQuitExitCode(bool ) const {
+  exited = m_quit_exit_code.hasValue();
+  if (exited)
+return *m_quit_exit_code;
+  return 0;
+}
+
 void CommandInterpreter::ResolveCommand(const char *command_line,
 CommandReturnObject ) {
   std::string command = command_line;
Index: lldb/trunk/source/Commands/CommandObjectQuit.cpp
===
--- lldb/trunk/source/Commands/CommandObjectQuit.cpp
+++ lldb/trunk/source/Commands/CommandObjectQuit.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Target/Process.h"
+#include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -26,7 +27,7 @@
 
 CommandObjectQuit::CommandObjectQuit(CommandInterpreter )
 : CommandObjectParsed(interpreter, "quit", "Quit the LLDB debugger.",
-  "quit") {}
+  "quit [exit-code]") {}
 
 CommandObjectQuit::~CommandObjectQuit() {}
 
@@ -77,6 +78,41 @@
   return false;
 }
   }
+
+  if (command.GetArgumentCount() > 1) {
+result.AppendError("Too many arguments for 'quit'. Only an optional exit "
+   "code is allowed");
+result.SetStatus(eReturnStatusFailed);
+return false;
+  }
+
+  if (command.GetArgumentCount() > 1) {
+result.AppendError("Too many arguments for 'quit'. Only an optional exit "
+   "code is allowed");
+result.SetStatus(eReturnStatusFailed);
+return false;
+  }
+
+  // We parse the exit code argument if there is one.
+  if (command.GetArgumentCount() == 1) {
+llvm::StringRef arg = command.GetArgumentAtIndex(0);
+int exit_code;
+if (arg.getAsInteger(/*autodetect radix*/ 0, exit_code)) {
+  lldb_private::StreamString s;
+  std::string arg_str = arg.str();
+  s.Printf("Couldn't parse '%s' as integer for exit code.", arg_str.data());
+  result.AppendError(s.GetString());
+

[Lldb-commits] [lldb] r336824 - Allow specifying an exit code for the 'quit' command

2018-07-11 Thread Raphael Isemann via lldb-commits 
Author: teemperor
Date: Wed Jul 11 10:18:01 2018
New Revision: 336824

URL: http://llvm.org/viewvc/llvm-project?rev=336824=rev
Log:
Allow specifying an exit code for the 'quit' command

Summary:
This patch adds the possibility to specify an exit code when calling quit.
We accept any int, even though it depends on the user what happens if the int is
out of the range of what the operating system supports as exit codes.

Fixes rdar://problem/38452312

Reviewers: davide, jingham, clayborg

Reviewed By: jingham

Subscribers: clayborg, jingham, lldb-commits

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

Added:
lldb/trunk/lit/Quit/
lldb/trunk/lit/Quit/TestQuitExitCode-30.test
lldb/trunk/lit/Quit/TestQuitExitCode0.test
lldb/trunk/lit/Quit/TestQuitExitCode30.test
lldb/trunk/lit/Quit/TestQuitExitCodeHex0.test
lldb/trunk/lit/Quit/TestQuitExitCodeHexA.test
lldb/trunk/lit/Quit/TestQuitExitCodeImplicit0.test
lldb/trunk/lit/Quit/TestQuitExitCodeNonInt.test
lldb/trunk/lit/Quit/TestQuitExitCodeTooManyArgs.test
lldb/trunk/lit/Quit/expect_exit_code.py   (with props)
lldb/trunk/lit/Quit/lit.local.cfg
lldb/trunk/packages/Python/lldbsuite/test/quit/
lldb/trunk/packages/Python/lldbsuite/test/quit/TestQuit.py
Modified:
lldb/trunk/include/lldb/API/SBCommandInterpreter.h
lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
lldb/trunk/scripts/interface/SBCommandInterpreter.i
lldb/trunk/source/API/SBCommandInterpreter.cpp
lldb/trunk/source/Commands/CommandObjectQuit.cpp
lldb/trunk/source/Interpreter/CommandInterpreter.cpp
lldb/trunk/tools/driver/Driver.cpp
lldb/trunk/tools/driver/Driver.h

Modified: lldb/trunk/include/lldb/API/SBCommandInterpreter.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/API/SBCommandInterpreter.h?rev=336824=336823=336824=diff
==
--- lldb/trunk/include/lldb/API/SBCommandInterpreter.h (original)
+++ lldb/trunk/include/lldb/API/SBCommandInterpreter.h Wed Jul 11 10:18:01 2018
@@ -208,6 +208,25 @@ public:
   void SetPromptOnQuit(bool b);
 
   //--
+  /// Sets whether the command interpreter should allow custom exit codes
+  /// for the 'quit' command.
+  //--
+  void AllowExitCodeOnQuit(bool allow);
+
+  //--
+  /// Returns true if the user has called the 'quit' command with a custom exit
+  /// code.
+  //--
+  bool HasCustomQuitExitCode();
+
+  //--
+  /// Returns the exit code that the user has specified when running the
+  /// 'quit' command. Returns 0 if the user hasn't called 'quit' at all or
+  /// without a custom exit code.
+  //--
+  int GetQuitStatus();
+
+  //--
   /// Resolve the command just as HandleCommand would, expanding abbreviations
   /// and aliases.  If successful, result->GetOutput has the full expansion.
   //--

Modified: lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h?rev=336824=336823=336824=diff
==
--- lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h (original)
+++ lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h Wed Jul 11 
10:18:01 2018
@@ -455,6 +455,30 @@ public:
 
   void SetPromptOnQuit(bool b);
 
+  //--
+  /// Specify if the command interpreter should allow that the user can
+  /// specify a custom exit code when calling 'quit'.
+  //--
+  void AllowExitCodeOnQuit(bool allow);
+
+  //--
+  /// Sets the exit code for the quit command.
+  /// @param[in] exit_code
+  /// The exit code that the driver should return on exit.
+  /// @return True if the exit code was successfully set; false if the
+  /// interpreter doesn't allow custom exit codes.
+  /// @see AllowExitCodeOnQuit
+  //--
+  LLVM_NODISCARD bool SetQuitExitCode(int exit_code);
+
+  //--
+  /// Returns the exit code that the user has specified when running the
+  /// 'quit' command.
+  /// @param[out] exited
+  /// Set to true if the user has called quit with a custom

[Lldb-commits] [PATCH] D49192: [FileCheck] Add -allow-deprecated-dag-overlap to failing lldb tests

2018-07-11 Thread Joel E. Denny via Phabricator via lldb-commits 
jdenny created this revision.
jdenny added a reviewer: probinson.
Herald added a subscriber: JDevlieghere.

See https://reviews.llvm.org/D47106 for details.


https://reviews.llvm.org/D49192

Files:
  lldb/lit/SymbolFile/DWARF/find-basic-function.cpp


Index: lldb/lit/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/lit/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/lit/SymbolFile/DWARF/find-basic-function.cpp
@@ -7,7 +7,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method 
%t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t 
| \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full 
%t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function 
--function-flags=base %t | \
@@ -21,7 +21,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method 
%t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t 
| \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full 
%t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function 
--function-flags=base %t | \
@@ -36,7 +36,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method 
%t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t 
| \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full 
%t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function 
--function-flags=base %t | \


Index: lldb/lit/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/lit/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/lit/SymbolFile/DWARF/find-basic-function.cpp
@@ -7,7 +7,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
@@ -21,7 +21,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
@@ -36,7 +36,7 @@
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
-// RUN:   FileCheck --check-prefix=FULL %s
+// RUN:   FileCheck -allow-deprecated-dag-overlap --check-prefix=FULL %s
 // RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

2018-07-11 Thread Venkata Ramanaiah via Phabricator via lldb-commits 
ramana-nvr updated this revision to Diff 154947.
ramana-nvr added a comment.

For now, leaving the m_thread_pcs.clear() in UpdateThreadIDList() unchanged as 
it is not causing any problems.


https://reviews.llvm.org/D48868

Files:
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp


Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
 size_t
 ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string ) 
{
   m_thread_ids.clear();
-  m_thread_pcs.clear();
   size_t comma_pos;
   lldb::tid_t tid;
   while ((comma_pos = value.find(',')) != std::string::npos) {


Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
 size_t
 ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string ) {
   m_thread_ids.clear();
-  m_thread_pcs.clear();
   size_t comma_pos;
   lldb::tid_t tid;
   while ((comma_pos = value.find(',')) != std::string::npos) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits