date:20210609

[Lldb-commits] [lldb] db93e4e - Revert "[lldb] Set return status to failed when adding a command error"

2021-06-09 Thread David Spickett via lldb-commits


Author: David Spickett
Date: 2021-06-09T09:41:59+01:00
New Revision: db93e4e70aa453e5ba04ba0d9e01f581882b6c81

URL: 
https://github.com/llvm/llvm-project/commit/db93e4e70aa453e5ba04ba0d9e01f581882b6c81
DIFF: 
https://github.com/llvm/llvm-project/commit/db93e4e70aa453e5ba04ba0d9e01f581882b6c81.diff

LOG: Revert "[lldb] Set return status to failed when adding a command error"

This reverts commit e05b03cf4f45ac5ee63c59a3464e7d484884645c.

While I investigate a register test failure:
http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/

Added: 
lldb/test/Shell/Commands/command-backtrace.test

Modified: 
lldb/source/Interpreter/CommandReturnObject.cpp
lldb/test/API/commands/register/register/register_command/TestRegisters.py

Removed: 
lldb/test/Shell/Commands/command-backtrace-parser-1.test
lldb/test/Shell/Commands/command-backtrace-parser-2.test



diff  --git a/lldb/source/Interpreter/CommandReturnObject.cpp 
b/lldb/source/Interpreter/CommandReturnObject.cpp
index 531c1f246bd86..c3f32a4c45a9e 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -44,8 +44,6 @@ CommandReturnObject::CommandReturnObject(bool colors)
 : m_out_stream(colors), m_err_stream(colors) {}
 
 void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
-  SetStatus(eReturnStatusFailed);
-
   if (!format)
 return;
   va_list args;
@@ -100,7 +98,6 @@ void CommandReturnObject::AppendWarning(llvm::StringRef 
in_string) {
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   if (in_string.empty())
 return;
-  SetStatus(eReturnStatusFailed);
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
@@ -117,6 +114,7 @@ void CommandReturnObject::SetError(llvm::StringRef 
error_str) {
 return;
 
   AppendError(error_str);
+  SetStatus(eReturnStatusFailed);
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
@@ -126,7 +124,6 @@ void CommandReturnObject::AppendRawError(llvm::StringRef 
in_string) {
   if (in_string.empty())
 return;
   GetErrorStream() << in_string;
-  SetStatus(eReturnStatusFailed);
 }
 
 void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }

diff  --git 
a/lldb/test/API/commands/register/register/register_command/TestRegisters.py 
b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
index 7acf3a4098756..5ec46c175e621 100644
--- a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -41,18 +41,13 @@ def test_register_commands(self):
 self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
 substrs=['registers were unavailable'], matching=False)
 
-all_registers = self.res.GetOutput()
-
 if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
 self.runCmd("register read xmm0")
-if "ymm15 = " in all_registers:
-  self.runCmd("register read ymm15")  # may be available
-if "bnd0 = " in all_registers:
-  self.runCmd("register read bnd0")  # may be available
+self.runCmd("register read ymm15")  # may be available
+self.runCmd("register read bnd0")  # may be available
 elif self.getArchitecture() in ['arm', 'armv7', 'armv7k', 'arm64', 
'arm64e', 'arm64_32']:
 self.runCmd("register read s0")
