[Lldb-commits] [lldb] r372946 - [lldb][NFC] Use AppendEmptyArgument in CompletionRequest constructor

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Thu Sep 26 00:06:05 2019
New Revision: 372946

URL: http://llvm.org/viewvc/llvm-project?rev=372946&view=rev
Log:
[lldb][NFC] Use AppendEmptyArgument in CompletionRequest constructor

We now have a utility function for this purpose.

(Also fixing the typo in the related comment while I'm at it.)

Modified:
lldb/trunk/source/Utility/CompletionRequest.cpp

Modified: lldb/trunk/source/Utility/CompletionRequest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/CompletionRequest.cpp?rev=372946&r1=372945&r2=372946&view=diff
==
--- lldb/trunk/source/Utility/CompletionRequest.cpp (original)
+++ lldb/trunk/source/Utility/CompletionRequest.cpp Thu Sep 26 00:06:05 2019
@@ -35,14 +35,10 @@ CompletionRequest::CompletionRequest(llv
 
   // The cursor is after a space but the space is not part of the argument.
   // Let's add an empty fake argument to the end to make sure the completion
-  // code Note: The space could be part of the last argument when it's quoted.
+  // code. Note: The space could be part of the last argument when it's quoted.
   if (partial_command.endswith(" ") &&
-  !GetCursorArgumentPrefix().endswith(" ")) {
-m_parsed_line.AppendArgument(llvm::StringRef());
-// Set the cursor to the start of the fake argument.
-m_cursor_index++;
-m_cursor_char_position = 0;
-  }
+  !GetCursorArgumentPrefix().endswith(" "))
+AppendEmptyArgument();
 }
 
 std::string CompletionResult::Completion::GetUniqueKey() const {


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

[Lldb-commits] [lldb] r372952 - SystemInitializer: Use Targets.def to selectively initialize ABI plugins

[Pavel Labath via lldb-commits]

Author: labath
Date: Thu Sep 26 02:47:32 2019
New Revision: 372952

URL: http://llvm.org/viewvc/llvm-project?rev=372952&view=rev
Log:
SystemInitializer: Use Targets.def to selectively initialize ABI plugins

This avoids having to define additional macros in the cmake file, and
and also makes the logic in the cpp files more compact. It is also
easily extendible to other plugin types (instruction emulation?) that
should only be initialized if the corresponding llvm target is built.

Thanks to Ilya Birukov for pointing me to this file.

Modified:
lldb/trunk/source/API/CMakeLists.txt
lldb/trunk/source/API/SystemInitializerFull.cpp
lldb/trunk/tools/lldb-test/CMakeLists.txt
lldb/trunk/tools/lldb-test/SystemInitializerTest.cpp

Modified: lldb/trunk/source/API/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/CMakeLists.txt?rev=372952&r1=372951&r2=372952&view=diff
==
--- lldb/trunk/source/API/CMakeLists.txt (original)
+++ lldb/trunk/source/API/CMakeLists.txt Thu Sep 26 02:47:32 2019
@@ -103,11 +103,6 @@ add_lldb_library(liblldb SHARED
   ${option_install_prefix}
 )
 
-foreach (t ${LLVM_TARGETS_TO_BUILD})
-  set_property(SOURCE SystemInitializerFull.cpp APPEND PROPERTY
-COMPILE_DEFINITIONS "LLVM_TARGET_${t}_BUILT")
-endforeach()
-
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS 
/bigobj)
 endif()

Modified: lldb/trunk/source/API/SystemInitializerFull.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SystemInitializerFull.cpp?rev=372952&r1=372951&r2=372952&view=diff
==
--- lldb/trunk/source/API/SystemInitializerFull.cpp (original)
+++ lldb/trunk/source/API/SystemInitializerFull.cpp Thu Sep 26 02:47:32 2019
@@ -131,6 +131,36 @@ SystemInitializerFull::SystemInitializer
 
 SystemInitializerFull::~SystemInitializerFull() {}
 
