[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2674
+  // bit-field if this is the first field since the gap can be
+  // attributed to the memebers from the base class. This 
assumption
+  // is not correct if the first field of the derived class is

memembers


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

https://reviews.llvm.org/D76808



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

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/test/API/lang/cpp/bitfields/main.cpp:90
+  derived.b_a=2;
+  derived.d_a=1;
 

clang-format


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

https://reviews.llvm.org/D76808



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

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp:333
+
+  // When we generate debug info we add template parameters to names.
+  // Since we don't add a space between the name and the template parameter in

"we" is super ambiguous. This is LLDB, so you may want to say `Clang` and/or 
`GCC` does this.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp:339
+  //
+  // In some cases we with then parse incorrectly. This fixes the issue by
+  // detecting this case and inserting tok::less in place of tok::lessless

`we with then`



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp:343
+  if (token.getKind() == tok::lessless) {
+// Make sure we have more tokens before attempting to look ahead one more
+if (m_next_token_index + 1 < m_tokens.size()) {

`.`



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp:345
+if (m_next_token_index + 1 < m_tokens.size()) {
+  // Look ahead two tokens
+  clang::Token n_token = m_tokens[m_next_token_index + 1];

`.`



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp:347
+  clang::Token n_token = m_tokens[m_next_token_index + 1];
+  // If we find ( or < then this is indeed operator<< no need for fix
+  if (n_token.getKind() != tok::l_paren && n_token.getKind() != tok::less) 
{

`.`



Comment at: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp:147
+   "`anonymous namespace'::S::<<::__l2", "Foo"},
+  // These cases are idiosyncratic to how we generate debug info for names
+  // when we have template parameters. They are not valid C++ names but if

... `how Clang generates debug info`?


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

https://reviews.llvm.org/D76168



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

[Lldb-commits] [PATCH] D76891: [lldb-vscode] fix breakpoint result ordering

[Greg Clayton via Phabricator via lldb-commits]

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

LGTM as long as all test are passing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76891



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

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay updated this revision to Diff 253013.
emrekultursay added a comment.

- Apply code review comments from labath@
- Wipe source cache when user sets the property to false
- Also expose set/get methods for use-source-cache in SBDebugger


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804

Files:
  lldb/include/lldb/API/SBDebugger.h
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/SourceManager.cpp

Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -72,7 +72,7 @@
   FileSP file_sp;
   if (same_as_previous)
 file_sp = m_last_file_sp;
-  else if (debugger_sp)
+  else if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +95,7 @@
 else
   file_sp = std::make_shared(file_spec, debugger_sp);
 
-if (debugger_sp)
+if (debugger_sp && debugger_sp->GetUseSourceCache())
   debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
   }
   return file_sp;
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -212,6 +212,11 @@
   // use-color changed. Ping the prompt so it can reset the ansi terminal
   // codes.
   SetPrompt(GetPrompt());
+} else if (property_path == g_debugger_properties[ePropertyUseSourceCache].name) {
+  // use-source-cache changed. Wipe out the cache contents if it was disabled.
+  if (!GetUseSourceCache()) {
+m_source_file_cache.Clear();
+  }
 } else if (is_load_script && target_sp &&
load_script_old_value == eLoadScriptFromSymFileWarn) {
   if (target_sp->TargetProperties::GetLoadScriptFromSymbolFile() ==
@@ -338,6 +343,20 @@
   return ret;
 }
 
+bool Debugger::GetUseSourceCache() const {
+  const uint32_t idx = ePropertyUseSourceCache;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
+bool Debugger::SetUseSourceCache(bool b) {
+  const uint32_t idx = ePropertyUseSourceCache;
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  if (!ret) {
+m_source_file_cache.Clear();
+  }
+  return ret;
+}
 bool Debugger::GetHighlightSource() const {
   const uint32_t idx = ePropertyHighlightSource;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -103,6 +103,10 @@
 Global,
 DefaultTrue,
 Desc<"Whether to use Ansi color codes or not.">;
+  def UseSourceCache: Property<"use-source-cache", "Boolean">,
+Global,
+DefaultTrue,
+Desc<"Whether to cache source files in memory or not.">;
   def AutoOneLineSummaries: Property<"auto-one-line-summaries", "Boolean">,
 Global,
 DefaultTrue,
Index: lldb/source/API/SBDebugger.cpp
===
--- lldb/source/API/SBDebugger.cpp
+++ lldb/source/API/SBDebugger.cpp
@@ -1374,6 +1374,18 @@
   return (m_opaque_sp ? m_opaque_sp->GetUseColor() : false);
 }
 
+bool SBDebugger::SetUseSourceCache(bool value) {
+  LLDB_RECORD_METHOD(bool, SBDebugger, SetUseSourceCache, (bool), value);
+
+  return (m_opaque_sp ? m_opaque_sp->SetUseSourceCache(value) : false);
+}
+
+bool SBDebugger::GetUseSourceCache() const {
+  LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBDebugger, GetUseSourceCache);
+
+  return (m_opaque_sp ? m_opaque_sp->GetUseSourceCache() : false);
+}
+
 bool SBDebugger::GetDescription(SBStream &description) {
   LLDB_RECORD_METHOD(bool, SBDebugger, GetDescription, (lldb::SBStream &),
  description);
Index: lldb/include/lldb/Core/SourceManager.h
===
--- lldb/include/lldb/Core/SourceManager.h
+++ lldb/include/lldb/Core/SourceManager.h
@@ -101,6 +101,9 @@
 void AddSourceFile(const FileSP &file_sp);
 FileSP FindSourceFile(const FileSpec &file_spec) const;
 
+// Removes all elements from the cache.
+void Clear() { m_file_cache.clear(); }
+
   protected:
 typedef std::map FileCache;
 FileCache m_file_cache;
Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -273,6 +273,10 @@
 
   bool SetUseColor(bool use_color);
 
+  bool GetUseSourceCache() const;
+

[Lldb-commits] [PATCH] D76891: [lldb-vscode] fix breakpoint result ordering

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: clayborg, aadsm, kusmour, labath.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The DAP specifies the following for the SetBreakpoints request:

  The breakpoints returned are in the same order as the elements of the 
'breakpoints' arguments

This was not followed, as lldb-vscode was returning the breakpoints in a 
different order, because they were first stored into a map, and then traversed. 
Of course, maps normally don't preserve ordering.

See this log I captured:

  -->
  {"command":"setBreakpoints",
   "arguments":{
 "source":{
   "name":"main.cpp",
   
"path":"/Users/wallace/fbsource/xplat/sand/test-projects/buck-cpp/main.cpp"
 },
 "lines":[6,10,11],
 "breakpoints":[{"line":6},{"line":10},{"line":11}],
 "sourceModified":false
   },
   "type":"request",
   "seq":3 
  }
  
  <-- 
  {"body":{
 "breakpoints":[
   {"id":1, 
"line":11,"source":{"name":"main.cpp","path":"xplat/sand/test-projects/buck-cpp/main.cpp"},"verified":true},
   
{"id":2,"line":6,"source":{"name":"main.cpp","path":"xplat/sand/test-projects/buck-cpp/main.cpp"},"verified":true},
   
{"id":3,"line":10,"source":{"name":"main.cpp","path":"xplat/sand/test-projects/buck-cpp/main.cpp"},"verified":true}]},
 "command":"setBreakpoints",
 "request_seq":3,
 "seq":0,
 "success":true,
 "type":"response"
  }

As you can see, the order was not respected. This was causing the IDE not to be 
able to disable/enable breakpoints by clicking on them in the breakpoint view 
in the lower corner of the Debug tab.

This diff fixes the ordering problem. The traversal + querying was done very 
fast in O(nlogn) time. I'm keeping the same complexity.

I also updated a couple of tests to account for the ordering.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76891

Files:
  lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.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
@@ -1733,70 +1733,45 @@
   const auto path = GetString(source, "path");
   auto breakpoints = arguments->getArray("breakpoints");
   llvm::json::Array response_breakpoints;
+
   // Decode the source breakpoint infos for this "setBreakpoints" request
   SourceBreakpointMap request_bps;
   for (const auto &bp : *breakpoints) {
 auto bp_obj = bp.getAsObject();
 if (bp_obj) {
   SourceBreakpoint src_bp(*bp_obj);
-  request_bps[src_bp.line] = std::move(src_bp);
+  request_bps[src_bp.line] = src_bp;
+
+  // We check if this breakpoint already exists to update it
+  auto existing_source_bps = g_vsc.source_breakpoints.find(path);
+  if (existing_source_bps != g_vsc.source_breakpoints.end()) {
+const auto &existing_bp = existing_source_bps->second.find(src_bp.line);
+if (existing_bp != existing_source_bps->second.end()) {
+  existing_bp->second.UpdateBreakpoint(src_bp);
+  AppendBreakpoint(existing_bp->second.bp, response_breakpoints);
+  continue;
+}
+  }
+  // At this point the breakpoint is new
+  src_bp.SetBreakpoint(path.data());
+  AppendBreakpoint(src_bp.bp, response_breakpoints);
+  g_vsc.source_breakpoints[path][src_bp.line] = std::move(src_bp);
 }
   }
 
-  // See if we already have breakpoints set for this source file from a
-  // previous "setBreakpoints" request
+  // Delete any breakpoints in this source file that aren't in the
+  // request_bps set. There is no call to remove breakpoints other than
+  // calling this function with a smaller or empty "breakpoints" list.
   auto old_src_bp_pos = g_vsc.source_breakpoints.find(path);
   if (old_src_bp_pos != g_vsc.source_breakpoints.end()) {
-
-// We have already set breakpoints in this source file and they are giving
-// use a new list of lines to set breakpoints on. Some breakpoints might
-// already be set, and some might not. We need to remove any breakpoints
-// whose lines are not contained in the any breakpoints lines in in the
-// "breakpoints" array.
-
-// Delete any breakpoints in this source file that aren't in the
-// request_bps set. There is no call to remove breakpoints other than
-// calling this function with a smaller or empty "breakpoints" list.
-std::vector remove_lines;
-for (auto &pair: old_src_bp_pos->second) {
-  auto request_pos = request_bps.find(pair.first);
+for (auto &old_bp : old_src_bp_pos->second) {
+  auto request_pos = request_bps.find(old_bp.first);
   if (request_pos == request_bps.end()) {
 // This breakpoint no longer exists in this source file, delete it
-g_vsc.target.BreakpointDelete(pair.second.bp.GetID());
-remove_lines.push_back(pair.first);
-

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 252984.
shafik marked 4 inline comments as done.
shafik added a comment.

- Expanded comments
- Adjusted conditionals


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

https://reviews.llvm.org/D76808

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
  lldb/test/API/lang/cpp/bitfields/main.cpp


Index: lldb/test/API/lang/cpp/bitfields/main.cpp
===
--- lldb/test/API/lang/cpp/bitfields/main.cpp
+++ lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -60,6 +60,16 @@
 } 
   } clang_example;
 
+  class B {
+  public:
+uint32_t b_a;
+  };
+
+  class D : public B {
+  public:
+uint32_t d_a:1;
+  } derived;
+
   lba.a = 2;
 
   lbb.a = 1;
@@ -76,6 +86,8 @@
   lbd.arr[2] = '\0';
   lbd.a = 5;
 
+  derived.b_a=2;
+  derived.d_a=1;
 
   return 0; // Set break point at this line.
 }
Index: lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
===
--- lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -103,3 +103,10 @@
'(uint64_t:1) k = 1',
 ])
 
+self.expect(
+"frame variable --show-types derived",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+'(uint32_t) b_a = 2',
+'(uint32_t:1) d_a = 1',
+])
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2668,9 +2668,19 @@
   }
 
   // If we have a gap between the last_field_end and the current
-  // field we have an unnamed bit-field
+  // field we have an unnamed bit-field.
+  // If we have a base class, we assume there is no unnamed
+  // bit-field if this is the first field since the gap can be
+  // attributed to the memebers from the base class. This 
assumption
+  // is not correct if the first field of the derived class is
+  // indeed an unnamed bit-field. We currently do not have the
+  // machinary to track the offset of the last field of classes we
+  // have seen before, so we are not handling this case.
   if (this_field_info.bit_offset != last_field_end &&
-  !(this_field_info.bit_offset < last_field_end)) {
+  this_field_info.bit_offset > last_field_end &&
+  !(last_field_info.bit_offset == 0 &&
+last_field_info.bit_size == 0 &&
+layout_info.base_offsets.size() != 0)) {
 unnamed_field_info = FieldInfo{};
 unnamed_field_info->bit_size =
 this_field_info.bit_offset - last_field_end;


Index: lldb/test/API/lang/cpp/bitfields/main.cpp
===
--- lldb/test/API/lang/cpp/bitfields/main.cpp
+++ lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -60,6 +60,16 @@
 } 
   } clang_example;
 
+  class B {
+  public:
+uint32_t b_a;
+  };
+
+  class D : public B {
+  public:
+uint32_t d_a:1;
+  } derived;
+
   lba.a = 2;
 
   lbb.a = 1;
@@ -76,6 +86,8 @@
   lbd.arr[2] = '\0';
   lbd.a = 5;
 
+  derived.b_a=2;
+  derived.d_a=1;
 
   return 0; // Set break point at this line.
 }
Index: lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
===
--- lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -103,3 +103,10 @@
'(uint64_t:1) k = 1',
 ])
 
+self.expect(
+"frame variable --show-types derived",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+'(uint32_t) b_a = 2',
+'(uint32_t:1) d_a = 1',
+])
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2668,9 +2668,19 @@
   }
 
   // If we have a gap between the last_field_end and the current
