[Lldb-commits] [PATCH] D154759: [lldb][NFCI] Change type of UnwindPlan::m_source_name

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D154759#4487381 , @jasonmolenda 
wrote:

> I guess simplest is we can have all the UnwindPlan creators have a `static 
> g_unwindplan_name` and save a pointer to that.  Each one is unique, don't 
> need anything fancier than that. There's the risk that someone will create a 
> new UnwindPlan and forget to make it a static string, so unwind logging would 
> try to print random memory, but it wouldn't break normal debugging.

I was thinking about this earlier, since we know what all the possible strings 
are going to be, we can create them all statically at compile time and just 
point to them. We can refactor a bit and then most of these source names just 
become `static const char *` or `static constexpr llvm::StringLiteral` or 
something like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154759

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

[Lldb-commits] [PATCH] D154759: [lldb][NFCI] Change type of UnwindPlan::m_source_name

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I guess simplest is we can have all the UnwindPlan creators have a `static 
g_unwindplan_name` and save a pointer to that.  Each one is unique, don't need 
anything fancier than that. There's the risk that someone will create a new 
UnwindPlan and forget to make it a static string, so unwind logging would try 
to print random memory, but it wouldn't break normal debugging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154759

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

[Lldb-commits] [PATCH] D154759: [lldb][NFCI] Change type of UnwindPlan::m_source_name

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

probably the real correct change is to have constant c-strings in as globals 
and have pointers to those.  Then you're using the same space as a ConstString 
in each UnwindPlan (a pointer) but creation is cheaper.  There's no dynamic 
aspect to the names when the UnwindPlans are created, each method creating them 
knows what the name will be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154759

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

[Lldb-commits] [PATCH] D154759: [lldb][NFCI] Change type of UnwindPlan::m_source_name

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

(to say it more clearly: I'm fine if you'd like to reduce the places where 
ConstStrings are being used, but I don't think there's any good reason to 
remove this use of them.  It will increase our memory footprint a bit.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154759

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

[Lldb-commits] [PATCH] D154759: [lldb][NFCI] Change type of UnwindPlan::m_source_name

[Jason Molenda via Phabricator via lldb-commits]

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

I'm fine with this.  It does increase the size of the UnwindPlan object, and 
there are a couple of them for every function that we see on a stack during a 
program execution, but I believe that won't be more than a few hundred.  These 
are short strings so they won't expand to use heap, they will fit inline.  
Strictly I think it is more memory efficient to leave these half-dozen strings 
in the constant string pool than to have an extra 24 bytes in each UnwindPlan 
with them duplicated, but it's not enough memory use for me to be actually 
worried about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154759

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

[Lldb-commits] [PATCH] D154759: [lldb][NFCI] Change type of UnwindPlan::m_source_name

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, mib, jasonmolenda, jingham.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

m_source_name is currently a ConstString. Comparing the source names are
not a performance-critical operation, so that portion isn't really
necessary. It may benefit from deduplication (since there are only
a handful of possible values) but this means they will stick around
forever in the StringPool, even if they are never used again. As such, I
think it would be reasonable to change its type to `std::string`
instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154759

Files:
  lldb/include/lldb/Symbol/UnwindPlan.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
  lldb/source/Symbol/UnwindPlan.cpp
  lldb/source/Target/RegisterContextUnwind.cpp

Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -300,9 +300,8 @@
   UnwindLogMsg("initialized frame current pc is 0x%" PRIx64 " cfa is 0x%" PRIx64
" afa is 0x%" PRIx64 " using %s UnwindPlan",
(uint64_t)m_current_pc.GetLoadAddress(exe_ctx.GetTargetPtr()),
-   (uint64_t)m_cfa,
-   (uint64_t)m_afa,
-   m_full_unwind_plan_sp->GetSourceName().GetCString());
+   (uint64_t)m_cfa, (uint64_t)m_afa,
+   m_full_unwind_plan_sp->GetSourceName().c_str());
 }
 
 // Initialize a RegisterContextUnwind for the non-zeroth frame -- rely on the
@@ -645,7 +644,7 @@
   active_row->Dump(active_row_strm, m_fast_unwind_plan_sp.get(), _thread,
m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
   UnwindLogMsg("Using fast unwind plan '%s'",
-   m_fast_unwind_plan_sp->GetSourceName().AsCString());
+   m_fast_unwind_plan_sp->GetSourceName().c_str());
   UnwindLogMsg("active row: %s", active_row_strm.GetData());
 }
   } else {
@@ -661,7 +660,7 @@
  _thread,
  m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
 UnwindLogMsg("Using full unwind plan '%s'",
- m_full_unwind_plan_sp->GetSourceName().AsCString());
+ m_full_unwind_plan_sp->GetSourceName().c_str());
 UnwindLogMsg("active row: %s", active_row_strm.GetData());
   }
 }
@@ -957,7 +956,7 @@
 if (unwind_plan_sp && unwind_plan_sp->PlanValidAtAddress(m_current_pc)) {
   UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because the "
   "DynamicLoader suggested we prefer it",
-  unwind_plan_sp->GetSourceName().GetCString());
+  unwind_plan_sp->GetSourceName().c_str());
   return unwind_plan_sp;
 }
   }
@@ -995,7 +994,7 @@
   UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because this "
   "is the non-call site unwind plan and this is a "
   "zeroth frame",
-  unwind_plan_sp->GetSourceName().GetCString());
+  unwind_plan_sp->GetSourceName().c_str());
   return unwind_plan_sp;
 }
 
@@ -1007,9 +1006,10 @@
   func_unwinders_sp->GetUnwindPlanArchitectureDefaultAtFunctionEntry(
   m_thread);
   if (unwind_plan_sp) {
-UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because we are at "
-"the first instruction of a function",
-unwind_plan_sp->GetSourceName().GetCString());
+UnwindLogMsgVerbose(
+"frame uses %s for full UnwindPlan because we are at "
+"the first instruction of a function",
+unwind_plan_sp->GetSourceName().c_str());
 return unwind_plan_sp;
   }
 }
@@ -1024,7 +1024,7 @@
   if (IsUnwindPlanValidForCurrentPC(unwind_plan_sp)) {
 UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because this "
 "is the call-site unwind plan",
-unwind_plan_sp->GetSourceName().GetCString());
+unwind_plan_sp->GetSourceName().c_str());
 return unwind_plan_sp;
   }
 
@@ -1061,9 +1061,10 @@
   }
 
   if (IsUnwindPlanValidForCurrentPC(unwind_plan_sp)) {
-UnwindLogMsgVerbose("frame uses %s for full UnwindPlan because we "
-"failed to find a call-site unwind plan that would work",
-unwind_plan_sp->GetSourceName().GetCString());
+UnwindLogMsgVerbose(
+"frame uses %s for full UnwindPlan because we "
+"failed to find a call-site unwind plan that would work",
+