[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 5 inline comments as done.
jankratochvil added a comment.

In D66654#1643082 , @JDevlieghere 
wrote:

> How much work would it be to return an `llvm::Expected` from 
> `Get_Impl`? I'd really prefer that over the current approach.


Done.  Although now  if (Get(...)) became confusing which I have commented in 
`lldb_private::FormatDropExpected()`.

BTW I haven't found how to format a table for Doxygen.  Also by searching 
around I have found other tables in LLDB/LLVM are also not recognized by 
Doxygen as tables.




Comment at: lldb/include/lldb/DataFormatters/FormattersContainer.h:296
 
-  bool Get_Impl(ConstString key, MapValueType &value,
+  bool Get_Impl(ConstString key, MapValueType &value, ValueObject *valobj,
 lldb::RegularExpressionSP *dummy) {

JDevlieghere wrote:
> If returning an expected isn't feasible, we should pass at least a `Status*` 
> instead of setting the error in the `ValueObject` directly. 
With `Expected` this is no longer applicable.



Comment at: lldb/source/DataFormatters/TypeCategory.cpp:303
+if (GetTypeFormatsContainer()->Get(type_name, format_sp,
+   nullptr /*valobj*/)) {
   if (matching_category)

JDevlieghere wrote:
> It's really unfortunate that so many call sites now require a null pointer. 
> Can we make it a default argument?
With `Expected` this is no longer applicable.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654



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

[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 217052.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654

Files:
  lldb/include/lldb/Core/ValueObject.h
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-advanced/TestDataFormatterAdv.py
  lldb/source/API/SBTypeCategory.cpp
  lldb/source/DataFormatters/FormatClasses.cpp
  lldb/source/DataFormatters/TypeCategory.cpp

Index: lldb/source/DataFormatters/TypeCategory.cpp
===
--- lldb/source/DataFormatters/TypeCategory.cpp
+++ lldb/source/DataFormatters/TypeCategory.cpp
@@ -101,12 +101,22 @@
lldb::TypeFormatImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeFormatsContainer()->Get(candidates, entry, reason))
+  llvm::Expected plain =
+  GetTypeFormatsContainer()->Get(candidates, entry, reason);
+  if (plain && *plain)
 return true;
-  bool regex = GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
-  if (regex && reason)
-*reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
-  return regex;
+  llvm::Expected regex =
+  GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
+  if (regex && *regex) {
+FormatDropExpected(std::move(plain));
+if (reason)
+  *reason |=
+  lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
+return true;
+  }
+  valobj.TakeError(std::move(plain));
+  valobj.TakeError(std::move(regex));
+  return false;
 }
 
 bool TypeCategoryImpl::Get(ValueObject &valobj,
@@ -114,12 +124,22 @@
lldb::TypeSummaryImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeSummariesContainer()->Get(candidates, entry, reason))
+  llvm::Expected plain =
+  GetTypeSummariesContainer()->Get(candidates, entry, reason);
+  if (plain && *plain)
+return true;
+  llvm::Expected regex =
+  GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
+  if (regex && *regex) {
+FormatDropExpected(std::move(plain));
+if (reason)
+  *reason |=
+  lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
 return true;
-  bool regex = GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
-  if (regex && reason)
-*reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
-  return regex;
+  }
+  valobj.TakeError(std::move(plain));
+  valobj.TakeError(std::move(regex));
+  return false;
 }
 
 bool TypeCategoryImpl::Get(ValueObject &valobj,
@@ -132,20 +152,38 @@
   bool regex_filter = false;
   // first find both Filter and Synth, and then check which is most recent
 
-  if (!GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter))
-regex_filter = GetRegexTypeFiltersContainer()->Get(candidates, filter_sp,
-   &reason_filter);
+  llvm::Expected error_filter_regex(false);
+  llvm::Expected error_filter_plain =
+  GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter);
+  if (!error_filter_plain || !*error_filter_plain) {
+FormatDropExpected(std::move(error_filter_regex));
+error_filter_regex = GetRegexTypeFiltersContainer()->Get(
+candidates, filter_sp, &reason_filter);
+if (error_filter_regex && *error_filter_regex)
+  regex_filter = true;
+  }
 
   bool regex_synth = false;
   uint32_t reason_synth = 0;
   bool pick_synth = false;
   ScriptedSyntheticChildren::SharedPointer synth;
-  if (!GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth))
-regex_synth = GetRegexTypeSyntheticsContainer()->Get(candidates, synth,
- &reason_synth);
-  if (!filter_sp.get() && !synth.get())
+  llvm::Expected error_synth_regex(false);
+  llvm::Expected error_synth_plain =
+  GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth);
+  if (!error_synth_plain || !*error_synth_plain) {
+FormatDropExpected(std::move(error_synth_regex));
+error_synth_regex = GetRegexTypeSyntheticsContainer()->Get(
+candidates, synth, &reason_synth);
+if (error_synth_regex && *error_synth_regex)
+  regex_synth = true;
+  }
+  if (!filter_sp.get() && !synth.get()) {
+valobj.TakeError(std::move(error_filter_plain));
+valobj.TakeError(std::move(error_filter_regex));
+valobj.TakeError(std::move(error_synth_plain));
+valobj.TakeError(std::move(error_synth_regex));
 return false;
-  else if (!filter_sp.get() && synth.get())
+  } else if (!filter_sp.get() && synth.get())
 pick_synth = true;
 
   else if (filter_sp.get() && !synth.get())