+#define LLDB_PROCESS_AArch64(op)   
\
+  ABIMacOSX_arm64::op();   
\
+  ABISysV_arm64::op();
+#define LLDB_PROCESS_ARM(op)   
\
+  ABIMacOSX_arm::op(); 
\
+  ABISysV_arm::op();
+#define LLDB_PROCESS_Hexagon(op) ABISysV_hexagon::op();
+#define LLDB_PROCESS_Mips(op)  
\
+  ABISysV_mips::op();  
\
+  ABISysV_mips64::op();
+#define LLDB_PROCESS_PowerPC(op)   
\
+  ABISysV_ppc::op();  \
+  ABISysV_ppc64::op();
+#define LLDB_PROCESS_SystemZ(op) ABISysV_s390x::op();
+#define LLDB_PROCESS_X86(op)   
\
+  ABIMacOSX_i386::op();
\
+  ABISysV_i386::op();  
\
+  ABISysV_x86_64::op();
\
+  ABIWindows_x86_64::op();
+
+#define LLDB_PROCESS_AMDGPU(op)
+#define LLDB_PROCESS_BPF(op)
+#define LLDB_PROCESS_Lanai(op)
+#define LLDB_PROCESS_MSP430(op)
+#define LLDB_PROCESS_NVPTX(op)
+#define LLDB_PROCESS_RISCV(op)
+#define LLDB_PROCESS_Sparc(op)
+#define LLDB_PROCESS_WebAssembly(op)
+#define LLDB_PROCESS_XCore(op)
+
 llvm::Error SystemInitializerFull::Initialize() {
   if (auto e = SystemInitializerCommon::Initialize())
 return e;
@@ -174,34 +204,8 @@ llvm::Error SystemInitializerFull::Initi
 
   ClangASTContext::Initialize();
 
-#ifdef LLVM_TARGET_AArch64_BUILT
-  ABIMacOSX_arm64::Initialize();
-  ABISysV_arm64::Initialize();
-#endif
-#ifdef LLVM_TARGET_ARM_BUILT
-  ABIMacOSX_arm::Initialize();
-  ABISysV_arm::Initialize();
-#endif
-#ifdef LLVM_TARGET_Hexagon_BUILT
-  ABISysV_hexagon::Initialize();
-#endif
-#ifdef LLVM_TARGET_Mips_BUILT
-  ABISysV_mips::Initialize();
-  ABISysV_mips64::Initialize();
-#endif
-#ifdef LLVM_TARGET_PowerPC_BUILT
-  ABISysV_ppc::Initialize();
-  ABISysV_ppc64::Initialize();
-#endif
-#ifdef LLVM_TARGET_SystemZ_BUILT
-  ABISysV_s390x::Initialize();
-#endif
-#ifdef LLVM_TARGET_X86_BUILT
-  ABIMacOSX_i386::Initialize();
-  ABISysV_i386::Initialize();
-  ABISysV_x86_64::Initialize();
-  ABIWindows_x86_64::Initialize();
-#endif
+#define LLVM_TARGET(t) LLDB_PROCESS_ ## t(Initialize)
+#include "llvm/Config/Targets.def"
 
   ArchitectureArm::Initialize();
   ArchitectureMips::Initialize();
@@ -302,34 +306,8 @@ void SystemInitializerFull::Terminate()
   ArchitectureMips::Terminate();
   ArchitecturePPC64::Terminate();
 
-#ifdef LLVM_TARGET_AArch64_BUILT
-  ABIMacOSX_arm64::Terminate();
-  ABISysV_arm64::Terminate();
-#endif
-#ifdef LLVM_TARGET_ARM_BUILT
-  ABIMacOSX_arm::Terminate();
-  ABISysV_arm::Terminate();
-#endif
-#ifdef LLVM_TARGET_

[Lldb-commits] [lldb] r372961 - Don't stop execution in batch mode when process stops with SIGINT or SIGSTOP

[Tatyana Krasnukha via lldb-commits]

Author: tkrasnukha
Date: Thu Sep 26 03:57:11 2019
New Revision: 372961

URL: http://llvm.org/viewvc/llvm-project?rev=372961&view=rev
Log:
Don't stop execution in batch mode when process stops with SIGINT or SIGSTOP

Summary: Usually, SIGINT and SIGSTOP don't imply a crash, e.g. SIGSTOP is sent 
on process launch and attach on some platforms.

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

Modified:
lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h
lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py
lldb/trunk/source/Commands/CommandObjectProcess.cpp
lldb/trunk/source/Interpreter/CommandInterpreter.cpp
lldb/trunk/source/Interpreter/CommandReturnObject.cpp

Modified: lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h?rev=372961&r1=372960&r2=372961&view=diff
==
--- lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h (original)
+++ lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h Thu Sep 26 
03:57:11 2019
@@ -502,6 +502,8 @@ protected:
 
   void GetProcessOutput();
 
+  bool DidProcessStopAbnormally() const;
+
   void SetSynchronous(bool value);
 
   lldb::CommandObjectSP GetCommandSP(llvm::StringRef cmd,

Modified: lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h?rev=372961&r1=372960&r2=372961&view=diff
==
--- lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h (original)
+++ lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h Thu Sep 26 
03:57:11 2019
@@ -144,14 +144,6 @@ public:
 
   void SetInteractive(bool b);
 
-  bool GetAbnormalStopWasExpected() const {
-return m_abnormal_stop_was_expected;
-  }
-
-  void SetAbnormalStopWasExpected(bool signal_was_expected) {
-m_abnormal_stop_was_expected = signal_was_expected;
-  }
-
 private:
   enum { eStreamStringIndex = 0, eImmediateStreamIndex = 1 };
 
@@ -162,14 +154,6 @@ private:
   bool m_did_change_process_state;
   bool m_interactive; // If true, then the input handle from the debugger will
   // be hooked up
-  bool m_abnormal_stop_was_expected; // This is to support
- // eHandleCommandFlagStopOnCrash vrs.
- // attach.
-  // The attach command often ends up with the process stopped due to a signal.
-  // Normally that would mean stop on crash should halt batch execution, but we
-  // obviously don't want that for attach.  Using this flag, the attach command
-  // (and anything else for which this is relevant) can say that the signal is
-  // expected, and batch command execution can continue.
 };
 
 } // namespace lldb_private

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py?rev=372961&r1=372960&r2=372961&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py 
(original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py 
Thu Sep 26 03:57:11 2019
@@ -78,6 +78,35 @@ class DriverBatchModeTest(PExpectTest):
 import pexpect
 child.expect(pexpect.EOF)
 
+@expectedFlakeyFreeBSD("llvm.org/pr25172 fails rarely on the buildbot")
+def test_batch_mode_launch_stop_at_entry(self):
+"""Test that the lldb driver's batch mode works correctly for process 
launch."""
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+
+# Launch with the option '--stop-at-entry' stops with a signal 
(usually SIGSTOP)
+# that should be suppressed since it doesn't imply a crash and
+# this is not a reason to exit batch mode.
+extra_args = ['-b',
+'-o', 'process launch --stop-at-entry',
+'-o', 'continue',
+]
+self.launch(executable=exe, extra_args=extra_args)
+child = self.child
+
+# Check that the process has been launched:
+child.expect("Process ([0-9]+) launched:")
+# We should have continued:
+child.expect_exact("continue")
+# The App should have not have crashed:
+child.expect_exact("Got there on time and it did not crash.")
+
+# Then lldb should exit.
+child.expect_exact("exited")
+import pexpect
+child.expect(pexpect.EOF)
+
 def closeVictim(self):
 if self.victim is not None:
 self.victim.close()

Modified: lldb/trunk/source/Commands/CommandObject

[Lldb-commits] [PATCH] D67776: Don't stop execution in batch mode when process stops with SIGINT or SIGSTOP

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372961: Don't stop execution in batch mode when process 
stops with SIGINT or SIGSTOP (authored by tkrasnukha, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67776?vs=221744&id=221908#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67776

Files:
  lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
  lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h
  lldb/trunk/packages/Python/lldbsuite/test/driver/batch_mode/TestBatchMode.py
  lldb/trunk/source/Commands/CommandObjectProcess.cpp
  lldb/trunk/source/Interpreter/CommandInterpreter.cpp
  lldb/trunk/source/Interpreter/CommandReturnObject.cpp

Index: lldb/trunk/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/trunk/source/Interpreter/CommandReturnObject.cpp
+++ lldb/trunk/source/Interpreter/CommandReturnObject.cpp
@@ -33,8 +33,7 @@
 
 CommandReturnObject::CommandReturnObject()
 : m_out_stream(), m_err_stream(), m_status(eReturnStatusStarted),
-  m_did_change_process_state(false), m_interactive(true),
-  m_abnormal_stop_was_expected(false) {}
+  m_did_change_process_state(false), m_interactive(true) {}
 
 CommandReturnObject::~CommandReturnObject() {}
 
Index: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp
@@ -63,8 +63,10 @@
 #include "lldb/Utility/Args.h"
 
 #include "lldb/Target/Process.h"
+#include "lldb/Target/StopInfo.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/Thread.h"
+#include "lldb/Target/UnixSignals.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -2138,6 +2140,45 @@
   return platform_sp;
 }
 
+bool CommandInterpreter::DidProcessStopAbnormally() const {
+  TargetSP target_sp = m_debugger.GetTargetList().GetSelectedTarget();
+  if (!target_sp)
+return false;
+
+  ProcessSP process_sp(target_sp->GetProcessSP());
+  if (!process_sp)
+return false;
+
+  if (eStateStopped != process_sp->GetState())
+return false;
+
+  for (const auto &thread_sp : process_sp->GetThreadList().Threads()) {
+StopInfoSP stop_info = thread_sp->GetStopInfo();
+if (!stop_info)
+  return false;
+
+const StopReason reason = stop_info->GetStopReason();
+if (reason == eStopReasonException || reason == eStopReasonInstrumentation)
+  return true;
+
+if (reason == eStopReasonSignal) {
+  const auto stop_signal = static_cast(stop_info->GetValue());
+  UnixSignalsSP signals_sp = process_sp->GetUnixSignals();
+  if (!signals_sp || !signals_sp->SignalIsValid(stop_signal))
+// The signal is unknown, treat it as abnormal.
+return true;
+
+  const auto sigint_num = signals_sp->GetSignalNumberFromName("SIGINT");
+  const auto sigstop_num = signals_sp->GetSignalNumberFromName("SIGSTOP");
+  if ((stop_signal != sigint_num) && (stop_signal != sigstop_num))
+// The signal very likely implies a crash.
+return true;
+}
+  }
+
+  return false;
+}
+
 void CommandInterpreter::HandleCommands(const StringList &commands,
 ExecutionContext *override_context,
 CommandInterpreterRunOptions &options,
@@ -2248,38 +2289,22 @@
 }
 
 // Also check for "stop on crash here:
-bool should_stop = false;
-if (tmp_result.GetDidChangeProcessState() && options.GetStopOnCrash()) {
-  TargetSP target_sp(m_debugger.GetTargetList().GetSelectedTarget());
-  if (target_sp) {
-ProcessSP process_sp(target_sp->GetProcessSP());
-if (process_sp) {
-  for (ThreadSP thread_sp : process_sp->GetThreadList().Threads()) {
-StopReason reason = thread_sp->GetStopReason();
-if (reason == eStopReasonSignal || reason == eStopReasonException ||
-reason == eStopReasonInstrumentation) {
-  should_stop = true;
-  break;
-}
-  }
-}
-  }
-  if (should_stop) {
-if (idx != num_lines - 1)
-  result.AppendErrorWithFormat(
-  "Aborting reading of commands after command #%" PRIu64
-  ": '%s' stopped with a signal or exception.\n",
-  (uint64_t)idx + 1, cmd);
-else
-  result.AppendMessageWithFormat(
-  "Command #%" PRIu64 " '%s' stopped with a signal or exception.\n",
-  (uint64_t)idx + 1, cmd);
+if (tmp_result.GetDidChangeProcessState() && options.GetStopOnCrash() &&
+DidProcessStopAbnormally()) {
+  if (idx != num_lines - 1)
+result.Appe

[Lldb-commits] [PATCH] D68048: [WIP][RFC] Improve fetching the process list on the android platform

[Pavel Labath via Phabricator via lldb-commits]

labath added reviewers: srhines, danalbert.
labath added a comment.

I think we should take a step back first. What is the behavior we expect from 
the  "platform process list" command ? I think the current expectation is (and 
this is consistent with @clayborg's comment in 
https://reviews.llvm.org/D68048#1683238) is that it should only return the 
processes that we can attach to. The way lldb-server implements that right now 
is via comparing user ids 
https://github.com/llvm-mirror/lldb/blob/master/source/Host/linux/Host.cpp#L250.
 While these user ids don't really correspond to what android calls "users", 
they in fact do implement the "can I attach to this" semantics, as you cannot 
attach to a process with a different user id (without doing some serious run-as 
magic, which lldb-server does not know how to do right now).

So, I am not sure if there anything to change here, really... If your goal is 
to be able to see _all_ processes, even those you cannot attach to, then maybe 
we should have a separate flag/command for that. The FindProcesses API already 
supports being able to show all processes, but it does not look like there's a 
way to set that from the "platform process list" command...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68048



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

[Lldb-commits] [PATCH] D67994: [WIP] Modify lldb-test to print out ASTs from symbol file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D67994#1683440 , @shafik wrote:

> I believe this is due to us being lazy as to when we import.


Yes, but doesn't calling `Module::ParseAllDebugSymbols` force us to parse 
everything?  "image dump ast" does dump only the things that have been parsed, 
but that doesn't mean it lldb-test needs to do that too.

IOW, I was not saying you should use "image dump ast" to write the test you 
wanted to write. I was merely saying that we should try to make "lldb-test 
-dump-ast" use the same dumping code as "image dump ast" does (assuming the 
latter outputs the kind of data that you need, but it seems to me that it 
does...). "image dump ast" can remain lazy, and only show the things that have 
been parsed so far (which is also useful sometimes), while "lldb-test" can do 
whatever it takes to parse everything (I would hope that is merely calling 
Module::ParseAllDebugSymbols).


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

https://reviews.llvm.org/D67994



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

[Lldb-commits] [lldb] r372965 - [lldb][modern-type-lookup] Add test for using the ClangModulesDeclVendor

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Thu Sep 26 04:30:41 2019
New Revision: 372965

URL: http://llvm.org/viewvc/llvm-project?rev=372965&view=rev
Log:
[lldb][modern-type-lookup] Add test for using the ClangModulesDeclVendor

Added:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/

lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/Makefile

lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/TestObjModulesModernTypeLookup.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/main.m

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/Makefile
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/Makefile?rev=372965&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/Makefile
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/Makefile
 Thu Sep 26 04:30:41 2019
@@ -0,0 +1,4 @@
+OBJC_SOURCES := main.m
+LD_EXTRAS := -lobjc -framework Foundation
+
+include Makefile.rules

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/TestObjModulesModernTypeLookup.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/TestObjModulesModernTypeLookup.py?rev=372965&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/TestObjModulesModernTypeLookup.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/TestObjModulesModernTypeLookup.py
 Thu Sep 26 04:30:41 2019
@@ -0,0 +1,26 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestObjcModulesModernTypeLookup(TestBase):
+  mydir = TestBase.compute_mydir(__file__)
+
+  @skipUnlessDarwin
+  # gmodules causes this to crash as we seem to get a NSURL type from the 
debug information.
+  @skipIf(debug_info="gmodules")
+  def test(self):
+self.build()
+# Activate modern-type-lookup.
+# FIXME: This has to happen before we create any target otherwise we 
crash...
+self.runCmd("settings set target.experimental.use-modern-type-lookup true")
+(target, process, thread, main_breakpoint) = 
lldbutil.run_to_source_breakpoint(self,
+  "break here", lldb.SBFileSpec("main.m"))
+self.expect("expr @import Foundation")
+self.expect(
+"p *[NSURL URLWithString:@\"http://lldb.llvm.org\"]";,
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+"NSURL",
+"isa",
+"_urlString"])

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/main.m
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/main.m?rev=372965&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/main.m
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/objc-modules/main.m
 Thu Sep 26 04:30:41 2019
@@ -0,0 +1,6 @@
+#import 
+
+int main() {
+  NSLog(@"Hello World");
+  return 0; // break here
+}


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

[Lldb-commits] [lldb] r372971 - [lldb][www] Update bot links

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Thu Sep 26 04:48:45 2019
New Revision: 372971

URL: http://llvm.org/viewvc/llvm-project?rev=372971&view=rev
Log:
[lldb][www] Update bot links

Modified:
lldb/trunk/docs/resources/bots.rst

Modified: lldb/trunk/docs/resources/bots.rst
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/docs/resources/bots.rst?rev=372971&r1=372970&r2=372971&view=diff
==
--- lldb/trunk/docs/resources/bots.rst (original)
+++ lldb/trunk/docs/resources/bots.rst Thu Sep 26 04:48:45 2019
@@ -10,8 +10,8 @@ build slaves. Everyone can `add a buildb
 
 
 * `lldb-x64-windows-ninja 
`_
-* `lldb-x86_64-debian `_
-* `lldb-x86_64-fedora `_
+* `lldb-x86_64-debian `_
+* `lldb-x86_64-fedora `_
 
 Documentation
 -


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

[Lldb-commits] [PATCH] D68069: Explicitly set entry point arch when it's thumb

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This sounds like a reasonable thing to do, but it seems to me we make that more 
generic. Having a symbol for the entry point would help in other cases too, and 
we already go through a lot of trouble to track down various symbol addresses 
(plt parsing, eh_frame parsing, ...), so this would fit the general pattern 
there. So, I think we should just remove the if(arm) check and always create a 
"synthetic" symbol for the entry point if there is no symbol at that address 
already.

For the test, what would you say to writing that as a lit test instead (testing 
the address class deduction via the disassemble command you mentioned)? The 
yaml is actually fairly readable as is, but given that you felt the need to 
include the commands used to produce that input, it might be better to actually 
produce that file via the commands as a part of the test (substituting 
`llvm-mc` for `as` and `ld.lld` for linker).




Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2728
+  symbol_id,
+  "",  // Symbol name.
+  false,   // Is the symbol name mangled?

`GetNextSyntheticSymbolName().GetCString()`



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2737-2738
+  0,  // Offset in section or symbol value.
+  2,  // Size.
+  true,   // Size is valid.
+  false,  // Contains linker annotations?

This is pretty arbitrary. Would it be possible to just set size_is_valid=false, 
and let the usual symbol size deduction logic figure out something reasonable 
here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68069



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

[Lldb-commits] [PATCH] D67996: Convert FileSystem::Open() to return Expected

[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 to me, a extra couple of  small comments inline. Thank you for 
taking your time to do this.




Comment at: lldb/include/lldb/Core/StreamFile.h:38
 
+  StreamFile(std::shared_ptr file) : m_file_sp(file){};
+

assert `file` is not null?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1034
 
-bool PythonFile::GetUnderlyingFile(File &file) const {
+bool PythonFile::GetUnderlyingFile(FileSP &file) const {
   if (!IsValid())

This only has one caller, so it should be pretty simple to make it return the 
file as an actual return value (and it looks like it could be returning a 
unique_ptr).



Comment at: lldb/unittests/Host/FileSystemTest.cpp:292-322
+TEST(FileSystemTest, OpenErrno) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;

lawrence_danna wrote:
> labath wrote:
> > What's the point of having both of these tests? The error code should be 
> > the same no matter how you retrieve it from the expected object, so this is 
> > more of a unit test for the Expected class, than anything else...
> GDBRemoteCommunicationServerCommon.cpp collects and errno value and sends it 
> over the network.   I wanted to confirm FileSystem::Open() was returning and 
> Error value that could be correctly converted into errno.
Yes, I got that part (and it's great that you've added it). But why test the 
same thing two ways? My point was that these two tests should always either 
both succeed, or both fail (any other result would be a bug in the Expected 
class).

BTW, if you wanted to be really fancy, you could write this as something like
```
EXPECT_THAT_EXPECTED(fs.Open(spec, File::eOpenOptionRead, 0, true), 
  llvm::Failed(testing::Property(&ECError::convertToErrorCode, 
 std::error_code(ENOENT, 
std::system_category(;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67996



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

[Lldb-commits] [lldb] r372974 - [lldb][modern-type-lookup] Fix crash when activating modern-type-lookup on Linux

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Thu Sep 26 05:33:48 2019
New Revision: 372974

URL: http://llvm.org/viewvc/llvm-project?rev=372974&view=rev
Log:
[lldb][modern-type-lookup] Fix crash when activating modern-type-lookup on Linux

There is no ClangModulesDeclVendor on Linux so that cast is triggering an 
assert.
Let's just remove it as it just casts the type to itself.

Modified:
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp?rev=372974&r1=372973&r2=372974&view=diff
==
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp 
(original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp Thu Sep 
26 05:33:48 2019
@@ -108,8 +108,7 @@ void ClangASTSource::InstallASTContext(c
 } while (false);
 
 do {
-  auto *modules_decl_vendor = llvm::cast(
-  m_target->GetClangModulesDeclVendor());
+  auto *modules_decl_vendor = m_target->GetClangModulesDeclVendor();
 
   if (!modules_decl_vendor)
 break;


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

[Lldb-commits] [PATCH] D68083: Simplify SBCommandReturnObject

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, jingham, clayborg.
jankratochvil added a project: LLDB.
Herald added a subscriber: JDevlieghere.

A simplification for D67589 . `m_opaque_up` 
can never be `nullptr` (unless one calls a ctor with `nullptr` or one uses 
`SetLLDBObjectPtr` with `nullptr`).
Also protected `SetLLDBObjectPtr` is not used anywhere (I haven't found it 
would ever be used).


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D68083

Files:
  lldb/include/lldb/API/SBCommandReturnObject.h
  lldb/source/API/SBCommandReturnObject.cpp

Index: lldb/source/API/SBCommandReturnObject.cpp
===
--- lldb/source/API/SBCommandReturnObject.cpp
+++ lldb/source/API/SBCommandReturnObject.cpp
@@ -35,6 +35,7 @@
 : m_opaque_up(ptr) {
   LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
   (lldb_private::CommandReturnObject *), ptr);
+  assert(ptr != nullptr);
 }
 
 SBCommandReturnObject::~SBCommandReturnObject() = default;
@@ -65,41 +66,34 @@
 SBCommandReturnObject::operator bool() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandReturnObject, operator bool);
 
-  return m_opaque_up != nullptr;
+  // This method is not useful but it needs to stay to keep SB API stable.
+  return true;
 }
 
 const char *SBCommandReturnObject::GetOutput() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetOutput);
 
-  if (m_opaque_up) {
-ConstString output(m_opaque_up->GetOutputData());
-return output.AsCString(/*value_if_empty*/ "");
-  }
-
-  return nullptr;
+  ConstString output(m_opaque_up->GetOutputData());
+  return output.AsCString(/*value_if_empty*/ "");
 }
 
 const char *SBCommandReturnObject::GetError() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetError);
 
-  if (m_opaque_up) {
-ConstString output(m_opaque_up->GetErrorData());
-return output.AsCString(/*value_if_empty*/ "");
-  }
-
-  return nullptr;
+  ConstString output(m_opaque_up->GetErrorData());
+  return output.AsCString(/*value_if_empty*/ "");
 }
 
 size_t SBCommandReturnObject::GetOutputSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetOutputSize);
 
-  return (m_opaque_up ? m_opaque_up->GetOutputData().size() : 0);
+  return m_opaque_up->GetOutputData().size();
 }
 
 size_t SBCommandReturnObject::GetErrorSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetErrorSize);
 
