[Lldb-commits] [lldb] f6e3abc - [lldb/gdb-remote] Remove last_stop_packet_mutex

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2021-09-29T11:23:14+02:00
New Revision: f6e3abc53021dbbba4944a4fa38c5731994823e9

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

LOG: [lldb/gdb-remote] Remove last_stop_packet_mutex

This is a remnant of the non-stop mode.

Added: 


Modified: 
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp 
b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 903c132dbf0ca..4d3a4c7a072a7 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -251,8 +251,7 @@ bool ProcessGDBRemote::CanDebug(lldb::TargetSP target_sp,
 ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp,
ListenerSP listener_sp)
 : Process(target_sp, listener_sp),
-  m_debugserver_pid(LLDB_INVALID_PROCESS_ID), m_last_stop_packet_mutex(),
-  m_register_info_sp(nullptr),
+  m_debugserver_pid(LLDB_INVALID_PROCESS_ID), m_register_info_sp(nullptr),
   m_async_broadcaster(nullptr, 
"lldb.process.gdb-remote.async-broadcaster"),
   m_async_listener_sp(
   Listener::MakeListener("lldb.process.gdb-remote.async-listener")),
@@ -1462,37 +1461,30 @@ bool ProcessGDBRemote::UpdateThreadIDList() {
 // See if we can get the thread IDs from the current stop reply packets
 // that might contain a "threads" key/value pair
 
-// Lock the thread stack while we access it
-// Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);
-std::unique_lock stop_stack_lock(
-m_last_stop_packet_mutex, std::defer_lock);
-if (stop_stack_lock.try_lock()) {
-  if (m_last_stop_packet) {
-// Get the thread stop info
-StringExtractorGDBRemote &stop_info = *m_last_stop_packet;
-const std::string &stop_info_str =
-std::string(stop_info.GetStringRef());
+if (m_last_stop_packet) {
+  // Get the thread stop info
+  StringExtractorGDBRemote &stop_info = *m_last_stop_packet;
+  const std::string &stop_info_str = std::string(stop_info.GetStringRef());
 
-m_thread_pcs.clear();
-const size_t thread_pcs_pos = stop_info_str.find(";thread-pcs:");
-if (thread_pcs_pos != std::string::npos) {
-  const size_t start = thread_pcs_pos + strlen(";thread-pcs:");
-  const size_t end = stop_info_str.find(';', start);
-  if (end != std::string::npos) {
-std::string value = stop_info_str.substr(start, end - start);
-UpdateThreadPCsFromStopReplyThreadsValue(value);
-  }
+  m_thread_pcs.clear();
+  const size_t thread_pcs_pos = stop_info_str.find(";thread-pcs:");
+  if (thread_pcs_pos != std::string::npos) {
+const size_t start = thread_pcs_pos + strlen(";thread-pcs:");
+const size_t end = stop_info_str.find(';', start);
+if (end != std::string::npos) {
+  std::string value = stop_info_str.substr(start, end - start);
+  UpdateThreadPCsFromStopReplyThreadsValue(value);
 }
+  }
 
-const size_t threads_pos = stop_info_str.find(";threads:");
-if (threads_pos != std::string::npos) {
-  const size_t start = threads_pos + strlen(";threads:");
-  const size_t end = stop_info_str.find(';', start);
-  if (end != std::string::npos) {
-std::string value = stop_info_str.substr(start, end - start);
-if (UpdateThreadIDsFromStopReplyThreadsValue(value))
-  return true;
-  }
+  const size_t threads_pos = stop_info_str.find(";threads:");
+  if (threads_pos != std::string::npos) {
+const size_t start = threads_pos + strlen(";threads:");
+const size_t end = stop_info_str.find(';', start);
+if (end != std::string::npos) {
+  std::string value = stop_info_str.substr(start, end - start);
+  if (UpdateThreadIDsFromStopReplyThreadsValue(value))
+return true;
 }
   }
 }
@@ -2317,14 +2309,9 @@ void ProcessGDBRemote::RefreshStateAfterStop() {
   // date before we do that or we might overwrite what was computed here.
   UpdateThreadListIfNeeded();
 
-  // Scope for the lock
-  {
-// Lock the thread stack while we access it
-std::lock_guard guard(m_last_stop_packet_mutex);
-if (m_last_stop_packet)
-  SetThreadStopInfo(*m_last_stop_packet);
-m_last_stop_packet.reset();
-  }
+  if (m_last_stop_packet)
+SetThreadStopInfo(*m_last_stop_packet);
+  m_last_stop_packet.reset();
 
   // If we have queried for a default thread id
   if (m_initial_tid != LLDB_INVALID_THREAD_ID) {
@@ -2571,13 +2558,7 @@ void ProcessGDBRemote::S

[Lldb-commits] [PATCH] D110693: [lldb] [Host] Remove TerminalStateSwitcher

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, krytarowski, jingham, teemperor, JDevlieghere, 
emaste.
mgorny requested review of this revision.

Remove TerminalStateSwitcher class.  It is not used anywhere and its API
is really weird.  This is the first step towards cleaning up Terminal.h.


https://reviews.llvm.org/D110693

Files:
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Host/common/Terminal.cpp

Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -188,50 +188,3 @@
 bool TerminalState::ProcessGroupIsValid() const {
   return static_cast(m_process_group) != -1;
 }
-
-// Constructor
-TerminalStateSwitcher::TerminalStateSwitcher() = default;
-
-// Destructor
-TerminalStateSwitcher::~TerminalStateSwitcher() = default;
-
-// Returns the number of states that this switcher contains
-uint32_t TerminalStateSwitcher::GetNumberOfStates() const {
-  return llvm::array_lengthof(m_ttystates);
-}
-
-// Restore the state at index "idx".
-//
-// Returns true if the restore was successful, false otherwise.
-bool TerminalStateSwitcher::Restore(uint32_t idx) const {
-  const uint32_t num_states = GetNumberOfStates();
-  if (idx >= num_states)
-return false;
-
-  // See if we already are in this state?
-  if (m_currentState < num_states && (idx == m_currentState) &&
-  m_ttystates[idx].IsValid())
-return true;
-
-  // Set the state to match the index passed in and only update the current
-  // state if there are no errors.
-  if (m_ttystates[idx].Restore()) {
-m_currentState = idx;
-return true;
-  }
-
-  // We failed to set the state. The tty state was invalid or not initialized.
-  return false;
-}
-
-// Save the state at index "idx" for file descriptor "fd" and save the process
-// group if requested.
-//
-// Returns true if the restore was successful, false otherwise.
-bool TerminalStateSwitcher::Save(uint32_t idx, int fd,
- bool save_process_group) {
-  const uint32_t num_states = GetNumberOfStates();
-  if (idx < num_states)
-return m_ttystates[idx].Save(fd, save_process_group);
-  return false;
-}
Index: lldb/include/lldb/Host/Terminal.h
===
--- lldb/include/lldb/Host/Terminal.h
+++ lldb/include/lldb/Host/Terminal.h
@@ -124,59 +124,6 @@
   lldb::pid_t m_process_group = -1; ///< Cached process group information.
 };
 
-/// \class TerminalStateSwitcher Terminal.h "lldb/Host/Terminal.h"
-/// A TTY state switching class.
-///
-/// This class can be used to remember 2 TTY states for a given file
-/// descriptor and switch between the two states.
-class TerminalStateSwitcher {
-public:
-  /// Constructor
-  TerminalStateSwitcher();
-
-  /// Destructor
-  ~TerminalStateSwitcher();
-
-  /// Get the number of possible states to save.
-  ///
-  /// \return
-  /// The number of states that this TTY switcher object contains.
-  uint32_t GetNumberOfStates() const;
-
-  /// Restore the TTY state for state at index \a idx.
-  ///
-  /// \return
-  /// Returns \b true if the TTY state was successfully restored,
-  /// \b false otherwise.
-  bool Restore(uint32_t idx) const;
-
-  /// Save the TTY state information for the state at index \a idx. The TTY
-  /// state is saved for the file descriptor \a fd and the process group
-  /// information will also be saved if requested by \a save_process_group.
-  ///
-  /// \param[in] idx
-  /// The index into the state array where the state should be
-  /// saved.
-  ///
-  /// \param[in] fd
-  /// The file descriptor for which to save the settings.
-  ///
-  /// \param[in] save_process_group
-  /// If \b true, save the process group information for the TTY.
-  ///
-  /// \return
-  /// Returns \b true if the save was successful, \b false
-  /// otherwise.
-  bool Save(uint32_t idx, int fd, bool save_process_group);
-
-protected:
-  // Member variables
-  mutable uint32_t m_currentState =
-  UINT32_MAX; ///< The currently active TTY state index.
-  TerminalState
-  m_ttystates[2]; ///< The array of TTY states that holds saved TTY info.
-};
-
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110693: [lldb] [Host] Remove TerminalStateSwitcher

[Pavel Labath via Phabricator via lldb-commits]

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

yay


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

https://reviews.llvm.org/D110693

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

[Lldb-commits] [PATCH] D110693: [lldb] [Host] Remove TerminalStateSwitcher

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.

LGTM


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

https://reviews.llvm.org/D110693

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

[Lldb-commits] [lldb] 52b04ef - [lldb] [Host] Remove TerminalStateSwitcher

[Michał Górny via lldb-commits]

Author: Michał Górny
Date: 2021-09-29T13:45:41+02:00
New Revision: 52b04efa0197d5b1f65f4824a6528ea634bdf3db

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

LOG: [lldb] [Host] Remove TerminalStateSwitcher

Remove TerminalStateSwitcher class.  It is not used anywhere and its API
is really weird.  This is the first step towards cleaning up Terminal.h.

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

Added: 


Modified: 
lldb/include/lldb/Host/Terminal.h
lldb/source/Host/common/Terminal.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/Terminal.h 
b/lldb/include/lldb/Host/Terminal.h
index ca91d6b59720a..b22b96776c335 100644
--- a/lldb/include/lldb/Host/Terminal.h
+++ b/lldb/include/lldb/Host/Terminal.h
@@ -124,59 +124,6 @@ class TerminalState {
   lldb::pid_t m_process_group = -1; ///< Cached process group information.
 };
 
-/// \class TerminalStateSwitcher Terminal.h "lldb/Host/Terminal.h"
-/// A TTY state switching class.
-///
-/// This class can be used to remember 2 TTY states for a given file
-/// descriptor and switch between the two states.
-class TerminalStateSwitcher {
-public:
-  /// Constructor
-  TerminalStateSwitcher();
-
-  /// Destructor
-  ~TerminalStateSwitcher();
-
-  /// Get the number of possible states to save.
-  ///
-  /// \return
-  /// The number of states that this TTY switcher object contains.
-  uint32_t GetNumberOfStates() const;
-
-  /// Restore the TTY state for state at index \a idx.
-  ///
-  /// \return
-  /// Returns \b true if the TTY state was successfully restored,
-  /// \b false otherwise.
-  bool Restore(uint32_t idx) const;
-
-  /// Save the TTY state information for the state at index \a idx. The TTY
-  /// state is saved for the file descriptor \a fd and the process group
-  /// information will also be saved if requested by \a save_process_group.
-  ///
-  /// \param[in] idx
-  /// The index into the state array where the state should be
-  /// saved.
-  ///
-  /// \param[in] fd
-  /// The file descriptor for which to save the settings.
-  ///
-  /// \param[in] save_process_group
-  /// If \b true, save the process group information for the TTY.
-  ///
-  /// \return
-  /// Returns \b true if the save was successful, \b false
-  /// otherwise.
-  bool Save(uint32_t idx, int fd, bool save_process_group);
-
-protected:
-  // Member variables
-  mutable uint32_t m_currentState =
-  UINT32_MAX; ///< The currently active TTY state index.
-  TerminalState
-  m_ttystates[2]; ///< The array of TTY states that holds saved TTY info.
-};
-
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)

diff  --git a/lldb/source/Host/common/Terminal.cpp 
b/lldb/source/Host/common/Terminal.cpp
index 2301abe9afb1d..147bbdcde137d 100644
--- a/lldb/source/Host/common/Terminal.cpp
+++ b/lldb/source/Host/common/Terminal.cpp
@@ -188,50 +188,3 @@ bool TerminalState::TTYStateIsValid() const {
 bool TerminalState::ProcessGroupIsValid() const {
   return static_cast(m_process_group) != -1;
 }
-
-// Constructor
-TerminalStateSwitcher::TerminalStateSwitcher() = default;
-
-// Destructor
-TerminalStateSwitcher::~TerminalStateSwitcher() = default;
-
-// Returns the number of states that this switcher contains
-uint32_t TerminalStateSwitcher::GetNumberOfStates() const {
-  return llvm::array_lengthof(m_ttystates);
-}
-
-// Restore the state at index "idx".
-//
-// Returns true if the restore was successful, false otherwise.
-bool TerminalStateSwitcher::Restore(uint32_t idx) const {
-  const uint32_t num_states = GetNumberOfStates();
-  if (idx >= num_states)
-return false;
-
-  // See if we already are in this state?
-  if (m_currentState < num_states && (idx == m_currentState) &&
-  m_ttystates[idx].IsValid())
-return true;
-
-  // Set the state to match the index passed in and only update the current
-  // state if there are no errors.
-  if (m_ttystates[idx].Restore()) {
-m_currentState = idx;
-return true;
-  }
-
-  // We failed to set the state. The tty state was invalid or not initialized.
-  return false;
-}
-
-// Save the state at index "idx" for file descriptor "fd" and save the process
-// group if requested.
-//
-// Returns true if the restore was successful, false otherwise.
-bool TerminalStateSwitcher::Save(uint32_t idx, int fd,
- bool save_process_group) {
-  const uint32_t num_states = GetNumberOfStates();
-  if (idx < num_states)
-return m_ttystates[idx].Save(fd, save_process_group);
-  return false;
-}



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

[Lldb-commits] [PATCH] D110693: [lldb] [Host] Remove TerminalStateSwitcher

[Michał Górny via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG52b04efa0197: [lldb] [Host] Remove TerminalStateSwitcher 
(authored by mgorny).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110693

Files:
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Host/common/Terminal.cpp

Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -188,50 +188,3 @@
 bool TerminalState::ProcessGroupIsValid() const {
   return static_cast(m_process_group) != -1;
 }
-
-// Constructor
-TerminalStateSwitcher::TerminalStateSwitcher() = default;
-
-// Destructor
-TerminalStateSwitcher::~TerminalStateSwitcher() = default;
-
-// Returns the number of states that this switcher contains
-uint32_t TerminalStateSwitcher::GetNumberOfStates() const {
-  return llvm::array_lengthof(m_ttystates);
-}
-
-// Restore the state at index "idx".
-//
-// Returns true if the restore was successful, false otherwise.
-bool TerminalStateSwitcher::Restore(uint32_t idx) const {
-  const uint32_t num_states = GetNumberOfStates();
-  if (idx >= num_states)
-return false;
-
-  // See if we already are in this state?
-  if (m_currentState < num_states && (idx == m_currentState) &&
-  m_ttystates[idx].IsValid())
-return true;
-
-  // Set the state to match the index passed in and only update the current
-  // state if there are no errors.
-  if (m_ttystates[idx].Restore()) {
-m_currentState = idx;
-return true;
-  }
-
-  // We failed to set the state. The tty state was invalid or not initialized.
-  return false;
-}
-
-// Save the state at index "idx" for file descriptor "fd" and save the process
-// group if requested.
-//
-// Returns true if the restore was successful, false otherwise.
-bool TerminalStateSwitcher::Save(uint32_t idx, int fd,
- bool save_process_group) {
-  const uint32_t num_states = GetNumberOfStates();
-  if (idx < num_states)
-return m_ttystates[idx].Save(fd, save_process_group);
-  return false;
-}
Index: lldb/include/lldb/Host/Terminal.h
===
--- lldb/include/lldb/Host/Terminal.h
+++ lldb/include/lldb/Host/Terminal.h
@@ -124,59 +124,6 @@
   lldb::pid_t m_process_group = -1; ///< Cached process group information.
 };
 
-/// \class TerminalStateSwitcher Terminal.h "lldb/Host/Terminal.h"
-/// A TTY state switching class.
-///
-/// This class can be used to remember 2 TTY states for a given file
-/// descriptor and switch between the two states.
-class TerminalStateSwitcher {
-public:
-  /// Constructor
-  TerminalStateSwitcher();
-
-  /// Destructor
-  ~TerminalStateSwitcher();
-
-  /// Get the number of possible states to save.
-  ///
-  /// \return
-  /// The number of states that this TTY switcher object contains.
-  uint32_t GetNumberOfStates() const;
-
-  /// Restore the TTY state for state at index \a idx.
-  ///
-  /// \return
-  /// Returns \b true if the TTY state was successfully restored,
-  /// \b false otherwise.
-  bool Restore(uint32_t idx) const;
-
-  /// Save the TTY state information for the state at index \a idx. The TTY
-  /// state is saved for the file descriptor \a fd and the process group
-  /// information will also be saved if requested by \a save_process_group.
-  ///
-  /// \param[in] idx
-  /// The index into the state array where the state should be
-  /// saved.
-  ///
-  /// \param[in] fd
-  /// The file descriptor for which to save the settings.
-  ///
-  /// \param[in] save_process_group
-  /// If \b true, save the process group information for the TTY.
-  ///
-  /// \return
-  /// Returns \b true if the save was successful, \b false
-  /// otherwise.
-  bool Save(uint32_t idx, int fd, bool save_process_group);
-
-protected:
-  // Member variables
-  mutable uint32_t m_currentState =
-  UINT32_MAX; ///< The currently active TTY state index.
-  TerminalState
-  m_ttystates[2]; ///< The array of TTY states that holds saved TTY info.
-};
-
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110535: [llvm] [ADT] Update llvm::Split() per Pavel Labath's suggestions

[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, but I'd give @joerg a chance to respond first.

In D110535#3028450 , @joerg wrote:

> Why are all the changes from separator character to separator string 
> necessary or desirable?

There's no good place to store the separator character. The functional version 
of split is implemented by creating a `StringRef(&the_char, 1)`, but that won't 
work in the iterator version. Keeping it doesn't seem to be worth the trouble, 
as it does not have any performance benefits,  and doesn't make the callsites 
more complicated.


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

https://reviews.llvm.org/D110535

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Pavel Labath via Phabricator via lldb-commits]

labath added reviewers: clayborg, shafik.
labath added a comment.

In D110571#3027283 , @jarin wrote:

> In D110571#3027173 , @labath wrote:
>
>> Judging by your description, I take it you parse these variables only once, 
>> regardless of how many functions they are inlined in. Could we fix that my 
>> creating a fresh variable object for each inlined instance? Then it could 
>> maybe be correctly made to point to the actual block and function it is 
>> inlined into(?)
>
> Yes, they are parsed only once. This is because there is a DIE->Variable map 
> (see SymbolFileDWARF::GetDIEToVariable) that makes sure no DIE gets parsed 
> twice. Are you suggesting to index the map with a pair 
> ? That would indeed create healthier 
> structure (even though I could not spot any problems even with my current 
> somewhat flawed approach).

I am not really suggesting anything (at least not yet). I'm just trying to map 
out the problem space. Having separate variables could be nice, but it could 
also be a needless complication. I've added some people to see what they make 
of this.

>> After that, we could think about adding a c++ test case. Although tests with 
>> optimized code are tricky, it is often possible (with judicious use of 
>> noinline, always_inline and optnone attributes) to constrain the optimizer 
>> in a way that it has no choice but to do exactly what we want.
>
> I have actually use the attributes when experimenting with the patch - if you 
> think this is useful, I can certainly provide those tests.

If you have something ready, feel free to include it and we can see what to do 
then. If you don't have them, maybe wait and see how the other approach pans 
out first...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 375865.
jarin added a comment.

Rewrote the recursive parser to use a plain method.

Pruned and annotated the test.

Added other test cases:

- all parameters unused,
- inlining from two different functions,
- stack trace.

This still uses `frame variable` rather than `image lookup`, mostly because 
`frame variable` tests better the user experience and the cognitive overhead 
for making the code runnable does not seem to be too high.


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

https://reviews.llvm.org/D110571

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
  lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

Index: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
@@ -0,0 +1,69 @@
+# UNSUPPORTED: system-darwin, system-windows
+
+# REQUIRES: lld
+
+# RUN: llvm-mc -filetype=obj %S/Inputs/unused-inlined-params.s \
+# RUN: -triple x86_64-pc-linux -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+
+# In this test we verify that inlined functions still mention
+# all their parameters in `frame variable`, even when those
+# parameters were completely optimized away from the concrete
+# instance of the inlined function in the debug info.
+# The debugger should look up the parameters in the abstract
+# origin of the concrete instance.
+
+breakpoint set -n main
+process launch
+# CHECK: Process {{.*}} stopped
+
+# Let us check that unused parameters of an inlined function are listed
+# at the inlined function entry.
+breakpoint set -n break_at_inlined_f_in_main
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 42
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = 1
+# CHECK: (int) unused3 = <
+
+# Step out of the live range of the 'partial' parameter and verify that
+# all variables are still displayed.
+breakpoint set -n break_at_inlined_f_in_main_between_printfs
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 43
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = <
+# CHECK: (int) unused3 = <
+
+# Check that we show parameters even if all of them are compiled away.
+breakpoint set -n break_at_inlined_g_in_main
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (int) unused = <
+
+# Check that even the other inlined instance of f displays correct
+# parameters.
+breakpoint set -n break_at_inlined_f_in_other
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 1
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = 2
+# CHECK: (int) unused3 = <
+
+# Check that the stack trace contains the inlined function with
+# all the arguments
+thread backtrace
+CHECK: other [inlined] f(unused1=, used=1, unused2=, \
+partial=2, unused3=)
\ No newline at end of file
Index: lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
@@ -0,0 +1,458 @@
+# The below program is roughly derived from the following C program.
+# To see the annotated debug info, look for the section 
+# '.section.debug_info' below.
+#
+# __attribute__((always_inline))
+# void f(void* unused1, int used, int unused2, int partial, int unused3) {
+#   used += partial;
+#   printf("f %i", partial);
+#   printf("f %i", used);   // |partial| is not live at this line.
+# }
+#
+# void g(int unused) {
+#   printf("Hello");
+# }
+#
+# __attribute__((noinline))
+# void other() {
+#   f(nullptr, 1, 0, 2, 0);
+# }
+#
+# int main(int argc, char** argv) {
+#   f(argv, 42, 1, argc, 2);
+#   g(1);
+#   other();
+#   return 0;
+# }
+
+.text
+.file"unused-inlined-params.c"
+
+.Lcu_begin:
+
+.globlother
+other:
+nop
+.Linlined_f_in_other:
+break_at_inlined_f_in_other:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_between_printfs:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_end:
+retq
+.Lother_end:
+.sizeother, .Lother_end-other
+
+.globlmain
+main:
+.file1 "/example" "unused-inlined-params.c"
+movl$1, %esi
+.Linlined_f:
+break_at_inlined_f_in_main:
+leal42(%rsi), %ebx
+.Linlined_f_before_printf:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_between_printfs:
+break_at_inlined_f_in_main_between_printfs:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_end:
+.Linlined_g:
+break_at_inlined_g_in_main:
+ 

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D110571#3030192 , @jarin wrote:

> This still uses `frame variable` rather than `image lookup`, mostly because 
> `frame variable` tests better the user experience and the cognitive overhead 
> for making the code runnable does not seem to be too high.

This is not really about "cognitive overhead", but "who can run this test". 
With a running process the answer is "a person with a linux x86 machine". With 
image lookup it's "everyone". It's also easier to debug failures in the test, 
as less code gets run before you get to the interesting part.


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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 375886.
jarin added a comment.

Added a C test (I have also verified that the C test fails without the 
SymbolFileDWARF patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/API/functionalities/unused-inlined-parameters/Makefile
  
lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py
  lldb/test/API/functionalities/unused-inlined-parameters/main.c
  lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
  lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

Index: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
@@ -0,0 +1,69 @@
+# UNSUPPORTED: system-darwin, system-windows
+
+# REQUIRES: lld
+
+# RUN: llvm-mc -filetype=obj %S/Inputs/unused-inlined-params.s \
+# RUN: -triple x86_64-pc-linux -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+
+# In this test we verify that inlined functions still mention
+# all their parameters in `frame variable`, even when those
+# parameters were completely optimized away from the concrete
+# instance of the inlined function in the debug info.
+# The debugger should look up the parameters in the abstract
+# origin of the concrete instance.
+
+breakpoint set -n main
+process launch
+# CHECK: Process {{.*}} stopped
+
+# Let us check that unused parameters of an inlined function are listed
+# at the inlined function entry.
+breakpoint set -n break_at_inlined_f_in_main
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 42
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = 1
+# CHECK: (int) unused3 = <
+
+# Step out of the live range of the 'partial' parameter and verify that
+# all variables are still displayed.
+breakpoint set -n break_at_inlined_f_in_main_between_printfs
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 43
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = <
+# CHECK: (int) unused3 = <
+
+# Check that we show parameters even if all of them are compiled away.
+breakpoint set -n break_at_inlined_g_in_main
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (int) unused = <
+
+# Check that even the other inlined instance of f displays correct
+# parameters.
+breakpoint set -n break_at_inlined_f_in_other
+continue
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 1
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = 2
+# CHECK: (int) unused3 = <
+
+# Check that the stack trace contains the inlined function with
+# all the arguments
+thread backtrace
+CHECK: other [inlined] f(unused1=, used=1, unused2=, \
+partial=2, unused3=)
\ No newline at end of file
Index: lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
@@ -0,0 +1,458 @@
+# The below program is roughly derived from the following C program.
+# To see the annotated debug info, look for the section 
+# '.section.debug_info' below.
+#
+# __attribute__((always_inline))
+# void f(void* unused1, int used, int unused2, int partial, int unused3) {
+#   used += partial;
+#   printf("f %i", partial);
+#   printf("f %i", used);   // |partial| is not live at this line.
+# }
+#
+# void g(int unused) {
+#   printf("Hello");
+# }
+#
+# __attribute__((noinline))
+# void other() {
+#   f(nullptr, 1, 0, 2, 0);
+# }
+#
+# int main(int argc, char** argv) {
+#   f(argv, 42, 1, argc, 2);
+#   g(1);
+#   other();
+#   return 0;
+# }
+
+.text
+.file"unused-inlined-params.c"
+
+.Lcu_begin:
+
+.globlother
+other:
+nop
+.Linlined_f_in_other:
+break_at_inlined_f_in_other:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_between_printfs:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_end:
+retq
+.Lother_end:
+.sizeother, .Lother_end-other
+
+.globlmain
+main:
+.file1 "/example" "unused-inlined-params.c"
+movl$1, %esi
+.Linlined_f:
+break_at_inlined_f_in_main:
+leal42(%rsi), %ebx
+.Linlined_f_before_printf:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_between_printfs:
+break_at_inlined_f_in_main_between_printfs:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_end:
+.Linlined_g:
+break_at_inlined_g_in_main:
+callqprintf# Omitted the setup of 

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

In D110571#3030222 , @labath wrote:

> In D110571#3030192 , @jarin wrote:
>
>> This still uses `frame variable` rather than `image lookup`, mostly because 
>> `frame variable` tests better the user experience and the cognitive overhead 
>> for making the code runnable does not seem to be too high.
>
> This is not really about "cognitive overhead", but "who can run this test". 
> With a running process the answer is "a person with a linux x86 machine". 
> With image lookup it's "everyone". It's also easier to debug failures in the 
> test, as less code gets run before you get to the interesting part.

Good point. Would you prefer to recast the test in terms of image lookup and 
get rid of checking the stack trace?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [lldb] f939a32 - [lldb] Fix TestImportStdModule on some setups by testing minmax instead of abs

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-09-29T17:03:37+02:00
New Revision: f939a32e5c483686af16561211d77c06a5579011

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

LOG: [lldb] Fix TestImportStdModule on some setups by testing minmax instead of 
abs

Some downstream forks of LLDB change parts of the test setup in a way that
causes lldb to somehow resolve `std::abs` (probably to `::abs`). This patch
changes the tested function here to be `std::minmax` which (hopefully) doesn't
have any identically named functions that LLDB could find and call. Just to be
extra safe this also explicitly specified the template arguments so that in
case there is a `minmax` non-template function we still don't end up calling it
from this test.

Added: 


Modified: 

lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py

Removed: 




diff  --git 
a/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py
 
b/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py
index c60e280dd65d6..695d00b08a0f5 100644
--- 
a/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py
+++ 
b/lldb/test/API/commands/expression/import-std-module/basic/TestImportStdModule.py
@@ -24,6 +24,8 @@ def test(self):
 self.runCmd("settings set target.import-std-module true")
 # Calling some normal std functions that return non-template types.
 self.expect_expr("std::abs(-42)", result_type="int", result_value="42")
+self.expect_expr("std::minmax(1, 2).first", result_type="const 
int",
+result_value="1")
 self.expect_expr("std::div(2, 1).quot",
  result_type="int",
  result_value="2")
@@ -50,7 +52,7 @@ def test_non_cpp_language(self):
 self.runCmd("settings set target.import-std-module true")
 # These languages don't support C++ modules, so they shouldn't
 # be able to evaluate the expression.
-self.expect("expr -l C -- std::abs(-42)", error=True)
-self.expect("expr -l C99 -- std::abs(-42)", error=True)
-self.expect("expr -l C11 -- std::abs(-42)", error=True)
-self.expect("expr -l ObjC -- std::abs(-42)", error=True)
+self.expect("expr -l C -- std::minmax(1, 2).first", error=True)
+self.expect("expr -l C99 -- std::minmax(1, 2).first", error=True)
+self.expect("expr -l C11 -- std::minmax(1, 2).first", error=True)
+self.expect("expr -l ObjC -- std::minmax(1, 2).first", error=True)



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

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Merge TerminalState into Terminal

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, teemperor, emaste, krytarowski.
mgorny requested review of this revision.

Merge the routines from the TerminalState class into Terminal.  There
does not seem to be a real advantage from having the two classes split,
and the TerminalState class has a weird API.  It uses the Terminal class
internally but neither accepts its instance as an argument, nor exposes
the internal instance to the caller.  As a result, some callers ended up
combining Terminal and TerminalState classes (effectively having two
Terminal classes).


https://reviews.llvm.org/D110721

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Core/Debugger.cpp
  lldb/source/Host/common/Terminal.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -4351,9 +4351,8 @@
 
 SetIsDone(false);
 const int read_fd = m_read_file.GetDescriptor();
-TerminalState terminal_state;
-terminal_state.Save(read_fd, false);
 Terminal terminal(read_fd);
+terminal.SaveState(false);
 terminal.SetCanonical(false);
 terminal.SetEcho(false);
 // FD_ZERO, FD_SET are not supported on windows
@@ -4399,7 +4398,7 @@
 }
 m_is_running = false;
 #endif
-terminal_state.Restore();
+terminal.RestoreState();
   }
 
   void Cancel() override {
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -430,11 +430,10 @@
   int stdin_fd = GetInputFD();
   if (stdin_fd >= 0) {
 Terminal terminal(stdin_fd);
-TerminalState terminal_state;
 const bool is_a_tty = terminal.IsATerminal();
 
 if (is_a_tty) {
-  terminal_state.Save(stdin_fd, false);
+  terminal.SaveState(false);
   terminal.SetCanonical(false);
   terminal.SetEcho(true);
 }
@@ -466,7 +465,7 @@
 PyRun_SimpleString(run_string.GetData());
 
 if (is_a_tty)
-  terminal_state.Restore();
+  terminal.RestoreState();
   }
 }
 SetIsDone(true);
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -355,7 +355,6 @@
 PyEval_InitThreads();
   }
 
-  TerminalState m_stdin_tty_state;
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;
   bool m_was_already_initialized = false;
 };
Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -81,21 +81,7 @@
   return false;
 }
 
-// Default constructor
-TerminalState::TerminalState()
-: m_tty()
-#if LLDB_ENABLE_TERMIOS
-  ,
-  m_termios_up()
-#endif
-{
-}
-
-// Destructor
-TerminalState::~TerminalState() = default;
-
-void TerminalState::Clear() {
-  m_tty.Clear();
+void Terminal::ClearSavedState() {
   m_tflags = -1;
 #if LLDB_ENABLE_TERMIOS
   m_termios_up.reset();
@@ -103,51 +89,43 @@
   m_process_group = -1;
 }
 
-// Save the current state of the TTY for the file descriptor "fd" and if
-// "save_process_group" is true, attempt to save the process group info for the
-// TTY.
-bool TerminalState::Save(int fd, bool save_process_group) {
-  m_tty.SetFileDescriptor(fd);
-  if (m_tty.IsATerminal()) {
+bool Terminal::SaveState(bool save_process_group) {
+  if (IsATerminal()) {
 #if LLDB_ENABLE_POSIX
-m_tflags = ::fcntl(fd, F_GETFL, 0);
+m_tflags = ::fcntl(m_fd, F_GETFL, 0);
 #endif
 #if LLDB_ENABLE_TERMIOS
 if (m_termios_up == nullptr)
   m_termios_up.reset(new struct termios);
-int err = ::tcgetattr(fd, m_termios_up.get());
+int err = ::tcgetattr(m_fd, m_termios_up.get());
 if (err != 0)
   m_termios_up.reset();
 #endif // #if LLDB_ENABLE_TERMIOS
 #if LLDB_ENABLE_POSIX
 if (save_process_group)
-  m_process_group = ::tcgetpgrp(0);
+  m_process_group = ::tcgetpgrp(m_fd);
 else
   m_process_group = -1;
 #endif
   } else {
-m_tty.Clear();
 m_tflags = -1;
 #if LLDB_ENABLE_TERMIOS
 m_termios_up.reset();
 #endif
 m_process_group = -1;
   }
-  return IsValid();
+  return IsStateValid();
 }
 
-// Restore the state of the TTY using the cached values from a previous call to
-// Save().
-bool TerminalState::Restore() 

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Merge TerminalState into Terminal

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: lldb/source/Host/common/Terminal.cpp:124
 if (save_process_group)
-  m_process_group = ::tcgetpgrp(0);
 else

I think passing `0` here was a mistake. The `Restore()` method passed `fd` 
instead.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:358
 
-  TerminalState m_stdin_tty_state;
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;

This instance didn't seem to be used at all.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:468
 if (is_a_tty)
-  terminal_state.Restore();
+  terminal.RestoreState();
   }

I'm wondering if we could use the RAII approach here and restore state (if 
saved) in the destructor. I'm just not 100% sure if that's what `Debugger` 
expects.


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

https://reviews.llvm.org/D110721

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 375901.
jarin added a comment.

Changed the test to avoid running the process and use `image lookup` instead of 
`frame variable`.

I think I would still slightly prefer `frame variable`, mostly because there 
seem to be differences between what `image lookup` and `frame variable` show 
(`image lookup` omit variables that have DW_AT_location disjoint from the 
inspected address). As opposed to `image lookup`, `frame variable` tests more 
directly what the users would actually use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/test/API/functionalities/unused-inlined-parameters/Makefile
  
lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py
  lldb/test/API/functionalities/unused-inlined-parameters/main.c
  lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
  lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

Index: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
@@ -0,0 +1,49 @@
+# UNSUPPORTED: system-darwin, system-windows
+
+# RUN: llvm-mc -filetype=obj %S/Inputs/unused-inlined-params.s \
+# RUN: -triple x86_64-pc-linux -o %t.o
+# RUN: %lldb %t.o -s %s -o exit | FileCheck %s
+
+
+# In this test we verify that inlined functions still mention
+# all their parameters in `frame variable`, even when those
+# parameters were completely optimized away from the concrete
+# instance of the inlined function in the debug info.
+# The debugger should look up the parameters in the abstract
+# origin of the concrete instance.
+
+# Let us check that unused parameters of an inlined function are listed
+# at the inlined function entry.
+image lookup -v -s break_at_inlined_f_in_main
+# CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main
+# CHECK: name = "unused1", type = "void *", location = 
+# CHECK: name = "used", type = "int", location = DW_OP_consts +42
+# CHECK: name = "unused2", type = "int", location = 
+# CHECK: name = "partial", type = "int", location = DW_OP_reg4 RSI
+# CHECK: name = "unused3", type = "int", location = 
+
+# Show variables outsid of the live range of the 'partial' parameter
+# and verify that the output is as expected.
+image lookup -v -s break_at_inlined_f_in_main_between_printfs
+# CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main_between_printfs
+# CHECK: name = "unused1", type = "void *", location = 
+# CHECK: name = "used", type = "int", location = DW_OP_reg3 RBX
+# CHECK: name = "unused2", type = "int", location = 
+# Note: image lookup does not show variables outside of their
+#   location, so |partial| is missing here.
+# CHECK: name = "unused3", type = "int", location = 
+
+# Check that we show parameters even if all of them are compiled away.
+image lookup -v -s  break_at_inlined_g_in_main
+# CHECK-LABEL: image lookup -v -s  break_at_inlined_g_in_main
+# CHECK: name = "unused", type = "int", location = 
+
+# Check that even the other inlined instance of f displays the correct
+# parameters.
+image lookup -v -s  break_at_inlined_f_in_other
+# CHECK-LABEL: image lookup -v -s  break_at_inlined_f_in_other
+# CHECK: name = "unused1", type = "void *", location = 
+# CHECK: name = "used", type = "int", location = DW_OP_consts +1
+# CHECK: name = "unused2", type = "int", location = 
+# CHECK: name = "partial", type = "int", location =  DW_OP_consts +2
+# CHECK: name = "unused3", type = "int", location = 
\ No newline at end of file
Index: lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s
@@ -0,0 +1,458 @@
+# The below program is roughly derived from the following C program.
+# To see the annotated debug info, look for the section 
+# '.section.debug_info' below.
+#
+# __attribute__((always_inline))
+# void f(void* unused1, int used, int unused2, int partial, int unused3) {
+#   used += partial;
+#   printf("f %i", partial);
+#   printf("f %i", used);   // |partial| is not live at this line.
+# }
+#
+# void g(int unused) {
+#   printf("Hello");
+# }
+#
+# __attribute__((noinline))
+# void other() {
+#   f(nullptr, 1, 0, 2, 0);
+# }
+#
+# int main(int argc, char** argv) {
+#   f(argv, 42, 1, argc, 2);
+#   g(1);
+#   other();
+#   return 0;
+# }
+
+.text
+.file"unused-inlined-params.c"
+
+.Lcu_begin:
+
+.globlother
+other:
+nop
+.Linlined_f_in_other:
+break_at_inlined_f_in_other:
+callqprintf# Omitted the setup of arguments.
+.Linlined_f_in_other_between_printfs:
+call

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 375908.
mgorny retitled this revision from "[lldb] [Host] Merge TerminalState into 
Terminal" to "[lldb] [Host] Refactor TerminalState [WIP]".
mgorny edited the summary of this revision.
mgorny added a comment.

Ok, let's start with smaller refactoring and see where it gets us.


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

https://reviews.llvm.org/D110721

Files:
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Host/common/Terminal.cpp

Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -15,10 +15,6 @@
 #include 
 #include 
 
-#if LLDB_ENABLE_TERMIOS
-#include 
-#endif
-
 using namespace lldb_private;
 
 bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
@@ -81,25 +77,10 @@
   return false;
 }
 
-// Default constructor
-TerminalState::TerminalState()
-: m_tty()
-#if LLDB_ENABLE_TERMIOS
-  ,
-  m_termios_up()
-#endif
-{
-}
-
-// Destructor
-TerminalState::~TerminalState() = default;
-
 void TerminalState::Clear() {
   m_tty.Clear();
   m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-  m_termios_up.reset();
-#endif
+  m_termios_saved = false;
   m_process_group = -1;
 }
 
@@ -107,31 +88,18 @@
 // "save_process_group" is true, attempt to save the process group info for the
 // TTY.
 bool TerminalState::Save(int fd, bool save_process_group) {
+  Clear();
   m_tty.SetFileDescriptor(fd);
   if (m_tty.IsATerminal()) {
 #if LLDB_ENABLE_POSIX
 m_tflags = ::fcntl(fd, F_GETFL, 0);
-#endif
 #if LLDB_ENABLE_TERMIOS
-if (m_termios_up == nullptr)
-  m_termios_up.reset(new struct termios);
-int err = ::tcgetattr(fd, m_termios_up.get());
-if (err != 0)
-  m_termios_up.reset();
-#endif // #if LLDB_ENABLE_TERMIOS
-#if LLDB_ENABLE_POSIX
+if (::tcgetattr(fd, &m_termios) == 0)
+  m_termios_saved = true;
+#endif // LLDB_ENABLE_TERMIOS
 if (save_process_group)
   m_process_group = ::tcgetpgrp(0);
-else
-  m_process_group = -1;
-#endif
-  } else {
-m_tty.Clear();
-m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-m_termios_up.reset();
-#endif
-m_process_group = -1;
+#endif // LLDB_ENABLE_POSIX
   }
   return IsValid();
 }
@@ -147,8 +115,8 @@
 
 #if LLDB_ENABLE_TERMIOS
 if (TTYStateIsValid())
-  tcsetattr(fd, TCSANOW, m_termios_up.get());
-#endif // #if LLDB_ENABLE_TERMIOS
+  tcsetattr(fd, TCSANOW, &m_termios);
+#endif // LLDB_ENABLE_TERMIOS
 
 if (ProcessGroupIsValid()) {
   // Save the original signal handler.
@@ -161,7 +129,7 @@
 }
 return true;
   }
-#endif
+#endif // LLDB_ENABLE_POSIX
   return false;
 }
 