@@ -155,18 +193,22 @@
   {

[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 217053.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654

Files:
  lldb/include/lldb/Core/ValueObject.h
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-advanced/TestDataFormatterAdv.py
  lldb/source/API/SBTypeCategory.cpp
  lldb/source/DataFormatters/FormatClasses.cpp
  lldb/source/DataFormatters/TypeCategory.cpp

Index: lldb/source/DataFormatters/TypeCategory.cpp
===
--- lldb/source/DataFormatters/TypeCategory.cpp
+++ lldb/source/DataFormatters/TypeCategory.cpp
@@ -101,12 +101,22 @@
lldb::TypeFormatImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeFormatsContainer()->Get(candidates, entry, reason))
+  llvm::Expected plain =
+  GetTypeFormatsContainer()->Get(candidates, entry, reason);
+  if (plain && *plain)
 return true;
-  bool regex = GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
-  if (regex && reason)
-*reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
-  return regex;
+  llvm::Expected regex =
+  GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
+  if (regex && *regex) {
+FormatDropExpected(std::move(plain));
+if (reason)
+  *reason |=
+  lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
+return true;
+  }
+  valobj.TakeError(std::move(plain));
+  valobj.TakeError(std::move(regex));
+  return false;
 }
 
 bool TypeCategoryImpl::Get(ValueObject &valobj,
@@ -114,12 +124,22 @@
lldb::TypeSummaryImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeSummariesContainer()->Get(candidates, entry, reason))
+  llvm::Expected plain =
+  GetTypeSummariesContainer()->Get(candidates, entry, reason);
+  if (plain && *plain)
+return true;
+  llvm::Expected regex =
+  GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
+  if (regex && *regex) {
+FormatDropExpected(std::move(plain));
+if (reason)
+  *reason |=
+  lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
 return true;
-  bool regex = GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
-  if (regex && reason)
-*reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
-  return regex;
+  }
+  valobj.TakeError(std::move(plain));
+  valobj.TakeError(std::move(regex));
+  return false;
 }
 
 bool TypeCategoryImpl::Get(ValueObject &valobj,
@@ -132,20 +152,38 @@
   bool regex_filter = false;
   // first find both Filter and Synth, and then check which is most recent
 
-  if (!GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter))
-regex_filter = GetRegexTypeFiltersContainer()->Get(candidates, filter_sp,
-   &reason_filter);
+  llvm::Expected error_filter_regex(false);
+  llvm::Expected error_filter_plain =
+  GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter);
+  if (!error_filter_plain || !*error_filter_plain) {
+FormatDropExpected(std::move(error_filter_regex));
+error_filter_regex = GetRegexTypeFiltersContainer()->Get(
+candidates, filter_sp, &reason_filter);
+if (error_filter_regex && *error_filter_regex)
+  regex_filter = true;
+  }
 
   bool regex_synth = false;
   uint32_t reason_synth = 0;
   bool pick_synth = false;
   ScriptedSyntheticChildren::SharedPointer synth;
-  if (!GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth))
-regex_synth = GetRegexTypeSyntheticsContainer()->Get(candidates, synth,
- &reason_synth);
-  if (!filter_sp.get() && !synth.get())
+  llvm::Expected error_synth_regex(false);
+  llvm::Expected error_synth_plain =
+  GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth);
+  if (!error_synth_plain || !*error_synth_plain) {
+FormatDropExpected(std::move(error_synth_regex));
+error_synth_regex = GetRegexTypeSyntheticsContainer()->Get(
+candidates, synth, &reason_synth);
+if (error_synth_regex && *error_synth_regex)
+  regex_synth = true;
+  }
+  if (!filter_sp.get() && !synth.get()) {
+valobj.TakeError(std::move(error_filter_plain));
+valobj.TakeError(std::move(error_filter_regex));
+valobj.TakeError(std::move(error_synth_plain));
+valobj.TakeError(std::move(error_synth_regex));
 return false;
-  else if (!filter_sp.get() && synth.get())
+  } else if (!filter_sp.get() && synth.get())
 pick_synth = true;
 
   else if (filter_sp.get() && !synth.get())
