[Lldb-commits] [PATCH] D26676: Patch for lldb bug 26322 “core load hangs”

[Howard Hellyer via lldb-commits]

hhellyer updated this revision to Diff 78524.
hhellyer added a comment.

Updating the patch to include the test cases and scripts to produce the core 
dumps.
I haven't included the core dumps and when I do I'll need to update the pid and
tid values in the test scripts. That can wait until we've decided what to do 
about
the core dumps though.


https://reviews.llvm.org/D26676

Files:
  packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/TestGCore.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/main.cpp
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/main.mk
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/make-core.sh
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/main.cpp
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/main.mk
  
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/make-core.sh
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/elf-core/ThreadElfCore.cpp
  source/Plugins/Process/elf-core/ThreadElfCore.h

Index: source/Plugins/Process/elf-core/ThreadElfCore.h
===
--- source/Plugins/Process/elf-core/ThreadElfCore.h
+++ source/Plugins/Process/elf-core/ThreadElfCore.h
@@ -82,6 +82,38 @@
 static_assert(sizeof(ELFLinuxPrStatus) == 112,
   "sizeof ELFLinuxPrStatus is not correct!");
 
+struct ELFLinuxSigInfo {
+  int32_t si_signo;
+  int32_t si_code;
+  int32_t si_errno;
+
+  ELFLinuxSigInfo();
+
+  lldb_private::Error Parse(lldb_private::DataExtractor &data,
+const lldb_private::ArchSpec &arch);
+
+  // Return the bytesize of the structure
+  // 64 bit - just sizeof
+  // 32 bit - hardcoded because we are reusing the struct, but some of the
+  // members are smaller -
+  // so the layout is not the same
+  static size_t GetSize(const lldb_private::ArchSpec &arch) {
+switch (arch.GetCore()) {
+case lldb_private::ArchSpec::eCore_x86_64_x86_64:
+  return sizeof(ELFLinuxSigInfo);
+case lldb_private::ArchSpec::eCore_s390x_generic:
+case lldb_private::ArchSpec::eCore_x86_32_i386:
+case lldb_private::ArchSpec::eCore_x86_32_i486:
+  return 12;
+default:
+  return 0;
+}
+  }
+};
+
+static_assert(sizeof(ELFLinuxSigInfo) == 12,
+  "sizeof ELFLinuxSigInfo is not correct!");
+
 // PRPSINFO structure's size differs based on architecture.
 // This is the layout in the x86-64 arch case.
 // In the i386 case we parse it manually and fill it again
@@ -133,7 +165,8 @@
   lldb_private::DataExtractor fpregset;
   lldb_private::DataExtractor vregset;
   lldb::tid_t tid;
-  int signo;
+  int signo = 0;
+  int prstatus_sig = 0;
   std::string name;
 };
 
Index: source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -320,3 +320,45 @@
 
   return error;
 }
+
+//
+// Parse SIGINFO from NOTE entry
+//
+ELFLinuxSigInfo::ELFLinuxSigInfo() {
+  memset(this, 0, sizeof(ELFLinuxSigInfo));
+}
+
+Error ELFLinuxSigInfo::Parse(DataExtractor &data, const ArchSpec &arch) {
+  Error error;
+  ByteOrder byteorder = data.GetByteOrder();
+  if (GetSize(arch) > data.GetByteSize()) {
+error.SetErrorStringWithFormat(
+"NT_SIGINFO size should be %zu, but the remaining bytes are: %" PRIu64,
+GetSize(arch), data.GetByteSize());
+return error;
+  }
+
+  switch (arch.GetCore()) {
+  case ArchSpec::eCore_x86_64_x86_64:
+data.ExtractBytes(0, sizeof(ELFLinuxPrStatus), byteorder, this);
+break;
+  case ArchSpec::eCore_s390x_generic:
+  case ArchSpec::eCore_x86_32_i386:
+  case ArchSpec::eCore_x86_32_i486: {
+// Parsing from a 32 bit ELF core file, and populating/reusing the structure
+// properly, because the struct is for the 64 bit version
+offset_t offset = 0;
+si_signo = data.GetU32(&offset);
+si_code = data.GetU32(&offset);
+si_errno = data.GetU32(&offset);
+
+break;
+  }
+  default:
+error.SetErrorStringWithFormat("ELFLinuxSigInfo::%s Unknown architecture",
+   __FUNCTION__);
+break;
+  }
+
+  return error;
+}
Index: source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ source/Plugins

[Lldb-commits] [PATCH] D26676: Patch for lldb bug 26322 “core load hangs”

[Pavel Labath via lldb-commits]

labath added a comment.

We probably don't want to check in zillions of core files, but I believe having 
a couple of them is fine, particularly when care is taken to minimize their 
size. I believe the benefits (being able to run tests on a piece of code in a 
reproducible and platform-independent manner) outweigh the downsides. And I 
don't expect these files to change every week (probably more like never), so 
they won't bloat the history. Compare that with a 7MB python reference html 
file we currently store in the repo, which we should update (but we don't) on 
every API change.


https://reviews.llvm.org/D26676



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

[Lldb-commits] [lldb] r287354 - Resubmit "Remove an output-parameter from Variable function".

[Zachary Turner via lldb-commits]

Author: zturner
Date: Fri Nov 18 11:55:04 2016
New Revision: 287354

URL: http://llvm.org/viewvc/llvm-project?rev=287354&view=rev
Log:
Resubmit "Remove an output-parameter from Variable function".

The scanning algorithm had a few little subtleties that I
overlooked, but this patch should fix everything.

I still haven't changed the function to take a StringRef since
that has some trickle down effect and is mostly mechanical,
I just wanted to get the tricky part as isolated as possible.

Modified:
lldb/trunk/include/lldb/Core/ValueObject.h
lldb/trunk/source/Core/FormatEntity.cpp
lldb/trunk/source/Core/ValueObject.cpp
lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
lldb/trunk/source/Symbol/Variable.cpp

Modified: lldb/trunk/include/lldb/Core/ValueObject.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ValueObject.h?rev=287354&r1=287353&r2=287354&view=diff
==
--- lldb/trunk/include/lldb/Core/ValueObject.h (original)
+++ lldb/trunk/include/lldb/Core/ValueObject.h Fri Nov 18 11:55:04 2016
@@ -394,7 +394,7 @@ public:
   GetExpressionPathFormat = eGetExpressionPathFormatDereferencePointers);
 
   lldb::ValueObjectSP GetValueForExpressionPath(
-  const char *expression, const char **first_unparsed = nullptr,
+  const char *expression,
   ExpressionPathScanEndReason *reason_to_stop = nullptr,
   ExpressionPathEndResultType *final_value_type = nullptr,
   const GetValueForExpressionPathOptions &options =
@@ -1002,8 +1002,7 @@ private:
   virtual CompilerType MaybeCalculateCompleteType();
 
   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
-  const char *expression_cstr, const char **first_unparsed,
-  ExpressionPathScanEndReason *reason_to_stop,
+  const char *expression_cstr, ExpressionPathScanEndReason *reason_to_stop,
   ExpressionPathEndResultType *final_value_type,
   const GetValueForExpressionPathOptions &options,
   ExpressionPathAftermath *final_task_on_target);

Modified: lldb/trunk/source/Core/FormatEntity.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/FormatEntity.cpp?rev=287354&r1=287353&r2=287354&view=diff
==
--- lldb/trunk/source/Core/FormatEntity.cpp (original)
+++ lldb/trunk/source/Core/FormatEntity.cpp Fri Nov 18 11:55:04 2016
@@ -614,7 +614,6 @@ static ValueObjectSP ExpandIndexedExpres
   if (log)
 log->Printf("[ExpandIndexedExpression] name to deref: %s",
 ptr_deref_buffer.c_str());
-  const char *first_unparsed;
   ValueObject::GetValueForExpressionPathOptions options;
   ValueObject::ExpressionPathEndResultType final_value_type;
   ValueObject::ExpressionPathScanEndReason reason_to_stop;