-  return (m_opaque_up ? m_opaque_up->GetErrorData().size() : 0);
+  return m_opaque_up->GetErrorData().size();
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
@@ -127,51 +121,47 @@
 void SBCommandReturnObject::Clear() {
   LLDB_RECORD_METHOD_NO_ARGS(void, SBCommandReturnObject, Clear);
 
-  if (m_opaque_up)
-m_opaque_up->Clear();
+  m_opaque_up->Clear();
 }
 
 lldb::ReturnStatus SBCommandReturnObject::GetStatus() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::ReturnStatus, SBCommandReturnObject,
  GetStatus);
 
-  return (m_opaque_up ? m_opaque_up->GetStatus() : lldb::eReturnStatusInvalid);
+  return m_opaque_up->GetStatus();
 }
 
 void SBCommandReturnObject::SetStatus(lldb::ReturnStatus status) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetStatus,
  (lldb::ReturnStatus), status);
 
-  if (m_opaque_up)
-m_opaque_up->SetStatus(status);
+  m_opaque_up->SetStatus(status);
 }
 
 bool SBCommandReturnObject::Succeeded() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, Succeeded);
 
-  return (m_opaque_up ? m_opaque_up->Succeeded() : false);
+  return m_opaque_up->Succeeded();
 }
 
 bool SBCommandReturnObject::HasResult() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, HasResult);
 
-  return (m_opaque_up ? m_opaque_up->HasResult() : false);
+  return m_opaque_up->HasResult();
 }
 
 void SBCommandReturnObject::AppendMessage(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendMessage, (const char *),
  message);
 
-  if (m_opaque_up)
-m_opaque_up->AppendMessage(message);
+  m_opaque_up->AppendMessage(message);
 }
 
 void SBCommandReturnObject::AppendWarning(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendWarning, (const char *),
  message);
 
-  if (m_opaque_up)
-m_opaque_up->AppendWarning(message);
+  m_opaque_up->AppendWarning(message);
 }
 
 CommandReturnObject *SBCommandReturnObject::operator->() const {
@@ -192,36 +182,28 @@
   return *(m_opaque_up.get());
 }
 
-void SBCommandReturnObject::SetLLDBObjectPtr(CommandReturnObject *ptr) {
-  if (m_opaque_up)
-m_opaque_up.reset(ptr);
-}
-
 bool SBCommandReturnObject::GetDescription(SBStream &description) {
   LLDB_RECORD_METHOD(bool, SBCommandReturnObject, GetDescription,
  (lldb::SBStream &), description);
 
   Stream &strm = description.ref();
 
-  if (m_opaqu

[Lldb-commits] [PATCH] D68083: Simplify SBCommandReturnObject

[Pavel Labath via Phabricator via lldb-commits]

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

cool


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68083



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

[Lldb-commits] [lldb] r372976 - [lldb] Code cleanup: Simplify SBCommandReturnObject

[Jan Kratochvil via lldb-commits]

Author: jankratochvil
Date: Thu Sep 26 06:31:59 2019
New Revision: 372976

URL: http://llvm.org/viewvc/llvm-project?rev=372976&view=rev
Log:
[lldb] Code cleanup: Simplify SBCommandReturnObject

A simplification for D67589. m_opaque_up can never be nullptr (unless one calls
a ctor with nullptr or one uses SetLLDBObjectPtr with nullptr).

Also protected SetLLDBObjectPtr is not used anywhere (I haven't found it would
ever be used).

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

Modified:
lldb/trunk/include/lldb/API/SBCommandReturnObject.h
lldb/trunk/source/API/SBCommandReturnObject.cpp

Modified: lldb/trunk/include/lldb/API/SBCommandReturnObject.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/API/SBCommandReturnObject.h?rev=372976&r1=372975&r2=372976&view=diff
==
--- lldb/trunk/include/lldb/API/SBCommandReturnObject.h (original)
+++ lldb/trunk/include/lldb/API/SBCommandReturnObject.h Thu Sep 26 06:31:59 2019
@@ -98,8 +98,6 @@ protected:
 
   lldb_private::CommandReturnObject &ref() const;
 
-  void SetLLDBObjectPtr(lldb_private::CommandReturnObject *ptr);
-
 private:
   std::unique_ptr m_opaque_up;
 };

Modified: lldb/trunk/source/API/SBCommandReturnObject.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SBCommandReturnObject.cpp?rev=372976&r1=372975&r2=372976&view=diff
==
--- lldb/trunk/source/API/SBCommandReturnObject.cpp (original)
+++ lldb/trunk/source/API/SBCommandReturnObject.cpp Thu Sep 26 06:31:59 2019
@@ -35,6 +35,7 @@ SBCommandReturnObject::SBCommandReturnOb
 : m_opaque_up(ptr) {
   LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
   (lldb_private::CommandReturnObject *), ptr);
+  assert(ptr != nullptr);
 }
 
 SBCommandReturnObject::~SBCommandReturnObject() = default;
@@ -65,41 +66,34 @@ bool SBCommandReturnObject::IsValid() co
 SBCommandReturnObject::operator bool() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandReturnObject, operator bool);
 
-  return m_opaque_up != nullptr;
+  // This method is not useful but it needs to stay to keep SB API stable.
+  return true;
 }
 
 const char *SBCommandReturnObject::GetOutput() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetOutput);
 
-  if (m_opaque_up) {
-ConstString output(m_opaque_up->GetOutputData());
-return output.AsCString(/*value_if_empty*/ "");
-  }
-
-  return nullptr;
+  ConstString output(m_opaque_up->GetOutputData());
+  return output.AsCString(/*value_if_empty*/ "");
 }
 
 const char *SBCommandReturnObject::GetError() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetError);
 
-  if (m_opaque_up) {
-ConstString output(m_opaque_up->GetErrorData());
-return output.AsCString(/*value_if_empty*/ "");
-  }
-
-  return nullptr;
+  ConstString output(m_opaque_up->GetErrorData());
+  return output.AsCString(/*value_if_empty*/ "");
 }
 
 size_t SBCommandReturnObject::GetOutputSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetOutputSize);
 
-  return (m_opaque_up ? m_opaque_up->GetOutputData().size() : 0);
+  return m_opaque_up->GetOutputData().size();
 }
 
 size_t SBCommandReturnObject::GetErrorSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetErrorSize);
 
-  return (m_opaque_up ? m_opaque_up->GetErrorData().size() : 0);
+  return m_opaque_up->GetErrorData().size();
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
@@ -127,51 +121,47 @@ size_t SBCommandReturnObject::PutError(F
 void SBCommandReturnObject::Clear() {
   LLDB_RECORD_METHOD_NO_ARGS(void, SBCommandReturnObject, Clear);
 
-  if (m_opaque_up)
-m_opaque_up->Clear();
+  m_opaque_up->Clear();
 }
 
 lldb::ReturnStatus SBCommandReturnObject::GetStatus() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::ReturnStatus, SBCommandReturnObject,
  GetStatus);
 
-  return (m_opaque_up ? m_opaque_up->GetStatus() : lldb::eReturnStatusInvalid);
+  return m_opaque_up->GetStatus();
 }
 
 void SBCommandReturnObject::SetStatus(lldb::ReturnStatus status) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetStatus,
  (lldb::ReturnStatus), status);
 
-  if (m_opaque_up)
-m_opaque_up->SetStatus(status);
+  m_opaque_up->SetStatus(status);
 }
 
 bool SBCommandReturnObject::Succeeded() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, Succeeded);
 
-  return (m_opaque_up ? m_opaque_up->Succeeded() : false);
+  return m_opaque_up->Succeeded();
 }
 
 bool SBCommandReturnObject::HasResult() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, HasResult);
 
-  return (m_opaque_up ? m_opaque_up->HasResult() : false);
+  return m_opaque_up->HasResult();
 }
 
 void SBCommandReturnObject::AppendMessage(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendMessage, (const char 
*

[Lldb-commits] [PATCH] D68083: Simplify SBCommandReturnObject

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372976: [lldb] Code cleanup: Simplify SBCommandReturnObject 
(authored by jankratochvil, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D68083?vs=221930&id=221934#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D68083

Files:
  lldb/trunk/include/lldb/API/SBCommandReturnObject.h
  lldb/trunk/source/API/SBCommandReturnObject.cpp

Index: lldb/trunk/source/API/SBCommandReturnObject.cpp
===
--- lldb/trunk/source/API/SBCommandReturnObject.cpp
+++ lldb/trunk/source/API/SBCommandReturnObject.cpp
@@ -35,6 +35,7 @@
 : m_opaque_up(ptr) {
   LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
   (lldb_private::CommandReturnObject *), ptr);
+  assert(ptr != nullptr);
 }
 
 SBCommandReturnObject::~SBCommandReturnObject() = default;
@@ -65,41 +66,34 @@
 SBCommandReturnObject::operator bool() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandReturnObject, operator bool);
 
-  return m_opaque_up != nullptr;
+  // This method is not useful but it needs to stay to keep SB API stable.
+  return true;
 }
 
 const char *SBCommandReturnObject::GetOutput() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetOutput);
 
-  if (m_opaque_up) {
-ConstString output(m_opaque_up->GetOutputData());
-return output.AsCString(/*value_if_empty*/ "");
-  }
-
-  return nullptr;
+  ConstString output(m_opaque_up->GetOutputData());
+  return output.AsCString(/*value_if_empty*/ "");
 }
 
 const char *SBCommandReturnObject::GetError() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetError);
 
-  if (m_opaque_up) {
-ConstString output(m_opaque_up->GetErrorData());
-return output.AsCString(/*value_if_empty*/ "");
-  }
-
-  return nullptr;
+  ConstString output(m_opaque_up->GetErrorData());
+  return output.AsCString(/*value_if_empty*/ "");
 }
 
 size_t SBCommandReturnObject::GetOutputSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetOutputSize);
 
-  return (m_opaque_up ? m_opaque_up->GetOutputData().size() : 0);
+  return m_opaque_up->GetOutputData().size();
 }
 
 size_t SBCommandReturnObject::GetErrorSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetErrorSize);
 
-  return (m_opaque_up ? m_opaque_up->GetErrorData().size() : 0);
+  return m_opaque_up->GetErrorData().size();
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
@@ -127,51 +121,47 @@
 void SBCommandReturnObject::Clear() {
   LLDB_RECORD_METHOD_NO_ARGS(void, SBCommandReturnObject, Clear);
 
-  if (m_opaque_up)
-m_opaque_up->Clear();
+  m_opaque_up->Clear();
 }
 
 lldb::ReturnStatus SBCommandReturnObject::GetStatus() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::ReturnStatus, SBCommandReturnObject,
  GetStatus);
 
-  return (m_opaque_up ? m_opaque_up->GetStatus() : lldb::eReturnStatusInvalid);
+  return m_opaque_up->GetStatus();
 }
 
 void SBCommandReturnObject::SetStatus(lldb::ReturnStatus status) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetStatus,
  (lldb::ReturnStatus), status);
 
-  if (m_opaque_up)
-m_opaque_up->SetStatus(status);
+  m_opaque_up->SetStatus(status);
 }
 
 bool SBCommandReturnObject::Succeeded() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, Succeeded);
 
-  return (m_opaque_up ? m_opaque_up->Succeeded() : false);
+  return m_opaque_up->Succeeded();
 }
 
 bool SBCommandReturnObject::HasResult() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, HasResult);
 
-  return (m_opaque_up ? m_opaque_up->HasResult() : false);
+  return m_opaque_up->HasResult();
 }
 
 void SBCommandReturnObject::AppendMessage(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendMessage, (const char *),
  message);
 
-  if (m_opaque_up)
-m_opaque_up->AppendMessage(message);
+  m_opaque_up->AppendMessage(message);
 }
 
 void SBCommandReturnObject::AppendWarning(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendWarning, (const char *),
  message);
 
-  if (m_opaque_up)
-m_opaque_up->AppendWarning(message);
+  m_opaque_up->AppendWarning(message);
 }
 
 CommandReturnObject *SBCommandReturnObject::operator->() const {
@@ -192,36 +182,28 @@
   return *(m_opaque_up.get());
 }
 
