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

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay updated this revision to Diff 252734.
emrekultursay added a comment.
Herald added a subscriber: mgorny.

Add unit tests for SourceManager::SourceFileCache


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,64 @@
+//===-- 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 "lldb/Utility/RegularExpression.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, AddSourceFile) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec file_spec("foo");
+  auto file_sp = std::make_shared(file_spec, nullptr);
+  cache.AddSourceFile(file_sp);
+}
+
+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
+  FileSpec another_foo_file_spec("foo");
+  SourceManager::FileSP result = cache.FindSourceFile(another_foo_file_spec);
+
+  // Expect found.
+  ASSERT_EQ(result, 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
+  FileSpec bar_file_spec("bar");
+  SourceManager::FileSP result = cache.FindSourceFile(bar_file_spec);
+
+  // Expect not found.
+  ASSERT_EQ(result, 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 _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,64 @@
+//===-- 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 "lldb/Utility/RegularExpression.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, AddSourceFile) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec file_spec("foo");
+  auto file_sp = std::make_shared(file_spec, nullptr);
+  cache.AddSourceFile(file_sp);
+}
+
+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
+  FileSpec another_foo_file_spec("foo");
+  SourceManager::FileSP result = cache.FindSourceFile(another_foo_file_spec);
+
+  // Expect found.
+  ASSERT_EQ(result, foo_file_sp);
+}
+

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

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay added a comment.

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


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

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

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: clayborg, labath, friss.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
jingham added a parent revision: D75880: [NFC} Move ThreadPlans stacks into 
their own class, store it in Process by TID.

This is the final patch in the work I've done so far to support Process plugins 
(or their associated OS Plugins) not reporting all threads on every stop.

This commit moves the thread plan updating and dumping into the 
ThreadPlanStackMap.  It adds a setting 
(target.process.plugin-reports-all-threads) you can use to indicate that you 
want us to preserve unreported thread plans, and a "thread plan prune" command 
you can use to remove any orphaned plans.

If this is approved, I'd like to check in all these pieces.  At this stage if 
you opt into preserving ThreadPlanStacks, the process is pretty minimal.  I 
want to get this more by-hand version working because there are a lot of OS 
Plugins in the wild (there's on in every mach_kernel dSYM) and we have not 
supported them well.  So I want to get something that supports the extant 
plugins.

The next step is to add a way for the plugins to sync up with the thread plans, 
and then call that at public stop time, but I'd like to do that as a separate 
patch.


Repository:
  rG LLVM Github Monorepo

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:
+ 

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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

IIUC, you should be able to test the actual effect you are trying to achieve by 
having a large source file, running "source list" to get it into the File 
Manager, then try to open the source file for writing in a process in a 
subshell that won't have the same mmap.  That should fail without the patch (or 
with the setting turned to cache-on) and succeed with it.


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] D76805: Fix SourceManager::SourceFileCache insertion

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Could we add a unit test for this?


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] D76805: Fix SourceManager::SourceFileCache insertion

[Emre Kultursay via Phabricator via lldb-commits]

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

fix link warning


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


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 _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/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -696,7 +696,7 @@
 }
 
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP _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;
___
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 accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Couple of minor suggestions inside.




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 &&

do we need to amend the comment to describe the new conditions?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2673
   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 &&

`(this_field_info.bit_offset > last_field_end)` ?



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 &&