@@ -622,20 +621,18 @@ static ValueObjectSP ExpandIndexedExpres
   (deref_pointer ? ValueObject::eExpressionPathAftermathDereference
  : ValueObject::eExpressionPathAftermathNothing);
   ValueObjectSP item = valobj->GetValueForExpressionPath(
-  ptr_deref_buffer.c_str(), &first_unparsed, &reason_to_stop,
-  &final_value_type, options, &what_next);
+  ptr_deref_buffer.c_str(), &reason_to_stop, &final_value_type, options,
+  &what_next);
   if (!item) {
 if (log)
-  log->Printf("[ExpandIndexedExpression] ERROR: unparsed portion = %s, why 
"
-  "stopping = %d,"
+  log->Printf("[ExpandIndexedExpression] ERROR: why stopping = %d,"
   " final_value_type %d",
-  first_unparsed, reason_to_stop, final_value_type);
+  reason_to_stop, final_value_type);
   } else {
 if (log)
-  log->Printf("[ExpandIndexedExpression] ALL RIGHT: unparsed portion = %s, 
"
-  "why stopping = %d,"
+  log->Printf("[ExpandIndexedExpression] ALL RIGHT: why stopping = %d,"
   " final_value_type %d",
-  first_unparsed, reason_to_stop, final_value_type);
+  reason_to_stop, final_value_type);
   }
   return item;
 }