@@ -155,18 +193,22 @@
   {

[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 217079.
jankratochvil added a comment.

Testcase for the regex sanity check is failing - `ValueObject::m_error` remains 
set even after `type summary clear`. `ValueObject::UpdateValueIfNeeded` will 
find `did_change_formats==true` but it does not help it. I think there is some 
bug in `ValueObject` updating but maybe that setting of `ValueObject::m_error` 
is a wrong idea.

  runCmd: type summary clear
  output:
  
  runCmd: type summary add --summary-string "${var[0-1]}" -x "int \[[0-9]\]"
  output:
  
  runCmd: frame variable int_array
  output: (int [5]) int_array = [-1,2]
  
  
  Expecting sub string: 1,2
  Matched
  
  runCmd: type summary add --summary-string "${var[0-1]}" "int []"
  output:
  
  runCmd: frame variable int_array
  output: (int [5]) int_array = 
  
  Expecting pattern: int_array = 
  Matched
  
  runCmd: type summary clear
  output:
  
  runCmd: type summary add --summary-string "${var[0-1]}" "int []"
  output:
  
  runCmd: frame variable int_array
  output: (int [5]) int_array = 
  
  Expecting sub string: 1,2
  Not matched
  
  FAIL


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654

Files:
  lldb/include/lldb/Core/ValueObject.h
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-advanced/TestDataFormatterAdv.py
  lldb/source/API/SBTypeCategory.cpp
  lldb/source/DataFormatters/FormatClasses.cpp
  lldb/source/DataFormatters/TypeCategory.cpp

Index: lldb/source/DataFormatters/TypeCategory.cpp
===
--- lldb/source/DataFormatters/TypeCategory.cpp
+++ lldb/source/DataFormatters/TypeCategory.cpp
@@ -101,12 +101,22 @@
lldb::TypeFormatImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeFormatsContainer()->Get(candidates, entry, reason))
+  llvm::Expected plain =
+  GetTypeFormatsContainer()->Get(candidates, entry, reason);
+  if (plain && *plain)
 return true;
-  bool regex = GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
-  if (regex && reason)
-*reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
-  return regex;
+  llvm::Expected regex =
+  GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
+  if (regex && *regex) {
+FormatDropExpected(std::move(plain));
+if (reason)
+  *reason |=
+  lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
+return true;
+  }
+  valobj.TakeError(std::move(plain));
+  valobj.TakeError(std::move(regex));
+  return false;
 }
 
 bool TypeCategoryImpl::Get(ValueObject &valobj,
@@ -114,12 +124,22 @@
lldb::TypeSummaryImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeSummariesContainer()->Get(candidates, entry, reason))
+  llvm::Expected plain =
+  GetTypeSummariesContainer()->Get(candidates, entry, reason);
+  if (plain && *plain)
+return true;
+  llvm::Expected regex =
+  GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
+  if (regex && *regex) {
+FormatDropExpected(std::move(plain));
+if (reason)
+  *reason |=
+  lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
 return true;
-  bool regex = GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
-  if (regex && reason)
-*reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
-  return regex;
+  }
+  valobj.TakeError(std::move(plain));
+  valobj.TakeError(std::move(regex));
+  return false;
 }
 
 bool TypeCategoryImpl::Get(ValueObject &valobj,
@@ -132,20 +152,38 @@
   bool regex_filter = false;
   // first find both Filter and Synth, and then check which is most recent
 
-  if (!GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter))
-regex_filter = GetRegexTypeFiltersContainer()->Get(candidates, filter_sp,
-   &reason_filter);
+  llvm::Expected error_filter_regex(false);
+  llvm::Expected error_filter_plain =
+  GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter);
+  if (!error_filter_plain || !*error_filter_plain) {
+FormatDropExpected(std::move(error_filter_regex));
+error_filter_regex = GetRegexTypeFiltersContainer()->Get(
+candidates, filter_sp, &reason_filter);
+if (error_filter_regex && *error_filter_regex)
+  regex_filter = true;
+  }
 
   bool regex_synth = false;
   uint32_t reason_synth = 0;
   bool pick_synth = false;
   ScriptedSyntheticChildren::SharedPointer synth;
-  if (!GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth))
-regex_synth = GetRegexTypeSyntheticsContai

[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

[Aleksandr Urakov via Phabricator via lldb-commits]

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

LGTM, thanks!




Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:58
  llvm::BumpPtrAllocator &alloc) {
-  llvm::DenseMap dependent_programs;
-
-  size_t cur = 0;
-  while (true) {
-size_t assign_index = program.find('=', cur);
-if (assign_index == llvm::StringRef::npos) {
-  llvm::StringRef tail = program.slice(cur, llvm::StringRef::npos);
-  if (!tail.trim().empty()) {
-// missing assign operator
-return nullptr;
-  }
-  break;
-}
-llvm::StringRef assignment_program = program.slice(cur, assign_index);
-
-llvm::StringRef lvalue_name;
-Node *rvalue_ast = nullptr;
-if (!ParseFPOSingleAssignmentProgram(assignment_program, alloc, 
lvalue_name,
- rvalue_ast)) {
-  return nullptr;
-}
-
-lldbassert(rvalue_ast);
+  std::vector> parsed =
+  postfix::ParseFPOProgram(program, alloc);

Do I understand right, you use a vector of pairs instead of a map due to the 
small number of expressions in a program (then a search on a small vector will 
be faster than on a map)?


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

https://reviews.llvm.org/D66634



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