```
(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?


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] D76804: Add new LLDB setting: use-source-cache

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Testing the effect of this change might be hard, but please add or extend an 
existing test to check that you can set and read the new property.


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

[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 created this revision.
shafik added reviewers: aprantl, labath.
shafik added a reviewer: vsk.

When parsing DWARF and laying out bit-fields we currently don't take into 
account whether we have a base class or not. Currently if the first field is a 
bit-field but the bit offset is due a field we inherit from a base class we 
currently treat it as an unnamed bit-field and therefore add an extra field.

This fix will not check if we have a base class and assume that this offset is 
due to members we are inheriting from the base. We are currently seeing asserts 
during codegen when debugging `clang::DiagnosticOptions`.

This assumption will fail in the case where the first field in the derived 
class in an unnamed bit-field. Fixing the first field being an unnamed 
bit-field looks like it will require a larger change since we will need a way 
to track or discover the last field offset of the bases(s).


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
@@ -2670,7 +2670,10 @@
   // 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 &&
-  !(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
@@ -2670,7 +2670,10 @@
   // 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 &&
-  !(this_field_info.bit_offset < last_field_end)) {
+  !(this_field_info.bit_offset < last_field_end) &&
+  

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

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
emrekultursay added a child revision: D76805: Fix 
SourceManager::SourceFileCache insertion.

LLDB memory-maps large source files, and at the same time, caches
all source files in the Source Cache.

On Windows, memory-mapped source files are not writeable, causing
bad user experience in IDEs (such as errors when saving edited files).
IDEs should have the ability to disable the Source Cache at LLDB
startup, so that users can edit source files while debugging.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76804

Files:
  lldb/include/lldb/Core/Debugger.h
  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
@@ -338,6 +338,18 @@
   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);
+  SetPrompt(GetPrompt());
+  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/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;
+
+  bool SetUseSourceCache(bool use_source_cache);
+
   bool GetHighlightSource() const;
 
   lldb::StopShowColumn GetStopShowColumn() const;


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
@@ -338,6 +338,18 @@
   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);
+  SetPrompt(GetPrompt());
+  return ret;
+}
 bool Debugger::GetHighlightSource() const {
   const uint32_t idx = ePropertyHighlightSource;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Core/CoreProperties.td

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

[Emre Kultursay via Phabricator via lldb-commits]

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

- Remove m_last_file_sp from SourceManager


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76803

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  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
@@ -52,27 +52,21 @@
 
 // SourceManager constructor
 SourceManager::SourceManager(const TargetSP _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 _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(), m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec _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)
+  if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +89,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;
@@ -178,10 +172,11 @@
   m_last_line = start_line;
   m_last_count = count;
 
-  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;
 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 +214,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 +250,11 @@
   else
 start_line = 1;
 
-  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()) {
 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 +264,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(GetFile(m_last_file_spec));
+  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 +307,22 @@
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec _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 _spec, uint32_t ) {
-  if (m_last_file_sp) {
-file_spec = m_last_file_sp->GetFileSpec();
+  FileSP 

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

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

...and replace it with m_last_file_spec instead.

When Source Cache is enabled, the value stored in m_last_file_sp is
already in the Source Cache, and caching it again in SourceManager
brings no extra benefit. All we need is to "remember" the last used
file, and FileSpec can serve the same purpose.

When Source Cache is disabled, the user explicitly requested no caching
of source files, and therefore, m_last_file_sp should NOT be used.

Depends on D76805 .


Repository:
  rG LLVM Github Monorepo

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 _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 _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(), m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec _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,11 @@
   m_last_line = start_line;
   m_last_count = count;
 
-  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;
 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 +214,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 +250,11 @@
   else
 start_line = 1;
 
-  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()) {
 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 +264,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(GetFile(m_last_file_spec));
+  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 +307,22 @@
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec _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 _spec, uint32_t ) {
-  if (m_last_file_sp) {

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

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
emrekultursay added a child revision: D76806: Remove m_last_file_sp from 
SourceManager.

Lookup and subsequent insert was done using uninitialized
FileSpec object, which caused the cache to be a no-op.

Depends on D76804 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76805

Files:
  lldb/source/Core/SourceManager.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 _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/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -696,7 +696,7 @@
 }
 
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP _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;
___
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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil accepted this revision.
jankratochvil added a comment.

The ASan GCC report was valid ( 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94299#c6 ) but that was really 
just the first/previous version of the patch.
I will try to check why clang ASan does not report it (and also GCC ASan on a 
minimal reproducer does not report it for me).


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] D76803: Remove m_last_file_sp from SourceManager

[Emre Kultursay via Phabricator via lldb-commits]

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

No changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76803

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 _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 _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(), m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec _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,11 @@
   m_last_line = start_line;
   m_last_count = count;
 
-  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;
 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 +214,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 +250,11 @@
   else
 start_line = 1;
 
-  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()) {
 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 +264,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(GetFile(m_last_file_spec));
+  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 +307,22 @@
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec _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 _spec, uint32_t ) {
-  if (m_last_file_sp) {
-file_spec = m_last_file_sp->GetFileSpec();
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  if (last_file_sp) {
+file_spec = last_file_sp->GetFileSpec();
 line = m_last_line;
 return true;
   } else if (!m_default_set) {
@@ -355,7 +352,7 @@
 .GetBaseAddress()
 .CalculateSymbolContextLineEntry(line_entry)) {
   SetDefaultFileAndLine(line_entry.file, line_entry.line);

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

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
emrekultursay updated this revision to Diff 252675.
emrekultursay added a comment.
emrekultursay updated this revision to Diff 252677.

No changes


emrekultursay added a comment.

Adding 3rd commit to diff


...and replace it with m_last_file_spec instead.

When Source Cache is enabled, the value stored in m_last_file_sp is
already in the Source Cache, and caching it again in SourceManager
brings no extra benefit. All we need is to "remember" the last used
file, and FileSpec can serve the same purpose.

When Source Cache is disabled, the user explicitly requested no caching
of source files, and therefore, m_last_file_sp should NOT be used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76803

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  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
@@ -52,27 +52,21 @@
 
 // SourceManager constructor
 SourceManager::SourceManager(const TargetSP _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 _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(), m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec _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)
+  if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +89,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;
@@ -178,10 +172,11 @@
   m_last_line = start_line;
   m_last_count = count;
 
-  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;
 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 +214,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 +250,11 @@
   else
 start_line = 1;
 
-  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()) {
 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 +264,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(GetFile(m_last_file_spec));
+  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 +307,22 @@
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec _spec,
  

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

[Emre Kultursay via Phabricator via lldb-commits]

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

Adding 3rd commit to diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76803

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  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
@@ -52,27 +52,21 @@
 
 // SourceManager constructor
 SourceManager::SourceManager(const TargetSP _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 _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(), m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec _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)
+  if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +89,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;
@@ -178,10 +172,11 @@
   m_last_line = start_line;
   m_last_count = count;
 
-  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;
 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 +214,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 +250,11 @@
   else
 start_line = 1;
 
-  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()) {
 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 +264,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(GetFile(m_last_file_spec));
+  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 +307,22 @@
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec _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 _spec, uint32_t ) {
-  if (m_last_file_sp) {
-file_spec = m_last_file_sp->GetFileSpec();
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+ 

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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a reviewer: clayborg.
jingham added a comment.

Greg originally designed the macOS equivalent of this, so I've added him.

I totally agree that you should only do wide searches for source files when 
there's no way to get a narrower context.  Even "source list" could use the 
current thread & frame as a start context, and only fall back to a full search 
when that fails.  In a big project with many shlibs, you might have multiple 
files of the same name, but if someone specifies foo.cpp while stopped in a 
method of libMyLib.dylib, they probably do mean the one in libMyLib.dylib, if 
it exists...

I'm not terribly happy with the way the module-level source-file remapping 
interacts with debug info.  It overrides the source-map without a way to undo 
that.  The dSYM's that Apple uses internally have module-level source remapping 
in them that point to some NFS mounted directory.  We've had problems where 
somebody has copied over one of these dSYM's and the associated sources, but 
doesn't want to remote mount the directories that host the sources.  The only 
way to point lldb at those is to either copy them over to a directory structure 
that matches the remote path, or edit the dSYM to remove the source  remapping. 
 That's pretty annoying.

On the other hand, I don't think we should show the build locations (or at 
least not as a primary thing) for source files that have come from debuginfod 
or from another module level remapping.  That's confusing to anyone who wants 
to open the file in some other way (for instance if you wanted to hand the file 
off to an external editor.)  If we a way to get at both pieces of information, 
the `source info` command could be used to show the "debug info path" and the 
"local path" for a given source file.  We might want some API (on SBFileSpec?) 
to get the original path - that would actually be useful when trying to figure 
out what you should use for a target.source-map.  But if we have a local copy 
of the file you need to be able to get to that path.

The test does seem like it would be much better as a Python test.


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] [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 252649.
JDevlieghere added a comment.

Make the whole thing generic.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672

Files:
  lldb/include/lldb/Host/FileSystem.h
  lldb/include/lldb/Utility/FileCollector.h
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/FileCollector.cpp
  lldb/test/Shell/Reproducer/TestDSYM.test
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/FileCollectorTest.cpp
  llvm/include/llvm/Support/FileCollector.h
  llvm/lib/Support/FileCollector.cpp

Index: llvm/lib/Support/FileCollector.cpp
===
--- llvm/lib/Support/FileCollector.cpp
+++ llvm/lib/Support/FileCollector.cpp
@@ -61,26 +61,25 @@
   return true;
 }
 
-void FileCollector::addFile(const Twine ) {
-  std::lock_guard lock(Mutex);
-  std::string FileStr = file.str();
-  if (markAsSeen(FileStr))
-addFileImpl(FileStr);
-}
+void FileCollector::addFile(const Twine ) { addFileImpl(file.str()); }
 
 void FileCollector::addDirectory(const Twine ) {
   assert(sys::fs::is_directory(dir));
   std::error_code EC;
   sys::fs::recursive_directory_iterator Iter(dir, EC);
   sys::fs::recursive_directory_iterator End;
-  addFile(dir); // Add the directory in case it's empty.
+  addFileImpl(dir.str()); // Add the directory in case it's empty.
   for (; Iter != End && !EC; Iter.increment(EC)) {
 if (sys::fs::is_regular_file(Iter->path()))
-  addFile(Iter->path());
+  addFileImpl(Iter->path());
   }
 }
 
 void FileCollector::addFileImpl(StringRef SrcPath) {
+  std::lock_guard lock(Mutex);
+  if (!markAsSeen(SrcPath))
+return;
+
   // We need an absolute src path to append to the root.
   SmallString<256> AbsoluteSrc = SrcPath;
   sys::fs::make_absolute(AbsoluteSrc);
Index: llvm/include/llvm/Support/FileCollector.h
===
--- llvm/include/llvm/Support/FileCollector.h
+++ llvm/include/llvm/Support/FileCollector.h
@@ -45,7 +45,7 @@
   createCollectorVFS(IntrusiveRefCntPtr BaseFS,
  std::shared_ptr Collector);
 
-private:
+protected:
   void addFileImpl(StringRef SrcPath);
 
   bool markAsSeen(StringRef Path) {
@@ -60,7 +60,6 @@
 VFSWriter.addFileMapping(VirtualPath, RealPath);
   }
 
-protected:
   /// Synchronizes adding files.
   std::mutex Mutex;
 
Index: lldb/unittests/Utility/FileCollectorTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/FileCollectorTest.cpp
@@ -0,0 +1,112 @@
+//===-- ReproducerTest.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 "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/FileCollector.h"
+
+using namespace lldb_private;
+
+class TestFileCollector : public FileCollector {
+public:
+  TestFileCollector() : FileCollector("", ""){};
+
+  using FileCollector::getAction;
+  void addDirectory(const llvm::Twine ) override {
+directories.emplace_back(file.str());
+  }
+  std::vector directories;
+};
+
+TEST(FileCollectorTest, GetActionTest) {
+  {
+TestFileCollector collector;
+collector.registerMatcher("foo", FileCollector::Action::KeepContents,
+  FileCollector::Matcher::Full);
+
+auto action = collector.getAction("foo");
+ASSERT_TRUE(static_cast(action));
+EXPECT_EQ(FileCollector::Action::KeepContents, *action);
+
+action = collector.getAction("bar");
+ASSERT_FALSE(static_cast(action));
+
+action = collector.getAction("foobar");
+ASSERT_FALSE(static_cast(action));
+
+// Register a new matcher that overrides the previous one.
+collector.registerMatcher("foo", FileCollector::Action::KeepContents,
+  FileCollector::Matcher::Substring);
+
+action = collector.getAction("foo");
+ASSERT_TRUE(static_cast(action));
+EXPECT_EQ(FileCollector::Action::KeepContents, *action);
+
+action = collector.getAction("bar");
+ASSERT_FALSE(static_cast(action));
+
+action = collector.getAction("foobar");
+ASSERT_TRUE(static_cast(action));
+EXPECT_EQ(FileCollector::Action::KeepContents, *action);
+  }
+
+  {
+TestFileCollector collector;
+collector.registerMatcher(".foo", FileCollector::Action::KeepContents,
+  FileCollector::Matcher::Extension);
+
+auto action = collector.getAction("foo");
+ASSERT_FALSE(static_cast(action));
+
+action = collector.getAction("bar");
+  

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

[Emre Kultursay via Phabricator via lldb-commits]

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

- Added comments to Android-specific test cases, as suggested by labath@.
- Reformatted lines that exceeded 80 chars.




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] D76736: [LLDB] Fix parsing of IPv6 host:port inside brackets

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay added a comment.

I don't have commit access.


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

https://reviews.llvm.org/D76736



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

[Lldb-commits] [PATCH] D75488: Preserve the owning module information from DWARF in the synthesized AST

[Adrian Prantl via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:1254
+  auto parent_desc = ast_source->getSourceDescriptor(parent.GetValue());
+  std::tie(module, created) = m_module_map_up->findOrCreateModule(
+  name, parent_desc ? parent_desc->getModuleOrNull() : nullptr,

@teemperor This is the place where we need `Module *` instead of a `const 
Module *`.


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

https://reviews.llvm.org/D75488



___
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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I only have a bunch of superficial comments but this seems reasonable as far as 
I can tell.




Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1988
+bool add_this_entry = false;
 if (EXPORT_SYMBOL_FLAGS_REEXPORT & e.entry.flags && import_name &&
 import_name[0]) {

`Flags(e.entry.flags).Test(EXPORT_SYMBOL_FLAGS_REEXPORT)` ?



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;

Super nit-picky nit pick:

`// Add symbols that are reexport symbols with a valid import name.`



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:2012
   }