-  // field we have an unnamed bit-field
+  // field we have an unnamed bit-field.
+  // If we have a base class, we assume there is no unnamed
+  // bit-field if this is the first field since the gap can be
+  // attributed to the memebers from the base class. This assumption
+  // is not correct if 

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2671
   // If we have a gap between the last_field_end and the current
   // field we have an unnamed bit-field
   if (this_field_info.bit_offset != last_field_end &&

aprantl wrote:
> do we need to amend the comment to describe the new conditions?
Good catch, I forgot about that.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2674
+  !(this_field_info.bit_offset < last_field_end) &&
+  !(last_field_info.bit_offset == 0 &&
+last_field_info.bit_size == 0 &&

aprantl wrote:
> ```
> (last_field_info.bit_offset > 0) ||
> (last_field_info.bit_size > 0) ||
> !layout_info.base_offsets.size())
> ```
> 
> Does this get more readable if you sink the ! into the subexpressions?
I see what your doing but it feels too clever, I would never get what it means 
coming back to it a couple of months from now.


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

https://reviews.llvm.org/D76808



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

[Lldb-commits] [PATCH] D76814: Preserve ThreadPlanStacks for unreported threads

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Addressed most of the review comments.  I don't see that using file check would 
make the plan output checking any easier.  The current function seems clear and 
easy to use.  Trying to dynamically insert the passed in matches into a 
filecheck test file and then run file check on that doesn't seem like it would 
be any easier than what is here.  I don't need any of the substitutions or any 
of the other fancy things FileCheck provides, that just seems like overkill.  
But maybe I'm misunderstanding what you meant.




Comment at: lldb/source/Target/TargetProperties.td:183
 Desc<"A path to a python OS plug-in module file that contains a 
OperatingSystemPlugIn class.">;
+  def PluginReportsAllThreads: Property<"plugin-reports-all-threads", 
"Boolean">,
+Global,

labath wrote:
> If this is relevant only for os plugins, then it would be good to reflect 
> that in the name as well.
I thought about that.  In the future a regular Process plugin might decide it 
was too expensive to report all threads as well.  There's nothing in this patch 
that wouldn't "just work" with that case as well.  Leaving the OS out was meant 
to indicate this was about how the Process plugin OR any of its helpers (e.g. 
the OS Plugin) produces threads.



Comment at: 
lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py:23
+
+def check_list_output(self, command, active_plans = [], completed_plans = 
[], discarded_plans = []):
+# Check the "thread plan list" output against a list of active & 
completed and discarded plans.

labath wrote:
> this looks like a model example for using `self.filecheck`
I don't see that.  Do you mean write a file check matching file with the 
contents of the three arrays in this function, and then run file check on that? 
 Or did you mean convert the call sites into embedded patterns and then call 
file check on myself?  But then I'd have to also embed the headers in the body 
such that they came in the right order for all the tests.  Neither of these 
seem like attractive options to me.  It doesn't seem like it will be hard to 
maintain this little checker, and I don't see what I would gain by using file 
check instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76814



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

[Lldb-commits] [PATCH] D76814: Preserve ThreadPlanStacks for unreported threads

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 252982.
jingham marked 2 inline comments as done.
jingham added a comment.

Addressed most of Pavel's review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76814

Files:
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStack.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Target/Process.cpp
  lldb/source/Target/TargetProperties.td
  lldb/source/Target/Thread.cpp
  lldb/source/Target/ThreadList.cpp
  lldb/source/Target/ThreadPlan.cpp
  lldb/source/Target/ThreadPlanStack.cpp
  lldb/source/Target/ThreadPlanStepOut.cpp
  
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/Makefile
  
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/TestOSPluginStepping.py
  
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp
  
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/operating_system.py
  lldb/test/API/functionalities/thread_plan/Makefile
  lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
  lldb/test/API/functionalities/thread_plan/main.c

Index: lldb/test/API/functionalities/thread_plan/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/thread_plan/main.c
@@ -0,0 +1,16 @@
+#include 
+
+void
+call_me(int value) {
+  printf("called with %d\n", value); // Set another here.
+}
+
+int
+main(int argc, char **argv)
+{
+  call_me(argc); // Set a breakpoint here.
+  printf("This just spaces the two calls\n");
+  call_me(argc); // Run here to step over again.
+  printf("More spacing\n");
+  return 0; // Make sure we get here on last continue
+}
Index: lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
===
--- /dev/null
+++ lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
@@ -0,0 +1,160 @@
+"""
+Test that thread plan listing, and deleting works.
+"""
+
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestThreadPlanCommands(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_thread_plan_actions(self):
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.thread_plan_test()
+
+def check_list_output(self, command, active_plans = [], completed_plans = [], discarded_plans = []):
+# Check the "thread plan list" output against a list of active & completed and discarded plans.
+# If all three check arrays are empty, that means the command is expected to fail. 
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+
+num_active = len(active_plans)
+num_completed = len(completed_plans)
+num_discarded = len(discarded_plans)
+
+interp.HandleCommand(command, result)
+if num_active == 0 and num_completed == 0 and num_discarded == 0:
+self.assertFalse(result.Succeeded(), "command: '%s' succeeded when it should have failed: '%s'"%
+ (command, result.GetError()))
+return
+
+self.assertTrue(result.Succeeded(), "command: '%s' failed: '%s'"%(command, result.GetError()))
+result_arr = result.GetOutput().splitlines()
+num_results = len(result_arr)
+
+# Match the expected number of elements.
+# Adjust the count for the number of header lines we aren't matching:
+fudge = 0
+
+if num_completed == 0 and num_discarded == 0:
+# The fudge is 3: Thread header, Active Plan header and base plan
+fudge = 3
+elif num_completed == 0 or num_discarded == 0:
+# The fudge is 4: The above plus either the Completed or Discarded Plan header:
+fudge = 4
+else:
+# The fudge is 5 since we have both headers:
+fudge = 5
+
+self.assertEqual(num_results, num_active + num_completed + num_discarded + fudge,
+ "Too many elements in match arrays")
+
+# Now iterate through the results array and pick out the results.
+result_idx = 0
+self.assertTrue("thread #" in result_arr[result_idx], "Found thread header") ; result_idx += 1
+self.assertTrue("Active plan stack" in result_arr[result_idx], "Found active header") ; result_idx += 1
+self.assertTrue("Element 0: Base thread plan" in result_arr[result_idx], "Found base plan") ; result_idx += 1
+
+for text in active_plans:
+self.assertFalse("Completed plan stack" in result_arr[re

[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D76758#1944612 , @clayborg wrote:

> So I know the mach-o symbol table parsing code is a mess already, but it 
> seems like this patch can be simpler if we make a std::set at 
> the top of ObjectFileMachO::ParseSymtab() and every time we add a symbol that 
> has a valid address value, add the file addr to this set. This will avoid the 
> need to do any sorting. This std::set can be used to not add 
> LC_FUNCTION_START entries that already have a symbol at the address, and it 
> can be used to only add TrieEntry values that have symbols. Thoughts?


That's a solid idea, let me try rewriting for that.  LC_FUNCTION_STARTS already 
has a 'done' bool with each one to indicate whether it should be added to the 
symtab at the end.  & yeah, I should have just added a new flag to indicate 
that the thing had already been seen -- for the exported symbols from the trie 
I wasn't use flags at all so I just clobbered it, but that's not a very nice 
way to do it.  Let me try doing the std::set thing I think that'll remove a 
bunch of code across the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76758



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

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 252941.
shafik marked 4 inline comments as done.
shafik added a comment.

Addressing comments:

- Adding more detailed comments
- Adding test for cases that currently fail b/c we don't enable C++20


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

https://reviews.llvm.org/D76168

Files:
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp
  lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp


Index: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -140,12 +140,20 @@
"std::vector>",
"_M_emplace_back_aux"},
   {"`anonymous namespace'::foo", "`anonymous namespace'", "foo"},
-  {"`operator<'::`2'::B<0>::operator>",
-   "`operator<'::`2'::B<0>",
+  {"`operator<'::`2'::B<0>::operator>", "`operator<'::`2'::B<0>",
"operator>"},
   {"`anonymous namespace'::S::<<::__l2::Foo",
-   "`anonymous namespace'::S::<<::__l2",
-   "Foo"}};
+   "`anonymous namespace'::S::<<::__l2", "Foo"},
+  // These cases are idiosyncratic to how we generate debug info for names
+  // when we have template parameters. They are not valid C++ names but if
+  // we fix this we need to support them for older compilers.
+  {"A::operator>", "A", "operator>"},
+  {"operator>", "", "operator>"},
+  {"A::operator<", "A", "operator<"},
+  {"operator<", "", "operator<"},
+  {"A::operator<<", "A", "operator<<"},
+  {"operator<<", "", "operator<<"},
+  };
 
   llvm::StringRef context, basename;
   for (const auto &test : test_cases) {
@@ -169,6 +177,12 @@
   "abc::", context, basename));
   EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
   "f>", context, basename));
+
+  // We expect these cases to fail until we turn on C++2a
+  EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
+  "A::operator<=>", context, basename));
+  EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
+  "operator<=>", context, basename));
 }
 
 static std::set FindAlternate(llvm::StringRef Name) {
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp
@@ -329,6 +329,37 @@
   }
 
   const auto &token = Peek();
+
+  // When we generate debug info we add template parameters to names.
+  // Since we don't add a space between the name and the template parameter in
+  // some cases we are not generating valid C++ names e.g.:
+  //
+  //   operator<
+  //
+  // In some cases we with then parse incorrectly. This fixes the issue by
+  // detecting this case and inserting tok::less in place of tok::lessless
+  // and returning successfully that we consumed the operator.
+  if (token.getKind() == tok::lessless) {
+// Make sure we have more tokens before attempting to look ahead one more
+if (m_next_token_index + 1 < m_tokens.size()) {
+  // Look ahead two tokens
+  clang::Token n_token = m_tokens[m_next_token_index + 1];
+  // If we find ( or < then this is indeed operator<< no need for fix
+  if (n_token.getKind() != tok::l_paren && n_token.getKind() != tok::less) 
{
+clang::Token tmp_tok;
+
+tmp_tok.setLength(1);
+tmp_tok.setLocation(token.getLocation().getLocWithOffset(1));
+tmp_tok.setKind(tok::less);
+
+m_tokens[m_next_token_index] = tmp_tok;
+
+start_position.Remove();
+return true;
+  }
+}
+  }
+
   switch (token.getKind()) {
   case tok::kw_new:
   case tok::kw_delete:


Index: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -140,12 +140,20 @@
"std::vector>",
"_M_emplace_back_aux"},
   {"`anonymous namespace'::foo", "`anonymous namespace'", "foo"},
-  {"`operator<'::`2'::B<0>::operator>",
-   "`operator<'::`2'::B<0>",
+  {"`operator<'::`2'::B<0>::operator>", "`operator<'::`2'::B<0>",
"operator>"},
   {"`anonymous namespace'::S::<<::__l2::Foo",
-   "`anonymous namespace'::S::<<::__l2",
-   "Foo"}};
+   "`anonymous namespace'::S::<<::__l2", "Foo"},
+  // These cases are idiosyncratic to how we generate debug info for names
+  // when we have template parameters. They are not valid C++ names but if
+  // we fix this we need to support them for older compilers.
+  {"A::operator>", "A", "operator>"},
+  {"operator>", "", "operator>"},
+  {"A::operator<", "A", "operator<"},
+  {"ope

[Lldb-commits] [PATCH] D76872: [intel-pt] Fix existing support in LLDB

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

His last activity was in Jun 2017. I hope he's still around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76872



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

[Lldb-commits] [PATCH] D76872: [intel-pt] Fix existing support in LLDB

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 252936.
wallace added a comment.

update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76872

Files:
  lldb/tools/intel-features/CMakeLists.txt
  lldb/tools/intel-features/intel-pt/Decoder.cpp
  lldb/tools/intel-features/intel-pt/Decoder.h

Index: lldb/tools/intel-features/intel-pt/Decoder.h
===
--- lldb/tools/intel-features/intel-pt/Decoder.h
+++ lldb/tools/intel-features/intel-pt/Decoder.h
@@ -245,6 +245,22 @@
   lldb::SBError &sberror) const;
   void DecodeTrace(struct pt_insn_decoder *decoder,
Instructions &instruction_list, lldb::SBError &sberror);
+  int HandlePTInstructionEvents(pt_insn_decoder *decoder, int errcode,
+Instructions &instruction_list,
+lldb::SBError &sberror);
+
+  int AppendErrorToInstructionList(int errcode, pt_insn_decoder *decoder,
+   Instructions &instruction_list,
+   lldb::SBError &sberror);
+
+  void AppendErrorWithOffsetToInstructionList(int errcode,
+  uint64_t decoder_offset,
+  Instructions &instruction_list,
+  lldb::SBError &sberror);
+
+  void AppendErrorWithoutOffsetToInstructionList(int errcode,
+ Instructions &instruction_list,
+ lldb::SBError &sberror);
 
   // Function to diagnose and indicate errors during raw trace decoding
   void Diagnose(struct pt_insn_decoder *decoder, int errcode,
Index: lldb/tools/intel-features/intel-pt/Decoder.cpp
===
--- lldb/tools/intel-features/intel-pt/Decoder.cpp
+++ lldb/tools/intel-features/intel-pt/Decoder.cpp
@@ -564,6 +564,61 @@
   }
 }
 