-void SBCommandReturnObject::SetLLDBObjectPtr(CommandReturnObject *ptr) {
-  if (m_opaque_up)
-m_opaque_up.reset(ptr);
-}
-
 bool SBCommandReturnObject::GetDescription(SBStream &description) {
   LLDB_RECORD_METHOD(bool, SBCommandReturnObject, GetDescription,
  (lldb::SBStream &), description);
 
   Stream &strm = description.ref();
 
-  if (m_opaque_up) {
-des

[Lldb-commits] [PATCH] D67965: Have ABI plugins vend llvm MCRegisterInfo data

[Sylvestre Ledru via Phabricator via lldb-commits]

sylvestre.ledru added a comment.

@labath 
I am building with AVR as experimental target and this change probably broke 
the build.

  In file included from 
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:208:
  
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/build-llvm/tools/clang/stage2-bins/include/llvm/Config/Targets.def:42:1:
 error: reference to non-static member function must be called; did you mean to 
call it with no arguments?
  LLVM_TARGET(AVR)
  ^~~~
  
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:207:43:
 note: expanded from macro 'LLVM_TARGET'
  #define LLVM_TARGET(t) LLDB_PROCESS_ ## t(Initialize)
^~
  In file included from 
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:208:
  
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/build-llvm/tools/clang/stage2-bins/include/llvm/Config/Targets.def:42:1:
 error: use of undeclared identifier 'LLDB_PROCESS_AVR'
  
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:207:24:
 note: expanded from macro 'LLVM_TARGET'
  #define LLVM_TARGET(t) LLDB_PROCESS_ ## t(Initialize)
 ^

Could you please have a look?
thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67965



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

[Lldb-commits] [PATCH] D68088: Set eRegisterKindEHFrame register numbers for 32 bit ARM register contexts in minidumps

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, aadsm, dvlahovski.
Herald added a subscriber: kristof.beyls.

Stack unwinding was sometimes failing when trying to unwind stacks in 32 bit 
ARM. I discovered this was because the EH frame register numbers were not set. 
This patch fixes this issue and adds a unit test to verify this doesn't regress.


https://reviews.llvm.org/D68088

Files:
  lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
  lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h
  lldb/unittests/Process/minidump/RegisterContextMinidumpTest.cpp

Index: lldb/unittests/Process/minidump/RegisterContextMinidumpTest.cpp
===
--- lldb/unittests/Process/minidump/RegisterContextMinidumpTest.cpp
+++ lldb/unittests/Process/minidump/RegisterContextMinidumpTest.cpp
@@ -10,7 +10,9 @@
 #include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
 #include "Plugins/Process/minidump/RegisterContextMinidump_x86_32.h"
 #include "Plugins/Process/minidump/RegisterContextMinidump_x86_64.h"
+#include "Plugins/Process/minidump/RegisterContextMinidump_ARM.h"
 #include "lldb/Utility/DataBuffer.h"
+#include "llvm/ADT/StringRef.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
@@ -143,3 +145,56 @@
   EXPECT_EQ(Context.ds, reg64(*Buf, Info[lldb_ds_x86_64]));
   EXPECT_EQ(Context.es, reg64(*Buf, Info[lldb_es_x86_64]));
 }
+
+static void TestARMRegInfo(const lldb_private::RegisterInfo *info) {
+  // Make sure we have valid register numbers for eRegisterKindEHFrame and
+  // eRegisterKindDWARF for GPR registers r0-r15 so that we can unwind
+  // correctly when using this information.
+  llvm::StringRef name(info->name);
+  llvm::StringRef alt_name(info->alt_name);
+  if (name.startswith("r") || alt_name.startswith("r")) {
+EXPECT_NE(info->kinds[lldb::eRegisterKindEHFrame], LLDB_INVALID_REGNUM);
+EXPECT_NE(info->kinds[lldb::eRegisterKindDWARF], LLDB_INVALID_REGNUM);
+  }
+  // Verify generic register are set correctly
+  if (name == "r0")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_ARG1);
+  else if (name == "r1")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_ARG2);
+  else if (name == "r2")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_ARG3);
+  else if (name == "r3")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_ARG4);
+  else if (name == "sp")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_SP);
+  else if (name == "fp")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_FP);
+  else if (name == "lr")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_RA);
+  else if (name == "pc")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_PC);
+  else if (name == "cpsr")
+EXPECT_EQ(info->kinds[lldb::eRegisterKindGeneric],
+  (uint32_t)LLDB_REGNUM_GENERIC_FLAGS);
+}
+
+TEST(RegisterContextMinidump, CheckRegisterContextMinidump_ARM) {
+  size_t num_regs = RegisterContextMinidump_ARM::GetRegisterCountStatic();
+  const lldb_private::RegisterInfo *reg_info;
+  for (size_t reg=0; reg___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 221968.
kwk added a comment.

- Adjust other occurrence of AddSymbol where ELF plays a role
- Working tests
- Use unordered_set for storing unique elf symbols


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -12,6 +12,8 @@
 #include 
 
 #include 
+#include 
+#include 
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
@@ -55,7 +57,7 @@
 /// This class provides a generic ELF (32/64 bit) reader plugin implementing
 /// the ObjectFile protocol.
 class ObjectFileELF : public lldb_private::ObjectFile {
-public:
+public:  
   // Static Functions
   static void Initialize();
 
@@ -184,6 +186,8 @@
 
   typedef std::map
   FileAddressToAddressClassMap;
+  
+  typedef std::unordered_set UniqueElfSymbolColl;
 
   /// Version of this reader common to all plugins based on this class.
   static const uint32_t m_plugin_version = 1;
@@ -278,7 +282,8 @@
   /// will parse the symbols only once.  Returns the number of symbols parsed.
   unsigned ParseSymbolTable(lldb_private::Symtab *symbol_table,
 lldb::user_id_t start_id,
-lldb_private::Section *symtab);
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Helper routine for ParseSymbolTable().
   unsigned ParseSymbols(lldb_private::Symtab *symbol_table,
@@ -286,7 +291,8 @@
 lldb_private::SectionList *section_list,
 const size_t num_symbols,
 const lldb_private::DataExtractor &symtab_data,
-const lldb_private::DataExtractor &strtab_data);
+const lldb_private::DataExtractor &strtab_data,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Scans the relocation entries and adds a set of artificial symbols to the
   /// given symbol table for each PLT slot.  Returns the number of symbols
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -39,6 +39,7 @@
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/MipsABIFlags.h"
+#include "lldb/Utility/StreamString.h"
 
 #define CASE_AND_STREAM(s, def, width) \
   case def:\
@@ -1871,7 +1872,8 @@
  SectionList *section_list,
  const size_t num_symbols,
  const DataExtractor &symtab_data,
- const DataExtractor &strtab_data) {
+ const DataExtractor &strtab_data,
+ UniqueElfSymbolColl &unique_elf_symbols) {
   ELFSymbol symbol;
   lldb::offset_t offset = 0;
 
@@ -2196,20 +2198,28 @@
 symbol_size_valid,  // Symbol size is valid
 has_suffix, // Contains linker annotations?
 flags); // Symbol flags.
-symtab->AddSymbol(dc_symbol);
+
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);
+}
   }
   return i;
 }
 
-unsigned ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
- user_id_t start_id,
- lldb_private::Section *symtab) {
+unsigned
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id,
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols) {
   if (symtab->GetObjectFile() != this) {
 // If the symbol table section is owned by a different object file, have it
 // do the parsing.
 ObjectFileELF *obj_fil

[Lldb-commits] [PATCH] D66451: [ClangExpressionParser] Add ClangDeclVendor

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.
Herald added a subscriber: usaxena95.



Comment at: 
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:110
 do {
-  DeclVendor *modules_decl_vendor =
-  m_target->GetClangModulesDeclVendor();
+  auto *modules_decl_vendor = llvm::cast(
+  m_target->GetClangModulesDeclVendor());

This part of the change was recently reverted, see: 
http://llvm.org/viewvc/llvm-project?view=revision&revision=372974

I am wondering if the change right above for `runtime_decl_vendor` is also 
problematic as well. Can you explain the rationale?

CC @teemperor 




Repository:
  rL LLVM

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

https://reviews.llvm.org/D66451



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

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So overall approach is good. See inline comments for issue and questions.




Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373
+  r.st_name = st_name;
+  return elf::ELFSymbol::operator==(r) &&
+ st_name_string == rhs.st_name_string;

I would almost just manually compare only the things we care about here. Again, 
what about st_shndx when comparing a symbol from the main symbol table and one 
from the .gnu_debugdata symbol table. Are those section indexes guaranteed to 
match? I would think not. 



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:289
+struct NamedELFSymbol : ELFSymbol {
+  lldb_private::ConstString st_name_string; ///< Actual name of the ELF symbol
+

Do we need a ConstString for the st_shndx as well so we can correctly compare a 
section from one file to a section from another as in the .gnu_debugdata case?



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:427
+std::size_t h2 = std::hash()(s.st_size);
+std::size_t h3 = std::hash()(s.st_name);
+std::size_t h4 = std::hash()(s.st_info);

An ELF symbol from one symbol table can have the same name as another yet with 
a different st_name string table offset. Do we even want to be able to hash an 
ELFSymbol on its own? Maybe remove this enture function and only hash 
NamedELFSymbol?



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:430
+std::size_t h5 = std::hash()(s.st_other);
+std::size_t h6 = std::hash()(s.st_shndx);
+return llvm::hash_combine(h1, h2, h3, h4, h5, h6);

I know the section index will match between for symbol tables in the same ELF 
file, but what about a symbol table in an external file like .gnu_debugdata?



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:440
+tmp.st_name = 0;
+std::size_t h1 = std::hash()(tmp);
+std::size_t h2 = std::hash()(s.st_name_string.AsCString());

Don't we need to hash everything we care about except the st_name? Those 
indexes can differ if they come from a different string table? Shouldn't this 
be:

```
std::size_t h1 = std::hash()(s.st_value);
std::size_t h2 = std::hash()(s.st_size);
// Skip std::size_t h3 = std::hash()(s.st_name);
std::size_t h4 = std::hash()(s.st_info);
std::size_t h5 = std::hash()(s.st_other);
std::size_t h6 = std::hash()(s.st_shndx);
std::size_t h7 = std::hash()(s.st_name_string.AsCString());
return llvm::hash_combine(h1, h2, h4, h5, h6, h7);
```
I left the "h" variables with the same names to show we don't want "h3".



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2202-2206
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);

Do we even need NamedELFSymbol? Can we just make an unordered_set of 
lldb_private::Symbol values?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390



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

[Lldb-commits] [PATCH] D67966: Use llvm for dumping DWARF expressions

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

nice!


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

https://reviews.llvm.org/D67966



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

[Lldb-commits] [lldb] r372998 - SystemInitializer: Define macros for experimental targets too

[Pavel Labath via lldb-commits]

Author: labath
Date: Thu Sep 26 10:15:18 2019
New Revision: 372998

URL: http://llvm.org/viewvc/llvm-project?rev=372998&view=rev
Log:
SystemInitializer: Define macros for experimental targets too

Modified:
lldb/trunk/source/API/SystemInitializerFull.cpp
lldb/trunk/tools/lldb-test/SystemInitializerTest.cpp

Modified: lldb/trunk/source/API/SystemInitializerFull.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SystemInitializerFull.cpp?rev=372998&r1=372997&r2=372998&view=diff
==
--- lldb/trunk/source/API/SystemInitializerFull.cpp (original)
+++ lldb/trunk/source/API/SystemInitializerFull.cpp Thu Sep 26 10:15:18 2019
@@ -152,6 +152,8 @@ SystemInitializerFull::~SystemInitialize
   ABIWindows_x86_64::op();
 
 #define LLDB_PROCESS_AMDGPU(op)
+#define LLDB_PROCESS_ARC(op)
+#define LLDB_PROCESS_AVR(op)
 #define LLDB_PROCESS_BPF(op)
 #define LLDB_PROCESS_Lanai(op)
 #define LLDB_PROCESS_MSP430(op)

Modified: lldb/trunk/tools/lldb-test/SystemInitializerTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-test/SystemInitializerTest.cpp?rev=372998&r1=372997&r2=372998&view=diff
==
--- lldb/trunk/tools/lldb-test/SystemInitializerTest.cpp (original)
+++ lldb/trunk/tools/lldb-test/SystemInitializerTest.cpp Thu Sep 26 10:15:18 
2019
@@ -133,6 +133,8 @@ SystemInitializerTest::~SystemInitialize
   ABIWindows_x86_64::op();
 
 #define LLDB_PROCESS_AMDGPU(op)
+#define LLDB_PROCESS_ARC(op)
+#define LLDB_PROCESS_AVR(op)
 #define LLDB_PROCESS_BPF(op)
 #define LLDB_PROCESS_Lanai(op)
 #define LLDB_PROCESS_MSP430(op)


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

[Lldb-commits] [PATCH] D67965: Have ABI plugins vend llvm MCRegisterInfo data

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D67965#1684297 , @sylvestre.ledru 
wrote:

> @labath 
>  I am building with AVR as experimental target and this change probably broke 
> the build.
>
> Could you please have a look?
> thanks


Thanks for the heads up. r372998 ought to fix that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67965



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

[Lldb-commits] [PATCH] D67996: Convert FileSystem::Open() to return Expected

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added a comment.

done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67996



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

[Lldb-commits] [PATCH] D67996: Convert FileSystem::Open() to return Expected

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 221985.
lawrence_danna marked 3 inline comments as done.
lawrence_danna added a comment.

fixed the remaining comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67996

Files:
  lldb/include/lldb/Core/StreamFile.h
  lldb/include/lldb/Host/FileCache.h
  lldb/include/lldb/Host/FileSystem.h
  lldb/include/lldb/lldb-forward.h
  lldb/scripts/Python/python-typemaps.swig
  lldb/source/API/SBStream.cpp
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Core/StreamFile.cpp
  lldb/source/Expression/REPL.cpp
  lldb/source/Host/common/FileCache.cpp
  lldb/source/Host/common/FileSystem.cpp
  lldb/source/Host/windows/Host.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  
lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Target/ModuleCache.cpp
  lldb/source/Target/Platform.cpp
  lldb/unittests/Host/FileSystemTest.cpp
  lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/lldb-enumerations.h"
+#include "llvm/Testing/Support/Error.h"
 
 #include "PythonTestSuite.h"
 
@@ -581,10 +582,10 @@
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonFile) {
-  File file;
-  FileSystem::Instance().Open(file, FileSpec(FileSystem::DEV_NULL),
-  File::eOpenOptionRead);
-  PythonFile py_file(file, "r");
+  auto file = FileSystem::Instance().Open(FileSpec(FileSystem::DEV_NULL),
+  File::eOpenOptionRead);
+  ASSERT_THAT_EXPECTED(file, llvm::Succeeded());
+  PythonFile py_file(*file.get(), "r");
   EXPECT_TRUE(PythonFile::Check(py_file.get()));
 }
 
Index: lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
@@ -6,6 +6,7 @@
   LINK_LIBS
 lldbHost
 lldbPluginScriptInterpreterPython
+LLVMTestingSupport
   LINK_COMPONENTS
 Support
   )
\ No newline at end of file
Index: lldb/unittests/Host/FileSystemTest.cpp
===
--- lldb/unittests/Host/FileSystemTest.cpp
+++ lldb/unittests/Host/FileSystemTest.cpp
@@ -288,3 +288,18 @@
   EXPECT_THAT(visited,
   testing::UnorderedElementsAre("/foo", "/bar", "/baz", "/qux"));
 }