-  output.push_back(e);
+  if (EXPORT_SYMBOL_FLAGS_REEXPORT & e.entry.flags) {
+reexports.push_back(e);

`Flags(e.entry.flags).Test(EXPORT_SYMBOL_FLAGS_REEXPORT)` ?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4496
+  const TrieEntryWithOffset ) -> bool {
+  return b.entry.address < a.entry.address;
+};

Ah I see. the normal `operator<` sorts by offset.



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4499
+std::sort(external_sym_trie_entries.begin(),
+  external_sym_trie_entries.end(), sort_by_address);
+for (uint32_t i = 0; i < sym_idx; i++) {

Do we need llvm::stable_sort here to have reproducible behavior or does it not 
matter?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4500
+  external_sym_trie_entries.end(), sort_by_address);
+for (uint32_t i = 0; i < sym_idx; i++) {
+  TrieEntryWithOffset ent(0);

Does `for (auto _sym : sym)` work here?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4512
+  if (is_arm && sym[i].GetType() == eSymbolTypeCode &&
+  (sym[i].GetFlags() & MACHO_NLIST_ARM_SYMBOL_IS_THUMB)) {
+ent.entry.address = symbol_lookup_file_addr + 1;

`GetFlags().test(MACHO_NLIST_ARM_SYMBOL_IS_THUMB)` ?

Up to taste; it's more verbose but it's kind of confusing to combine && and & 
in the same expression.



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4514
+ent.entry.address = symbol_lookup_file_addr + 1;
+const auto it = std::lower_bound(external_sym_trie_entries.begin(),
+ external_sym_trie_entries.end(), ent,

This block looks like it could be a lambda because it's the same code as above?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4546
+bool symbol_is_thumb = false;
+if (is_arm && e.entry.address & 1) {
+  symbol_is_thumb = true;

Shows that I don't know the operater precedence rules of C very well, but 
without extra parenthesis this makes me uneasy :-)



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4554
+  FunctionStarts::Entry *func_start_entry =
+  function_starts.FindEntry(e.entry.address, !is_arm);
+  if (func_start_entry) {

Wouldn't this automatically fail if `function_starts_count == 0`?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4556
+  if (func_start_entry) {
+if (symbol_is_thumb == false &&
+func_start_entry->addr == e.entry.address) {

`if (!symbol_is_thumb` ?



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4566
+
+Address symbol_addr;
+if (module_sp->ResolveFileAddress(e.entry.address, symbol_addr)) {

High-level comment what this block is doing?



Comment at: lldb/test/API/macosx/dyld-trie-symbols/Makefile:10
+a.out: main.o
+   $(CC) $(CFLAGS_NO_DEBUG) -dynamiclib -o a.out main.o
+

What does `$(CFLAGS_NO_DEBUG)` achieve in the *linker* invocation?

Could you get rid of this entire rule and just specify `LD_EXTRAS = 
-dynamiclib`?



Comment at: lldb/test/API/macosx/dyld-trie-symbols/Makefile:17
+main.o: main.cpp
+   $(CC) $(CFLAGS_NO_DEBUG) -c $< -o $@

Same question here


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] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via lldb-commits]

HsiangKai updated this revision to Diff 252591.

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.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.0 

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

[Adrian McCarthy via lldb-commits]

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.


>
>
>> 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.
>>
>
> I would be thrilled if either of those worked for me.  Somehow, check-lldb
> works, but the check-lldb-... doesn't.
>
> The lldb-dotest solution always fails for me because I can't figure out
> WTF it wants for PYTHONHOME and PYTHONPATH.
>
>
>>
>> pl
>>
>
___
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

[Adrian McCarthy via lldb-commits]

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).


> 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.
>

I would be thrilled if either of those worked for me.  Somehow, check-lldb
works, but the check-lldb-... doesn't.

The lldb-dotest solution always fails for me because I can't figure out WTF
it wants for PYTHONHOME and PYTHONPATH.


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

[Lldb-commits] [lldb] c726753 - [lldb] add lit.local.cfg for breakpad tests

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-03-25T17:00:46+01:00
New Revision: c72675394a8592dbe9733c6eef305d094f7f8119

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

LOG: [lldb] add lit.local.cfg for breakpad tests

The reason is to add .yaml as a valid test suffix. The test folder
contains one yaml file, which wasn't being run because of that.

Unsurprisingly the test fails, but this was not because the underlying
functionality was broken, but rather because the test was setup
incorrectly (most likely due to overly aggressive simplification of the
test data on my part).

Therefore this patch also tweaks the test inputs in order to test what
they are supposed to test, and also updates some other breakpad tests
(because they depend on the same inputs as this one) to be more
realistic -- specifically it avoids putting symbols to the first page of
the module, as that's where normally the COFF header would reside.

Added: 
lldb/test/Shell/SymbolFile/Breakpad/lit.local.cfg

Modified: 
lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-raSearch.syms
lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.syms
lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.yaml
lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
lldb/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win-no-memory-info.yaml
lldb/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test

Removed: 




diff  --git 
a/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-raSearch.syms 
b/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-raSearch.syms
index 329a280bde85..eb4d615da044 100644
--- a/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-raSearch.syms
+++ b/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-raSearch.syms
@@ -1,15 +1,15 @@
 MODULE windows x86 897DD83EA8C8411897F3A925EE4BF7411 unwind-via-stack-win.pdb
 INFO CODE_ID 5D499B5C5000 unwind-via-stack-win.exe
-PUBLIC  0 0 dummy
-PUBLIC 10 0 call_many
-PUBLIC 80 0 main
-PUBLIC 90 0 many_pointer_args
-PUBLIC 100 0 complex_rasearch
-PUBLIC 110 0 esp_rasearch
-PUBLIC 120 0 nonzero_frame_size
-STACK WIN 4 10 6d 0 0 0 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 80 8 0 0 0 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 90 5 0 0 50 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 100 4 0 0 0 0 0 0 1 $T0 .raSearch 80 + = $eip $T0 ^ = $esp $T0 4 + 
=
-STACK WIN 4 110 4 0 0 0 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp .raSearch 4 
+ =
-STACK WIN 4 120 4 0 0 0 4 8 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
+PUBLIC 1000  0 dummy
+PUBLIC 1010 0 call_many
+PUBLIC 1080 0 main
+PUBLIC 1090 0 many_pointer_args
+PUBLIC 1100 0 complex_rasearch
+PUBLIC 1110 0 esp_rasearch
+PUBLIC 1120 0 nonzero_frame_size
+STACK WIN 4 1010 6d 0 0 0 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
+STACK WIN 4 1080 8 0 0 0 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
+STACK WIN 4 1090 5 0 0 50 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =
+STACK WIN 4 1100 4 0 0 0 0 0 0 1 $T0 .raSearch 80 + = $eip $T0 ^ = $esp $T0 4 
+ =
+STACK WIN 4 1110 4 0 0 0 0 0 0 1 $T0 .raSearch = $eip $T0 ^ = $esp .raSearch 4 
+ =
+STACK WIN 4 1120 4 0 0 0 4 8 0 1 $T0 .raSearch = $eip $T0 ^ = $esp $T0 4 + =

diff  --git 
a/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.syms 
b/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.syms
index b9b8a67095f0..24b6a21c9232 100644
--- a/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.syms
+++ b/lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.syms
@@ -1,17 +1,17 @@
 MODULE windows x86 897DD83EA8C8411897F3A925EE4BF7411 unwind-via-stack-win.pdb
 INFO CODE_ID 5D499B5C5000 unwind-via-stack-win.exe
-PUBLIC  0 0 dummy
-PUBLIC 10 0 call_many
-PUBLIC 80 0 main
-PUBLIC 90 0 many_pointer_args
-PUBLIC 100 0 bogus_rule
-PUBLIC 110 0 bogus_cfa_rhs
-PUBLIC 120 0 bogus_esp_rhs
-PUBLIC 130 0 temporary_var
-STACK WIN 4 10 6d 0 0 0 0 0 0 1 $T0 $esp 80 + = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 80 8 0 0 0 0 0 0 1 $T0 $esp = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 90 5 0 0 50 0 0 0 1 $T0 $esp = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 100 4 0 0 0 0 0 0 1 bogus
-STACK WIN 4 110 4 0 0 0 0 0 0 1 $T0 $bogus = $eip $T0 ^ = $esp $T0 4 + =
-STACK WIN 4 120 4 0 0 0 0 0 0 1 $T0 $esp = $eip $T0 ^ = $esp $bogus 4 + =
-STACK WIN 4 130 4 0 0 0 0 0 0 1 $T0 $esp = $bogus $T0 = $eip $bogus ^ = $esp 
$T0 4 + =
+PUBLIC 1000 0 dummy
+PUBLIC 1010 0 call_many
+PUBLIC 1080 0 main
+PUBLIC 1090 0 many_pointer_args
+PUBLIC 1100 0 bogus_rule
+PUBLIC 1110 0 bogus_cfa_rhs
+PUBLIC 1120 0 bogus_esp_rhs
+PUBLIC 1130 0 temporary_var
+STACK WIN 4 1010 6d 0 0 0 0 0 0 1 $T0 $esp 80 + = $eip $T0 ^ = $esp $T0 4 + =
+STACK WIN 4 1080 8 0 0 0 0 0 

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

[Hsiangkai Wang via Phabricator via lldb-commits]

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

Fix Harbormaster test failures.


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.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] [lldb] 7754b65 - [lldb][NFC] lldb_assert->lldbassert in ClangExpressionParser

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-03-25T14:10:48+01:00
New Revision: 7754b652b3bf0c4b63cccf6e0d4066503684e94a

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

LOG: [lldb][NFC] lldb_assert->lldbassert in ClangExpressionParser

lldbassert is the macro that takes care of passing along line/file/function
to the lldb_assert function. Let's call that instead of manually calling the
function.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 3faf6f238b23..698fea4c2d3c 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -281,23 +281,22 @@ ClangExpressionParser::ClangExpressionParser(
 
   // We can't compile expressions without a target.  So if the exe_scope is
   // null or doesn't have a target, then we just need to get out of here.  I'll
-  // lldb_assert and not make any of the compiler objects since
+  // lldbassert and not make any of the compiler objects since
   // I can't return errors directly from the constructor.  Further calls will
   // check if the compiler was made and
   // bag out if it wasn't.
 
   if (!exe_scope) {
-lldb_assert(exe_scope, "Can't make an expression parser with a null 
scope.",
-__FUNCTION__, __FILE__, __LINE__);
+lldbassert(exe_scope &&
+   "Can't make an expression parser with a null scope.");
 return;
   }
 
   lldb::TargetSP target_sp;
   target_sp = exe_scope->CalculateTarget();
   if (!target_sp) {
-lldb_assert(target_sp.get(),
-"Can't make an expression parser with a null target.",
-__FUNCTION__, __FILE__, __LINE__);
+lldbassert(target_sp.get() &&
+   "Can't make an expression parser with a null target.");
 return;
   }
 



___
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.

Ok, you really want to collect the full dsym, then who am I to stop you. :)

But implementing that collection in the FileCollector does not seem right, 
because at the filesystem level, dsyms are not special (which is why you needed 
to reverse-engineer them from the path). Maybe if we create some sort of an API 
to register files/directories of "special interest", then we could have 
something (`SymbolVendorMacOSX`, most likely) call it to record the full dsym 
when it knows it has encountered it.

Then we could have a similar api to register files of special **dis**interest, 
which could be used to selectively omit things from the reproducer. That part 
may be also interesting for using reproducers internally at google, because the 
binaries there are too big to be shipped around all the time.


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] D76697: [lldb] Replace debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType with lldbassert

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 252517.
teemperor retitled this revision from "[lldb] Remove Debug-only assert in 
AppleObjCTypeEncodingParser::BuildObjCObjectPointerType" to "[lldb] Replace 
debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType 
with lldbassert".
teemperor edited the summary of this revision.
teemperor added a comment.

- moved to lldbassert


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

https://reviews.llvm.org/D76697

Files:
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp


Index: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
@@ -230,15 +230,12 @@
 
 auto types = decl_vendor->FindTypes(ConstString(name), /*max_matches*/ 1);
 
-// The user can forward-declare something that has no definition.  The runtime
-// doesn't prohibit this at all. This is a rare and very weird case.  We keep
-// this assert in debug builds so we catch other weird cases.
-#ifdef LLDB_CONFIGURATION_DEBUG
-assert(!types.empty());
-#else
+// The user can forward-declare something that has no definition.  The 
runtime
+// doesn't prohibit this at all. This is a rare and very weird case.  We 
keep
+// this assert in debug builds so we catch other weird cases.
+lldbassert(!types.empty());
 if (types.empty())
   return ast_ctx.getObjCIdType();
-#endif
 
 return ClangUtil::GetQualType(types.front().GetPointerType());
   } else {


Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
@@ -230,15 +230,12 @@
 
 auto types = decl_vendor->FindTypes(ConstString(name), /*max_matches*/ 1);
 
-// The user can forward-declare something that has no definition.  The runtime
-// doesn't prohibit this at all. This is a rare and very weird case.  We keep
-// this assert in debug builds so we catch other weird cases.
-#ifdef LLDB_CONFIGURATION_DEBUG
-assert(!types.empty());
-#else
+// The user can forward-declare something that has no definition.  The runtime
+// doesn't prohibit this at all. This is a rare and very weird case.  We keep
+// this assert in debug builds so we catch other weird cases.
+lldbassert(!types.empty());
 if (types.empty())
   return ast_ctx.getObjCIdType();
-#endif
 
 return ClangUtil::GetQualType(types.front().GetPointerType());
   } else {
___
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]

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

Looks good. You already mention that in the commit message, but it may be nice 
to also mention somewhere near the test case that these kinds of "URL"s can 
occur when and connecting to and android device over IP (in which case the port 
is part of the "hostname"). So one isn't left wondering why are we testing such 
"nonsensical" hostnames...

Also, do you have commit access?


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

https://reviews.llvm.org/D76736



___
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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath requested changes to this revision.
labath added a subscriber: jingham.
labath added a comment.
This revision now requires changes to proceed.

Adding @jingham. Jim, what do you make of this patch and the feature overall?

I know I said this looks "mostly good", but thinking about this further (and 
reading Jan's comments), I do find that there are still couple of things that 
trouble me here. :/

The first is the module_sp searching logic. I think that was previously here 
mainly to support the case when one enters `source list 
I_am_too_lazy_to_enter_the_full_path.cc`, and it would not normally fire when 
displaying the context after the process stops. But this makes a full-fledged 
feature out of it, as it will run every time we look up a file (if debuginfod 
is enabled, etc.). It seems fine to do this for the "source list" command 
(though it also may be nice to give the user an option to override this logic, 
just like he can specify a full path if he wants to), but doing it for 
stop-context purposes seems wrong, as there we should already have right module 
somewhere up the stack.

The second is the interaction between this and the `target.source-map` setting. 
For searching the file on the local filesystem, we want to use the remapped 
path, but in case of debuginfod, we would want to use the original path 
(ideally the one which doesn't even have the per-module mappings 

 applied). The two of these things make me wonder if this new code is plugged 
in at the right level.

The last one is the test case. I've already said why I don't think this is a 
good test. Now I'll just add one more reason. With python it would be easy to 
create a function which handles the details of starting a fake debug info 
server. With lit, each new test for this (there are going to be more that one, 
I hope) will have to copy the `// RUN:` goo needed to start the server in a 
separate process. Sure, maybe you could do something similar here too and move 
that logic into a shell script, but then this will look even less like a 
"normal" lit test: a RUN line, which invokes a shell script, which invokes 
python in a background process... -- it would be much simpler (and portable) if 
it was python all the way.




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

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.



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

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.


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] [PATCH] D74187: [lldb] Add method Language::IsMangledName

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

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

I agree with @friss that there is a change in behavior. I'm not sure it is 
fixable with what I wrote but I think so.




Comment at: lldb/include/lldb/Target/Language.h:191
 
+  virtual bool IsMangledName(llvm::StringRef name) const = 0;
+

In `Mangled::GuessLanguage()` you call this function with `if 
(lang->IsMangledName(mangled.GetCString()))`. `mangled` in that case is a 
`ConstString`. I wonder why you don't pass that to `IsMangledName`? Is a 
`StringRef` faster? I'm asking because I see  `GetMethodNameVariants` below and 
that also must be called with a `ConstString`.



Comment at: lldb/source/Core/Mangled.cpp:416
-  return lldb::eLanguageTypeC_plus_plus;
-else if (ObjCLanguage::IsPossibleObjCMethodName(mangled_name))
-  return lldb::eLanguageTypeObjC;

In the `ObjCLanguage`'s implementation of `IsMangledName()` I hope to see  a 
call to `sPossibleObjCMethodName`. I think that is what @friss is aiming for 
with D74187#1865342. 



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:97
+  bool IsMangledName(llvm::StringRef name) const override {
+return false;
+  }

Essentially, just `return ObjCLanguage::IsPossibleObjCMethodName(name)`, no? It 
is defined only a few lines further down below.


@friss is that what you mean in order to restore the original behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74187



___
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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

LGTM but I haven't tested it.


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

https://reviews.llvm.org/D76736



___
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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

still lgtm, just make sure to clang-format before committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76593



___
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 252505.
HsiangKai added a comment.

Rebase on master.


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.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 

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 252510.
kwk marked 10 inline comments as done.
kwk added a comment.

- Add newline to end of FindDebuginfod.cmake
- Describe empty string returned from debuginfod::findSource()
- Don't treat build IDs of len <= 8 as an error but simply as not found
- move inexpensive debuginfod::isAvailable() check to beginning of if-stmt
- Simplify line number check in test file to avoid adjusting the line number 
every time the test changes
- Add newline to source-list.cpp test file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750

Files:
  lldb/cmake/modules/FindDebuginfod.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/include/lldb/Host/DebugInfoD.h
  lldb/source/Core/SourceManager.cpp
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/common/DebugInfoD.cpp
  lldb/test/CMakeLists.txt
  lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
  lldb/test/Shell/lit.cfg.py
  lldb/test/Shell/lit.site.cfg.py.in

Index: lldb/test/Shell/lit.site.cfg.py.in
===
--- lldb/test/Shell/lit.site.cfg.py.in
+++ lldb/test/Shell/lit.site.cfg.py.in
@@ -16,6 +16,7 @@
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
 config.lldb_enable_lzma = @LLDB_ENABLE_LZMA@
+config.lldb_enable_debuginfod = @LLDB_ENABLE_DEBUGINFOD@
 config.host_triple = "@LLVM_HOST_TRIPLE@"
 config.lldb_bitness = 64 if @LLDB_IS_64_BITS@ else 32
 config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -117,6 +117,9 @@
 if config.lldb_enable_lzma:
 config.available_features.add('lzma')
 
+if config.lldb_enable_debuginfod:
+config.available_features.add('debuginfod')
+
 if find_executable('xz') != None:
 config.available_features.add('xz')
 
Index: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
@@ -0,0 +1,135 @@
+// clang-format off
+// REQUIRES: debuginfod
+// UNSUPPORTED: darwin, windows
+
+//  Test that we can display the source of functions using debuginfod when the
+//  original source file is no longer present.
+//  
+//  The debuginfod client requires a buildid in the binary, so we compile one in.
+//  We can create the directory structure on disc that the client expects on a
+//  webserver that serves source files. Then we fire up a python based http
+//  server in the root of that mock directory, set the DEBUGINFOD_URLS
+//  environment variable and let LLDB do the rest. 
+//  
+//  Go here to find more about debuginfod:
+//  https://sourceware.org/elfutils/Debuginfod.html
+
+
+//We copy this file because we want to compile and later move it away
+
+// RUN: mkdir -p %t.buildroot
+// RUN: cp %s %t.buildroot/test.cpp
+
+//We use the prefix map in order to overwrite all DW_AT_decl_file paths
+//in the DWARF. We cd into the directory before compiling to get
+//DW_AT_comp_dir pickup %t.buildroot as well so it will be replaced by
+///my/new/path.
+
+// RUN: cd %t.buildroot
+// RUN: %clang_host \
+// RUN:   -g \
+// RUN:   -Wl,--build-id="0xaabbccdd" \
+// RUN:   -fdebug-prefix-map=%t.buildroot=/my/new/path \
+// RUN:   -o %t \
+// RUN:   %t.buildroot/test.cpp
+
+
+//We move the original source file to a directory that looks like a debuginfod
+//URL part.
+
+// RUN: rm -rf %t.mock
+// RUN: mkdir -p   %t.mock/buildid/aabbccdd/source/my/new/path
+// RUN: mv%t.buildroot/test.cpp %t.mock/buildid/aabbccdd/source/my/new/path
+
+
+//Adjust where debuginfod stores cache files:
+
+// RUN: rm -rf %t.debuginfod_cache_path
+// RUN: mkdir -p %t.debuginfod_cache_path
+// RUN: export DEBUGINFOD_CACHE_PATH=%t.debuginfod_cache_path
+
+
+//Start HTTP file server on port picked by OS and wait until it is ready
+//The server will be closed on exit of the test.
+
+// RUN: rm -f "%t.server.log"
+// 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
+
+
+//Extract HTTP address from the first line of the server log
+//(e.g. "Serving HTTP on 127.0.0.1 port 40587 (http://127.0.0.1:40587/) ..")
+
+// RUN: echo -n "Waiting for server to be ready"
+// RUN: SERVER_ADDRESS=""
+// RUN: while [ -z "$SERVER_ADDRESS" ]; do \
+// RUN: echo -n "."; \
+// RUN: sleep 0.01; \
+// RUN: SERVER_ADDRESS=$(head -n1 %t.server.log | grep "http://.\+/\+; -o); \
+// RUN: done
+// RUN: echo "DONE"
+
+
+//-- TEST 1 --  No debuginfod awareness 
+
+
+// RUN: 

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

@jankratochvil thanks for this thorough review. I have to think about one 
comment more precisely but the rest was fixed.




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

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).



Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:103
+// TEST-3: File: /my/new/path/test.cpp
+// TEST-3: 123
+// TEST-3-NEXT:{{[0-9]+}}   // Some context lines before