@@ -176,13 +144,7 @@
 bool TerminalState::TFlagsIsValid() const { return m_tflags != -1; }
 
 // Returns true if m_ttystate is valid
-bool TerminalState::TTYStateIsValid() const {
-#if LLDB_ENABLE_TERMIOS
-  return m_termios_up != nullptr;
-#else
-  return false;
-#endif
-}
+bool TerminalState::TTYStateIsValid() const { return m_termios_saved; }
 
 // Returns true if m_process_group is valid
 bool TerminalState::ProcessGroupIsValid() const {
Index: lldb/include/lldb/Host/Terminal.h
===
--- lldb/include/lldb/Host/Terminal.h
+++ lldb/include/lldb/Host/Terminal.h
@@ -13,6 +13,10 @@
 #include "lldb/Host/Config.h"
 #include "lldb/lldb-private.h"
 
+#if LLDB_ENABLE_TERMIOS
+#include 
+#endif
+
 struct termios;
 
 namespace lldb_private {
@@ -48,12 +52,6 @@
 /// descriptor and later restore that state as it originally was.
 class TerminalState {
 public:
-  /// Default constructor
-  TerminalState();
-
-  /// Destructor
-  ~TerminalState();
-
   /// Save the TTY state for \a fd.
   ///
   /// Save the current state of the TTY for the file descriptor "fd" and if
@@ -115,11 +113,11 @@
   bool ProcessGroupIsValid() const;
 
   // Member variables
-  Terminal m_tty; ///< A terminal
-  int m_tflags = -1; ///< Cached tflags information.
+  Terminal m_tty;   ///< A terminal
+  int m_tflags = -1;///< Cached tflags information.
+  bool m_termios_saved = false; ///< Indication whether termios was saved.
 #if LLDB_ENABLE_TERMIOS
-  std::unique_ptr
-  m_termios_up; ///< Cached terminal state information.
+  struct termios m_termios; ///< Cached terminal state information.
 #endif
   lldb::pid_t m_process_group = -1; ///< Cached process group information.
 };
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 375919.
mgorny edited the summary of this revision.

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

https://reviews.llvm.org/D110721

Files:
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Host/common/Terminal.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -4351,9 +4351,8 @@
 
 SetIsDone(false);
 const int read_fd = m_read_file.GetDescriptor();
-TerminalState terminal_state;
-terminal_state.Save(read_fd, false);
 Terminal terminal(read_fd);
+TerminalState terminal_state(read_fd, false);
 terminal.SetCanonical(false);
 terminal.SetEcho(false);
 // FD_ZERO, FD_SET are not supported on windows
@@ -4399,7 +4398,6 @@
 }
 m_is_running = false;
 #endif
-terminal_state.Restore();
   }
 
   void Cancel() override {
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -430,11 +430,9 @@
   int stdin_fd = GetInputFD();
   if (stdin_fd >= 0) {
 Terminal terminal(stdin_fd);
-TerminalState terminal_state;
-const bool is_a_tty = terminal.IsATerminal();
+TerminalState terminal_state(stdin_fd);
 
-if (is_a_tty) {
-  terminal_state.Save(stdin_fd, false);
+if (terminal.IsATerminal()) {
   terminal.SetCanonical(false);
   terminal.SetEcho(true);
 }
@@ -464,9 +462,6 @@
 run_string.Printf("run_python_interpreter (%s)",
   m_python->GetDictionaryName());
 PyRun_SimpleString(run_string.GetData());
-
-if (is_a_tty)
-  terminal_state.Restore();
   }
 }
 SetIsDone(true);
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -355,7 +355,6 @@
 PyEval_InitThreads();
   }
 