+
+TEST(FileSystemTest, OpenErrno) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;
+  auto file = fs.Open(spec, File::eOpenOptionRead, 0, true);
+  ASSERT_FALSE(file);
+  std::error_code code = errorToErrorCode(file.takeError());
+  EXPECT_EQ(code.category(), std::system_category());
+  EXPECT_EQ(code.value(), ENOENT);
+}
+
Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1226,15 +1226,15 @@
   if (fs::is_symlink_file(source.GetPath()))
 source_open_options |= File::eOpenOptionDontFollowSymlinks;
 
-  File source_file;
-  Status error = FileSystem::Instance().Open(
-  source_file, source, source_open_options, lldb::eFilePermissionsUserRW);
-  uint32_t permissions = source_file.GetPermissions(error);
+  auto source_file = FileSystem::Instance().Open(
+  source, source_open_options, lldb::eFilePermissionsUserRW);
+  if (!source_file)
+return Status(source_file.takeError());
+  Status error;
+  uint32_t permissions = source_file.get()->GetPermissions(error);
   if (permissions == 0)
 permissions = lldb::eFilePermissionsFileDefault;
 
-  if (!source_file.IsValid())
-return Status("PutFile: unable to open source file");
   lldb::user_id_t dest_file = OpenFile(
   destination, File::eOpenOptionCanCreate | File::eOpenOptionWrite |
File::eOpenOptionTruncate | File::eOpenOpt

[Lldb-commits] [PATCH] D67996: Convert FileSystem::Open() to return Expected

[Lawrence D'Anna via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373003: Convert FileSystem::Open() to return 
Expected (authored by lawrence_danna, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67996?vs=221985&id=221988#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67996

Files:
  lldb/trunk/include/lldb/Core/StreamFile.h
  lldb/trunk/include/lldb/Host/FileCache.h
  lldb/trunk/include/lldb/Host/FileSystem.h
  lldb/trunk/include/lldb/lldb-forward.h
  lldb/trunk/scripts/Python/python-typemaps.swig
  lldb/trunk/source/API/SBStream.cpp
  lldb/trunk/source/Commands/CommandObjectMemory.cpp
  lldb/trunk/source/Core/StreamFile.cpp
  lldb/trunk/source/Expression/REPL.cpp
  lldb/trunk/source/Host/common/FileCache.cpp
  lldb/trunk/source/Host/common/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Interpreter/CommandInterpreter.cpp
  
lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  
lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/trunk/source/Target/ModuleCache.cpp
  lldb/trunk/source/Target/Platform.cpp
  lldb/trunk/unittests/Host/FileSystemTest.cpp
  lldb/trunk/unittests/ScriptInterpreter/Python/CMakeLists.txt
  lldb/trunk/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/trunk/unittests/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/trunk/unittests/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/trunk/unittests/ScriptInterpreter/Python/CMakeLists.txt
@@ -6,6 +6,7 @@
   LINK_LIBS
 lldbHost
 lldbPluginScriptInterpreterPython
+LLVMTestingSupport
   LINK_COMPONENTS
 Support
   )
\ No newline at end of file
Index: lldb/trunk/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/trunk/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/trunk/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/lldb-enumerations.h"
+#include "llvm/Testing/Support/Error.h"
 
 #include "PythonTestSuite.h"
 
@@ -581,10 +582,10 @@
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonFile) {
-  File file;
-  FileSystem::Instance().Open(file, FileSpec(FileSystem::DEV_NULL),
-  File::eOpenOptionRead);
-  PythonFile py_file(file, "r");
+  auto file = FileSystem::Instance().Open(FileSpec(FileSystem::DEV_NULL),
+  File::eOpenOptionRead);
+  ASSERT_THAT_EXPECTED(file, llvm::Succeeded());
+  PythonFile py_file(*file.get(), "r");
   EXPECT_TRUE(PythonFile::Check(py_file.get()));
 }
 
Index: lldb/trunk/unittests/Host/FileSystemTest.cpp
===
--- lldb/trunk/unittests/Host/FileSystemTest.cpp
+++ lldb/trunk/unittests/Host/FileSystemTest.cpp
@@ -288,3 +288,18 @@
   EXPECT_THAT(visited,
   testing::UnorderedElementsAre("/foo", "/bar", "/baz", "/qux"));
 }
+
+TEST(FileSystemTest, OpenErrno) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;
+  auto file = fs.Open(spec, File::eOpenOptionRead, 0, true);
+  ASSERT_FALSE(file);
+  std::error_code code = errorToErrorCode(file.takeError());
+  EXPECT_EQ(code.category(), std::system_category());
+  EXPECT_EQ(code.value(), ENOENT);
+}
+
Index: lldb/trunk/source/Host/windows/Host.cpp
===
--- lldb/trunk/source/Host/windows/Host.cpp
+++ lldb/trunk/source/Host/windows/Host.cpp
@@ -34,9 +34,11 @@
 bool GetTripleForProcess(const FileSpec &executable, llvm::Triple &triple) {
   // Open the PE File as a binary file, and parse just enough information to
   // determine the machine type.
-  File imageBinary;
-  FileSystem::Instance().Open(imageBinary, executable, File::eOpenOptionRead,
-  lldb::eFilePermissionsUserRead);
+  auto imageBinaryP = FileSystem::Instance().Open(
+  executable, File::eOpenOptionRead, lldb::eFilePermissionsUserRead);
+  if (!imageBinaryP)
+return llvm::errorToBool(image

[Lldb-commits] [PATCH] D66451: [ClangExpressionParser] Add ClangDeclVendor

[Alex Langford via Phabricator via lldb-commits]

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



Comment at: 
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:110
 do {
-  DeclVendor *modules_decl_vendor =
-  m_target->GetClangModulesDeclVendor();
+  auto *modules_decl_vendor = llvm::cast(
+  m_target->GetClangModulesDeclVendor());

shafik wrote:
> This part of the change was recently reverted, see: 
> http://llvm.org/viewvc/llvm-project?view=revision&revision=372974
> 
> I am wondering if the change right above for `runtime_decl_vendor` is also 
> problematic as well. Can you explain the rationale?
> 
> CC @teemperor 
> 
> 
This is actually a bug. I intended to cast it to a `ClangDeclVendor`. The 
instance above for `runtime_decl_vendor` shouldn't be problematic. The 
rationale behind this change was to remove the clang-specific code in the 
DeclVendor class by introducing a ClangDeclVendor class to sit in between 
DeclVendor and its clang-specific subclasses (ClangModulesDeclVendor and 
AppleObjCDeclVendor). That partial revert is probably fine honestly since it's 
using ClangModulesDeclVendor directly.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66451



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

[Lldb-commits] [PATCH] D68096: Add Linux signal support to ProcessMinidump

[Joseph Tremoulet via Phabricator via lldb-commits]

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

Set the UnixSignals object when isOSLinux is true, and force a SIGSTOP
if there's no other signal when loading a core dump (the same as
ProcessElfCore::DoLoadCore does) since the loading process reports a stop.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68096

Files:
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.h

Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.h
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -109,6 +109,8 @@
   lldb::DataBufferSP m_core_data;
   llvm::ArrayRef m_thread_list;
   const MinidumpExceptionStream *m_active_exception;
+  uint32_t m_synthetic_exception_signal;
+  lldb::tid_t m_synthetic_exception_thread_id;
   lldb::CommandObjectSP m_command_sp;
   bool m_is_wow64;
 };
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -163,7 +163,8 @@
  const FileSpec &core_file,
  DataBufferSP core_data)
 : Process(target_sp, listener_sp), m_core_file(core_file),