+void Decoder::AppendErrorWithOffsetToInstructionList(
+int errcode, uint64_t decoder_offset, Instructions &instruction_list,
+lldb::SBError &sberror) {
+  sberror.SetErrorStringWithFormat(
+  "processor trace decoding library: \"%s\"  [decoder_offset] => "
+  "[0x%" PRIu64 "]",
+  pt_errstr(pt_errcode(errcode)), decoder_offset);
+  instruction_list.emplace_back(sberror.GetCString());
+}
+
+void Decoder::AppendErrorWithoutOffsetToInstructionList(
+int errcode, Instructions &instruction_list, lldb::SBError &sberror) {
+  sberror.SetErrorStringWithFormat("processor trace decoding library: \"%s\"",
+   pt_errstr(pt_errcode(errcode)));
+  instruction_list.emplace_back(sberror.GetCString());
+}
+
+int Decoder::AppendErrorToInstructionList(int errcode, pt_insn_decoder *decoder,
+  Instructions &instruction_list,
+  lldb::SBError &sberror) {
+  uint64_t decoder_offset = 0;
+  int errcode_off = pt_insn_get_offset(decoder, &decoder_offset);
+  if (errcode_off < 0) {
+AppendErrorWithoutOffsetToInstructionList(errcode, instruction_list,
+  sberror);
+return errcode_off;
+  }
+  AppendErrorWithOffsetToInstructionList(errcode, decoder_offset,
+ instruction_list, sberror);
+  return 0;
+}
+
+int Decoder::HandlePTInstructionEvents(pt_insn_decoder *decoder, int errcode,
+   Instructions &instruction_list,
+   lldb::SBError &sberror) {
+  while (errcode & pts_event_pending) {
+pt_event event;
+errcode = pt_insn_event(decoder, &event, sizeof(event));
+if (errcode < 0)
+  return errcode;
+
+// The list of events are in
+// https://github.com/intel/libipt/blob/master/doc/man/pt_qry_event.3.md
+if (event.type == ptev_overflow) {
+  int append_errcode = AppendErrorToInstructionList(
+  errcode, decoder, instruction_list, sberror);
+  if (append_errcode < 0)
+return append_errcode;
+}
+// Other events don't signal stream errors
+  }
+
+  return 0;
+}
+
 // Start actual decoding of raw trace
 void Decoder::DecodeTrace(struct pt_insn_decoder *decoder,
   Instructions &instruction_list,
@@ -585,10 +640,8 @@
 
   int errcode_off = pt_insn_get_offset(decoder, &decoder_offset);
   if (errcode_off < 0) {
-sberror.SetErrorStringWithFormat(
-"processor trace decoding library: \"%s\"",
-pt_errstr(pt_errcode(errcode)));
-instruction_list.emplace_back(sberror.GetCString());
+AppendErrorWithoutOffsetToInstructionList(errcode, instruction_list,
+  sberror);
 return;
   }
 
@@ -619,16 +672,22 @@
 

[Lldb-commits] [PATCH] D76872: [intel-pt] Fix existing support in LLDB

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks fine to me, but the original author should probably approve this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76872



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

[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So I know the mach-o symbol table parsing code is a mess already, but it seems 
like this patch can be simpler if we make a std::set at the top of 
ObjectFileMachO::ParseSymtab() and every time we add a symbol that has a valid 
address value, add the file addr to this set. This will avoid the need to do 
any sorting. This std::set can be used to not add LC_FUNCTION_START entries 
that already have a symbol at the address, and it can be used to only add 
TrieEntry values that have symbols. Thoughts?




Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1924
+  // symbol table, and this entry should not be added.
+  void SymbolAlreadyPresent() { flags = LLDB_INVALID_ADDRESS; }
+  bool IsSymbolAlreadyPresent() {

 The only bits that are used in this field are:

```
enum {
  EXPORT_SYMBOL_FLAGS_KIND_MASK = 0x03u,
  EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION = 0x04u,
  EXPORT_SYMBOL_FLAGS_REEXPORT = 0x08u,
  EXPORT_SYMBOL_FLAGS_STUB_AND_RESOLVER = 0x10u
};

enum ExportSymbolKind {
  EXPORT_SYMBOL_FLAGS_KIND_REGULAR = 0x00u,
  EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL = 0x01u,
  EXPORT_SYMBOL_FLAGS_KIND_ABSOLUTE = 0x02u
};
```

So why not just set the highest bit and avoid clobbering all of the other 
flags?:

```
#define EXPORT_SYMBOL_FLAGS_LLDB_ALREADY_PRESENT 1ull << 63
void SymbolAlreadyPresent() { flags |= 
EXPORT_SYMBOL_FLAGS_LLDB_ALREADY_PRESENT; }
```



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1926
+  bool IsSymbolAlreadyPresent() {
+if (flags == LLDB_INVALID_ADDRESS)
+  return true;

jingham wrote:
> JDevlieghere wrote:
> > `return flags == LLDB_INVALID_ADDRESS`
> This sort of change has the downside that you can't break when flags == 
> LLDB_INVALID_ADDRESS without adding a condition.  That seems in this case 
> like a condition you are likely to want to break on, and given this looks 
> like a function that gets called a lot, it's probably good to pay the cost of 
> a condition.
> 
> I'm not sure I'm in favor of this sort of rewrite.  It just saves one line, 
> and isn't particularly easier to read.  Does it have some benefit I'm 
> missing?  
So is "flags" initially used just after parsing, but before we mark TrieEntry 
values as already present? These flags mean something when we first parse them. 
I would rather add an extra bool entry to this structure, since they don't stay 
around for long.



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1926
+  bool IsSymbolAlreadyPresent() {
+if (flags == LLDB_INVALID_ADDRESS)
+  return true;

clayborg wrote:
> jingham wrote:
> > JDevlieghere wrote:
> > > `return flags == LLDB_INVALID_ADDRESS`
> > This sort of change has the downside that you can't break when flags == 
> > LLDB_INVALID_ADDRESS without adding a condition.  That seems in this case 
> > like a condition you are likely to want to break on, and given this looks 
> > like a function that gets called a lot, it's probably good to pay the cost 
> > of a condition.
> > 
> > I'm not sure I'm in favor of this sort of rewrite.  It just saves one line, 
> > and isn't particularly easier to read.  Does it have some benefit I'm 
> > missing?  
> So is "flags" initially used just after parsing, but before we mark TrieEntry 
> values as already present? These flags mean something when we first parse 
> them. I would rather add an extra bool entry to this structure, since they 
> don't stay around for long.
```
return (flags & EXPORT_SYMBOL_FLAGS_LLDB_ALREADY_PRESENT) != 0;
```



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1965
+  // Terminal node -- end of a branch, possibly add this to
+  // the symbol table or resoler table.
   const uint64_t terminalSize = data.GetULEB128(&offset);

s/resoler/resolver/



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:2503-2516
 ConstString text_segment_name("__TEXT");
 SectionSP text_segment_sp =
 GetSectionList()->FindSectionByName(text_segment_name);
 if (text_segment_sp) {
   const lldb::addr_t text_segment_file_addr =
   text_segment_sp->GetFileAddress();
   if (text_segment_file_addr != LLDB_INVALID_ADDRESS) {

Maybe we should run this before ParseTrieEntries and pass 
text_segment_file_addr in as an argument and add this to the offset as we parse 
the TrieEntry objects?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4490
 
+  // Look through the current symbol table, if any symbols match
+  // an entry in the external_sym_trie_entries vector, mark it

Might be easier to create a std::set at the top of the 
ObjectFileMachO::ParseSymtab() function. Anyone who adds a symbol, would add 
the file address to that set. Then we wouldn't have to sort these entries, we 
woul

[Lldb-commits] [PATCH] D76872: [intel-pt] fix python building

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
Herald added subscribers: lldb-commits, mgorny.
Herald added a project: LLDB.

fix PT decoding


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76872

Files:
  lldb/tools/intel-features/CMakeLists.txt
  lldb/tools/intel-features/intel-pt/Decoder.cpp
  lldb/tools/intel-features/intel-pt/Decoder.h

Index: lldb/tools/intel-features/intel-pt/Decoder.h
===
--- lldb/tools/intel-features/intel-pt/Decoder.h
+++ lldb/tools/intel-features/intel-pt/Decoder.h
@@ -245,6 +245,22 @@
   lldb::SBError &sberror) const;
   void DecodeTrace(struct pt_insn_decoder *decoder,
Instructions &instruction_list, lldb::SBError &sberror);
+  int HandlePTInstructionEvents(pt_insn_decoder *decoder, int errcode,
+Instructions &instruction_list,
+lldb::SBError &sberror);
+
+  int AppendErrorToInstructionList(int errcode, pt_insn_decoder *decoder,
+   Instructions &instruction_list,
+   lldb::SBError &sberror);
+
+  void AppendErrorWithOffsetToInstructionList(int errcode,
+  uint64_t decoder_offset,
+  Instructions &instruction_list,
+  lldb::SBError &sberror);
+
+  void AppendErrorWithoutOffsetToInstructionList(int errcode,
+ Instructions &instruction_list,
+ lldb::SBError &sberror);
 
   // Function to diagnose and indicate errors during raw trace decoding
   void Diagnose(struct pt_insn_decoder *decoder, int errcode,
Index: lldb/tools/intel-features/intel-pt/Decoder.cpp
===
--- lldb/tools/intel-features/intel-pt/Decoder.cpp
+++ lldb/tools/intel-features/intel-pt/Decoder.cpp
@@ -564,6 +564,63 @@
   }
 }
 
+void Decoder::AppendErrorWithOffsetToInstructionList(
+int errcode, uint64_t decoder_offset, Instructions &instruction_list,
+lldb::SBError &sberror) {
+  sberror.SetErrorStringWithFormat(
+  "processor trace decoding library: \"%s\"  [decoder_offset] => "
+  "[0x%" PRIu64 "]",
+  pt_errstr(pt_errcode(errcode)), decoder_offset);
+  instruction_list.emplace_back(sberror.GetCString());
+}
+
+void Decoder::AppendErrorWithoutOffsetToInstructionList(
+int errcode, Instructions &instruction_list, lldb::SBError &sberror) {
+  sberror.SetErrorStringWithFormat("processor trace decoding library: \"%s\"",
+   pt_errstr(pt_errcode(errcode)));
+  instruction_list.emplace_back(sberror.GetCString());
+}
+
+int Decoder::AppendErrorToInstructionList(int errcode, pt_insn_decoder *decoder,
+  Instructions &instruction_list,
+  lldb::SBError &sberror) {
+  uint64_t decoder_offset = 0;
+  int errcode_off;
+
+  errcode_off = pt_insn_get_offset(decoder, &decoder_offset);
+  if (errcode_off < 0) {
+AppendErrorWithoutOffsetToInstructionList(errcode, instruction_list,
+  sberror);
+return errcode_off;
+  }
+  AppendErrorWithOffsetToInstructionList(errcode, decoder_offset,
+ instruction_list, sberror);
+  return 0;
+}
+
+int Decoder::HandlePTInstructionEvents(pt_insn_decoder *decoder, int errcode,
+   Instructions &instruction_list,
+   lldb::SBError &sberror) {
+  while (errcode & pts_event_pending) {
+pt_event event;
+errcode = pt_insn_event(decoder, &event, sizeof(event));
+if (errcode < 0)
+  return errcode;
+
+// The list of events are in
+// https://github.com/intel/libipt/blob/master/doc/man/pt_qry_event.3.md
+if (event.type == ptev_overflow) {
+  int append_errcode = AppendErrorToInstructionList(
+  errcode, decoder, instruction_list, sberror);
+  if (append_errcode < 0)
+return append_errcode;
+}
+// Other events don't signal stream errors
+  }
+
+  return 0;
+}
+
 // Start actual decoding of raw trace
 void Decoder::DecodeTrace(struct pt_insn_decoder *decoder,
   Instructions &instruction_list,
@@ -585,10 +642,8 @@
 
   int errcode_off = pt_insn_get_offset(decoder, &decoder_offset);
   if (errcode_off < 0) {
-sberror.SetErrorStringWithFormat(
-"processor trace decoding library: \"%s\"",
-pt_errstr(pt_errcode(errcode)));
-instruction_list.emplace_back(sberror.GetCString());
+AppendErrorWithoutOffsetToInstructionList(errcode, instruction_list,
+  sberror);
 return;
   }
 
@@ -619,16 +674,22 @@
   // progress fu

[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1926
+  bool IsSymbolAlreadyPresent() {
+if (flags == LLDB_INVALID_ADDRESS)
+  return true;

JDevlieghere wrote:
> `return flags == LLDB_INVALID_ADDRESS`
This sort of change has the downside that you can't break when flags == 
LLDB_INVALID_ADDRESS without adding a condition.  That seems in this case like 
a condition you are likely to want to break on, and given this looks like a 
function that gets called a lot, it's probably good to pay the cost of a 
condition.

I'm not sure I'm in favor of this sort of rewrite.  It just saves one line, and 
isn't particularly easier to read.  Does it have some benefit I'm missing?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76758



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 252898.
JDevlieghere added a comment.

Implement Pavel's suggestion


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

https://reviews.llvm.org/D76672

Files:
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  lldb/source/Utility/Reproducer.cpp
  lldb/test/Shell/Reproducer/TestDSYM.test

Index: lldb/test/Shell/Reproducer/TestDSYM.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestDSYM.test
@@ -0,0 +1,11 @@
+# REQUIRES: system-darwin
+# Ensure that the reproducers captures the whole dSYM bundle.
+
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: touch %t.out.dSYM/foo.bar
+
+# RUN: %lldb -x -b --capture --capture-path %t.repro %t.out -o 'b main' -o 'run' -o 'reproducer generate'
+
+# RUN: %lldb -b -o 'reproducer dump -p files -f %t.repro' | FileCheck %s --check-prefix FILES
+# FILES: foo.bar
Index: lldb/source/Utility/Reproducer.cpp
===
--- lldb/source/Utility/Reproducer.cpp
+++ lldb/source/Utility/Reproducer.cpp
@@ -321,6 +321,11 @@
   os << m_cwd << "\n";
 }
 
+void FileProvider::recordInterestingDirectory(const llvm::Twine &dir) {
+  if (m_collector)
+m_collector->addDirectory(dir);
+}
+
 void ProviderBase::anchor() {}
 char CommandProvider::ID = 0;
 char FileProvider::ID = 0;
Index: lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
===
--- lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -20,6 +20,7 @@
 #include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
@@ -145,6 +146,14 @@
 }
 
 if (dsym_fspec) {
+  // Compute dSYM root.
+  std::string dsym_root = dsym_fspec.GetPath();
+  const size_t pos = dsym_root.find("/Contents/Resources/");
+  if (pos == std::string::npos)
+dsym_root.clear();
+  else
+dsym_root = dsym_root.substr(0, pos);
+
   DataBufferSP dsym_file_data_sp;
   lldb::offset_t dsym_file_data_offset = 0;
   dsym_objfile_sp =
@@ -154,136 +163,131 @@
   if (UUIDsMatch(module_sp.get(), dsym_objfile_sp.get(), feedback_strm)) {
 // We need a XML parser if we hope to parse a plist...
 if (XMLDocument::XMLEnabled()) {
-  char dsym_path[PATH_MAX];
-  if (module_sp->GetSourceMappingList().IsEmpty() &&
-  dsym_fspec.GetPath(dsym_path, sizeof(dsym_path))) {
+  if (module_sp->GetSourceMappingList().IsEmpty()) {
 lldb_private::UUID dsym_uuid = dsym_objfile_sp->GetUUID();
 if (dsym_uuid) {
   std::string uuid_str = dsym_uuid.GetAsString();
-  if (!uuid_str.empty()) {
-char *resources = strstr(dsym_path, "/Contents/Resources/");
-if (resources) {
-  char dsym_uuid_plist_path[PATH_MAX];
-  resources[strlen("/Contents/Resources/")] = '\0';
-  snprintf(dsym_uuid_plist_path, sizeof(dsym_uuid_plist_path),
-   "%s%s.plist", dsym_path, uuid_str.c_str());
-  FileSpec dsym_uuid_plist_spec(dsym_uuid_plist_path);
-  if (FileSystem::Instance().Exists(dsym_uuid_plist_spec)) {
-ApplePropertyList plist(dsym_uuid_plist_path);
-if (plist) {
-  std::string DBGBuildSourcePath;
-  std::string DBGSourcePath;
-
-  // DBGSourcePathRemapping is a dictionary in the plist
-  // with keys which are DBGBuildSourcePath file paths and
-  // values which are DBGSourcePath file paths
-
-  StructuredData::ObjectSP plist_sp =
-  plist.GetStructuredData();
-  if (plist_sp.get() && plist_sp->GetAsDictionary() &&
-  plist_sp->GetAsDictionary()->HasKey(
-  "DBGSourcePathRemapping") &&
-  plist_sp->GetAsDictionary()
-  ->GetValueForKey("DBGSourcePathRemapping")
-  ->GetAsDictionary()) {
-
-// If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping.
-// If DBGVersion 2, strip last two components of path remappings from
-//  entries to fix an issue with a specific set of
-//  DBGSourcePathRemapping entries that lldb worked
-//  with.
-

[Lldb-commits] [PATCH] D76827: [lldb/CMake] Fix `install` for multi-configuration generators.

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG17e4c38739aa: [lldb/CMake] Fix `install` for 
multi-configuration generators. (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76827

Files:
  lldb/CMakeLists.txt


Index: lldb/CMakeLists.txt
===
--- lldb/CMakeLists.txt
+++ lldb/CMakeLists.txt
@@ -224,6 +224,8 @@
   else()
 set(LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_RELATIVE_PATH})
   endif()
+  string(REPLACE ${CMAKE_CFG_INTDIR} "\$\{CMAKE_INSTALL_CONFIG_NAME\}" 
LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_INSTALL_PATH})
+  string(REPLACE ${CMAKE_CFG_INTDIR} "\$\{CMAKE_INSTALL_CONFIG_NAME\}" 
lldb_python_build_path ${lldb_python_build_path})
   add_custom_target(lldb-python-scripts)
   add_dependencies(lldb-python-scripts finish_swig)
   install(DIRECTORY ${lldb_python_build_path}/../


Index: lldb/CMakeLists.txt
===
--- lldb/CMakeLists.txt
+++ lldb/CMakeLists.txt
@@ -224,6 +224,8 @@
   else()
 set(LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_RELATIVE_PATH})
   endif()
+  string(REPLACE ${CMAKE_CFG_INTDIR} "\$\{CMAKE_INSTALL_CONFIG_NAME\}" LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_INSTALL_PATH})
+  string(REPLACE ${CMAKE_CFG_INTDIR} "\$\{CMAKE_INSTALL_CONFIG_NAME\}" lldb_python_build_path ${lldb_python_build_path})
   add_custom_target(lldb-python-scripts)
   add_dependencies(lldb-python-scripts finish_swig)
   install(DIRECTORY ${lldb_python_build_path}/../
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay updated this revision to Diff 252888.
emrekultursay added a comment.

Applied suggestions from labath@


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76806

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

Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -52,27 +52,21 @@
 
 // SourceManager constructor
 SourceManager::SourceManager(const TargetSP &target_sp)
-: m_last_file_sp(), m_last_line(0), m_last_count(0), m_default_set(false),
+: m_last_line(0), m_last_count(0), m_default_set(false),
   m_target_wp(target_sp),
   m_debugger_wp(target_sp->GetDebugger().shared_from_this()) {}
 
 SourceManager::SourceManager(const DebuggerSP &debugger_sp)
-: m_last_file_sp(), m_last_line(0), m_last_count(0), m_default_set(false),
-  m_target_wp(), m_debugger_wp(debugger_sp) {}
+: m_last_line(0), m_last_count(0), m_default_set(false), m_target_wp(),
+  m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
-  bool same_as_previous =
-  m_last_file_sp &&
-  FileSpec::Match(file_spec, m_last_file_sp->GetFileSpec());
-
   DebuggerSP debugger_sp(m_debugger_wp.lock());
   FileSP file_sp;
-  if (same_as_previous)
-file_sp = m_last_file_sp;
-  else if (debugger_sp && debugger_sp->GetUseSourceCache())
+  if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -178,10 +172,10 @@
   m_last_line = start_line;
   m_last_count = count;
 
-  if (m_last_file_sp.get()) {
+  if (FileSP last_file_sp = GetFile(m_last_file_spec)) {
 const uint32_t end_line = start_line + count - 1;
 for (uint32_t line = start_line; line <= end_line; ++line) {
-  if (!m_last_file_sp->LineIsValid(line)) {
+  if (!last_file_sp->LineIsValid(line)) {
 m_last_line = UINT32_MAX;
 break;
   }
@@ -219,12 +213,12 @@
 columnToHighlight = column - 1;
 
   size_t this_line_size =
-  m_last_file_sp->DisplaySourceLines(line, columnToHighlight, 0, 0, s);
+  last_file_sp->DisplaySourceLines(line, columnToHighlight, 0, 0, s);
   if (column != 0 && line == curr_line &&
   should_show_stop_column_with_caret(debugger_sp)) {
 // Display caret cursor.
 std::string src_line;
-m_last_file_sp->GetLine(line, src_line);
+last_file_sp->GetLine(line, src_line);
 s->Printf("\t");
 // Insert a space for every non-tab character in the source line.
 for (size_t i = 0; i + 1 < column && i < src_line.length(); ++i)
@@ -255,10 +249,11 @@
   else
 start_line = 1;
 
-  if (m_last_file_sp.get() != file_sp.get()) {
+  FileSP last_file_sp(GetLastFile());
+  if (last_file_sp.get() != file_sp.get()) {
 if (line == 0)
   m_last_line = 0;
-m_last_file_sp = file_sp;
+m_last_file_spec = file_spec;
   }
   return DisplaySourceLinesWithLineNumbersUsingLastFile(
   start_line, count, line, column, current_line_cstr, s, bp_locs);
@@ -268,14 +263,15 @@
 Stream *s, uint32_t count, bool reverse, const SymbolContextList *bp_locs) {
   // If we get called before anybody has set a default file and line, then try
   // to figure it out here.
-  const bool have_default_file_line = m_last_file_sp && m_last_line > 0;
+  FileSP last_file_sp(GetLastFile());
+  const bool have_default_file_line = last_file_sp && m_last_line > 0;
   if (!m_default_set) {
 FileSpec tmp_spec;
 uint32_t tmp_line;
 GetDefaultFileAndLine(tmp_spec, tmp_line);
   }
 
-  if (m_last_file_sp) {
+  if (last_file_sp) {
 if (m_last_line == UINT32_MAX)
   return 0;
 
@@ -310,22 +306,21 @@
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec &file_spec,
   uint32_t line) {
-  FileSP old_file_sp = m_last_file_sp;
-  m_last_file_sp = GetFile(file_spec);
-
   m_default_set = true;
-  if (m_last_file_sp) {
+  FileSP file_sp(GetFile(file_spec));
+
+  if (file_sp) {
 m_last_line = line;
+m_last_file_spec = file_spec;
 return true;
   } else {
-m_last_file_sp = old_file_sp;
 return false;
   }
 }
 
 bool SourceManager::GetDefaultFileAndLine(FileSpec &file_spec, uint32_t &line) {
-  if (m_last_file_sp) {
-file_spec = m_last_file_sp->GetFileSpec();
+  if (FileSP last_file_sp = GetLastFile()) {
+file_spec = last_file_sp->GetFileSpec();
 line = m_last_line;
 return true;
   } else if (!m_default_set) {
@@ -355,7 +350,7 @@
 .GetBaseAddress()
 .CalculateSymbolContextLineEntry(line_entry)) {
   SetDefaultFileAndLine

[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay marked 3 inline comments as done.
emrekultursay added a comment.

In D76806#1943062 , @labath wrote:

> It's not clear to me what is the user-visible effect of this. It sounds like 
> there should be one, but I don't know what it is...


If LLDB will cache FileSP shared pointers, there should be only one place for 
caching, and it's the Source File Cache, which can be enabled/disabled by the 
user. 
User-facing effect of this change can be found in bug: llvm.org/PR45310


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76806



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

[Lldb-commits] [PATCH] D76827: [lldb/CMake] Fix `install` for multi-configuration generators.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D76827#1944229 , @NathanielMcVicar 
wrote:

> This patch resolved the issue for our VS build. Thanks! Unfortunately, I 
> don't have any way to test it for Xcode. Should I still accept the revision?


I've tested it for Xcode, so feel free to accept it :-)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76827



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

[Lldb-commits] [lldb] 17e4c38 - [lldb/CMake] Fix `install` for multi-configuration generators.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-03-26T09:51:29-07:00
New Revision: 17e4c38739aa78638c783dac6c149858d1c0a550

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

LOG: [lldb/CMake] Fix `install` for multi-configuration generators.

For multi-generator builds like MSVC and Xcode, the install source and
destination of the lldb-python-scripts target contains configuration
dependent paths and therefore need to be substituted.

Differential revision: https://reviews.llvm.org/D76827

Added: 


Modified: 
lldb/CMakeLists.txt

Removed: 




diff  --git a/lldb/CMakeLists.txt b/lldb/CMakeLists.txt
index 00b06119d64a..7e8890da0204 100644
--- a/lldb/CMakeLists.txt
+++ b/lldb/CMakeLists.txt
@@ -224,6 +224,8 @@ if (LLDB_ENABLE_PYTHON)
   else()
 set(LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_RELATIVE_PATH})
   endif()
+  string(REPLACE ${CMAKE_CFG_INTDIR} "\$\{CMAKE_INSTALL_CONFIG_NAME\}" 
LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_INSTALL_PATH})
+  string(REPLACE ${CMAKE_CFG_INTDIR} "\$\{CMAKE_INSTALL_CONFIG_NAME\}" 
lldb_python_build_path ${lldb_python_build_path})
   add_custom_target(lldb-python-scripts)
   add_dependencies(lldb-python-scripts finish_swig)
   install(DIRECTORY ${lldb_python_build_path}/../



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

[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1990
 import_name[0]) {
+  // add symbols that are reexport symbols with a valid import name
+  add_this_entry = true;

aprantl wrote:
> Super nit-picky nit pick:
> 
> `// Add symbols that are reexport symbols with a valid import name.`
Mega nit-picky nit pick: comment should be sentences and start with a capital.




Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1926
+  bool IsSymbolAlreadyPresent() {
+if (flags == LLDB_INVALID_ADDRESS)
+  return true;

`return flags == LLDB_INVALID_ADDRESS`



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4508
+return it;
+  else
+return entries.end();

You don't need the else after a return. 



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4569
+  }
+  if (symbol_is_thumb && func_start_entry->addr == (e.entry.address & 1)) {
+func_start_entry->data = true;

I guess this could be an `else if`



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4598
+uint32_t symbol_flags = 0;
+if (symbol_is_thumb)
+  symbol_flags = MACHO_NLIST_ARM_SYMBOL_IS_THUMB;

This could make a great ternary operator ;-) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76758



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

[Lldb-commits] [PATCH] D76805: Fix SourceManager::SourceFileCache insertion

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay updated this revision to Diff 252881.
emrekultursay marked 3 inline comments as done.
emrekultursay added a comment.

Applied labath@'s suggestions on test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76805

Files:
  lldb/source/Core/SourceManager.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/SourceManagerTest.cpp


Index: lldb/unittests/Core/SourceManagerTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/SourceManagerTest.cpp
@@ -0,0 +1,48 @@
+//===-- SourceManagerTest.cpp 
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/SourceManager.h"
+#include "lldb/Host/FileSystem.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class SourceFileCache : public ::testing::Test {
+public:
+  void SetUp() override { FileSystem::Initialize(); }
+  void TearDown() override { FileSystem::Terminate(); }
+};
+
+TEST_F(SourceFileCache, FindSourceFileFound) {
+  SourceManager::SourceFileCache cache;  
+  
+  // Insert: foo
+  FileSpec foo_file_spec("foo");
+  auto foo_file_sp = std::make_shared(foo_file_spec, 
nullptr);
+  cache.AddSourceFile(foo_file_sp);
+
+  // Query: foo, expect found.
+  FileSpec another_foo_file_spec("foo");
+  ASSERT_EQ(cache.FindSourceFile(another_foo_file_spec), foo_file_sp);
+}
+
+TEST_F(SourceFileCache, FindSourceFileNotFound) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec foo_file_spec("foo");
+  auto foo_file_sp =
+  std::make_shared(foo_file_spec, nullptr);
+  cache.AddSourceFile(foo_file_sp);
+   
+  // Query: bar, expect not found.
+  FileSpec bar_file_spec("bar");
+  ASSERT_EQ(cache.FindSourceFile(bar_file_spec), nullptr);
+}
Index: lldb/unittests/Core/CMakeLists.txt
===
--- lldb/unittests/Core/CMakeLists.txt
+++ lldb/unittests/Core/CMakeLists.txt
@@ -1,6 +1,7 @@
 add_lldb_unittest(LLDBCoreTests
   MangledTest.cpp
   RichManglingContextTest.cpp
+  SourceManagerTest.cpp
   StreamCallbackTest.cpp
   UniqueCStringMapTest.cpp
 
Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -696,7 +696,7 @@
 }
 
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP &file_sp) {
-  FileSpec file_spec;
+  FileSpec file_spec = file_sp->GetFileSpec();
   FileCache::iterator pos = m_file_cache.find(file_spec);
   if (pos == m_file_cache.end())
 m_file_cache[file_spec] = file_sp;


Index: lldb/unittests/Core/SourceManagerTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/SourceManagerTest.cpp
@@ -0,0 +1,48 @@
+//===-- SourceManagerTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/SourceManager.h"
+#include "lldb/Host/FileSystem.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class SourceFileCache : public ::testing::Test {
+public:
+  void SetUp() override { FileSystem::Initialize(); }
+  void TearDown() override { FileSystem::Terminate(); }
+};
+
+TEST_F(SourceFileCache, FindSourceFileFound) {
+  SourceManager::SourceFileCache cache;  
+  
+  // Insert: foo
+  FileSpec foo_file_spec("foo");
+  auto foo_file_sp = std::make_shared(foo_file_spec, nullptr);
+  cache.AddSourceFile(foo_file_sp);
+
+  // Query: foo, expect found.
+  FileSpec another_foo_file_spec("foo");
+  ASSERT_EQ(cache.FindSourceFile(another_foo_file_spec), foo_file_sp);
+}
+
+TEST_F(SourceFileCache, FindSourceFileNotFound) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec foo_file_spec("foo");
+  auto foo_file_sp =
+  std::make_shared(foo_file_spec, nullptr);
+  cache.AddSourceFile(foo_file_sp);
+   
+  // Query: bar, expect not found.
+  FileSpec bar_file_spec("bar");
+  ASSERT_EQ(cache.FindSourceFile(bar_file_spec), nullptr);
+}
Index: lldb/unittests/Core/CMakeLists.txt
===
--- lldb/unittests/Core/CMakeLists.txt
+++ lldb/unittests/Core/CMakeLists.txt
@@ -1,6 +1,7 @@
 add_lldb_unittest(LLDBCoreTests
   MangledTest.cpp
   RichManglingContextTest.cpp
+  Sou

[Lldb-commits] [PATCH] D76827: [lldb/CMake] Fix `install` for multi-configuration generators.

[Nathaniel McVicar via Phabricator via lldb-commits]

NathanielMcVicar added a comment.

This patch resolved the issue for our VS build. Thanks! Unfortunately, I don't 
have any way to test it for Xcode. Should I still accept the revision?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76827



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

[Lldb-commits] [PATCH] D76805: Fix SourceManager::SourceFileCache insertion

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay added a comment.

> Does this actually depend on the other patch? It looks like an independent 
> fix we could commit separately.

This bug seems to have existed forever. Fixing it means we will have source 
file cache enabled for the first time. If it causes any unforeseen issues, I'd 
like users to have the ability to disable the cache, which is why I made this 
change depend on the other change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76805



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

[Lldb-commits] [PATCH] D76593: [lldb-vscode] Convert launch_info and attach_info to local variables

[Tatyana Krasnukha via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa92673fe9a08: [lldb-vscode] Convert launch_info and 
attach_info to local variables (authored by anton.kolesov, committed by 
tatyana-krasnukha).

Changed prior to commit:
  https://reviews.llvm.org/D76593?vs=252503&id=252873#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76593

Files:
  lldb/tools/lldb-vscode/VSCode.cpp
  lldb/tools/lldb-vscode/VSCode.h
  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
@@ -508,13 +508,14 @@
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
+  lldb::SBAttachInfo attach_info;
   auto arguments = request.getObject("arguments");
   const lldb::pid_t pid =
   GetUnsigned(arguments, "pid", LLDB_INVALID_PROCESS_ID);
   if (pid != LLDB_INVALID_PROCESS_ID)
-g_vsc.attach_info.SetProcessID(pid);
+attach_info.SetProcessID(pid);
   const auto wait_for = GetBoolean(arguments, "waitFor", false);
-  g_vsc.attach_info.SetWaitForLaunch(wait_for, false /*async*/);
+  attach_info.SetWaitForLaunch(wait_for, false /*async*/);
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
@@ -547,20 +548,19 @@
   g_vsc.RunPreRunCommands();
 
   if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
-char attach_info[256];
-auto attach_info_len =
-snprintf(attach_info, sizeof(attach_info),
- "Waiting to attach to \"%s\"...",
- g_vsc.target.GetExecutable().GetFilename());
-g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_info,
-  attach_info_len));
+char attach_msg[256];
+auto attach_msg_len = snprintf(attach_msg, sizeof(attach_msg),
+   "Waiting to attach to \"%s\"...",
+   g_vsc.target.GetExecutable().GetFilename());
+g_vsc.SendOutput(OutputType::Console,
+ llvm::StringRef(attach_msg, attach_msg_len));
   }
   if (attachCommands.empty()) {
 // No "attachCommands", just attach normally.
 // Disable async events so the attach will be successful when we return from
 // the launch call and the launch will happen synchronously
 g_vsc.debugger.SetAsync(false);
-g_vsc.target.Attach(g_vsc.attach_info, error);
+g_vsc.target.Attach(attach_info, error);
 // Reenable async events
 g_vsc.debugger.SetAsync(true);
   } else {
@@ -1381,26 +1381,26 @@
   }
 
   // Instantiate a launch info instance for the target.
-  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+  auto launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
-g_vsc.launch_info.SetWorkingDirectory(cwd.data());
+launch_info.SetWorkingDirectory(cwd.data());
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())
-g_vsc.launch_info.SetArguments(MakeArgv(args).data(), true);
+launch_info.SetArguments(MakeArgv(args).data(), true);
 
   // Pass any environment variables along that the user specified.
   auto envs = GetStrings(arguments, "env");
   if (!envs.empty())
-g_vsc.launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
+launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
 
-  auto flags = g_vsc.launch_info.GetLaunchFlags();
+  auto flags = launch_info.GetLaunchFlags();
 
   if (GetBoolean(arguments, "disableASLR", true))
 flags |= lldb::eLaunchFlagDisableASLR;
@@ -1409,9 +1409,9 @@
   if (GetBoolean(arguments, "shellExpandArguments", false))
 flags |= lldb::eLaunchFlagShellExpandArguments;
   const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
-  g_vsc.launch_info.SetDetachOnError(detatchOnError);
-  g_vsc.launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
-   lldb::eLaunchFlagStopAtEntry);
+  launch_info.SetDetachOnError(detatchOnError);
+  launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |
+ lldb::eLaunchFlagStopAtEntry);
 
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
@@ -1419,7 +1419,7 @@
 // Disable async events so the launch will be successful when we return from
 // the launch call and the launch will happen synchronously
 g_vsc.debugger.SetAsync(false);
-g

[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows

[Adrian McCarthy via Phabricator via lldb-commits]

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

The code is correct, so I'm approving, but I suggest a couple fixes to the 
comments before committing..




Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:27
+  // Environment buffer is a list of null-terminated list of strings. It ends
+  // with a double null.
   for (const auto &KV : env) {

You have an extra "list of" between null-terminated and "strings".  And I think 
we should point out that it's UTF-16 encoded, so maybe:

```
// The buffer is a list of null-terminated UTF-16 strings, followed by an
// extra L'\0' (two bytes of 0).  An empty environment must have one
// empty string, followed by an extra L'\0'.
```





Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:39
   buffer.push_back(0);
+  // If the environment was empty, we must insert an extra byte to ensure a
+  // double null.

"... an extra **two** bytes ..."



Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:41
+  // double null.
+  if (env.empty()) {
+buffer.push_back(0);

There would be no harm in always adding the extra terminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76835



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

[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows

[Frederic Riss via Phabricator via lldb-commits]

friss added a comment.

Thanks for fixing this Pavel!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76835



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

[Lldb-commits] [lldb] a92673f - [lldb-vscode] Convert launch_info and attach_info to local variables

[Tatyana Krasnukha via lldb-commits]

Author: Anton Kolesov
Date: 2020-03-26T18:48:40+03:00
New Revision: a92673fe9a08b5ed4f67cc52782bacc29cf945ec

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

LOG: [lldb-vscode] Convert launch_info and attach_info to local variables

Those fields inside of the global variable can be local variables because
they are used in only inside of one function: request_launch for launch_info
and request_attach for attach_info.

To avoid confusion an already existing local variable attach_info of
request_attach has been renamed to better reflect its purpose.

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

Added: 


Modified: 
lldb/tools/lldb-vscode/VSCode.cpp
lldb/tools/lldb-vscode/VSCode.h
lldb/tools/lldb-vscode/lldb-vscode.cpp

Removed: 




diff  --git a/lldb/tools/lldb-vscode/VSCode.cpp 
b/lldb/tools/lldb-vscode/VSCode.cpp
index 8112f4c8d0d9..36bc8ec8ebfd 100644
--- a/lldb/tools/lldb-vscode/VSCode.cpp
+++ b/lldb/tools/lldb-vscode/VSCode.cpp
@@ -28,8 +28,8 @@ namespace lldb_vscode {
 VSCode g_vsc;
 
 VSCode::VSCode()
-: launch_info(nullptr), variables(), broadcaster("lldb-vscode"),
-  num_regs(0), num_locals(0), num_globals(0), log(),
+: variables(), broadcaster("lldb-vscode"), num_regs(0), num_locals(0),
+  num_globals(0), log(),
   exception_breakpoints(
   {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
{"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},

diff  --git a/lldb/tools/lldb-vscode/VSCode.h b/lldb/tools/lldb-vscode/VSCode.h
index f23b24d0114e..5298d7554f4d 100644
--- a/lldb/tools/lldb-vscode/VSCode.h
+++ b/lldb/tools/lldb-vscode/VSCode.h
@@ -70,8 +70,6 @@ struct VSCode {
   OutputStream output;
   lldb::SBDebugger debugger;
   lldb::SBTarget target;
-  lldb::SBAttachInfo attach_info;
-  lldb::SBLaunchInfo launch_info;
   lldb::SBValueList variables;
   lldb::SBBroadcaster broadcaster;
   int64_t num_regs;

diff  --git a/lldb/tools/lldb-vscode/lldb-vscode.cpp 
b/lldb/tools/lldb-vscode/lldb-vscode.cpp
index 8c68dd0e7055..ec8000c264ae 100644
--- a/lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ b/lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -508,13 +508,14 @@ void request_attach(const llvm::json::Object &request) {
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
+  lldb::SBAttachInfo attach_info;
   auto arguments = request.getObject("arguments");
   const lldb::pid_t pid =
   GetUnsigned(arguments, "pid", LLDB_INVALID_PROCESS_ID);
   if (pid != LLDB_INVALID_PROCESS_ID)
-g_vsc.attach_info.SetProcessID(pid);
+attach_info.SetProcessID(pid);
   const auto wait_for = GetBoolean(arguments, "waitFor", false);
-  g_vsc.attach_info.SetWaitForLaunch(wait_for, false /*async*/);
+  attach_info.SetWaitForLaunch(wait_for, false /*async*/);
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
@@ -547,20 +548,19 @@ void request_attach(const llvm::json::Object &request) {
   g_vsc.RunPreRunCommands();
 
   if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
-char attach_info[256];
-auto attach_info_len =
-snprintf(attach_info, sizeof(attach_info),
- "Waiting to attach to \"%s\"...",
- g_vsc.target.GetExecutable().GetFilename());
-g_vsc.SendOutput(OutputType::Console, llvm::StringRef(attach_info,
-  attach_info_len));
+char attach_msg[256];
+auto attach_msg_len = snprintf(attach_msg, sizeof(attach_msg),
+   "Waiting to attach to \"%s\"...",
+   g_vsc.target.GetExecutable().GetFilename());
+g_vsc.SendOutput(OutputType::Console,
+ llvm::StringRef(attach_msg, attach_msg_len));
   }
   if (attachCommands.empty()) {
 // No "attachCommands", just attach normally.
 // Disable async events so the attach will be successful when we return 
from
 // the launch call and the launch will happen synchronously
 g_vsc.debugger.SetAsync(false);
-g_vsc.target.Attach(g_vsc.attach_info, error);
+g_vsc.target.Attach(attach_info, error);
 // Reenable async events
 g_vsc.debugger.SetAsync(true);
   } else {
@@ -1381,26 +1381,26 @@ void request_launch(const llvm::json::Object &request) {
   }
 
   // Instantiate a launch info instance for the target.
-  g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
+  auto launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
-g

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[James Henderson via Phabricator via lldb-commits]

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

I haven't really reviewed the funcional parts of this change in the attribute 
parser stuff, but everything else LGTM. Please wait for somebody else to review 
the attribute parser bits.




Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:66-68
+const TagNameMap llvm::ARMBuildAttrs::ARMAttributeTags(tagData,
+   sizeof(tagData) /
+   sizeof(TagNameItem));

clang-format appears to be complaining here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai added a comment.

I create a patch, D76819 , to apply 
clang-format to the ELF header file and ARM build attributes files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Pavel Labath via lldb-commits]

On 25/03/2020 01:04, Adrian McCarthy wrote:
> I took a stab at this, but I'm not seeing any new test failures. 

That is odd.

I was doing some stuff on windows today, so I figured I'd take a stab at
this. I was kind of right that the check in ProcessLauncher windows
prevents us from passing an empty environment.

However, the interesting part starts when I tried to remove that check.
Then the test started behaving nondeterministically -- sometimes passing
and sometimes failing due to ERROR_INVALID_PARAMETER being returned from
CreateProcessW. I can see how windows might need some environment
variables to start up a process correctly, but I do not understand why
this should be nondeterministic...

This is beyond my knowledge of windows. It might be interesting to
reduce this to a simple test case (independent of lldb) and show it to
some windows expert.

I am attaching a patch with the lldb changes I've made, in case you want
to play around with it.

> I assume
> the problem you're seeing is in TestSettings.py, but I've never figured
> out how to run individual Python-based lldb tests since all the
> dotest.py stuff was re-written.

These days we have multiple ways of achieving that. :)

One way would be via the "check-lldb-api-commands-settings" target which
would run all tests under api/commands/settings (i.e. TestSettings.py
and TestQuoting.py).

Another option would be via the lldb-dotest script.
"python bin\lldb-dotest -p TestSettings.py" would just run that single
file. That script passes all of its options to dotest, so you can use
any of the dotest options that you used to use.

pl
From 0e01f02b9b01c9700787c640ac96923acdf0cb0f Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Wed, 25 Mar 2020 13:39:05 +0100
Subject: [PATCH] [lldb] (Almost?) fix TestSettings.test_pass_host_env_vars

A defensive check in ProcessLauncherWindows meant that we would never
attempt to launch a process with a completely empty environment -- the
host environment would be used instead.

After applying this fix, the relevant test starts to pass, but it does
so (at least on my machine) only sporadically. Sometimes, CreateProcessW
seems to fail with ERROR_INVALID_PARAMETER.

My windows-fu is not sufficient to understand what is going on here.
---
 lldb/source/Host/windows/ProcessLauncherWindows.cpp | 6 +-
 lldb/test/API/commands/settings/TestSettings.py | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
index 31101944d..77c7bc269 100644
--- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,9 +23,6 @@ using namespace lldb_private;
 namespace {
 void CreateEnvironmentBuffer(const Environment &env,
  std::vector &buffer) {
-  if (env.size() == 0)
-return;
-
   // Environment buffer is a null terminated list of null terminated strings
   for (const auto &KV : env) {
 std::wstring warg;
@@ -94,8 +91,7 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
 
   LPVOID env_block = nullptr;
   ::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
-  if (!environment.empty())
-env_block = environment.data();
+  env_block = environment.data();
 
   executable = launch_info.GetExecutableFile().GetPath();
   GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);
diff --git a/lldb/test/API/commands/settings/TestSettings.py b/lldb/test/API/commands/settings/TestSettings.py
index c0cdc085f..29360856a 100644
--- a/lldb/test/API/commands/settings/TestSettings.py
+++ b/lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@ class SettingsCommandTestCase(TestBase):
 "Environment variable 'MY_ENV_VAR' successfully passed."])
 
 @skipIfRemote  # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
 def test_pass_host_env_vars(self):
 """Test that the host env vars are passed to the launched process."""
 self.build()
-- 
2.19.2.windows.1

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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai marked an inline comment as done.
HsiangKai added inline comments.



Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:66-68
+const TagNameMap llvm::ARMBuildAttrs::ARMAttributeTags(tagData,
+   sizeof(tagData) /
+   sizeof(TagNameItem));

jhenderson wrote:
> clang-format appears to be complaining here?
I will rerun clang-format for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai updated this revision to Diff 252801.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023

Files:
  lld/ELF/InputFiles.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/include/llvm/Object/ELFObjectFile.h
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/ELFAttributeParser.h
  llvm/include/llvm/Support/ELFAttributes.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/lib/Object/ELF.cpp
  llvm/lib/Object/ELFObjectFile.cpp
  llvm/lib/ObjectYAML/ELFYAML.cpp
  llvm/lib/Support/ARMAttributeParser.cpp
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/ELFAttributes.cpp
  llvm/lib/Support/RISCVAttributeParser.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
  llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/attribute.s
  llvm/test/MC/RISCV/invalid-attribute.s
  llvm/test/tools/llvm-objdump/RISCV/lit.local.cfg
  llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test
  llvm/tools/llvm-readobj/ELFDumper.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/ELFAttributeParserTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -0,0 +1,70 @@
+//===- unittests/RISCVAttributeParserTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "llvm/Support/RISCVAttributeParser.h"
+#include "llvm/Support/ARMBuildAttributes.h"
+#include "llvm/Support/ELFAttributes.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+
+struct RISCVAttributeSection {
+  unsigned Tag;
+  unsigned Value;
+
+  RISCVAttributeSection(unsigned tag, unsigned value)
+  : Tag(tag), Value(value) {}
+
+  void write(raw_ostream &OS) {
+OS.flush();
+// length = length + "riscv\0" + TagFile + ByteSize + Tag + Value;
+// length = 17 bytes
+
+OS << 'A' << (uint8_t)17 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+OS << "riscv" << '\0';
+OS << (uint8_t)1 << (uint8_t)7 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+OS << (uint8_t)Tag << (uint8_t)Value;
+  }
+};
+
+static bool testAttribute(unsigned Tag, unsigned Value, unsigned ExpectedTag,
+  unsigned ExpectedValue) {
+  std::string buffer;
+  raw_string_ostream OS(buffer);
+  RISCVAttributeSection Section(Tag, Value);
+  Section.write(OS);
+  ArrayRef Bytes(reinterpret_cast(OS.str().c_str()),
+  OS.str().size());
+
+  RISCVAttributeParser Parser;
+  cantFail(Parser.parse(Bytes, support::little));
+
+  Optional Attr = Parser.getAttributeValue(ExpectedTag);
+  return Attr.hasValue() && Attr.getValue() == ExpectedValue;
+}
+
+static bool testTagString(unsigned Tag, const char *name) {
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+ .str() == name;
+}
+
+TEST(StackAlign, testAttribute) {
+  EXPECT_TRUE(testTagString(4, "Tag_stack_align"));
+  EXPECT_TRUE(
+  testAttribute(4, 4, RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4));
+  EXPECT_TRUE(
+  testAttribute(4, 16, RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16));
+}
+
+TEST(UnalignedAccess, testAttribute) {
+  EXPECT_TRUE(testTagString(6, "Tag_unaligned_access"));
+  EXPECT_TRUE(testAttribute(6, 0, RISCVAttrs::UNALIGNED_ACCESS,
+RISCVAttrs::NOT_ALLOWED));
+  EXPECT_TRUE(
+  testAttribute(6, 1, RISCVAttrs::UNALIGNED_ACCESS, RISCVAttrs::ALLOWED));
+}
Index: llvm/unittests/Support/ELFAttributeParserTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/ELFAttributeParserTest.cpp
@@ -0,0 +1,63 @@
+//===- unittests/ELFAttributeParserTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apa

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai added a comment.

Harbormaster result:

  Unit tests pass. 64040 tests passed, 0 failed and 650 were skipped.
  clang-tidy fail. clang-tidy found 1 errors and 18 warnings. 0 of them are 
added as review comments why?.
  clang-format fail. Please format your changes with clang-format by running 
`git-clang-format HEAD^` or applying this patch.

These errors are caused by formatting in ELF.h, ARMBuildAttributes.h, and 
ARMBuildAttrs.cpp. Although they are located in the same files this patch 
modified, they are not related to this patch. These errors could be ignored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I remember the patch caused many failures yesterday. The new diff is good.




Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:16
+
+TagNameMap emptyTagNameMap;
+

`static const`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai updated this revision to Diff 252729.
HsiangKai added a comment.

Rebase on D76819 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023

Files:
  lld/ELF/InputFiles.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/include/llvm/Object/ELFObjectFile.h
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/ELFAttributeParser.h
  llvm/include/llvm/Support/ELFAttributes.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/lib/Object/ELF.cpp
  llvm/lib/Object/ELFObjectFile.cpp
  llvm/lib/ObjectYAML/ELFYAML.cpp
  llvm/lib/Support/ARMAttributeParser.cpp
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/ELFAttributes.cpp
  llvm/lib/Support/RISCVAttributeParser.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
  llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/attribute.s
  llvm/test/MC/RISCV/invalid-attribute.s
  llvm/test/tools/llvm-objdump/RISCV/lit.local.cfg
  llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test
  llvm/tools/llvm-readobj/ELFDumper.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/ELFAttributeParserTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -0,0 +1,70 @@
+//===- unittests/RISCVAttributeParserTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "llvm/Support/RISCVAttributeParser.h"
+#include "llvm/Support/ARMBuildAttributes.h"
+#include "llvm/Support/ELFAttributes.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+
+struct RISCVAttributeSection {
+  unsigned Tag;
+  unsigned Value;
+
+  RISCVAttributeSection(unsigned tag, unsigned value)
+  : Tag(tag), Value(value) {}
+
+  void write(raw_ostream &OS) {
+OS.flush();
+// length = length + "riscv\0" + TagFile + ByteSize + Tag + Value;
+// length = 17 bytes
+
+OS << 'A' << (uint8_t)17 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+OS << "riscv" << '\0';
+OS << (uint8_t)1 << (uint8_t)7 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+OS << (uint8_t)Tag << (uint8_t)Value;
+  }
+};
+
+static bool testAttribute(unsigned Tag, unsigned Value, unsigned ExpectedTag,
+  unsigned ExpectedValue) {
+  std::string buffer;
+  raw_string_ostream OS(buffer);
+  RISCVAttributeSection Section(Tag, Value);
+  Section.write(OS);
+  ArrayRef Bytes(reinterpret_cast(OS.str().c_str()),
+  OS.str().size());
+
+  RISCVAttributeParser Parser;
+  cantFail(Parser.parse(Bytes, support::little));
+
+  Optional Attr = Parser.getAttributeValue(ExpectedTag);
+  return Attr.hasValue() && Attr.getValue() == ExpectedValue;
+}
+
+static bool testTagString(unsigned Tag, const char *name) {
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+ .str() == name;
+}
+
+TEST(StackAlign, testAttribute) {
+  EXPECT_TRUE(testTagString(4, "Tag_stack_align"));
+  EXPECT_TRUE(
+  testAttribute(4, 4, RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4));
+  EXPECT_TRUE(
+  testAttribute(4, 16, RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16));
+}
+
+TEST(UnalignedAccess, testAttribute) {
+  EXPECT_TRUE(testTagString(6, "Tag_unaligned_access"));
+  EXPECT_TRUE(testAttribute(6, 0, RISCVAttrs::UNALIGNED_ACCESS,
+RISCVAttrs::NOT_ALLOWED));
+  EXPECT_TRUE(
+  testAttribute(6, 1, RISCVAttrs::UNALIGNED_ACCESS, RISCVAttrs::ALLOWED));
+}
Index: llvm/unittests/Support/ELFAttributeParserTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/ELFAttributeParserTest.cpp
@@ -0,0 +1,63 @@
+//===- unittests/ELFAttributeParserTest.cpp

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai updated this revision to Diff 252710.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023

Files:
  lld/ELF/InputFiles.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/include/llvm/Object/ELFObjectFile.h
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/ELFAttributeParser.h
  llvm/include/llvm/Support/ELFAttributes.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/lib/Object/ELF.cpp
  llvm/lib/Object/ELFObjectFile.cpp
  llvm/lib/ObjectYAML/ELFYAML.cpp
  llvm/lib/Support/ARMAttributeParser.cpp
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/ELFAttributes.cpp
  llvm/lib/Support/RISCVAttributeParser.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
  llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/attribute-with-insts.s
  llvm/test/MC/RISCV/attribute.s
  llvm/test/MC/RISCV/invalid-attribute.s
  llvm/test/tools/llvm-objdump/RISCV/lit.local.cfg
  llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test
  llvm/tools/llvm-readobj/ELFDumper.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/ELFAttributeParserTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -0,0 +1,69 @@
+//===- unittests/RISCVAttributeParserTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "llvm/Support/RISCVAttributeParser.h"
+#include "llvm/Support/ARMBuildAttributes.h"
+#include "llvm/Support/ELFAttributes.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+
+struct RISCVAttributeSection {
+  unsigned Tag;
+  unsigned Value;
+
+  RISCVAttributeSection(unsigned tag, unsigned value) : Tag(tag), Value(value) {}
+
+  void write(raw_ostream &OS) {
+OS.flush();
+// length = length + "riscv\0" + TagFile + ByteSize + Tag + Value;
+// length = 17 bytes
+
+OS << 'A' << (uint8_t)17 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+OS << "riscv" << '\0';
+OS << (uint8_t)1 << (uint8_t)7 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+OS << (uint8_t)Tag << (uint8_t)Value;
+  }
+};
+
+static bool testAttribute(unsigned Tag, unsigned Value, unsigned ExpectedTag,
+   unsigned ExpectedValue) {
+  std::string buffer;
+  raw_string_ostream OS(buffer);
+  RISCVAttributeSection Section(Tag, Value);
+  Section.write(OS);
+  ArrayRef Bytes(reinterpret_cast(OS.str().c_str()),
+  OS.str().size());
+
+  RISCVAttributeParser Parser;
+  cantFail(Parser.parse(Bytes, support::little));
+
+  Optional Attr = Parser.getAttributeValue(ExpectedTag);
+  return Attr.hasValue() && Attr.getValue() == ExpectedValue;
+}
+
+static bool testTagString(unsigned Tag, const char *name) {
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+ .str() == name;
+}
+
+TEST(StackAlign, testAttribute) {
+  EXPECT_TRUE(testTagString(4, "Tag_stack_align"));
+  EXPECT_TRUE(
+  testAttribute(4, 4, RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4));
+  EXPECT_TRUE(
+  testAttribute(4, 16, RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16));
+}
+
+TEST(UnalignedAccess, testAttribute) {
+  EXPECT_TRUE(testTagString(6, "Tag_unaligned_access"));
+  EXPECT_TRUE(testAttribute(6, 0, RISCVAttrs::UNALIGNED_ACCESS,
+RISCVAttrs::NOT_ALLOWED));
+  EXPECT_TRUE(
+  testAttribute(6, 1, RISCVAttrs::UNALIGNED_ACCESS, RISCVAttrs::ALLOWED));
+}
Index: llvm/unittests/Support/ELFAttributeParserTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/ELFAttributeParserTest.cpp
@@ -0,0 +1,63 @@
+//===- unittests/ELFAttributeParserTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2

[Lldb-commits] [PATCH] D76840: [lldb] Fix another crash in covariant type handling

[Raphael Isemann via Phabricator via lldb-commits]

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

To summarize IRC: I think the underlying cause here is that we get the Imported 
callback from the ASTImporter and then recursively start a completely new 
import process from that. I don't think that's actually supported by the 
ASTImporter so we probably should do the same thing we do elsewhere and queue 
all decls that need to be imported (see `CompleteTagDeclsScope`). But this 
improves the overall situation so this should land until this is properly fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76840



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

[Lldb-commits] [PATCH] D76840: [lldb] Fix another crash in covariant type handling

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: teemperor, shafik.
Herald added a project: LLDB.
teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

To summarize IRC: I think the underlying cause here is that we get the Imported 
callback from the ASTImporter and then recursively start a completely new 
import process from that. I don't think that's actually supported by the 
ASTImporter so we probably should do the same thing we do elsewhere and queue 
all decls that need to be imported (see `CompleteTagDeclsScope`). But this 
improves the overall situation so this should land until this is properly fixed.


D73024  seems to have fixed one set crash, but 
it introduced another.
Namely, if a class contains a covariant method returning itself, the
logic in MaybeCompleteReturnType could cause us to attempt a recursive
import, which would result in an assertion failure in
clang::DeclContext::removeDecl.

For some reason, this only manifested itself if the class contained at
least two member variables, and the class itself was imported as a
result of a recursive covariant import.

This patch fixes the crash by not attempting to import classes which are
already completed in MaybeCompleteReturnType. However, it's not clear to
me if this is the right fix, or if this should be handled automatically
by functions lower in the stack.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76840

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
  lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
  lldb/test/API/lang/cpp/covariant-return-types/main.cpp


Index: lldb/test/API/lang/cpp/covariant-return-types/main.cpp
===
--- lldb/test/API/lang/cpp/covariant-return-types/main.cpp
+++ lldb/test/API/lang/cpp/covariant-return-types/main.cpp
@@ -28,6 +28,23 @@
   OtherDerived& getOtherRef() override { return other_derived; }
 };
 
+// A regression test for a class with at least two members containing a
+// covariant function, which is referenced through another covariant function.
+struct BaseWithMembers {
+  int a = 42;
+  int b = 47;
+  virtual BaseWithMembers *get() { return this; }
+};
+struct DerivedWithMembers: BaseWithMembers {
+  DerivedWithMembers *get() override { return this; }
+};
+struct ReferencingBase {
+  virtual BaseWithMembers *getOther() { return new BaseWithMembers(); }
+};
+struct ReferencingDerived: ReferencingBase {
+  DerivedWithMembers *getOther() { return new DerivedWithMembers(); }
+};
+
 int main() {
   Derived derived;
   Base base;
@@ -36,5 +53,7 @@
   (void)base_ptr_to_derived->getRef();
   (void)base_ptr_to_derived->getOtherPtr();
   (void)base_ptr_to_derived->getOtherRef();
+
+  ReferencingDerived referencing_derived;
   return 0; // break here
 }
Index: lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
===
--- lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
+++ lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
@@ -38,3 +38,5 @@
 self.expect_expr("derived.getOtherRef().value()", 
result_summary='"derived"')
 self.expect_expr("base_ptr_to_derived->getOtherRef().value()", 
result_summary='"derived"')
 self.expect_expr("base.getOtherRef().value()", result_summary='"base"')
+
+self.expect_expr("referencing_derived.getOther()->get()->a", 
result_value='42')
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -997,6 +997,8 @@
   clang::RecordDecl *rd = return_type->getPointeeType()->getAsRecordDecl();
   if (!rd)
 return;
+  if (rd->getDefinition())
+return;
 
   importer.CompleteTagDecl(rd);
 }


Index: lldb/test/API/lang/cpp/covariant-return-types/main.cpp
===
--- lldb/test/API/lang/cpp/covariant-return-types/main.cpp
+++ lldb/test/API/lang/cpp/covariant-return-types/main.cpp
@@ -28,6 +28,23 @@
   OtherDerived& getOtherRef() override { return other_derived; }
 };
 
+// A regression test for a class with at least two members containing a
+// covariant function, which is referenced through another covariant function.
+struct BaseWithMembers {
+  int a = 42;
+  int b = 47;
+  virtual BaseWithMembers *get() { return this; }
+};
+struct DerivedWithMembers: BaseWithMembers {
+  DerivedWithMembers *get() override { return this; }
+};
+struct ReferencingBase {
+  virtual BaseWithMembers *getOther() { return new BaseWithMembers(); }
+};
+struct ReferencingDerived: ReferencingBase {
+  DerivedWithMembers *getOther() { return new De

[Lldb-commits] [lldb] e22f0da - [lldb/breakpad] Fix register resolution on arm

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-03-26T13:51:27+01:00
New Revision: e22f0dabcf976668d35595e17645095e97a8177a

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

LOG: [lldb/breakpad] Fix register resolution on arm

In breakpad, only x86 (and mips) registers have a leading '$' in their
names. Arm architectures use plain register names.

Previously, lldb was assuming all registers have a '$'. Fix the code to
match the (unfortunately, inconsistent) reality.

Added: 
lldb/test/Shell/SymbolFile/Breakpad/Inputs/stack-cfi-arm.syms
lldb/test/Shell/SymbolFile/Breakpad/stack-cfi-arm.yaml

Modified: 
lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp 
b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
index 9c2cea4d9727..eeec7296747e 100644
--- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
+++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
@@ -408,20 +408,25 @@ GetRule(llvm::StringRef &unwind_rules) {
 }
 
 static const RegisterInfo *
-ResolveRegister(const SymbolFile::RegisterInfoResolver &resolver,
+ResolveRegister(const llvm::Triple &triple,
+const SymbolFile::RegisterInfoResolver &resolver,
 llvm::StringRef name) {
-  if (name.consume_front("$"))
-return resolver.ResolveName(name);
-
-  return nullptr;
+  if (triple.isX86() || triple.isMIPS()) {
+// X86 and MIPS registers have '$' in front of their register names. Arm 
and
+// AArch64 don't.
+if (!name.consume_front("$"))
+  return nullptr;
+  }
+  return resolver.ResolveName(name);
 }
 
 static const RegisterInfo *
-ResolveRegisterOrRA(const SymbolFile::RegisterInfoResolver &resolver,
+ResolveRegisterOrRA(const llvm::Triple &triple,
+const SymbolFile::RegisterInfoResolver &resolver,
 llvm::StringRef name) {
   if (name == ".ra")
 return resolver.ResolveNumber(eRegisterKindGeneric, 
LLDB_REGNUM_GENERIC_PC);
-  return ResolveRegister(resolver, name);
+  return ResolveRegister(triple, resolver, name);
 }
 
 llvm::ArrayRef SymbolFileBreakpad::SaveAsDWARF(postfix::Node &node) {
@@ -440,6 +445,7 @@ bool SymbolFileBreakpad::ParseCFIUnwindRow(llvm::StringRef 
unwind_rules,
   Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_SYMBOLS);
 
   llvm::BumpPtrAllocator node_alloc;
+  llvm::Triple triple = m_objfile_sp->GetArchitecture().GetTriple();
   while (auto rule = GetRule(unwind_rules)) {
 node_alloc.Reset();
 llvm::StringRef lhs = rule->first;
@@ -455,7 +461,8 @@ bool SymbolFileBreakpad::ParseCFIUnwindRow(llvm::StringRef 
unwind_rules,
   if (name == ".cfa" && lhs != ".cfa")
 return postfix::MakeNode(node_alloc);
 
-  if (const RegisterInfo *info = ResolveRegister(resolver, name)) {
+  if (const RegisterInfo *info =
+  ResolveRegister(triple, resolver, name)) {
 return postfix::MakeNode(
 node_alloc, info->kinds[eRegisterKindLLDB]);
   }
@@ -470,7 +477,8 @@ bool SymbolFileBreakpad::ParseCFIUnwindRow(llvm::StringRef 
unwind_rules,
 llvm::ArrayRef saved = SaveAsDWARF(*rhs);
 if (lhs == ".cfa") {
   row.GetCFAValue().SetIsDWARFExpression(saved.data(), saved.size());
-} else if (const RegisterInfo *info = ResolveRegisterOrRA(resolver, lhs)) {
+} else if (const RegisterInfo *info =
+   ResolveRegisterOrRA(triple, resolver, lhs)) {
   UnwindPlan::Row::RegisterLocation loc;
   loc.SetIsDWARFExpression(saved.data(), saved.size());
   row.SetRegisterInfo(info->kinds[eRegisterKindLLDB], loc);
@@ -574,6 +582,7 @@ SymbolFileBreakpad::ParseWinUnwindPlan(const Bookmark 
&bookmark,
 return nullptr;
   }
   auto it = program.begin();
+  llvm::Triple triple = m_objfile_sp->GetArchitecture().GetTriple();
   const auto &symbol_resolver =
   [&](postfix::SymbolNode &symbol) -> postfix::Node * {
 llvm::StringRef name = symbol.GetName();
@@ -581,7 +590,7 @@ SymbolFileBreakpad::ParseWinUnwindPlan(const Bookmark 
&bookmark,
   if (rule.first == name)
 return rule.second;
 }
-if (const RegisterInfo *info = ResolveRegister(resolver, name))
+if (const RegisterInfo *info = ResolveRegister(triple, resolver, name))
   return postfix::MakeNode(
   node_alloc, info->kinds[eRegisterKindLLDB]);
 return nullptr;
@@ -611,7 +620,7 @@ SymbolFileBreakpad::ParseWinUnwindPlan(const Bookmark 
&bookmark,
 
   // Now process the rest of the assignments.
   for (++it; it != program.end(); ++it) {
-const RegisterInfo *info = ResolveRegister(resolver, it->first);
+const RegisterInfo *info = ResolveRegister(triple, resolver, it->first);
 /

[Lldb-commits] [lldb] 2bfe2b8 - [lldb][testsuite] Check that process is launched successfully in inline tests

[Tatyana Krasnukha via lldb-commits]

Author: Tatyana Krasnukha
Date: 2020-03-26T15:05:30+03:00
New Revision: 2bfe2b878a62db5b008735401d376f68dd0e34ea

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

LOG: [lldb][testsuite] Check that process is launched successfully in inline 
tests

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/lldbinline.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/lldbinline.py 
b/lldb/packages/Python/lldbsuite/test/lldbinline.py
index 22668b673fda..3df26356d908 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbinline.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbinline.py
@@ -137,6 +137,8 @@ def do_test(self):
 parser.set_breakpoints(target)
 
 process = target.LaunchSimple(None, None, 
self.get_process_working_directory())
+self.assertIsNotNone(process, PROCESS_IS_VALID)
+
 hit_breakpoints = 0
 
 while lldbutil.get_stopped_thread(process, lldb.eStopReasonBreakpoint):



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

[Lldb-commits] [lldb] ccf1c30 - [lldb][testsuite] Add lldb-server category

[Tatyana Krasnukha via lldb-commits]

Author: Tatyana Krasnukha
Date: 2020-03-26T15:05:30+03:00
New Revision: ccf1c30cde6e1e763e7c9cdd48a609a805166699

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

LOG: [lldb][testsuite] Add lldb-server category

Added: 
lldb/packages/Python/lldbsuite/test/tools/lldb-server/.categories

Modified: 
lldb/packages/Python/lldbsuite/test/test_categories.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/test_categories.py 
b/lldb/packages/Python/lldbsuite/test/test_categories.py
index 05ce2a15d844..f0d6b9ce17de 100644
--- a/lldb/packages/Python/lldbsuite/test/test_categories.py
+++ b/lldb/packages/Python/lldbsuite/test/test_categories.py
@@ -37,6 +37,7 @@
 'darwin-log': 'Darwin log tests',
 'watchpoint': 'Watchpoint-related tests',
 'lldb-vscode': 'Visual Studio Code debug adaptor tests',
+'lldb-server': 'Tests related to lldb-server',
 }
 
 

diff  --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-server/.categories 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-server/.categories
new file mode 100644
index ..1966eb022b49
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-server/.categories
@@ -0,0 +1 @@
+lldb-server



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

[Lldb-commits] [PATCH] D76697: [lldb] Replace debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType with lldbassert

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

Let's summarize this.

1. In the old and the new version, the program is aborted with an `assert()` if 
the configuration is `LLDB_CONFIGURATION_DEBUG`.
2. In the old version, nothing happens, when the configuration is non-Debug.
3. In the new version, the assert in non-Debug configurations will be turned 
into a warning and *print a warning and encourage the user to file a bug 
report* (@JDevlieghere that is the noise you're referring to, right?)


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

https://reviews.llvm.org/D76697



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

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Pavel Labath via lldb-commits]

On 25/03/2020 17:51, Adrian McCarthy via lldb-commits wrote:
> 
> 
> On Wed, Mar 25, 2020 at 9:49 AM Adrian McCarthy  > wrote:
> 
> 
> 
> On Wed, Mar 25, 2020 at 9:10 AM Pavel Labath  > wrote:
> 
> On 25/03/2020 01:04, Adrian McCarthy wrote:
> > I took a stab at this, but I'm not seeing any new test failures.
> 
> That is odd.
> 
> I was doing some stuff on windows today, so I figured I'd take a
> stab at
> this. I was kind of right that the check in ProcessLauncher windows
> prevents us from passing an empty environment.
> 
> However, the interesting part starts when I tried to remove that
> check.
> Then the test started behaving nondeterministically -- sometimes
> passing
> and sometimes failing due to ERROR_INVALID_PARAMETER being
> returned from
> CreateProcessW. I can see how windows might need some environment
> variables to start up a process correctly, but I do not
> understand why
> this should be nondeterministic...
> 
> 
> Oh, I have a guess.  CreateProcessW takes a pointer to an
> environment block.  If that pointer is null, the process will
> inherit the parent environment.  If you want to pass it an empty
> environment, you have to have a valid pointer to an empty string (or
> possibly to a string with TWO terminating '\0's).
> 
> 
> Scratch the "or possibly."  You definitely need two terminating zeros. 
> Since it's in UTF-16, it wants two L'\0', which is four consecutive zero
> bytes.
>  

Thanks. This is exactly what was needed. It's not what I would have
expected, as the documentation says this is "null-terminated block of
null-terminated strings" -- in my book that would mean that an empty
list of null-terminated strings ends with a single L'\0'. But I guess
this is a very weird corner case, as an empty environment is literally
the only way to *not* need a double null terminator.

Anyway, D76835 is the patch for that.

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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Wow. This is a lot more generic than I had mind. But at the same time, it does 
not seem generic enough. :( Like, for instance, using only component-based 
matching, one couldn't ignore `/usr/lib` while keeping `$BUILD/lib` or whatever.

What I had in mind for your use case was a simple function (*) like 
`repro::???->recordInterestingDirectory(StringRef)`, and calling this 
dynamically from SymbolVendorMacOSX when it opens a dsym (so, approximately 
around here 
,
 maybe after some refactoring to avoid computing the dsym directory twice). 
That seems to be a lot more flexible, requires less plumbing, and about as 
equally obtrusive for the resulting code as attempting to statically describe 
the interesting directories via these matchers.

It's true that may need some kind of matchers for ignoring directories (**), 
but these should probably work on the full path, not individual components. And 
maybe they too shouldn't be fully static, so that they can be controlled by the 
context, or the user... I don't know -- we don't have to design this part now.

So what do you think about just adding the `recordInterestingDirectory` 
function? Would that be sufficient for your needs?

(*) I don't know which class should that function me a member of (if any). 
However, I think it would be nice if it was something in the repro namespace 
(and not the FileSystem class) to make it clear that this is happening for the 
sake of reproducers.

(**) I can see how my previous comment could be interpreted as a request to 
implement ignoring directories. That was not my intention, I am sorry. I was 
just musing about possible future directions (e.g. some function like 
`ignoreUniniterestingDirectory`).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76736: [LLDB] Fix parsing of IPv6 host:port inside brackets

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG57be22fa1797: [LLDB] Fix parsing of IPv6 host:port inside 
brackets (authored by emrekultursay, committed by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76736

Files:
  lldb/source/Utility/UriParser.cpp
  lldb/unittests/Utility/UriParserTest.cpp


Index: lldb/unittests/Utility/UriParserTest.cpp
===
--- lldb/unittests/Utility/UriParserTest.cpp
+++ lldb/unittests/Utility/UriParserTest.cpp
@@ -74,12 +74,19 @@
   VALIDATE
 }
 
-TEST(UriParserTest, TypicalPortPath) {
+TEST(UriParserTest, TypicalPortPathIPv4) {
   const UriTestCase testCase("connect://192.168.100.132:5432/", "connect",
  "192.168.100.132", 5432, "/");
   VALIDATE;
 }
 
+TEST(UriParserTest, TypicalPortPathIPv6) {
+  const UriTestCase testCase(
+  "connect://[2601:600:107f:db64:a42b:4faa:284:3082]:5432/", "connect",
+  "2601:600:107f:db64:a42b:4faa:284:3082", 5432, "/");
+  VALIDATE;
+}
+
 TEST(UriParserTest, BracketedHostnamePort) {
   const UriTestCase testCase("connect://[192.168.100.132]:5432/", "connect",
  "192.168.100.132", 5432, "/");
@@ -102,6 +109,21 @@
   VALIDATE
 }
 
+TEST(UriParserTest, BracketedHostnameWithPortIPv4) {
+  // Android device over IPv4: port is a part of the hostname.
+  const UriTestCase testCase("connect://[192.168.100.132:1234]", "connect",
+ "192.168.100.132:1234", -1, "/");
+  VALIDATE
+}
+
+TEST(UriParserTest, BracketedHostnameWithPortIPv6) {
+  // Android device over IPv6: port is a part of the hostname.
+  const UriTestCase testCase(
+  "connect://[[2601:600:107f:db64:a42b:4faa:284]:1234]", "connect",
+  "[2601:600:107f:db64:a42b:4faa:284]:1234", -1, "/");
+  VALIDATE
+}
+
 TEST(UriParserTest, BracketedHostnameWithColon) {
   const UriTestCase testCase("connect://[192.168.100.132:]:1234", 
"connect",
  "192.168.100.132:", 1234, "/");
Index: lldb/source/Utility/UriParser.cpp
===
--- lldb/source/Utility/UriParser.cpp
+++ lldb/source/Utility/UriParser.cpp
@@ -42,7 +42,7 @@
   // Extract hostname
   if (!host_port.empty() && host_port[0] == '[') {
 // hostname is enclosed with square brackets.
-pos = host_port.find(']');
+pos = host_port.rfind(']');
 if (pos == std::string::npos)
   return false;
 


Index: lldb/unittests/Utility/UriParserTest.cpp
===
--- lldb/unittests/Utility/UriParserTest.cpp
+++ lldb/unittests/Utility/UriParserTest.cpp
@@ -74,12 +74,19 @@
   VALIDATE
 }
 
-TEST(UriParserTest, TypicalPortPath) {
+TEST(UriParserTest, TypicalPortPathIPv4) {
   const UriTestCase testCase("connect://192.168.100.132:5432/", "connect",
  "192.168.100.132", 5432, "/");
   VALIDATE;
 }
 
+TEST(UriParserTest, TypicalPortPathIPv6) {
+  const UriTestCase testCase(
+  "connect://[2601:600:107f:db64:a42b:4faa:284:3082]:5432/", "connect",
+  "2601:600:107f:db64:a42b:4faa:284:3082", 5432, "/");
+  VALIDATE;
+}
+
 TEST(UriParserTest, BracketedHostnamePort) {
   const UriTestCase testCase("connect://[192.168.100.132]:5432/", "connect",
  "192.168.100.132", 5432, "/");
@@ -102,6 +109,21 @@
   VALIDATE
 }
 
+TEST(UriParserTest, BracketedHostnameWithPortIPv4) {
+  // Android device over IPv4: port is a part of the hostname.
+  const UriTestCase testCase("connect://[192.168.100.132:1234]", "connect",
+ "192.168.100.132:1234", -1, "/");
+  VALIDATE
+}
+
+TEST(UriParserTest, BracketedHostnameWithPortIPv6) {
+  // Android device over IPv6: port is a part of the hostname.
+  const UriTestCase testCase(
+  "connect://[[2601:600:107f:db64:a42b:4faa:284]:1234]", "connect",
+  "[2601:600:107f:db64:a42b:4faa:284]:1234", -1, "/");
+  VALIDATE
+}
+
 TEST(UriParserTest, BracketedHostnameWithColon) {
   const UriTestCase testCase("connect://[192.168.100.132:]:1234", "connect",
  "192.168.100.132:", 1234, "/");
Index: lldb/source/Utility/UriParser.cpp
===
--- lldb/source/Utility/UriParser.cpp
+++ lldb/source/Utility/UriParser.cpp
@@ -42,7 +42,7 @@
   // Extract hostname
   if (!host_port.empty() && host_port[0] == '[') {
 // hostname is enclosed with square brackets.
-pos = host_port.find(']');
+pos = host_port.rfind(']');
 if (pos == std::string::npos)
   return false;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: amccarth, friss.
Herald added a project: LLDB.

A defensive check in ProcessLauncherWindows meant that we would never
attempt to launch a process with a completely empty environment -- the
host environment would be used instead. Insted, I make function add an
extra null wchar_t at the end of an empty environment. The documentation
on this is a bit fuzzy, but it seems to be what is needed to make
windows accept these kinds of environments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76835

Files:
  lldb/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/test/API/commands/settings/TestSettings.py


Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@
 "Environment variable 'MY_ENV_VAR' successfully passed."])
 
 @skipIfRemote  # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
 def test_pass_host_env_vars(self):
 """Test that the host env vars are passed to the launched process."""
 self.build()
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,10 +23,8 @@
 namespace {
 void CreateEnvironmentBuffer(const Environment &env,
  std::vector &buffer) {
-  if (env.size() == 0)
-return;
-
-  // Environment buffer is a null terminated list of null terminated strings
+  // Environment buffer is a list of null-terminated list of strings. It ends
+  // with a double null.
   for (const auto &KV : env) {
 std::wstring warg;
 if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
@@ -38,6 +36,12 @@
   // One null wchar_t (to end the block) is two null bytes
   buffer.push_back(0);
   buffer.push_back(0);
+  // If the environment was empty, we must insert an extra byte to ensure a
+  // double null.
+  if (env.empty()) {
+buffer.push_back(0);
+buffer.push_back(0);
+  }
 }
 
 bool GetFlattenedWindowsCommandString(Args args, std::string &command) {
@@ -94,8 +98,7 @@
 
   LPVOID env_block = nullptr;
   ::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
-  if (!environment.empty())
-env_block = environment.data();
+  env_block = environment.data();
 
   executable = launch_info.GetExecutableFile().GetPath();
   GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);


Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@
 "Environment variable 'MY_ENV_VAR' successfully passed."])
 
 @skipIfRemote  # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
 def test_pass_host_env_vars(self):
 """Test that the host env vars are passed to the launched process."""
 self.build()
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,10 +23,8 @@
 namespace {
 void CreateEnvironmentBuffer(const Environment &env,
  std::vector &buffer) {
-  if (env.size() == 0)
-return;
-
-  // Environment buffer is a null terminated list of null terminated strings
+  // Environment buffer is a list of null-terminated list of strings. It ends
+  // with a double null.
   for (const auto &KV : env) {
 std::wstring warg;
 if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
@@ -38,6 +36,12 @@
   // One null wchar_t (to end the block) is two null bytes
   buffer.push_back(0);
   buffer.push_back(0);
+  // If the environment was empty, we must insert an extra byte to ensure a
+  // double null.
+  if (env.empty()) {
+buffer.push_back(0);
+buffer.push_back(0);
+  }
 }
 
 bool GetFlattenedWindowsCommandString(Args args, std::string &command) {
@@ -94,8 +98,7 @@
 
   LPVOID env_block = nullptr;
   ::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
-  if (!environment.empty())
-env_block = environment.data();
+  env_block = environment.data();
 
   executable = launch_info.GetExecutableFile().GetPath();
   GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76569: Convert CommandObjectCommands functions to return StringRefs

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdaed98e5b5d1: Convert CommandObjectCommands functions to 
return StringRefs (authored by shivammittal99, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76569

Files:
  lldb/source/Commands/CommandObjectCommands.cpp


Index: lldb/source/Commands/CommandObjectCommands.cpp
===
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -527,14 +527,13 @@
 m_option_group.NotifyOptionParsingStarting(&exe_ctx);
 
 OptionsWithRaw args_with_suffix(raw_command_line);
-const char *remainder = args_with_suffix.GetRawPart().c_str();
 
 if (args_with_suffix.HasArgs())
   if (!ParseOptionsAndNotify(args_with_suffix.GetArgs(), result,
  m_option_group, exe_ctx))
 return false;
 
-llvm::StringRef raw_command_string(remainder);
+llvm::StringRef raw_command_string = args_with_suffix.GetRawPart();
 Args args(raw_command_string);
 
 if (args.GetArgumentCount() < 2) {
@@ -1171,14 +1170,9 @@
   return llvm::makeArrayRef(g_regex_options);
 }
 
-// TODO: Convert these functions to return StringRefs.
-const char *GetHelp() {
-  return (m_help.empty() ? nullptr : m_help.c_str());
-}
+llvm::StringRef GetHelp() { return m_help; }
 
-const char *GetSyntax() {
-  return (m_syntax.empty() ? nullptr : m_syntax.c_str());
-}
+llvm::StringRef GetSyntax() { return m_syntax; }
 
   protected:
 // Instance variables to hold the values for command options.


Index: lldb/source/Commands/CommandObjectCommands.cpp
===
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -527,14 +527,13 @@
 m_option_group.NotifyOptionParsingStarting(&exe_ctx);
 
 OptionsWithRaw args_with_suffix(raw_command_line);
-const char *remainder = args_with_suffix.GetRawPart().c_str();
 
 if (args_with_suffix.HasArgs())
   if (!ParseOptionsAndNotify(args_with_suffix.GetArgs(), result,
  m_option_group, exe_ctx))
 return false;
 
-llvm::StringRef raw_command_string(remainder);
+llvm::StringRef raw_command_string = args_with_suffix.GetRawPart();
 Args args(raw_command_string);
 
 if (args.GetArgumentCount() < 2) {
@@ -1171,14 +1170,9 @@
   return llvm::makeArrayRef(g_regex_options);
 }
 
-// TODO: Convert these functions to return StringRefs.
-const char *GetHelp() {
-  return (m_help.empty() ? nullptr : m_help.c_str());
-}
+llvm::StringRef GetHelp() { return m_help; }
 
-const char *GetSyntax() {
-  return (m_syntax.empty() ? nullptr : m_syntax.c_str());
-}
+llvm::StringRef GetSyntax() { return m_syntax; }
 
   protected:
 // Instance variables to hold the values for command options.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 57be22f - [LLDB] Fix parsing of IPv6 host:port inside brackets

[Pavel Labath via lldb-commits]

Author: Emre Kultursay
Date: 2020-03-26T11:35:54+01:00
New Revision: 57be22fa179703b78b1807217b72eaffa0c518bf

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

LOG: [LLDB] Fix parsing of IPv6 host:port inside brackets

Summary:
When using IPv6 host:port pairs, typically the host is put inside
brackets, such as [2601:1234:...:0213]:, and the UriParser
can handle this format.

However, the Android infrastructure in LLDB assumes an additional
brackets around the host:port pair, such that the entire host:port
string can be treated as the host (which is used as an Android Serial
Number), and UriParser cannot handle multiple brackets. Parsing
inputs with such extra backets requires searching the closing bracket
from the right.

Test: BracketedHostnameWithPortIPv6 covers the case mentioned above

Reviewers: #lldb, labath

Reviewed By: labath

Subscribers: kwk, shafik, lldb-commits

Tags: #lldb

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

Added: 


Modified: 
lldb/source/Utility/UriParser.cpp
lldb/unittests/Utility/UriParserTest.cpp

Removed: 




diff  --git a/lldb/source/Utility/UriParser.cpp 
b/lldb/source/Utility/UriParser.cpp
index a6172caa8c5c..8169b0eee121 100644
--- a/lldb/source/Utility/UriParser.cpp
+++ b/lldb/source/Utility/UriParser.cpp
@@ -42,7 +42,7 @@ bool UriParser::Parse(llvm::StringRef uri, llvm::StringRef 
&scheme,
   // Extract hostname
   if (!host_port.empty() && host_port[0] == '[') {
 // hostname is enclosed with square brackets.
-pos = host_port.find(']');
+pos = host_port.rfind(']');
 if (pos == std::string::npos)
   return false;
 

diff  --git a/lldb/unittests/Utility/UriParserTest.cpp 
b/lldb/unittests/Utility/UriParserTest.cpp
index c07d59a55e01..c88a647ef937 100644
--- a/lldb/unittests/Utility/UriParserTest.cpp
+++ b/lldb/unittests/Utility/UriParserTest.cpp
@@ -74,12 +74,19 @@ TEST(UriParserTest, LongPath) {
   VALIDATE
 }
 
-TEST(UriParserTest, TypicalPortPath) {
+TEST(UriParserTest, TypicalPortPathIPv4) {
   const UriTestCase testCase("connect://192.168.100.132:5432/", "connect",
  "192.168.100.132", 5432, "/");
   VALIDATE;
 }
 
+TEST(UriParserTest, TypicalPortPathIPv6) {
+  const UriTestCase testCase(
+  "connect://[2601:600:107f:db64:a42b:4faa:284:3082]:5432/", "connect",
+  "2601:600:107f:db64:a42b:4faa:284:3082", 5432, "/");
+  VALIDATE;
+}
+
 TEST(UriParserTest, BracketedHostnamePort) {
   const UriTestCase testCase("connect://[192.168.100.132]:5432/", "connect",
  "192.168.100.132", 5432, "/");
@@ -102,6 +109,21 @@ TEST(UriParserTest, BracketedHostname) {
   VALIDATE
 }
 
+TEST(UriParserTest, BracketedHostnameWithPortIPv4) {
+  // Android device over IPv4: port is a part of the hostname.
+  const UriTestCase testCase("connect://[192.168.100.132:1234]", "connect",
+ "192.168.100.132:1234", -1, "/");
+  VALIDATE
+}
+
+TEST(UriParserTest, BracketedHostnameWithPortIPv6) {
+  // Android device over IPv6: port is a part of the hostname.
+  const UriTestCase testCase(
+  "connect://[[2601:600:107f:db64:a42b:4faa:284]:1234]", "connect",
+  "[2601:600:107f:db64:a42b:4faa:284]:1234", -1, "/");
+  VALIDATE
+}
+
 TEST(UriParserTest, BracketedHostnameWithColon) {
   const UriTestCase testCase("connect://[192.168.100.132:]:1234", 
"connect",
  "192.168.100.132:", 1234, "/");



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

[Lldb-commits] [lldb] daed98e - Convert CommandObjectCommands functions to return StringRefs

[Raphael Isemann via lldb-commits]

Author: Shivam Mittal
Date: 2020-03-26T11:20:38+01:00
New Revision: daed98e5b5d1b4a2ab29b3f757b53519ed070439

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

LOG: Convert CommandObjectCommands functions to return StringRefs

Reviewers: jingham, aprantl, labath, jankratochvil

Reviewed By: labath, jankratochvil

Subscribers: labath, jankratochvil, lldb-commits

Tags: #lldb

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectCommands.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectCommands.cpp 
b/lldb/source/Commands/CommandObjectCommands.cpp
index cf5125dc9306..b12377d71512 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -527,14 +527,13 @@ rather than using a positional placeholder:"
 m_option_group.NotifyOptionParsingStarting(&exe_ctx);
 
 OptionsWithRaw args_with_suffix(raw_command_line);
-const char *remainder = args_with_suffix.GetRawPart().c_str();
 
 if (args_with_suffix.HasArgs())
   if (!ParseOptionsAndNotify(args_with_suffix.GetArgs(), result,
  m_option_group, exe_ctx))
 return false;
 
-llvm::StringRef raw_command_string(remainder);
+llvm::StringRef raw_command_string = args_with_suffix.GetRawPart();
 Args args(raw_command_string);
 
 if (args.GetArgumentCount() < 2) {
@@ -1171,14 +1170,9 @@ a number follows 'f':"
   return llvm::makeArrayRef(g_regex_options);
 }
 
-// TODO: Convert these functions to return StringRefs.
-const char *GetHelp() {
-  return (m_help.empty() ? nullptr : m_help.c_str());
-}
+llvm::StringRef GetHelp() { return m_help; }
 
-const char *GetSyntax() {
-  return (m_syntax.empty() ? nullptr : m_syntax.c_str());
-}
+llvm::StringRef GetSyntax() { return m_syntax; }
 
   protected:
 // Instance variables to hold the values for command options.



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

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk marked 3 inline comments as done.
kwk added a comment.

@labath I made a signficant simplification of starting and killing the server. 
I hope you like that better.




Comment at: lldb/source/Core/SourceManager.cpp:408
+  if ((!file_spec.GetDirectory() && file_spec.GetFilename()) ||
+  !FileSystem::Instance().Exists(m_file_spec)) {
 // If this is just a file name, lets see if we can find it in the

labath wrote:
> jankratochvil wrote:
> > I do not like this extra line as it changes behavior of LLDB unrelated to 
> > `debuginfod`. IIUC if the source file with fully specified 
> > directory+filename in DWARF does not exist but the same filename exists in 
> > a different directory of the sourcetree LLDB will now quietly use the 
> > different file. That's a bug.
> > I think it is there as you needed to initialize `sc.module_sp`.
> Yes, that does not sound right. It may be good to break this function into 
> smaller pieces so you can invoke the thing you need when you need it.
My intention wasn't to leave this as is to be honest. I had comments in here 
that I removed upon request but they existed to remind myself that I haven't 
double checked the logic well enough. I just wanted access to the symbol 
context further down below and thought, that I can take it from up here.



Comment at: lldb/source/Host/common/DebugInfoD.cpp:50
+   "invalid build ID: %s",
+   buildID.GetAsString("").c_str());
+

labath wrote:
> kwk wrote:
> > jankratochvil wrote:
> > > It should not be an error:
> > > ```
> > > echo 'int main(void) { return 0; }' >/tmp/main2.c;gcc -o /tmp/main2 
> > > /tmp/main2.c -Wall -g -Wl,--build-id=none;rm 
> > > /tmp/main2.c;DEBUGINFOD_URLS=http://localhost:8002/ ./bin/lldb /tmp/main2 
> > > -o 'l main' -o q
> > > (lldb) target create "/tmp/main2"
> > > Current executable set to '/tmp/main2' (x86_64).
> > > (lldb) l main
> > > warning: (x86_64) /tmp/main2 An error occurred while finding the source 
> > > file /tmp/main2.c using debuginfod for build ID A9C3D738: invalid build 
> > > ID: A9C3D738
> > > File: /tmp/main2.c
> > > (lldb) q
> > > ```
> > > 
> > Okay, I'll have it return just an empty string. And adjust the comment on 
> > the empty string in findSource documentation. I fully understand that an 
> > error is undesirable in your test case. My question is if the caller should 
> > sanitize it's parameters passed to `findSource` of if the latter should 
> > silently ignore those wrong UUIDs. For now I silently ignore them and treat 
> > a wrong build ID like a not found (e.g. empty string is returned).
> It would be nice to make a test case out of that.
I agree, a test would be nice but not at this stage, where the whole patch 
seems to be at danger.



Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:57
+// RUN: timeout 5 python3 -u -m http.server 0 --directory %t.mock --bind 
"localhost" &> %t.server.log & export PID=$!
+// RUN: trap 'kill $PID;' EXIT INT
+

@labath My bad. I interpreted `timeout 5` wrongly. It will kill the python 
server after `5 seconds` no matter what. If we increase this time to `timeout 
5m` it will kill the server after 5 minutes and we don't need the bash trap. 
Does that sound better? At least the only ugly part would be done this way.  
The whole section would look like this:

```lang=yaml
// RUN: rm -f "%t.server.log"
// RUN: timeout 5m python3 -u -m http.server 0 --directory %t.mock --bind 
"localhost" &> %t.server.log &
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750



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

[Lldb-commits] [lldb] 703a856 - [lldb] Fix TestVSCode_completions for clang 159a9f7

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-03-26T10:24:33+01:00
New Revision: 703a856a1005b651a9087ad16b6df2cc19883dc0

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

LOG: [lldb] Fix TestVSCode_completions for clang 159a9f7

Printing of types has changed slightly.

Also improve the error messages the test gives when it fails.

Added: 


Modified: 
lldb/test/API/tools/lldb-vscode/completions/TestVSCode_completions.py

Removed: 




diff  --git 
a/lldb/test/API/tools/lldb-vscode/completions/TestVSCode_completions.py 
b/lldb/test/API/tools/lldb-vscode/completions/TestVSCode_completions.py
index 155d476bb58a..06ab8f414549 100644
--- a/lldb/test/API/tools/lldb-vscode/completions/TestVSCode_completions.py
+++ b/lldb/test/API/tools/lldb-vscode/completions/TestVSCode_completions.py
@@ -17,10 +17,10 @@ class 
TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase):
 
 def verify_completions(self, actual_list, expected_list, 
not_expected_list=[]):
 for expected_item in expected_list:
-self.assertTrue(expected_item in actual_list)
+self.assertIn(expected_item, actual_list)
 
 for not_expected_item in not_expected_list:
-self.assertFalse(not_expected_item in actual_list)
+self.assertNotIn(not_expected_item, actual_list)
 
 @skipIfWindows
 @skipIfDarwin # Skip this test for now until we can figure out why tings 
aren't working on build bots
@@ -44,7 +44,7 @@ def test_completions(self):
 [
 {
 "text": "var",
-"label": "var -- vector, allocator >, allocator, allocator > > > &",
+"label": "var -- vector, allocator>, allocator, allocator>>> &",
 }
 ],
 [{"text": "var1", "label": "var1 -- int &"}],



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

[Lldb-commits] [PATCH] D76814: Preserve ThreadPlanStacks for unreported threads

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I still don't know much about thread plans, but I didn't see anything that 
would stand out. All my comments are fairly superficial.




Comment at: lldb/source/Target/TargetProperties.td:183
 Desc<"A path to a python OS plug-in module file that contains a 
OperatingSystemPlugIn class.">;
+  def PluginReportsAllThreads: Property<"plugin-reports-all-threads", 
"Boolean">,
+Global,

If this is relevant only for os plugins, then it would be good to reflect that 
in the name as well.



Comment at: 
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp:1-7
+//===-- main.cpp *- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//

We don't put licence headers on tests.



Comment at: 
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp:20
+#include 
+#include 
+

unistd.h is not portable, and it doesn't look like you are using anything from 
it.



Comment at: 
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp:33
+  g_cv.notify_one();
+  g_cv.wait(func_lock);
+}

The synchronization here seems to rely on the fact that there will be no 
spurious wakeups. The simplest way to guard against that is to pass a lambda 
into the wait function `g_cv.wait(func_lock, [&] { return g_value == 2; })`



Comment at: 
lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp:38-41
+  step_out_of_here(); // Expect to stop here after step-out (clang)
+
+  // Return
+  return NULL; // Expect to stop here after step-out (icc and gcc)

The clang/gcc/icc anotations are not used, and who knows if they are even 
accurate at this point.



Comment at: 
lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py:23
+
+def check_list_output(self, command, active_plans = [], completed_plans = 
[], discarded_plans = []):
+# Check the "thread plan list" output against a list of active & 
completed and discarded plans.

this looks like a model example for using `self.filecheck`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76814



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

[Lldb-commits] [PATCH] D76569: Convert CommandObjectCommands functions to return StringRefs

[Shivam Mittal via Phabricator via lldb-commits]

shivammittal99 added a comment.

Can someone please land this patch for me? I do not have commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76569



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

[Lldb-commits] [PATCH] D76806: Remove m_last_file_sp from SourceManager

[Pavel Labath via Phabricator via lldb-commits]

labath added reviewers: labath, jingham.
labath added a comment.

It's not clear to me what is the user-visible effect of this. It sounds like 
there should be one, but I don't know what it is...




Comment at: lldb/source/Core/SourceManager.cpp:175-176
 
-  if (m_last_file_sp.get()) {
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  if (last_file_sp.get()) {
 const uint32_t end_line = start_line + count - 1;

`if(FileSP last_file_sp = GetLastFile())`



Comment at: lldb/source/Core/SourceManager.cpp:253
 
-  if (m_last_file_sp.get() != file_sp.get()) {
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  if (last_file_sp.get() != file_sp.get()) {

Use `GetLastFile`, since we already have it.



Comment at: lldb/source/Core/SourceManager.cpp:267
   // to figure it out here.
-  const bool have_default_file_line = m_last_file_sp && m_last_line > 0;
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  const bool have_default_file_line = last_file_sp && m_last_line > 0;

same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76806



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

[Lldb-commits] [PATCH] D76805: Fix SourceManager::SourceFileCache insertion

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath marked an inline comment as done.
labath added a comment.
This revision is now accepted and ready to land.

Looks good. I'm just picking some nits in the test. I'm assuming that 
@jingham's comment refers to the other patch (and it more-or-less matches what 
I wrote there already).

Does this actually depend on the other patch? It looks like an independent fix 
we could commit separately.




Comment at: lldb/source/Core/SourceManager.cpp:699
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP &file_sp) {
-  FileSpec file_spec;
   FileCache::iterator pos = m_file_cache.find(file_spec);

Woops :)



Comment at: lldb/unittests/Core/SourceManagerTest.cpp:11
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Utility/RegularExpression.h"
+

This doesn't appear to be needed.



Comment at: lldb/unittests/Core/SourceManagerTest.cpp:24-31
+TEST_F(SourceFileCache, AddSourceFile) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec file_spec("foo");
+  auto file_sp = std::make_shared(file_spec, nullptr);
+  cache.AddSourceFile(file_sp);

A test without any assertion is weird. I'd recommend just deleting this since 
insertion is already tested in the other tests.



Comment at: lldb/unittests/Core/SourceManagerTest.cpp:60-63
+  SourceManager::FileSP result = cache.FindSourceFile(bar_file_spec);
+
+  // Expect not found.
+  ASSERT_EQ(result, nullptr);

I'd recommend folding these two statements into one 
(ASSERT_EQ(cache.FindSourceFile(...), nullptr)). That way we'll have at least a 
semi-reasonable error message when this fails instead of a "0xdeadbeef != 0"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76805



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

[Lldb-commits] [PATCH] D76569: Convert CommandObjectCommands functions to return StringRefs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D76569#1942271 , @jankratochvil 
wrote:

> The ASan GCC report was valid ( 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94299#c6 )


Ah, that is cute. :) Thanks for following this up.

Now that the condition is gone (as it should be), we should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76569



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

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath added a comment.

Yeah, the differences between windows&posix handling of memory mapped files is 
quite annoying (though I can't really say that the windows behavior is worse 
than randomly sending out SIGBUSes).

The alternative (which is used in other tools like clangd, I believe) is to not 
use mmap when opening these kinds of files. But I am not sure if that is 
actually better than not caching at all.

In D76804#1942723 , @emrekultursay 
wrote:

> I did not see any existing tests that validate setting/reading of properties. 
> Can you point to me if you know any that exists?


`TestSettings.py` does various setting-related things, but I'm not sure how 
much would reading/writing of this specific setting be valueable, as it would 
literally only test the single declaration in `CoreProperties.td`. It would be 
better to have a test which sets this setting to false and then demonstrates 
that one is able to read/write to the source file. This is something that 
already works elsewhere, but it would at least test this functionality on 
windows (and given that we don't have a failing test, we probably don't have a 
test for this functionality anyway).




Comment at: lldb/source/Core/Debugger.cpp:350
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  SetPrompt(GetPrompt());
+  return ret;

this looks like copy-paste gone wrong. SetPrompt is present in the use-color 
setting because that actually affects the value of the prompt -- this doesn't.

But this does raise the question of whether we should nuke the existing cache 
when this setting is set to false (I think we should).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804



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