-  TerminalState m_stdin_tty_state;
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;
   bool m_was_already_initialized = false;
 };
Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -15,10 +15,6 @@
 #include 
 #include 
 
-#if LLDB_ENABLE_TERMIOS
-#include 
-#endif
-
 using namespace lldb_private;
 
 bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
@@ -81,63 +77,39 @@
   return false;
 }
 
-// Default constructor
-TerminalState::TerminalState()
-: m_tty()
-#if LLDB_ENABLE_TERMIOS
-  ,
-  m_termios_up()
-#endif
-{
+TerminalState::TerminalState(int fd, bool save_process_group) : m_tty(fd) {
+  if (fd != -1)
+Save(fd, save_process_group);
 }
 
-// Destructor
-TerminalState::~TerminalState() = default;
+TerminalState::~TerminalState() {
+  Restore();
+}
 
 void TerminalState::Clear() {
   m_tty.Clear();
   m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-  m_termios_up.reset();
-#endif
+  m_termios_saved = false;
   m_process_group = -1;
 }
 
-// Save the current state of the TTY for the file descriptor "fd" and if
-// "save_process_group" is true, attempt to save the process group info for the
-// TTY.
 bool TerminalState::Save(int fd, bool save_process_group) {
+  Clear();
   m_tty.SetFileDescriptor(fd);
   if (m_tty.IsATerminal()) {
 #if LLDB_ENABLE_POSIX
 m_tflags = ::fcntl(fd, F_GETFL, 0);
-#endif
 #if LLDB_ENABLE_TERMIOS
-if (m_termios_up == nullptr)
-  m_termios_up.reset(new struct termios);
-int err = ::tcgetattr(fd, m_termios_up.get());
-if (err != 0)
-  m_termios_up.reset();
-#endif // #if LLDB_ENABLE_TERMIOS
-#if LLDB_ENABLE_POSIX
+if (::tcgetattr(fd, &m_termios) == 0)
+  m_termios_saved = true;
+#endif // LLDB_ENABLE_TERMIOS
 if (save_process_group)
   m_process_group = ::tcgetpgrp(0);
-else
-  m_process_group = -1;
-#endif
-  } else {
-m_tty.Clear();
-m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-m_termios_up.reset();
-#endif
-m_process_group = -1;
+#endif // LLDB_ENABLE_POSIX
   }
   return IsValid();
 }
 
-// Restore the state of the TTY using the cached values from a previous call to
-// Save().
 bool TerminalState::Restore() const {
 #if LLDB_ENABLE_POSIX
   if (IsValid()) {
@@ -147,8 +119,8 @@
 
 #if LLDB_ENABLE_TERMIOS
 if (TTYStateIsV

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 375928.
mgorny added a comment.

Accept `Terminal` arg in place of fd.


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

https://reviews.llvm.org/D110721

Files:
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Host/common/Terminal.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -4351,9 +4351,8 @@
 
 SetIsDone(false);
 const int read_fd = m_read_file.GetDescriptor();
-TerminalState terminal_state;
-terminal_state.Save(read_fd, false);
 Terminal terminal(read_fd);
+TerminalState terminal_state(terminal, false);
 terminal.SetCanonical(false);
 terminal.SetEcho(false);
 // FD_ZERO, FD_SET are not supported on windows
@@ -4399,7 +4398,6 @@
 }
 m_is_running = false;
 #endif
-terminal_state.Restore();
   }
 
   void Cancel() override {
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -430,11 +430,9 @@
   int stdin_fd = GetInputFD();
   if (stdin_fd >= 0) {
 Terminal terminal(stdin_fd);
-TerminalState terminal_state;
-const bool is_a_tty = terminal.IsATerminal();
+TerminalState terminal_state(terminal);
 
-if (is_a_tty) {
-  terminal_state.Save(stdin_fd, false);
+if (terminal.IsATerminal()) {
   terminal.SetCanonical(false);
   terminal.SetEcho(true);
 }
@@ -464,9 +462,6 @@
 run_string.Printf("run_python_interpreter (%s)",
   m_python->GetDictionaryName());
 PyRun_SimpleString(run_string.GetData());
-
-if (is_a_tty)
-  terminal_state.Restore();
   }
 }
 SetIsDone(true);
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -355,7 +355,6 @@
 PyEval_InitThreads();
   }
 
-  TerminalState m_stdin_tty_state;
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;
   bool m_was_already_initialized = false;
 };
Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -15,10 +15,6 @@
 #include 
 #include 
 
-#if LLDB_ENABLE_TERMIOS
-#include 
-#endif
-
 using namespace lldb_private;
 
 bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
@@ -81,63 +77,38 @@
   return false;
 }
 
