[Lldb-commits] [PATCH] D147370: [lldb] fixing #61727 fixing incorrect variable displaying with DW_OP_div

[LU Hongyi via Phabricator via lldb-commits]

jwnhy created this revision.
jwnhy added reviewers: Michael137, DavidSpickett.
Herald added a project: All.
jwnhy requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch resolves an issue where a value 
is incorrectly displayed if it is represented 
by DW_OP_div.

This issue is caused by lldb evaluating 
operands of DW_OP_div as unsigned
and performed unintended unsigned 
division.

This issue is resolved by creating two
temporary signed scalar and performing
signed division.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147370

Files:
  lldb/source/Expression/DWARFExpression.cpp


Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1436,8 +1436,12 @@
   return false;
 } else {
   stack.pop_back();
-  stack.back() =
-  stack.back().ResolveValue(exe_ctx) / tmp.ResolveValue(exe_ctx);
+  Scalar divisor, dividend;
+  divisor = tmp.ResolveValue(exe_ctx);
+  dividend = stack.back().ResolveValue(exe_ctx);
+  divisor.MakeSigned();
+  dividend.MakeSigned();
+  stack.back() = dividend / divisor;
   if (!stack.back().ResolveValue(exe_ctx).IsValid()) {
 if (error_ptr)
   error_ptr->SetErrorString("Divide failed.");


Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1436,8 +1436,12 @@
   return false;
 } else {
   stack.pop_back();
-  stack.back() =
-  stack.back().ResolveValue(exe_ctx) / tmp.ResolveValue(exe_ctx);
+  Scalar divisor, dividend;
+  divisor = tmp.ResolveValue(exe_ctx);
+  dividend = stack.back().ResolveValue(exe_ctx);
+  divisor.MakeSigned();
+  dividend.MakeSigned();
+  stack.back() = dividend / divisor;
   if (!stack.back().ResolveValue(exe_ctx).IsValid()) {
 if (error_ptr)
   error_ptr->SetErrorString("Divide failed.");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147362: [lldb] Add Clang module import logging

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 510234.
kastiglione added a comment.

Add -Rmodule-import flag


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147362

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -219,6 +219,13 @@
 LLDB_LOG(log, "Finished building Clang module {0}", module_name);
 return true;
   }
+  case clang::diag::remark_module_import: {
+const auto &module_name = info.getArgStdStr(0);
+const auto &module_path = info.getArgStdStr(1);
+LLDB_LOG(log, "Importing Clang module {0} from {1}", module_name,
+ module_path);
+return true;
+  }
   default:
 return false;
   }
@@ -671,7 +678,8 @@
   "-fmodules-validate-system-headers",
   "-Werror=non-modular-include-in-framework-module",
   "-Xclang=-fincremental-extensions",
-  "-Rmodule-build"};
+  "-Rmodule-build",
+  "-Rmodule-import"};
 
   target.GetPlatform()->AddClangModuleCompilationOptions(
   &target, compiler_invocation_arguments);


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -219,6 +219,13 @@
 LLDB_LOG(log, "Finished building Clang module {0}", module_name);
 return true;
   }
+  case clang::diag::remark_module_import: {
+const auto &module_name = info.getArgStdStr(0);
+const auto &module_path = info.getArgStdStr(1);
+LLDB_LOG(log, "Importing Clang module {0} from {1}", module_name,
+ module_path);
+return true;
+  }
   default:
 return false;
   }
@@ -671,7 +678,8 @@
   "-fmodules-validate-system-headers",
   "-Werror=non-modular-include-in-framework-module",
   "-Xclang=-fincremental-extensions",
-  "-Rmodule-build"};
+  "-Rmodule-build",
+  "-Rmodule-import"};
 
   target.GetPlatform()->AddClangModuleCompilationOptions(
   &target, compiler_invocation_arguments);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147370: [lldb] fixing #61727 fixing incorrect variable displaying with DW_OP_div

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

The change looks good: it matches what the DWARFv5 standard says:

> The DW_OP_div operation pops the top two stack values, divides the former 
> second entry by the former top of the stack using signed division, and pushes 
> the result.

Can you please add a regression test for the corrected behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147370

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

[Lldb-commits] [PATCH] D147385: [lldb] Add fixits for "frame variable"

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added reviewers: aprantl, jingham, JDevlieghere, augusto2112.
Herald added a project: All.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The `frame variable` command can handle mistaken use of `.` for `->`, and vice 
versa,
but up until now it has been an error when the user makes such a mistake. This 
change
adds a flag to `frame variable` which allows these mistakes to be fixed. The 
idea is why
make the user manually edit the command if lldb can figure it out.

