[Lldb-commits] [PATCH] D135648: [lldb] Add matching based on Python callbacks for data formatters.

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe created this revision.
jgorbe added reviewers: jingham, labath.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.

This patch adds a new matching method for data formatters, in addition
to the existing exact typename and regex-based matching. The new method
allows users to specify the name of a Python callback function that
takes a `SBType` object and decides whether the type is a match or not.

Here is an overview of the changes performed:

- Add a new `eFormatterMatchCallback` matching type, and logic to handle it in 
`TypeMatcher` and `SBTypeNameSpecifier`.

- Extend `FormattersMatchCandidate` instances with a pointer to the current 
`ScriptInterpreter` and the `TypeImpl` corresponding to the candidate type, so 
we can run registered callbacks and pass the type to them. All matcher search 
functions now receive a `FormattersMatchCandidate` instead of a type name.

- Add some glue code to ScriptInterpreterPython and the SWIG bindings to allow 
calling a formatter matching callback. Most of this code is modeled after the 
equivalent code for watchpoint callback functions.

- Add an API test for the new callback-based matching feature.

For more context, please check the RFC thread where this feature was
originally discussed:
https://discourse.llvm.org/t/rfc-python-callback-for-data-formatters-type-matching/64204/11


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135648

Files:
  lldb/bindings/python/python-swigsafecast.swig
  lldb/bindings/python/python-wrapper.swig
  lldb/include/lldb/API/SBType.h
  lldb/include/lldb/DataFormatters/DataVisualization.h
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormatManager.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  lldb/include/lldb/DataFormatters/TypeCategory.h
  lldb/include/lldb/DataFormatters/TypeCategoryMap.h
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Target/Language.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/SBTypeNameSpecifier.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/DataFormatters/DataVisualization.cpp
  lldb/source/DataFormatters/FormatManager.cpp
  lldb/source/DataFormatters/TypeCategory.cpp
  lldb/source/DataFormatters/TypeCategoryMap.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
  lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Language.cpp
  lldb/test/API/functionalities/data-formatter/callback-matching/Makefile
  
lldb/test/API/functionalities/data-formatter/callback-matching/TestDataFormatterCallbackMatching.py
  
lldb/test/API/functionalities/data-formatter/callback-matching/formatters_with_callback.py
  lldb/test/API/functionalities/data-formatter/callback-matching/main.cpp
  lldb/unittests/DataFormatter/FormattersContainerTest.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -67,6 +67,12 @@
   return false;
 }
 
+bool lldb_private::LLDBSwigPythonFormatterCallbackFunction(
+const char *python_function_name, const char *session_dictionary_name,
+lldb::TypeImplSP type_impl_sp) {
+  return false;
+}
+
 bool lldb_private::LLDBSwigPythonCallTypeScript(
 const char *python_function_name, const void *session_dictionary,
 const lldb::ValueObjectSP &valobj_sp, void **pyfunct_wrapper,
Index: lldb/unittests/DataFormatter/FormattersContainerTest.cpp
===
--- lldb/unittests/DataFormatter/FormattersContainerTest.cpp
+++ lldb/unittests/DataFormatter/FormattersContainerTest.cpp
@@ -7,12 +7,20 @@
 //===--===//
 
 #include "lldb/DataFormatters/FormattersContainer.h"
+#include "lldb/DataFormatters/FormatClasses.h"
 
 #include "gtest/gtest.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
+// Creates a dummy candidate with just a type name in order to test the string
+// matching (exact name match and regex match) paths.
+FormattersMatchCandidate CandidateFromTypeName(const char *type_name) {
+  return FormattersMatchCandidate(ConstString(type_name), nullptr, TypeImpl(),
+  FormattersMatchCandidate::Flags());
+}
+
 // All the prefixes that the exact name matching will strip from the type.
 static const std::vector exact_name_prefixes = {
 "", // no prefix.
@@ -25,63 +33,63 @@
 SCOPED_TRACE("Prefix: " + prefix);

[Lldb-commits] [PATCH] D135461: [LLDB] Fix crash when printing a struct with a static wchar_t member

[David Spickett via Phabricator via lldb-commits]

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

> although those changes don't actually test the code path changed here

Yeah I'm just cargo culting on that one so it doesn't look strange that a few 
are missing.

If we're going to change the suffix that can be done elsewhere and reviewed by 
someone who understands them better than me :)

Not crashing is an improvement in itself, LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135461

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

[Lldb-commits] [PATCH] D134991: [lldb] Add diagnostics

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Utility/Diagnostics.cpp:43
+  std::lock_guard guard(m_callbacks_mutex);
+  m_callbacks.push_back(callback);
+}

DavidSpickett wrote:
> Is it worth adding an assert that the callback is not already in the list?
> 
> (and below, that the callback is in fact in the list)
Is this still relevant?


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

https://reviews.llvm.org/D134991

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

[Lldb-commits] [PATCH] D135621: [lldb] Add an always-on log channel

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure if this will do what you wanted it to do. Despite the title, this 
patch is not adding a new log *channel* -- it is adding a new log *category* to 
the "lldb" channel. Our logging infra, perhaps somewhat misleadingly, manages 
all of the log state on a per-channel basis. This means that it is not e.g. 
possible to redirect one log category to a different sink than a different 
category in the same channel. I haven't checked this, but I suspect that for 
this reason, any explicit "log enable" commands by the user will redirect this 
special channel into the new (user-provided) destination. If this is not what 
you intended to do then, you probably ought to add a new log channel, for real.

That said, without knowing what kind of information do you want to log here, I 
am wondering if the same kind of information could be gathered by reusing some 
existing data collection mechanism, like session transcripts, or the 
yet-to-be-implemented telemetry data.


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

https://reviews.llvm.org/D135621

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

[Lldb-commits] [PATCH] D135631: [lldb] Copy log files into diagnostic directory (RFC)

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

seems fine to me.




Comment at: lldb/test/Shell/Diagnostics/TestCopyLogs.test:3
+# RUN: mkdir -p %t
+# The "ll''db" below is not a typo but a way to prevent lit from substituting 
'lldb'.
+# RUN: %lldb -o 'log enable ll''db commands -f %t/commands.log' -o 
'diagnostics dump -d %t/diags'

That's cute, but I suspect windows will have a problem with that. `-s %s` (and 
putting the commands in this file) would be safer.


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

https://reviews.llvm.org/D135631

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

[Lldb-commits] [PATCH] D134962: [LLDB] Change pointer to ref in EmulateInstruction::ReadRegister methods

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

No problem, thanks for the reviews!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134962

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

[Lldb-commits] [lldb] 2a627e2 - [LLDB] Change pointer to ref in EmulateInstruction::ReadRegister methods

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-10-11T12:31:55Z
New Revision: 2a627e2ad1d80b660fcb02dde85b346f61193ade

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

LOG: [LLDB] Change pointer to ref in EmulateInstruction::ReadRegister methods

ReadRegister and ReadRegisterAsUnsigned are always passed valid pointers,
so the parameter should be a ref to make the intent clear.

Reviewed By: clayborg

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

Added: 


Modified: 
lldb/include/lldb/Core/EmulateInstruction.h
lldb/source/Core/EmulateInstruction.cpp
lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/EmulateInstruction.h 
b/lldb/include/lldb/Core/EmulateInstruction.h
index dc15fe25cb3f2..444059d94b6e1 100644
--- a/lldb/include/lldb/Core/EmulateInstruction.h
+++ b/lldb/include/lldb/Core/EmulateInstruction.h
@@ -388,9 +388,9 @@ class EmulateInstruction : public PluginInterface {
uint32_t reg_num, std::string 
®_name);
 
   // RegisterInfo variants
-  bool ReadRegister(const RegisterInfo *reg_info, RegisterValue ®_value);
+  bool ReadRegister(const RegisterInfo ®_info, RegisterValue ®_value);
 