-// Default constructor
-TerminalState::TerminalState()
-: m_tty()
-#if LLDB_ENABLE_TERMIOS
-  ,
-  m_termios_up()
-#endif
-{
+TerminalState::TerminalState(Terminal term, bool save_process_group)
+: m_tty(term) {
+  Save(term, save_process_group);
 }
 
-// Destructor
-TerminalState::~TerminalState() = default;
+TerminalState::~TerminalState() { Restore(); }
 
 void TerminalState::Clear() {
   m_tty.Clear();
   m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-  m_termios_up.reset();
-#endif
+  m_termios_saved = false;
   m_process_group = -1;
 }
 
-// Save the current state of the TTY for the file descriptor "fd" and if
-// "save_process_group" is true, attempt to save the process group info for the
-// TTY.
-bool TerminalState::Save(int fd, bool save_process_group) {
-  m_tty.SetFileDescriptor(fd);
+bool TerminalState::Save(Terminal term, bool save_process_group) {
+  Clear();
+  m_tty = term;
   if (m_tty.IsATerminal()) {
+int fd = m_tty.GetFileDescriptor();
 #if LLDB_ENABLE_POSIX
 m_tflags = ::fcntl(fd, F_GETFL, 0);
-#endif
 #if LLDB_ENABLE_TERMIOS
-if (m_termios_up == nullptr)
-  m_termios_up.reset(new struct termios);
-int err = ::tcgetattr(fd, m_termios_up.get());
-if (err != 0)
-  m_termios_up.reset();
-#endif // #if LLDB_ENABLE_TERMIOS
-#if LLDB_ENABLE_POSIX
+if (::tcgetattr(fd, &m_termios) == 0)
+  m_termios_saved = true;
+#endif // LLDB_ENABLE_TERMIOS
 if (save_process_group)
   m_process_group = ::tcgetpgrp(0);
-else
-  m_process_group = -1;
-#endif
-  } else {
-m_tty.Clear();
-m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-m_termios_up.reset();
-#endif
-m_process_group = -1;
+#endif // LLDB_ENABLE_POSIX
   }
   return IsValid();
 }
 
-// Restore the state of the TTY using the cached values from a previous call to
-// Save().
 bool Ter

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/include/lldb/Host/Terminal.h:20
+
 struct termios;
 

I don't think we need this forward declaration anymore.


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

https://reviews.llvm.org/D110721

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

[Lldb-commits] [lldb] d35702e - Fix LLDB build on old Linux kernels

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-09-29T09:42:32-07:00
New Revision: d35702efe73010409c75b1f1b64f205cc3b2f6d3

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

LOG: Fix LLDB build on old Linux kernels

Usage of aux_size is guarded against elsewhere in this file, but is missing 
here.

Reviewed By: wallace

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

Original Author: calebzulawski

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/IntelPTManager.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/Linux/IntelPTManager.cpp 
b/lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
index 0bd48933d4d3e..c5575fb30c98a 100644
--- a/lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
+++ b/lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
@@ -145,7 +145,11 @@ static Error CheckPsbPeriod(size_t psb_period) {
 }
 
 size_t IntelPTThreadTrace::GetTraceBufferSize() const {
+#ifndef PERF_ATTR_SIZE_VER5
+  llvm_unreachable("Intel PT Linux perf event not supported");
+#else
   return m_mmap_meta->aux_size;
+#endif
 }
 
 static Expected



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

[Lldb-commits] [PATCH] D110269: Fix LLDB build on old Linux kernels

[Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd35702efe730: Fix LLDB build on old Linux kernels (authored 
by Walter Erquinigo ).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110269

Files:
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp


Index: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
===
--- lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
+++ lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
@@ -145,7 +145,11 @@
 }
 
 size_t IntelPTThreadTrace::GetTraceBufferSize() const {
+#ifndef PERF_ATTR_SIZE_VER5
+  llvm_unreachable("Intel PT Linux perf event not supported");
+#else
   return m_mmap_meta->aux_size;
+#endif
 }
 
 static Expected


Index: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
===
--- lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
+++ lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
@@ -145,7 +145,11 @@
 }
 
 size_t IntelPTThreadTrace::GetTraceBufferSize() const {
+#ifndef PERF_ATTR_SIZE_VER5
+  llvm_unreachable("Intel PT Linux perf event not supported");
+#else
   return m_mmap_meta->aux_size;
+#endif
 }
 
 static Expected
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D110298: Add "command multiword add" and the ability to add script commands into a user multiword hierarchy

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D110298#3026579 , @clayborg wrote:

> The changes look fine from a quick read. See inline comments

The command add/command delete situation is a little fractured.  We currently 
have:

command alias/command unalias - Handles aliases to commands
command regex/command delete - Adds and deletes regex commands
command script add/command script delete

It would be weird for "command delete" to delete regex and multiword commands 
but not script commands and not aliases.

We could centralize everything to "command add --multiword".  Then it would 
make sense to delete everything in command delete as well.  But that might 
break a bunch of people's .lldbinit's etc.  And anyway, I don't have time to do 
all that right now.

Moreover, I actually think that deleting a multiword command as a separate 
gesture isn't a bad idea.  Deleting a script command deletes one thing and can 
be easily undone (by re-adding that one command).  Deleting a multiword command 
can potentially wipe out a whole hierarchy that you had built up.  It would be 
really annoying to have:

(lldb) command delete foo bar

where I forgot to put "baz" on the end would delete baz and everything else in 
the container.  You could add --force to delete, but I'm not sure that's a 
better interface.

If somebody can think of a more apt name than "multiword" I'm not wedded to 
that name.  "container" might be okay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110298

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

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 375945.
mgorny added a comment.

Fix `tcgetpgrp()` call (again). Remove unneeded forward decl.


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

https://reviews.llvm.org/D110721

Files:
  lldb/include/lldb/Host/Terminal.h
  lldb/source/Host/common/Terminal.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -4351,9 +4351,8 @@
 
 SetIsDone(false);
 const int read_fd = m_read_file.GetDescriptor();
-TerminalState terminal_state;
-terminal_state.Save(read_fd, false);
 Terminal terminal(read_fd);
+TerminalState terminal_state(terminal, false);
 terminal.SetCanonical(false);
 terminal.SetEcho(false);
 // FD_ZERO, FD_SET are not supported on windows
@@ -4399,7 +4398,6 @@
 }
 m_is_running = false;
 #endif