This is similar to `expression` which supports fixits supplied by the compiler, 
which
include correcting `.` to `->`, and vice versa.

There are a couple of details that differ slightly from `expression`:

1. The `--apply-fixits`/`-X` flag doesn't take a bool value, this matches the 
convention

used by many `frame variable` flags.

2. The fixed expression is printed, but there is no diagnostic message to draw 
attention

to the fix. I wasn't sure it was worth the noise, so I went with the simple 
approach.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147385

Files:
  lldb/include/lldb/Interpreter/OptionGroupVariable.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Interpreter/OptionGroupVariable.cpp
  lldb/test/API/commands/frame/var/fixits/Makefile
  lldb/test/API/commands/frame/var/fixits/TestFrameVarApplyFixits.py
  lldb/test/API/commands/frame/var/fixits/main.c

Index: lldb/test/API/commands/frame/var/fixits/main.c
===
--- /dev/null
+++ lldb/test/API/commands/frame/var/fixits/main.c
@@ -0,0 +1,10 @@
+struct Structure {
+  int num;
+};
+
+int main() {
+  struct Structure s = {30};
+  struct Structure *p = &s;
+  // break here
+  return 0;
+}
Index: lldb/test/API/commands/frame/var/fixits/TestFrameVarApplyFixits.py
===
--- /dev/null
+++ lldb/test/API/commands/frame/var/fixits/TestFrameVarApplyFixits.py
@@ -0,0 +1,24 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+def test_frame_variable_apply_fixits(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+
+self.expect(
+"frame variable s->num",
+error=True,
+startstr='error: "s" is not a pointer and -> was used to attempt to access "num". Did you mean "s.num"?',
+)
+self.expect("frame variable --apply-fixits s->num", startstr="(int) s.num = 30")
+
+self.expect(
+"frame variable p.num",
+error=True,
+startstr='error: "p" is a pointer and . was used to attempt to access "num". Did you mean "p->num"?',
+)
+self.expect("frame variable --apply-fixits p.num", startstr="(int) p->num = 30")
Index: lldb/test/API/commands/frame/var/fixits/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/frame/var/fixits/Makefile
@@ -0,0 +1,2 @@
+C_SOURCES := main.c
+include Makefile.rules
Index: lldb/source/Interpreter/OptionGroupVariable.cpp
===
--- lldb/source/Interpreter/OptionGroupVariable.cpp
+++ lldb/source/Interpreter/OptionGroupVariable.cpp
@@ -42,6 +42,9 @@
 {LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "scope", 's',
  OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone,
  "Show variable scope (argument, local, global, static)."},
+{LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "apply-fixits", 'X',
+ OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone,
+ "simple fix-it corrections will be applied to the variable path."},
 {LLDB_OPT_SET_1, false, "summary", 'y', OptionParser::eRequiredArgument,
  nullptr, {}, 0, eArgTypeName,
  "Specify the summary that the variable output should use."},
@@ -70,7 +73,8 @@
 : include_frame_options(show_frame_options), show_args(false),
   show_recognized_args(false), show_locals(false), show_globals(false),
   use_regex(false), show_scope(false), show_decl(false),
-  summary(ValidateNamedSummary), summary_string(ValidateSummaryString) {}
+  apply_fixits(false), summary(ValidateNamedSummary),
+  summary_string(ValidateSummaryString) {}
 
 Status
 OptionGroupVariable::SetOptionValue(uint32_t option_idx,
@@ -102,6 +106,9 @@
   case 't':
 show_recognized_args = false;
 break;
+  case 'X':
+apply_fixits = true;
+break;
   case 'y':
 error = summary.SetCurrentValue(option_arg);
 break;
@@ -124,6 +131,7 @@
   show_decl = false;
   use_regex = false;
   show_scope = false;
+  apply_fixits = false;
   summary.Clear();
   summary_string.Clear();
 }
Index: lldb/source/Commands/CommandObjectFrame.cpp

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

There seems to be overlap in the code added in this patch and D146679 
. Does one supersede the other or is there a 
dependency? If it's the latter you should extract that into a separate patch 
and make it a parent revision of this and D146679 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

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

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

In D147292#4238820 , @JDevlieghere 
wrote:

> There seems to be overlap in the code added in this patch and D146679 
> . Does one supersede the other or is there 
> a dependency? If it's the latter you should extract that into a separate 
> patch and make it a parent revision of this and D146679 
> .

Yes, sorry, I'm abandoning the other one in favor of this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

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

[Lldb-commits] [PATCH] D146679: [lldb] Add support for the DW_AT_trampoline attribute with mangled names

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 abandoned this revision.
augusto2112 added a comment.

Abandoning in favor of https://reviews.llvm.org/D147292/.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146679

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

[Lldb-commits] [PATCH] D147385: [lldb] Add fixits for "frame variable"

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 510280.
kastiglione added a comment.

Change --apply-fixits take an argument


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147385

Files:
  lldb/include/lldb/Interpreter/OptionGroupVariable.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Interpreter/OptionGroupVariable.cpp
  lldb/test/API/commands/frame/var/fixits/Makefile
  lldb/test/API/commands/frame/var/fixits/TestFrameVarApplyFixits.py
  lldb/test/API/commands/frame/var/fixits/main.c

Index: lldb/test/API/commands/frame/var/fixits/main.c
===
--- /dev/null
+++ lldb/test/API/commands/frame/var/fixits/main.c
@@ -0,0 +1,10 @@
+struct Structure {
+  int num;
+};
+
+int main() {
+  struct Structure s = {30};
+  struct Structure *p = &s;
+  // break here
+  return 0;
+}
Index: lldb/test/API/commands/frame/var/fixits/TestFrameVarApplyFixits.py
===
--- /dev/null
+++ lldb/test/API/commands/frame/var/fixits/TestFrameVarApplyFixits.py
@@ -0,0 +1,24 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+def test_frame_variable_apply_fixits(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+
+self.expect(
+"frame variable s->num",
+error=True,
+startstr='error: "s" is not a pointer and -> was used to attempt to access "num". Did you mean "s.num"?',
+)
+self.expect("frame variable --apply-fixits on s->num", startstr="(int) s.num = 30")
+
+self.expect(
+"frame variable p.num",
+error=True,
+startstr='error: "p" is a pointer and . was used to attempt to access "num". Did you mean "p->num"?',
+)
+self.expect("frame variable --apply-fixits on p.num", startstr="(int) p->num = 30")
Index: lldb/test/API/commands/frame/var/fixits/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/frame/var/fixits/Makefile
@@ -0,0 +1,2 @@
+C_SOURCES := main.c
+include Makefile.rules
Index: lldb/source/Interpreter/OptionGroupVariable.cpp
===
--- lldb/source/Interpreter/OptionGroupVariable.cpp
+++ lldb/source/Interpreter/OptionGroupVariable.cpp
@@ -11,14 +11,14 @@
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Status.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-// if you add any options here, remember to update the counters in
-// OptionGroupVariable::GetNumDefinitions()
+// clang-format off
 static constexpr OptionDefinition g_variable_options[] = {
 {LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "no-args", 'a',
  OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone,
@@ -42,6 +42,9 @@
 {LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "scope", 's',
  OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone,
  "Show variable scope (argument, local, global, static)."},
+{LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "apply-fixits", 'X',
+ OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeBoolean,
+ "simple fix-it corrections will be applied to the variable path."},
 {LLDB_OPT_SET_1, false, "summary", 'y', OptionParser::eRequiredArgument,
  nullptr, {}, 0, eArgTypeName,
  "Specify the summary that the variable output should use."},
@@ -49,6 +52,7 @@
  OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeName,
  "Specify a summary string to use to format the variable output."},
 };
+// clang-format on
 
 static Status ValidateNamedSummary(const char *str, void *) {
   if (!str || !str[0])
@@ -70,7 +74,8 @@
 : include_frame_options(show_frame_options), show_args(false),
   show_recognized_args(false), show_locals(false), show_globals(false),
   use_regex(false), show_scope(false), show_decl(false),
-  summary(ValidateNamedSummary), summary_string(ValidateSummaryString) {}
+  apply_fixits(false), summary(ValidateNamedSummary),
+  summary_string(ValidateSummaryString) {}
 
 Status
 OptionGroupVariable::SetOptionValue(uint32_t option_idx,
@@ -102,6 +107,15 @@
   case 't':
 show_recognized_args = false;
 break;
+  case 'X': {
+bool success = false;
+apply_fixits = OptionArgParser::ToBoolean(option_arg, false, &success);
+if (!success)
+  error.SetErrorStringWithFormat(
+  "Invalid boolean '%s' passed for -X option",
+  option_arg.str().c_str());
+break;
+  }
   case 'y':
 error = sum

[Lldb-commits] [PATCH] D147370: [lldb] fixing #61727 fixing incorrect variable displaying with DW_OP_div

[LU Hongyi via Phabricator via lldb-commits]

jwnhy updated this revision to Diff 510285.
jwnhy added a comment.

Update the patch to include a small
regression test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147370

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/API/commands/expression/signed-dw-op-div/Makefile
  lldb/test/API/commands/expression/signed-dw-op-div/TestSignedDiv.py
  lldb/test/API/commands/expression/signed-dw-op-div/main.c


Index: lldb/test/API/commands/expression/signed-dw-op-div/main.c
===
--- /dev/null
+++ lldb/test/API/commands/expression/signed-dw-op-div/main.c
@@ -0,0 +1,14 @@
+#include "stdint.h"
+static volatile uint64_t g = 0;
+
+static const int32_t f() {
+  uint32_t i;
+  for (i = 0; (i != 10); i++)
+++g; // break here
+  return 0;
+}
+
+int main() {
+  f();
+  return 0;
+}
Index: lldb/test/API/commands/expression/signed-dw-op-div/TestSignedDiv.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/signed-dw-op-div/TestSignedDiv.py
@@ -0,0 +1,14 @@
+"""Test that we can print values that represented via DW_OP_div"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+
+class SignedDivTestCase(TestBase):
+def test_signed_div(self):
+"""Test that we can print values that represented via DW_OP_div"""
+self.build()
+_, process, _, bkpt = lldbutil.run_to_source_breakpoint(self, '// 
break here', lldb.SBFileSpec('main.c'))
+self.expect_expr('i', result_type='uint32_t', result_value='0')
+for i in range(1, 10):
+lldbutil.continue_to_breakpoint(process, bkpt)
+self.expect_expr('i', result_type='uint32_t', result_value=str(i))
Index: lldb/test/API/commands/expression/signed-dw-op-div/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/expression/signed-dw-op-div/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -g -O1
+
+include Makefile.rules
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1436,8 +1436,12 @@
   return false;
 } else {
   stack.pop_back();
-  stack.back() =
-  stack.back().ResolveValue(exe_ctx) / tmp.ResolveValue(exe_ctx);
+  Scalar divisor, dividend;
+  divisor = tmp.ResolveValue(exe_ctx);
+  dividend = stack.back().ResolveValue(exe_ctx);
+  divisor.MakeSigned();
+  dividend.MakeSigned();
+  stack.back() = dividend / divisor;
   if (!stack.back().ResolveValue(exe_ctx).IsValid()) {
 if (error_ptr)
   error_ptr->SetErrorString("Divide failed.");


Index: lldb/test/API/commands/expression/signed-dw-op-div/main.c
===
--- /dev/null
+++ lldb/test/API/commands/expression/signed-dw-op-div/main.c
@@ -0,0 +1,14 @@
+#include "stdint.h"
+static volatile uint64_t g = 0;
+
+static const int32_t f() {
+  uint32_t i;
+  for (i = 0; (i != 10); i++)
+++g; // break here
+  return 0;
+}
+
+int main() {
+  f();
+  return 0;
+}
Index: lldb/test/API/commands/expression/signed-dw-op-div/TestSignedDiv.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/signed-dw-op-div/TestSignedDiv.py
@@ -0,0 +1,14 @@
+"""Test that we can print values that represented via DW_OP_div"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+
+class SignedDivTestCase(TestBase):
+def test_signed_div(self):
+"""Test that we can print values that represented via DW_OP_div"""
+self.build()
+_, process, _, bkpt = lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec('main.c'))
+self.expect_expr('i', result_type='uint32_t', result_value='0')
+for i in range(1, 10):
+lldbutil.continue_to_breakpoint(process, bkpt)
+self.expect_expr('i', result_type='uint32_t', result_value=str(i))
Index: lldb/test/API/commands/expression/signed-dw-op-div/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/expression/signed-dw-op-div/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -g -O1
+
+include Makefile.rules
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -1436,8 +1436,12 @@
   return false;
 } else {
   stack.pop_back();
-  stack.back() =
-  stack.back().ResolveValue(exe_ctx) / tmp.ResolveValue