jankratochvil wrote:
> `s/123/{{[0-9]+}}/?`
Both are fine, but I'll go with your's if that helps. If you can tell me how to 
get a lit `CHECK` statement that checks for incremental numbers, that'll be 
awesome ;)


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] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: clayborg, JDevlieghere, aprantl.
jasonmolenda added a project: LLDB.
Herald added a subscriber: mgrang.

In addition to the traditional nlist records in Mach-O binaries, the dynamic 
linker dyld has a trie structure that it uses to record symbols with external 
linkage.  It uses this structure when resolving symbols at runtime; the nlist 
records can be completely stripped from a binary without detriment.  lldb has 
read the special class of reexport symbols out of the dyld trie until now.

This patch has four parts -

1. Remove checks for an empty string table / nlist table as meaning there is no 
symbol table.

2. Changes ParseTrieEntries to recognize the externally-visible symbols and add 
them to a second array of TrieEntries.

3. After populating the symbol table from the nlist records, looks for any 
matching symbol addresses that we read from the trie, marks them as already 
seen so we don't add duplicated symbols in the table.

4. Adds the trie entries that hadn't already been seen, and marks any function 
starts with those addresses as already-added.

There is a test case that has a variety of text and data symbols with different 
linkage visibility and a test case that checks that we don't have duplicate 
symbol table entries, and that we can still find the externally visible symbols 
after stripping the binary.