-terminal_state.Restore();
   }
 
   void Cancel() override {
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -430,11 +430,9 @@
   int stdin_fd = GetInputFD();
   if (stdin_fd >= 0) {
 Terminal terminal(stdin_fd);
-TerminalState terminal_state;
-const bool is_a_tty = terminal.IsATerminal();
+TerminalState terminal_state(terminal);
 
-if (is_a_tty) {
-  terminal_state.Save(stdin_fd, false);
+if (terminal.IsATerminal()) {
   terminal.SetCanonical(false);
   terminal.SetEcho(true);
 }
@@ -464,9 +462,6 @@
 run_string.Printf("run_python_interpreter (%s)",
   m_python->GetDictionaryName());
 PyRun_SimpleString(run_string.GetData());
-
-if (is_a_tty)
-  terminal_state.Restore();
   }
 }
 SetIsDone(true);
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -355,7 +355,6 @@
 PyEval_InitThreads();
   }
 
-  TerminalState m_stdin_tty_state;
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;
   bool m_was_already_initialized = false;
 };
Index: lldb/source/Host/common/Terminal.cpp
===
--- lldb/source/Host/common/Terminal.cpp
+++ lldb/source/Host/common/Terminal.cpp
@@ -15,10 +15,6 @@
 #include 
 #include 
 
-#if LLDB_ENABLE_TERMIOS
-#include 
-#endif
-
 using namespace lldb_private;
 
 bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
@@ -81,63 +77,38 @@
   return false;
 }
 