@@ -724,7 +721,6 @@ static bool DumpValue(Stream &s, const S
   int64_t index_lower = -1;
   int64_t index_higher = -1;
   bool is_array_range = false;
-  const char *first_unparsed;
   bool was_plain_var = false;
   bool was_var_format = false;
   bool was_var_indexed = false;
@@ -762,25 +758,23 @@ static bool DumpValue(Stream &s, const S
   log->Printf("[Debugger::FormatPrompt] symbol to expand: %s",
   expr_path.c_str());
 
-target = valobj
- ->GetValueForExpressionPath(expr_path.c_str(), 
&first_unparsed,
- &reason_to_stop, 
&final_value_type,
- options, &what_next)
- .get();
+target =
+valobj
+->GetValueForExpressionPath(expr_path.c_str(), &reason_to_stop,
+  

[Lldb-commits] [lldb] r287367 - Re-add the StringRef interface changes for Variable.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Fri Nov 18 13:23:39 2016
New Revision: 287367

URL: http://llvm.org/viewvc/llvm-project?rev=287367&view=rev
Log:
Re-add the StringRef interface changes for Variable.

This concludes the changes I originally tried to make and then
had to back out.  This way if anything is still broken, it
should be easier to bisect it back to a more specific changeset.

Modified:
lldb/trunk/include/lldb/Core/ValueObject.h
lldb/trunk/include/lldb/Symbol/Variable.h
lldb/trunk/source/Core/ValueObject.cpp
lldb/trunk/source/Symbol/Variable.cpp

Modified: lldb/trunk/include/lldb/Core/ValueObject.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ValueObject.h?rev=287367&r1=287366&r2=287367&view=diff
==
--- lldb/trunk/include/lldb/Core/ValueObject.h (original)
+++ lldb/trunk/include/lldb/Core/ValueObject.h Fri Nov 18 13:23:39 2016
@@ -394,7 +394,7 @@ public:
   GetExpressionPathFormat = eGetExpressionPathFormatDereferencePointers);
 
   lldb::ValueObjectSP GetValueForExpressionPath(
-  const char *expression,
+  llvm::StringRef expression,
   ExpressionPathScanEndReason *reason_to_stop = nullptr,
   ExpressionPathEndResultType *final_value_type = nullptr,
   const GetValueForExpressionPathOptions &options =
@@ -1002,7 +1002,8 @@ private:
   virtual CompilerType MaybeCalculateCompleteType();
 
   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
-  const char *expression_cstr, ExpressionPathScanEndReason *reason_to_stop,
+  llvm::StringRef expression_cstr,
+  ExpressionPathScanEndReason *reason_to_stop,
   ExpressionPathEndResultType *final_value_type,
   const GetValueForExpressionPathOptions &options,
   ExpressionPathAftermath *final_task_on_target);

Modified: lldb/trunk/include/lldb/Symbol/Variable.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/Variable.h?rev=287367&r1=287366&r2=287367&view=diff
==
--- lldb/trunk/include/lldb/Symbol/Variable.h (original)
+++ lldb/trunk/include/lldb/Symbol/Variable.h Fri Nov 18 13:23:39 2016
@@ -98,7 +98,7 @@ public:
 VariableList &var_list);
 
   static Error GetValuesForVariableExpressionPath(
-  const char *variable_expr_path, ExecutionContextScope *scope,
+  llvm::StringRef variable_expr_path, ExecutionContextScope *scope,
   GetVariableCallback callback, void *baton, VariableList &variable_list,
   ValueObjectList &valobj_list);
 

Modified: lldb/trunk/source/Core/ValueObject.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/ValueObject.cpp?rev=287367&r1=287366&r2=287367&view=diff
==
--- lldb/trunk/source/Core/ValueObject.cpp (original)
+++ lldb/trunk/source/Core/ValueObject.cpp Fri Nov 18 13:23:39 2016
@@ -2166,7 +2166,7 @@ void ValueObject::GetExpressionPath(Stre
 }
 
 ValueObjectSP ValueObject::GetValueForExpressionPath(
-const char *expression, ExpressionPathScanEndReason *reason_to_stop,
+llvm::StringRef expression, ExpressionPathScanEndReason *reason_to_stop,
 ExpressionPathEndResultType *final_value_type,
 const GetValueForExpressionPathOptions &options,
 ExpressionPathAftermath *final_task_on_target) {
@@ -2234,16 +2234,16 @@ ValueObjectSP ValueObject::GetValueForEx
 }
 
 ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
-const char *expression_cstr2, ExpressionPathScanEndReason *reason_to_stop,
+llvm::StringRef expression, ExpressionPathScanEndReason *reason_to_stop,
 ExpressionPathEndResultType *final_result,
 const GetValueForExpressionPathOptions &options,
 ExpressionPathAftermath *what_next) {
   ValueObjectSP root = GetSP();
 
-  if (!root.get())
-return ValueObjectSP();
+  if (!root)
+return nullptr;
 
-  llvm::StringRef remainder(expression_cstr2);
+  llvm::StringRef remainder = expression;
 
   while (true) {
 llvm::StringRef temp_expression = remainder;
@@ -2565,7 +2565,7 @@ ValueObjectSP ValueObject::GetValueForEx
   pointee_compiler_type_info.Test(eTypeIsScalar)) {
 Error error;
 root = root->Dereference(error);
-if (error.Fail() || !root.get()) {
+if (error.Fail() || !root) {
   *reason_to_stop =
   ValueObject::eExpressionPathScanEndReasonDereferencingFailed;
   *final_result = ValueObject::eExpressionPathEndResultTypeInvalid;
@@ -2588,7 +2588,7 @@ ValueObjectSP ValueObject::GetValueForEx
   root = root->GetSyntheticValue()->GetChildAtIndex(index, true);
 } else
   root = root->GetSyntheticArrayMember(index, true);
-if (!root.get()) {
+if (!root) {
   *reason_to_stop =
   ValueObject::eExpre

Re: [Lldb-commits] [PATCH] D26676: Patch for lldb bug 26322 “core load hangs”

[Jim Ingham via lldb-commits]

If we are going to do that, I wonder if we should start having tests share 
their inputs.  We don’t do that for source files because the cost of 
duplication is so small, and then you don’t have the hassle of adding something 
to a source fie for test A messes up test B.  But for binaries like core files, 
it might be worth that hassle.  Especially as we need to pick up core files for 
a bunch of different architectures.

Jim

> On Nov 18, 2016, at 8:42 AM, Pavel Labath  wrote:
> 
> labath added a comment.
> 
> We probably don't want to check in zillions of core files, but I believe 
> having a couple of them is fine, particularly when care is taken to minimize 
> their size. I believe the benefits (being able to run tests on a piece of 
> code in a reproducible and platform-independent manner) outweigh the 
> downsides. And I don't expect these files to change every week (probably more 
> like never), so they won't bloat the history. Compare that with a 7MB python 
> reference html file we currently store in the repo, which we should update 
> (but we don't) on every API change.
> 
> 
> https://reviews.llvm.org/D26676
> 
> 
> 

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

Re: [Lldb-commits] [lldb] r287354 - Resubmit "Remove an output-parameter from Variable function".

[Jim Ingham via lldb-commits]

I didn’t see this patch go up for review.  The parameter you removed might have 
been vestigial or might have been one that the owner of this code was planning 
to do something with.  Anyway, this seems to go beyond purely formal changes 
and so should have been discussed.  My apologies if I just missed the review.

Jim

> On Nov 18, 2016, at 9:55 AM, Zachary Turner via lldb-commits 
>  wrote:
> 
> Author: zturner
> Date: Fri Nov 18 11:55:04 2016
> New Revision: 287354
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=287354&view=rev
> Log:
> Resubmit "Remove an output-parameter from Variable function".
> 
> The scanning algorithm had a few little subtleties that I
> overlooked, but this patch should fix everything.
> 
> I still haven't changed the function to take a StringRef since
> that has some trickle down effect and is mostly mechanical,
> I just wanted to get the tricky part as isolated as possible.
> 
> Modified:
>lldb/trunk/include/lldb/Core/ValueObject.h
>lldb/trunk/source/Core/FormatEntity.cpp
>lldb/trunk/source/Core/ValueObject.cpp
>lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
>lldb/trunk/source/Symbol/Variable.cpp
> 
> Modified: lldb/trunk/include/lldb/Core/ValueObject.h
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ValueObject.h?rev=287354&r1=287353&r2=287354&view=diff
> ==
> --- lldb/trunk/include/lldb/Core/ValueObject.h (original)
> +++ lldb/trunk/include/lldb/Core/ValueObject.h Fri Nov 18 11:55:04 2016
> @@ -394,7 +394,7 @@ public:
>   GetExpressionPathFormat = eGetExpressionPathFormatDereferencePointers);
> 
>   lldb::ValueObjectSP GetValueForExpressionPath(
> -  const char *expression, const char **first_unparsed = nullptr,
> +  const char *expression,
>   ExpressionPathScanEndReason *reason_to_stop = nullptr,
>   ExpressionPathEndResultType *final_value_type = nullptr,
>   const GetValueForExpressionPathOptions &options =
> @@ -1002,8 +1002,7 @@ private:
>   virtual CompilerType MaybeCalculateCompleteType();
> 
>   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
> -  const char *expression_cstr, const char **first_unparsed,
> -  ExpressionPathScanEndReason *reason_to_stop,
> +  const char *expression_cstr, ExpressionPathScanEndReason 
> *reason_to_stop,
>   ExpressionPathEndResultType *final_value_type,
>   const GetValueForExpressionPathOptions &options,
>   ExpressionPathAftermath *final_task_on_target);
> 
> Modified: lldb/trunk/source/Core/FormatEntity.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/FormatEntity.cpp?rev=287354&r1=287353&r2=287354&view=diff
> ==
> --- lldb/trunk/source/Core/FormatEntity.cpp (original)
> +++ lldb/trunk/source/Core/FormatEntity.cpp Fri Nov 18 11:55:04 2016
> @@ -614,7 +614,6 @@ static ValueObjectSP ExpandIndexedExpres
>   if (log)
> log->Printf("[ExpandIndexedExpression] name to deref: %s",
> ptr_deref_buffer.c_str());
> -  const char *first_unparsed;
>   ValueObject::GetValueForExpressionPathOptions options;
>   ValueObject::ExpressionPathEndResultType final_value_type;
>   ValueObject::ExpressionPathScanEndReason reason_to_stop;
> @@ -622,20 +621,18 @@ static ValueObjectSP ExpandIndexedExpres
>   (deref_pointer ? ValueObject::eExpressionPathAftermathDereference
>  : ValueObject::eExpressionPathAftermathNothing);
>   ValueObjectSP item = valobj->GetValueForExpressionPath(
> -  ptr_deref_buffer.c_str(), &first_unparsed, &reason_to_stop,
> -  &final_value_type, options, &what_next);
> +  ptr_deref_buffer.c_str(), &reason_to_stop, &final_value_type, options,
> +  &what_next);
>   if (!item) {
> if (log)
> -  log->Printf("[ExpandIndexedExpression] ERROR: unparsed portion = %s, 
> why "
> -  "stopping = %d,"
> +  log->Printf("[ExpandIndexedExpression] ERROR: why stopping = %d,"
>   " final_value_type %d",
> -  first_unparsed, reason_to_stop, final_value_type);
> +  reason_to_stop, final_value_type);
>   } else {
> if (log)
> -  log->Printf("[ExpandIndexedExpression] ALL RIGHT: unparsed portion = 
> %s, "
> -  "why stopping = %d,"
> +  log->Printf("[ExpandIndexedExpression] ALL RIGHT: why stopping = %d,"
>   " final_value_type %d",
> -  first_unparsed, reason_to_stop, final_value_type);
> +  reason_to_stop, final_value_type);
>   }
>   return item;
> }
> @@ -724,7 +721,6 @@ static bool DumpValue(Stream &s, const S
>   int64_t index_lower = -1;
>   int64_t index_higher = -1;
>   bool is_array_range = false;
> -  const char *first_unparsed;
>   bool was_plain_var = false;
>   bool was_var_format = false;
>   bool was_var_indexed = false;
> @@ -762,25 +758,23 @@ static

Re: [Lldb-commits] [lldb] r287354 - Resubmit "Remove an output-parameter from Variable function".

[Zachary Turner via lldb-commits]

I admit I've been moving somewhat fast with these changes.  The parameter
in question was only used in one logging statement, so the benefit did not
seem worth the added cost of complexity.  But you're right, that involved a
judgement call on my part that I didn't vet first.

+Enrico Granata  , who can hopefully do a post-commit
review on this.  I can add it back without much effort if he feels it's
important.

On Fri, Nov 18, 2016 at 12:02 PM Jim Ingham  wrote:

> I didn’t see this patch go up for review.  The parameter you removed might
> have been vestigial or might have been one that the owner of this code was
> planning to do something with.  Anyway, this seems to go beyond purely
> formal changes and so should have been discussed.  My apologies if I just
> missed the review.
>
> Jim
>
> > On Nov 18, 2016, at 9:55 AM, Zachary Turner via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > Author: zturner
> > Date: Fri Nov 18 11:55:04 2016
> > New Revision: 287354
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=287354&view=rev
> > Log:
> > Resubmit "Remove an output-parameter from Variable function".
> >
> > The scanning algorithm had a few little subtleties that I
> > overlooked, but this patch should fix everything.
> >
> > I still haven't changed the function to take a StringRef since
> > that has some trickle down effect and is mostly mechanical,
> > I just wanted to get the tricky part as isolated as possible.
> >
> > Modified:
> >lldb/trunk/include/lldb/Core/ValueObject.h
> >lldb/trunk/source/Core/FormatEntity.cpp
> >lldb/trunk/source/Core/ValueObject.cpp
> >lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
> >lldb/trunk/source/Symbol/Variable.cpp
> >
> > Modified: lldb/trunk/include/lldb/Core/ValueObject.h
> > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ValueObject.h?rev=287354&r1=287353&r2=287354&view=diff
> >
> ==
> > --- lldb/trunk/include/lldb/Core/ValueObject.h (original)
> > +++ lldb/trunk/include/lldb/Core/ValueObject.h Fri Nov 18 11:55:04 2016
> > @@ -394,7 +394,7 @@ public:
> >   GetExpressionPathFormat =
> eGetExpressionPathFormatDereferencePointers);
> >
> >   lldb::ValueObjectSP GetValueForExpressionPath(
> > -  const char *expression, const char **first_unparsed = nullptr,
> > +  const char *expression,
> >   ExpressionPathScanEndReason *reason_to_stop = nullptr,
> >   ExpressionPathEndResultType *final_value_type = nullptr,
> >   const GetValueForExpressionPathOptions &options =
> > @@ -1002,8 +1002,7 @@ private:
> >   virtual CompilerType MaybeCalculateCompleteType();
> >
> >   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
> > -  const char *expression_cstr, const char **first_unparsed,
> > -  ExpressionPathScanEndReason *reason_to_stop,
> > +  const char *expression_cstr, ExpressionPathScanEndReason
> *reason_to_stop,
> >   ExpressionPathEndResultType *final_value_type,
> >   const GetValueForExpressionPathOptions &options,
> >   ExpressionPathAftermath *final_task_on_target);
> >
> > Modified: lldb/trunk/source/Core/FormatEntity.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/FormatEntity.cpp?rev=287354&r1=287353&r2=287354&view=diff
> >
> ==
> > --- lldb/trunk/source/Core/FormatEntity.cpp (original)
> > +++ lldb/trunk/source/Core/FormatEntity.cpp Fri Nov 18 11:55:04 2016
> > @@ -614,7 +614,6 @@ static ValueObjectSP ExpandIndexedExpres
> >   if (log)
> > log->Printf("[ExpandIndexedExpression] name to deref: %s",
> > ptr_deref_buffer.c_str());
> > -  const char *first_unparsed;
> >   ValueObject::GetValueForExpressionPathOptions options;
> >   ValueObject::ExpressionPathEndResultType final_value_type;
> >   ValueObject::ExpressionPathScanEndReason reason_to_stop;
> > @@ -622,20 +621,18 @@ static ValueObjectSP ExpandIndexedExpres
> >   (deref_pointer ? ValueObject::eExpressionPathAftermathDereference
> >  : ValueObject::eExpressionPathAftermathNothing);
> >   ValueObjectSP item = valobj->GetValueForExpressionPath(
> > -  ptr_deref_buffer.c_str(), &first_unparsed, &reason_to_stop,
> > -  &final_value_type, options, &what_next);
> > +  ptr_deref_buffer.c_str(), &reason_to_stop, &final_value_type,
> options,
> > +  &what_next);
> >   if (!item) {
> > if (log)
> > -  log->Printf("[ExpandIndexedExpression] ERROR: unparsed portion =
> %s, why "
> > -  "stopping = %d,"
> > +  log->Printf("[ExpandIndexedExpression] ERROR: why stopping = %d,"
> >   " final_value_type %d",
> > -  first_unparsed, reason_to_stop, final_value_type);
> > +  reason_to_stop, final_value_type);
> >   } else {
> > if (log)
> > -  log->Printf("[ExpandIndexedExpression] ALL RIGHT: un

Re: [Lldb-commits] [lldb] r287354 - Resubmit "Remove an output-parameter from Variable function".

[Jim Ingham via lldb-commits]

Not a big deal.  Formal changes are fine, but even dead code in areas you don’t 
work on might represent a work in progress, so it’s good to ask first.  For 
instance, Enrico and Greg originally had plans for “frame var” to be able to 
print slices of arrays - that’s presumably the dead code functions you stripped 
in a change before this one.  That idea was probably abandoned long enough ago 
that we’re unlikely to revive it, but you never know.

Jim.

> On Nov 18, 2016, at 12:32 PM, Zachary Turner  wrote:
> 
> I admit I've been moving somewhat fast with these changes.  The parameter in 
> question was only used in one logging statement, so the benefit did not seem 
> worth the added cost of complexity.  But you're right, that involved a 
> judgement call on my part that I didn't vet first. 
> 
> +Enrico Granata  , who can hopefully do a 
> post-commit review on this.  I can add it back without much effort if he 
> feels it's important. 
> 
> On Fri, Nov 18, 2016 at 12:02 PM Jim Ingham  > wrote:
> I didn’t see this patch go up for review.  The parameter you removed might 
> have been vestigial or might have been one that the owner of this code was 
> planning to do something with.  Anyway, this seems to go beyond purely formal 
> changes and so should have been discussed.  My apologies if I just missed the 
> review.
> 
> Jim
> 
> > On Nov 18, 2016, at 9:55 AM, Zachary Turner via lldb-commits 
> > mailto:lldb-commits@lists.llvm.org>> wrote:
> >
> > Author: zturner
> > Date: Fri Nov 18 11:55:04 2016
> > New Revision: 287354
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=287354&view=rev 
> > 
> > Log:
> > Resubmit "Remove an output-parameter from Variable function".
> >
> > The scanning algorithm had a few little subtleties that I
> > overlooked, but this patch should fix everything.
> >
> > I still haven't changed the function to take a StringRef since
> > that has some trickle down effect and is mostly mechanical,
> > I just wanted to get the tricky part as isolated as possible.
> >
> > Modified:
> >lldb/trunk/include/lldb/Core/ValueObject.h
> >lldb/trunk/source/Core/FormatEntity.cpp
> >lldb/trunk/source/Core/ValueObject.cpp
> >lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
> >lldb/trunk/source/Symbol/Variable.cpp
> >
> > Modified: lldb/trunk/include/lldb/Core/ValueObject.h
> > URL: 
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ValueObject.h?rev=287354&r1=287353&r2=287354&view=diff
> >  
> > 
> > ==
> > --- lldb/trunk/include/lldb/Core/ValueObject.h (original)
> > +++ lldb/trunk/include/lldb/Core/ValueObject.h Fri Nov 18 11:55:04 2016
> > @@ -394,7 +394,7 @@ public:
> >   GetExpressionPathFormat = 
> > eGetExpressionPathFormatDereferencePointers);
> >
> >   lldb::ValueObjectSP GetValueForExpressionPath(
> > -  const char *expression, const char **first_unparsed = nullptr,
> > +  const char *expression,
> >   ExpressionPathScanEndReason *reason_to_stop = nullptr,
> >   ExpressionPathEndResultType *final_value_type = nullptr,
> >   const GetValueForExpressionPathOptions &options =
> > @@ -1002,8 +1002,7 @@ private:
> >   virtual CompilerType MaybeCalculateCompleteType();
> >
> >   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
> > -  const char *expression_cstr, const char **first_unparsed,
> > -  ExpressionPathScanEndReason *reason_to_stop,
> > +  const char *expression_cstr, ExpressionPathScanEndReason 
> > *reason_to_stop,
> >   ExpressionPathEndResultType *final_value_type,
> >   const GetValueForExpressionPathOptions &options,
> >   ExpressionPathAftermath *final_task_on_target);
> >
> > Modified: lldb/trunk/source/Core/FormatEntity.cpp
> > URL: 
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/FormatEntity.cpp?rev=287354&r1=287353&r2=287354&view=diff
> >  
> > 
> > ==
> > --- lldb/trunk/source/Core/FormatEntity.cpp (original)
> > +++ lldb/trunk/source/Core/FormatEntity.cpp Fri Nov 18 11:55:04 2016
> > @@ -614,7 +614,6 @@ static ValueObjectSP ExpandIndexedExpres
> >   if (log)
> > log->Printf("[ExpandIndexedExpression] name to deref: %s",
> > ptr_deref_buffer.c_str());
> > -  const char *first_unparsed;
> >   ValueObject::GetValueForExpressionPathOptions options;
> >   ValueObject::ExpressionPathEndResultType final_value_type;
> >   ValueObject::ExpressionPathScanEndReason reason_to_stop;
> > @@ -622,20 +621,18 @@ static ValueObjectSP ExpandIndexedExpres
> >   (de

[Lldb-commits] [lldb] r287376 - Change CreateTarget and dependents to accept StringRef.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Fri Nov 18 14:44:46 2016
New Revision: 287376

URL: http://llvm.org/viewvc/llvm-project?rev=287376&view=rev
Log:
Change CreateTarget and dependents to accept StringRef.

Modified:
lldb/trunk/include/lldb/Interpreter/OptionGroupArchitecture.h
lldb/trunk/include/lldb/Target/TargetList.h
lldb/trunk/source/API/SBDebugger.cpp
lldb/trunk/source/Commands/CommandObjectProcess.cpp
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
lldb/trunk/source/Plugins/Platform/Linux/PlatformLinux.cpp
lldb/trunk/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp
lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/trunk/source/Target/Platform.cpp
lldb/trunk/source/Target/TargetList.cpp

Modified: lldb/trunk/include/lldb/Interpreter/OptionGroupArchitecture.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Interpreter/OptionGroupArchitecture.h?rev=287376&r1=287375&r2=287376&view=diff
==
--- lldb/trunk/include/lldb/Interpreter/OptionGroupArchitecture.h (original)
+++ lldb/trunk/include/lldb/Interpreter/OptionGroupArchitecture.h Fri Nov 18 
14:44:46 2016
@@ -33,7 +33,6 @@ public:
 
   Error SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
ExecutionContext *execution_context) override;
-  Error SetOptionValue(uint32_t, const char *, ExecutionContext *) = delete;
 
   void OptionParsingStarting(ExecutionContext *execution_context) override;
 
@@ -41,9 +40,7 @@ public:
 
   bool ArchitectureWasSpecified() const { return !m_arch_str.empty(); }
 
-  const char *GetArchitectureName() {
-return (m_arch_str.empty() ? nullptr : m_arch_str.c_str());
-  }
+  llvm::StringRef GetArchitectureName() const { return m_arch_str; }
 
 protected:
   std::string m_arch_str; // Save the arch triple in case a platform is

Modified: lldb/trunk/include/lldb/Target/TargetList.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/TargetList.h?rev=287376&r1=287375&r2=287376&view=diff
==
--- lldb/trunk/include/lldb/Target/TargetList.h (original)
+++ lldb/trunk/include/lldb/Target/TargetList.h Fri Nov 18 14:44:46 2016
@@ -91,8 +91,8 @@ public:
   /// @return
   /// An error object that indicates success or failure
   //--
-  Error CreateTarget(Debugger &debugger, const char *user_exe_path,
- const char *triple_cstr, bool get_dependent_modules,
+  Error CreateTarget(Debugger &debugger, llvm::StringRef user_exe_path,
+ llvm::StringRef triple_str, bool get_dependent_modules,
  const OptionGroupPlatform *platform_options,
  lldb::TargetSP &target_sp);
 
@@ -102,7 +102,7 @@ public:
   /// Same as the function above, but used when you already know the
   /// platform you will be using
   //--
-  Error CreateTarget(Debugger &debugger, const char *user_exe_path,
+  Error CreateTarget(Debugger &debugger, llvm::StringRef user_exe_path,
  const ArchSpec &arch, bool get_dependent_modules,
  lldb::PlatformSP &platform_sp, lldb::TargetSP &target_sp);
 
@@ -211,15 +211,17 @@ protected:
 private:
   lldb::TargetSP GetDummyTarget(lldb_private::Debugger &debugger);
 
-  Error CreateDummyTarget(Debugger &debugger, const char *specified_arch_name,
+  Error CreateDummyTarget(Debugger &debugger,
+  llvm::StringRef specified_arch_name,
   lldb::TargetSP &target_sp);
 
-  Error CreateTargetInternal(Debugger &debugger, const char *user_exe_path,
- const char *triple_cstr, bool get_dependent_files,
+  Error CreateTargetInternal(Debugger &debugger, llvm::StringRef user_exe_path,
+ llvm::StringRef triple_str,
+ bool get_dependent_files,
  const OptionGroupPlatform *platform_options,
  lldb::TargetSP &target_sp, bool is_dummy_target);
 
-  Error CreateTargetInternal(Debugger &debugger, const char *user_exe_path,
+  Error CreateTargetInternal(Debugger &debugger, llvm::StringRef user_exe_path,
  const ArchSpec &arch, bool get_dependent_modules,
  lldb::PlatformSP &platform_sp,
  lldb::TargetSP &target_sp, bool is_dummy_target);

Modified: lldb/trunk/source/API/SBDebugger.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SBDebugger.cpp?rev=287376&r1=2

Re: [Lldb-commits] [lldb] r287354 - Resubmit "Remove an output-parameter from Variable function".

[Enrico Granata via lldb-commits]

I tend to agree with Jim's point. I have sometimes put in things that 
look like "dead/vestigial code", which I am actually either planning to 
use later on, or actively using somewhere else. This is a quite 
realistic scenario due to Swift support for LLDB existing in an entirely 
separate universe. I have multiple times committed changes to llvm.org 
which are - on their face - glorified no-ops, but I actually end up 
using on the other side of the Swift fence. Removing that kind of change 
without asking first is A Bad Thing(TM).


Also, in the not-too-distant future, people who aren't the original 
authors of this area of the code will end up having to own and make 
calls about it. IMO, It becomes even more important to give everyone 
advance notice at that point.


With all of that said, in this case, I believe the change itself is 
fine. I have - admittedly - not delved deep into the mechanics of each 
diff, but I am just making the "judgment call" on the removal of 
first_unparsed. Removing that argument is fine.
As things stand, it is indeed unused. One day, we might decide we want 
to do better diagnostics for summary formatters and/or frame variable 
(e,g, be able to say things like "foo.bar->baz[3] not valid - reason: 
baz doesn't have a member named [3] as it's not of array or pointer 
type") - and then the combination of first_unparsed and reason_to_stop 
would be exactly the data to use. With that said, we don't do this now, 
and AFAIK there are no explicit plans around doing it.


tl;dr Thanks for asking, LGTM


On 11/18/16 12:32 PM, Zachary Turner wrote:
I admit I've been moving somewhat fast with these changes.  The 
parameter in question was only used in one logging statement, so the 
benefit did not seem worth the added cost of complexity.  But you're 
right, that involved a judgement call on my part that I didn't vet first.


+Enrico Granata  , who can hopefully do a 
post-commit review on this.  I can add it back without much effort if 
he feels it's important.


On Fri, Nov 18, 2016 at 12:02 PM Jim Ingham > wrote:


I didn’t see this patch go up for review.  The parameter you
removed might have been vestigial or might have been one that the
owner of this code was planning to do something with.  Anyway,
this seems to go beyond purely formal changes and so should have
been discussed.  My apologies if I just missed the review.

Jim

> On Nov 18, 2016, at 9:55 AM, Zachary Turner via lldb-commits
mailto:lldb-commits@lists.llvm.org>>
wrote:
>
> Author: zturner
> Date: Fri Nov 18 11:55:04 2016
> New Revision: 287354
>
> URL: http://llvm.org/viewvc/llvm-project?rev=287354&view=rev
> Log:
> Resubmit "Remove an output-parameter from Variable function".
>
> The scanning algorithm had a few little subtleties that I
> overlooked, but this patch should fix everything.
>
> I still haven't changed the function to take a StringRef since
> that has some trickle down effect and is mostly mechanical,
> I just wanted to get the tricky part as isolated as possible.
>
> Modified:
>lldb/trunk/include/lldb/Core/ValueObject.h
>lldb/trunk/source/Core/FormatEntity.cpp
>lldb/trunk/source/Core/ValueObject.cpp
> lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
>lldb/trunk/source/Symbol/Variable.cpp
>
> Modified: lldb/trunk/include/lldb/Core/ValueObject.h
> URL:

http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ValueObject.h?rev=287354&r1=287353&r2=287354&view=diff
>

==
> --- lldb/trunk/include/lldb/Core/ValueObject.h (original)
> +++ lldb/trunk/include/lldb/Core/ValueObject.h Fri Nov 18
11:55:04 2016
> @@ -394,7 +394,7 @@ public:
>   GetExpressionPathFormat =
eGetExpressionPathFormatDereferencePointers);
>
>   lldb::ValueObjectSP GetValueForExpressionPath(
> -  const char *expression, const char **first_unparsed =
nullptr,
> +  const char *expression,
>   ExpressionPathScanEndReason *reason_to_stop = nullptr,
>   ExpressionPathEndResultType *final_value_type = nullptr,
>   const GetValueForExpressionPathOptions &options =
> @@ -1002,8 +1002,7 @@ private:
>   virtual CompilerType MaybeCalculateCompleteType();
>
>   lldb::ValueObjectSP GetValueForExpressionPath_Impl(
> -  const char *expression_cstr, const char **first_unparsed,
> -  ExpressionPathScanEndReason *reason_to_stop,
> +  const char *expression_cstr, ExpressionPathScanEndReason
*reason_to_stop,
>   ExpressionPathEndResultType *final_value_type,
>   const GetValueForExpressionPathOptions &options,
>   ExpressionPathAftermath *final_task_on_target);
>
> Modified: lldb/trunk/source/Core/FormatEn

[Lldb-commits] [lldb] r287386 - Fix "thread step until" handling of multiple line inputs.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Fri Nov 18 16:06:10 2016
New Revision: 287386

URL: http://llvm.org/viewvc/llvm-project?rev=287386&view=rev
Log:
Fix "thread step until" handling of multiple line inputs.

Also document that it handles same, and add some tests.

Added:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/

lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/Makefile

lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/TestStepUntil.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/main.c
Modified:
lldb/trunk/source/Commands/CommandObjectThread.cpp

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/Makefile
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/Makefile?rev=287386&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/Makefile
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/Makefile
 Fri Nov 18 16:06:10 2016
@@ -0,0 +1,5 @@
+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/TestStepUntil.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/TestStepUntil.py?rev=287386&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/TestStepUntil.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/step_until/TestStepUntil.py
 Fri Nov 18 16:06:10 2016
@@ -0,0 +1,92 @@
+"""Test stepping over vrs. hitting breakpoints & subsequent stepping in 
various forms."""
+
+from __future__ import print_function
+
+
+import os
+import time
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCStepping(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def getCategories(self):
+return ['basic_process']
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line numbers that we will step to in main:
+self.main_source = "main.c"
+self.less_than_two = line_number('main.c', 'Less than 2')
+self.greater_than_two = line_number('main.c', 'Greater than or equal 
to 2.')
+self.back_out_in_main = line_number('main.c', 'Back out in main')
+
+def do_until (self, args, until_lines, expected_linenum):
+self.build()
+exe = os.path.join(os.getcwd(), "a.out")
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+main_source_spec = lldb.SBFileSpec(self.main_source)
+break_before = target.BreakpointCreateBySourceRegex(
+'At the start',
+main_source_spec)
+self.assertTrue(break_before, VALID_BREAKPOINT)
+
+# Now launch the process, and do not stop at entry point.
+process = target.LaunchSimple(
+args, None, self.get_process_working_directory())
+
+self.assertTrue(process, PROCESS_IS_VALID)
+
+# The stop reason of the thread should be breakpoint.
+threads = lldbutil.get_threads_stopped_at_breakpoint(
+process, break_before)
+
+if len(threads) != 1:
+self.fail("Failed to stop at first breakpoint in main.")
+
+thread = threads[0]
+return thread
+
+thread = self.common_setup(None)
+
+cmd_interp = self.dbg.GetCommandInterpreter()
+ret_obj = lldb.SBCommandReturnObject()
+
+cmd_line = "thread until"
+for line_num in until_lines:
+cmd_line += " %d"%(line_num)
+ 
+cmd_interp.HandleCommand(cmd_line, ret_obj)
+self.assertTrue(ret_obj.Succeeded(), "'%s' failed: %s."%(cmd_line, 
ret_obj.GetError()))
+
+frame = thread.frames[0]
+line = frame.GetLineEntry().GetLine()
+self.assertEqual(line, expected_linenum, 'Did not get the expected 
stop line number')
+
+def test_hitting_one (self):
+"""Test thread step until - targeting one line and hitting it."""
+self.do_until(None, [self.less_than_two], self.less_than_two)
+
+def test_targetting_two_hitting_first (self):
+"""Test thread step until - targeting two lines and hitting one."""
+self.do_until(["foo", "bar", "baz"], [self.less_than_two, 
self.greater_than_two], self.greater_than_two)
+
+def test_targetting_two_hitting_second (self):
+"""Test thread step until - targeting two lines and hitting the other 
one."""
+self.do_until(None, [self.less_than_two, self.

[Lldb-commits] [lldb] r287397 - Removing myself from CODE_OWNERS, and distributing those duties among other members of the community

[Enrico Granata via lldb-commits]

Author: enrico
Date: Fri Nov 18 17:18:11 2016
New Revision: 287397

URL: http://llvm.org/viewvc/llvm-project?rev=287397&view=rev
Log:
Removing myself from CODE_OWNERS, and distributing those duties among other 
members of the community

That's All, Folks


Modified:
lldb/trunk/CODE_OWNERS.txt

Modified: lldb/trunk/CODE_OWNERS.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/CODE_OWNERS.txt?rev=287397&r1=287396&r2=287397&view=diff
==
--- lldb/trunk/CODE_OWNERS.txt (original)
+++ lldb/trunk/CODE_OWNERS.txt Fri Nov 18 17:18:11 2016
@@ -16,18 +16,14 @@ N: Greg Clayton
 E: clayb...@gmail.com (Phabricator)
 E: gclay...@apple.com (Direct)
 D: Overall LLDB architecture, Host (common+macosx), Symbol, API, ABI, 
Mac-specific code, 
-D: DynamicLoader, ObjectFile, IOHandler, EditLine, ValueObject, Watchpoints, 
debugserver
+D: DynamicLoader, ObjectFile, IOHandler, EditLine, Core/Value*, Watchpoints, 
debugserver
 D: Build scripts, Test suite, Platform, gdb-remote, Anything not covered by 
this file
 
-N: Enrico Granata
-E: granata.enr...@gmail.com
-D: Data Formatters, Core/Value*, Objective C Language runtime, Test suite, 
Xcode build
-D: SWIG
-
 N: Jim Ingham
 E: jing...@apple.com
 D: Overall LLDB architecture, Thread plans, Expression parser, ValueObject, 
Breakpoints, ABI
 D: Watchpoints, Trampolines, Target, Command Interpreter, C++ / Objective C 
Language runtime
+D: Data Formatters
 
 N: Ilia K
 E: ki.s...@gmail.com
@@ -48,6 +44,7 @@ D: lldb-mi
 N: Zachary Turner
 E: ztur...@google.com
 D: CMake build, Host (common+windows), Plugins/Process/Windows, Anything 
Windows-specific
+D: Test suite
 
 N: Pavel Labath
 E: lab...@google.com
@@ -55,5 +52,5 @@ D: Linux, Android
 
 N: Todd Fiala
 E: todd.fi...@gmail.com
-D: Test Suite subsystems (concurrent test runners, test events, TestResults 
system), gdb-remote protocol tests
+D: Test Suite, esp. subsystems (concurrent test runners, test events, 
TestResults system), gdb-remote protocol tests
 


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

[Lldb-commits] [lldb] r287401 - Convert CommandHistory functions to StringRef.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Fri Nov 18 17:22:42 2016
New Revision: 287401

URL: http://llvm.org/viewvc/llvm-project?rev=287401&view=rev
Log:
Convert CommandHistory functions to StringRef.

Modified:
lldb/trunk/include/lldb/Interpreter/CommandHistory.h
lldb/trunk/source/Interpreter/CommandHistory.cpp
lldb/trunk/source/Interpreter/CommandInterpreter.cpp

Modified: lldb/trunk/include/lldb/Interpreter/CommandHistory.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Interpreter/CommandHistory.h?rev=287401&r1=287400&r2=287401&view=diff
==
--- lldb/trunk/include/lldb/Interpreter/CommandHistory.h (original)
+++ lldb/trunk/include/lldb/Interpreter/CommandHistory.h Fri Nov 18 17:22:42 
2016
@@ -33,15 +33,15 @@ public:
 
   bool IsEmpty() const;
 
-  const char *FindString(const char *input_str) const;
+  llvm::Optional FindString(llvm::StringRef input_str) const;
 
-  const char *GetStringAtIndex(size_t idx) const;
+  llvm::StringRef GetStringAtIndex(size_t idx) const;
 
-  const char *operator[](size_t idx) const;
+  llvm::StringRef operator[](size_t idx) const;
 
-  const char *GetRecentmostString() const;
+  llvm::StringRef GetRecentmostString() const;
 
-  void AppendString(const std::string &str, bool reject_if_dupe = true);
+  void AppendString(llvm::StringRef str, bool reject_if_dupe = true);
 
   void Clear();
 

Modified: lldb/trunk/source/Interpreter/CommandHistory.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Interpreter/CommandHistory.cpp?rev=287401&r1=287400&r2=287401&view=diff
==
--- lldb/trunk/source/Interpreter/CommandHistory.cpp (original)
+++ lldb/trunk/source/Interpreter/CommandHistory.cpp Fri Nov 18 17:22:42 2016
@@ -29,57 +29,62 @@ bool CommandHistory::IsEmpty() const {
   return m_history.empty();
 }
 
-const char *CommandHistory::FindString(const char *input_str) const {
+llvm::Optional
+CommandHistory::FindString(llvm::StringRef input_str) const {
   std::lock_guard guard(m_mutex);
-  if (!input_str)
-return nullptr;
+  if (input_str.size() < 2)
+return llvm::None;
+
   if (input_str[0] != g_repeat_char)
-return nullptr;
-  if (input_str[1] == '-') {
-bool success;
-size_t idx = StringConvert::ToUInt32(input_str + 2, 0, 0, &success);
-if (!success)
-  return nullptr;
+return llvm::None;
+
+  if (input_str[1] == g_repeat_char) {
+if (m_history.empty())
+  return llvm::None;
+return m_history.back();
+  }
+
+  input_str = input_str.drop_front();
+
+  size_t idx = 0;
+  if (input_str.front() == '-') {
+if (input_str.drop_front(2).getAsInteger(0, idx))
+  return llvm::None;
 if (idx > m_history.size())
-  return nullptr;
+  return llvm::None;
 idx = m_history.size() - idx;
-return m_history[idx].c_str();
+return m_history[idx];
 
-  } else if (input_str[1] == g_repeat_char) {
-if (m_history.empty())
-  return nullptr;
-else
-  return m_history.back().c_str();
   } else {
-bool success;
-uint32_t idx = StringConvert::ToUInt32(input_str + 1, 0, 0, &success);
-if (!success)
-  return nullptr;
+if (input_str.drop_front().getAsInteger(0, idx))
+  return llvm::None;
+if (idx > m_history.size())
+  return llvm::None;
 if (idx >= m_history.size())
-  return nullptr;
-return m_history[idx].c_str();
+  return llvm::None;
+return m_history[idx];
   }
 }
 
-const char *CommandHistory::GetStringAtIndex(size_t idx) const {
+llvm::StringRef CommandHistory::GetStringAtIndex(size_t idx) const {
   std::lock_guard guard(m_mutex);
   if (idx < m_history.size())
-return m_history[idx].c_str();
-  return nullptr;
+return m_history[idx];
+  return "";
 }
 
-const char *CommandHistory::operator[](size_t idx) const {
+llvm::StringRef CommandHistory::operator[](size_t idx) const {
   return GetStringAtIndex(idx);
 }
 
-const char *CommandHistory::GetRecentmostString() const {
+llvm::StringRef CommandHistory::GetRecentmostString() const {
   std::lock_guard guard(m_mutex);
   if (m_history.empty())
-return nullptr;
-  return m_history.back().c_str();
+return "";
+  return m_history.back();
 }
 
-void CommandHistory::AppendString(const std::string &str, bool reject_if_dupe) 
{
+void CommandHistory::AppendString(llvm::StringRef str, bool reject_if_dupe) {
   std::lock_guard guard(m_mutex);
   if (reject_if_dupe) {
 if (!m_history.empty()) {
@@ -87,7 +92,7 @@ void CommandHistory::AppendString(const
 return;
 }
   }
-  m_history.push_back(std::string(str));
+  m_history.push_back(str);
 }
 
 void CommandHistory::Clear() {

Modified: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Interpreter/CommandInterpreter.cpp?rev=287401&r1=287400&r2=287401&view=diff
=

[Lldb-commits] [lldb] r287408 - [CMake] NFC. Updating CMake dependency specifications

[Chris Bieneman via lldb-commits]

Author: cbieneman
Date: Fri Nov 18 17:31:53 2016
New Revision: 287408

URL: http://llvm.org/viewvc/llvm-project?rev=287408&view=rev
Log:
[CMake] NFC. Updating CMake dependency specifications

This patch updates a bunch of places where add_dependencies was being 
explicitly called to add dependencies on intrinsics_gen to instead use the 
DEPENDS named parameter. This cleanup is needed for a patch I'm working on to 
add a dependency debugging mode to the build system.

Modified:
lldb/trunk/cmake/modules/AddLLDB.cmake
lldb/trunk/source/Expression/CMakeLists.txt
lldb/trunk/source/Plugins/ExpressionParser/Clang/CMakeLists.txt

lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt

Modified: lldb/trunk/cmake/modules/AddLLDB.cmake
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/cmake/modules/AddLLDB.cmake?rev=287408&r1=287407&r2=287408&view=diff
==
--- lldb/trunk/cmake/modules/AddLLDB.cmake (original)
+++ lldb/trunk/cmake/modules/AddLLDB.cmake Fri Nov 18 17:31:53 2016
@@ -23,7 +23,7 @@ macro(add_lldb_library name)
   cmake_parse_arguments(PARAM
 "MODULE;SHARED;STATIC;OBJECT"
 ""
-""
+"DEPENDS"
 ${ARGN})
   llvm_process_sources(srcs ${PARAM_UNPARSED_ARGUMENTS})
 
@@ -61,14 +61,16 @@ macro(add_lldb_library name)
 llvm_add_library(${name} ${libkind} ${srcs} LINK_LIBS
 -Wl,--start-group ${LLDB_USED_LIBS} 
-Wl,--end-group
 -Wl,--start-group ${CLANG_USED_LIBS} 
-Wl,--end-group
+DEPENDS ${PARAM_DEPENDS}
   )
   else()
 llvm_add_library(${name} ${libkind} ${srcs} LINK_LIBS
 ${LLDB_USED_LIBS} ${CLANG_USED_LIBS}
+DEPENDS ${PARAM_DEPENDS}
   )
   endif()
 else()
-  llvm_add_library(${name} ${libking} ${srcs})
+  llvm_add_library(${name} ${libkind} ${srcs} DEPENDS ${PARAM_DEPENDS})
 endif()
 
 if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ${name} STREQUAL "liblldb")

Modified: lldb/trunk/source/Expression/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Expression/CMakeLists.txt?rev=287408&r1=287407&r2=287408&view=diff
==
--- lldb/trunk/source/Expression/CMakeLists.txt (original)
+++ lldb/trunk/source/Expression/CMakeLists.txt Fri Nov 18 17:31:53 2016
@@ -1,3 +1,7 @@
+if(NOT LLDB_BUILT_STANDALONE)
+  set(tablegen_deps intrinsics_gen)
+endif()
+
 add_lldb_library(lldbExpression
   DiagnosticManager.cpp
   DWARFExpression.cpp
@@ -14,8 +18,7 @@ add_lldb_library(lldbExpression
   REPL.cpp
   UserExpression.cpp
   UtilityFunction.cpp
-  )
 
-if(NOT LLDB_BUILT_STANDALONE)
-  add_dependencies(lldbExpression intrinsics_gen)
-endif()
+  DEPENDS
+  ${tablegen_deps}
+  )

Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/CMakeLists.txt?rev=287408&r1=287407&r2=287408&view=diff
==
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/CMakeLists.txt (original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/CMakeLists.txt Fri Nov 18 
17:31:53 2016
@@ -1,3 +1,7 @@
+if(NOT LLDB_BUILT_STANDALONE)
+  set(tablegen_deps intrinsics_gen)
+endif()
+
 add_lldb_library(lldbPluginExpressionParserClang
   ASTDumper.cpp
   ASTResultSynthesizer.cpp
@@ -12,8 +16,7 @@ add_lldb_library(lldbPluginExpressionPar
   ClangUserExpression.cpp
   ClangUtilityFunction.cpp
   IRForTarget.cpp
-  )
 
-if(NOT LLDB_BUILT_STANDALONE)
-  add_dependencies(lldbPluginExpressionParserClang intrinsics_gen)
-endif()
+  DEPENDS
+  ${tablegen_deps}
+  )

Modified: 
lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt?rev=287408&r1=287407&r2=287408&view=diff
==
--- 
lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt
 (original)
+++ 
lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt
 Fri Nov 18 17:31:53 2016
@@ -1,10 +1,14 @@
+if(NOT LLDB_BUILT_STANDALONE)
+  set(tablegen_deps intrinsics_gen)
+endif()
+
+
 add_lldb_library(lldbPluginRenderScriptRuntime
   RenderScriptRuntime.cpp
   RenderScriptExpressionOpts.cpp
   RenderScriptx86ABIFixups.cpp
   RenderScriptScriptGroup.cpp
-  )
 
-if(NOT LLDB_BUILT_STANDALONE)
-  add_dependencies(lldbPluginRenderScriptRuntime intrinsics_gen)
-endif()
+  DEPENDS
+  ${tablegen_deps}
+  )


___
lldb-commits mailing list
lldb-commits@lists.

[Lldb-commits] [lldb] r287409 - Fix some build errors.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Fri Nov 18 17:32:37 2016
New Revision: 287409

URL: http://llvm.org/viewvc/llvm-project?rev=287409&view=rev
Log:
Fix some build errors.

Modified:
lldb/trunk/source/Interpreter/CommandHistory.cpp

Modified: lldb/trunk/source/Interpreter/CommandHistory.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Interpreter/CommandHistory.cpp?rev=287409&r1=287408&r2=287409&view=diff
==
--- lldb/trunk/source/Interpreter/CommandHistory.cpp (original)
+++ lldb/trunk/source/Interpreter/CommandHistory.cpp Fri Nov 18 17:32:37 2016
@@ -41,7 +41,7 @@ CommandHistory::FindString(llvm::StringR
   if (input_str[1] == g_repeat_char) {
 if (m_history.empty())
   return llvm::None;
-return m_history.back();
+return llvm::StringRef(m_history.back());
   }
 
   input_str = input_str.drop_front();
@@ -50,20 +50,17 @@ CommandHistory::FindString(llvm::StringR
   if (input_str.front() == '-') {
 if (input_str.drop_front(2).getAsInteger(0, idx))
   return llvm::None;
-if (idx > m_history.size())
+if (idx >= m_history.size())
   return llvm::None;
 idx = m_history.size() - idx;
-return m_history[idx];
-
   } else {
 if (input_str.drop_front().getAsInteger(0, idx))
   return llvm::None;
-if (idx > m_history.size())
-  return llvm::None;
 if (idx >= m_history.size())
   return llvm::None;
-return m_history[idx];
   }
+
+  return llvm::StringRef(m_history[idx]);
 }
 
 llvm::StringRef CommandHistory::GetStringAtIndex(size_t idx) const {


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

[Lldb-commits] [lldb] r287412 - Fix some accidental Prints of StringRefs that snuck in.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Fri Nov 18 18:50:29 2016
New Revision: 287412

URL: http://llvm.org/viewvc/llvm-project?rev=287412&view=rev
Log:
Fix some accidental Prints of StringRefs that snuck in.

Modified:
lldb/trunk/source/Symbol/Variable.cpp
lldb/trunk/source/Target/TargetList.cpp

Modified: lldb/trunk/source/Symbol/Variable.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Variable.cpp?rev=287412&r1=287411&r2=287412&view=diff
==
--- lldb/trunk/source/Symbol/Variable.cpp (original)
+++ lldb/trunk/source/Symbol/Variable.cpp Fri Nov 18 18:50:29 2016
@@ -393,12 +393,14 @@ Error Variable::GetValuesForVariableExpr
 variable_list.Clear();
 if (!g_regex.Execute(variable_expr_path, ®ex_match)) {
   error.SetErrorStringWithFormat(
-  "unable to extract a variable name from '%s'", variable_expr_path);
+  "unable to extract a variable name from '%s'",
+  variable_expr_path.str().c_str());
   return error;
 }
 if (!regex_match.GetMatchAtIndex(variable_expr_path, 1, variable_name)) {
   error.SetErrorStringWithFormat(
-  "unable to extract a variable name from '%s'", variable_expr_path);
+  "unable to extract a variable name from '%s'",
+  variable_expr_path.str().c_str());
   return error;
 }
 if (!callback(baton, variable_name.c_str(), variable_list)) {
@@ -434,7 +436,8 @@ Error Variable::GetValuesForVariableExpr
 if (!valobj_sp) {
   error.SetErrorStringWithFormat(
   "invalid expression path '%s' for variable '%s'",
-  variable_sub_expr_path, var_sp->GetName().GetCString());
+  variable_sub_expr_path.str().c_str(),
+  var_sp->GetName().GetCString());
   variable_list.RemoveVariableAtIndex(i);
   continue;
 }

Modified: lldb/trunk/source/Target/TargetList.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/TargetList.cpp?rev=287412&r1=287411&r2=287412&view=diff
==
--- lldb/trunk/source/Target/TargetList.cpp (original)
+++ lldb/trunk/source/Target/TargetList.cpp Fri Nov 18 18:50:29 2016
@@ -327,7 +327,8 @@ Error TargetList::CreateTargetInternal(D
bool is_dummy_target) {
   Timer scoped_timer(LLVM_PRETTY_FUNCTION,
  "TargetList::CreateTarget (file = '%s', arch = '%s')",
- user_exe_path, specified_arch.GetArchitectureName());
+ user_exe_path.str().c_str(),
+ specified_arch.GetArchitectureName());
   Error error;
 
   ArchSpec arch(specified_arch);


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

[Lldb-commits] [PATCH] D26883: Demonstrate proposed new Args API

[Zachary Turner via lldb-commits]

zturner created this revision.
zturner added reviewers: jingham, beanz, clayborg, labath.
zturner added a subscriber: lldb-commits.

The purpose of this patch is to demonstrate my proposed new API to the `Args` 
class.  There are a couple of things I'm trying to demonstrate here:

1. range-based for loops by iterating directly over the `Args` instance itself. 
 This should be the preferred way to iterate over arguments and should always 
be used unless index-based iteration is required.

2. range-based for loops by iterating over `args.entries()`.  This should be 
used when you only want to iterate a subset of the arguments, like if you want 
to skip the first one (e.g. `for (const auto &entry : 
args.entries().drop_front())`.  This prevents many types of bugs, such as those 
posted to the list recently in which we repeatedly access `args[0]` instead of 
`args[i]`.

3. Index-based iteration through the use of `operator[]`.  Should only be used 
when you need to do look ahead or look-behind, or a command takes positional 
arguments.  Prefer range-based for otherwise.  This method supersedes 
`GetArgumentAtIndex` and the long term plan is to delete `GetArgumentAtIndex` 
after all call-sites have been converted.

4. Direct access to `StringRef` through the use of `entry.ref`.  Should always 
be used unless you're printing or passing to a C-api.

5. Direct access to the quote char through the use of `entry.quote`.  
Supersedes `GetArgumentQuoteCharAtIndex`.

6. Access to a null-terminated c-string.  Should only be used when printf'ing 
the value or passing to a C-api (which, btw, should almost never be done for 
any kind of string manipulation operation, as `StringRef` provides everything 
you need).  Long term I plan to delete this accessor, the only thing blocking 
this is the heavy use of printf-style formatting (which I have a solution for 
in mind).  In any case, it should not be relied on for any kind of algorithmic 
access to the underlying string.

7. A couple of simple examples of how this proposed new API can trivially 
reduce string copies and improve efficiency.

All dependent calls have already been updated to take `StringRef`, so the 
"phase 2", so to speak, will be to begin eliminating calls to 
`GetArgumentAtIndex` using the methods described above.  Once they are all 
gone, the plan is to delete `GetArgumentAtIndex` and `GetQuoteCharAtIndex`.

In the more distant future, if I can make my `Printf` alternative a reality, I 
plan to delete c-style string access entirely by removing the `c_str()` method 
from `ArgEntry`.


https://reviews.llvm.org/D26883

Files:
  include/lldb/Interpreter/Args.h
  source/Breakpoint/BreakpointIDList.cpp
  source/Commands/CommandObjectCommands.cpp
  source/Commands/CommandObjectFrame.cpp
  source/Commands/CommandObjectProcess.cpp
  source/Commands/CommandObjectSettings.cpp
  source/Interpreter/CommandInterpreter.cpp
  source/Interpreter/OptionValueDictionary.cpp

Index: source/Interpreter/OptionValueDictionary.cpp
===
--- source/Interpreter/OptionValueDictionary.cpp
+++ source/Interpreter/OptionValueDictionary.cpp
@@ -101,73 +101,74 @@
   case eVarSetOperationAppend:
   case eVarSetOperationReplace:
   case eVarSetOperationAssign:
-if (argc > 0) {
-  for (size_t i = 0; i < argc; ++i) {
-llvm::StringRef key_and_value(args.GetArgumentAtIndex(i));
-if (!key_and_value.empty()) {
-  if (key_and_value.find('=') == llvm::StringRef::npos) {
-error.SetErrorString(
-"assign operation takes one or more key=value arguments");
-return error;
-  }
+if (argc == 0) {
+  error.SetErrorString(
+  "assign operation takes one or more key=value arguments");
+  return error;
+}
+for (const auto &entry : args) {
+  if (entry.ref.empty()) {
+error.SetErrorString("empty argument");
+return error;
+  }
+  if (!entry.ref.contains('=')) {
+error.SetErrorString(
+"assign operation takes one or more key=value arguments");
+return error;
+  }
+
+  llvm::StringRef key, value;
+  std::tie(key, value) = entry.ref.split('=');
+  bool key_valid = false;
+  if (key.empty()) {
+error.SetErrorString("empty dictionary key");
+return error;
+  }
 
-  std::pair kvp(
-  key_and_value.split('='));
-  llvm::StringRef key = kvp.first;
-  bool key_valid = false;
-  if (!key.empty()) {
-if (key.front() == '[') {
-  // Key name starts with '[', so the key value must be in single or
-  // double quotes like:
-  // ['']
-  // [""]
-  if ((key.size() > 2) && (key.back() == ']')) {
-// Strip leading '[' and trailing ']'
-key = key.substr(1, key.size() - 2);
-const char quote_char = key.front();