-  m_core_data(std::move(core_data)), m_is_wow64(false) {}
+  m_core_data(std::move(core_data)),
+  m_synthetic_exception_signal(0), m_is_wow64(false) {}
 
 ProcessMinidump::~ProcessMinidump() {
   Clear();
@@ -215,6 +216,25 @@
 
   m_thread_list = m_minidump_parser->GetThreads();
   m_active_exception = m_minidump_parser->GetExceptionStream();
+
+  if (arch.GetTriple().isOSLinux()) {
+
+SetUnixSignals(UnixSignals::Create(GetArchitecture()));
+
+if (!m_thread_list.empty() &&
+(!m_active_exception ||
+ !m_active_exception->exception_record.exception_code)) {
+  // No active signal, but we're going to process a stop event
+  // upon loading the core, so force a SIGSTOP on the first
+  // thread.
+  m_synthetic_exception_signal =
+GetUnixSignals()->GetSignalNumberFromName("SIGSTOP");
+
+  m_synthetic_exception_thread_id =
+m_thread_list.front().ThreadId;
+}
+  }
+
   ReadModuleList();
 
   llvm::Optional pid = m_minidump_parser->GetPid();
@@ -234,24 +254,40 @@
 Status ProcessMinidump::DoDestroy() { return Status(); }
 
 void ProcessMinidump::RefreshStateAfterStop() {
-  if (!m_active_exception)
-return;
 
-  if (m_active_exception->exception_record.exception_code ==
-  MinidumpException::DumpRequested) {
-return;
+  lldb::tid_t exception_thread_id;
+
+  if (m_synthetic_exception_signal) {
+exception_thread_id = m_synthetic_exception_thread_id;
+  } else {
+if (!m_active_exception)
+  return;
+
+if (m_active_exception->exception_record.exception_code ==
+MinidumpException::DumpRequested) {
+  return;
+}
+
+exception_thread_id = m_active_exception->thread_id;
   }
 
   lldb::StopInfoSP stop_info;
   lldb::ThreadSP stop_thread;
 
-  Process::m_thread_list.SetSelectedThreadByID(m_active_exception->thread_id);
+  Process::m_thread_list.SetSelectedThreadByID(exception_thread_id);
   stop_thread = Process::m_thread_list.GetSelectedThread();
   ArchSpec arch = GetArchitecture();
 
   if (arch.GetTriple().getOS() == llvm::Triple::Linux) {
+uint32_t signo;
+if (m_synthetic_exception_signal) {
+  signo = m_synthetic_exception_signal;
+  m_synthetic_exception_signal = 0;
+} else {
+  signo = m_active_exception->exception_record.exception_code;
+}
 stop_info = StopInfo::CreateStopReasonWithSignal(
-*stop_thread, m_active_exception->exception_record.exception_code);
+*stop_thread, signo);
   } else if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
 stop_info = StopInfoMachException::CreateStopReasonWithMachException(
 *stop_thread, m_active_exception->exception_record.exception_code, 2,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68096: Add Linux signal support to ProcessMinidump

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

I'm trying to fix an issue where opening a minidump created by breakpad in lldb 
just hangs, but if I use breakpad's minidump-2-core on that same dump then 
opening the core dump works fine.  From debugging the two cases I can see that 
the critical logic that ProcessElfCore has which ProcessMinidump lacks is here 
,
 and the code in this patch is my attempt to replicate that.

I'd love some help on how to create an appropriate test for this.  From looking 
at existing tests, I gather it would make sense to add a test to 
lldb/lit/Minidump or 
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new (aside:  
what's the difference?  is one preferred?) that has a yaml-ized minidump, the 
test would try to open the dump and make sure it didn't hang.  I'm running into 
issues when I try to create that test, and I've put details in a gist 
.  
There's a program 

 there which launches another program 

 and then uses breakpad to capture a minidump of the child process (another 
aside:  it looks like the existing tests all dump their own process, I think 
that dumping another process is the reason I'm running into this but others 
haven't).  If I load that minidump without the fixes in this patch, lldb hangs. 
 If I load that minidump with the fixes in this patch, lldb reads it and can 
show an accurate backtrace, I've put a log 

 of that in the gist.  But if I then run that minidump through obj-2-yaml and 
yaml-2-obj, if I load the resulting minidump, while it doesn't hang, it can't 
show an accurate backtrace.  I've also put up a log 

 of that.

I'd imagine my next step would be to look into what information is getting 
dropped when I try to do the round-trip through obj-2-yaml and yaml-2-obj, then 
update those utilities to preserve more, so that I can come back and add this 
test with a yaml input... does that sound right, or have I gone off track 
somewhere?

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68096



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

[Lldb-commits] [PATCH] D68048: [WIP][RFC] Improve fetching the process list on the android platform

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

We were discussing things over here and were wondering if we even need the 
lldb-server in order to connect to the platform, or if we should just implement 
everything through command line commands like "adb", or link against a ADB 
shared library (is there one?), or another way? There are complexities that 
make the lldb-server in platform mode less useful like needing to run 
lldb-platform as the user for the application in order to get things working.

For example, if we just want to connect to a device before we have an app we 
want to debug, like if we wanted to just attach to a process, we currently do:

  (lldb) platform select remote-android
  (lldb) platform connect tcp://1234

And the user must launch lldb-server in platform mode themselves, which is not 
optimal. A better experience might be to do:

  (lldb) platform select remote-android
  (lldb) platform connect

with no args to "platform connect" and LLDB will auto connect to a single 
device if only one device is available. If multiple devices are available we 
would need to name the device like:

  (lldb) platform connect device123

where "device123" is the name or could be the serial number or what ever 
uniquely identifies a device.

If we use lldb-platform in server mode, what user do we launch it as if we have 
no app? If we use the "shell" user? If so, is the platform connection useful 
still once we need to upload things into an application's data directory? Or 
would we need to launch another lldb-server in platform mode that runs as a 
user? Do we currently need to run lldb-server in platform mode as the user if 
we want to launch a debug session?

Seems like it would be cleaner to avoid using lldb-server and just have 
PlatformAndroid use any command line tools ("adb", etc) to run each command and 
each command can pick the user that would be required. With this patch in the 
current form we are kind of going the route of adding to the lldb-server, but 
is that the right decision? I want to avoid having multiple lldb-server 
binaries being run as multiple different users if we can avoid it. Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68048



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

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 222021.
jankratochvil edited the summary of this revision.
jankratochvil added a comment.

Changed @clayborg's `lldb_private::CommandReturnObjectImpl` to 
`lldb_private::SBCommandReturnObjectImpl` - both variants are used in LLDB 
`SB*.cpp` and the `SB*` looks more logical to me.
Used @clayborg's `lldb_private::SBCommandReturnObjectImpl::m_owned` with 
conditional in dtor but personally I would use two inherited classes + virtual 
dtor instead.

In D67589#1676511 , @labath wrote:

> I think I would like that better than the swap trick. Since you're now inside 
> a pimpl, you can replace the two members with a llvm::PointerIntPair, if you 
> are so inclined.


Not done.  I proposed `llvm::PointerIntPair` originally to match your goal of 
keeping single pointer in `SBCommandReturnObject`. But when there is already a 
`new` instance of `lldb_private::CommandReturnObjectImpl` then 
`llvm::PointerIntPair` therein would be more a burden both in the source code 
and for CPU.

| `m_opaque_up` type  | ABI compatible | CPU overhead   
| formal http://lldb.llvm.org/resources/sbapi.html compliance |
| `shared_ptr` with two deleters  | no (size 16)   | high ('lock' 
instructions) | yes (a bug in the sbapi.html doc?)  
|
| `llvm::PointerIntPair`  | yes (size 8)   | low
| no (sort of)|
| `unique_ptr` | yes (size 8)   | high (extra object 
allocation) | yes |
|

But I am fine with this patch implementing `SBCommandReturnObjectImpl` as this 
is not any performance critical part of code.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67589

Files:
  lldb/include/lldb/API/SBCommandReturnObject.h
  lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile
  
lldb/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
  lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
  lldb/scripts/Python/python-wrapper.swig
  lldb/source/API/SBCommandInterpreter.cpp
  lldb/source/API/SBCommandReturnObject.cpp

Index: lldb/source/API/SBCommandReturnObject.cpp
===
--- lldb/source/API/SBCommandReturnObject.cpp
+++ lldb/source/API/SBCommandReturnObject.cpp
@@ -18,11 +18,43 @@
 using namespace lldb;
 using namespace lldb_private;
 
+class lldb_private::SBCommandReturnObjectImpl {
+public:
+  SBCommandReturnObjectImpl()
+  : m_ptr(new CommandReturnObject()), m_owned(true) {}
+  SBCommandReturnObjectImpl(CommandReturnObject &ref)
+  : m_ptr(&ref), m_owned(false) {}
+  SBCommandReturnObjectImpl(const SBCommandReturnObjectImpl &rhs)
+  : m_ptr(new CommandReturnObject(*rhs.m_ptr)), m_owned(rhs.m_owned) {}
+  SBCommandReturnObjectImpl &operator=(const SBCommandReturnObjectImpl &rhs) {
+SBCommandReturnObjectImpl copy(rhs);
+std::swap(*this, copy);
+return *this;
+  }
+  // rvalue ctor+assignment are not used by SBCommandReturnObject.
+  ~SBCommandReturnObjectImpl() {
+if (m_owned)
+  delete m_ptr;
+  }
+
+  CommandReturnObject &operator*() const { return *m_ptr; }
+
+private:
+  CommandReturnObject *m_ptr;
+  bool m_owned;
+};
+
 SBCommandReturnObject::SBCommandReturnObject()
-: m_opaque_up(new CommandReturnObject()) {
+: m_opaque_up(new SBCommandReturnObjectImpl()) {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBCommandReturnObject);
 }
 
+SBCommandReturnObject::SBCommandReturnObject(CommandReturnObject &ref)
+: m_opaque_up(new SBCommandReturnObjectImpl(ref)) {
+  LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
+  (lldb_private::CommandReturnObject &), ref);
+}
+
 SBCommandReturnObject::SBCommandReturnObject(const SBCommandReturnObject &rhs)
 : m_opaque_up() {
   LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
@@ -31,26 +63,10 @@
   m_opaque_up = clone(rhs.m_opaque_up);
 }
 
-SBCommandReturnObject::SBCommandReturnObject(CommandReturnObject *ptr)
-: m_opaque_up(ptr) {
-  LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
-  (lldb_private::CommandReturnObject *), ptr);
-  assert(ptr != nullptr);
-}
-
-SBCommandReturnObject::~SBCommandReturnObject() = default;
-
-CommandReturnObject *SBCommandReturnObject::Release() {
-  LLDB_RECORD_METHOD_NO_ARGS(lldb_private::CommandReturnObject *,
- SBCommandReturnObject, Release);
-
-  return LLDB_RECORD_RESULT(m_opaque_up.release());
-}
-
-const SBCommandReturnObject &SBCommandReturnObject::
+SBCommandReturnObject &SBCommandReturnObject::
 operator=(const SBCommandReturnObject &rhs) {
   LLDB_RECORD_METHOD(
-  const lldb::SBCommandReturnObject &,
+  lldb::SBCommandReturnObject &,
   SBCommandRetu

[Lldb-commits] [lldb] r373016 - [lldb-vscode] correctly handle multiple sourceMap entries

[Alex Langford via lldb-commits]

Author: xiaobai
Date: Thu Sep 26 14:18:37 2019
New Revision: 373016

URL: http://llvm.org/viewvc/llvm-project?rev=373016&view=rev
Log:
[lldb-vscode] correctly handle multiple sourceMap entries

Summary:
`lldb-vscode` concatenates a string of sourceMap entries
specified in the config, but fails to put a space between
each entry, which causes the settings command to fail.
This patch adds a space between each mapping.

Patch by Richard Howell

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

Modified:
lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp

Modified: lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp?rev=373016&r1=373015&r2=373016&view=diff
==
--- lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp (original)
+++ lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp Thu Sep 26 14:18:37 2019
@@ -436,7 +436,7 @@ void SetSourceMapFromArguments(const llv
   }
   auto mapFrom = GetAsString((*mapping)[0]);
   auto mapTo = GetAsString((*mapping)[1]);
-  strm << "\"" << mapFrom << "\" \"" << mapTo << "\"";
+  strm << "\"" << mapFrom << "\" \"" << mapTo << "\" ";
 }
   } else {
 if (ObjectContainsKey(arguments, "sourceMap")) {
@@ -2582,7 +2582,7 @@ const std::maphttps://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 222029.
kwk added a comment.

- Cleanup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -12,6 +12,8 @@
 #include 
 
 #include 
+#include 
+#include 
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
@@ -55,7 +57,7 @@
 /// This class provides a generic ELF (32/64 bit) reader plugin implementing
 /// the ObjectFile protocol.
 class ObjectFileELF : public lldb_private::ObjectFile {
-public:
+public:  
   // Static Functions
   static void Initialize();
 
@@ -184,6 +186,8 @@
 
   typedef std::map
   FileAddressToAddressClassMap;
+  
+  typedef std::unordered_set UniqueElfSymbolColl;
 
   /// Version of this reader common to all plugins based on this class.
   static const uint32_t m_plugin_version = 1;
@@ -278,7 +282,8 @@
   /// will parse the symbols only once.  Returns the number of symbols parsed.
   unsigned ParseSymbolTable(lldb_private::Symtab *symbol_table,
 lldb::user_id_t start_id,
-lldb_private::Section *symtab);
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Helper routine for ParseSymbolTable().
   unsigned ParseSymbols(lldb_private::Symtab *symbol_table,
@@ -286,7 +291,8 @@
 lldb_private::SectionList *section_list,
 const size_t num_symbols,
 const lldb_private::DataExtractor &symtab_data,
-const lldb_private::DataExtractor &strtab_data);
+const lldb_private::DataExtractor &strtab_data,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Scans the relocation entries and adds a set of artificial symbols to the
   /// given symbol table for each PLT slot.  Returns the number of symbols
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1871,7 +1871,8 @@
  SectionList *section_list,
  const size_t num_symbols,
  const DataExtractor &symtab_data,
- const DataExtractor &strtab_data) {
+ const DataExtractor &strtab_data,
+ UniqueElfSymbolColl &unique_elf_symbols) {
   ELFSymbol symbol;
   lldb::offset_t offset = 0;
 
@@ -2196,20 +2197,28 @@
 symbol_size_valid,  // Symbol size is valid
 has_suffix, // Contains linker annotations?
 flags); // Symbol flags.
-symtab->AddSymbol(dc_symbol);
+
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);
+}
   }
   return i;
 }
 
-unsigned ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
- user_id_t start_id,
- lldb_private::Section *symtab) {
+unsigned
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id,
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols) {
   if (symtab->GetObjectFile() != this) {
 // If the symbol table section is owned by a different object file, have it
 // do the parsing.
 ObjectFileELF *obj_file_elf =
 static_cast(symtab->GetObjectFile());
-return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab);
+return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab,
+  unique_elf_symbols);
   }
 
   // Get section list for this object file.
@@ -2237,7 +2246,7 @@
   size_t num_symbols = symtab_data.GetByteSize() / symtab_hdr->sh_entsize;
 
   return ParseSymbols(symbol_t

[Lldb-commits] [PATCH] D68106: Fix a crasher due to an assert when two files have the same UUID but different paths.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 222032.
clayborg added a comment.

Remove printf that was left in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68106

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-arm-same-uuids.yaml
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -49,8 +49,8 @@
 class PlaceholderObjectFile : public ObjectFile {
 public:
   PlaceholderObjectFile(const lldb::ModuleSP &module_sp,
-const ModuleSpec &module_spec, lldb::offset_t base,
-lldb::offset_t size)
+const ModuleSpec &module_spec, lldb::addr_t base,
+lldb::addr_t size)
   : ObjectFile(module_sp, &module_spec.GetFileSpec(), /*file_offset*/ 0,
/*length*/ 0, /*data_sp*/ nullptr, /*data_offset*/ 0),
 m_arch(module_spec.GetArchitecture()), m_uuid(module_spec.GetUUID()),
@@ -58,7 +58,10 @@
 m_symtab_up = std::make_unique(this);
   }
 
-  ConstString GetPluginName() override { return ConstString("placeholder"); }
+  static ConstString GetStaticPluginName() {
+return ConstString("placeholder");
+  }
+  ConstString GetPluginName() override { return GetStaticPluginName(); }
   uint32_t GetPluginVersion() override { return 1; }
   bool ParseHeader() override { return true; }
   Type CalculateType() override { return eTypeUnknown; }
@@ -109,11 +112,12 @@
   GetFileSpec(), m_base, m_base + m_size);
   }
 
+  lldb::addr_t GetBaseAddress() const { return m_base; }
 private:
   ArchSpec m_arch;
   UUID m_uuid;
-  lldb::offset_t m_base;
-  lldb::offset_t m_size;
+  lldb::addr_t m_base;
+  lldb::addr_t m_size;
 };
 } // namespace
 
@@ -351,14 +355,15 @@
   std::vector filtered_modules =
   m_minidump_parser->GetFilteredModuleList();
 
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES));
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
 
   for (auto module : filtered_modules) {
 std::string name = cantFail(m_minidump_parser->GetMinidumpFile().getString(
 module->ModuleNameRVA));
+const uint64_t load_addr = module->BaseOfImage;
+const uint64_t load_size = module->SizeOfImage;
 LLDB_LOG(log, "found module: name: {0} {1:x10}-{2:x10} size: {3}", name,
- module->BaseOfImage, module->BaseOfImage + module->SizeOfImage,
- module->SizeOfImage);
+ load_addr, load_addr + load_size, load_size);
 
 // check if the process is wow64 - a 32 bit windows process running on a
 // 64 bit windows
@@ -373,7 +378,7 @@
 Status error;
 // Try and find a module with a full UUID that matches. This function will
 // add the module to the target if it finds one.
-lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, 
+lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec,
  true /* notify */, &error);
 if (!module_sp) {
   // Try and find a module without specifying the UUID and only looking for
@@ -386,8 +391,8 @@
   ModuleSpec basename_module_spec(module_spec);
   basename_module_spec.GetUUID().Clear();
   basename_module_spec.GetFileSpec().GetDirectory().Clear();
-  module_sp = GetTarget().GetOrCreateModule(basename_module_spec, 
- true /* notify */, &error);
+  module_sp = GetTarget().GetOrCreateModule(basename_module_spec,
+true /* notify */, &error);
   if (module_sp) {
 // We consider the module to be a match if the minidump UUID is a
 // prefix of the actual UUID, or if either of the UUIDs are empty.
@@ -401,6 +406,18 @@
 }
   }
 }