-  uint64_t ReadRegisterUnsigned(const RegisterInfo *reg_info,
+  uint64_t ReadRegisterUnsigned(const RegisterInfo ®_info,
 uint64_t fail_value, bool *success_ptr);
 
   bool WriteRegister(const Context &context, const RegisterInfo *ref_info,

diff  --git a/lldb/source/Core/EmulateInstruction.cpp 
b/lldb/source/Core/EmulateInstruction.cpp
index b497b0254a12f..54d4d93fb4d06 100644
--- a/lldb/source/Core/EmulateInstruction.cpp
+++ b/lldb/source/Core/EmulateInstruction.cpp
@@ -72,10 +72,10 @@ EmulateInstruction::FindPlugin(const ArchSpec &arch,
 
 EmulateInstruction::EmulateInstruction(const ArchSpec &arch) : m_arch(arch) {}
 
-bool EmulateInstruction::ReadRegister(const RegisterInfo *reg_info,
+bool EmulateInstruction::ReadRegister(const RegisterInfo ®_info,
   RegisterValue ®_value) {
   if (m_read_reg_callback != nullptr)
-return m_read_reg_callback(this, m_baton, reg_info, reg_value);
+return m_read_reg_callback(this, m_baton, ®_info, reg_value);
   return false;
 }
 
@@ -84,7 +84,7 @@ bool EmulateInstruction::ReadRegister(lldb::RegisterKind 
reg_kind,
   RegisterValue ®_value) {
   llvm::Optional reg_info = GetRegisterInfo(reg_kind, reg_num);
   if (reg_info)
-return ReadRegister(&(*reg_info), reg_value);
+return ReadRegister(*reg_info, reg_value);
   return false;
 }
 
@@ -100,7 +100,7 @@ uint64_t 
EmulateInstruction::ReadRegisterUnsigned(lldb::RegisterKind reg_kind,
   return fail_value;
 }
 
-uint64_t EmulateInstruction::ReadRegisterUnsigned(const RegisterInfo *reg_info,
+uint64_t EmulateInstruction::ReadRegisterUnsigned(const RegisterInfo ®_info,
   uint64_t fail_value,
   bool *success_ptr) {
   RegisterValue reg_value;

diff  --git a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp 
b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
index d71fff36e5613..14a30a5857fcf 100644
--- a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
+++ b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
@@ -2620,7 +2620,7 @@ bool EmulateInstructionARM::EmulateVPUSH(const uint32_t 
opcode,
   GetRegisterInfo(eRegisterKindDWARF, start_reg + d + i);
   context.SetRegisterToRegisterPlusOffset(*dwarf_reg, *sp_reg, addr - sp);
   // uint64_t to accommodate 64-bit registers.
-  uint64_t reg_value = ReadRegisterUnsigned(&(*dwarf_reg), 0, &success);
+  uint64_t reg_value = ReadRegisterUnsigned(*dwarf_reg, 0, &success);
   if (!success)
 return false;
   if (!MemAWrite(context, addr, reg_value, reg_byte_size))

diff  --git a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp 
b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
index 92af4da23fb49..e8a77a3c86519 100644
--- a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -824,7 +824,7 @@ bool EmulateInstructionARM64::EmulateLDPSTP(const uint32_t 
opcode) {
 context_t2.SetRegisterToRegisterPlusOffset(*reg_info_Rt2, *reg_info_base,
size);
 
-if (!ReadRegister(&(*reg_info_Rt), 

[Lldb-commits] [PATCH] D134962: [LLDB] Change pointer to ref in EmulateInstruction::ReadRegister methods

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2a627e2ad1d8: [LLDB] Change pointer to ref in 
EmulateInstruction::ReadRegister methods (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134962

Files:
  lldb/include/lldb/Core/EmulateInstruction.h
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp

Index: lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
===
--- lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
+++ lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
@@ -1160,7 +1160,7 @@
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(&(*reg_info_base), data_src))
+if (!ReadRegister(*reg_info_base, data_src))
   return false;
 
 if (data_src.GetAsMemoryData(&(*reg_info_src), buffer,
Index: lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
===
--- lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
+++ lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
@@ -1266,7 +1266,7 @@
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(&(*reg_info_base), data_src))
+if (!ReadRegister(*reg_info_base, data_src))
   return false;
 
 if (data_src.GetAsMemoryData(&(*reg_info_src), buffer,
@@ -1526,7 +1526,7 @@
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(&(*reg_info_base), data_src))
+if (!ReadRegister(*reg_info_base, data_src))
   return false;
 
 if (data_src.GetAsMemoryData(®_info_src, buffer, reg_info_src.byte_size,
@@ -1608,7 +1608,7 @@
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(&(*reg_info_base), data_src))
+if (!ReadRegister(*reg_info_base, data_src))
   return false;
 
 if (data_src.GetAsMemoryData(&(*reg_info_src), buffer,
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -824,7 +824,7 @@
 context_t2.SetRegisterToRegisterPlusOffset(*reg_info_Rt2, *reg_info_base,
size);
 
-if (!ReadRegister(&(*reg_info_Rt), data_Rt))
+if (!ReadRegister(*reg_info_Rt, data_Rt))
   return false;
 
 if (data_Rt.GetAsMemoryData(&(*reg_info_Rt), buffer, reg_info_Rt->byte_size,
@@ -834,7 +834,7 @@
 if (!WriteMemory(context_t, address + 0, buffer, reg_info_Rt->byte_size))
   return false;
 
-if (!ReadRegister(&(*reg_info_Rt2), data_Rt2))
+if (!ReadRegister(*reg_info_Rt2, data_Rt2))
   return false;
 
 if (data_Rt2.GetAsMemoryData(&(*reg_info_Rt2), buffer,
@@ -995,7 +995,7 @@
 context.SetRegisterToRegisterPlusOffset(*reg_info_Rt, *reg_info_base,
 postindex ? 0 : offset);
 
-if (!ReadRegister(&(*reg_info_Rt), data_Rt))
+if (!ReadRegister(*reg_info_Rt, data_Rt))
   return false;
 
 if (data_Rt.GetAsMemoryData(&(*reg_info_Rt), buffer, reg_info_Rt->byte_size,
Index: lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
@@ -2620,7 +2620,7 @@
   GetRegisterInfo(eRegisterKindDWARF, start_reg + d + i);
   context.SetRegisterToRegisterPlusOffset(*dwarf_reg, *sp_reg, addr - sp);
   // uint64_t to accommodate 64-bit registers.
-  uint64_t reg_value = ReadRegisterUnsigned(&(*dwarf_reg), 0, &success);
+  uint64_t reg_value = ReadRegisterUnsigned(*dwarf_reg, 0, &success);
   if (!success)
 return false;
   if (!MemAWrite(context, addr, reg_value, reg_byte_size))
Index: lldb/source/Core/EmulateInstruction.cpp
===
--- lldb/source/Core/EmulateInstruction.cpp
+++ lldb/source/Core/EmulateInstruction.cpp
@@ -72,10 +72,10 @@
 
 EmulateInstruction::EmulateInstruction(const ArchSpec &arch) : m_arch(arch) {}
 
-bool EmulateInstruction::ReadRegister(const RegisterInfo *reg_info,
+bool EmulateInstruction::ReadRegister(const RegisterInfo ®_info,
   RegisterVa

[Lldb-commits] [PATCH] D135622: [lldb] Add a "diagnostics dump" command

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Commands/CommandObjectDiagnostics.cpp:85
+  result.AppendError(llvm::toString(directory.takeError()));
+  result.SetStatus(eReturnStatusFailed);
+  return result.Succeeded();

AppendError sets the status for you now.



Comment at: lldb/source/Commands/CommandObjectDiagnostics.cpp:107
+CommandInterpreter &interpreter)
+: CommandObjectMultiword(interpreter, "log",
+ "Commands controlling LLDB diagnostics.",

Is `"log"` correct here?



Comment at: lldb/source/Commands/Options.td:348
+  def diagnostics_dump_directory : Option<"directory", "d">, Group<1>,
+Arg<"Path">, Desc<"Dump the diagnostics to the given directory.">;
+}

Worth clarifying whether the path can be absolute or relative to the current 
working dir?

I'd guess either works.



Comment at: lldb/source/Utility/Diagnostics.cpp:62
 stream << "unable to create diagnostic dir: "
-   << toString(errorCodeToError(ec)) << '\n';
+   << toString(diagnostics_dir.takeError()) << '\n';
 return false;

You `std::move`'d some errors above does that apply here?

(I'm not sure exactly what requires it and what doesn't)



Comment at: lldb/source/Utility/Diagnostics.cpp:70
 
-  Error error = Create(FileSpec(diagnostics_dir.str()));
+  Error error = Create(*diagnostics_dir);
   if (error) {

Silly question, is it intentional that we write out files after saying they 
have been written?

Of course it makes sense to print something up front, otherwise the error here 
would come out of the blue, but perhaps "LLDB diagnostics will be written 
to..." instead?

(this is probably better changed in that earlier patch not sure if that went in 
yet)



Comment at: lldb/test/Shell/Diagnostics/TestDump.test:1
+# Check that the diagnostics dump command uses the correct directory.
+

"correct directory and creates a new one if needed." ?


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

https://reviews.llvm.org/D135622

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

[Lldb-commits] [PATCH] D135631: [lldb] Copy log files into diagnostic directory (RFC)

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

So either way, if someone was running with no logs enabled we would have to ask 
them to enable them then re-run and get a new set of diagnostics (which is fine 
as is , not a criticism).

Option 1 they would need to add `-o "log enable gdb-remote packets -f "`.
Option 2 they would need to add `-o "log enable gdb-remote packets"`.

So a few made up file names vs a performance decrease I'd take the former 
(without any measurement of the latter).

Given that diagnostics are going to be written on crash is it any more "safe" 
to just copy a file than rely on a ring buffer in memory? I don't think it is, 
unless the crash itself was in logging itself and all bets are off then in any 
case.

Another disadvantage to the ring buffer is a size limit, so you could lose 
early log data from a long session before you get to the crash. However you 
could work around that by sending each log to a file, once you had realised 
that the buffer didn't go far back enough (and we're asking people to re-run 
the reproducer in any case).

Saving logs that are already written to a file seems logical and isn't much 
harder to guide people how to do, vs. the ring buffer.


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

https://reviews.llvm.org/D135631

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

[Lldb-commits] [PATCH] D135631: [lldb] Copy log files into diagnostic directory (RFC)

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Also is the buffer option the same as "circular" here?

  -h  ( --log-handler  )
   Specify a log handler which determines where log messages are written.
   Values: default | stream | circular | os

So we could have diagnostics also save anything kept in "circular", on the off 
chance that writing to a file isn't possible.


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

https://reviews.llvm.org/D135631

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

[Lldb-commits] [lldb] 81832af - [LLDB] Switch to RegisterInfo& for EmulateInstruction::WriteRegister

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-10-11T13:24:55Z
New Revision: 81832afc04e130a386a89f57d3f4f2d7f19006f7

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

LOG: [LLDB] Switch to RegisterInfo& for EmulateInstruction::WriteRegister

WriteRegister and WriteRegisterUnsigned were never pased nullptr,
and only one of them appeared to handle it. Switch to ref to make
the intent clear.

Depends on D134962

Reviewed By: clayborg

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

Added: 


Modified: 
lldb/include/lldb/Core/EmulateInstruction.h
lldb/source/Core/EmulateInstruction.cpp
lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/EmulateInstruction.h 
b/lldb/include/lldb/Core/EmulateInstruction.h
index 444059d94b6e1..68b40673faab2 100644
--- a/lldb/include/lldb/Core/EmulateInstruction.h
+++ b/lldb/include/lldb/Core/EmulateInstruction.h
@@ -393,11 +393,11 @@ class EmulateInstruction : public PluginInterface {
   uint64_t ReadRegisterUnsigned(const RegisterInfo ®_info,
 uint64_t fail_value, bool *success_ptr);
 
-  bool WriteRegister(const Context &context, const RegisterInfo *ref_info,
+  bool WriteRegister(const Context &context, const RegisterInfo &ref_info,
  const RegisterValue ®_value);
 
   bool WriteRegisterUnsigned(const Context &context,
- const RegisterInfo *reg_info, uint64_t reg_value);
+ const RegisterInfo ®_info, uint64_t reg_value);
 
   // Register kind and number variants
   bool ReadRegister(lldb::RegisterKind reg_kind, uint32_t reg_num,

diff  --git a/lldb/source/Core/EmulateInstruction.cpp 
b/lldb/source/Core/EmulateInstruction.cpp
index 54d4d93fb4d06..73cbcc722ef64 100644
--- a/lldb/source/Core/EmulateInstruction.cpp
+++ b/lldb/source/Core/EmulateInstruction.cpp
@@ -112,10 +112,10 @@ uint64_t EmulateInstruction::ReadRegisterUnsigned(const 
RegisterInfo ®_info,
 }
 
 bool EmulateInstruction::WriteRegister(const Context &context,
-   const RegisterInfo *reg_info,
+   const RegisterInfo ®_info,
const RegisterValue ®_value) {
   if (m_write_reg_callback != nullptr)
-return m_write_reg_callback(this, m_baton, context, reg_info, reg_value);
+return m_write_reg_callback(this, m_baton, context, ®_info, reg_value);
   return false;
 }
 
@@ -125,7 +125,7 @@ bool EmulateInstruction::WriteRegister(const Context 
&context,
const RegisterValue ®_value) {
   llvm::Optional reg_info = GetRegisterInfo(reg_kind, reg_num);
   if (reg_info)
-return WriteRegister(context, &(*reg_info), reg_value);
+return WriteRegister(context, *reg_info, reg_value);
   return false;
 }
 
@@ -137,19 +137,17 @@ bool EmulateInstruction::WriteRegisterUnsigned(const 
Context &context,
   if (reg_info) {
 RegisterValue reg_value;
 if (reg_value.SetUInt(uint_value, reg_info->byte_size))
-  return WriteRegister(context, &(*reg_info), reg_value);
+  return WriteRegister(context, *reg_info, reg_value);
   }
   return false;
 }
 
 bool EmulateInstruction::WriteRegisterUnsigned(const Context &context,
-   const RegisterInfo *reg_info,
+   const RegisterInfo ®_info,
uint64_t uint_value) {
-  if (reg_info != nullptr) {
-RegisterValue reg_value;
-if (reg_value.SetUInt(uint_value, reg_info->byte_size))
-  return WriteRegister(context, reg_info, reg_value);
-  }
+  RegisterValue reg_value;
+  if (reg_value.SetUInt(uint_value, reg_info.byte_size))
+return WriteRegister(context, reg_info, reg_value);
   return false;
 }
 

diff  --git a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp 
b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
index 14a30a5857fcf..a9ea14aee06a6 100644
--- a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
+++ b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
@@ -2713,7 +2713,7 @@ bool EmulateInstructionARM::EmulateVPOP(const uint32_t 
opcode,
   data = MemARead(context, addr, reg_byte_size, 0, &success);
   if (!success)
 return false;
-  if (!WriteRegisterUnsigned(context, &(*dwarf_reg), data))
+  if (!WriteRegisterUnsigned(context, *dwarf_reg, data))
 return false;
   addr += reg_byte_siz

[Lldb-commits] [PATCH] D134963: [LLDB] Switch to RegisterInfo& for EmulateInstruction::WriteRegister

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG81832afc04e1: [LLDB] Switch to RegisterInfo& for 
EmulateInstruction::WriteRegister (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134963

Files:
  lldb/include/lldb/Core/EmulateInstruction.h
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp

Index: lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
===
--- lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
+++ lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
@@ -1217,7 +1217,7 @@
 Context context;
 context.type = eContextRegisterLoad;
 
-return WriteRegister(context, &(*reg_info_src), data_src);
+return WriteRegister(context, *reg_info_src, data_src);
   }
 
   return false;
Index: lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
===
--- lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
+++ lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
@@ -1321,7 +1321,7 @@
 context.type = eContextPopRegisterOffStack;
 context.SetAddress(address);
 
-return WriteRegister(context, &(*reg_info_src), data_src);
+return WriteRegister(context, *reg_info_src, data_src);
   }
 
   return false;
@@ -1661,7 +1661,7 @@
 context.type = eContextPopRegisterOffStack;
 context.SetAddress(base_address);
 
-return WriteRegister(context, &(*reg_info_src), data_src);
+return WriteRegister(context, *reg_info_src, data_src);
   }
 
   return false;
@@ -1723,7 +1723,7 @@
 context.type = eContextPopRegisterOffStack;
 context.SetAddress(base_address + (i * 4));
 
-if (!WriteRegister(context, &(*reg_info_dst), data_dst))
+if (!WriteRegister(context, *reg_info_dst, data_dst))
   return false;
   }
 
Index: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
===
--- lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -876,7 +876,7 @@
 if (!vector && is_signed && !data_Rt.SignExtend(datasize))
   return false;
 
-if (!WriteRegister(context_t, &(*reg_info_Rt), data_Rt))
+if (!WriteRegister(context_t, *reg_info_Rt, data_Rt))
   return false;
 
 if (!rt_unknown) {
@@ -893,7 +893,7 @@
 if (!vector && is_signed && !data_Rt2.SignExtend(datasize))
   return false;
 
-if (!WriteRegister(context_t2, &(*reg_info_Rt2), data_Rt2))
+if (!WriteRegister(context_t2, *reg_info_Rt2, data_Rt2))
   return false;
   } break;
 
@@ -910,7 +910,7 @@
   context.type = eContextAdjustStackPointer;
 else
   context.type = eContextAdjustBaseRegister;
-WriteRegisterUnsigned(context, &(*reg_info_base), wb_address);
+WriteRegisterUnsigned(context, *reg_info_base, wb_address);
   }
   return true;
 }
@@ -1023,7 +1023,7 @@
   error) == 0)
   return false;
 
-if (!WriteRegister(context, &(*reg_info_Rt), data_Rt))
+if (!WriteRegister(context, *reg_info_Rt, data_Rt))
   return false;
 break;
   default:
@@ -1040,7 +1040,7 @@
   context.type = eContextAdjustBaseRegister;
 context.SetImmediateSigned(offset);
 
-if (!WriteRegisterUnsigned(context, &(*reg_info_base), address))
+if (!WriteRegisterUnsigned(context, *reg_info_base, address))
   return false;
   }
   return true;
Index: lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
@@ -2713,7 +2713,7 @@
   data = MemARead(context, addr, reg_byte_size, 0, &success);
   if (!success)
 return false;
-  if (!WriteRegisterUnsigned(context, &(*dwarf_reg), data))
+  if (!WriteRegisterUnsigned(context, *dwarf_reg, data))
 return false;
   addr += reg_byte_size;
 }
Index: lldb/source/Core/EmulateInstruction.cpp
===
--- lldb/source/Core/EmulateInstruction.cpp
+++ lldb/source/Core/EmulateInstruction.cpp
@@ -112,10 +112,10 @@
 }
 
 bool EmulateInstruction::WriteRegister(const Context &context,
-   const RegisterInfo *reg_info,
+   const RegisterInfo ®_info,
   

lldb-commits@lists.llvm.org

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-10-11T13:59:05Z
New Revision: c7ddbd62d811524726e6f8e293ab1576c5b289f3

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

LOG: [LLDB] Change RegisterValue::GetAsMemoryData to const RegisterInfo&

Most of the paths to this never passed nullptr intentionally. Those
that possibly could have were assuming it was not null elsehwere,
so would have crashed.

I've added asserts in those cases.

At least one case was relying on GetAsMemoryData to return an error
when it was given nullptr. So I've hoisted that error setting code
out into the caller.

Depends on D134963

Reviewed By: clayborg

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

Added: 


Modified: 
lldb/include/lldb/Utility/RegisterValue.h
lldb/source/Host/common/NativeRegisterContext.cpp
lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
lldb/source/Plugins/ABI/X86/ABISysV_i386.cpp
lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp
lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
lldb/source/Target/RegisterContext.cpp
lldb/source/Utility/RegisterValue.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/RegisterValue.h 
b/lldb/include/lldb/Utility/RegisterValue.h
index a0bd493d80941..b39e2980abacc 100644
--- a/lldb/include/lldb/Utility/RegisterValue.h
+++ b/lldb/include/lldb/Utility/RegisterValue.h
@@ -95,7 +95,7 @@ class RegisterValue {
   // value, or pad the destination with zeroes if the register byte size is
   // shorter that "dst_len" (all while correctly abiding the "dst_byte_order").
   // Returns the number of bytes copied into "dst".
-  uint32_t GetAsMemoryData(const RegisterInfo *reg_info, void *dst,
+  uint32_t GetAsMemoryData(const RegisterInfo ®_info, void *dst,
uint32_t dst_len, lldb::ByteOrder dst_byte_order,
Status &error) const;
 

diff  --git a/lldb/source/Host/common/NativeRegisterContext.cpp 
b/lldb/source/Host/common/NativeRegisterContext.cpp
index 7e4ffe8dfa184..0110a8ac9e2d4 100644
--- a/lldb/source/Host/common/NativeRegisterContext.cpp
+++ b/lldb/source/Host/common/NativeRegisterContext.cpp
@@ -385,18 +385,20 @@ Status NativeRegisterContext::ReadRegisterValueFromMemory(
 Status NativeRegisterContext::WriteRegisterValueToMemory(
 const RegisterInfo *reg_info, lldb::addr_t dst_addr, size_t dst_len,
 const RegisterValue ®_value) {
-
-  uint8_t dst[RegisterValue::kMaxRegisterByteSize];
-
   Status error;
+  if (reg_info == nullptr) {
+error.SetErrorString("Invalid register info argument.");
+return error;
+  }
 
+  uint8_t dst[RegisterValue::kMaxRegisterByteSize];
   NativeProcessProtocol &process = m_thread.GetProcess();
 
   // TODO: we might need to add a parameter to this function in case the byte
   // order of the memory data doesn't match the process. For now we are
   // assuming they are the same.
   const size_t bytes_copied = reg_value.GetAsMemoryData(
-  reg_info, dst, dst_len, process.GetByteOrder(), error);
+  *reg_info, dst, dst_len, process.GetByteOrder(), error);
 
   if (error.Success()) {
 if (bytes_copied == 0) {

diff  --git a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp 
b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
index 7b948d8fa8cb2..d942d53376e86 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
@@ -537,8 +537,8 @@ static bool LoadValueFromConsecutiveGPRRegisters(
 // Make sure we have enough room in "heap_data_up"
 if ((data_offset + *base_byte_size) <= heap_data_up->GetByteSize()) {
   const size_t bytes_copied = reg_value.GetAsMemoryData(
-  reg_info, heap_data_up->GetBytes() + data_offset, 
*base_byte_size,
-  byte_order, error);
+  *reg_info, heap_data_up->GetBytes() + data_offset,
+  *base_byte_size, byte_order, error);
   if (bytes_copied != *base_byte_size)
 return false;
   data_offset += bytes_copied;
@@ -577,7 +577,7 @@ static bool LoadValueFromConsecutiveGPRRegisters(
 
   const size_t curr_byte_size = std::min(8, bytes_left);
   const size_

lldb-commits@lists.llvm.org

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc7ddbd62d811: [LLDB] Change RegisterValue::GetAsMemoryData 
to const RegisterInfo& (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134965

Files:
  lldb/include/lldb/Utility/RegisterValue.h
  lldb/source/Host/common/NativeRegisterContext.cpp
  lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp
  lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp
  lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp
  lldb/source/Plugins/ABI/X86/ABISysV_i386.cpp
  lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
  lldb/source/Target/RegisterContext.cpp
  lldb/source/Utility/RegisterValue.cpp

Index: lldb/source/Utility/RegisterValue.cpp
===
--- lldb/source/Utility/RegisterValue.cpp
+++ lldb/source/Utility/RegisterValue.cpp
@@ -35,21 +35,16 @@
   return data.SetData(GetBytes(), GetByteSize(), GetByteOrder()) > 0;
 }
 
-uint32_t RegisterValue::GetAsMemoryData(const RegisterInfo *reg_info, void *dst,
+uint32_t RegisterValue::GetAsMemoryData(const RegisterInfo ®_info, void *dst,
 uint32_t dst_len,
 lldb::ByteOrder dst_byte_order,
 Status &error) const {
-  if (reg_info == nullptr) {
-error.SetErrorString("invalid register info argument.");
-return 0;
-  }
-
   // ReadRegister should have already been called on this object prior to
   // calling this.
   if (GetType() == eTypeInvalid) {
 // No value has been read into this object...
 error.SetErrorStringWithFormat(
-"invalid register value type for register %s", reg_info->name);
+"invalid register value type for register %s", reg_info.name);
 return 0;
   }
 
@@ -58,7 +53,7 @@
 return 0;
   }
 
-  const uint32_t src_len = reg_info->byte_size;
+  const uint32_t src_len = reg_info.byte_size;
 
   // Extract the register data into a data extractor
   DataExtractor reg_data;
@@ -76,7 +71,7 @@
dst_byte_order); // dst byte order
   if (bytes_copied == 0)
 error.SetErrorStringWithFormat(
-"failed to copy data for register write of %s", reg_info->name);
+"failed to copy data for register write of %s", reg_info.name);
 
   return bytes_copied;
 }
Index: lldb/source/Target/RegisterContext.cpp
===
--- lldb/source/Target/RegisterContext.cpp
+++ lldb/source/Target/RegisterContext.cpp
@@ -368,37 +368,41 @@
 Status RegisterContext::WriteRegisterValueToMemory(
 const RegisterInfo *reg_info, lldb::addr_t dst_addr, uint32_t dst_len,
 const RegisterValue ®_value) {
-  uint8_t dst[RegisterValue::kMaxRegisterByteSize];
-
   Status error;
-
   ProcessSP process_sp(m_thread.GetProcess());
-  if (process_sp) {
 
-// TODO: we might need to add a parameter to this function in case the byte
-// order of the memory data doesn't match the process. For now we are
-// assuming they are the same.
+  if (!process_sp) {
+error.SetErrorString("invalid process");
+return error;
+  }
+
+  if (reg_info == nullptr) {
+error.SetErrorString("Invalid register info argument.");
+return error;
+  }
 
-const uint32_t bytes_copied = reg_value.GetAsMemoryData(
-reg_info, dst, dst_len, process_sp->GetByteOrder(), error);
-
-if (error.Success()) {
-  if (bytes_copied == 0) {
-error.SetErrorString("byte copy failed.");
-  } else {
-const uint32_t bytes_written =
-process_sp->WriteMemory(dst_addr, dst, bytes_copied, error);
-if (bytes_written != bytes_copied) {
-  if (error.Success()) {
-// This might happen if we read _some_ bytes but not all
-error.SetErrorStringWithFormat("only wrote %u of %u bytes",
-   bytes_written, bytes_copied);
-  }
+  // TODO: we might need to add a parameter to this function in case the byte
+  // order of the memory data doesn't match the process. For now we are
+  // assuming they are the same.
+  uint8_t dst[RegisterValue::kMaxRegisterByteSize];
+  const uint32_t bytes_copied = reg_value.GetAsMemoryData(
+  *reg_info, dst, dst_len, process_sp->GetByteOrder(), error);
+
+  if (er

lldb-commits@lists.llvm.org

[David Spickett via Phabricator via lldb-commits]

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

All callers were either assuming their pointer was not null before calling
this, or checking beforehand.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135668

Files:
  lldb/include/lldb/Utility/RegisterValue.h
  lldb/source/Host/common/NativeRegisterContext.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
  lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
  lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
  lldb/source/Target/RegisterContext.cpp
  lldb/source/Utility/RegisterValue.cpp
  lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp

Index: lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
===
--- lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -215,7 +215,7 @@
   RegisterValue Value;
   Status ST;
   Value.SetFromMemoryData(
-  &Info, Bytes.data(), Bytes.size(),
+  Info, Bytes.data(), Bytes.size(),
   Endian == support::little ? eByteOrderLittle : eByteOrderBig, ST);
   if (ST.Fail())
 return ST.ToError();
Index: lldb/source/Utility/RegisterValue.cpp
===
--- lldb/source/Utility/RegisterValue.cpp
+++ lldb/source/Utility/RegisterValue.cpp
@@ -76,15 +76,10 @@
   return bytes_copied;
 }
 
-uint32_t RegisterValue::SetFromMemoryData(const RegisterInfo *reg_info,
+uint32_t RegisterValue::SetFromMemoryData(const RegisterInfo ®_info,
   const void *src, uint32_t src_len,
   lldb::ByteOrder src_byte_order,
   Status &error) {
-  if (reg_info == nullptr) {
-error.SetErrorString("invalid register info argument.");
-return 0;
-  }
-
   // Moving from addr into a register
   //
   // Case 1: src_len == dst_len
@@ -107,12 +102,12 @@
 return 0;
   }
 
-  const uint32_t dst_len = reg_info->byte_size;
+  const uint32_t dst_len = reg_info.byte_size;
 
   if (src_len > dst_len) {
 error.SetErrorStringWithFormat(
 "%u bytes is too big to store in register %s (%u bytes)", src_len,
-reg_info->name, dst_len);
+reg_info.name, dst_len);
 return 0;
   }
 
@@ -120,7 +115,7 @@
   // register value
   DataExtractor src_data(src, src_len, src_byte_order, 4);
 
-  error = SetValueFromData(reg_info, src_data, 0, true);
+  error = SetValueFromData(®_info, src_data, 0, true);
   if (error.Fail())
 return 0;
 
Index: lldb/source/Target/RegisterContext.cpp
===
--- lldb/source/Target/RegisterContext.cpp
+++ lldb/source/Target/RegisterContext.cpp
@@ -357,7 +357,7 @@
 // TODO: we might need to add a parameter to this function in case the byte
 // order of the memory data doesn't match the process. For now we are
 // assuming they are the same.
-reg_value.SetFromMemoryData(reg_info, src, src_len,
+reg_value.SetFromMemoryData(*reg_info, src, src_len,
 process_sp->GetByteOrder(), error);
   } else
 error.SetErrorString("invalid process");
Index: lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
===
--- lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
+++ lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
@@ -806,7 +806,7 @@
  RegisterValue ®_value) {
   Status error;
   reg_value.SetFromMemoryData(
-  reg_info, (const uint8_t *)&m_regs + reg_info->byte_offset,
+  *reg_info, (const uint8_t *)&m_regs + reg_info->byte_offset,
   reg_info->byte_size, lldb::eByteOrderLittle, error);
   return error.Success();
 }
Index: lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
===
--- lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
+++ lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
@@ -521,7 +521,7 @@
RegisterValue ®_value) {
   Status error;
   reg_value.SetFromMemoryData(
-  reg_info, (const uint8_t *)&m_regs + reg_info->byte_offset,
+  *reg_info, (const uint8_t *)&m_regs + reg_info->byte_offset,
   reg_info->byte_size, lld

lldb-commits@lists.llvm.org

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: clayborg.
DavidSpickett added a comment.
Herald added a subscriber: JDevlieghere.

This set of 4 patches will be it for a while on this front, honest :)

Thanks for the reviews so far!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135668

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

[Lldb-commits] [PATCH] D135670: [LLDB] Pass const RegisterInfo& to RegisterValue::SetValueFromData

[David Spickett via Phabricator via lldb-commits]

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

Familiar story, callers are either checking upfront that the pointer
wasn't null or not checking at all. SetValueFromData itself didn't
check either.

So make the parameter a ref and fixup the few places where a nullptr
check seems needed.

Depends on D135668 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135670

Files:
  lldb/include/lldb/Utility/RegisterValue.h
  lldb/source/Core/ValueObjectRegister.cpp
  lldb/source/Core/ValueObjectVariable.cpp
  lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextMemory.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
  lldb/source/Utility/RegisterValue.cpp

Index: lldb/source/Utility/RegisterValue.cpp
===
--- lldb/source/Utility/RegisterValue.cpp
+++ lldb/source/Utility/RegisterValue.cpp
@@ -115,7 +115,7 @@
   // register value
   DataExtractor src_data(src, src_len, src_byte_order, 4);
 
-  error = SetValueFromData(®_info, src_data, 0, true);
+  error = SetValueFromData(reg_info, src_data, 0, true);
   if (error.Fail())
 return 0;
 
@@ -150,16 +150,20 @@
 void RegisterValue::Clear() { m_type = eTypeInvalid; }
 
 RegisterValue::Type RegisterValue::SetType(const RegisterInfo *reg_info) {
+  assert(reg_info && "Expected non null reg_info.");
   // To change the type, we simply copy the data in again, using the new format
   RegisterValue copy;
   DataExtractor copy_data;
-  if (copy.CopyValue(*this) && copy.GetData(copy_data))
-SetValueFromData(reg_info, copy_data, 0, true);
+  if (copy.CopyValue(*this) && copy.GetData(copy_data)) {
+Status error = SetValueFromData(*reg_info, copy_data, 0, true);
+assert(error.Success() && "Expected SetValueFromData to succeed.");
+UNUSED_IF_ASSERT_DISABLED(error);
+  }
 
   return m_type;
 }
 
-Status RegisterValue::SetValueFromData(const RegisterInfo *reg_info,
+Status RegisterValue::SetValueFromData(const RegisterInfo ®_info,
DataExtractor &src,
lldb::offset_t src_offset,
bool partial_data_ok) {
@@ -170,22 +174,22 @@
 return error;
   }
 
-  if (reg_info->byte_size == 0) {
+  if (reg_info.byte_size == 0) {
 error.SetErrorString("invalid register info.");
 return error;
   }
 
   uint32_t src_len = src.GetByteSize() - src_offset;
 
-  if (!partial_data_ok && (src_len < reg_info->byte_size)) {
+  if (!partial_data_ok && (src_len < reg_info.byte_size)) {
 error.SetErrorString("not enough data.");
 return error;
   }
 
   // Cap the data length if there is more than enough bytes for this register
   // value
-  if (src_len > reg_info->byte_size)
-src_len = reg_info->byte_size;
+  if (src_len > reg_info.byte_size)
+src_len = reg_info.byte_size;
 
   // Zero out the value in case we get partial data...
   memset(buffer.bytes, 0, sizeof(buffer.bytes));
@@ -193,20 +197,20 @@
   type128 int128;
 
   m_type = eTypeInvalid;
-  switch (reg_info->encoding) {
+  switch (reg_info.encoding) {
   case eEncodingInvalid:
 break;
   case eEncodingUint:
   case eEncodingSint:
-if (reg_info->byte_size == 1)
+if (reg_info.byte_size == 1)
   SetUInt8(src.GetMaxU32(&src_offset, src_len));
-else if (reg_info->byte_size <= 2)
+else if (reg_info.byte_size <= 2)
   SetUInt16(src.GetMaxU32(&src_offset, src_len));
-else if (reg_info->byte_size <= 4)
+else if (reg_info.byte_size <= 4)
   SetUInt32(src.GetMaxU32(&src_offset, src_len));
-else if (reg_info->byte_size <= 8)
+else if (reg_info.byte_size <= 8)
   SetUInt64(src.GetMaxU64(&src_offset, src_len));
-else if (reg_info->byte_size <= 16) {
+else if (reg_info.byte_size <= 16) {
   uint64_t data1 = src.GetU64(&src_offset);
   uint64_t data2 = src.GetU64(&src_offset);
   if (src.GetByteSize() == eByteOrderBig) {
@@ -220,16 +224,16 @@
 }
 break;
   case eEncodingIEEE754:
-if (reg_info->byte_size == sizeof(float))
+if (reg_info.byte_size == sizeof(float))
   SetFloat(src.GetFloat(&src_offset));
-else if (reg_info->byte_size == sizeof(double))
+else if (reg_info.byte_size == sizeof(double))
   SetDouble(src.GetDouble(&src_offset));
-else if (reg_info->byte_size == sizeof(long double))
+else if (reg_info.byte_size == sizeof(long double))
   SetLongDouble(src.GetLongDouble(&src_offset));
 break;
   case eEncodingVector: {
 m_type = eTypeBytes;
-buffer.length = reg_info->byte_size;
+buffer.length = reg_info.byte_size;
 buffer.byte_order = src.GetByteOrder();
 assert(buffer.length <= kMaxRegi

lldb-commits@lists.llvm.org

[David Spickett via Phabricator via lldb-commits]

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

No one was pasing nullptr here.

Depends on D135670 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135671

Files:
  lldb/include/lldb/Utility/RegisterValue.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
  lldb/source/Utility/RegisterValue.cpp


Index: lldb/source/Utility/RegisterValue.cpp
===
--- lldb/source/Utility/RegisterValue.cpp
+++ lldb/source/Utility/RegisterValue.cpp
@@ -149,13 +149,12 @@
 
 void RegisterValue::Clear() { m_type = eTypeInvalid; }
 
-RegisterValue::Type RegisterValue::SetType(const RegisterInfo *reg_info) {
-  assert(reg_info && "Expected non null reg_info.");
+RegisterValue::Type RegisterValue::SetType(const RegisterInfo ®_info) {
   // To change the type, we simply copy the data in again, using the new format
   RegisterValue copy;
   DataExtractor copy_data;
   if (copy.CopyValue(*this) && copy.GetData(copy_data)) {
-Status error = SetValueFromData(*reg_info, copy_data, 0, true);
+Status error = SetValueFromData(reg_info, copy_data, 0, true);
 assert(error.Success() && "Expected SetValueFromData to succeed.");
 UNUSED_IF_ASSERT_DISABLED(error);
   }
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
@@ -136,7 +136,7 @@
   // then use the type specified by reg_info rather than the uint64_t
   // default
   if (reg_value.GetByteSize() > reg_info->byte_size)
-reg_value.SetType(reg_info);
+reg_value.SetType(*reg_info);
 }
 return error;
   }
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
@@ -75,7 +75,7 @@
 memcpy(dst + (reg_info->byte_offset & 0x1), src, src_size);
 // Set this full register as the value to write.
 value_to_write.SetBytes(dst, full_value.GetByteSize(), byte_order);
-value_to_write.SetType(full_reg_info);
+value_to_write.SetType(*full_reg_info);
 reg_to_write = full_reg;
   }
 }
Index: lldb/include/lldb/Utility/RegisterValue.h
===
--- lldb/include/lldb/Utility/RegisterValue.h
+++ lldb/include/lldb/Utility/RegisterValue.h
@@ -84,7 +84,7 @@
 
   void SetType(RegisterValue::Type type) { m_type = type; }
 
-  RegisterValue::Type SetType(const RegisterInfo *reg_info);
+  RegisterValue::Type SetType(const RegisterInfo ®_info);
 
   bool GetData(DataExtractor &data) const;
 


Index: lldb/source/Utility/RegisterValue.cpp
===
--- lldb/source/Utility/RegisterValue.cpp
+++ lldb/source/Utility/RegisterValue.cpp
@@ -149,13 +149,12 @@
 
 void RegisterValue::Clear() { m_type = eTypeInvalid; }
 
-RegisterValue::Type RegisterValue::SetType(const RegisterInfo *reg_info) {
-  assert(reg_info && "Expected non null reg_info.");
+RegisterValue::Type RegisterValue::SetType(const RegisterInfo ®_info) {
   // To change the type, we simply copy the data in again, using the new format
   RegisterValue copy;
   DataExtractor copy_data;
   if (copy.CopyValue(*this) && copy.GetData(copy_data)) {
-Status error = SetValueFromData(*reg_info, copy_data, 0, true);
+Status error = SetValueFromData(reg_info, copy_data, 0, true);
 assert(error.Success() && "Expected SetValueFromData to succeed.");
 UNUSED_IF_ASSERT_DISABLED(error);
   }
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
@@ -136,7 +136,7 @@
   // then use the type specified by reg_info rather than the uint64_t
   // default
   if (reg_value.GetByteSize() > reg_info->byte_size)
-reg_value.SetType(reg_info);
+reg_value.SetType(*reg_info);
 }
 return error;
   }
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
@@ -75,7 +75,7 @@
 memcpy(dst +

[Lldb-commits] [PATCH] D135672: [LLDB] Change EmulateInstriction::ReadRegister to return Optional

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: atanasyan, jrtc27.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Making it easier to understand and harder to misuse.
This only applies to the ReadRegister(const RegisterInfo ®_info) variant.

Depends on D135671 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135672

Files:
  lldb/include/lldb/Core/EmulateInstruction.h
  lldb/source/Core/EmulateInstruction.cpp
  lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp

Index: lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
===
--- lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
+++ lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
@@ -1153,18 +1153,19 @@
   /* We look for sp based non-volatile register stores */
   if (nonvolatile_reg_p(src)) {
 Context context;
-RegisterValue data_src;
 context.type = eContextPushRegisterOnStack;
 context.SetRegisterToRegisterPlusOffset(*reg_info_src, *reg_info_base, 0);
 
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(*reg_info_base, data_src))
+llvm::Optional data_src = ReadRegister(*reg_info_base);
+if (!data_src)
   return false;
 
-if (data_src.GetAsMemoryData(*reg_info_src, buffer, reg_info_src->byte_size,
- eByteOrderLittle, error) == 0)
+if (data_src->GetAsMemoryData(*reg_info_src, buffer,
+  reg_info_src->byte_size, eByteOrderLittle,
+  error) == 0)
   return false;
 
 if (!WriteMemory(context, address, buffer, reg_info_src->byte_size))
Index: lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
===
--- lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
+++ lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
@@ -1259,18 +1259,19 @@
   return false;
 
 Context context;
-RegisterValue data_src;
 context.type = eContextPushRegisterOnStack;
 context.SetRegisterToRegisterPlusOffset(*reg_info_src, *reg_info_base, 0);
 
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(*reg_info_base, data_src))
+llvm::Optional data_src = ReadRegister(*reg_info_base);
+if (!data_src)
   return false;
 
-if (data_src.GetAsMemoryData(*reg_info_src, buffer, reg_info_src->byte_size,
- eByteOrderLittle, error) == 0)
+if (data_src->GetAsMemoryData(*reg_info_src, buffer,
+  reg_info_src->byte_size, eByteOrderLittle,
+  error) == 0)
   return false;
 
 if (!WriteMemory(context, address, buffer, reg_info_src->byte_size))
@@ -1518,18 +1519,18 @@
   if (base == dwarf_sp_mips && nonvolatile_reg_p(src)) {
 RegisterInfo reg_info_src = {};
 Context context;
-RegisterValue data_src;
 context.type = eContextPushRegisterOnStack;
 context.SetRegisterToRegisterPlusOffset(reg_info_src, *reg_info_base, 0);
 
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(*reg_info_base, data_src))
+llvm::Optional data_src = ReadRegister(*reg_info_base);
+if (!data_src)
   return false;
 
-if (data_src.GetAsMemoryData(reg_info_src, buffer, reg_info_src.byte_size,
- eByteOrderLittle, error) == 0)
+if (data_src->GetAsMemoryData(reg_info_src, buffer, reg_info_src.byte_size,
+  eByteOrderLittle, error) == 0)
   return false;
 
 if (!WriteMemory(context, address, buffer, reg_info_src.byte_size))
@@ -1600,18 +1601,19 @@
   return false;
 
 Context context;
-RegisterValue data_src;
 context.type = eContextPushRegisterOnStack;
 context.SetRegisterToRegisterPlusOffset(*reg_info_src, *reg_info_base, 0);
 
 uint8_t buffer[RegisterValue::kMaxRegisterByteSize];
 Status error;
 
-if (!ReadRegister(*reg_info_base, data_src))
+llvm::Optional data_src = ReadRegister(*reg_info_base);
+if (!data_src)
   return false;
 
-if (data_src.GetAsMemoryData(*reg_info_src, buffer, reg_info_src->byte_size,
- eByteOrderLittle, error) == 0)
+if (data_src->GetAsMemoryData(*reg_info_src, buffer,
+  reg_info_src->byte_size, eByteOrderLittle,
+  error) == 0)
   return false;
 
 if (!WriteMemory(context, base_address, buffer, reg_info_

[Lldb-commits] [PATCH] D135631: [lldb] Copy log files into diagnostic directory (RFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

In D135631#3849295 , @DavidSpickett 
wrote:

> So either way, if someone was running with no logs enabled we would have to 
> ask them to enable them then re-run and get a new set of diagnostics (which 
> is fine as is , not a criticism).

Yeah, the idea is that if we ask someone to enable logs (which is still very 
likely even with the diagnostics) we'd automatically include them in the 
diagnostics so they don't need to go look in multiple places.

In D135631#3849307 , @DavidSpickett 
wrote:

> Also is the buffer option the same as "circular" here?
>
>   -h  ( --log-handler  )
>Specify a log handler which determines where log messages are written.
>Values: default | stream | circular | os

Yup

> So we could have diagnostics also save anything kept in "circular", on the 
> off chance that writing to a file isn't possible.

Technically yes, but then we have two ad-hoc solutions that support half the 
log handlers. I think if we care about that we might as well go with the 
T-based log handler behind the scenes.




Comment at: lldb/test/Shell/Diagnostics/TestCopyLogs.test:3
+# RUN: mkdir -p %t
+# The "ll''db" below is not a typo but a way to prevent lit from substituting 
'lldb'.
+# RUN: %lldb -o 'log enable ll''db commands -f %t/commands.log' -o 
'diagnostics dump -d %t/diags'

labath wrote:
> That's cute, but I suspect windows will have a problem with that. `-s %s` 
> (and putting the commands in this file) would be safer.
I thought about that, but I still need `%t` to be expanded. I guess I could put 
`log enable lldb commands -f ` in a file and append `%t/commands.log` to it. 


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

https://reviews.llvm.org/D135631

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

[Lldb-commits] [PATCH] D135621: [lldb] Add an always-on log channel

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D135621#3849043 , @labath wrote:

> I'm not sure if this will do what you wanted it to do. Despite the title, 
> this patch is not adding a new log *channel* -- it is adding a new log 
> *category* to the "lldb" channel. Our logging infra, perhaps somewhat 
> misleadingly, manages all of the log state on a per-channel basis. This means 
> that it is not e.g. possible to redirect one log category to a different sink 
> than a different category in the same channel. I haven't checked this, but I 
> suspect that for this reason, any explicit "log enable" commands by the user 
> will redirect this special channel into the new (user-provided) destination. 
> If this is not what you intended to do then, you probably ought to add a new 
> log channel, for real.

You're absolutely right. I actually came to that same conclusion myself when I 
implemented an earlier iteration of this patch a long while ago but evidently 
didn't think of it before recreating the patch from memory.

> That said, without knowing what kind of information do you want to log here, 
> I am wondering if the same kind of information could be gathered by reusing 
> some existing data collection mechanism, like session transcripts, or the 
> yet-to-be-implemented telemetry data.

I see those two things as orthogonal. Always-on logging is something that has 
comes up quite frequently. @kastiglione can talk about our downstream log 
channel for Swift and @Michael137 was also interested in this to triage common 
issues with the expression evaluator.


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

https://reviews.llvm.org/D135621

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

[Lldb-commits] [PATCH] D135607: Summary:

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D135607#3848334 , @HenriqueBucher 
wrote:

> To be frank, I think this formatting is silly and wrong now that I am looking 
> at it. 
> Looking at the rendered markdown at Github 
> 
>  you can see that the browser itself word wraps for you to the allocated 
> width. 
> There is no need to break down manually. 
> However if you break down inside the tables it will break the syntax of 
> markdown tables.

Yes, this is an example where you have to exceed the 80 column limit.

> And the wrapping does not affect at all the output on the web.

That's correct, the 80 column limit is not a technical limitation, but rather a 
coding documentation style one. If you take a look at the other docs you'll 
notice everything is using the 80 column limit.

> I'd say delete this patch and forget about this 80 column limit. 
> But it's up to you, I'm here just as a helper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135607

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

[Lldb-commits] [PATCH] D135577: Summary:

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135577

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

[Lldb-commits] [PATCH] D135577: Summary:

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Henrique, do you have commit access?

Whoever ends up committing this, please make sure the commit has a reasonable 
commit description and not "Summary:".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135577

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

[Lldb-commits] [lldb] eab5c2f - [LLDB] Fix crash when printing a struct with a static wchar_t member

[Arthur Eubanks via lldb-commits]

Author: Arthur Eubanks
Date: 2022-10-11T11:04:32-07:00
New Revision: eab5c2f94f5aae17c3fc513ee347ee9bc1d2bcae

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

LOG: [LLDB] Fix crash when printing a struct with a static wchar_t member

Similar to D135170.

Reviewed By: DavidSpickett

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

Added: 


Modified: 
clang/lib/AST/StmtPrinter.cpp

lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
lldb/test/API/lang/cpp/const_static_integral_member/main.cpp

Removed: 




diff  --git a/clang/lib/AST/StmtPrinter.cpp b/clang/lib/AST/StmtPrinter.cpp
index fabffbd323648..a29f762e10c14 100644
--- a/clang/lib/AST/StmtPrinter.cpp
+++ b/clang/lib/AST/StmtPrinter.cpp
@@ -1293,6 +1293,9 @@ void StmtPrinter::VisitIntegerLiteral(IntegerLiteral 
*Node) {
 break; // no suffix.
   case BuiltinType::UInt128:
 break; // no suffix.
+  case BuiltinType::WChar_S:
+  case BuiltinType::WChar_U:
+break; // no suffix
   }
 }
 

diff  --git 
a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
 
b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
index 91ed14ed48ab7..ed7cd2514ae84 100644
--- 
a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -42,6 +42,7 @@ def test(self):
 self.expect_expr("A::ulong_max == ulong_max", result_value="true")
 self.expect_expr("A::longlong_max == longlong_max", 
result_value="true")
 self.expect_expr("A::ulonglong_max == ulonglong_max", 
result_value="true")
+self.expect_expr("A::wchar_max == wchar_max", result_value="true")
 
 self.expect_expr("A::char_min == char_min", result_value="true")
 self.expect_expr("A::schar_min == schar_min", result_value="true")
@@ -52,6 +53,7 @@ def test(self):
 self.expect_expr("A::ulong_min == ulong_min", result_value="true")
 self.expect_expr("A::longlong_min == longlong_min", 
result_value="true")
 self.expect_expr("A::ulonglong_min == ulonglong_min", 
result_value="true")
+self.expect_expr("A::wchar_min == wchar_min", result_value="true")
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")

diff  --git a/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp 
b/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
index 09ab9e6698132..4cd4933275ae4 100644
--- a/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ b/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -36,6 +36,7 @@ struct A {
   const static auto longlong_max = std::numeric_limits::max();
   const static auto ulonglong_max =
   std::numeric_limits::max();
+  const static auto wchar_max = std::numeric_limits::max();
 
   const static auto char_min = std::numeric_limits::min();
   const static auto schar_min = std::numeric_limits::min();
@@ -47,6 +48,7 @@ struct A {
   const static auto longlong_min = std::numeric_limits::min();
   const static auto ulonglong_min =
   std::numeric_limits::min();
+  const static auto wchar_min = std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
@@ -93,6 +95,7 @@ int main() {
   auto ulong_max = A::ulong_max;
   auto longlong_max = A::longlong_max;
   auto ulonglong_max = A::ulonglong_max;
+  auto wchar_max = A::wchar_max;
 
   auto char_min = A::char_min;
   auto schar_min = A::schar_min;
@@ -103,6 +106,7 @@ int main() {
   auto ulong_min = A::ulong_min;
   auto longlong_min = A::longlong_min;
   auto ulonglong_min = A::ulonglong_min;
+  auto wchar_min = A::wchar_min;
 
   int member_copy = ClassWithOnlyConstStatic::member;
 



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

[Lldb-commits] [PATCH] D135461: [LLDB] Fix crash when printing a struct with a static wchar_t member

[Arthur Eubanks via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGeab5c2f94f5a: [LLDB] Fix crash when printing a struct with a 
static wchar_t member (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135461

Files:
  clang/lib/AST/StmtPrinter.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -36,6 +36,7 @@
   const static auto longlong_max = std::numeric_limits::max();
   const static auto ulonglong_max =
   std::numeric_limits::max();
+  const static auto wchar_max = std::numeric_limits::max();
 
   const static auto char_min = std::numeric_limits::min();
   const static auto schar_min = std::numeric_limits::min();
@@ -47,6 +48,7 @@
   const static auto longlong_min = std::numeric_limits::min();
   const static auto ulonglong_min =
   std::numeric_limits::min();
+  const static auto wchar_min = std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
@@ -93,6 +95,7 @@
   auto ulong_max = A::ulong_max;
   auto longlong_max = A::longlong_max;
   auto ulonglong_max = A::ulonglong_max;
+  auto wchar_max = A::wchar_max;
 
   auto char_min = A::char_min;
   auto schar_min = A::schar_min;
@@ -103,6 +106,7 @@
   auto ulong_min = A::ulong_min;
   auto longlong_min = A::longlong_min;
   auto ulonglong_min = A::ulonglong_min;
+  auto wchar_min = A::wchar_min;
 
   int member_copy = ClassWithOnlyConstStatic::member;
 
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -42,6 +42,7 @@
 self.expect_expr("A::ulong_max == ulong_max", result_value="true")
 self.expect_expr("A::longlong_max == longlong_max", 
result_value="true")
 self.expect_expr("A::ulonglong_max == ulonglong_max", 
result_value="true")
+self.expect_expr("A::wchar_max == wchar_max", result_value="true")
 
 self.expect_expr("A::char_min == char_min", result_value="true")
 self.expect_expr("A::schar_min == schar_min", result_value="true")
@@ -52,6 +53,7 @@
 self.expect_expr("A::ulong_min == ulong_min", result_value="true")
 self.expect_expr("A::longlong_min == longlong_min", 
result_value="true")
 self.expect_expr("A::ulonglong_min == ulonglong_min", 
result_value="true")
+self.expect_expr("A::wchar_min == wchar_min", result_value="true")
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -1293,6 +1293,9 @@
 break; // no suffix.
   case BuiltinType::UInt128:
 break; // no suffix.
+  case BuiltinType::WChar_S:
+  case BuiltinType::WChar_U:
+break; // no suffix
   }
 }
 


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -36,6 +36,7 @@
   const static auto longlong_max = std::numeric_limits::max();
   const static auto ulonglong_max =
   std::numeric_limits::max();
+  const static auto wchar_max = std::numeric_limits::max();
 
   const static auto char_min = std::numeric_limits::min();
   const static auto schar_min = std::numeric_limits::min();
@@ -47,6 +48,7 @@
   const static auto longlong_min = std::numeric_limits::min();
   const static auto ulonglong_min =
   std::numeric_limits::min();
+  const static auto wchar_min = std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
@@ -93,6 +95,7 @@
   auto ulong_max = A::ulong_max;
   auto longlong_max = A::longlong_max;
   auto ulonglong_max = A::ulonglong_max;
+  auto wchar_max = A::wchar_max;
 
   auto char_min = A::char_min;
   auto schar_min = A::schar_min;
@@ -103,6 +106,7 @@
   auto ulong_min = A::ulong_min;
   auto longlong_min = A::longlong_min;
   auto ulonglong_min = A::ulonglong_min;
+  auto wchar_min = A::wchar_min;
 
   int member_copy = ClassWithOnlyConstStatic::member;
 
Index:

[Lldb-commits] [PATCH] D135621: [lldb] Add an always-on log channel

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Maybe use LLDBLog::Diagnostics instead of LLDBLog::AlwaysOn? and maybe 
"diagnostics" instead of "always-on"?

I am also questioning if the these even belong in the LLDB logging stuff? Seems 
like it would be just as easy to create a diagnostic message by calling 
Diagnostics::Report(...). Do we really want to modify the log channels here? 
Seems like always on diagnostics should just a dedicated API.

It might be nice to also have a "diagnostics" log channel that _can_ be 
enabled, and any diagnostic messages would then also be included in the log 
output but only if enabled by the user?




Comment at: lldb/include/lldb/Utility/Diagnostics.h:49
   static llvm::Optional &InstanceImpl();
+  static const size_t g_log_messages;
+

Move out of header file as a file static only? Better name might be a good 
idea. "g_log_messages" sounds like a boolean setting. Maybe 
"g_max_log_messages" or "g_num_log_messages"?


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

https://reviews.llvm.org/D135621

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

[Lldb-commits] [PATCH] D135622: [lldb] Add a "diagnostics dump" command

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Commands/CommandObjectDiagnostics.cpp:84
+if (!directory) {
+  result.AppendError(llvm::toString(directory.takeError()));
+  result.SetStatus(eReturnStatusFailed);

Do we want to just create a temp directory if the user didn't specify one here 
and them emit the path to where things were saved instead of erroring out? 



Comment at: lldb/source/Utility/Diagnostics.cpp:58
 
 bool Diagnostics::Dump(raw_ostream &stream) {
+  Expected diagnostics_dir = CreateUniqueDirectory();

How are we dumping these to the directory the user specifies the directory in 
"diagnostic dump"? Below we create a unique directory. Seems like this API 
should take a "Optional directory" parameter and if it doesn't have a 
value, then create a unique directory?


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

https://reviews.llvm.org/D135622

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

[Lldb-commits] [PATCH] D135621: [lldb] Add an always-on log channel

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

> It might be nice to also have a "diagnostics" log channel that _can_ be 
> enabled, and any diagnostic messages would then also be included in the log 
> output but only if enabled by the user?

And if the logging gets enabled, we can write out all of the diagnostics and 
are in the circular buffer if the "log enable lldb diagnostics" is enabled, 
then as each diagnostic is reported, the Diagnostics::Report(...) call could 
emit the log messages real time.


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

https://reviews.llvm.org/D135621

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

[Lldb-commits] [PATCH] D135620: Prevent lldb-vscode tests from source lldbinit file

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan updated this revision to Diff 466909.
yinghuitan added a comment.

Add comment per feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135620

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1444,8 +1444,15 @@
   auto log_cb = [](const char *buf, void *baton) -> void {
 g_vsc.SendOutput(OutputType::Console, llvm::StringRef{buf});
   };
+
+  auto arguments = request.getObject("arguments");
+  // sourceInitFile option is not from formal DAP specification. It is only
+  // used by unit tests to prevent sourcing .lldbinit files from environment
+  // which may affect the outcome of tests.
+  bool source_init_file = GetBoolean(arguments, "sourceInitFile", true);
+
   g_vsc.debugger =
-  lldb::SBDebugger::Create(true /*source_init_files*/, log_cb, nullptr);
+  lldb::SBDebugger::Create(source_init_file, log_cb, nullptr);
   g_vsc.progress_event_thread = std::thread(ProgressEventThreadFunction);
 
   // Start our event thread so we can receive events from the debugger, target,
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -613,7 +613,7 @@
 }
 return self.send_recv(command_dict)
 
-def request_initialize(self):
+def request_initialize(self, sourceInitFile):
 command_dict = {
 'command': 'initialize',
 'type': 'request',
@@ -626,7 +626,8 @@
 'pathFormat': 'path',
 'supportsRunInTerminalRequest': True,
 'supportsVariablePaging': True,
-'supportsVariableType': True
+'supportsVariableType': True,
+'sourceInitFile': sourceInitFile
 }
 }
 response = self.send_recv(command_dict)
@@ -1004,7 +1005,7 @@
 
 
 def run_vscode(dbg, args, options):
-dbg.request_initialize()
+dbg.request_initialize(options.sourceInitFile)
 if attach_options_specified(options):
 response = dbg.request_attach(program=options.program,
   pid=options.pid,
@@ -1112,6 +1113,13 @@
 default=False,
 help='Pause waiting for a debugger to attach to the debug adaptor')
 
+parser.add_option(
+'--sourceInitFile',
+action='store_true',
+dest='sourceInitFile',
+default=False,
+help='Whether lldb-vscode should source .lldbinit file or not')
+
 parser.add_option(
 '--port',
 type='int',
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -253,7 +253,7 @@
initCommands=None, preRunCommands=None, stopCommands=None,
exitCommands=None, attachCommands=None, coreFile=None,
disconnectAutomatically=True, terminateCommands=None,
-   postRunCommands=None, sourceMap=None):
+   postRunCommands=None, sourceMap=None, sourceInitFile=False):
 '''Build the default Makefile target, create the VSCode debug adaptor,
and attach to the process.
 '''
@@ -267,7 +267,7 @@
 # Execute the cleanup function during test case tear down.
 self.addTearDownHook(cleanup)
 # Initialize and launch the program
-self.vscode.request_initialize()
+self.vscode.request_initialize(sourceInitFile)
 response = self.vscode.request_attach(
 program=program, pid=pid, waitFor=waitFor, trace=trace,
 initCommands=initCommands, preRunCommands=preRunCommands,
@@ -284,7 +284,7 @@
disableSTDIO=False, shellExpandArguments=False,
trace=False, initCommands=None, preRunCommands=None,
stopCommands=None, exitCommands=None, terminateCommands=None,
-   sourcePath=None, debuggerRoot=None, launchCommands=None,
+   sourcePath=None, debuggerRoot=None, sourceInitFile=False, launchCommands=None,
sourceMap=None, disconnectAutomatically=True, runInTerminal=False,
expectFailure=False, postRunCommands=None):
 '''Sending launch request to vscode
@@ -301,7 +301,7 @@
 self.addTearDownHook(cleanup)
 
   

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via lldb-commits]

davrec accepted this revision.
davrec added a comment.

Looks good, over to @ChuanqiXu




Comment at: clang/include/clang/Sema/Template.h:80
+struct ArgumentListLevel {
+  Decl *AssociatedDecl;
+  ArgList Args;

mizvekov wrote:
> davrec wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > Actually I think this one should be changed back to `ReplacedDecl` :)
> > > > ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make 
> > > > better sense in STTPT.
> > > I would be against introducing another term to refer to the same thing...
> > The reason we need this unfortunately vague term "AssociatedDecl" in STTPT 
> > is because it can either be a template/template-like declaration *or* a 
> > TemplateTypeParmDecl.  But here in MLTAL, it won't be a TTPD, will it?  It 
> > will always be the parent template/template-like declaration, right?  So 
> > there is no need for vagueness.  `ReplacedDecl` or `ParentTemplate` or 
> > something like that seems more appropriate.  
> No, it can be the TTPD which is used to represent the invented template for a 
> requires substitution.
K, makes sense to keep it the same then


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [lldb] ef25a21 - Prevent lldb-vscode tests from source lldbinit file

[Jeffrey Tan via lldb-commits]

Author: Jeffrey Tan
Date: 2022-10-11T15:43:35-07:00
New Revision: ef25a217266aaf3b6df68ac155537e3a1171dccf

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

LOG: Prevent lldb-vscode tests from source lldbinit file

lldb-vscode is hard-coded to source .lldbinit file which causes some tests to
fail on my machine.
This patch adds a new option to control this:
1. vscode.py and lldb-vscode tests will not source .lldbinit by default
2. lldb-vscode will source .lldbinit in production if not specified otherwise

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

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
lldb/tools/lldb-vscode/lldb-vscode.cpp

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
index 90d90d959592d..8ba8e0e4bf3b4 100644
--- 
a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -253,7 +253,7 @@ def attach(self, program=None, pid=None, waitFor=None, 
trace=None,
initCommands=None, preRunCommands=None, stopCommands=None,
exitCommands=None, attachCommands=None, coreFile=None,
disconnectAutomatically=True, terminateCommands=None,
-   postRunCommands=None, sourceMap=None):
+   postRunCommands=None, sourceMap=None, sourceInitFile=False):
 '''Build the default Makefile target, create the VSCode debug adaptor,
and attach to the process.
 '''
@@ -267,7 +267,7 @@ def cleanup():
 # Execute the cleanup function during test case tear down.
 self.addTearDownHook(cleanup)
 # Initialize and launch the program
-self.vscode.request_initialize()
+self.vscode.request_initialize(sourceInitFile)
 response = self.vscode.request_attach(
 program=program, pid=pid, waitFor=waitFor, trace=trace,
 initCommands=initCommands, preRunCommands=preRunCommands,
@@ -284,7 +284,7 @@ def launch(self, program=None, args=None, cwd=None, 
env=None,
disableSTDIO=False, shellExpandArguments=False,
trace=False, initCommands=None, preRunCommands=None,
stopCommands=None, exitCommands=None, terminateCommands=None,
-   sourcePath=None, debuggerRoot=None, launchCommands=None,
+   sourcePath=None, debuggerRoot=None, sourceInitFile=False, 
launchCommands=None,
sourceMap=None, disconnectAutomatically=True, 
runInTerminal=False,
expectFailure=False, postRunCommands=None):
 '''Sending launch request to vscode
@@ -301,7 +301,7 @@ def cleanup():
 self.addTearDownHook(cleanup)
 
 # Initialize and launch the program
-self.vscode.request_initialize()
+self.vscode.request_initialize(sourceInitFile)
 response = self.vscode.request_launch(
 program,
 args=args,
@@ -344,7 +344,7 @@ def build_and_launch(self, program, args=None, cwd=None, 
env=None,
  trace=False, initCommands=None, preRunCommands=None,
  stopCommands=None, exitCommands=None,
  terminateCommands=None, sourcePath=None,
- debuggerRoot=None, runInTerminal=False,
+ debuggerRoot=None, sourceInitFile=False, 
runInTerminal=False,
  disconnectAutomatically=True, postRunCommands=None,
  lldbVSCodeEnv=None):
 '''Build the default Makefile target, create the VSCode debug adaptor,
@@ -356,6 +356,7 @@ def build_and_launch(self, program, args=None, cwd=None, 
env=None,
 return self.launch(program, args, cwd, env, stopOnEntry, disableASLR,
 disableSTDIO, shellExpandArguments, trace,
 initCommands, preRunCommands, stopCommands, exitCommands,
-terminateCommands, sourcePath, debuggerRoot, 
runInTerminal=runInTerminal,
+terminateCommands, sourcePath, debuggerRoot, 
sourceInitFile,
+runInTerminal=runInTerminal,
 disconnectAutomatically=disconnectAutomatically,
 postRunCommands=postRunCommands)

diff  --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
index 0996d8696cd25..d6a6abca53e38 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ b/ll

[Lldb-commits] [PATCH] D135620: Prevent lldb-vscode tests from source lldbinit file

[jeffrey tan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGef25a217266a: Prevent lldb-vscode tests from source lldbinit 
file (authored by yinghuitan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135620

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1444,8 +1444,15 @@
   auto log_cb = [](const char *buf, void *baton) -> void {
 g_vsc.SendOutput(OutputType::Console, llvm::StringRef{buf});
   };
+
+  auto arguments = request.getObject("arguments");
+  // sourceInitFile option is not from formal DAP specification. It is only
+  // used by unit tests to prevent sourcing .lldbinit files from environment
+  // which may affect the outcome of tests.
+  bool source_init_file = GetBoolean(arguments, "sourceInitFile", true);
+
   g_vsc.debugger =
-  lldb::SBDebugger::Create(true /*source_init_files*/, log_cb, nullptr);
+  lldb::SBDebugger::Create(source_init_file, log_cb, nullptr);
   g_vsc.progress_event_thread = std::thread(ProgressEventThreadFunction);
 
   // Start our event thread so we can receive events from the debugger, target,
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -613,7 +613,7 @@
 }
 return self.send_recv(command_dict)
 
-def request_initialize(self):
+def request_initialize(self, sourceInitFile):
 command_dict = {
 'command': 'initialize',
 'type': 'request',
@@ -626,7 +626,8 @@
 'pathFormat': 'path',
 'supportsRunInTerminalRequest': True,
 'supportsVariablePaging': True,
-'supportsVariableType': True
+'supportsVariableType': True,
+'sourceInitFile': sourceInitFile
 }
 }
 response = self.send_recv(command_dict)
@@ -1004,7 +1005,7 @@
 
 
 def run_vscode(dbg, args, options):
-dbg.request_initialize()
+dbg.request_initialize(options.sourceInitFile)
 if attach_options_specified(options):
 response = dbg.request_attach(program=options.program,
   pid=options.pid,
@@ -1112,6 +1113,13 @@
 default=False,
 help='Pause waiting for a debugger to attach to the debug adaptor')
 
+parser.add_option(
+'--sourceInitFile',
+action='store_true',
+dest='sourceInitFile',
+default=False,
+help='Whether lldb-vscode should source .lldbinit file or not')
+
 parser.add_option(
 '--port',
 type='int',
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -253,7 +253,7 @@
initCommands=None, preRunCommands=None, stopCommands=None,
exitCommands=None, attachCommands=None, coreFile=None,
disconnectAutomatically=True, terminateCommands=None,
-   postRunCommands=None, sourceMap=None):
+   postRunCommands=None, sourceMap=None, sourceInitFile=False):
 '''Build the default Makefile target, create the VSCode debug adaptor,
and attach to the process.
 '''
@@ -267,7 +267,7 @@
 # Execute the cleanup function during test case tear down.
 self.addTearDownHook(cleanup)
 # Initialize and launch the program
-self.vscode.request_initialize()
+self.vscode.request_initialize(sourceInitFile)
 response = self.vscode.request_attach(
 program=program, pid=pid, waitFor=waitFor, trace=trace,
 initCommands=initCommands, preRunCommands=preRunCommands,
@@ -284,7 +284,7 @@
disableSTDIO=False, shellExpandArguments=False,
trace=False, initCommands=None, preRunCommands=None,
stopCommands=None, exitCommands=None, terminateCommands=None,
-   sourcePath=None, debuggerRoot=None, launchCommands=None,
+   sourcePath=None, debuggerRoot=None, sourceInitFile=False, launchCommands=None,
sourceMap=None, disconnectAutomatically=True, runInTerminal=False,
expectFailure=False, postRunCommands=None):
 '''Sending launch 

[Lldb-commits] [PATCH] D135577: Summary:

[Henrique Bucher via Phabricator via lldb-commits]

HenriqueBucher added a comment.

I assume not since I have never committed on any LLVM project. 
But I would like to as I plan to contribute more in the future. 
Just sent Chris Clattner an email with the request. 
Note: The reason the summary is so terse is that the ARC tool opens two windows 
and I was very confused about where to write what. 
Maybe a little paragraph in the documentation would make this clearer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135577

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