date:20220329

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

2022-03-29 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D121631#3387077 , @clayborg wrote:

> In D121631#3386244 , @labath wrote:
>
>> In D121631#3384124 , @yinghuitan 
>> wrote:
>>
 unify with `target.preload-symbols` feature
>>>
>>> I do not have strong opinion on this. One concern is that 
>>> `target.preload-symbols` provides full experience but 
>>> `symbols.load-on-demand` provides trade-off experience (for example some 
>>> global expression evaluation may fail because we won't automatically 
>>> hydrate based on type query).
>>
>> That is a fair point, but the having them separate also has downsides, as we 
>> have to figure out what is the meaning of on-demand=true&preload=true combo 
>> going to be. I think it would be strange to have on-demand take precedence 
>> in this setup, but then if it doesn't, then you need to change *two* 
>> settings to switch from "preload everything" (the current default) to the 
>> "on demand mode". I am not particularly invested in any solution, but I 
>> think we should make a conscious choice one way or the other.
>
> These two settings can co-exist pretty easily. If preload is on, preloading 
> can be done for anything that has debug info enabled, and would be done as 
> soon as debug info was enabled for on demand. It would be like saying "please 
> preload symbols as soon as debug info is enabled". Right now if we enable 
> debug info due to a breakpoint, function, or global lookup that matches a 
> symbol, we can immediately preload the symbols so they are ready regardless 
> of wether a name lookup happens just like we do normally.
>
> If preload is off, then we wait until someone makes a global name lookup call.

I understand your point, and I see how having a ternary setting where (exactly) 
one of the values creates a completely different debug experience could be 
confusing. When you describe it like this, I think the behavior makes sense. 
The part I am worried about is someone getting the relationship backwards (with 
preload-symbols being "on top"), and then wondering "why the %#@# is aren't my 
symbols loaded when I said you should preload them". And I do think one could 
think this is the expected behavior, since the (whether you like it or not) 
idea behind the preloading was to load everything up-front instead of at a 
random point in the debug session -- something which is no longer true when the 
on-demand feature is enabled.

Overall, I am still fairly fond of the ternary idea, but I am not going to push 
for it if there's interest in it. Having two settings makes sense too, if we 
can clearly communicate their relationship. One thing that might help is 
renaming (one or both) something so that both settings aren't using the word 
"load" (I'll note that you used the phrase "debug info is enabled" in your 
exposition), though I am not sure what would those names be.

In D121631#3397355 , @yinghuitan 
wrote:

> Remove ondemand test category/flavor
> Add new testcases:
>
> - source text regex breakpoint
> - regex breakpoint filter by language
> - global variable symbol table match hydration

Awesome. Thanks.

Comment at: 
lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+

Is this here because there is no portable way to create shared libraries in 
shell tests? Would it be possible to run this test on other systems as well if 
this were written as an API test (where we have a shared lib portability layer)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D122660: [lldb] Avoid duplicate vdso modules when opening core files

2022-03-29 Thread Pavel Labath via Phabricator via lldb-commits

labath created this revision.
labath added reviewers: mgorny, clayborg, JDevlieghere.
Herald added subscribers: pmatos, asb, sunfish, sbc100.
Herald added a project: All.
labath requested review of this revision.
Herald added a subscriber: aheejin.
Herald added a project: LLDB.

When opening core files (and also in some other situations) we could end
up with two vdso modules. This could happen because the vdso module is
very special, and over the years, we have accumulated various ways to
load it.

In D10800 , we added one mechanism for loading 
it, which took the form of
a generic load-from-memory capability. Unfortunately loading an elf file
from memory is not possible (because the loader never loads the entire
file), and our attempts to do so were causing crashes. So, in D34352 
, we
partially reverted D10800  and implemented a 
custom mechanism specific to
the vdso.

Unfortunately, enough of D10800  remained such 
that, under the right
circumstances, it could end up loading a second (non-functional) copy of
the vdso module. This happened when the process plugin did not support
the extended MemoryRegionInfo query (added in D22219 
, to workaround a
different bug), which meant that the loader plugin was not able to
recognise that the linux-vdso.so.1 module (this is how the loader calls
it) is in fact the same as the [vdso] module (the name used in
/proc/$PID/maps) we loaded before. This typically happened in a core
file, as they don't store this kind of information.

This patch fixes the issue by completing the revert of D10800 
 -- the
memory loading code is removed completely. It also reduces the scope of
the hackaround introduced in D22219  -- it 
isn't completely sound and is
only relevant for fairly old (but still supported) versions of android.

I added the memory loading logic to the wasm dynamic loader, which has
since appeared and is relying on this feature (it even has a test). As
far as I can tell loading wasm modules from memory is possible and
reliable. MachO memory loading is not affected by this patch, as it uses
a completely different code path.

Since the scenarios/patches I described came without test cases, I have
created two new gdb-client tests cases for them. They're not
particularly readable, but right now, this is the best way we can
simulate the behavior (bugs) of a particular dynamic linker.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122660

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/packages/Python/lldbsuite/test/gdbclientutils.py
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
  lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
  lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
  lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
  lldb/test/API/functionalities/gdb_remote_client/TestWasm.py
  lldb/test/API/functionalities/gdb_remote_client/module_load.yaml

Index: lldb/test/API/functionalities/gdb_remote_client/module_load.yaml
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/module_load.yaml
@@ -0,0 +1,53 @@
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+Address: 0x
+AddressAlign:0x4
+Content: "c3c3c3c3"
+  - Name:.data
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC ]
+Address: 0x1000
+AddressAlign:0x4
+Size:0x28
+  - Name:.dynamic
+Type:SHT_DYNAMIC
+Flags:   [ SHF_WRITE, SHF_ALLOC ]
+AddressAlign:0x8
+Entries:
+  - Tag: DT_DEBUG
+Value:   0xdead
+
+ProgramHeaders:
+  - Type: PT_LOAD
+Flags: [ PF_X, PF_R ]
+VAddr: 0x
+Align: 0x4
+FirstSec: .text
+LastSec:  .text
+  - Type: PT_LOAD
+Flags: [ PF_R, PF_W ]
+VAddr: 0x1000
+Align: 0x4
+FirstSec: .data
+LastSec:  .dynamic
+  - Type:PT_DYNAMIC
+Flags:   [ PF_W, PF_R ]
+VAddr:   0x1028
+FirstSec:.dynamic
+LastSec: .dynamic
+DynamicSymbols:
+  - Name:_r_debug
+Type:STT_OBJECT
+Section: .data
+Binding: STB_GLOBAL
+Value:   0x0
+Size:0x28
+
Index: lldb/test/API/functionalities/gdb_remote_client/TestWasm.py

[Lldb-commits] [lldb] 13a3b0b - [lldb] Remove usages of case-insensitive c-string functions

2022-03-29 Thread Pavel Labath via lldb-commits


Author: Pavel Labath
Date: 2022-03-29T17:59:17+02:00
New Revision: 13a3b0bb4b64fea08f26c654b7a810f9ca217c7a

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

LOG: [lldb] Remove usages of case-insensitive c-string functions

They are not portable (which meant we had a hand-rolled implementation
for windows), and llvm::StringRef provides equivalent functionality.

Added: 


Modified: 
lldb/include/lldb/Host/windows/PosixApi.h
lldb/source/API/SBFrame.cpp
lldb/source/DataFormatters/FormatManager.cpp
lldb/source/Host/windows/Windows.cpp
lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.h
lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.h
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/windows/PosixApi.h 
b/lldb/include/lldb/Host/windows/PosixApi.h
index 563af50fd229e..85e828c80eef1 100644
--- a/lldb/include/lldb/Host/windows/PosixApi.h
+++ b/lldb/include/lldb/Host/windows/PosixApi.h
@@ -89,14 +89,6 @@ typedef uint32_t pid_t;
 // Various useful posix functions that are not present in Windows.  We provide
 // custom implementations.
 int vasprintf(char **ret, const char *fmt, va_list ap);
-char *strcasestr(const char *s, const char *find);
-
-#ifdef _MSC_VER
-
-int strcasecmp(const char *s1, const char *s2);
-int strncasecmp(const char *s1, const char *s2, size_t n);
-
-#endif // _MSC_VER
 
 // empty functions
 inline int posix_openpt(int flag) { LLVM_BUILTIN_UNREACHABLE; }