+if (module_sp) {
+  // Watch out for place holder modules that have different paths, but the
+  // same UUID. If the base address is different, create a new module. If
+  // we don't then we will end up setting the load address of a different
+  // PlaceholderObjectFile and an assertion will fire.
+  auto *objfile = module_sp->GetObjectFile();
+  if (objfile && objfile->GetPluginName() ==
+  PlaceholderObjectFile::GetStaticPluginName()) {
+if (((PlaceholderObjectFile *)objfile)->GetBaseAddress() != load_addr)
+  module_sp.reset();
+  }
+}
 if (!module_sp) {
   // We failed to locate a matching local object file. Fortunately, the
   // minidump format encodes enough informatio

[Lldb-commits] [PATCH] D68106: Fix a crasher due to an assert when two files have the same UUID but different paths.

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, aadsm, dvlahovski.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
clayborg updated this revision to Diff 222032.
clayborg added a comment.

Remove printf that was left in.


The PlaceholderObjectFile has an assert in SetLoadAddress that fires if "m_base 
== value" is not true. To avoid this, we create check that the base address 
matches, and if it doesn't we clear the module that was found using the UUID so 
that we create a new PlaceholderObjectFile. Added a test to cover this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68106

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-arm-same-uuids.yaml
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -49,8 +49,8 @@
 class PlaceholderObjectFile : public ObjectFile {
 public:
   PlaceholderObjectFile(const lldb::ModuleSP &module_sp,
-const ModuleSpec &module_spec, lldb::offset_t base,
-lldb::offset_t size)
+const ModuleSpec &module_spec, lldb::addr_t base,
+lldb::addr_t size)
   : ObjectFile(module_sp, &module_spec.GetFileSpec(), /*file_offset*/ 0,
/*length*/ 0, /*data_sp*/ nullptr, /*data_offset*/ 0),
 m_arch(module_spec.GetArchitecture()), m_uuid(module_spec.GetUUID()),
@@ -58,7 +58,10 @@
 m_symtab_up = std::make_unique(this);
   }
 
-  ConstString GetPluginName() override { return ConstString("placeholder"); }
+  static ConstString GetStaticPluginName() {
+return ConstString("placeholder");
+  }
+  ConstString GetPluginName() override { return GetStaticPluginName(); }
   uint32_t GetPluginVersion() override { return 1; }
   bool ParseHeader() override { return true; }
   Type CalculateType() override { return eTypeUnknown; }
@@ -109,11 +112,12 @@
   GetFileSpec(), m_base, m_base + m_size);
   }
 
+  lldb::addr_t GetBaseAddress() const { return m_base; }
 private:
   ArchSpec m_arch;
   UUID m_uuid;
-  lldb::offset_t m_base;
-  lldb::offset_t m_size;
+  lldb::addr_t m_base;
+  lldb::addr_t m_size;
 };
 } // namespace
 
@@ -351,14 +355,15 @@
   std::vector filtered_modules =
   m_minidump_parser->GetFilteredModuleList();
 
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES));
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
 
   for (auto module : filtered_modules) {
 std::string name = cantFail(m_minidump_parser->GetMinidumpFile().getString(
 module->ModuleNameRVA));
+const uint64_t load_addr = module->BaseOfImage;
+const uint64_t load_size = module->SizeOfImage;
 LLDB_LOG(log, "found module: name: {0} {1:x10}-{2:x10} size: {3}", name,
- module->BaseOfImage, module->BaseOfImage + module->SizeOfImage,
- module->SizeOfImage);
+ load_addr, load_addr + load_size, load_size);
 
 // check if the process is wow64 - a 32 bit windows process running on a
 // 64 bit windows
@@ -373,7 +378,7 @@
 Status error;
 // Try and find a module with a full UUID that matches. This function will
 // add the module to the target if it finds one.
-lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, 
+lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec,
  true /* notify */, &error);
 if (!module_sp) {
   // Try and find a module without specifying the UUID and only looking for
@@ -386,8 +391,8 @@
   ModuleSpec basename_module_spec(module_spec);
   basename_module_spec.GetUUID().Clear();
   basename_module_spec.GetFileSpec().GetDirectory().Clear();
-  module_sp = GetTarget().GetOrCreateModule(basename_module_spec, 
- true /* notify */, &error);
+  module_sp = GetTarget().GetOrCreateModule(basename_module_spec,
+true /* notify */, &error);
   if (module_sp) {
 // We consider the module to be a match if the minidump UUID is a
 // prefix of the actual UUID, or if either of the UUIDs are empty.
@@ -401,6 +406,18 @@
 }
   }
 }
+if (module_sp) {
+  // Watch out for place holder modules that have different paths, but the
+  // same UUID. If the base address is different, create a new module. If
+  // we don't then we will end up setting the load address of a different
+  // PlaceholderObjectFile and an assertion will fire.
+  auto *objfile = modul

[Lldb-commits] [PATCH] D68069: Explicitly set entry point arch when it's thumb

[António Afonso via Phabricator via lldb-commits]

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

> For the test, what would you say to writing that as a lit test instead 
> (testing the address class deduction via the disassemble command you 
> mentioned)?

I was actually keen on this since lit is the only type of test I haven't used 
yet but then thought that it wouldn't really test my change directly (just 
indirectly). I know I put that as a test in my summary but it was more like a 
sanity check. The real test here is checking the address class (which is what 
is changed in the code). There are different things in lldb that uses that like 
setting a breakpoint or disassembling instructions, that's why I don't feel 
that testing the consequence is the ideal test. What do you think?

> The yaml is actually fairly readable as is, but given that you felt the need 
> to include the commands used to produce that input, it might be better to 
> actually produce that file via the commands as a part of the test 
> (substituting llvm-mc for as and ld.lld for linker).

I just put it there for completion sake, I always like to have the source of 
things when I didn't do it by hand. In this case I prefer to have the yaml 
because I'm not sure if in all scenarios that we run the test we'll be able to 
assemble arm assembly into an object?




Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2737-2738
+  0,  // Offset in section or symbol value.
+  2,  // Size.
+  true,   // Size is valid.
+  false,  // Contains linker annotations?

labath wrote:
> This is pretty arbitrary. Would it be possible to just set 
> size_is_valid=false, and let the usual symbol size deduction logic figure out 
> something reasonable here?
To be honest I'm not sure how the size_is_valid = false is used as I've only 
seen it being used when going through the EH frame FDE entries.

Setting the size to 0 is problematic though, when the symbol is added to the 
symtab its size will automatically span to the next function symbol boundary. I 
think this can be dangerous because the symtab for the object file might not 
have all function boundaries defined and in the event that we have mixed 
arm/thumb code in it, it will incorrectly mark arm code as thumb. This is why I 
wanted to be conservative here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68069



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

[Lldb-commits] [PATCH] D68069: Explicitly set entry point arch when it's thumb

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 222063.
aadsm added a comment.

Use GetNextSyntheticSymbolName() to generate the symbol name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68069

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp

Index: lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
===
--- lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
+++ lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
@@ -172,3 +172,131 @@
   Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9", 20);
   EXPECT_EQ(Spec.GetUUID(), Uuid);
 }
+
+TEST_F(ObjectFileELFTest, GetSymtab_NoSymEntryPointArmThumbAddressClass) {
+  /*
+  // nosym-entrypoint-arm-thumb.s
+  .global _Start
+  .thumb_func
+  _start:
+  mov r0, #42
+  mov r7, #1
+  svc #0
+  // arm-linux-androideabi-as nosym-entrypoint-arm-thumb.s
+  //   -o nosym-entrypoint-arm-thumb.o
+  // arm-linux-androideabi-ld nosym-entrypoint-arm-thumb.o
+  //   -o nosym-entrypoint-arm-thumb -e 0x8075 -s
+  */
+  auto ExpectedFile = TestFile::fromYaml(R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_ARM
+  Flags:   [ EF_ARM_SOFT_FLOAT, EF_ARM_EABI_VER5 ]
+  Entry:   0x8075
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+Address: 0x8074
+AddressAlign:0x0002
+Content: 2A20012700DF
+  - Name:.data
+Type:SHT_PROGBITS
+Flags:   [ SHF_WRITE, SHF_ALLOC ]
+Address: 0x9000
+AddressAlign:0x0001
+Content: ''
+  - Name:.bss
+Type:SHT_NOBITS
+Flags:   [ SHF_WRITE, SHF_ALLOC ]
+Address: 0x9000
+AddressAlign:0x0001
+  - Name:.note.gnu.gold-version
+Type:SHT_NOTE
+AddressAlign:0x0004
+Content: 040009000400474E5500676F6C6420312E313100
+  - Name:.ARM.attributes
+Type:SHT_ARM_ATTRIBUTES
+AddressAlign:0x0001
+Content: '41130061656162690001090006020901'
+...
+)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+  ModuleSpec spec{FileSpec(ExpectedFile->name())};
+  spec.GetSymbolFileSpec().SetFile(ExpectedFile->name(),
+   FileSpec::Style::native);
+  auto module_sp = std::make_shared(spec);
+
+  auto entry_point_addr = module_sp->GetObjectFile()->GetEntryPointAddress();
+  ASSERT_TRUE(entry_point_addr.GetOffset() & 1);
+  // Decrease the offsite by 1 to make it into a breakable address since this
+  // is Thumb.
+  entry_point_addr.SetOffset(entry_point_addr.GetOffset() - 1);
+  ASSERT_EQ(entry_point_addr.GetAddressClass(),
+AddressClass::eCodeAlternateISA);
+}
+
+TEST_F(ObjectFileELFTest, GetSymtab_NoSymEntryPointArmAddressClass) {
+  /*
+  // nosym-entrypoint-arm.s
+  .global _Start
+  _start:
+  mov r0, #42
+  mov r7, #1
+  svc #0
+  // arm-linux-androideabi-as nosym-entrypoint-arm.s
+  //   -o nosym-entrypoint-arm.o
+  // arm-linux-androideabi-ld nosym-entrypoint-arm.o
+  //   -o nosym-entrypoint-arm -e 0x8074 -s
+  */
+  auto ExpectedFile = TestFile::fromYaml(R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_ARM
+  Flags:   [ EF_ARM_SOFT_FLOAT, EF_ARM_EABI_VER5 ]
+  Entry:   0x8074
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+Address: 0x8074
+AddressAlign:0x0004
+Content: 2A00A0E30170A0E300EF
+  - Name:.data
+Type:SHT_PROGBITS
+Flags:   [ SHF_WRITE, SHF_ALLOC ]
+Address: 0x9000
+AddressAlign:0x0001
+Content: ''
+  - Name:.bss
+Type:SHT_NOBITS
+Flags:   [ SHF_WRITE, SHF_ALLOC ]
+Address: 0x9000
+AddressAlign:0x0001
+  - Name:.note.gnu.gold-version
+Type:SHT_NOTE
+AddressAlign:0x0004
+Content: 040009000400474E5500676F6C6420312E313100
+  - Name:.ARM.attributes
+Type:SHT_ARM_ATTRIBUTES
+AddressAlign:0x0001
+Content: '41130061656162690001090006010801'
+...
+)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+  ModuleSpec spec{FileSpec(ExpectedF

[Lldb-commits] [PATCH] D67891: remove File::SetStream(), make new files instead.

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added a comment.

@labath how's this one looking?  need any changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67891



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

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 222073.
kwk added a comment.

- Cleanup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -12,6 +12,7 @@
 #include 
 
 #include 
+#include 
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
@@ -55,7 +56,7 @@
 /// This class provides a generic ELF (32/64 bit) reader plugin implementing
 /// the ObjectFile protocol.
 class ObjectFileELF : public lldb_private::ObjectFile {
-public:
+public:  
   // Static Functions
   static void Initialize();
 
@@ -184,6 +185,8 @@
 
   typedef std::map
   FileAddressToAddressClassMap;
+  
+  typedef std::unordered_set UniqueElfSymbolColl;
 
   /// Version of this reader common to all plugins based on this class.
   static const uint32_t m_plugin_version = 1;
@@ -278,7 +281,8 @@
   /// will parse the symbols only once.  Returns the number of symbols parsed.
   unsigned ParseSymbolTable(lldb_private::Symtab *symbol_table,
 lldb::user_id_t start_id,
-lldb_private::Section *symtab);
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Helper routine for ParseSymbolTable().
   unsigned ParseSymbols(lldb_private::Symtab *symbol_table,
@@ -286,7 +290,8 @@
 lldb_private::SectionList *section_list,
 const size_t num_symbols,
 const lldb_private::DataExtractor &symtab_data,
-const lldb_private::DataExtractor &strtab_data);
+const lldb_private::DataExtractor &strtab_data,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Scans the relocation entries and adds a set of artificial symbols to the
   /// given symbol table for each PLT slot.  Returns the number of symbols
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1871,7 +1871,8 @@
  SectionList *section_list,
  const size_t num_symbols,
  const DataExtractor &symtab_data,
- const DataExtractor &strtab_data) {
+ const DataExtractor &strtab_data,
+ UniqueElfSymbolColl &unique_elf_symbols) {
   ELFSymbol symbol;
   lldb::offset_t offset = 0;
 
@@ -2196,20 +2197,28 @@
 symbol_size_valid,  // Symbol size is valid
 has_suffix, // Contains linker annotations?
 flags); // Symbol flags.
-symtab->AddSymbol(dc_symbol);
+
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);
+}
   }
   return i;
 }
 
-unsigned ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
- user_id_t start_id,
- lldb_private::Section *symtab) {
+unsigned
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id,
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols) {
   if (symtab->GetObjectFile() != this) {
 // If the symbol table section is owned by a different object file, have it
 // do the parsing.
 ObjectFileELF *obj_file_elf =
 static_cast(symtab->GetObjectFile());
-return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab);
+return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab,
+  unique_elf_symbols);
   }
 
   // Get section list for this object file.