-if "q15 = " in all_registers:
-  self.runCmd("register read q15")  # may be available
+self.runCmd("register read q15")  # may be available
 
 self.expect(
 "register read -s 4",
@@ -418,8 +413,7 @@ def fp_register_write(self):
 self.write_and_read(currentFrame, "ymm7", new_value)
 self.expect("expr $ymm0", substrs=['vector_type'])
 else:
-self.expect("register read ymm0", substrs=["Invalid register 
name 'ymm0'"],
-error=True)
+self.runCmd("register read ymm0")
 
 if has_mpx:
 # Test write and read for bnd0.
@@ -434,8 +428,7 @@ def fp_register_write(self):
 self.write_and_read(currentFrame, "bndstatus", new_value)
 self.expect("expr $bndstatus", substrs = ['vector_type'])
 else:
-self.expect("register read bnd0", substrs=["Invalid register 
name 'bnd0'"],
-error=True)
+self.runCmd("register read bnd0")
 
 def convenience_registers(self):
 """Test convenience registers."""
@@ -457,7 +450,7 @@ def convenience_registers(self):
 # Now write rax with a unique bit pattern and test that eax indeed
 # represents the lower half of rax.

[Lldb-commits] [PATCH] D103701: [lldb] Set return status to failed when adding a command error

2021-06-09 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

1 failure on MacOS "lldb-api :: 
commands/register/register/register_command/TestRegisters.py", 
http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/,
 hence the revert.

Should be easy enough to fix the test logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103701

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

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

2021-06-09 Thread Raphael Isemann via Phabricator via lldb-commits

teemperor accepted this revision.
teemperor added a comment.

This doesn't compile for me (on Linux):

  In file included from 
/home/teemperor/work/ci/llvm-project/lldb/unittests/Platform/PlatformAppleSimulatorTest.cpp:11:
  In file included from 
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h:15:
  
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:96:14:
 error: use of undeclared identifier 'nil'
id m_dev = nil;
   ^
  
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:132:14:
 error: use of undeclared identifier 'nil'
id m_dev = nil;
   ^
  
/home/teemperor/work/ci/llvm-project/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:172:14:
 error: use of undeclared identifier 'nil'
id m_dev = nil;
   ^

The `nil` is some Obj-C thing but the headers are includes from a C++ file. I 
don't think we should add Obj-C includes to the header as we really need to 
keep the Objective-C++ code in LLDB as small as possible, so maybe change this 
to `nullptr`?

Beside that I couldn't find anything that would block this patch. There are a 
bunch of additional NFC cleanups possible but that seems out of scope, so LGTM 
modulo `nil -> nullptr`.


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

https://reviews.llvm.org/D103483

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

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

2021-06-09 Thread Jan Kratochvil via Phabricator via lldb-commits

jankratochvil added inline comments.



Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:62
+// FIXME: Move it to ReservedBit4?
+HANDLE_DI_FLAG((1 << 30), IsZeroSize)
 

This would need to be handled better if approved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101237

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

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

2021-06-09 Thread Jan Kratochvil via Phabricator via lldb-commits

jankratochvil updated this revision to Diff 350361.
jankratochvil added a comment.
Herald added subscribers: llvm-commits, cfe-commits, dexonsmith, sstefan1, 
martong, hiraditya.
Herald added a reviewer: shafik.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: shafik.
Herald added projects: clang, LLVM.

`DW_AT_byte_size 0` implementation as suggested by @teemperor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101237

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/test/Shell/Expr/no_unique_address.cpp
  llvm/include/llvm/IR/DebugInfoFlags.def
  llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

Index: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -1603,12 +1603,16 @@
   } else {
 addUInt(MemberDie, dwarf::DW_AT_data_bit_offset, None, Offset);
   }
+  assert(!(DT->getFlags() & DINode::FlagIsZeroSize) &&
+ "bitfields cannot have [[no_unique_address]]");
 } else {
   // This is not a bitfield.
   OffsetInBytes = DT->getOffsetInBits() / 8;
   if (AlignInBytes)
 addUInt(MemberDie, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
 AlignInBytes);
+  if (DT->getFlags() & DINode::FlagIsZeroSize)
+addUInt(MemberDie, dwarf::DW_AT_byte_size, None, 0);
 }
 
 if (DD->getDwarfVersion() <= 2) {
Index: llvm/include/llvm/IR/DebugInfoFlags.def
===
--- llvm/include/llvm/IR/DebugInfoFlags.def
+++ llvm/include/llvm/IR/DebugInfoFlags.def
@@ -58,6 +58,8 @@
 HANDLE_DI_FLAG((1 << 27), BigEndian)
 HANDLE_DI_FLAG((1 << 28), LittleEndian)
 HANDLE_DI_FLAG((1 << 29), AllCallsDescribed)
+// FIXME: Move it to ReservedBit4?
+HANDLE_DI_FLAG((1 << 30), IsZeroSize)
 
 // To avoid needing a dedicated value for IndirectVirtualBase, we use
 // the bitwise or of Virtual and FwdDecl, which does not otherwise
@@ -67,7 +69,7 @@
 #ifdef DI_FLAG_LARGEST_NEEDED
 // intended to be used with ADT/BitmaskEnum.h
 // NOTE: always must be equal to largest flag, check this when adding new flag
-HANDLE_DI_FLAG((1 << 29), Largest)
+HANDLE_DI_FLAG((1 << 30), Largest)
 #undef DI_FLAG_LARGEST_NEEDED
 #endif
 
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -843,11 +843,10 @@
CompilerType *child_type = nullptr);
 
   // Modifying RecordType
-  static clang::FieldDecl *AddFieldToRecordType(const CompilerType &type,
-llvm::StringRef name,
-const CompilerType &field_type,
-lldb::AccessType access,
-uint32_t bitfield_bit_size);
+  static clang::FieldDecl *
+  AddFieldToRecordType(const CompilerType &type, llvm::StringRef name,
+   const CompilerType &field_type, lldb::AccessType access,
+   uint32_t bitfield_bit_size, bool IsZeroSize = false);
 
   static void BuildIndirectFields(const CompilerType &type);
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7164,7 +7164,7 @@
 clang::FieldDecl *TypeSystemClang::AddFieldToRecordType(
 const CompilerType &type, llvm::StringRef name,
 const CompilerType &field_clang_type, AccessType access,
-uint32_t bitfield_bit_size) {
+uint32_t bitfield_bit_size, bool IsZeroSize) {
   if (!type.IsValid() || !field_clang_type.IsValid())
 re

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

2021-06-09 Thread Jan Kratochvil via Phabricator via lldb-commits

jankratochvil added a comment.

[Dwarf-Discuss] How to map [[no_unique_address]] into DWARF: 
http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2021-June/004825.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101237

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

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

2021-06-09 Thread David Blaikie via Phabricator via lldb-commits

dblaikie created this revision.
dblaikie added reviewers: hokein, aaron.ballman, jyknight, mehdi_amini, kuhnel.
Herald added subscribers: dcaballe, cota, teijeong, rdzhabarov, tatianashp, 
msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, 
mgester, arpith-jacob, antiagainst, shauheen, rriddle.
dblaikie requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, Sanitizers, cfe-commits, 
stephenneuendorffer, nicolasvasilache, aheejin.
Herald added projects: clang, Sanitizers, LLDB, MLIR, LLVM.

In the interests of disabling misc-no-recursion across LLVM (this seems
like a stylistic choice that is not consistent with LLVM's
style/development approach) this NFC preliminary change adjusts all the
.clang-tidy files to inherit from their parents as much as possible.

This change specifically preserves all the quirks of the current configs
in order to make it easier to review as NFC.

I validatad the change is NFC as follows:

for X in `cat ../files.txt`;
do

  mkdir -p ../tmp/$(dirname $X)
  touch $(dirname $X)/blaikie.cpp
  clang-tidy -dump-config $(dirname $X)/blaikie.cpp > ../tmp/$(dirname $X)/after
  rm $(dirname $X)/blaikie.cpp

done

(similarly for the "before" state, without this patch applied)

for X in `cat ../files.txt`;
do

  echo $X
  diff \
../tmp/$(dirname $X)/before \
<(cat ../tmp/$(dirname $X)/after \
  | sed -e 
"s/,readability-identifier-naming\(.*\),-readability-identifier-naming/\1/" \
  | sed -e "s/,-llvm-include-order\(.*\),llvm-include-order/\1/" \
  | sed -e "s/,-misc-no-recursion\(.*\),misc-no-recursion/\1/" \
  | sed -e "s/,-clang-diagnostic-\*\(.*\),clang-diagnostic-\*/\1/")

done

(using sed to strip some add/remove pairs to reduce the diff and make it easier 
to read)

The resulting report is:

  .clang-tidy
  clang/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-readability-identifier-naming,-misc-no-recursion'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion'
  compiler-rt/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,-llvm-header-guard,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-llvm-header-guard'
  flang/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,llvm-*,-llvm-include-order,misc-*,-misc-no-recursion,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-llvm-include-order,-misc-no-recursion'
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

The `clang/.clang-tidy` change is a no-op, disabling an option that was never 
enabled.
The compiler-rt and flang changes are no-op reorderings of the same flags.

(side note, the .clang-tidy file in parallel-libs is broken and crashes
clang-tidy because it uses "lowerCase" as the style instead of "lower_case" -
so I'll deal with that separately)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103842

Files:
  clang/.clang-tidy
  compiler-rt/.clang-tidy
  flang/.clang-tidy
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

Index: mlir/.clang-tidy
===
--- mlir/.clang-tidy
+++ mlir/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+InheritParentConfig: true
 CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
   - key: readability-identifier-naming.MemberCase
 value:   camelBack
   - key: readability-identifier-naming.ParameterCase
 value:

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

2021-06-09 Thread Aaron Ballman via Phabricator via lldb-commits

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103842

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

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

2021-06-09 Thread David Blaikie via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc5d56fec502f: NFC: .clang-tidy: Inherit configs from parents 
to improve maintainability (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103842

Files:
  clang/.clang-tidy
  compiler-rt/.clang-tidy
  flang/.clang-tidy
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

Index: mlir/.clang-tidy
===
--- mlir/.clang-tidy
+++ mlir/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+InheritParentConfig: true
 CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
   - key: readability-identifier-naming.MemberCase
 value:   camelBack
   - key: readability-identifier-naming.ParameterCase
 value:   camelBack
-  - key: readability-identifier-naming.UnionCase
-value:   CamelCase
   - key: readability-identifier-naming.VariableCase
 value:   camelBack
-  - key: readability-identifier-naming.IgnoreMainLikeFunctions
-value:   1
Index: llvm/tools/split-file/.clang-tidy
===
--- llvm/tools/split-file/.clang-tidy
+++ llvm/tools/split-file/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+InheritParentConfig: true
 CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
   - key: readability-identifier-naming.MemberCase
 value:   camelBack
   - key: readability-identifier-naming.ParameterCase
 value:   camelBack
-  - key: readability-identifier-naming.UnionCase
-value:   CamelCase
   - key: readability-identifier-naming.VariableCase
 value:   camelBack
-  - key: readability-identifier-naming.IgnoreMainLikeFunctions
-value:   1
Index: llvm/.clang-tidy
===
--- llvm/.clang-tidy
+++ llvm/.clang-tidy
@@ -1,19 +1 @@
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
-CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
-  - key: readability-identifier-naming.MemberCase
-value:   CamelCase
-  - key: readability-identifier-naming.ParameterCase
-value:   CamelCase
-  - key: readability-identifier-naming.UnionCase
-value:   CamelCase
-  - key: readability-identifier-naming.VariableCase
-value:   CamelCase
-  - key: readability-identifier-naming.IgnoreMainLikeFunctions
-value:   1
-
+InheritParentConfig: true
Index: lldb/.clang-tidy
===
--- lldb/.clang-tidy
+++ lldb/.clang-tidy
@@ -1,2 +1,2 @@
-# Checks enabled in the top-level .clang-tidy minus readability-identifier-naming
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
+Checks: '-readability-identifier-naming'
+InheritParentConfig: true
Index: lld/.clang-tidy
===
--- lld/.clang-tidy
+++ lld/.clang-tidy
@@ -1,19 +1,8 @@
-# Almost identical to the top-level .clang-tidy, except that {Member,Parameter,Variable}Case use camelBack.
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-mis

Re: [Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error

2021-06-09 Thread David Spickett via lldb-commits

I figured something would fail. I've reverted and will reland with fixed tests.

On Tue, 8 Jun 2021 at 22:17, Jim Ingham  wrote:
>
> Hey, David,
>
> This commit seems to have caused a new failure in TestRegisters.py, e.g.:
>
> http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/
>
> Do you have time to take a look?
>
> Jim
>
>
>
>
> > On Jun 8, 2021, at 1:41 AM, David Spickett via lldb-commits 
> >  wrote:
> >
> >
> > Author: David Spickett
> > Date: 2021-06-08T09:41:07+01:00
> > New Revision: e05b03cf4f45ac5ee63c59a3464e7d484884645c
> >
> > URL: 
> > https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c
> > DIFF: 
> > https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c.diff
> >
> > LOG: [lldb] Set return status to failed when adding a command error
> >
> > There is a common pattern:
> > result.AppendError(...);
> > result.SetStatus(eReturnStatusFailed);
> >
> > I found that some commands don't actually "fail" but only
> > print "error: ..." because the second line got missed.
> >
> > This can cause you to miss a failed command when you're
> > using the Python interface during testing.
> > (and produce some confusing script results)
> >
> > I did not find any place where you would want to add
> > an error without setting the return status, so just
> > set eReturnStatusFailed whenever you add an error to
> > a command result.
> >
> > This change does not remove any of the now redundant
> > SetStatus. This should allow us to see if there are any
> > tests that have commands unexpectedly fail with this change.
> > (the test suite passes for me but I don't have access to all
> > the systems we cover so there could be some corner cases)
> >
> > Some tests that failed on x86 and AArch64 have been modified
> > to work with the new behaviour.
> >
> > Differential Revision: https://reviews.llvm.org/D103701
> >
> > Added:
> >lldb/test/Shell/Commands/command-backtrace-parser-1.test
> >lldb/test/Shell/Commands/command-backtrace-parser-2.test
> >
> > Modified:
> >lldb/source/Interpreter/CommandReturnObject.cpp
> >
> > lldb/test/API/commands/register/register/register_command/TestRegisters.py
> >
> > Removed:
> >lldb/test/Shell/Commands/command-backtrace.test
> >
> >
> > 
> > diff  --git a/lldb/source/Interpreter/CommandReturnObject.cpp 
> > b/lldb/source/Interpreter/CommandReturnObject.cpp
> > index 77d94bd9389c3..980d39bfb1681 100644
> > --- a/lldb/source/Interpreter/CommandReturnObject.cpp
> > +++ b/lldb/source/Interpreter/CommandReturnObject.cpp
> > @@ -46,6 +46,8 @@ CommandReturnObject::CommandReturnObject(bool colors)
> >   m_interactive(true) {}
> >
> > void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
> > +  SetStatus(eReturnStatusFailed);
> > +
> >   if (!format)
> > return;
> >   va_list args;
> > @@ -100,6 +102,7 @@ void CommandReturnObject::AppendWarning(llvm::StringRef 
> > in_string) {
> > void CommandReturnObject::AppendError(llvm::StringRef in_string) {
> >   if (in_string.empty())
> > return;
> > +  SetStatus(eReturnStatusFailed);
> >   error(GetErrorStream()) << in_string.rtrim() << '\n';
> > }
> >
> > @@ -116,7 +119,6 @@ void CommandReturnObject::SetError(llvm::StringRef 
> > error_str) {
> > return;
> >
> >   AppendError(error_str);
> > -  SetStatus(eReturnStatusFailed);
> > }
> >
> > // Similar to AppendError, but do not prepend 'Status: ' to message, and 
> > don't
> > @@ -126,6 +128,7 @@ void 
> > CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
> >   if (in_string.empty())
> > return;
> >   GetErrorStream() << in_string;
> > +  SetStatus(eReturnStatusFailed);
> > }
> >
> > void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = 
> > status; }
> >
> > diff  --git 
> > a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> >  
> > b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> > index 5ec46c175e621..7acf3a4098756 100644
> > --- 
> > a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> > +++ 
> > b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> > @@ -41,13 +41,18 @@ def test_register_commands(self):
> > self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
> > substrs=['registers were unavailable'], matching=False)
> >
> > +all_registers = self.res.GetOutput()
> > +
> > if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
> > self.runCmd("register read xmm0")
> > -self.runCmd("register read ymm15")  # may be available
> > -self.runCmd("register read bnd0")  # may be available
> > +if "ymm15 = " in all_registers:
> > +  self.runCmd("register read ymm15")  # may be available
> > +if

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

2021-06-09 Thread Jan Kratochvil via Phabricator via lldb-commits

jankratochvil updated this revision to Diff 350900.
jankratochvil edited the summary of this revision.
jankratochvil added a comment.

This patch now requires: D101236 , D103910 
 and D103966 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101237

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/test/Shell/Expr/no_unique_address.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

Index: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -1603,12 +1603,16 @@
   } else {
 addUInt(MemberDie, dwarf::DW_AT_data_bit_offset, None, Offset);
   }
+  assert(!(DT->getFlags() & DINode::FlagIsZeroSize) &&
+ "bitfields cannot have [[no_unique_address]]");
 } else {
   // This is not a bitfield.
   OffsetInBytes = DT->getOffsetInBits() / 8;
   if (AlignInBytes)
 addUInt(MemberDie, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
 AlignInBytes);
+  if (DT->getFlags() & DINode::FlagIsZeroSize)
+addUInt(MemberDie, dwarf::DW_AT_byte_size, None, 0);
 }
 
 if (DD->getDwarfVersion() <= 2) {
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -843,11 +843,10 @@
CompilerType *child_type = nullptr);
 
   // Modifying RecordType
-  static clang::FieldDecl *AddFieldToRecordType(const CompilerType &type,
-llvm::StringRef name,
-const CompilerType &field_type,
-lldb::AccessType access,
-uint32_t bitfield_bit_size);
+  static clang::FieldDecl *
+  AddFieldToRecordType(const CompilerType &type, llvm::StringRef name,
+   const CompilerType &field_type, lldb::AccessType access,
+   uint32_t bitfield_bit_size, bool IsZeroSize = false);
 
   static void BuildIndirectFields(const CompilerType &type);
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7164,7 +7164,7 @@
 clang::FieldDecl *TypeSystemClang::AddFieldToRecordType(
 const CompilerType &type, llvm::StringRef name,
 const CompilerType &field_clang_type, AccessType access,
-uint32_t bitfield_bit_size) {
+uint32_t bitfield_bit_size, bool IsZeroSize) {
   if (!type.IsValid() || !field_clang_type.IsValid())
 return nullptr;
   TypeSystemClang *ast =
@@ -7196,6 +7196,8 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+if (IsZeroSize)
+  field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1264,8 +1264,10 @@
   if (location_type == PDB_LocType::ThisRel)
 bit_size *= 8;
 
+  bool IsZeroSize = false; // FIXME: Is there [[no_unique_address]] in PDB?
   auto decl = TypeSystemClang::AddFieldToRecordType(
-  record_type, member_name.c_str(), member_comp_type, access, bit_size);
+  record_type, member_name.c_str()

Re: [Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error

2021-06-09 Thread Jim Ingham via lldb-commits

Thanks!

Jim

> On Jun 9, 2021, at 1:44 AM, David Spickett  wrote:
> 
> I figured something would fail. I've reverted and will reland with fixed 
> tests.
> 
> On Tue, 8 Jun 2021 at 22:17, Jim Ingham  wrote:
>> 
>> Hey, David,
>> 
>> This commit seems to have caused a new failure in TestRegisters.py, e.g.:
>> 
>> http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/
>> 
>> Do you have time to take a look?
>> 
>> Jim
>> 
>> 
>> 
>> 
>>> On Jun 8, 2021, at 1:41 AM, David Spickett via lldb-commits 
>>>  wrote:
>>> 
>>> 
>>> Author: David Spickett
>>> Date: 2021-06-08T09:41:07+01:00
>>> New Revision: e05b03cf4f45ac5ee63c59a3464e7d484884645c
>>> 
>>> URL: 
>>> https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c
>>> DIFF: 
>>> https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c.diff
>>> 
>>> LOG: [lldb] Set return status to failed when adding a command error
>>> 
>>> There is a common pattern:
>>> result.AppendError(...);
>>> result.SetStatus(eReturnStatusFailed);
>>> 
>>> I found that some commands don't actually "fail" but only
>>> print "error: ..." because the second line got missed.
>>> 
>>> This can cause you to miss a failed command when you're
>>> using the Python interface during testing.
>>> (and produce some confusing script results)
>>> 
>>> I did not find any place where you would want to add
>>> an error without setting the return status, so just
>>> set eReturnStatusFailed whenever you add an error to
>>> a command result.
>>> 
>>> This change does not remove any of the now redundant
>>> SetStatus. This should allow us to see if there are any
>>> tests that have commands unexpectedly fail with this change.
>>> (the test suite passes for me but I don't have access to all
>>> the systems we cover so there could be some corner cases)
>>> 
>>> Some tests that failed on x86 and AArch64 have been modified
>>> to work with the new behaviour.
>>> 
>>> Differential Revision: https://reviews.llvm.org/D103701
>>> 
>>> Added:
>>>   lldb/test/Shell/Commands/command-backtrace-parser-1.test
>>>   lldb/test/Shell/Commands/command-backtrace-parser-2.test
>>> 
>>> Modified:
>>>   lldb/source/Interpreter/CommandReturnObject.cpp
>>>   lldb/test/API/commands/register/register/register_command/TestRegisters.py
>>> 
>>> Removed:
>>>   lldb/test/Shell/Commands/command-backtrace.test
>>> 
>>> 
>>> 
>>> diff  --git a/lldb/source/Interpreter/CommandReturnObject.cpp 
>>> b/lldb/source/Interpreter/CommandReturnObject.cpp
>>> index 77d94bd9389c3..980d39bfb1681 100644
>>> --- a/lldb/source/Interpreter/CommandReturnObject.cpp
>>> +++ b/lldb/source/Interpreter/CommandReturnObject.cpp
>>> @@ -46,6 +46,8 @@ CommandReturnObject::CommandReturnObject(bool colors)
>>>  m_interactive(true) {}
>>> 
>>> void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
>>> +  SetStatus(eReturnStatusFailed);
>>> +
>>>  if (!format)
>>>return;
>>>  va_list args;
>>> @@ -100,6 +102,7 @@ void CommandReturnObject::AppendWarning(llvm::StringRef 
>>> in_string) {
>>> void CommandReturnObject::AppendError(llvm::StringRef in_string) {
>>>  if (in_string.empty())
>>>return;
>>> +  SetStatus(eReturnStatusFailed);
>>>  error(GetErrorStream()) << in_string.rtrim() << '\n';
>>> }
>>> 
>>> @@ -116,7 +119,6 @@ void CommandReturnObject::SetError(llvm::StringRef 
>>> error_str) {
>>>return;
>>> 
>>>  AppendError(error_str);
>>> -  SetStatus(eReturnStatusFailed);
>>> }
>>> 
>>> // Similar to AppendError, but do not prepend 'Status: ' to message, and 
>>> don't
>>> @@ -126,6 +128,7 @@ void 
>>> CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
>>>  if (in_string.empty())
>>>return;
>>>  GetErrorStream() << in_string;
>>> +  SetStatus(eReturnStatusFailed);
>>> }
>>> 
>>> void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = 
>>> status; }
>>> 
>>> diff  --git 
>>> a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
>>>  
>>> b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
>>> index 5ec46c175e621..7acf3a4098756 100644
>>> --- 
>>> a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
>>> +++ 
>>> b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
>>> @@ -41,13 +41,18 @@ def test_register_commands(self):
>>>self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
>>>substrs=['registers were unavailable'], matching=False)
>>> 
>>> +all_registers = self.res.GetOutput()
>>> +
>>>if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
>>>self.runCmd("register read xmm0")
>>> -self.runCmd("register read ymm15")  # may be available
>>> -self.runCmd("register read bnd0")  # may be available
>>> +if "ymm15 = " in all_regis

[Lldb-commits] [PATCH] D103575: Allow signposts to take advantage of deferred string substitution

2021-06-09 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added a comment.

In D103575#2804151 , @aprantl wrote:

> I'm not sure I fully understand the suggestion:
>
>> I think we should just remove the functionality form the timer class again. 
>> I only added it there because of the macro.
>
> ... and replace its uses with what?

I'm suggesting we no longer emit a signpost from the `lldb_private::Timer` 
class and instead exclusively emit a signpost from the `LLDB_SCOPED_TIMER` 
macro. We barely have any places where someone creates a timer directly and 
this would help discourage that further. The only reason it's currently in the 
timer class it because it seemed cleaner than doing it in the macro directly. 
Since we now have to emit the signpost from the `LLDB_SCOPED_TIMER` macro 
anyway, there's no reason to keep the functionality in the Timer class.

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

https://reviews.llvm.org/D103575

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

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

2021-06-09 Thread Vedant Kumar via Phabricator via lldb-commits

vsk added a comment.

In D103588#2806894 , @wallace wrote:

> I've been thinking about what you said and I'm having second thoughts on my 
> implementation. I'll share more context:
>
> - I want to work in the short term on reverse debugging and reconstruction of 
> stack traces, for that i'll need to know the instruction type of each 
> instruction in the trace, which will be used as part of some heuristics to 
> identify calls and returns between functions.
> - A future application that we plan to work on is adding profiling 
> information to the instructions
> - Right now the intel-pt plugin is storing the decoded instructions in a 
> vector, which works for small traces but wont' for gigantic traces. I imagine 
> that I'll end up removing that vector and make the TraverseInstruction API 
> decode instructions in place keeping one instruction in memory at a time 
> within the intel pt plugin for a given traversal. For that I'll need 
> accessors that can provide information of the current Instruction. As there 
> could be two or more concurrent traversals happening at the same time (I'm 
> trying to be generic here), it might make sense to create an abstract class 
> TraceInstruction that can be extended by each plug-in and implement its 
> getters.
>
> I'm thinking about something like this
>
>   class TraceInstruction {
> virtual lldb::addr_t GetLoadAddress() = 0;
> virtual TraceInstructionType() GetInstructionType() = 0;
> virtual uint64_t GetTimestamp() = 0;
> ... anything that can appear in the future 
>   };
>
> and have no members, leaving to each plug-in the decision of which of those 
> methods to implement and how.
>
> What do you think of this? I think this incorporates your feedback.

Thanks for the context.

I'm not convinced that retaining only 1 decoded instruction in memory at a time 
(under a TraverseInstructions callback) will be sufficient for the use cases 
you've outlined. Let's say the user sets a few breakpoints, then repeatedly 
does "rev-continue" followed by "bt". If lldb only holds a max of 1 decoded 
instruction in memory, it would need to repeatedly decode parts of the trace to 
evaluate those user commands, which would be slow as you pointed out earlier. 
OTOH there's not enough memory to retain all of the decoded instructions.

It seems like we need a way for generic trace analyses to request _batches_ of 
decoded trace data at a time, lazily demanding more from the trace plugin only 
if the analysis requires it to proceed, while freeing any stale decoded data 
it's not using.

Moreover, there doesn't seem to be a hard requirement that each trace plugin 
specialize a TraceInstruction class. For example, to support that 
breakpoint/rev-continue/bt workflow, you really only need a list of the places 
where _control flow changed_. So the generic layer could request that data from 
a trace plugin ('plugin->decodeControlFlowChanges(startPos, stopPos)'), then 
query more specific APIs that answer questions about how many control flow 
changes there were, and what the inst type + load address was at each control 
flow change point. Each plugin implementation can pick the most efficient way 
to implement those narrower APIs, instead of trying to cram all potentially 
useful information into a single TraceInst class.

The reason why I believe this to be such a crucial part of the design is 
because of the scaling problem I mentioned earlier. A single 1.5GHz core 
running at IPC=2 yields 3*10^9 instructions/second, so we hit "gigantic" all 
too quickly!

Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:117
+  case ptic_far_call:
+return lldb::eTraceInstructionFarCall;
+  case ptic_far_return:

Are all of these instruction types both necessary and generic? E.g. does having 
FarCall in addition to Call assist with backtrace reconstruction, and if so, 
what does FarCall mean precisely?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

2021-06-09 Thread walter erquinigo via Phabricator via lldb-commits

wallace added a comment.

Btw, thanks for the conversation. This is being really helpful.

> I'm not convinced that retaining only 1 decoded instruction in memory at a 
> time (under a TraverseInstructions callback) will be sufficient for the use 
> cases you've outlined. Let's say the user sets a few breakpoints, then 
> repeatedly does "rev-continue" followed by "bt". If lldb only holds a max of 
> 1 decoded instruction in memory, it would need to repeatedly decode parts of 
> the trace to evaluate those user commands, which would be slow as you pointed 
> out earlier. OTOH there's not enough memory to retain all of the decoded 
> instructions.
>
> It seems like we need a way for generic trace analyses to request _batches_ 
> of decoded trace data at a time, lazily demanding more from the trace plugin 
> only if the analysis requires it to proceed, while freeing any stale decoded 
> data it's not using.
>
> Moreover, there doesn't seem to be a hard requirement that each trace plugin 
> specialize a TraceInstruction class. For example, to support that 
> breakpoint/rev-continue/bt workflow, you really only need a list of the 
> places where _control flow changed_. So the generic layer could request that 
> data from a trace plugin ('plugin->decodeControlFlowChanges(startPos, 
> stopPos)'), then query more specific APIs that answer questions about how 
> many control flow changes there were, and what the inst type + load address 
> was at each control flow change point. Each plugin implementation can pick 
> the most efficient way to implement those narrower APIs, instead of trying to 
> cram all potentially useful information into a single TraceInst class.
>
> The reason why I believe this to be such a crucial part of the design is 
> because of the scaling problem I mentioned earlier. A single 1.5GHz core 
> running at IPC=2 yields 3*10^9 instructions/second, so we hit "gigantic" all 
> too quickly!

Yes, I agree with everything you said. It will really make the implementation 
simpler and more generic if we have these accessors based on indices instead of 
a struct/class, and we let each plug-in manage its data and its cache their own 
way. 
`decodeControlFlowChanges` is definitely a good idea, and other similar 
interfaces could be created following the same approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [lldb] db93e4e - Revert "[lldb] Set return status to failed when adding a command error"

[Lldb-commits] [PATCH] D103701: [lldb] Set return status to failed when adding a command error

[Lldb-commits] [PATCH] D103483: [lldb] Convert the default constructor’s member initializers into default member initializers

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

Re: [Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

Re: [Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error

[Lldb-commits] [PATCH] D103575: Allow signposts to take advantage of deferred string substitution

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

15 matches

Site Navigation

Mail list logo

Footer information