[Lldb-commits] [lldb] ae361d3 - [LLDB] Remove return value from DumpRegisterValue

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-01-23T10:52:20Z
New Revision: ae361d3d90a83ede308dd4ee56811966e8a42fe2

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

LOG: [LLDB] Remove return value from DumpRegisterValue

No one ever checks it. Also convert to early return.

Reviewed By: labath

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

Added: 


Modified: 
lldb/include/lldb/Core/DumpRegisterValue.h
lldb/source/Core/DumpRegisterValue.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/DumpRegisterValue.h 
b/lldb/include/lldb/Core/DumpRegisterValue.h
index 655003ad76348..08bf0af8f36b4 100644
--- a/lldb/include/lldb/Core/DumpRegisterValue.h
+++ b/lldb/include/lldb/Core/DumpRegisterValue.h
@@ -21,7 +21,7 @@ class Stream;
 
 // The default value of 0 for reg_name_right_align_at means no alignment at
 // all.
-bool DumpRegisterValue(const RegisterValue ®_val, Stream *s,
+void DumpRegisterValue(const RegisterValue ®_val, Stream *s,
const RegisterInfo *reg_info, bool prefix_with_name,
bool prefix_with_alt_name, lldb::Format format,
uint32_t reg_name_right_align_at = 0,

diff  --git a/lldb/source/Core/DumpRegisterValue.cpp 
b/lldb/source/Core/DumpRegisterValue.cpp
index 0417ea9553419..14659e03f18b4 100644
--- a/lldb/source/Core/DumpRegisterValue.cpp
+++ b/lldb/source/Core/DumpRegisterValue.cpp
@@ -15,66 +15,65 @@
 
 using namespace lldb;
 
-bool lldb_private::DumpRegisterValue(const RegisterValue ®_val, Stream *s,
+void lldb_private::DumpRegisterValue(const RegisterValue ®_val, Stream *s,
  const RegisterInfo *reg_info,
  bool prefix_with_name,
  bool prefix_with_alt_name, Format format,
  uint32_t reg_name_right_align_at,
  ExecutionContextScope *exe_scope) {
   DataExtractor data;
-  if (reg_val.GetData(data)) {
-bool name_printed = false;
-// For simplicity, alignment of the register name printing applies only in
-// the most common case where:
-//
-// prefix_with_name^prefix_with_alt_name is true
-//
-StreamString format_string;
-if (reg_name_right_align_at && (prefix_with_name ^ prefix_with_alt_name))
-  format_string.Printf("%%%us", reg_name_right_align_at);
-else
-  format_string.Printf("%%s");
-std::string fmt = std::string(format_string.GetString());
-if (prefix_with_name) {
-  if (reg_info->name) {
-s->Printf(fmt.c_str(), reg_info->name);
-name_printed = true;
-  } else if (reg_info->alt_name) {
-s->Printf(fmt.c_str(), reg_info->alt_name);
-prefix_with_alt_name = false;
-name_printed = true;
-  }
-}
-if (prefix_with_alt_name) {
-  if (name_printed)
-s->PutChar('/');
-  if (reg_info->alt_name) {
-s->Printf(fmt.c_str(), reg_info->alt_name);
-name_printed = true;
-  } else if (!name_printed) {
-// No alternate name but we were asked to display a name, so show the
-// main name
-s->Printf(fmt.c_str(), reg_info->name);
-name_printed = true;
-  }
+  if (!reg_val.GetData(data))
+return;
+
+  bool name_printed = false;
+  // For simplicity, alignment of the register name printing applies only in
+  // the most common case where:
+  //
+  // prefix_with_name^prefix_with_alt_name is true
+  //
+  StreamString format_string;
+  if (reg_name_right_align_at && (prefix_with_name ^ prefix_with_alt_name))
+format_string.Printf("%%%us", reg_name_right_align_at);
+  else
+format_string.Printf("%%s");
+  std::string fmt = std::string(format_string.GetString());
+  if (prefix_with_name) {
+if (reg_info->name) {
+  s->Printf(fmt.c_str(), reg_info->name);
+  name_printed = true;
+} else if (reg_info->alt_name) {
+  s->Printf(fmt.c_str(), reg_info->alt_name);
+  prefix_with_alt_name = false;
+  name_printed = true;
 }
+  }
+  if (prefix_with_alt_name) {
 if (name_printed)
-  s->PutCString(" = ");
+  s->PutChar('/');
+if (reg_info->alt_name) {
+  s->Printf(fmt.c_str(), reg_info->alt_name);
+  name_printed = true;
+} else if (!name_printed) {
+  // No alternate name but we were asked to display a name, so show the
+  // main name
+  s->Printf(fmt.c_str(), reg_info->name);
+  name_printed = true;
+}
+  }
+  if (name_printed)
+s->PutCString(" = ");
 
-if (format == eFormatDefault)
-  format = reg_info->format;
+  if (format == eFormatDefault)
+format = reg_info->format;
 
-DumpDataExtractor(data, s,
-  0,/

[Lldb-commits] [PATCH] D141687: [LLDB] Remove return value from DumpRegisterValue

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGae361d3d90a8: [LLDB] Remove return value from 
DumpRegisterValue (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141687

Files:
  lldb/include/lldb/Core/DumpRegisterValue.h
  lldb/source/Core/DumpRegisterValue.cpp

Index: lldb/source/Core/DumpRegisterValue.cpp
===
--- lldb/source/Core/DumpRegisterValue.cpp
+++ lldb/source/Core/DumpRegisterValue.cpp
@@ -15,66 +15,65 @@
 
 using namespace lldb;
 
-bool lldb_private::DumpRegisterValue(const RegisterValue ®_val, Stream *s,
+void lldb_private::DumpRegisterValue(const RegisterValue ®_val, Stream *s,
  const RegisterInfo *reg_info,
  bool prefix_with_name,
  bool prefix_with_alt_name, Format format,
  uint32_t reg_name_right_align_at,
  ExecutionContextScope *exe_scope) {
   DataExtractor data;
-  if (reg_val.GetData(data)) {
-bool name_printed = false;
-// For simplicity, alignment of the register name printing applies only in
-// the most common case where:
-//
-// prefix_with_name^prefix_with_alt_name is true
-//
-StreamString format_string;
-if (reg_name_right_align_at && (prefix_with_name ^ prefix_with_alt_name))
-  format_string.Printf("%%%us", reg_name_right_align_at);
-else
-  format_string.Printf("%%s");
-std::string fmt = std::string(format_string.GetString());
-if (prefix_with_name) {
-  if (reg_info->name) {
-s->Printf(fmt.c_str(), reg_info->name);
-name_printed = true;
-  } else if (reg_info->alt_name) {
-s->Printf(fmt.c_str(), reg_info->alt_name);
-prefix_with_alt_name = false;
-name_printed = true;
-  }
-}
-if (prefix_with_alt_name) {
-  if (name_printed)
-s->PutChar('/');
-  if (reg_info->alt_name) {
-s->Printf(fmt.c_str(), reg_info->alt_name);
-name_printed = true;
-  } else if (!name_printed) {
-// No alternate name but we were asked to display a name, so show the
-// main name
-s->Printf(fmt.c_str(), reg_info->name);
-name_printed = true;
-  }
+  if (!reg_val.GetData(data))
+return;
+
+  bool name_printed = false;
+  // For simplicity, alignment of the register name printing applies only in
+  // the most common case where:
+  //
+  // prefix_with_name^prefix_with_alt_name is true
+  //
+  StreamString format_string;
+  if (reg_name_right_align_at && (prefix_with_name ^ prefix_with_alt_name))
+format_string.Printf("%%%us", reg_name_right_align_at);
+  else
+format_string.Printf("%%s");
+  std::string fmt = std::string(format_string.GetString());
+  if (prefix_with_name) {
+if (reg_info->name) {
+  s->Printf(fmt.c_str(), reg_info->name);
+  name_printed = true;
+} else if (reg_info->alt_name) {
+  s->Printf(fmt.c_str(), reg_info->alt_name);
+  prefix_with_alt_name = false;
+  name_printed = true;
 }
+  }
+  if (prefix_with_alt_name) {
 if (name_printed)
-  s->PutCString(" = ");
+  s->PutChar('/');
+if (reg_info->alt_name) {
+  s->Printf(fmt.c_str(), reg_info->alt_name);
+  name_printed = true;
+} else if (!name_printed) {
+  // No alternate name but we were asked to display a name, so show the
+  // main name
+  s->Printf(fmt.c_str(), reg_info->name);
+  name_printed = true;
+}
+  }
+  if (name_printed)
+s->PutCString(" = ");
 
-if (format == eFormatDefault)
-  format = reg_info->format;
+  if (format == eFormatDefault)
+format = reg_info->format;
 
-DumpDataExtractor(data, s,
-  0,// Offset in "data"
-  format,   // Format to use when dumping
-  reg_info->byte_size,  // item_byte_size
-  1,// item_count
-  UINT32_MAX,   // num_per_line
-  LLDB_INVALID_ADDRESS, // base_addr
-  0,// item_bit_size
-  0,// item_bit_offset
-  exe_scope);
-return true;
-  }
-  return false;
+  DumpDataExtractor(data, s,
+0,// Offset in "data"
+format,   // Format to use when dumping
+reg_info->byte_size,  // item_byte_size
+1,// item_count
+UINT32_MAX,   // num_per_line
+LLDB_INVALID_ADDRESS, // base_addr
+0,// item_bit_size
+  

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix a null pointer check in LibCxx.cpp

[Shivam Gupta via Phabricator via lldb-commits]

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

The issue is that the SyntheticChildrenFrontEnd constructor is being
called with the dereferenced valobj_sp pointer, before the valobj_sp
pointer is verified to be non-null inside the body of the constructor function.

To fix this issue, we have move the call to the SyntheticChildrenFrontEnd
constructor to after the null check for valobj_sp in the constructor
function body, so that it will only be called if valobj_sp is non-null.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142341

Files:
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -220,8 +220,9 @@
 
 lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
 LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {
+: m_pair_ptr(), m_pair_sp() {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -220,8 +220,9 @@
 
 lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
 LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {
+: m_pair_ptr(), m_pair_sp() {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix a null pointer check in LibCxx.cpp

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

Seems reasonable to me. Could you also apply the check to 
`LibCxxUnorderedMapIteratorSyntheticFrontEnd` which suffers from the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341

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

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix a null pointer check in LibCxx.cpp

[Shivam Gupta via Phabricator via lldb-commits]

xgupta added a comment.

In D142341#4072810 , @Michael137 
wrote:

> Looks correct to me, given we seem to allow `valobj_sp` to be null. Though we 
> seem to be doing it in a lot of other places too around this file. So could 
> fix those all at once?

Yeah I am doing that, will update in 30 minutes or so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341

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

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix a null pointer check in LibCxx.cpp

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 491288.
xgupta added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341

Files:
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxInitializerList.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
  lldb/source/Plugins/Language/ObjC/NSArray.cpp
  lldb/source/Plugins/Language/ObjC/NSSet.cpp

Index: lldb/source/Plugins/Language/ObjC/NSSet.cpp
===
--- lldb/source/Plugins/Language/ObjC/NSSet.cpp
+++ lldb/source/Plugins/Language/ObjC/NSSet.cpp
@@ -401,8 +401,9 @@
 
 lldb_private::formatters::NSSetISyntheticFrontEnd::NSSetISyntheticFrontEnd(
 lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref() {
+: m_exe_ctx_ref() {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
@@ -668,9 +669,9 @@
 template 
 lldb_private::formatters::GenericNSSetMSyntheticFrontEnd<
 D32, D64>::GenericNSSetMSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(),
-  m_data_32(nullptr), m_data_64(nullptr) {
+: m_exe_ctx_ref(), m_data_32(nullptr), m_data_64(nullptr) {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
Index: lldb/source/Plugins/Language/ObjC/NSArray.cpp
===
--- lldb/source/Plugins/Language/ObjC/NSArray.cpp
+++ lldb/source/Plugins/Language/ObjC/NSArray.cpp
@@ -461,8 +461,9 @@
 
 lldb_private::formatters::NSArrayMSyntheticFrontEndBase::
 NSArrayMSyntheticFrontEndBase(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(), m_id_type() {
+: m_exe_ctx_ref(), m_id_type() {
   if (valobj_sp) {
+SyntheticChildrenFrontEnd(*valobj_sp);
 TypeSystemClangSP scratch_ts_sp = ScratchTypeSystemClang::GetForTarget(
 *valobj_sp->GetExecutionContextRef().GetTargetSP());
 if (scratch_ts_sp)
@@ -604,9 +605,9 @@
 template 
 lldb_private::formatters::GenericNSArrayISyntheticFrontEnd::
 GenericNSArrayISyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(),
-  m_data_32(nullptr), m_data_64(nullptr) {
+: m_exe_ctx_ref(), m_data_32(nullptr), m_data_64(nullptr) {
   if (valobj_sp) {
+SyntheticChildrenFrontEnd(*valobj_sp);
 CompilerType type = valobj_sp->GetCompilerType();
 if (type) {
   TypeSystemClangSP scratch_ts_sp = ScratchTypeSystemClang::GetForTarget(
Index: lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
@@ -79,9 +79,10 @@
 
 LibstdcppMapIteratorSyntheticFrontEnd::LibstdcppMapIteratorSyntheticFrontEnd(
 lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(), m_pair_type(),
+: m_exe_ctx_ref(), m_pair_type(),
   m_pair_sp() {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
@@ -179,9 +180,9 @@
 lldb_private::formatters::VectorIteratorSyntheticFrontEnd::
 VectorIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp,
 llvm::ArrayRef item_names)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(),
-  m_item_names(item_names), m_item_sp() {
+: m_exe_ctx_ref(), m_item_names(item_names), m_item_sp() {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
@@ -350,9 +351,9 @@
 }
 
 LibStdcppSharedPtrSyntheticFrontEnd::LibStdcppSharedPtrSyntheticFrontEnd(
-lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp) {
+lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
@@ -69,8 +69,9 @@
 
 lldb_private::formatters::LibcxxStdVectorSyntheticFrontEnd::
 LibcxxStdVectorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp), m_element_type() {
+: m_element_type() {
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();
 }
 
@@ -172,9 +173,9 @@
 
 lldb_private::formatters::L

[Lldb-commits] [lldb] 291a7fc - [LLDB] Fix build error after D142214

[Jay Foad via lldb-commits]

Author: Jay Foad
Date: 2023-01-23T12:28:06Z
New Revision: 291a7fcf70db4d45c24b559fc867d3499b2e1e04

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

LOG: [LLDB] Fix build error after D142214

Added: 


Modified: 
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp 
b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index 6ae5f2ad9a2c..ed3b3e6da02b 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1384,7 +1384,7 @@ bool 
DisassemblerLLVMC::MCDisasmInstance::IsLoad(llvm::MCInst &mc_inst) const {
 
 bool DisassemblerLLVMC::MCDisasmInstance::IsAuthenticated(
 llvm::MCInst &mc_inst) const {
-  auto InstrDesc = m_instr_info_up->get(mc_inst.getOpcode());
+  const auto &InstrDesc = m_instr_info_up->get(mc_inst.getOpcode());
 
   // Treat software auth traps (brk 0xc470 + aut key, where 0x70 == 'p', 0xc4
   // == 'a' + 'c') as authenticated instructions for reporting purposes, in



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

[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for your response, Jim.

In D141605#4066649 , @jingham wrote:

> The part of handling the fork where we decide we're going to follow the child 
> and so we need to switch the process PID & TID does have to happen on event 
> receipt.  The point there is that until the client pulls an event from the 
> public event queue, it doesn't know that the state change has happened yet.  
> We don't want the current state that you access with GetState, GetPID, 
> GetSelectedThread, etc. to get out of sync with the event flow, so to the 
> public world it should appear to it as if the process were in the state of 
> the last event it actually received.  Having the PID or TID change before the 
> fork event gets pulled off the queue would break this illusion.
>
> However, the state of the process we aren't following isn't relevant to the 
> state of the process we are  following, so the part of the DidFork dealing 
> with the discarded side of the fork can happen when the fork event is 
> received, in ProcessGDBRemote::SetThreadStopInfo or thereabouts.  That does 
> seem like a better way to do this.

The first part makes sense to me, but I'm not sure how to reconcile it with the 
second paragraph. It seems to me that, if follow-fork-mode=child, it'd be 
pretty hard to detach from the original (parent) process early on, but still 
have the public API report on its state until the fork event gets actually 
pulled from the event queue. Technically, it's doable, but I think we'd have to 
maintain separate "private" and "public" copies of most of the process state 
(thread lists, etc.). And I'm not sure how useful would that be given that when 
the process is running, it's hard to guarantee the correctness of a piece of 
information anyway.

> However, I don't think this patch is wrong in itself.  Formally, the step 
> thread plans might want to know that a fork has happened.  But it's not 
> terribly important as they can't really do anything about except maybe report 
> an error.  And the same could be said of exec events which are already in 
> this list.  What's actually going to happen is once we've stopped for the 
> fork, all the step plans will be asked whether they are Stale, and given the 
> thread they were on is no longer present, they should unship themselves.  So 
> we're not losing anything by bypassing the plans for this event delivery.

Ok, so shall we go with this patch then?

> Note, it might be worth checking that the stepping thread plan's ShouldStop 
> check the  PID, not just the TID.  It's possible that the new TID was the 
> same as the old TID and the plan might try to keep operating, which would be 
> wrong.

AFAICT, the plans do not check the process PID, but ShouldStop seems like it's 
a bit too late for a plan to figure out it's working with a completely 
different thread that it was supposed to have. Wouldn't it be better to just 
remove/flush all the thread plans upon forking (if following the child)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141605

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

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:225
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();

this won't initialise the parent constructor though will it? Just creates a 
temporary and immediately destructs it? You might need to put it back into the 
initialiser list and use a ternary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341

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

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:225
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();

Michael137 wrote:
> this won't initialise the parent constructor though will it? Just creates a 
> temporary and immediately destructs it? You might need to put it back into 
> the initialiser list and use a ternary
Oh but `SyntheticChildrenFrontEnd` seems to always take a reference. The 
interfaces seem to be a little mismatched


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341

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

[Lldb-commits] [PATCH] D139957: [LLDB] Change OSO to use DieRef

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:32
 
-  DIERef(std::optional dwo_num, Section section,
+  DIERef(std::optional dwo_oso_num, Section section,
  dw_offset_t die_offset)

Where is this constructor being used?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp:10
 #include "SymbolFileDWARFDebugMap.h"
+#include "DIERef.h"
 #include "DWARFCompileUnit.h"

unused include?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139957

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

[Lldb-commits] [PATCH] D142341: [LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a subscriber: jingham.
Michael137 added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:225
   if (valobj_sp)
+SyntheticChildrenFrontEnd(*valobj_sp);
 Update();

Michael137 wrote:
> Michael137 wrote:
> > this won't initialise the parent constructor though will it? Just creates a 
> > temporary and immediately destructs it? You might need to put it back into 
> > the initialiser list and use a ternary
> Oh but `SyntheticChildrenFrontEnd` seems to always take a reference. The 
> interfaces seem to be a little mismatched
Perhaps the null-check in the constructor is just redundant and should actually 
be an assert.

@jingham might know more about the assumptions here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142341

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

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D138618#4060565 , @clayborg wrote:

> ...
> Since the user IDs of SymbolFileDWARF plug-ins mean nothing to anyone else, 
> we can make them what we need them to be so they work for us. I would suggest 
> to remove the use of DIERef from calculating the IDs of symbol files and have 
> .o files for mac and .dwo files for fission use a 1 based index as their ID 
> to make it easy to encode into a DIERef when needed for lldb::user_id_t 
> values that _are_ included in objects that we hand out. Is there anything 
> else that would need to be done to keep everyone happy here?

I think that the 1-based index thingy helps a lot here, but I haven't seen 
anything (in your reponse, or in the new patch) that would address my concernt 
DIERef<->user_id conversion ambiguity. I.e. how is one supposed to know what is 
the "right" way to convert a DIERef to a user_id:

- `die_ref.get_id()`
- or `symbol_file.GetUID(die_ref)` (which, funnily enough, will construct 
another DIERef, and *then* call get_id? (`return DIERef(GetID(), ref.section(), 
ref.die_offset()).get_id();`)

What's your position on that? That we should live with the ambiguity?




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:53
+
+  void set_die_offset(dw_offset_t offset) {
+lldbassert(offset <= DW_INVALID_OFFSET);

Can we remove this function now?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp:11
 
+#include "DIERef.h"
 #include "lldb/Core/Section.h"

nor this



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h:12
 
+#include "DIERef.h"
 #include "SymbolFileDWARF.h"

I guess this isn't necessary anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618

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

[Lldb-commits] [PATCH] D142150: [lldb] Remove timer from SBModule copy ctor

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

What about adding a `trivial` parameter to the macro that has the effect of 
skipping it in instrumentation if the instrumentation is the costly signpost 
mechanism?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142150

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

[Lldb-commits] [PATCH] D142150: [lldb] Remove timer from SBModule copy ctor

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

In D142150#4074015 , @aprantl wrote:

> What about adding a `trivial` parameter to the macro that has the effect of 
> skipping it in instrumentation if the instrumentation is the costly signpost 
> mechanism?

sounds good to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142150

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

[Lldb-commits] [lldb] 9bf0318 - Revert "[lldb] Remove timer from SBModule copy ctor"

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2023-01-23T09:52:11-08:00
New Revision: 9bf03187bd29f42902183e4bdac9b7f2d3008d00

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

LOG: Revert "[lldb] Remove timer from SBModule copy ctor"

This reverts commit 84c6129c943135e2c32b9254f08d0a2e7b21116a.

Added: 


Modified: 
lldb/source/API/SBModule.cpp

Removed: 




diff  --git a/lldb/source/API/SBModule.cpp b/lldb/source/API/SBModule.cpp
index c6bda9da9f61..e7c2b451eb9d 100644
--- a/lldb/source/API/SBModule.cpp
+++ b/lldb/source/API/SBModule.cpp
@@ -43,7 +43,9 @@ SBModule::SBModule(const SBModuleSpec &module_spec) {
 SetSP(module_sp);
 }
 
-SBModule::SBModule(const SBModule &rhs) = default;
+SBModule::SBModule(const SBModule &rhs) : m_opaque_sp(rhs.m_opaque_sp) {
+  LLDB_INSTRUMENT_VA(this, rhs);
+}
 
 SBModule::SBModule(lldb::SBProcess &process, lldb::addr_t header_addr) {
   LLDB_INSTRUMENT_VA(this, process, header_addr);



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

[Lldb-commits] [PATCH] D141910: [OpenMP][OMPIRBuilder]Move SIMD alignment calculation to LLVM Frontend

[Jan Sjödin via Phabricator via lldb-commits]

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

This looks good to me. Have @jdoerfert approve as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141910

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

[Lldb-commits] [PATCH] D142150: [lldb] Remove timer from SBModule copy ctor

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D142150#4074015 , @aprantl wrote:

> What about adding a `trivial` parameter to the macro that has the effect of 
> skipping it in instrumentation if the instrumentation is the costly signpost 
> mechanism?

The `_VA` macro is variadic so you'd have to define a new macro, but yeah 
that's the idea. As discussed offline with Dave, we probably never want 
timers/signposts for constructors. My suggestion is to re-instrument all 
constructors with that new "trivial" macro.  During the reproducer era 
constructs required different macros, so there should be code in `lldb-instr` 
that can already tell the two apart.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142150

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

[Lldb-commits] [lldb] 484bc2b - Run cmdline address expressions through ABI's FixAddress

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-01-23T10:44:19-08:00
New Revision: 484bc2bcc7990f4ecaf40f3d806ed870cdbdfd95

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

LOG: Run cmdline address expressions through ABI's FixAddress

On systems like ARM, where the non-addressable bits of a pointer
value may be used for metadata (ARMv8.3 pointer authentication, or
Type Byte Ignore), those bits need to be cleared before the address
points to a valid memory location.  Add a call to the target's ABI
to clear those from address expression arguments to the lldb
commands (e.g. `disassemble -a`).

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

Added: 
lldb/test/API/macosx/ptrauth-address-expressions/Makefile

lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
lldb/test/API/macosx/ptrauth-address-expressions/main.c

Modified: 
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/source/Interpreter/OptionArgParser.cpp

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index d0501ef6b9d10..97fe14e769cdd 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1243,6 +1243,8 @@ def isAArch64MTE(self):
 return self.isAArch64() and "mte" in self.getCPUInfo()
 
 def isAArch64PAuth(self):
+if self.getArchitecture() == "arm64e":
+return True
 return self.isAArch64() and "paca" in self.getCPUInfo()
 
 def getArchitecture(self):

diff  --git a/lldb/source/Interpreter/OptionArgParser.cpp 
b/lldb/source/Interpreter/OptionArgParser.cpp
index 93b01abde4bb9..63ca0f9d3d4d9 100644
--- a/lldb/source/Interpreter/OptionArgParser.cpp
+++ b/lldb/source/Interpreter/OptionArgParser.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/DataFormatters/FormatManager.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
@@ -157,6 +158,10 @@ lldb::addr_t OptionArgParser::ToAddress(const 
ExecutionContext *exe_ctx,
   if (!s.getAsInteger(0, addr)) {
 if (error_ptr)
   error_ptr->Clear();
+Process *process = exe_ctx->GetProcessPtr();
+if (process)
+  if (ABISP abi_sp = process->GetABI())
+addr = abi_sp->FixCodeAddress(addr);
 return addr;
   }
 

diff  --git a/lldb/test/API/macosx/ptrauth-address-expressions/Makefile 
b/lldb/test/API/macosx/ptrauth-address-expressions/Makefile
new file mode 100644
index 0..10495940055b6
--- /dev/null
+++ b/lldb/test/API/macosx/ptrauth-address-expressions/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules

diff  --git 
a/lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
 
b/lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
new file mode 100644
index 0..3d21c20601cef
--- /dev/null
+++ 
b/lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
@@ -0,0 +1,28 @@
+"""Test that AArch64 PAC bits are stripped from address expression arguments"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestPtrauthAddressExpressions(TestBase):
+
+NO_DEBUG_INFO_TESTCASE = True
+
+# On Darwin systems, arch arm64e means ARMv8.3 with ptrauth
+# ABI used.
+@skipIf(archs=no_match(['arm64e']))
+
+def test(self):
+
+# Skip this test if not running on AArch64 target that supports PAC
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+self.source = 'main.c'
+self.build()
+(self.target, process, thread, bkpt) = 
lldbutil.run_to_source_breakpoint(self,
+   "break here", lldb.SBFileSpec(self.source, False))
+
+self.expect("p fptr", substrs=[self.source])
+self.expect("ima loo -va fptr", substrs=[self.source])
+self.expect("break set -a fptr", substrs=[self.source])

diff  --git a/lldb/test/API/macosx/ptrauth-address-expressions/main.c 
b/lldb/test/API/macosx/ptrauth-address-expressions/main.c
new file mode 100644
index 0..388de7f4b16e8
--- /dev/null
+++ b/lldb/test/API/macosx/ptrauth-address-expressions/main.c
@@ -0,0 +1,10 @@
+#include 
+
+int foo () { return 10; }
+
+int main () 
+{
+  int (*fptr)() = foo;
+  printf ("%p\n", fptr); // break here
+  return fptr();
+}



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

[Lldb-commits] [PATCH] D141629: Run address expression argument values through ABI::FixCodeAddress to strip TBI/pointer auth bytes on AArch64

[Jason Molenda via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG484bc2bcc799: Run cmdline address expressions through 
ABI's FixAddress (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141629

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Interpreter/OptionArgParser.cpp
  lldb/test/API/macosx/ptrauth-address-expressions/Makefile
  
lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
  lldb/test/API/macosx/ptrauth-address-expressions/main.c


Index: lldb/test/API/macosx/ptrauth-address-expressions/main.c
===
--- /dev/null
+++ lldb/test/API/macosx/ptrauth-address-expressions/main.c
@@ -0,0 +1,10 @@
+#include 
+
+int foo () { return 10; }
+
+int main () 
+{
+  int (*fptr)() = foo;
+  printf ("%p\n", fptr); // break here
+  return fptr();
+}
Index: 
lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
===
--- /dev/null
+++ 
lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
@@ -0,0 +1,28 @@
+"""Test that AArch64 PAC bits are stripped from address expression arguments"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestPtrauthAddressExpressions(TestBase):
+
+NO_DEBUG_INFO_TESTCASE = True
+
+# On Darwin systems, arch arm64e means ARMv8.3 with ptrauth
+# ABI used.
+@skipIf(archs=no_match(['arm64e']))
+
+def test(self):
+
+# Skip this test if not running on AArch64 target that supports PAC
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+self.source = 'main.c'
+self.build()
+(self.target, process, thread, bkpt) = 
lldbutil.run_to_source_breakpoint(self,
+   "break here", lldb.SBFileSpec(self.source, False))
+
+self.expect("p fptr", substrs=[self.source])
+self.expect("ima loo -va fptr", substrs=[self.source])
+self.expect("break set -a fptr", substrs=[self.source])
Index: lldb/test/API/macosx/ptrauth-address-expressions/Makefile
===
--- /dev/null
+++ lldb/test/API/macosx/ptrauth-address-expressions/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Interpreter/OptionArgParser.cpp
===
--- lldb/source/Interpreter/OptionArgParser.cpp
+++ lldb/source/Interpreter/OptionArgParser.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/DataFormatters/FormatManager.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
@@ -157,6 +158,10 @@
   if (!s.getAsInteger(0, addr)) {
 if (error_ptr)
   error_ptr->Clear();
+Process *process = exe_ctx->GetProcessPtr();
+if (process)
+  if (ABISP abi_sp = process->GetABI())
+addr = abi_sp->FixCodeAddress(addr);
 return addr;
   }
 
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1243,6 +1243,8 @@
 return self.isAArch64() and "mte" in self.getCPUInfo()
 
 def isAArch64PAuth(self):
+if self.getArchitecture() == "arm64e":
+return True
 return self.isAArch64() and "paca" in self.getCPUInfo()
 
 def getArchitecture(self):


Index: lldb/test/API/macosx/ptrauth-address-expressions/main.c
===
--- /dev/null
+++ lldb/test/API/macosx/ptrauth-address-expressions/main.c
@@ -0,0 +1,10 @@
+#include 
+
+int foo () { return 10; }
+
+int main () 
+{
+  int (*fptr)() = foo;
+  printf ("%p\n", fptr); // break here
+  return fptr();
+}
Index: lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
===
--- /dev/null
+++ lldb/test/API/macosx/ptrauth-address-expressions/TestPtrauthAddressExpressions.py
@@ -0,0 +1,28 @@
+"""Test that AArch64 PAC bits are stripped from address expression arguments"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestPtrauthAddressExpressions(TestBase):
+
+NO_DEBUG_INFO_TESTCASE = True
+
+# On Darwin systems, arch arm64e means ARMv8.3 with ptrauth
+# ABI used.
+@skipIf(archs=no_match(['arm64e']))
+
+def test(self):
+
+# Skip this test if not r

[Lldb-commits] [PATCH] D142413: [lldb] Don't create Clang AST nodes in GetCPlusPlusQualifiedName

[Arthur Eubanks via Phabricator via lldb-commits]

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

Otherwise we may be inserting a decl into a DeclContext that's not fully 
defined yet.

GetCPlusPlusQualifiedName() doesn't care about exact spacing between '>', the 
names it gives just need to be unique. Places that use 
GetDIEClassTemplateParams() do care about exact spacing (at least until that 
issue is solved generally).

Reverts part of D138834 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142413

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/test/API/lang/cpp/nested-template/Makefile
  lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
  lldb/test/API/lang/cpp/nested-template/main.cpp

Index: lldb/test/API/lang/cpp/nested-template/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/nested-template/main.cpp
@@ -0,0 +1,11 @@
+struct Outer {
+  Outer() {}
+
+  template 
+  struct Inner {};
+};
+
+int main() {
+  Outer::Inner oi;
+  // Set breakpoint here
+}
Index: lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
@@ -0,0 +1,24 @@
+"""
+Test that a nested template parameter works with simple template names.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class NestedTemplateTestCase(TestBase):
+def do_test(self, debug_flags):
+self.build(dictionary=debug_flags)
+lldbutil.run_to_source_breakpoint(self, "// Set breakpoint here", lldb.SBFileSpec("main.cpp"))
+self.expect("image lookup -A -t 'Inner'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
+
+@skipIf(compiler=no_match("clang"))
+@skipIf(compiler_version=["<", "15.0"])
+def test_simple_template_names(self):
+self.do_test(dict(TEST_CFLAGS_EXTRAS="-gsimple-template-names"))
+
+@skipIf(compiler=no_match("clang"))
+@skipIf(compiler_version=["<", "15.0"])
+def test_no_simple_template_names(self):
+self.do_test(dict(TEST_CFLAGS_EXTRAS="-gno-simple-template-names"))
Index: lldb/test/API/lang/cpp/nested-template/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/nested-template/Makefile
@@ -0,0 +1,5 @@
+CXX_SOURCES := main.cpp
+
+CFLAGS_EXTRAS = $(TEST_CFLAGS_EXTRAS) $(LIMIT_DEBUG_INFO_FLAGS)
+
+include Makefile.rules
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -95,10 +95,10 @@
   /// parameters from the DIE name and instead always adds template parameter
   /// children DIEs.
   ///
-  /// Currently this is only called in two places, when uniquing C++ classes and
-  /// when looking up the definition for a declaration (which is then cached).
-  /// If this is ever called more than twice per DIE, we need to start caching
-  /// the results to prevent unbounded growth of the created clang AST nodes.
+  /// Currently this is only called when looking up the definition for a
+  /// declaration (which is then cached).  If this is ever called more than once
+  /// per DIE, we need to start caching the results to prevent unbounded growth
+  /// of the created clang AST nodes.
   ///
   /// \param die The struct/class DWARFDIE containing template parameters.
   /// \return A string, including surrounding '<>', of the template parameters.
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1530,6 +1530,41 @@
   return type_sp;
 }
 
+std::string
+DWARFASTParserClang::GetTemplateParametersString(const DWARFDIE &die) {
+  if (llvm::StringRef(die.GetName()).contains("<"))
+return std::string();
+  TypeSystemClang::TemplateParameterInfos template_param_infos;
+  if (!ParseTemplateParameterInfos(die, template_param_infos))
+return std::string();
+  std::string all_template_names;
+  llvm::SmallVector args =
+  template_param_infos.args;
+  if (template_param_infos.hasParameterPack())
+args.append(template_param_infos.packed_args->args);
+  if (args.empty())
+return std::string();
+  for (auto &arg : args) {
+std::string template_name;
+llvm::raw_string_ostream os(template_name);
+arg.print

[Lldb-commits] [PATCH] D142413: [lldb] Don't create Clang AST nodes in GetCPlusPlusQualifiedName

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks added a comment.

there may be an alternate solution involving making some declarations into 
definitions (at all? earlier?), but I don't have that level of understanding of 
lldb


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142413

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

[Lldb-commits] [PATCH] D141219: Add a .lldbinit file to autoload LLVM/Clang data formatters

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

IMO the correct solution to this problem is for the IDE to give affordances for 
specifying either absolute or project relative paths to "the debugger init 
file".  Then we don't have to clutter our sources with .lldbinit files in weird 
places hoping to catch the attention of one IDE or another.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141219

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

[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

[Jim Ingham via Phabricator via lldb-commits]

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

In D141605#4073158 , @labath wrote:

> Thanks for your response, Jim.
>
> In D141605#4066649 , @jingham wrote:
>
>> The part of handling the fork where we decide we're going to follow the 
>> child and so we need to switch the process PID & TID does have to happen on 
>> event receipt.  The point there is that until the client pulls an event from 
>> the public event queue, it doesn't know that the state change has happened 
>> yet.  We don't want the current state that you access with GetState, GetPID, 
>> GetSelectedThread, etc. to get out of sync with the event flow, so to the 
>> public world it should appear to it as if the process were in the state of 
>> the last event it actually received.  Having the PID or TID change before 
>> the fork event gets pulled off the queue would break this illusion.
>>
>> However, the state of the process we aren't following isn't relevant to the 
>> state of the process we are  following, so the part of the DidFork dealing 
>> with the discarded side of the fork can happen when the fork event is 
>> received, in ProcessGDBRemote::SetThreadStopInfo or thereabouts.  That does 
>> seem like a better way to do this.
>
> The first part makes sense to me, but I'm not sure how to reconcile it with 
> the second paragraph. It seems to me that, if follow-fork-mode=child, it'd be 
> pretty hard to detach from the original (parent) process early on, but still 
> have the public API report on its state until the fork event gets actually 
> pulled from the event queue. Technically, it's doable, but I think we'd have 
> to maintain separate "private" and "public" copies of most of the process 
> state (thread lists, etc.). And I'm not sure how useful would that be given 
> that when the process is running, it's hard to guarantee the correctness of a 
> piece of information anyway.

I don't think this is an actual problem currently in lldb.  After all, our 
current process model requires that when a process is stopped, it is stopped 
all the way and can't change state on us (so no new fork events).  And when 
it's running you can't ask questions about (other than whether it is in the 
running state) till you stop it.  Since you can't fork while you are stopped, 
the fork will always happen while the public state is Running, at which point 
you can only ask  "what is your state" or request an interruption.  And by the 
time you have fetched the Stop event so that you can start asking questions the 
parent -> child transition will have already happened, and you will have to 
factor that new information into the questions you were going to ask anyway.

So I don't actually think it would cause problems to handle releasing the child 
earlier.

This is one of the things we're going to have to handle more carefully once we 
actually do "no stop" debugging.  We're relying in a bunch of places on the 
simplification that when you're stopped, nothing can happen, and when you're 
running you have to stop first before you can ask questions of the process.  In 
this case, however, I don't think it will be that hard.  We can just set lldb 
up to always follow both sides of the fork, making a target for each.  Then 
depending on the user settings we can kill off one or the other at our leisure, 
and be able to ask questions of either in the meantime.

>> However, I don't think this patch is wrong in itself.  Formally, the step 
>> thread plans might want to know that a fork has happened.  But it's not 
>> terribly important as they can't really do anything about except maybe 
>> report an error.  And the same could be said of exec events which are 
>> already in this list.  What's actually going to happen is once we've stopped 
>> for the fork, all the step plans will be asked whether they are Stale, and 
>> given the thread they were on is no longer present, they should unship 
>> themselves.  So we're not losing anything by bypassing the plans for this 
>> event delivery.
>
> Ok, so shall we go with this patch then?

You should do this patch anyway.  It doesn't seem like the step plans should be 
the ones handling fork events, just like they aren't expected to handle exec 
events.  So this change is good anyway.

If it's not going to cause a problem to postpone continuing the side of the 
fork you aren't following to the next event receipt, then this patch should 
suffice.

>> Note, it might be worth checking that the stepping thread plan's ShouldStop 
>> check the  PID, not just the TID.  It's possible that the new TID was the 
>> same as the old TID and the plan might try to keep operating, which would be 
>> wrong.
>
> AFAICT, the plans do not check the process PID, but ShouldStop seems like 
> it's a bit too late for a plan to figure out it's working with a completely 
> different thread that it was sup

[Lldb-commits] [PATCH] D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

I don't think this code is going to compile, is it?  You took out the `if 
(!buffer) {` but left the RHS `}` so now the scopes are unbalanced.  I think 
you should just change how the shared pointer is defined - which is presumably 
the substance of the patch - and leave everything after as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142309

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

[Lldb-commits] [PATCH] D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 491597.
xgupta added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142309

Files:
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp


Index: 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
@@ -2368,7 +2368,7 @@
 
   // Allocate a buffer to copy data into
   const uint32_t size = *alloc->size.get();
-  std::shared_ptr buffer(new uint8_t[size]);
+  std::shared_ptr buffer(new uint8_t[size]);
   if (!buffer) {
 LLDB_LOGF(log, "%s - couldn't allocate a %" PRIu32 " byte buffer",
   __FUNCTION__, size);
@@ -2695,7 +2695,7 @@
   }
 
   // Create the headers describing the element type of the allocation.
-  std::shared_ptr element_header_buffer(
+  std::shared_ptr element_header_buffer(
   new uint8_t[element_header_size]);
   if (element_header_buffer == nullptr) {
 strm.Printf("Internal Error: Couldn't allocate %" PRIu64


Index: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
===
--- lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
+++ lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
@@ -2368,7 +2368,7 @@
 
   // Allocate a buffer to copy data into
   const uint32_t size = *alloc->size.get();
-  std::shared_ptr buffer(new uint8_t[size]);
+  std::shared_ptr buffer(new uint8_t[size]);
   if (!buffer) {
 LLDB_LOGF(log, "%s - couldn't allocate a %" PRIu32 " byte buffer",
   __FUNCTION__, size);
@@ -2695,7 +2695,7 @@
   }
 
   // Create the headers describing the element type of the allocation.
-  std::shared_ptr element_header_buffer(
+  std::shared_ptr element_header_buffer(
   new uint8_t[element_header_size]);
   if (element_header_buffer == nullptr) {
 strm.Printf("Internal Error: Couldn't allocate %" PRIu64
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp

[Shivam Gupta via Phabricator via lldb-commits]

xgupta added a comment.

In D142309#4075616 , @jingham wrote:

> I don't think this code is going to compile, is it?  You took out the `if 
> (!buffer) {` but left the RHS `}` so now the scopes are unbalanced.  I think 
> you should just change how the shared pointer is defined - which is 
> presumably the substance of the patch - and leave everything after as is.

Yeah, correct, I didn't notice carefully. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142309

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

[Lldb-commits] [PATCH] D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a subscriber: labath.
JDevlieghere added a comment.

LGTM, but I wonder when we'll be able to get rid of RenderScript completely (CC 
@labath)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142309

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

[Lldb-commits] [PATCH] D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 491601.
xgupta added a comment.

fix build


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142309

Files:
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h


Index: 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h
===
--- 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h
+++ 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h
@@ -528,7 +528,7 @@
 
   AllocationDetails *FindAllocByID(Stream &strm, const uint32_t alloc_id);
 
-  std::shared_ptr GetAllocationData(AllocationDetails *alloc,
+  std::shared_ptr GetAllocationData(AllocationDetails *alloc,
  StackFrame *frame_ptr);
 
   void SetElementSize(Element &elem);
@@ -538,7 +538,7 @@
 
   void FindStructTypeName(Element &elem, StackFrame *frame_ptr);
 
-  size_t PopulateElementHeaders(const std::shared_ptr header_buffer,
+  size_t PopulateElementHeaders(const std::shared_ptr 
header_buffer,
 size_t offset, const Element &elem);
 
   size_t CalculateElementHeaderSize(const Element &elem);
Index: 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
@@ -2346,7 +2346,7 @@
 // Given an allocation, this function copies the allocation contents from
 // device into a buffer on the heap. Returning a shared pointer to the buffer
 // containing the data.
-std::shared_ptr
+std::shared_ptr
 RenderScriptRuntime::GetAllocationData(AllocationDetails *alloc,
StackFrame *frame_ptr) {
   Log *log = GetLog(LLDBLog::Language);
@@ -2368,7 +2368,7 @@
 
   // Allocate a buffer to copy data into
   const uint32_t size = *alloc->size.get();
-  std::shared_ptr buffer(new uint8_t[size]);
+  std::shared_ptr buffer(new uint8_t[size]);
   if (!buffer) {
 LLDB_LOGF(log, "%s - couldn't allocate a %" PRIu32 " byte buffer",
   __FUNCTION__, size);
@@ -2557,7 +2557,7 @@
 // saved to the file as the ElementHeader struct followed by offsets to the
 // structs of all the element's children.
 size_t RenderScriptRuntime::PopulateElementHeaders(
-const std::shared_ptr header_buffer, size_t offset,
+const std::shared_ptr header_buffer, size_t offset,
 const Element &elem) {
   // File struct for an element header with all the relevant details copied
   // from elem. We assume members are valid already.
@@ -2661,7 +2661,7 @@
   }
 
   // Read allocation into buffer of heap memory
-  const std::shared_ptr buffer = GetAllocationData(alloc, frame_ptr);
+  const std::shared_ptr buffer = GetAllocationData(alloc, 
frame_ptr);
   if (!buffer) {
 strm.Printf("Error: Couldn't read allocation data into buffer");
 strm.EOL();
@@ -2695,7 +2695,7 @@
   }
 
   // Create the headers describing the element type of the allocation.
-  std::shared_ptr element_header_buffer(
+  std::shared_ptr element_header_buffer(
   new uint8_t[element_header_size]);
   if (element_header_buffer == nullptr) {
 strm.Printf("Internal Error: Couldn't allocate %" PRIu64
@@ -3214,7 +3214,7 @@
 __FUNCTION__, data_size);
 
   // Allocate a buffer to copy data into
-  std::shared_ptr buffer = GetAllocationData(alloc, frame_ptr);
+  std::shared_ptr buffer = GetAllocationData(alloc, frame_ptr);
   if (!buffer) {
 strm.Printf("Error: Couldn't read allocation data");
 strm.EOL();


Index: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h
===
--- lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h
+++ lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h
@@ -528,7 +528,7 @@
 
   AllocationDetails *FindAllocByID(Stream &strm, const uint32_t alloc_id);
 
-  std::shared_ptr GetAllocationData(AllocationDetails *alloc,
+  std::shared_ptr GetAllocationData(AllocationDetails *alloc,
  StackFrame *frame_ptr);
 
   void SetElementSize(Element &elem);
@@ -538,7 +538,7 @@
 
   void FindStructTypeName(Element &elem, StackFrame *frame_ptr);
 
-  size_t PopulateElementHeaders(const std::shared_ptr header_buffer,
+  size_t PopulateElementHeaders(const std::shared_ptr header_buffer,
 size_t