@@ -2237,7 +2246,7 @@
   size_t num_symbols = symtab_data.GetByteSize() / symtab_hdr->sh_entsize;
 
   return ParseSymbols(symbol_table, start

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

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

- Change order of compare members to match order of member definitions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -12,6 +12,7 @@
 #include 
 
 #include 
+#include 
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
@@ -55,7 +56,7 @@
 /// This class provides a generic ELF (32/64 bit) reader plugin implementing
 /// the ObjectFile protocol.
 class ObjectFileELF : public lldb_private::ObjectFile {
-public:
+public:  
   // Static Functions
   static void Initialize();
 
@@ -184,6 +185,8 @@
 
   typedef std::map
   FileAddressToAddressClassMap;
+  
+  typedef std::unordered_set UniqueElfSymbolColl;
 
   /// Version of this reader common to all plugins based on this class.
   static const uint32_t m_plugin_version = 1;
@@ -278,7 +281,8 @@
   /// will parse the symbols only once.  Returns the number of symbols parsed.
   unsigned ParseSymbolTable(lldb_private::Symtab *symbol_table,
 lldb::user_id_t start_id,
-lldb_private::Section *symtab);
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Helper routine for ParseSymbolTable().
   unsigned ParseSymbols(lldb_private::Symtab *symbol_table,
@@ -286,7 +290,8 @@
 lldb_private::SectionList *section_list,
 const size_t num_symbols,
 const lldb_private::DataExtractor &symtab_data,
-const lldb_private::DataExtractor &strtab_data);
+const lldb_private::DataExtractor &strtab_data,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Scans the relocation entries and adds a set of artificial symbols to the
   /// given symbol table for each PLT slot.  Returns the number of symbols
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1871,7 +1871,8 @@
  SectionList *section_list,
  const size_t num_symbols,
  const DataExtractor &symtab_data,
- const DataExtractor &strtab_data) {
+ const DataExtractor &strtab_data,
+ UniqueElfSymbolColl &unique_elf_symbols) {
   ELFSymbol symbol;
   lldb::offset_t offset = 0;
 
@@ -2196,20 +2197,28 @@
 symbol_size_valid,  // Symbol size is valid
 has_suffix, // Contains linker annotations?
 flags); // Symbol flags.
-symtab->AddSymbol(dc_symbol);
+
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);
+}
   }
   return i;
 }
 
-unsigned ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
- user_id_t start_id,
- lldb_private::Section *symtab) {
+unsigned
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id,
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols) {
   if (symtab->GetObjectFile() != this) {
 // If the symbol table section is owned by a different object file, have it
 // do the parsing.
 ObjectFileELF *obj_file_elf =
 static_cast(symtab->GetObjectFile());
-return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab);
+return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab,
+  unique_elf_symbols);
   }
 
   // Get section list for this object file.
@@ -2237,7 +2246,7 @@
   size_t num_symbols =

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

Not all is answered now but please respect: 
https://reviews.llvm.org/D67390#1683705




Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373
+  r.st_name = st_name;
+  return elf::ELFSymbol::operator==(r) &&
+ st_name_string == rhs.st_name_string;

clayborg wrote:
> I would almost just manually compare only the things we care about here. 
> Again, what about st_shndx when comparing a symbol from the main symbol table 
> and one from the .gnu_debugdata symbol table. Are those section indexes 
> guaranteed to match? I would think not. 
@clayborg I explicitly only compare what we care about and therefore always set 
the name index to  be the same.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:289
+struct NamedELFSymbol : ELFSymbol {
+  lldb_private::ConstString st_name_string; ///< Actual name of the ELF symbol
+

clayborg wrote:
> Do we need a ConstString for the st_shndx as well so we can correctly compare 
> a section from one file to a section from another as in the .gnu_debugdata 
> case?
That is a good point.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:440
+tmp.st_name = 0;
+std::size_t h1 = std::hash()(tmp);
+std::size_t h2 = std::hash()(s.st_name_string.AsCString());

clayborg wrote:
> Don't we need to hash everything we care about except the st_name? Those 
> indexes can differ if they come from a different string table? Shouldn't this 
> be:
> 
> ```
> std::size_t h1 = std::hash()(s.st_value);
> std::size_t h2 = std::hash()(s.st_size);
> // Skip std::size_t h3 = std::hash()(s.st_name);
> std::size_t h4 = std::hash()(s.st_info);
> std::size_t h5 = std::hash()(s.st_other);
> std::size_t h6 = std::hash()(s.st_shndx);
> std::size_t h7 = std::hash()(s.st_name_string.AsCString());
> return llvm::hash_combine(h1, h2, h4, h5, h6, h7);
> ```
> I left the "h" variables with the same names to show we don't want "h3".
That's why I explicitly set the name to 0 always which effectively ignores it 
because it has no effect on the hash then. Please see the specialization hash 
for `NamedELFSymbol`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390



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

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 222076.
kwk added a comment.

- make the section name part of NamedELFSymbol


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -12,6 +12,7 @@
 #include 
 
 #include 
+#include 
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
@@ -55,7 +56,7 @@
 /// This class provides a generic ELF (32/64 bit) reader plugin implementing
 /// the ObjectFile protocol.
 class ObjectFileELF : public lldb_private::ObjectFile {
-public:
+public:  
   // Static Functions
   static void Initialize();
 
@@ -184,6 +185,8 @@
 
   typedef std::map
   FileAddressToAddressClassMap;
+  
+  typedef std::unordered_set UniqueElfSymbolColl;
 
   /// Version of this reader common to all plugins based on this class.
   static const uint32_t m_plugin_version = 1;
@@ -278,7 +281,8 @@
   /// will parse the symbols only once.  Returns the number of symbols parsed.
   unsigned ParseSymbolTable(lldb_private::Symtab *symbol_table,
 lldb::user_id_t start_id,
-lldb_private::Section *symtab);
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Helper routine for ParseSymbolTable().
   unsigned ParseSymbols(lldb_private::Symtab *symbol_table,
@@ -286,7 +290,8 @@
 lldb_private::SectionList *section_list,
 const size_t num_symbols,
 const lldb_private::DataExtractor &symtab_data,
-const lldb_private::DataExtractor &strtab_data);
+const lldb_private::DataExtractor &strtab_data,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Scans the relocation entries and adds a set of artificial symbols to the
   /// given symbol table for each PLT slot.  Returns the number of symbols
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1871,7 +1871,8 @@
  SectionList *section_list,
  const size_t num_symbols,
  const DataExtractor &symtab_data,
- const DataExtractor &strtab_data) {
+ const DataExtractor &strtab_data,
+ UniqueElfSymbolColl &unique_elf_symbols) {
   ELFSymbol symbol;
   lldb::offset_t offset = 0;
 
@@ -2196,20 +2197,30 @@
 symbol_size_valid,  // Symbol size is valid
 has_suffix, // Contains linker annotations?
 flags); // Symbol flags.
-symtab->AddSymbol(dc_symbol);
+
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (symbol_section_sp)
+needle.st_section_name_string = ConstString(symbol_section_sp->GetName());
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);
+}
   }
   return i;
 }
 
-unsigned ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
- user_id_t start_id,
- lldb_private::Section *symtab) {
+unsigned
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id,
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols) {
   if (symtab->GetObjectFile() != this) {
 // If the symbol table section is owned by a different object file, have it
 // do the parsing.
 ObjectFileELF *obj_file_elf =
 static_cast(symtab->GetObjectFile());
-return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab);
+return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab,
+  unique_elf_symbols);
   }
 
   // Get section list for this object file.
@

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

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

I think I've finished the implementation now and should have answered all your 
comments and concerns. I run tests now. I would appreciate if you (@clayborg , 
@labath , @jankratochvil ) can take a look at this patch again.




Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:289
+struct NamedELFSymbol : ELFSymbol {
+  lldb_private::ConstString st_name_string; ///< Actual name of the ELF symbol
+

kwk wrote:
> clayborg wrote:
> > Do we need a ConstString for the st_shndx as well so we can correctly 
> > compare a section from one file to a section from another as in the 
> > .gnu_debugdata case?
> That is a good point.
@clayborg in fact I think this could be the reason not to use a set of 
`lldb_private::Symbol` objects because there we don't store the section name or 
symbol name but only addresses or indexes. I did add the 
`st_section_name_string` struct member.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:430
+std::size_t h5 = std::hash()(s.st_other);
+std::size_t h6 = std::hash()(s.st_shndx);
+return llvm::hash_combine(h1, h2, h3, h4, h5, h6);

clayborg wrote:
> I know the section index will match between for symbol tables in the same ELF 
> file, but what about a symbol table in an external file like .gnu_debugdata?
I did add the section name to `NamedELFSymbol` and explicitly ignore it when 
building the hash for the base `ELFSymbol`.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:42
 #include "llvm/Support/MipsABIFlags.h"
+#include "lldb/Utility/StreamString.h"
 

jankratochvil wrote:
> Is it really needed?
removed.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2205
+  m_unique_symbol_set.push_back(symbol);
+}
   }

jankratochvil wrote:
> What if the symbol is ignored, the function will then incorrectly return a 
> number of added symbols even when they were not added, wouldn't it?
@jankratochvil we already have places inside this `for`-loop where we 
`continue`. I hope it is okay to ask the same question back that you've asked 
for those `continue`-places. Why don't we adjust the returned number (`i`) in 
case symbols where skipped?



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2202-2206
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_bare);
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);

clayborg wrote:
> Do we even need NamedELFSymbol? Can we just make an unordered_set of 
> lldb_private::Symbol values?
@clayborg I find it much easier with `NamedELFSymbol` because all we have to do 
is derive from `ELFSymbol` and add the strings for the symbol name and the 
section name. If we were to use `lldb_private::Symbol` I would have to lookup 
the symbols manually each time I calculate a hash which seems bad. I mean, the 
symbol and section name already are `ConstString`s and should be stored and 
computed very efficiently. Also I wanted to keep things local to ELF and not 
mess with everything that uses `lldb_private::Symbol`. Makes sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390



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

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 222080.
kwk marked an inline comment as done.
kwk added a comment.

- Use symbol name including @VERSION suffix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390

Files:
  lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c
  lldb/lit/Modules/ELF/Inputs/load-symtab-and-dynsym.c
  lldb/lit/Modules/ELF/load-from-dynsym-alone.test
  lldb/lit/Modules/ELF/load-symtab-and-dynsym.test
  lldb/lit/helper/toolchain.py
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp
  lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -12,6 +12,7 @@
 #include 
 
 #include 
+#include 
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
@@ -55,7 +56,7 @@
 /// This class provides a generic ELF (32/64 bit) reader plugin implementing
 /// the ObjectFile protocol.
 class ObjectFileELF : public lldb_private::ObjectFile {
-public:
+public:  
   // Static Functions
   static void Initialize();
 
@@ -184,6 +185,8 @@
 
   typedef std::map
   FileAddressToAddressClassMap;
+  
+  typedef std::unordered_set UniqueElfSymbolColl;
 
   /// Version of this reader common to all plugins based on this class.
   static const uint32_t m_plugin_version = 1;
@@ -278,7 +281,8 @@
   /// will parse the symbols only once.  Returns the number of symbols parsed.
   unsigned ParseSymbolTable(lldb_private::Symtab *symbol_table,
 lldb::user_id_t start_id,
-lldb_private::Section *symtab);
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Helper routine for ParseSymbolTable().
   unsigned ParseSymbols(lldb_private::Symtab *symbol_table,
@@ -286,7 +290,8 @@
 lldb_private::SectionList *section_list,
 const size_t num_symbols,
 const lldb_private::DataExtractor &symtab_data,
-const lldb_private::DataExtractor &strtab_data);
+const lldb_private::DataExtractor &strtab_data,
+UniqueElfSymbolColl &unique_elf_symbols);
 
   /// Scans the relocation entries and adds a set of artificial symbols to the
   /// given symbol table for each PLT slot.  Returns the number of symbols
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1871,7 +1871,8 @@
  SectionList *section_list,
  const size_t num_symbols,
  const DataExtractor &symtab_data,
- const DataExtractor &strtab_data) {
+ const DataExtractor &strtab_data,
+ UniqueElfSymbolColl &unique_elf_symbols) {
   ELFSymbol symbol;
   lldb::offset_t offset = 0;
 
@@ -2196,20 +2197,30 @@
 symbol_size_valid,  // Symbol size is valid
 has_suffix, // Contains linker annotations?
 flags); // Symbol flags.
-symtab->AddSymbol(dc_symbol);
+
+NamedELFSymbol needle(symbol);
+needle.st_name_string = ConstString(symbol_ref);
+if (symbol_section_sp)
+needle.st_section_name_string = ConstString(symbol_section_sp->GetName());
+if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+  symtab->AddSymbol(dc_symbol);
+  unique_elf_symbols.insert(needle);
+}
   }
   return i;
 }
 
-unsigned ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
- user_id_t start_id,
- lldb_private::Section *symtab) {
+unsigned
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id,
+lldb_private::Section *symtab,
+UniqueElfSymbolColl &unique_elf_symbols) {
   if (symtab->GetObjectFile() != this) {
 // If the symbol table section is owned by a different object file, have it
 // do the parsing.
 ObjectFileELF *obj_file_elf =
 static_cast(symtab->GetObjectFile());
-return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab);
+return obj_file_elf->ParseSymbolTable(symbol_table, start_id, symtab,
+  unique_elf_symbols);
   }
 
   // Get s

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

Interesting. It looks like we indeed have a test (the only one failing atm.) 
that wants a symbol to be added twice:

  [ RUN  ] MangledTest.NameIndexes_FindFunctionSymbols
  /home/kkleine/llvm/lldb/unittests/Core/MangledTest.cpp:186: Failure
Expected: 1
  To be equal to: Count("puts@GLIBC_2.6", eFunctionNameTypeFull)
Which is: 0
  /home/kkleine/llvm/lldb/unittests/Core/MangledTest.cpp:187: Failure
Expected: 2
  To be equal to: Count("puts", eFunctionNameTypeFull)
Which is: 1
  /home/kkleine/llvm/lldb/unittests/Core/MangledTest.cpp:188: Failure
Expected: 2
  To be equal to: Count("puts", eFunctionNameTypeBase)
Which is: 1
  [  FAILED  ] MangledTest.NameIndexes_FindFunctionSymbols (1 ms)

Before I used the bare symbol name with stripped `@VERSION` suffix. Now I've 
changed the implementation of `NamedELFSymbol` to include the `@VERSION` 
suffix. Is that fine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390



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