The patch at this point is pretty straightforward.  It's easy to make mistakes 
in ObjectFileMachO::ParseSymtab, and in the process of writing this I think I 
made all of them.  But I'm open to any feedback about how things might be done 
more clearly.

Adrian, I wasn't sure how well I'm conforming to best practices on the 
testsuite Makefile, where I'm compiling my main.cpp has a dylib, then making a 
stripped copy.  This works, but if you have a chance to look at it and provide 
feedback, I would appreciate it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76758

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/test/API/macosx/dyld-trie-symbols/Makefile
  lldb/test/API/macosx/dyld-trie-symbols/TestDyldTrieSymbols.py
  lldb/test/API/macosx/dyld-trie-symbols/main.cpp

Index: lldb/test/API/macosx/dyld-trie-symbols/main.cpp
===
--- /dev/null
+++ lldb/test/API/macosx/dyld-trie-symbols/main.cpp
@@ -0,0 +1,29 @@
+int patval;  // external symbol, will not be completely stripped
+int pat(int in) {// external symbol, will not be completely stripped
+  if (patval == 0)
+patval = in;
+  return patval;
+}
+
+static int fooval;  // static symbol, stripped
+int foo() { // external symbol, will not be completely stripped
+  if (fooval == 0)
+fooval = 5;
+  return fooval;
+}
+
+int bazval = 10;   // external symbol, will not be completely stripped
+int baz () {   // external symbol, will not be completely stripped
+  return foo() + bazval;
+}
+
+static int barval = 15; // static symbol, stripped
+static int bar () { // static symbol, stripped; __lldb_unnamed_symbol from func starts
+  return baz() + barval;
+}
+
+int calculate ()   // external symbol, will not be completely stripped
+{
+  return bar();
+}
+
Index: lldb/test/API/macosx/dyld-trie-symbols/TestDyldTrieSymbols.py
===
--- /dev/null
+++ lldb/test/API/macosx/dyld-trie-symbols/TestDyldTrieSymbols.py
@@ -0,0 +1,87 @@
+"""
+Test that we read the exported symbols from the dyld trie
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class DyldTrieSymbolsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipIfRemote
+@skipUnlessDarwin
+
+def test_dyld_trie_symbols(self):
+"""Test that we make create symbol table entries from the dyld trie data structure."""
+self.build()
+unstripped_exe = self.getBuildArtifact("a.out")
+stripped_exe = self.getBuildArtifact("a.out-stripped")
+
+unstripped_target = self.dbg.CreateTarget(unstripped_exe)
+self.assertTrue(unstripped_target.IsValid(), "Got a vaid stripped target.")
+
+# Verify that the expected symbols are present in an unstripped
+# binary, and that we didn't duplicate the entries in the symbol
+# table.
+unstripped_bazval_symbols = unstripped_target.FindSymbols("bazval")
+self.assertEqual(unstripped_bazval_symbols.GetSize(), 1)
+unstripped_patval_symbols = unstripped_target.FindSymbols("patval")
+self.assertEqual(unstripped_patval_symbols.GetSize(), 1)
+unstripped_Z3foo_symbols = unstripped_target.FindSymbols("_Z3foov")
+self.assertEqual(unstripped_Z3foo_symbols.GetSize(), 1)
+unstripped_foo_symbols = unstripped_target.FindSymbols("foo")
+   

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

[Anton Kolesov via Phabricator via lldb-commits]

anton.kolesov updated this revision to Diff 252503.
anton.kolesov retitled this revision from "[lldb-vscode] Convert 
g_vsc.launch_info to a local variable" to "[lldb-vscode] Convert launch_info 
and attach_info to local variables".
anton.kolesov edited the summary of this revision.
anton.kolesov added a comment.

Added attach_info conversion into the same revision.


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,20 @@
   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),
+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_info,
-  attach_info_len));
+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 +1382,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 +1410,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 +1420,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_vsc.target.Launch(g_vsc.launch_info, error);
+