diff  --git a/lldb/source/API/SBFrame.cpp b/lldb/source/API/SBFrame.cpp
index a67663fb09601..ea9c2bb747e1e 100644
--- a/lldb/source/API/SBFrame.cpp
+++ b/lldb/source/API/SBFrame.cpp
@@ -637,9 +637,9 @@ SBValue SBFrame::FindValue(const char *name, ValueType 
value_type,
 for (uint32_t set_idx = 0; set_idx < num_sets; ++set_idx) {
   const RegisterSet *reg_set = reg_ctx->GetRegisterSet(set_idx);
   if (reg_set &&
-  ((reg_set->name && strcasecmp(reg_set->name, name) == 0) ||
-   (reg_set->short_name &&
-strcasecmp(reg_set->short_name, name) == 0))) {
+  (llvm::StringRef(reg_set->name).equals_insensitive(name) ||
+   llvm::StringRef(reg_set->short_name)
+   .equals_insensitive(name))) {
 value_sp =
 ValueObjectRegisterSet::Create(frame, reg_ctx, set_idx);
 sb_value.SetSP(value_sp);

diff  --git a/lldb/source/DataFormatters/FormatManager.cpp 
b/lldb/source/DataFormatters/FormatManager.cpp
index c42a5fbfcebb0..a8390c5d79c6a 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -89,11 +89,11 @@ static bool GetFormatFromFormatChar(char format_char, 
Format &format) {
   return false;
 }
 
-static bool GetFormatFromFormatName(const char *format_name,
+static bool GetFormatFromFormatName(llvm::StringRef format_name,
 bool partial_match_ok, Format &format) {
   uint32_t i;
   for (i = 0; i < g_num_format_infos; ++i) {
-if (strcasecmp(g_format_infos[i].format_name, format_name) == 0) {
+if (format_name.equals_insensitive(g_format_infos[i].format_name)) {
   format = g_format_infos[i].format;
   return true;
 }
@@ -101,8 +101,8 @@ static bool GetFormatFromFormatName(const char *format_name,
 
   if (partial_match_ok) {
 for (i = 0; i < g_num_format_infos; ++i) {
-  if (strcasestr(g_format_infos[i].format_name, format_name) ==
-  g_format_infos[i].format_name) {
+  if (llvm::StringRef(g_format_infos[i].format_name)
+  .startswith_insensitive(format_name)) {
 format = g_format_infos[i].format;
 return true;
   }

diff  --git a/lldb/source/Host/windows/Windows.cpp 
b/lldb/source/Host/windows/Windows.cpp
index d2561a6c5b6df..a74858301ee5a 100644
--- a/lldb/source/Host/windows/Windows.cpp
+++ b/lldb/source/Host/windows/Windows.cpp
@@ -44,32 +44,8 @@ int vasprintf(char **ret, const char *fmt, va_list ap) {
   return len;
 }
 
-char *strcasestr(const char *s, const char *find) {
-  char c, sc;
-  size_t len;
-
-  if ((c = *find++) != 0) {
-c = tolower((unsigned char)c);
-len = strlen(find);
-do {
-  do {
-if ((sc = *s++) == 0)
-  return 0;
-  } while ((char)tolower((unsigned char)sc) != c);
-} while (strncasecmp(s, find, len) != 0);
-s--;
-  }
-  return const_cast(s);
-}

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

2022-03-29 Thread jeffrey tan via Phabricator via lldb-commits

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



Comment at: 
lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+

labath wrote:
> Is this here because there is no portable way to create shared libraries in 
> shell tests? Would it be possible to run this test on other systems as well 
> if this were written as an API test (where we have a shared lib portability 
> layer)?
Yeah, that's one reason. Another reason is the name of the shared library (used 
for checking global variables/debug info) varies across platform. For example, 
it can be libfoo.dylib (Mac) vs libfoo.so (linux) vs *foo*.dll (windows). I 
find it cumbersome to support multiple platforms. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

2022-03-29 Thread Pavel Labath via Phabricator via lldb-commits

labath added inline comments.



Comment at: 
lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+

yinghuitan wrote:
> labath wrote:
> > Is this here because there is no portable way to create shared libraries in 
> > shell tests? Would it be possible to run this test on other systems as well 
> > if this were written as an API test (where we have a shared lib portability 
> > layer)?
> Yeah, that's one reason. Another reason is the name of the shared library 
> (used for checking global variables/debug info) varies across platform. For 
> example, it can be libfoo.dylib (Mac) vs libfoo.so (linux) vs *foo*.dll 
> (windows). I find it cumbersome to support multiple platforms. 
I'm not saying you have to go out of your way to support other platforms and 
their idiosyncrasies, but the differences in library names hardly seem like an 
insurmountable obstacle. We already have abstractions in the API tests 
(`self.platformContext.{shlib_prefix,shlib_extension}`) which exist precisely 
for this reason. There's no function to automatically construct the library 
name (you have to do the concatenation yourself), but I certainly wouldn't be 
opposed to adding one.

If it helps, think of it as courtesy towards other developers who may need to 
debug this feature when they break these tests -- that's much easier to achieve 
if you can reproduce the failure locally vs. just getting a failure email from 
a random bot (after you've already submitted your patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D122603: [wip][intelpt] Refactor timestamps out of `IntelPTInstruction`

2022-03-29 Thread Alisamar Husain via Phabricator via lldb-commits

zrthxn updated this revision to Diff 418937.
zrthxn marked 12 inline comments as done.
zrthxn added a comment.

Introduced TscRange for quicker operation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603

Files:
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -42,6 +42,8 @@
   DecodedThreadSP m_decoded_thread_sp;
   /// Internal instruction index currently pointing at.
   size_t m_pos;
+  /// Current instruction timestamp.
+  llvm::Optional m_current_tsc;
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -23,6 +23,7 @@
   assert(!m_decoded_thread_sp->GetInstructions().empty() &&
  "a trace should have at least one instruction or error");
   m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
+  m_current_tsc = m_decoded_thread_sp->GetTSCRange(m_pos);
 }
 
 size_t TraceCursorIntelPT::GetInternalInstructionSize() {
@@ -40,6 +41,12 @@
 
   while (canMoveOne()) {
 m_pos += IsForwards() ? 1 : -1;
+
+if (m_pos > m_current_tsc->end_index)
+  m_current_tsc = m_decoded_thread_sp->GetNextTSCRange(*m_current_tsc);
+if (m_pos < m_current_tsc->start_index)
+  m_current_tsc = m_decoded_thread_sp->GetPrevTSCRange(*m_current_tsc);
+
 if (!m_ignore_errors && IsError())
   return true;
 if (GetInstructionControlFlowType() & m_granularity)
@@ -61,20 +68,24 @@
   switch (origin) {
   case TraceCursor::SeekType::Set:
 m_pos = fitPosToBounds(offset);
+m_current_tsc = m_decoded_thread_sp->GetTSCRange(m_pos);
 return m_pos;
   case TraceCursor::SeekType::End:
 m_pos = fitPosToBounds(offset + last_index);
+// not sure if this should be last_index - m_pos or just m_pos
+m_current_tsc = m_decoded_thread_sp->GetTSCRange(last_index - m_pos);
 return last_index - m_pos;
   case TraceCursor::SeekType::Current:
 int64_t new_pos = fitPosToBounds(offset + m_pos);
 int64_t dist = m_pos - new_pos;
 m_pos = new_pos;
+m_current_tsc = m_decoded_thread_sp->GetTSCRange(m_pos);
 return std::abs(dist);
   }
 }
 
 bool TraceCursorIntelPT::IsError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
+  return m_decoded_thread_sp->IsInstructionAnError(m_pos);
 }
 
 const char *TraceCursorIntelPT::GetError() {
@@ -88,7 +99,7 @@
 Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
   switch (counter_type) {
 case lldb::eTraceCounterTSC:
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  return m_current_tsc->tsc;
   }
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
===
--- lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
+++ lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
@@ -62,9 +62,6 @@
 /// As mentioned, any gap is represented as an error in this class.
 class IntelPTInstruction {
 public:
-  IntelPTInstruction(const pt_insn &pt_insn, uint64_t timestamp)
-  : m_pt_insn(pt_insn), m_timestamp(timestamp), m_is_error(false) {}
-
   IntelPTInstruction(const pt_insn &pt_insn)
   : m_pt_insn(pt_insn), m_is_error(false) {}
 
@@ -85,13 +82,6 @@
   /// Get the size in bytes of an instance of this class
   static size_t GetMemoryUsage();
 
-  /// Get the timestamp associated with the current instruction. The timestamp
-  /// is similar to what a rdtsc instruction would return.
-  ///
-  /// \return
-  /// The timestamp or \b llvm::None if not available.
-  llvm::Optional GetTimestampCounter() const;
-
   /// Get the \a lldb::TraceInstructionControlFlowType categories of the
   /// instruction.
   ///
@@ -113,7 +103,6 @@
   // When adding new members to this class, make sure to update
   // IntelPTInstruction::GetNonErrorMemoryUsage() if needed.
   pt_insn m_pt_insn;
-  llvm::Optional m_timestamp;
   bool m_is_error;
 };
 
@@ -131,6 +120,15 @@
   /// Utility constructor that initializes the trace with a provided error.
   DecodedThread(lldb::ThreadSP thread_sp, llvm::Error &&err);
 
+  /// Append a successfully decoded instruction.
+  void AppendInstruction(const pt_insn& instruction);
+
+  /// Append a timestamp at the index of the last instruction.
+  void AppendInstruction(const pt_insn& instruction, uint64_t timestamp);
+
+  //

[Lldb-commits] [PATCH] D122603: [wip][intelpt] Refactor timestamps out of `IntelPTInstruction`

2022-03-29 Thread walter erquinigo via Phabricator via lldb-commits

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

I'm proposing a new interface for the TscRange. Let me know if you have 
questions




Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:95
+  m_instructions.emplace_back(insn);
+  if (__last_tsc != tsc) {
+m_instruction_timestamps.emplace(m_instructions.size() - 1, tsc);

we need to update it because of the optional



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:126-127
+
+  /// Append a timestamp at the index of the last instruction.
+  void AppendInstruction(const pt_insn& instruction, uint64_t timestamp);
+

Let's improve the documentation and let's use the word TSC more ubiquitously 



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:144
+
+  // Range of timestamp iterators for a given index
+  struct TscRange {





Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:145-153
+  struct TscRange {
+uint64_t tsc;
+size_t start_index;
+size_t end_index;
+std::map::const_iterator next;
+std::map::const_iterator prev;
+

Let's add more logic to this object so that it handles as much as it can and we 
reduce the logic that was added to DecodedThread. We also don't need two 
iterators, just one is enough.  Don't forget to add documentation to these new 
methods.


Given this new definition for TscRange. The only method we need to add in 
DecodedThread is CalculateTscRange(size_t insn_index), and mention the 
documentation that this operation is O(logn) and should be used sparingly.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:146-148
+uint64_t tsc;
+size_t start_index;
+size_t end_index;





Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:156-163
+  /// Get timestamp of an instruction by its index.
+  llvm::Optional GetTSCRange(size_t insn_index) const;
+
+  /// Get timestamp of an instruction by its index.
+  llvm::Optional GetNextTSCRange(const TscRange& range) const;
+  
+  /// Get timestamp of an instruction by its index.

Let's improve the documentation and also get rid of the `struct` keyword in 
return types. That's old style C



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:196-202
   lldb::ThreadSP m_thread_sp;
   std::vector m_instructions;
+  std::map m_instruction_timestamps;
   llvm::DenseMap m_errors;
   llvm::Optional m_raw_trace_size;
+
+  uint64_t __last_tsc;

don't start variable names with __, as may people think that those variables 
should be discarded. Let's just give it a proper name. Let's also use an 
Optional and let's add documentation to all the variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603

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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

2022-03-29 Thread jeffrey tan via Phabricator via lldb-commits

yinghuitan added inline comments.



Comment at: 
lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+

labath wrote:
> yinghuitan wrote:
> > labath wrote:
> > > Is this here because there is no portable way to create shared libraries 
> > > in shell tests? Would it be possible to run this test on other systems as 
> > > well if this were written as an API test (where we have a shared lib 
> > > portability layer)?
> > Yeah, that's one reason. Another reason is the name of the shared library 
> > (used for checking global variables/debug info) varies across platform. For 
> > example, it can be libfoo.dylib (Mac) vs libfoo.so (linux) vs *foo*.dll 
> > (windows). I find it cumbersome to support multiple platforms. 
> I'm not saying you have to go out of your way to support other platforms and 
> their idiosyncrasies, but the differences in library names hardly seem like 
> an insurmountable obstacle. We already have abstractions in the API tests 
> (`self.platformContext.{shlib_prefix,shlib_extension}`) which exist precisely 
> for this reason. There's no function to automatically construct the library 
> name (you have to do the concatenation yourself), but I certainly wouldn't be 
> opposed to adding one.
> 
> If it helps, think of it as courtesy towards other developers who may need to 
> debug this feature when they break these tests -- that's much easier to 
> achieve if you can reproduce the failure locally vs. just getting a failure 
> email from a random bot (after you've already submitted your patch).
> self.platformContext.{shlib_prefix,shlib_extension}

Ah, did not know their existence, thanks! Will add API tests for them. Do you 
prefer to keep these linux-only shell tests or completely deprecate them?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D122680: Add a setting to force overwriting commands in "command script add"

2022-03-29 Thread Jim Ingham via Phabricator via lldb-commits

jingham created this revision.
jingham added a reviewer: JDevlieghere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When I added containers it seemed like it would be easier to accidentally 
overwrite extant commands, so I required a --overwrite option before "command 
script add" will overwrite an extant command.  That still strikes me as a good 
idea for users of packages of commands, but when you are developing the 
commands it's annoying since it means you have to change the "command script 
add" commands in your package while iteratively adding them during development. 
 So this patch adds a setting to turn off the overwrite requirement.  That way 
you can set this setting while developing your package, then turn it off again 
when you're acting as a consumer of packages.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122680

Files:
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Interpreter/InterpreterProperties.td
  lldb/test/API/commands/command/container/TestContainerCommands.py

Index: lldb/test/API/commands/command/container/TestContainerCommands.py
===
--- lldb/test/API/commands/command/container/TestContainerCommands.py
+++ lldb/test/API/commands/command/container/TestContainerCommands.py
@@ -57,8 +57,9 @@
 self.expect("test-multi test-multi-sub welcome friend", "Test command works",
 substrs=["Hello friend, welcome to LLDB"])
 
-# Make sure overwriting works, first the leaf command:
-# We should not be able to remove extant commands by default:
+# Make sure overwriting works on the leaf command.  First using the
+# explicit option so we should not be able to remove extant commands by default:
+
 self.expect("command script add -c welcome.WelcomeCommand2 test-multi test-multi-sub welcome",
 "overwrite command w/o -o",
 substrs=["cannot add command: sub-command already exists"], error=True)
@@ -67,9 +68,15 @@
 # Make sure we really did overwrite:
 self.expect("test-multi test-multi-sub welcome friend", "Used the new command class",
 substrs=["Hello friend, welcome again to LLDB"])
-
 self.expect("apropos welcome", "welcome should show up in apropos", substrs=["A docstring for the second Welcome"])
 
+# Now switch the default and make sure we can now delete w/o the overwrite option:
+self.runCmd("settings set interpreter.require-overwrite 0")
+self.runCmd("command script add -c welcome.WelcomeCommand test-multi test-multi-sub welcome")
+# Make sure we really did overwrite:
+self.expect("test-multi test-multi-sub welcome friend", "Used the new command class",
+substrs=["Hello friend, welcome to LLDB"])
+
 # Make sure we give good errors when the input is wrong:
 self.expect("command script delete test-mult test-multi-sub welcome", "Delete script command - wrong first path component",
 substrs=["'test-mult' not found"], error=True)
Index: lldb/source/Interpreter/InterpreterProperties.td
===
--- lldb/source/Interpreter/InterpreterProperties.td
+++ lldb/source/Interpreter/InterpreterProperties.td
@@ -36,4 +36,8 @@
 Global,
 DefaultTrue,
 Desc<"If true, LLDB will repeat the previous command if no command was passed to the interpreter. If false, LLDB won't repeat the previous command but only return a new prompt.">;
+  def RequireCommandOverwrite: Property<"require-overwrite", "Boolean">,
+Global,
+DefaultTrue,
+Desc<"If true, require --overwrite in 'command script add' before overwriting existing user commands.">;
 }
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -244,6 +244,12 @@
   nullptr, idx, g_interpreter_properties[idx].default_uint_value != 0);
 }
 
+bool CommandInterpreter::GetRequireCommandOverwrite() const {
+  const uint32_t idx = ePropertyRequireCommandOverwrite;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_interpreter_properties[idx].default_uint_value != 0);
+}
+
 void CommandInterpreter::Initialize() {
   LLDB_SCOPED_TIMER();
 
Index: lldb/source/Commands/CommandObjectCommands.cpp
===
--- lldb/source/Commands/CommandObjectCommands.cpp
+++ lldb/source/Commands/CommandObjectCommands.cpp
@@ -1445,7 +1445,7 @@
   m_short_help = std::string(option_arg);
 bre

[Lldb-commits] [lldb] dfde354 - NFC. Fixing warnings from adding DXContainer

2022-03-29 Thread Chris Bieneman via lldb-commits


Author: Chris Bieneman
Date: 2022-03-29T14:46:24-05:00
New Revision: dfde354958ca49138349e1428bf2fc0414a73a43

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

LOG: NFC. Fixing warnings from adding DXContainer

Adds DXContainer to switch statements in Clang and LLDB to silence
warnings.

Added: 


Modified: 
clang/lib/CodeGen/BackendUtil.cpp
clang/lib/CodeGen/CGObjCMac.cpp
clang/lib/CodeGen/CodeGenModule.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index eaf34eedcb2bb..61677d368029e 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -287,6 +287,7 @@ static bool asanUseGlobalsGC(const Triple &T, const 
CodeGenOptions &CGOpts) {
   case Triple::XCOFF:
 llvm::report_fatal_error("ASan not implemented for XCOFF.");
   case Triple::Wasm:
+  case Triple::DXContainer:
   case Triple::UnknownObjectFormat:
 break;
   }

diff  --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp
index 94d495a993835..2ec9ef9ae0605 100644
--- a/clang/lib/CodeGen/CGObjCMac.cpp
+++ b/clang/lib/CodeGen/CGObjCMac.cpp
@@ -5069,6 +5069,7 @@ std::string CGObjCCommonMac::GetSectionName(StringRef 
Section,
   case llvm::Triple::Wasm:
   case llvm::Triple::GOFF:
   case llvm::Triple::XCOFF:
+  case llvm::Triple::DXContainer:
 llvm::report_fatal_error(
 "Objective-C support is unimplemented for object file format");
   }

diff  --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
index 5fe14953caf6c..08117809c56fb 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -5490,6 +5490,8 @@ CodeGenModule::GetAddrOfConstantCFString(const 
StringLiteral *Literal) {
 llvm_unreachable("GOFF is not yet implemented");
   case llvm::Triple::XCOFF:
 llvm_unreachable("XCOFF is not yet implemented");
+  case llvm::Triple::DXContainer:
+llvm_unreachable("DXContainer is not yet implemented");
   case llvm::Triple::COFF:
   case llvm::Triple::ELF:
   case llvm::Triple::Wasm:

diff  --git 
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp 
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index d258d18497b95..fc0b41fcd7ee2 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2230,6 +2230,7 @@ bool 
GDBRemoteCommunicationClient::GetCurrentProcessInfo(bool allow_lazy) {
 case llvm::Triple::GOFF:
 case llvm::Triple::Wasm:
 case llvm::Triple::XCOFF:
+case llvm::Triple::DXContainer:
   LLDB_LOGF(log, "error: not supported target architecture");
   return false;
 case llvm::Triple::UnknownObjectFormat:



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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

2022-03-29 Thread Pavel Labath via Phabricator via lldb-commits

labath added inline comments.



Comment at: 
lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+

yinghuitan wrote:
> labath wrote:
> > yinghuitan wrote:
> > > labath wrote:
> > > > Is this here because there is no portable way to create shared 
> > > > libraries in shell tests? Would it be possible to run this test on 
> > > > other systems as well if this were written as an API test (where we 
> > > > have a shared lib portability layer)?
> > > Yeah, that's one reason. Another reason is the name of the shared library 
> > > (used for checking global variables/debug info) varies across platform. 
> > > For example, it can be libfoo.dylib (Mac) vs libfoo.so (linux) vs 
> > > *foo*.dll (windows). I find it cumbersome to support multiple platforms. 
> > I'm not saying you have to go out of your way to support other platforms 
> > and their idiosyncrasies, but the differences in library names hardly seem 
> > like an insurmountable obstacle. We already have abstractions in the API 
> > tests (`self.platformContext.{shlib_prefix,shlib_extension}`) which exist 
> > precisely for this reason. There's no function to automatically construct 
> > the library name (you have to do the concatenation yourself), but I 
> > certainly wouldn't be opposed to adding one.
> > 
> > If it helps, think of it as courtesy towards other developers who may need 
> > to debug this feature when they break these tests -- that's much easier to 
> > achieve if you can reproduce the failure locally vs. just getting a failure 
> > email from a random bot (after you've already submitted your patch).
> > self.platformContext.{shlib_prefix,shlib_extension}
> 
> Ah, did not know their existence, thanks! Will add API tests for them. Do you 
> prefer to keep these linux-only shell tests or completely deprecate them?
> 
> 
If they're going to be testing the exact same thing, then I don't see a reason 
for keeping these around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, jingham, clayborg, jasonmolenda.
Herald added subscribers: mgrang, mgorny.
Herald added a project: All.
JDevlieghere requested review of this revision.

Currently we error out if multiple platforms can supports the architectures in 
a fat binary. For example, on Darwin, almost every architecture supported by 
the host platform is also supported by the remote-macosx platform. Currently, 
the only way to disambiguate between the two is to specify the desired 
architecture.

This patch changes the behavior to account for the selected and host platform. 
The new algorithm to pick a platform works as follows:

1. Prefer the selected platform if it supports all architectures in the binary,
2. Prefer the host platform if it supports all architectures in the binary,
3. Prefer the platform that supports all architectures in the binary,
4. Prefer the selected platform if it supports one architecture in the binary,
5. Prefer the host platform if it supports one architecture in the binary,
6. If none of the above apply, error out saying either no or multiple platforms 
support the architectures in the binary

I've added a bunch of unit tests to codify this new behavior.


https://reviews.llvm.org/D122684

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Target/Platform.cpp
  lldb/source/Target/TargetList.cpp
  lldb/unittests/Platform/CMakeLists.txt
  lldb/unittests/Platform/PlatformTest.cpp

Index: lldb/unittests/Platform/PlatformTest.cpp
===
--- /dev/null
+++ lldb/unittests/Platform/PlatformTest.cpp
@@ -0,0 +1,162 @@
+//===-- PlatformTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/Platform/POSIX/PlatformPOSIX.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Platform.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class TestPlatform : public PlatformPOSIX {
+public:
+  TestPlatform() : PlatformPOSIX(false) {}
+  using Platform::Clear;
+};
+
+class PlatformArm : public TestPlatform {
+public:
+  PlatformArm() = default;
+
+  std::vector
+  GetSupportedArchitectures(const ArchSpec &process_host_arch) override {
+return {ArchSpec("arm64-apple-ps4")};
+  }
+
+  llvm::StringRef GetPluginName() override { return "arm"; }
+  llvm::StringRef GetDescription() override { return "arm"; }
+};
+
+class PlatformIntel : public TestPlatform {
+public:
+  PlatformIntel() = default;
+
+  std::vector
+  GetSupportedArchitectures(const ArchSpec &process_host_arch) override {
+return {ArchSpec("x86_64-apple-ps4")};
+  }
+
+  llvm::StringRef GetPluginName() override { return "intel"; }
+  llvm::StringRef GetDescription() override { return "intel"; }
+};
+
+class PlatformThumb : public TestPlatform {
+public:
+  static void Initialize() {
+PluginManager::RegisterPlugin("thumb", "thumb",
+  PlatformThumb::CreateInstance);
+  }
+  static void Terminate() {
+PluginManager::UnregisterPlugin(PlatformThumb::CreateInstance);
+  }
+
+  static PlatformSP CreateInstance(bool force, const ArchSpec *arch) {
+return std::make_shared();
+  }
+
+  std::vector
+  GetSupportedArchitectures(const ArchSpec &process_host_arch) override {
+return {ArchSpec("thumbv7-apple-ps4"), ArchSpec("thumbv7f-apple-ps4")};
+  }
+
+  llvm::StringRef GetPluginName() override { return "thumb"; }
+  llvm::StringRef GetDescription() override { return "thumb"; }
+};
+
+class PlatformTest : public ::testing::Test {
+  SubsystemRAII subsystems;
+  void SetUp() override { TestPlatform::Clear(); }
+};
+
+TEST_F(PlatformTest, GetPlatformForArchitecturesHost) {
+  const PlatformSP host_platform_sp = std::make_shared();
+  Platform::SetHostPlatform(host_platform_sp);
+  ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp);
+
+  const std::vector archs = {ArchSpec("arm64-apple-ps4"),
+   ArchSpec("arm64e-apple-ps4")};
+  std::vector candidates;
+
+  // The host platform matches all architectures.
+  PlatformSP platform_sp =
+  Platform::GetPlatformForArchitectures(archs, {}, {}, candidates);
+  ASSERT_TRUE(platform_sp);
+  EXPECT_EQ(platform_sp, host_platform_sp);
+}
+
+TEST_F(PlatformTest, GetPlatformForArchitecturesSelected) {
+  const PlatformSP host_platform_sp = std::make_shared();
+  Platform::SetHostPlatform(host_platform_sp);
+  ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp);
+
+  const std::vector archs = {ArchSpec("arm64-apple-ps4"),
+

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

2022-03-29 Thread Alisamar Husain via Phabricator via lldb-commits

zrthxn updated this revision to Diff 418975.
zrthxn marked 7 inline comments as done.
zrthxn retitled this revision from "[wip][intelpt] Refactor timestamps out of 
`IntelPTInstruction`" to "[intelpt] Refactor timestamps out of 
IntelPTInstruction".
zrthxn added a comment.

Change TscRange to class


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603

Files:
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -42,6 +42,8 @@
   DecodedThreadSP m_decoded_thread_sp;
   /// Internal instruction index currently pointing at.
   size_t m_pos;
+  /// Current instruction timestamp.
+  llvm::Optional m_current_tsc;
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -23,6 +23,7 @@
   assert(!m_decoded_thread_sp->GetInstructions().empty() &&
  "a trace should have at least one instruction or error");
   m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
+  m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 }
 
 size_t TraceCursorIntelPT::GetInternalInstructionSize() {
@@ -40,6 +41,16 @@
 
   while (canMoveOne()) {
 m_pos += IsForwards() ? 1 : -1;
+
+if (!m_current_tsc)
+  m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
+else if (!m_current_tsc->InRange(m_pos)) {
+  if (m_pos > m_current_tsc->GetEnd())
+m_current_tsc = m_current_tsc->Next();
+  if (m_pos < m_current_tsc->GetStart())
+m_current_tsc = m_current_tsc->Prev();
+}
+
 if (!m_ignore_errors && IsError())
   return true;
 if (GetInstructionControlFlowType() & m_granularity)
@@ -61,20 +72,23 @@
   switch (origin) {
   case TraceCursor::SeekType::Set:
 m_pos = fitPosToBounds(offset);
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 return m_pos;
   case TraceCursor::SeekType::End:
 m_pos = fitPosToBounds(offset + last_index);
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(last_index - m_pos);
 return last_index - m_pos;
   case TraceCursor::SeekType::Current:
 int64_t new_pos = fitPosToBounds(offset + m_pos);
 int64_t dist = m_pos - new_pos;
 m_pos = new_pos;
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 return std::abs(dist);
   }
 }
 
 bool TraceCursorIntelPT::IsError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
+  return m_decoded_thread_sp->IsInstructionAnError(m_pos);
 }
 
 const char *TraceCursorIntelPT::GetError() {
@@ -85,10 +99,14 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+Optional
+TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  if (!m_current_tsc)
+return None;
+
   switch (counter_type) {
-case lldb::eTraceCounterTSC:
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  case lldb::eTraceCounterTSC:
+return m_current_tsc->GetTsc();
   }
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
===
--- lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
+++ lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
@@ -62,9 +62,6 @@
 /// As mentioned, any gap is represented as an error in this class.
 class IntelPTInstruction {
 public:
-  IntelPTInstruction(const pt_insn &pt_insn, uint64_t timestamp)
-  : m_pt_insn(pt_insn), m_timestamp(timestamp), m_is_error(false) {}
-
   IntelPTInstruction(const pt_insn &pt_insn)
   : m_pt_insn(pt_insn), m_is_error(false) {}
 
@@ -85,13 +82,6 @@
   /// Get the size in bytes of an instance of this class
   static size_t GetMemoryUsage();
 
-  /// Get the timestamp associated with the current instruction. The timestamp
-  /// is similar to what a rdtsc instruction would return.
-  ///
-  /// \return
-  /// The timestamp or \b llvm::None if not available.
-  llvm::Optional GetTimestampCounter() const;
-
   /// Get the \a lldb::TraceInstructionControlFlowType categories of the
   /// instruction.
   ///
@@ -113,7 +103,6 @@
   // When adding new members to this class, make sure to update
   // IntelPTInstruction::GetNonErrorMemoryUsage() if needed.
   pt_insn m_pt_insn;
-  llvm::Optional m_timestamp;
   bool m_is_e

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

2022-03-29 Thread Alisamar Husain via Phabricator via lldb-commits

zrthxn updated this revision to Diff 418977.
zrthxn added a comment.

Update memory calc function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603

Files:
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -120,7 +120,7 @@
   s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
(double)mem_used / 1024);
   s.Printf("  Average memory usage per instruction: %zu bytes\n",
-   mem_used / insn_len);
+   (mem_used - *raw_size) / insn_len);
   return;
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -42,6 +42,8 @@
   DecodedThreadSP m_decoded_thread_sp;
   /// Internal instruction index currently pointing at.
   size_t m_pos;
+  /// Current instruction timestamp.
+  llvm::Optional m_current_tsc;
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -23,6 +23,7 @@
   assert(!m_decoded_thread_sp->GetInstructions().empty() &&
  "a trace should have at least one instruction or error");
   m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
+  m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 }
 
 size_t TraceCursorIntelPT::GetInternalInstructionSize() {
@@ -40,6 +41,16 @@
 
   while (canMoveOne()) {
 m_pos += IsForwards() ? 1 : -1;
+
+if (!m_current_tsc)
+  m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
+else if (!m_current_tsc->InRange(m_pos)) {
+  if (m_pos > m_current_tsc->GetEnd())
+m_current_tsc = m_current_tsc->Next();
+  if (m_pos < m_current_tsc->GetStart())
+m_current_tsc = m_current_tsc->Prev();
+}
+
 if (!m_ignore_errors && IsError())
   return true;
 if (GetInstructionControlFlowType() & m_granularity)
@@ -61,20 +72,23 @@
   switch (origin) {
   case TraceCursor::SeekType::Set:
 m_pos = fitPosToBounds(offset);
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 return m_pos;
   case TraceCursor::SeekType::End:
 m_pos = fitPosToBounds(offset + last_index);
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(last_index - m_pos);
 return last_index - m_pos;
   case TraceCursor::SeekType::Current:
 int64_t new_pos = fitPosToBounds(offset + m_pos);
 int64_t dist = m_pos - new_pos;
 m_pos = new_pos;
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 return std::abs(dist);
   }
 }
 
 bool TraceCursorIntelPT::IsError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
+  return m_decoded_thread_sp->IsInstructionAnError(m_pos);
 }
 
 const char *TraceCursorIntelPT::GetError() {
@@ -85,10 +99,14 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+Optional
+TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  if (!m_current_tsc)
+return None;
+
   switch (counter_type) {
-case lldb::eTraceCounterTSC:
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  case lldb::eTraceCounterTSC:
+return m_current_tsc->GetTsc();
   }
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
===
--- lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
+++ lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
@@ -62,9 +62,6 @@
 /// As mentioned, any gap is represented as an error in this class.
 class IntelPTInstruction {
 public:
-  IntelPTInstruction(const pt_insn &pt_insn, uint64_t timestamp)
-  : m_pt_insn(pt_insn), m_timestamp(timestamp), m_is_error(false) {}
-
   IntelPTInstruction(const pt_insn &pt_insn)
   : m_pt_insn(pt_insn), m_is_error(false) {}
 
@@ -85,13 +82,6 @@
   /// Get the size in bytes of an instance of this class
   static size_t GetMemoryUsage();
 
-  /// Get the timestamp associated with the current instruction. The timestamp
-  /// is similar to what a rdtsc instruction would return.
-  ///
-  /// \return
-  /// The timestamp or \b llvm::None if not

[Lldb-commits] [PATCH] D121844: Applying clang-tidy modernize-use-equals-default over LLDB

2022-03-29 Thread Shafik Yaghmour via Phabricator via lldb-commits

shafik marked 2 inline comments as done.
shafik added inline comments.



Comment at: lldb/source/Core/Value.cpp:667
 
-const ValueList &ValueList::operator=(const ValueList &rhs) {
+const ValueList &ValueList::operator=(const ValueList &rhs) {  // 
NOLINT(modernize-use-equals-default)
   m_values = rhs.m_values;

labath wrote:
> shafik wrote:
> > I have to look into why we return a `const &` here. We do this in a few 
> > other places too.
> I don't think there's a good reason for that. Most people aren't aware that 
> built-in assignment operators return lvalues. And some of the people who are 
> aware of that think that it's a bad idea, so they make sure their operators 
> don't do it...
It produced build errors, so some of the users are relying on this. I didn't 
want to plumb into this since it was orthogonal to the change.



Comment at: lldb/source/Host/macosx/cfcpp/CFCMutableArray.cpp:18
 CFCMutableArray::CFCMutableArray(const CFCMutableArray &rhs)
-: CFCReleaser(rhs) // NOTE: this won't make a copy of 
the
+: CFCReleaser(rhs) // 
NOLINT(modernize-use-equals-default)
+  // NOTE: this won't make a copy of 
the

labath wrote:
> Why suppress this?
I wanted to preserve the comment since someone thought it was important.


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

https://reviews.llvm.org/D121844

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

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

2022-03-29 Thread Alisamar Husain via Phabricator via lldb-commits

zrthxn updated this revision to Diff 418981.
zrthxn added a comment.

Prevent crash on printing info when we have 0 instructions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603

Files:
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -119,8 +119,9 @@
   s.Printf("  Total number of instructions: %zu\n", insn_len);
   s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
(double)mem_used / 1024);
-  s.Printf("  Average memory usage per instruction: %zu bytes\n",
-   mem_used / insn_len);
+  if (insn_len != 0)
+s.Printf("  Average memory usage per instruction: %zu bytes\n",
+(mem_used - *raw_size) / insn_len);
   return;
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -42,6 +42,8 @@
   DecodedThreadSP m_decoded_thread_sp;
   /// Internal instruction index currently pointing at.
   size_t m_pos;
+  /// Current instruction timestamp.
+  llvm::Optional m_current_tsc;
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -23,6 +23,7 @@
   assert(!m_decoded_thread_sp->GetInstructions().empty() &&
  "a trace should have at least one instruction or error");
   m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
+  m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 }
 
 size_t TraceCursorIntelPT::GetInternalInstructionSize() {
@@ -40,6 +41,16 @@
 
   while (canMoveOne()) {
 m_pos += IsForwards() ? 1 : -1;
+
+if (!m_current_tsc)
+  m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
+else if (!m_current_tsc->InRange(m_pos)) {
+  if (m_pos > m_current_tsc->GetEnd())
+m_current_tsc = m_current_tsc->Next();
+  if (m_pos < m_current_tsc->GetStart())
+m_current_tsc = m_current_tsc->Prev();
+}
+
 if (!m_ignore_errors && IsError())
   return true;
 if (GetInstructionControlFlowType() & m_granularity)
@@ -61,20 +72,23 @@
   switch (origin) {
   case TraceCursor::SeekType::Set:
 m_pos = fitPosToBounds(offset);
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 return m_pos;
   case TraceCursor::SeekType::End:
 m_pos = fitPosToBounds(offset + last_index);
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(last_index - m_pos);
 return last_index - m_pos;
   case TraceCursor::SeekType::Current:
 int64_t new_pos = fitPosToBounds(offset + m_pos);
 int64_t dist = m_pos - new_pos;
 m_pos = new_pos;
+m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
 return std::abs(dist);
   }
 }
 
 bool TraceCursorIntelPT::IsError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
+  return m_decoded_thread_sp->IsInstructionAnError(m_pos);
 }
 
 const char *TraceCursorIntelPT::GetError() {
@@ -85,10 +99,14 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+Optional
+TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  if (!m_current_tsc)
+return None;
+
   switch (counter_type) {
-case lldb::eTraceCounterTSC:
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  case lldb::eTraceCounterTSC:
+return m_current_tsc->GetTsc();
   }
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
===
--- lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
+++ lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
@@ -62,9 +62,6 @@
 /// As mentioned, any gap is represented as an error in this class.
 class IntelPTInstruction {
 public:
-  IntelPTInstruction(const pt_insn &pt_insn, uint64_t timestamp)
-  : m_pt_insn(pt_insn), m_timestamp(timestamp), m_is_error(false) {}
-
   IntelPTInstruction(const pt_insn &pt_insn)
   : m_pt_insn(pt_insn), m_is_error(false) {}
 
@@ -85,13 +82,6 @@
   /// Get the size in bytes of an instance of this class
   static size_t GetMemoryUsage();
 
-  /// Get the timestam

[Lldb-commits] [lldb] 8991ad4 - [lldb] Make ModuleSpecList iterable (NFC)

2022-03-29 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2022-03-29T15:26:54-07:00
New Revision: 8991ad43ffc56420dda6c797811b2bababe842da

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

LOG: [lldb] Make ModuleSpecList iterable (NFC)

Added: 


Modified: 
lldb/include/lldb/Core/ModuleSpec.h

Removed: 




diff  --git a/lldb/include/lldb/Core/ModuleSpec.h 
b/lldb/include/lldb/Core/ModuleSpec.h
index bcf4ec30a673c..5789e6c8309f6 100644
--- a/lldb/include/lldb/Core/ModuleSpec.h
+++ b/lldb/include/lldb/Core/ModuleSpec.h
@@ -13,6 +13,7 @@
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Iterable.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/UUID.h"
 
@@ -287,7 +288,7 @@ class ModuleSpecList {
 if (this != &rhs) {
   std::lock(m_mutex, rhs.m_mutex);
   std::lock_guard lhs_guard(m_mutex, 
std::adopt_lock);
-  std::lock_guard rhs_guard(rhs.m_mutex, 
+  std::lock_guard rhs_guard(rhs.m_mutex,
   std::adopt_lock);
   m_specs = rhs.m_specs;
 }
@@ -387,8 +388,16 @@ class ModuleSpecList {
 }
   }
 
+  typedef std::vector collection;
+  typedef LockingAdaptedIterable
+  ModuleSpecIterable;
+
+  ModuleSpecIterable ModuleSpecs() {
+return ModuleSpecIterable(m_specs, m_mutex);
+  }
+
 protected:
-  typedef std::vector collection; ///< The module collection type.
   collection m_specs; ///< The collection of modules.
   mutable std::recursive_mutex m_mutex;
 };



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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

So this will fix LLDB complaining when a binary contains arm64 and x86_64 when 
debugging on either platform? Great, we ran into this quite a bit lately!




Comment at: lldb/source/Target/Platform.cpp:1277-1279
+  // Prefer the selected platform if it matches all architectures.
+  if (selected_platform_matches_all_archs)
+return selected_platform_sp;

Shouldn't we just check if the selected platform matches one architecture here 
and return the selected platform? Why do all architectures need to match. If I 
select a platform, and then do "file a.out" and "a.out" contained any number of 
architectures, it should just use the selected platform, no?



Comment at: lldb/source/Target/Platform.cpp:1281-1283
+  // Prefer the host platform if it matches all architectures.
+  if (host_platform_matches_all_archs)
+return host_platform_sp;

Again, why would we not just return the host platform if one arch matches? same 
kind of thing as above?



Comment at: lldb/source/Target/Platform.cpp:1285-1286
+
+  // If there's only one platform left then that means that means that it
+  // supports all architectures.
+  if (candidates.size() == archs.size()) {

Is this comment out of date? We are iterating through more than one platform 
here.



Comment at: lldb/source/Target/Platform.cpp:1287-1294
+  if (candidates.size() == archs.size()) {
+if (std::all_of(candidates.begin(), candidates.end(),
+[&](const PlatformSP &p) -> bool {
+  return p->GetName() == candidates.front()->GetName();
+})) {
+  return candidates.front();
+}

What is this doing? Comparing the first entry in candidates to each of the 
entries and if the name matches, returns the first entry? When would this ever 
not return true?



Comment at: lldb/source/Target/Platform.cpp:1296
+
+  // Prefer the selected platform if it matches at least on architecture.
+  if (selected_platform_matches_one_arch)





Comment at: lldb/source/Target/Platform.cpp:1300
+
+  // Prefer the host platform if it matches at least on architecture.
+  if (host_platform_matches_one_arch)




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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Adrian Prantl via Phabricator via lldb-commits

aprantl added inline comments.



Comment at: lldb/include/lldb/Target/Platform.h:102
 
+  static lldb::PlatformSP
+  GetPlatformForArchitectures(std::vector archs,

Would you mind adding a doxygen comment for this? I'm asking because the 
semantics are not clear to me just from looking at the function signature.



Comment at: lldb/source/Target/Platform.cpp:1228
+  platforms.erase(std::unique(platforms.begin(), platforms.end()),
+  platforms.end());
+}

would std::unique be more idiomatic?



Comment at: lldb/unittests/Platform/PlatformTest.cpp:1
+//===-- PlatformTest.cpp ===//
+//

super-nit: too few `-`s :-)


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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere marked 4 inline comments as done.
JDevlieghere added inline comments.

Comment at: lldb/source/Target/Platform.cpp:1277-1279
+  // Prefer the selected platform if it matches all architectures.
+  if (selected_platform_matches_all_archs)
+return selected_platform_sp;

clayborg wrote:
> Shouldn't we just check if the selected platform matches one architecture 
> here and return the selected platform? Why do all architectures need to 
> match. If I select a platform, and then do "file a.out" and "a.out" contained 
> any number of architectures, it should just use the selected platform, no?
That's a good question. I did this to mimic what we're doing today, i.e. 
preferring the platform that matches all architectures in the fat binary. 

I was trying to imagine what that could look like. One possible scenario is a 
fat binary with triples `arm64-apple-macosx` and `arm64-apple-ios`. In this 
case, both the host platform supports `arm64-apple-macosx` and 
`arm64-apple-ios` 
(https://support.apple.com/guide/app-store/iphone-ipad-apps-mac-apple-silicon-fird2c7092da/mac).
 And `remote-ios` supports `arm64-apple-ios`. If the binary is fat like that, 
it's more than likely meant to run on macosx, so with this algorithm, we'd do 
the right thing. 

The second reason is that the order of the architectures is pretty arbitrary. 
Let's consider the `arm64-apple-macosx` and `arm64-apple-ios` example again. If 
we say that we'll pick whichever matches first, then we'll pick the host 
platform. But if the order was `arm64-apple-ios` and  `arm64-apple-macosx` then 
we'll pick `remote-ios`. 

Anyway I'm not married to this approach. I personally think that picking the 
selected platform is one host matches makes sense. I'm less sure about the host 
platform. 

Comment at: lldb/source/Target/Platform.cpp:1281-1283
+  // Prefer the host platform if it matches all architectures.
+  if (host_platform_matches_all_archs)
+return host_platform_sp;

clayborg wrote:
> Again, why would we not just return the host platform if one arch matches? 
> same kind of thing as above?
See my reply above.

Comment at: lldb/source/Target/Platform.cpp:1285-1286
+
+  // If there's only one platform left then that means that means that it
+  // supports all architectures.
+  if (candidates.size() == archs.size()) {

clayborg wrote:
> Is this comment out of date? We are iterating through more than one platform 
> here.
Yup, I was uniquing the platforms originally.

Comment at: lldb/source/Target/Platform.cpp:1287-1294
+  if (candidates.size() == archs.size()) {
+if (std::all_of(candidates.begin(), candidates.end(),
+[&](const PlatformSP &p) -> bool {
+  return p->GetName() == candidates.front()->GetName();
+})) {
+  return candidates.front();
+}

clayborg wrote:
> What is this doing? Comparing the first entry in candidates to each of the 
> entries and if the name matches, returns the first entry? When would this 
> ever not return true?
This checks that all the platforms are identical. If they're all identical then 
that means that we have one platform that works for all architectures. This 
covers case (3) from the summary.

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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Adrian Prantl via Phabricator via lldb-commits

aprantl added inline comments.



Comment at: lldb/source/Target/Platform.cpp:1228
+  platforms.erase(std::unique(platforms.begin(), platforms.end()),
+  platforms.end());
+}

aprantl wrote:
> would std::unique be more idiomatic?
Not sure why I didn't see this.


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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere marked 2 inline comments as done.
JDevlieghere added a comment.

In D122684#3415303 , @clayborg wrote:

> So this will fix LLDB complaining when a binary contains arm64 and x86_64 
> when debugging on either platform? Great, we ran into this quite a bit lately!

Yes, exactly. The issue is was introduced by https://reviews.llvm.org/D113159. 
Previously, `IsHost()` would be false for `remote-macosx` and the `OSNAME` 
would expand to `ios` returning an absolutely bogus triples. But my change 
fixed that, causing the `remote-macosx` platform to be a real contender, and 
resulting in an error message:

  error: more than one platform supports this executable (host, remote-macosx), 
specify an architecture to disambiguate

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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added inline comments.



Comment at: lldb/include/lldb/Target/Platform.h:103
+  static lldb::PlatformSP
+  GetPlatformForArchitectures(std::vector archs,
+  const ArchSpec &process_host_arch,





Comment at: lldb/source/Target/Platform.cpp:1277-1279
+  // Prefer the selected platform if it matches all architectures.
+  if (selected_platform_matches_all_archs)
+return selected_platform_sp;

JDevlieghere wrote:
> clayborg wrote:
> > Shouldn't we just check if the selected platform matches one architecture 
> > here and return the selected platform? Why do all architectures need to 
> > match. If I select a platform, and then do "file a.out" and "a.out" 
> > contained any number of architectures, it should just use the selected 
> > platform, no?
> That's a good question. I did this to mimic what we're doing today, i.e. 
> preferring the platform that matches all architectures in the fat binary. 
> 
> I was trying to imagine what that could look like. One possible scenario is a 
> fat binary with triples `arm64-apple-macosx` and `arm64-apple-ios`. In this 
> case, both the host platform supports `arm64-apple-macosx` and 
> `arm64-apple-ios` 
> (https://support.apple.com/guide/app-store/iphone-ipad-apps-mac-apple-silicon-fird2c7092da/mac).
>  And `remote-ios` supports `arm64-apple-ios`. If the binary is fat like that, 
> it's more than likely meant to run on macosx, so with this algorithm, we'd do 
> the right thing. 
> 
> The second reason is that the order of the architectures is pretty arbitrary. 
> Let's consider the `arm64-apple-macosx` and `arm64-apple-ios` example again. 
> If we say that we'll pick whichever matches first, then we'll pick the host 
> platform. But if the order was `arm64-apple-ios` and  `arm64-apple-macosx` 
> then we'll pick `remote-ios`. 
> 
> Anyway I'm not married to this approach. I personally think that picking the 
> selected platform is one host matches makes sense. I'm less sure about the 
> host platform. 
I agree, so maybe lets switch to say if the selected platform matches at least 
one, then it is all good. Will the selected platform ever be the host platform? 



Comment at: lldb/source/Target/Platform.cpp:1287-1294
+  if (candidates.size() == archs.size()) {
+if (std::all_of(candidates.begin(), candidates.end(),
+[&](const PlatformSP &p) -> bool {
+  return p->GetName() == candidates.front()->GetName();
+})) {
+  return candidates.front();
+}

JDevlieghere wrote:
> clayborg wrote:
> > What is this doing? Comparing the first entry in candidates to each of the 
> > entries and if the name matches, returns the first entry? When would this 
> > ever not return true?
> This checks that all the platforms are identical. If they're all identical 
> then that means that we have one platform that works for all architectures. 
> This covers case (3) from the summary.
Sounds good, just update the comment then and all good.


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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added inline comments.



Comment at: lldb/source/Target/Platform.cpp:1277-1279
+  // Prefer the selected platform if it matches all architectures.
+  if (selected_platform_matches_all_archs)
+return selected_platform_sp;

clayborg wrote:
> JDevlieghere wrote:
> > clayborg wrote:
> > > Shouldn't we just check if the selected platform matches one architecture 
> > > here and return the selected platform? Why do all architectures need to 
> > > match. If I select a platform, and then do "file a.out" and "a.out" 
> > > contained any number of architectures, it should just use the selected 
> > > platform, no?
> > That's a good question. I did this to mimic what we're doing today, i.e. 
> > preferring the platform that matches all architectures in the fat binary. 
> > 
> > I was trying to imagine what that could look like. One possible scenario is 
> > a fat binary with triples `arm64-apple-macosx` and `arm64-apple-ios`. In 
> > this case, both the host platform supports `arm64-apple-macosx` and 
> > `arm64-apple-ios` 
> > (https://support.apple.com/guide/app-store/iphone-ipad-apps-mac-apple-silicon-fird2c7092da/mac).
> >  And `remote-ios` supports `arm64-apple-ios`. If the binary is fat like 
> > that, it's more than likely meant to run on macosx, so with this algorithm, 
> > we'd do the right thing. 
> > 
> > The second reason is that the order of the architectures is pretty 
> > arbitrary. Let's consider the `arm64-apple-macosx` and `arm64-apple-ios` 
> > example again. If we say that we'll pick whichever matches first, then 
> > we'll pick the host platform. But if the order was `arm64-apple-ios` and  
> > `arm64-apple-macosx` then we'll pick `remote-ios`. 
> > 
> > Anyway I'm not married to this approach. I personally think that picking 
> > the selected platform is one host matches makes sense. I'm less sure about 
> > the host platform. 
> I agree, so maybe lets switch to say if the selected platform matches at 
> least one, then it is all good. Will the selected platform ever be the host 
> platform? 
I believe the selected platform is the host platform if you didn't specify it 
explicitly.


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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere updated this revision to Diff 419005.
JDevlieghere edited the summary of this revision.
JDevlieghere added a comment.

Change the algorithm to pick the selected/host platform if it matches at least 
one architecture.


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

https://reviews.llvm.org/D122684

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Target/Platform.cpp
  lldb/source/Target/TargetList.cpp
  lldb/unittests/Platform/CMakeLists.txt
  lldb/unittests/Platform/PlatformTest.cpp

Index: lldb/unittests/Platform/PlatformTest.cpp
===
--- /dev/null
+++ lldb/unittests/Platform/PlatformTest.cpp
@@ -0,0 +1,162 @@
+//===-- PlatformTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/Platform/POSIX/PlatformPOSIX.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Platform.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class TestPlatform : public PlatformPOSIX {
+public:
+  TestPlatform() : PlatformPOSIX(false) {}
+  using Platform::Clear;
+};
+
+class PlatformArm : public TestPlatform {
+public:
+  PlatformArm() = default;
+
+  std::vector
+  GetSupportedArchitectures(const ArchSpec &process_host_arch) override {
+return {ArchSpec("arm64-apple-ps4")};
+  }
+
+  llvm::StringRef GetPluginName() override { return "arm"; }
+  llvm::StringRef GetDescription() override { return "arm"; }
+};
+
+class PlatformIntel : public TestPlatform {
+public:
+  PlatformIntel() = default;
+
+  std::vector
+  GetSupportedArchitectures(const ArchSpec &process_host_arch) override {
+return {ArchSpec("x86_64-apple-ps4")};
+  }
+
+  llvm::StringRef GetPluginName() override { return "intel"; }
+  llvm::StringRef GetDescription() override { return "intel"; }
+};
+
+class PlatformThumb : public TestPlatform {
+public:
+  static void Initialize() {
+PluginManager::RegisterPlugin("thumb", "thumb",
+  PlatformThumb::CreateInstance);
+  }
+  static void Terminate() {
+PluginManager::UnregisterPlugin(PlatformThumb::CreateInstance);
+  }
+
+  static PlatformSP CreateInstance(bool force, const ArchSpec *arch) {
+return std::make_shared();
+  }
+
+  std::vector
+  GetSupportedArchitectures(const ArchSpec &process_host_arch) override {
+return {ArchSpec("thumbv7-apple-ps4"), ArchSpec("thumbv7f-apple-ps4")};
+  }
+
+  llvm::StringRef GetPluginName() override { return "thumb"; }
+  llvm::StringRef GetDescription() override { return "thumb"; }
+};
+
+class PlatformTest : public ::testing::Test {
+  SubsystemRAII subsystems;
+  void SetUp() override { TestPlatform::Clear(); }
+};
+
+TEST_F(PlatformTest, GetPlatformForArchitecturesHost) {
+  const PlatformSP host_platform_sp = std::make_shared();
+  Platform::SetHostPlatform(host_platform_sp);
+  ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp);
+
+  const std::vector archs = {ArchSpec("arm64-apple-ps4"),
+   ArchSpec("arm64e-apple-ps4")};
+  std::vector candidates;
+
+  // The host platform matches all architectures.
+  PlatformSP platform_sp =
+  Platform::GetPlatformForArchitectures(archs, {}, {}, candidates);
+  ASSERT_TRUE(platform_sp);
+  EXPECT_EQ(platform_sp, host_platform_sp);
+}
+
+TEST_F(PlatformTest, GetPlatformForArchitecturesSelected) {
+  const PlatformSP host_platform_sp = std::make_shared();
+  Platform::SetHostPlatform(host_platform_sp);
+  ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp);
+
+  const std::vector archs = {ArchSpec("arm64-apple-ps4"),
+   ArchSpec("arm64e-apple-ps4")};
+  std::vector candidates;
+
+  // The host platform matches no architectures. No selected platform.
+  PlatformSP platform_sp =
+  Platform::GetPlatformForArchitectures(archs, {}, {}, candidates);
+  ASSERT_FALSE(platform_sp);
+
+  // The selected platform matches all architectures.
+  const PlatformSP selected_platform_sp = std::make_shared();
+  platform_sp = Platform::GetPlatformForArchitectures(
+  archs, {}, selected_platform_sp, candidates);
+  ASSERT_TRUE(platform_sp);
+  EXPECT_EQ(platform_sp, selected_platform_sp);
+}
+
+TEST_F(PlatformTest, GetPlatformForArchitecturesSelectedOverHost) {
+  const PlatformSP host_platform_sp = std::make_shared();
+  Platform::SetHostPlatform(host_platform_sp);
+  ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp);
+
+  const std::vector archs = {ArchSpec("arm64-apple-ps4"),
+   ArchSpec("x86_6

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

2022-03-29 Thread Greg Clayton via Phabricator via lldb-commits

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

Looks good. One nit that you can choose to fix if desired




Comment at: lldb/source/Target/Platform.cpp:1229-1230
+
+  if (archs.empty())
+return nullptr;
+

Move this above line 1227?


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

https://reviews.llvm.org/D122684

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

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Lldb-commits] [PATCH] D122660: [lldb] Avoid duplicate vdso modules when opening core files

[Lldb-commits] [lldb] 13a3b0b - [lldb] Remove usages of case-insensitive c-string functions

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Lldb-commits] [PATCH] D122603: [wip][intelpt] Refactor timestamps out of `IntelPTInstruction`

[Lldb-commits] [PATCH] D122603: [wip][intelpt] Refactor timestamps out of `IntelPTInstruction`

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Lldb-commits] [PATCH] D122680: Add a setting to force overwriting commands in "command script add"

[Lldb-commits] [lldb] dfde354 - NFC. Fixing warnings from adding DXContainer

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

[Lldb-commits] [PATCH] D121844: Applying clang-tidy modernize-use-equals-default over LLDB

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

[Lldb-commits] [lldb] 8991ad4 - [lldb] Make ModuleSpecList iterable (NFC)

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

26 matches

Site Navigation

Mail list logo

Footer information