-// Default constructor
-TerminalState::TerminalState()
-: m_tty()
-#if LLDB_ENABLE_TERMIOS
-  ,
-  m_termios_up()
-#endif
-{
+TerminalState::TerminalState(Terminal term, bool save_process_group)
+: m_tty(term) {
+  Save(term, save_process_group);
 }
 
-// Destructor
-TerminalState::~TerminalState() = default;
+TerminalState::~TerminalState() { Restore(); }
 
 void TerminalState::Clear() {
   m_tty.Clear();
   m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-  m_termios_up.reset();
-#endif
+  m_termios_saved = false;
   m_process_group = -1;
 }
 
-// Save the current state of the TTY for the file descriptor "fd" and if
-// "save_process_group" is true, attempt to save the process group info for the
-// TTY.
-bool TerminalState::Save(int fd, bool save_process_group) {
-  m_tty.SetFileDescriptor(fd);
+bool TerminalState::Save(Terminal term, bool save_process_group) {
+  Clear();
+  m_tty = term;
   if (m_tty.IsATerminal()) {
+int fd = m_tty.GetFileDescriptor();
 #if LLDB_ENABLE_POSIX
 m_tflags = ::fcntl(fd, F_GETFL, 0);
-#endif
 #if LLDB_ENABLE_TERMIOS
-if (m_termios_up == nullptr)
-  m_termios_up.reset(new struct termios);
-int err = ::tcgetattr(fd, m_termios_up.get());
-if (err != 0)
-  m_termios_up.reset();
-#endif // #if LLDB_ENABLE_TERMIOS
-#if LLDB_ENABLE_POSIX
+if (::tcgetattr(fd, &m_termios) == 0)
+  m_termios_saved = true;
+#endif // LLDB_ENABLE_TERMIOS
 if (save_process_group)
-  m_process_group = ::tcgetpgrp(0);
-else
-  m_process_group = -1;
-#endif
-  } else {
-m_tty.Clear();
-m_tflags = -1;
-#if LLDB_ENABLE_TERMIOS
-m_termios_up.reset();
-#endif
-m_process_group = -1;
+  m_process_group = ::tcgetpgrp(fd);
+#endif // LLDB_ENABLE_POSIX
   }
   return IsValid();
 }
 
-// Restore the state of the TTY usi

[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState

[Michał Górny via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Host/Terminal.h:20
+
 struct termios;
 

shafik wrote:
> I don't think we need this forward declaration anymore.
You are correct, thanks!


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

https://reviews.llvm.org/D110721

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D110571#3025527 , @jarin wrote:

> Hi, could you take a look at this change?
>
> Some discussion points:
>
> - In the ParseVariablesInFunctionContext method, we are using a lambda for 
> the recursive parser. We could also use a function-local class or inner class 
> of SymbolFileDWARF. Would any of these be preferable?

Real function is fine and preferable IMHO.

> - The variables created by ParseVariableDIE on abstract formal parameters are 
> fairly strange, especially if a function gets inlined into two different 
> functions. If that happens, then the parsed variable will refer to a symbol 
> context that does not contain the variable DIE and a block can contain a 
> variable that is not in the DIE of tree of the block. Is that a big problem? 
> (Quick testing of this situation did not reveal any strange stack traces or 
> `frame var` anomalies.) Unfortunately, there is no good way to provide the 
> correct block and the correct function because LLDB does not parse functions 
> and blocks for the abstract functions (i.e., for the DW_TAG_subroutines that 
> are referenced by DW_AT_abstract_origin of concrete functions).

LLDB doesn't parse function definitions that have no low/high PC into 
lldb_private::Function with lldb_private::Block objects, it only does this for 
instances of functions with address ranges.

I would expect that the SymbolContextScope (one pointer that can identify the 
SymbolContext) for each variable parsed by ParseVariableDIE to point to the 
DW_TAG_variable that is in the function with the address range. Are you saying 
that the symbol context points to the definition?

> - The provided test only tests the case of an inlined function where some 
> parameters are unused/omitted. Would it make sense to also provide tests for 
> other interesting cases or would that be too much bloat? The particularly 
> interesting cases are:
>   - Inlined function with all its parameters unused/omitted,
>   - Inlined function that is called from different top-level functions.
>   - Test correctness of the stack trace in the cases above.

Anything that tests what the compilers are emitting would be great to have if 
we can make them.

> - We could supply a test written in C, but it needs -O1 and is fairly 
> sensitive to the meaning of -O1 (e.g., clang started inlining and omitting 
> unsued inlined parameters only recently, so changes to -O1 could make a C 
> test easily meaningless). Any concerns here?

It is really hard to make sure the compiler generates what you want for a test 
case as it will change over time and you might not end up testing what you 
think you are testing. The easiest way to avoid this is to emit the assembly 
from the compiler and then use that as the source for the test since that will 
guarantee the same output.

> - The provided test is a bit verbose, mostly because we wanted to mostly 
> preserve the structure of the C compiler output. We could still cut the size 
> of the test down by removing the main function in favour of _start and by 
> removing all the file/line info. Would any of that make sense?

The image lookup as Pavel suggested is a good way to test info for various 
addresses without having to run the process or run to multiple locations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

This looks good to me. Pavel, are you ok with the testing strategy with the 
updated tests?




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3630-3631
+if (tag == DW_TAG_inlined_subroutine) {
+  // Walk abstract origins until we find DW_TAG_subprogram and extract
+  // its formal parameters.
+  DWARFDIE abs_die = die;

Maybe expand this comment a bit. If I understand the problem correctly it might 
read something like:

```
DW_TAG_inline_subroutine objects may omit DW_TAG_formal_parameter in instances 
of the function when they are unused or ... . The current 
DW_TAG_inline_subroutine may refer to another DW_TAG_inline_subroutine or 
DW_TAG_subprogram that might actually have the definitions of the parameters 
and we need to include these so they show up in the variables for this function.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

First of all, thank you, Greg and Pavel, for all the great feedback and 
discussion. I have followed all your suggestions (use plain method, use `image 
lookup`, added the additional tests). I have also packaged the C test, but as 
Greg notes I am not convinced it will keep testing what it's supposed to.

Now, let me answer the question regarding the context:

In D110571#3030913 , @clayborg wrote:

> LLDB doesn't parse function definitions that have no low/high PC into 
> lldb_private::Function with lldb_private::Block objects, it only does this 
> for instances of functions with address ranges.
>
> I would expect that the SymbolContextScope (one pointer that can identify the 
> SymbolContext) for each variable parsed by ParseVariableDIE to point to the 
> DW_TAG_variable that is in the function with the address range. Are you 
> saying that the symbol context points to the definition?

With this change, the symbol context for unused parameters points to one of the 
concrete inlined function block (DW_TAG_inlined_subroutine). That concrete 
inlined function will not contain that formal parameter because after the clang 
change https://reviews.llvm.org/D95617, unused formal parameters are deleted 
from their concrete inlined functions (i.e., from their 
DW_TAG_inlined_subroutine).

The point here is that if a function is inlined into two places, i.e., there 
are two corresponding DW_TAG_inlined_subroutines for the inlined function, we 
still create just one instance of Variable and its symbol context will be one 
randomly chosen DW_TAG_inlined_subroutine.

As Pavel suggested, an alternative would be creating one Variable instance per 
DW_TAG_inlined_subroutine. That would require some changes to other data 
structures because the existing code assumes there is just one Variable for 
each DIE (see SymbolFileDWARF::GetDIEToVariable).

For illustration:

  0x100  DW_TAG_subprogram
   DW_AT_name "inlined_function"
   ... no DW_AT_low_pc here ...
  0x110DW_TAG_formal_parameter
 DW_AT_name "unused"
 ...
 ...
  0x200  DW_TAG_subprogram
   DW_AT_name("top_level_function_with_address"
   DW_AT_low_pc  (0x3000)
   DW_AT_high_pc  (0x3100)
   ...
  0x210DW_TAG_inlined_subroutine
 DW_AT_abstract_origin (0x100 "inlined_function")
 DW_AT_low_pc  (0x3010)
 DW_AT_high_pc  (0x3020)
 # Note the missing DW_TAG_formal_parameter here!
   NULL
 ...
  0x400  DW_TAG_subprogram
   DW_AT_name("another_top_level_function_with_address"
   DW_AT_low_pc  (0x5000)
   DW_AT_high_pc  (0x5100)
   ...
  0x410DW_TAG_inlined_subroutine
 DW_AT_abstract_origin (0x100 "inlined_function")
 DW_AT_low_pc  (0x5030)
 DW_AT_high_pc  (0x5040)
 # Note the missing DW_TAG_formal_parameter here!
   NULL
 ...

Here, we will create just one variable for the formal parameter "unused" (DIE 
offset 0x110). That variable's symbol context will be randomly one of the 
DW_TAG_inline subroutine blocks (either 0x210 or 0x410), and the variable will 
be inserted into two variable lists, one for the Block associated with the DIE 
at 0x210 and one for DIE associated with 0x410.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [lldb] 385b218 - [lldb] Remove Expression's dependency on CPlusPlusLanguagePlugin

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2021-09-29T11:39:09-07:00
New Revision: 385b2189cc4446745e1ea4ac803c22b3daef73ec

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

LOG: [lldb] Remove Expression's dependency on CPlusPlusLanguagePlugin

This change accomplishes the following:
- Moves `IRExecutionUnit::FindBestAlternateMangledName` to `Language`.
- Renames `FindBestAlternateMangledName` to
  `FindBestAlternateFunctionMangledName`
- Changes the first parameter of said method from a `ConstString`
  representing a demangled name to a `Mangled`.
- Remove the use of CPlusPlusLanguage from Expression

Added: 


Modified: 
lldb/include/lldb/Target/Language.h
lldb/source/Expression/CMakeLists.txt
lldb/source/Expression/IRExecutionUnit.cpp
lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

Removed: 




diff  --git a/lldb/include/lldb/Target/Language.h 
b/lldb/include/lldb/Target/Language.h
index 41a0a5e13a0e0..0b0891c14029e 100644
--- a/lldb/include/lldb/Target/Language.h
+++ b/lldb/include/lldb/Target/Language.h
@@ -300,6 +300,12 @@ class Language : public PluginInterface {
 return std::vector();
   }
 
+  virtual ConstString
+  FindBestAlternateFunctionMangledName(const Mangled mangled,
+   const SymbolContext &sym_ctx) const {
+return ConstString();
+  }
+
 protected:
   // Classes that inherit from Language can see and modify these
 

diff  --git a/lldb/source/Expression/CMakeLists.txt 
b/lldb/source/Expression/CMakeLists.txt
index bf94361dd6c19..c935e1b1c04b9 100644
--- a/lldb/source/Expression/CMakeLists.txt
+++ b/lldb/source/Expression/CMakeLists.txt
@@ -23,7 +23,6 @@ add_lldb_library(lldbExpression
 lldbSymbol
 lldbTarget
 lldbUtility
-lldbPluginCPlusPlusLanguage
 lldbPluginObjectFileJIT
 
   LINK_COMPONENTS

diff  --git a/lldb/source/Expression/IRExecutionUnit.cpp 
b/lldb/source/Expression/IRExecutionUnit.cpp
index 39ece83086416..f2d22f7ed9ccd 100644
--- a/lldb/source/Expression/IRExecutionUnit.cpp
+++ b/lldb/source/Expression/IRExecutionUnit.cpp
@@ -26,6 +26,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
+#include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
@@ -33,7 +34,6 @@
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 
-#include "lldb/../../source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "lldb/../../source/Plugins/ObjectFile/JIT/ObjectFileJIT.h"
 
 using namespace lldb_private;
@@ -652,52 +652,6 @@ uint8_t 
*IRExecutionUnit::MemoryManager::allocateDataSection(
   return return_value;
 }
 
-static ConstString FindBestAlternateMangledName(ConstString demangled,
-const SymbolContext &sym_ctx) {
-  CPlusPlusLanguage::MethodName cpp_name(demangled);
-  std::string scope_qualified_name = cpp_name.GetScopeQualifiedName();
-
-  if (!scope_qualified_name.size())
-return ConstString();
-
-  if (!sym_ctx.module_sp)
-return ConstString();
-
-  lldb_private::SymbolFile *sym_file = sym_ctx.module_sp->GetSymbolFile();
-  if (!sym_file)
-return ConstString();
-
-  std::vector alternates;
-  sym_file->GetMangledNamesForFunction(scope_qualified_name, alternates);
-
-  std::vector param_and_qual_matches;
-  std::vector param_matches;
-  for (size_t i = 0; i < alternates.size(); i++) {
-ConstString alternate_mangled_name = alternates[i];
-Mangled mangled(alternate_mangled_name);
-ConstString demangled = mangled.GetDemangledName();
-
-CPlusPlusLanguage::MethodName alternate_cpp_name(demangled);
-if (!cpp_name.IsValid())
-  continue;
-
-if (alternate_cpp_name.GetArguments() == cpp_name.GetArguments()) {
-  if (alternate_cpp_name.GetQualifiers() == cpp_name.GetQualifiers())
-param_and_qual_matches.push_back(alternate_mangled_name);
-  else
-param_matches.push_back(alternate_mangled_name);
-}
-  }
-
-  if (param_and_qual_matches.size())
-return param_and_qual_matches[0]; // It is assumed that there will be only
-  // one!
-  else if (param_matches.size())
-return param_matches[0]; // Return one of them as a best match
-  else
-return ConstString();
-}
-
 void IRExecutionUnit::CollectCandidateCNames(std::vector &C_names,
  ConstString name) {
   if (m_strip_underscore && name.AsCString()[0] == '_')
@@ -712,10 +666,9 @@ void IRExecutionUnit::CollectCandidateCPlusPlusNames(
 for (const ConstString &name : C_names) {
   Mangled mangled(na

[Lldb-commits] [PATCH] D110115: [lldb] Remove Expression's dependency on CPlusPlusLanguagePlugin

[Alex Langford via Phabricator via lldb-commits]

bulbazord closed this revision.
bulbazord added a comment.

385b2189cc44 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110115

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

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D110571#3031140 , @jarin wrote:

> First of all, thank you, Greg and Pavel, for all the great feedback and 
> discussion. I have followed all your suggestions (use plain method, use 
> `image lookup`, added the additional tests). I have also packaged the C test, 
> but as Greg notes I am not convinced it will keep testing what it's supposed 
> to.
>
> Now, let me answer the question regarding the context:
>
> In D110571#3030913 , @clayborg 
> wrote:
>
>> LLDB doesn't parse function definitions that have no low/high PC into 
>> lldb_private::Function with lldb_private::Block objects, it only does this 
>> for instances of functions with address ranges.
>>
>> I would expect that the SymbolContextScope (one pointer that can identify 
>> the SymbolContext) for each variable parsed by ParseVariableDIE to point to 
>> the DW_TAG_variable that is in the function with the address range. Are you 
>> saying that the symbol context points to the definition?
>
> With this change, the symbol context for unused parameters points to one of 
> the concrete inlined function block (DW_TAG_inlined_subroutine). That 
> concrete inlined function will not contain that formal parameter because 
> after the clang change https://reviews.llvm.org/D95617, unused formal 
> parameters are deleted from their concrete inlined functions (i.e., from 
> their DW_TAG_inlined_subroutine).
>
> The point here is that if a function is inlined into two places, i.e., there 
> are two corresponding DW_TAG_inlined_subroutines for the inlined function, we 
> still create just one instance of Variable and its symbol context will be one 
> randomly chosen DW_TAG_inlined_subroutine.
>
> As Pavel suggested, an alternative would be creating one Variable instance 
> per DW_TAG_inlined_subroutine. That would require some changes to other data 
> structures because the existing code assumes there is just one Variable for 
> each DIE (see SymbolFileDWARF::GetDIEToVariable).
>
> For illustration:
>
>   0x100  DW_TAG_subprogram
>DW_AT_name "inlined_function"
>... no DW_AT_low_pc here ...
>   0x110DW_TAG_formal_parameter
>  DW_AT_name "unused"
>  ...
>  ...
>   0x200  DW_TAG_subprogram
>DW_AT_name("top_level_function_with_address"
>DW_AT_low_pc  (0x3000)
>DW_AT_high_pc  (0x3100)
>...
>   0x210DW_TAG_inlined_subroutine
>  DW_AT_abstract_origin (0x100 "inlined_function")
>  DW_AT_low_pc  (0x3010)
>  DW_AT_high_pc  (0x3020)
>  # Note the missing DW_TAG_formal_parameter here!
>NULL
>  ...
>   0x400  DW_TAG_subprogram
>DW_AT_name("another_top_level_function_with_address"
>DW_AT_low_pc  (0x5000)
>DW_AT_high_pc  (0x5100)
>...
>   0x410DW_TAG_inlined_subroutine
>  DW_AT_abstract_origin (0x100 "inlined_function")
>  DW_AT_low_pc  (0x5030)
>  DW_AT_high_pc  (0x5040)
>  # Note the missing DW_TAG_formal_parameter here!
>NULL
>  ...
>
> Here, we will create just one variable for the formal parameter "unused" (DIE 
> offset 0x110). That variable's symbol context will be randomly one of the 
> DW_TAG_inline subroutine blocks (either 0x210 or 0x410), and the variable 
> will be inserted into two variable lists, one for the Block associated with 
> the DIE at 0x210 and one for DIE associated with 0x410.

I hear what you are saying, but I am not sure this will be happening. Let me 
explain: for each concrete DW_TAG_subprogram (0x200 and 0x400 in your example 
above), we create a unique lldb_private::Function object whose UserID will be 
0x200 for "top_level_function_with_address" and 0x400 for 
"another_top_level_function_with_address". Each of those functions might be 
asked for their lldb_private::Block objects at some point and we should create 
unique lldb_private::Block for each DW_TAG_lexical_block and 
DW_TAG_inlined_subroutine that is contained within these unique DIEs. Each of 
these should now have a variable within the block that is a parameter whose 
name is "unused" and whose symbol context should be 0x210 for the 0x200 DIE, 
and 0x410 for the 0x400 DIE. So it would be great to make sure this happens 
correctly. From looking at the code, it seems like this should be happening 
correctly, but you might know better since you made these new modifications.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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

[Lldb-commits] [PATCH] D110787: Make "process attach -c" work again, add a test for it.

[Jim Ingham via Phabricator via lldb-commits]

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

process attach -c

stopped working at some point.  Since there was no test for this feature, I'm 
not sure when it broke, but I think it was when we started being more rigorous 
about tracking the interpreter execution context.

The continue in the CommandObjectProcessAttach was handled by calling 
`HandleCommand("process attach", ...)`.  That seems a little odd at first 
blush, but makes sense because it means the result of the continue gets 
reported just as an ordinary user "continue" would.

The problem is that though we've made a new process (and maybe even a new 
target) in the basic attach command, we haven't told the command interpreter 
about them yet, so CheckRequirements will fail before we get a chance to run 
the command.

In this case, since we know exactly which process we want to continue, we 
should just be explicit and pass an execution context for that process to 
HandleCommand.

Also added a test for this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110787

Files:
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/test/API/commands/process/attach/TestProcessAttach.py


Index: lldb/test/API/commands/process/attach/TestProcessAttach.py
===
--- lldb/test/API/commands/process/attach/TestProcessAttach.py
+++ lldb/test/API/commands/process/attach/TestProcessAttach.py
@@ -43,6 +43,23 @@
 process = target.GetProcess()
 self.assertTrue(process, PROCESS_IS_VALID)
 
+@skipIfiOSSimulator
+def test_attach_to_process_by_id_autocontinue(self):
+"""Test attach by process id"""
+self.build()
+exe = self.getBuildArtifact(exe_name)
+
+# Spawn a new process
+popen = self.spawnSubprocess(exe)
+
+self.runCmd("process attach -c -p " + str(popen.pid))
+
+target = self.dbg.GetSelectedTarget()
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertTrue(process.GetState(), lldb.eStateRunning)
+
 @skipIfReproducer # FIXME: Unexpected packet during (active) replay
 @skipIfWindows # This is flakey on Windows AND when it fails, it hangs: 
llvm.org/pr48806
 def test_attach_to_process_from_different_dir_by_id(self):
Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -418,9 +418,10 @@
 }
 
 StreamString stream;
+ProcessSP process_sp;
 const auto error = target->Attach(m_options.attach_info, &stream);
 if (error.Success()) {
-  ProcessSP process_sp(target->GetProcessSP());
+  process_sp = target->GetProcessSP();
   if (process_sp) {
 result.AppendMessage(stream.GetString());
 result.SetStatus(eReturnStatusSuccessFinishNoResult);
@@ -472,8 +473,13 @@
 
 // This supports the use-case scenario of immediately continuing the
 // process once attached.
-if (m_options.attach_info.GetContinueOnceAttached())
-  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
+if (m_options.attach_info.GetContinueOnceAttached()) {
+  // We have made a process but haven't told the interpreter about it yet,
+  // so CheckRequirements will fail for "process continue".  Set the 
override
+  // here:
+  ExecutionContext exe_ctx(process_sp);
+  m_interpreter.HandleCommand("process continue", eLazyBoolNo, exe_ctx, 
result);
+}
 
 return result.Succeeded();
   }


Index: lldb/test/API/commands/process/attach/TestProcessAttach.py
===
--- lldb/test/API/commands/process/attach/TestProcessAttach.py
+++ lldb/test/API/commands/process/attach/TestProcessAttach.py
@@ -43,6 +43,23 @@
 process = target.GetProcess()
 self.assertTrue(process, PROCESS_IS_VALID)
 
+@skipIfiOSSimulator
+def test_attach_to_process_by_id_autocontinue(self):
+"""Test attach by process id"""
+self.build()
+exe = self.getBuildArtifact(exe_name)
+
+# Spawn a new process
+popen = self.spawnSubprocess(exe)
+
+self.runCmd("process attach -c -p " + str(popen.pid))
+
+target = self.dbg.GetSelectedTarget()
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertTrue(process.GetState(), lldb.eStateRunning)
+
 @skipIfReproducer # FIXME: Unexpected packet during (active) replay
 @skipIfWindows # This is flakey on Windows AND when it fails, it hangs: llvm.org/pr48806
 def test_attach_to_process_from_different_dir_by_id(self):
Index: lldb/source/Commands/CommandObject

[Lldb-commits] [PATCH] D110787: Make "process attach -c" work again, add a test for it.

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM. Thanks Jim!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110787

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

[Lldb-commits] [PATCH] D110601: Add -relative-to-command-file to "command source"

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 376083.
jingham added a comment.

Made -C with an absolute path an error, added at test for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110601

Files:
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/command/source/TestCommandSource.py
  lldb/test/API/commands/command/source/commands2.txt
  lldb/test/API/commands/command/source/not-relative.txt
  lldb/test/API/commands/command/source/subdir/subcmds.txt

Index: lldb/test/API/commands/command/source/subdir/subcmds.txt
===
--- /dev/null
+++ lldb/test/API/commands/command/source/subdir/subcmds.txt
@@ -0,0 +1 @@
+command source -C ../commands.txt
Index: lldb/test/API/commands/command/source/not-relative.txt
===
--- /dev/null
+++ lldb/test/API/commands/command/source/not-relative.txt
@@ -0,0 +1,2 @@
+command source -C /tmp/somefile.txt
+script import my
Index: lldb/test/API/commands/command/source/commands2.txt
===
--- /dev/null
+++ lldb/test/API/commands/command/source/commands2.txt
@@ -0,0 +1 @@
+command source -C subdir/subcmds.txt
Index: lldb/test/API/commands/command/source/TestCommandSource.py
===
--- lldb/test/API/commands/command/source/TestCommandSource.py
+++ lldb/test/API/commands/command/source/TestCommandSource.py
@@ -21,7 +21,18 @@
 # Sourcing .lldb in the current working directory, which in turn imports
 # the "my" package that defines the date() function.
 self.runCmd("command source .lldb")
+self.check_results()
+
+@no_debug_info_test
+def test_command_source_relative(self):
+"""Test that lldb command "command source" works correctly with relative paths."""
 
+# Sourcing .lldb in the current working directory, which in turn imports
+# the "my" package that defines the date() function.
+self.runCmd("command source commands2.txt")
+self.check_results()
+
+def check_results(self, failure=False):
 # Python should evaluate "my.date()" successfully.
 command_interpreter = self.dbg.GetCommandInterpreter()
 self.assertTrue(command_interpreter, VALID_COMMAND_INTERPRETER)
@@ -29,6 +40,18 @@
 command_interpreter.HandleCommand("script my.date()", result)
 
 import datetime
-self.expect(result.GetOutput(), "script my.date() runs successfully",
-exe=False,
-substrs=[str(datetime.date.today())])
+if failure:
+self.expect(result.GetOutput(), "script my.date() runs successfully",
+exe=False, error=True)
+else: 
+self.expect(result.GetOutput(), "script my.date() runs successfully",
+exe=False,
+substrs=[str(datetime.date.today())])
+
+@no_debug_info_test
+def test_command_source_relative_error(self):
+"""Test that 'command source -C' gives an error for a relative path"""
+source_dir = self.getSourceDir()
+result = lldb.SBCommandReturnObject()
+self.runCmd("command source --stop-on-error 1 not-relative.txt")
+self.check_results(failure=True)
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -536,6 +536,10 @@
 Desc<"If true, stop executing commands on continue.">;
   def source_silent_run : Option<"silent-run", "s">, Arg<"Boolean">,
 Desc<"If true don't echo commands while executing.">;
+  def cmd_relative_to_command_file : Option<"relative-to-command-file", "C">,
+Desc<"Resolve non-absolute paths relative to the location of the "
+"current command file. This argument can only be used when the command is "
+"being sourced from a file.">;
 }
 
 let Command = "alias" in {
Index: lldb/source/Commands/CommandObjectCommands.cpp
===
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -77,7 +77,7 @@
   public:
 CommandOptions()
 : Options(), m_stop_on_error(true), m_silent_run(false),
-  m_stop_on_continue(true) {}
+  m_stop_on_continue(true), m_cmd_relative_to_command_file(false) {}
 
 ~CommandOptions() override = default;
 
@@ -95,6 +95,10 @@
 error = m_stop_on_continue.SetValueFromString(option_arg);
 break;
 
+  case 'C':
+m_cmd_relative_to_command_file = true;
+break;
+
   case 's':
 error = m_silent_run.SetValueFromString(option_arg);
 

[Lldb-commits] [PATCH] D110601: Add -relative-to-command-file to "command source"

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110601

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

[Lldb-commits] [lldb] 3bf3b96 - Add the --relative-to-command-file to "command source" so you can

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2021-09-29T19:33:41-07:00
New Revision: 3bf3b96629e8dfc55d01ba0cb05ca01a467017fa

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

LOG: Add the --relative-to-command-file to "command source" so you can
have linked command files in a source tree and get to them all from
one main command file.

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

Added: 
lldb/test/API/commands/command/source/commands2.txt
lldb/test/API/commands/command/source/not-relative.txt
lldb/test/API/commands/command/source/subdir/subcmds.txt

Modified: 
lldb/source/Commands/CommandObjectCommands.cpp
lldb/source/Commands/Options.td
lldb/test/API/commands/command/source/TestCommandSource.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectCommands.cpp 
b/lldb/source/Commands/CommandObjectCommands.cpp
index 9a8b81c007ad8..639279875e715 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -77,7 +77,7 @@ class CommandObjectCommandsSource : public 
CommandObjectParsed {
   public:
 CommandOptions()
 : Options(), m_stop_on_error(true), m_silent_run(false),
-  m_stop_on_continue(true) {}
+  m_stop_on_continue(true), m_cmd_relative_to_command_file(false) {}
 
 ~CommandOptions() override = default;
 
@@ -95,6 +95,10 @@ class CommandObjectCommandsSource : public 
CommandObjectParsed {
 error = m_stop_on_continue.SetValueFromString(option_arg);
 break;
 
+  case 'C':
+m_cmd_relative_to_command_file = true;
+break;
+
   case 's':
 error = m_silent_run.SetValueFromString(option_arg);
 break;
@@ -110,6 +114,7 @@ class CommandObjectCommandsSource : public 
CommandObjectParsed {
   m_stop_on_error.Clear();
   m_silent_run.Clear();
   m_stop_on_continue.Clear();
+  m_cmd_relative_to_command_file.Clear();
 }
 
 llvm::ArrayRef GetDefinitions() override {
@@ -121,6 +126,7 @@ class CommandObjectCommandsSource : public 
CommandObjectParsed {
 OptionValueBoolean m_stop_on_error;
 OptionValueBoolean m_silent_run;
 OptionValueBoolean m_stop_on_continue;
+OptionValueBoolean m_cmd_relative_to_command_file;
   };
 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
@@ -131,7 +137,29 @@ class CommandObjectCommandsSource : public 
CommandObjectParsed {
   return false;
 }
 
+FileSpec source_dir = {};
+if (m_options.m_cmd_relative_to_command_file) {
+  source_dir = GetDebugger().GetCommandInterpreter().GetCurrentSourceDir();
+  if (!source_dir) {
+result.AppendError("command source -C can only be specified "
+   "from a command file");
+result.SetStatus(eReturnStatusFailed);
+return false;
+  }
+}
+
 FileSpec cmd_file(command[0].ref());
+if (source_dir) {
+  // Prepend the source_dir to the cmd_file path:
+  if (!cmd_file.IsRelative()) {
+result.AppendError("command source -C can only be used "
+   "with a relative path.");
+result.SetStatus(eReturnStatusFailed);
+return false;
+  }
+  cmd_file.MakeAbsolute(source_dir);
+}
+
 FileSystem::Instance().Resolve(cmd_file);
 
 CommandInterpreterRunOptions options;

diff  --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 83df2ac22c578..3d69bb8ad8d05 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -536,6 +536,10 @@ let Command = "source" in {
 Desc<"If true, stop executing commands on continue.">;
   def source_silent_run : Option<"silent-run", "s">, Arg<"Boolean">,
 Desc<"If true don't echo commands while executing.">;
+  def cmd_relative_to_command_file : Option<"relative-to-command-file", "C">,
+Desc<"Resolve non-absolute paths relative to the location of the "
+"current command file. This argument can only be used when the command is "
+"being sourced from a file.">;
 }
 
 let Command = "alias" in {

diff  --git a/lldb/test/API/commands/command/source/TestCommandSource.py 
b/lldb/test/API/commands/command/source/TestCommandSource.py
index 6d2717b16e2dc..dc32e20ddba0a 100644
--- a/lldb/test/API/commands/command/source/TestCommandSource.py
+++ b/lldb/test/API/commands/command/source/TestCommandSource.py
@@ -21,7 +21,18 @@ def test_command_source(self):
 # Sourcing .lldb in the current working directory, which in turn 
imports
 # the "my" package that defines the date() function.
 self.runCmd("command source .lldb")
+self.check_results()
+
+@no_debug_info_test
+def test_command_source_relative(self):
+"""Test that lldb command

[Lldb-commits] [PATCH] D110601: Add -relative-to-command-file to "command source"

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3bf3b96629e8: Add the --relative-to-command-file to 
"command source" so you can (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110601

Files:
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/command/source/TestCommandSource.py
  lldb/test/API/commands/command/source/commands2.txt
  lldb/test/API/commands/command/source/not-relative.txt
  lldb/test/API/commands/command/source/subdir/subcmds.txt

Index: lldb/test/API/commands/command/source/subdir/subcmds.txt
===
--- /dev/null
+++ lldb/test/API/commands/command/source/subdir/subcmds.txt
@@ -0,0 +1 @@
+command source -C ../commands.txt
Index: lldb/test/API/commands/command/source/not-relative.txt
===
--- /dev/null
+++ lldb/test/API/commands/command/source/not-relative.txt
@@ -0,0 +1,2 @@
+command source -C /tmp/somefile.txt
+script import my
Index: lldb/test/API/commands/command/source/commands2.txt
===
--- /dev/null
+++ lldb/test/API/commands/command/source/commands2.txt
@@ -0,0 +1 @@
+command source -C subdir/subcmds.txt
Index: lldb/test/API/commands/command/source/TestCommandSource.py
===
--- lldb/test/API/commands/command/source/TestCommandSource.py
+++ lldb/test/API/commands/command/source/TestCommandSource.py
@@ -21,7 +21,18 @@
 # Sourcing .lldb in the current working directory, which in turn imports
 # the "my" package that defines the date() function.
 self.runCmd("command source .lldb")
+self.check_results()
+
+@no_debug_info_test
+def test_command_source_relative(self):
+"""Test that lldb command "command source" works correctly with relative paths."""
 
+# Sourcing .lldb in the current working directory, which in turn imports
+# the "my" package that defines the date() function.
+self.runCmd("command source commands2.txt")
+self.check_results()
+
+def check_results(self, failure=False):
 # Python should evaluate "my.date()" successfully.
 command_interpreter = self.dbg.GetCommandInterpreter()
 self.assertTrue(command_interpreter, VALID_COMMAND_INTERPRETER)
@@ -29,6 +40,18 @@
 command_interpreter.HandleCommand("script my.date()", result)
 
 import datetime
-self.expect(result.GetOutput(), "script my.date() runs successfully",
-exe=False,
-substrs=[str(datetime.date.today())])
+if failure:
+self.expect(result.GetOutput(), "script my.date() runs successfully",
+exe=False, error=True)
+else: 
+self.expect(result.GetOutput(), "script my.date() runs successfully",
+exe=False,
+substrs=[str(datetime.date.today())])
+
+@no_debug_info_test
+def test_command_source_relative_error(self):
+"""Test that 'command source -C' gives an error for a relative path"""
+source_dir = self.getSourceDir()
+result = lldb.SBCommandReturnObject()
+self.runCmd("command source --stop-on-error 1 not-relative.txt")
+self.check_results(failure=True)
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -536,6 +536,10 @@
 Desc<"If true, stop executing commands on continue.">;
   def source_silent_run : Option<"silent-run", "s">, Arg<"Boolean">,
 Desc<"If true don't echo commands while executing.">;
+  def cmd_relative_to_command_file : Option<"relative-to-command-file", "C">,
+Desc<"Resolve non-absolute paths relative to the location of the "
+"current command file. This argument can only be used when the command is "
+"being sourced from a file.">;
 }
 
 let Command = "alias" in {
Index: lldb/source/Commands/CommandObjectCommands.cpp
===
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -77,7 +77,7 @@
   public:
 CommandOptions()
 : Options(), m_stop_on_error(true), m_silent_run(false),
-  m_stop_on_continue(true) {}
+  m_stop_on_continue(true), m_cmd_relative_to_command_file(false) {}
 
 ~CommandOptions() override = default;
 
@@ -95,6 +95,10 @@
 error = m_stop_on_continue.SetValueFromString(option_arg);
 break;
 
+  case 'C':
+m_cmd_relative_to_command_file = true;
+break;
+
   case 's':
 

[Lldb-commits] [lldb] 2303391 - Make "process attach -c" work correctly, and add a test for it.

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2021-09-29T19:38:09-07:00
New Revision: 2303391d1f543f4e57f9ed0fc68bad2d4cf890dc

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

LOG: Make "process attach -c" work correctly, and add a test for it.

The issue here was that we were not updating the interpreter's
execution context when calling HandleCommand to continue the process.
Since we had just created the process, it wasn't in the interpreter's
execution context so HandleCommand failed at CheckRequirements.  The
patch fixes that by passing the process execution context directly
to HandleCommand.

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectProcess.cpp
lldb/test/API/commands/process/attach/TestProcessAttach.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectProcess.cpp 
b/lldb/source/Commands/CommandObjectProcess.cpp
index b3e2f6a1a02b7..f3d20b390c6d1 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -398,9 +398,10 @@ class CommandObjectProcessAttach : public 
CommandObjectProcessLaunchOrAttach {
 }
 
 StreamString stream;
+ProcessSP process_sp;
 const auto error = target->Attach(m_options.attach_info, &stream);
 if (error.Success()) {
-  ProcessSP process_sp(target->GetProcessSP());
+  process_sp = target->GetProcessSP();
   if (process_sp) {
 result.AppendMessage(stream.GetString());
 result.SetStatus(eReturnStatusSuccessFinishNoResult);
@@ -452,8 +453,13 @@ class CommandObjectProcessAttach : public 
CommandObjectProcessLaunchOrAttach {
 
 // This supports the use-case scenario of immediately continuing the
 // process once attached.
-if (m_options.attach_info.GetContinueOnceAttached())
-  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
+if (m_options.attach_info.GetContinueOnceAttached()) {
+  // We have made a process but haven't told the interpreter about it yet,
+  // so CheckRequirements will fail for "process continue".  Set the 
override
+  // here:
+  ExecutionContext exe_ctx(process_sp);
+  m_interpreter.HandleCommand("process continue", eLazyBoolNo, exe_ctx, 
result);
+}
 
 return result.Succeeded();
   }

diff  --git a/lldb/test/API/commands/process/attach/TestProcessAttach.py 
b/lldb/test/API/commands/process/attach/TestProcessAttach.py
index b33aeebccdd1e..4738ee5851e7a 100644
--- a/lldb/test/API/commands/process/attach/TestProcessAttach.py
+++ b/lldb/test/API/commands/process/attach/TestProcessAttach.py
@@ -43,6 +43,23 @@ def test_attach_to_process_by_id(self):
 process = target.GetProcess()
 self.assertTrue(process, PROCESS_IS_VALID)
 
+@skipIfiOSSimulator
+def test_attach_to_process_by_id_autocontinue(self):
+"""Test attach by process id"""
+self.build()
+exe = self.getBuildArtifact(exe_name)
+
+# Spawn a new process
+popen = self.spawnSubprocess(exe)
+
+self.runCmd("process attach -c -p " + str(popen.pid))
+
+target = self.dbg.GetSelectedTarget()
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertTrue(process.GetState(), lldb.eStateRunning)
+
 @skipIfReproducer # FIXME: Unexpected packet during (active) replay
 @skipIfWindows # This is flakey on Windows AND when it fails, it hangs: 
llvm.org/pr48806
 def test_attach_to_process_from_
diff erent_dir_by_id(self):



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

[Lldb-commits] [PATCH] D110787: Make "process attach -c" work again, add a test for it.

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2303391d1f54: Make "process attach -c" work 
correctly, and add a test for it. (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110787

Files:
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/test/API/commands/process/attach/TestProcessAttach.py


Index: lldb/test/API/commands/process/attach/TestProcessAttach.py
===
--- lldb/test/API/commands/process/attach/TestProcessAttach.py
+++ lldb/test/API/commands/process/attach/TestProcessAttach.py
@@ -43,6 +43,23 @@
 process = target.GetProcess()
 self.assertTrue(process, PROCESS_IS_VALID)
 
+@skipIfiOSSimulator
+def test_attach_to_process_by_id_autocontinue(self):
+"""Test attach by process id"""
+self.build()
+exe = self.getBuildArtifact(exe_name)
+
+# Spawn a new process
+popen = self.spawnSubprocess(exe)
+
+self.runCmd("process attach -c -p " + str(popen.pid))
+
+target = self.dbg.GetSelectedTarget()
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertTrue(process.GetState(), lldb.eStateRunning)
+
 @skipIfReproducer # FIXME: Unexpected packet during (active) replay
 @skipIfWindows # This is flakey on Windows AND when it fails, it hangs: 
llvm.org/pr48806
 def test_attach_to_process_from_different_dir_by_id(self):
Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -398,9 +398,10 @@
 }
 
 StreamString stream;
+ProcessSP process_sp;
 const auto error = target->Attach(m_options.attach_info, &stream);
 if (error.Success()) {
-  ProcessSP process_sp(target->GetProcessSP());
+  process_sp = target->GetProcessSP();
   if (process_sp) {
 result.AppendMessage(stream.GetString());
 result.SetStatus(eReturnStatusSuccessFinishNoResult);
@@ -452,8 +453,13 @@
 
 // This supports the use-case scenario of immediately continuing the
 // process once attached.
-if (m_options.attach_info.GetContinueOnceAttached())
-  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
+if (m_options.attach_info.GetContinueOnceAttached()) {
+  // We have made a process but haven't told the interpreter about it yet,
+  // so CheckRequirements will fail for "process continue".  Set the 
override
+  // here:
+  ExecutionContext exe_ctx(process_sp);
+  m_interpreter.HandleCommand("process continue", eLazyBoolNo, exe_ctx, 
result);
+}
 
 return result.Succeeded();
   }


Index: lldb/test/API/commands/process/attach/TestProcessAttach.py
===
--- lldb/test/API/commands/process/attach/TestProcessAttach.py
+++ lldb/test/API/commands/process/attach/TestProcessAttach.py
@@ -43,6 +43,23 @@
 process = target.GetProcess()
 self.assertTrue(process, PROCESS_IS_VALID)
 
+@skipIfiOSSimulator
+def test_attach_to_process_by_id_autocontinue(self):
+"""Test attach by process id"""
+self.build()
+exe = self.getBuildArtifact(exe_name)
+
+# Spawn a new process
+popen = self.spawnSubprocess(exe)
+
+self.runCmd("process attach -c -p " + str(popen.pid))
+
+target = self.dbg.GetSelectedTarget()
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertTrue(process.GetState(), lldb.eStateRunning)
+
 @skipIfReproducer # FIXME: Unexpected packet during (active) replay
 @skipIfWindows # This is flakey on Windows AND when it fails, it hangs: llvm.org/pr48806
 def test_attach_to_process_from_different_dir_by_id(self):
Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -398,9 +398,10 @@
 }
 
 StreamString stream;
+ProcessSP process_sp;
 const auto error = target->Attach(m_options.attach_info, &stream);
 if (error.Success()) {
-  ProcessSP process_sp(target->GetProcessSP());
+  process_sp = target->GetProcessSP();
   if (process_sp) {
 result.AppendMessage(stream.GetString());
 result.SetStatus(eReturnStatusSuccessFinishNoResult);
@@ -452,8 +453,13 @@
 
 // This supports the use-case scenario of immediately continuing the
 // process once attached.
-if (m_options.attach_info.GetContinueOnceAttached())
-  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
+if (m_options.attach_

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

In D110571#3031846 , @clayborg wrote:

> In D110571#3031140 , @jarin wrote:
>
>> For illustration:
>>
>>   0x100  DW_TAG_subprogram
>>DW_AT_name "inlined_function"
>>... no DW_AT_low_pc here ...
>>   0x110DW_TAG_formal_parameter
>>  DW_AT_name "unused"
>>  ...
>>  ...
>>   0x200  DW_TAG_subprogram
>>DW_AT_name("top_level_function_with_address"
>>DW_AT_low_pc  (0x3000)
>>DW_AT_high_pc  (0x3100)
>>...
>>   0x210DW_TAG_inlined_subroutine
>>  DW_AT_abstract_origin (0x100 "inlined_function")
>>  DW_AT_low_pc  (0x3010)
>>  DW_AT_high_pc  (0x3020)
>>  # Note the missing DW_TAG_formal_parameter here!
>>NULL
>>  ...
>>   0x400  DW_TAG_subprogram
>>DW_AT_name("another_top_level_function_with_address"
>>DW_AT_low_pc  (0x5000)
>>DW_AT_high_pc  (0x5100)
>>...
>>   0x410DW_TAG_inlined_subroutine
>>  DW_AT_abstract_origin (0x100 "inlined_function")
>>  DW_AT_low_pc  (0x5030)
>>  DW_AT_high_pc  (0x5040)
>>  # Note the missing DW_TAG_formal_parameter here!
>>NULL
>>  ...
>>
>> Here, we will create just one variable for the formal parameter "unused" 
>> (DIE offset 0x110). That variable's symbol context will be randomly one of 
>> the DW_TAG_inline subroutine blocks (either 0x210 or 0x410), and the 
>> variable will be inserted into two variable lists, one for the Block 
>> associated with the DIE at 0x210 and one for DIE associated with 0x410.
>
> I hear what you are saying, but I am not sure this will be happening. Let me 
> explain: for each concrete DW_TAG_subprogram (0x200 and 0x400 in your example 
> above), we create a unique lldb_private::Function object whose UserID will be 
> 0x200 for "top_level_function_with_address" and 0x400 for 
> "another_top_level_function_with_address". Each of those functions might be 
> asked for their lldb_private::Block objects at some point and we should 
> create unique lldb_private::Block for each DW_TAG_lexical_block and 
> DW_TAG_inlined_subroutine that is contained within these unique DIEs. Each of 
> these should now have a variable within the block that is a parameter whose 
> name is "unused" and whose symbol context should be 0x210 for the 0x200 DIE, 
> and 0x410 for the 0x400 DIE. So it would be great to make sure this happens 
> correctly. From looking at the code, it seems like this should be happening 
> correctly, but you might know better since you made these new modifications.

Hi Greg, thanks for the detailed description! What you say is indeed happening 
until the point "Each of these [blocks] should now have a variable within the 
block that is a parameter whose name is "unused" and whose symbol context 
should be 0x210 for the 0x200 DIE, and 0x410 for the 0x400 DIE.".

With my patch, LLDB creates only one variable here, its symbol context will be 
whichever block was parsed first and that variable will be inserted into the 
variable lists of blocks corresponding to 0x210 and 0x410. The reason why LLDB 
creates only one variable is that there is a cache of variables indexed by 
DIEs. When we call ParseVariableDIE 

 first time for the variable "unused" (DIE 0x110) and symbol context 0x210, the 
variable gets created 

 and inserted 

 under the key 0x110. When we call ParseVariableDIE second time for "unused" 
(still 0x110) and symbol context 0x410, we will find and return 

 the originally created variable (with symbol context 0x210!) and happily 
insert it into the block for 0x410.

From what you say, this is not the desired behavior? If we wanted two instances 
of the variable (one for each block), we could change the DIE-to-variable cache 

 to be indexed by a pair .

I have validated this with a simple example below, after adding printing of the 
variable address (var_sp.get()) at its creation point 

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

>> - We could supply a test written in C, but it needs -O1 and is fairly 
>> sensitive to the meaning of -O1 (e.g., clang started inlining and omitting 
>> unsued inlined parameters only recently, so changes to -O1 could make a C 
>> test easily meaningless). Any concerns here?
>
> It is really hard to make sure the compiler generates what you want for a 
> test case as it will change over time and you might not end up testing what 
> you think you are testing. The easiest way to avoid this is to emit the 
> assembly from the compiler and then use that as the source for the test since 
> that will guarantee the same output.

If anyone ever needs a hand constructing a stable debug info test case using 
clang (or other compilers for that matter) - I'm totally happy to help. It's 
quite possible to constrain the compiler enough and give it easy enough things 
to inline to make it pretty reliable - for instance for this sort of issue, I'd 
expect something like this is what I'd use to demonstrate a missing parameter:

  __attribute__((optnone)) __attribute__((nodebug)) void use(int*) { }
  inline void f1(int a, int b) {
use(&b);
  }
  int main() {
f1(5,  6);
  }

This compiled with some optimizations (-O1 or above should be adequate) should 
result in a single concrete subprogram for main, a single inlined subroutine 
with a single formal parameter in the inlined subroutine (for 'b') and the 
abstract origin will have both 'a' and 'b'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571

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