date:20220805

[Lldb-commits] [PATCH] D126359: [LLDB] Add basic floating point ops to IR interpreter

2022-08-05 Thread Pavel Kosov via Phabricator via lldb-commits

kpdev42 updated this revision to Diff 450244.
kpdev42 added a comment.

Update test case so it compares JIT’ed  and interpreted FP division results and 
also check operations on float type. Patch doesn’t implement long double, 
because IR interpreter currently doesn’t support instruction argument size of 
more than 64 bit which includes both fp128 and int128. This deserves a separate 
patch, I think


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126359

Files:
  lldb/source/Expression/IRInterpreter.cpp
  lldb/test/API/lang/c/fpeval/Makefile
  lldb/test/API/lang/c/fpeval/TestFPEval.py
  lldb/test/API/lang/c/fpeval/main.c

Index: lldb/test/API/lang/c/fpeval/main.c
===
--- /dev/null
+++ lldb/test/API/lang/c/fpeval/main.c
@@ -0,0 +1,16 @@
+double eval(double a, double b, int op) {
+  switch (op) {
+  case 0: return a+b;
+  case 1: return a-b;
+  case 2: return a/b;
+  case 3: return a*b;
+  default: return 0;
+  }
+}
+
+int main (int argc, char const *argv[])
+{
+double a = 42.0, b = 2.0;
+float f = 42.0, q = 2.0;
+return 0;  Set break point at this line.
+}
Index: lldb/test/API/lang/c/fpeval/TestFPEval.py
===
--- /dev/null
+++ lldb/test/API/lang/c/fpeval/TestFPEval.py
@@ -0,0 +1,101 @@
+"""Tests IR interpreter handling of basic floating point operations (fadd, fsub, etc)."""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class FPEvalTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+self.jit_opts = lldb.SBExpressionOptions()
+self.jit_opts.SetAllowJIT(True)
+self.no_jit_opts = lldb.SBExpressionOptions()
+self.no_jit_opts.SetAllowJIT(False)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set break point at this line.')
+
+def test(self):
+"""Test floating point expressions while jitter is disabled."""
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Break inside the main.
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+
+value = self.frame().EvaluateExpression("a + b", self.no_jit_opts)
+
+self.runCmd("run", RUN_SUCCEEDED)
+# test double
+self.expect("expr --allow-jit false  -- a + b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['double', '44'])
+self.expect("expr --allow-jit false  -- a - b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['double', '40'])
+self.expect("expr --allow-jit false  -- a / b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['double', '21'])
+self.expect("expr --allow-jit false  -- a * b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['double', '84'])
+self.expect("expr --allow-jit false  -- a + 2", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['double', '44'])
+self.expect("expr --allow-jit false  -- a > b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['true'])
+self.expect("expr --allow-jit false  -- a >= b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['true'])
+self.expect("expr --allow-jit false  -- a < b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['false'])
+self.expect("expr --allow-jit false  -- a <= b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['false'])
+self.expect("expr --allow-jit false  -- a == b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['false'])
+self.expect("expr --allow-jit false  -- a != b", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['true'])
+
+# test single
+self.expect("expr --allow-jit false  -- f + q", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['float', '44'])
+self.expect("expr --allow-jit false  -- f - q", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['float', '40'])
+self.expect("expr --allow-jit false  -- f / q", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['float', '21'])
+self.expect("expr --allow-jit false  -- f * q", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['float', '84'])
+self.expect("expr --allow-jit false  -- f + 2", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['float', '44'])
+self.expect("expr --allow-jit false  -- f > q", VARIABLES_DISPLAYED_CORRECTLY,
+substrs=['true'])
+self.expect("e

[Lldb-commits] [PATCH] D131159: [WIP][lldb] Use WSAEventSelect for MainLoop polling

2022-08-05 Thread Michał Górny via Phabricator via lldb-commits

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

Only guessing what various WSA functions do, it seems to all make sense to me. 
Just don't forget to `clang-format` ;-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

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

Looks like a typo from the past code changes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131244

Files:
  lldb/source/Plugins/Process/Utility/ARMUtils.h


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+// assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+// assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

fixathon added a comment.

Added in-code comments with details




Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:48-49
   }
   shift_t = SRType_Invalid;
   return UINT32_MAX;
 }

These lines were unreachable prior to the fix due to the **default** label with 
no break skipping to case 0.  Past code history suggests that was accidental.



Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:314-315
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+

Same here. Past code history suggests the break was missing by accident, i.e 
the **default** case was added to satisfy a static code checker without the 
intent to modify the execution flow. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131244

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

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

2022-08-05 Thread Michał Górny via Phabricator via lldb-commits

mgorny added a comment.

BTW do you need me to implement the POSIX counterpart to this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131160

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

[Lldb-commits] [PATCH] D113155: [lldb] Remove nested switches from ARMGetSupportedArchitectureAtIndex (NFC)

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

fixathon added a comment.
Herald added a project: All.

Added some follow up comments




Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:561
+  default:
+LLVM_FALLTHROUGH;
+  case ArchSpec::eCore_arm_arm64: {

This will default to the ArchSpec::eCore_arm_arm64 case if no matching ArchSpec 
is specified. Double-checking if this by intent.



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:640
+  }
+  return {};
+}

Could we delete this unreachable return statement?  It's tripping up static 
code checkers since all possible branches already have a return.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113155

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

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.

Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:29
+// assert(0 && "Invalid shift type");
+break;
   case 0:

Looking at the codepaths and the reason this function exists, it's either 
called with a 2 bit number which is the "type" field of an instruction, or it's 
not called because the type is known to be SRType_RRX already.

In the enum ARM_ShifterType SRType_RRX would be 4, which wouldn't fit the 2 bit 
expectation.

So I think we can safely re-enable this assert and move the two unreachable 
lines up here. I didn't get any test failures doing so and I don't see a 
codepath that could hit it.

(but it would be fairly easy for future changes to make this mistake since it's 
not very obvious from the types, so an assert is good here)

Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:314-315
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+

fixathon wrote:
> Same here. Past code history suggests the break was missing by accident, i.e 
> the **default** case was added to satisfy a static code checker without the 
> intent to modify the execution flow. 
This part LGTM. This "bits" function is returning uint32 so the compiler can't 
know it'll only be in a 2 bit range.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131244

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

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

@mib I like that idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

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

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Jim Ingham via Phabricator via lldb-commits

jingham added a comment.

In D131138#3698508 , @mib wrote:

> This is awesome 🤩 ! I was also thinking of changing the way enums are exposed 
> to python: instead of having everything added to the `lldb` python module, we 
> could create a class per enum and have static attributes for each enum value 
> so we could do something like `lldb.StopReason.Breakpoint`. That static 
> variable could be a pair with the value/string representation or maybe we 
> could use the `__str__` method to make it very pythonic. Just throwing some 
> ideas here for later, but this LGTM 😊 !

I'm not sure you could do this w/o breaking binary compatibility, since we pass 
these enums to a bunch of the SB API's.  But if you can make that work, this 
would be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

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

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added a comment.

In D131138#3702608 , @jingham wrote:

> In D131138#3698508 , @mib wrote:
>
>> This is awesome 🤩 ! I was also thinking of changing the way enums are 
>> exposed to python: instead of having everything added to the `lldb` python 
>> module, we could create a class per enum and have static attributes for each 
>> enum value so we could do something like `lldb.StopReason.Breakpoint`. That 
>> static variable could be a pair with the value/string representation or 
>> maybe we could use the `__str__` method to make it very pythonic. Just 
>> throwing some ideas here for later, but this LGTM 😊 !
>
> I'm not sure you could do this w/o breaking binary compatibility, since we 
> pass these enums to a bunch of the SB API's.  But if you can make that work, 
> this would be nice.

I don't think that's possible, at least not without having both, which then 
defeats the purpose. I suggested to Ismail that we should start a document to 
keep track of all the things we want to improve for a V2 of the SB API. These 
kind of ideas come up every once in a while and it would be good to have them 
tracked somewhere so they don't get lost with time if/when we ever decide to 
redo the stable API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

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

[Lldb-commits] [lldb] 0948f1c - Reapply the commits to enable accurate hit-count detection for watchpoints.

2022-08-05 Thread Jim Ingham via lldb-commits


Author: Jim Ingham
Date: 2022-08-05T11:01:27-07:00
New Revision: 0948f1cf8177e378bdea2239b8c3ffb9db31f9ad

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

LOG: Reapply the commits to enable accurate hit-count detection for watchpoints.

This commit combines the initial commit (7c240de609af), a fix for x86_64 Linux
(3a0581501e76) and a fix for thinko in a last minute rewrite that I really
should have run the testsuite on.

Also, make sure that all the "I need to step over watchpoint" plans execute
before we call a public stop.  Otherwise, e.g. if you have N watchpoints and
a Signal, the signal stop info will get us to stop with the watchpoints in a
half-done state.

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

Added: 


Modified: 
lldb/include/lldb/Breakpoint/Watchpoint.h
lldb/include/lldb/Target/StopInfo.h
lldb/include/lldb/Target/Thread.h
lldb/include/lldb/Target/ThreadPlan.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Target/StopInfo.cpp
lldb/source/Target/Thread.cpp
lldb/source/Target/ThreadList.cpp

lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py

Removed: 




diff  --git a/lldb/include/lldb/Breakpoint/Watchpoint.h 
b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 41b723a66b6a3..a5a72e3ad5a1b 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -75,7 +75,7 @@ class Watchpoint : public 
std::enable_shared_from_this,
   bool IsHardware() const override;
 
   bool ShouldStop(StoppointCallbackContext *context) override;
-
+  
   bool WatchpointRead() const;
   bool WatchpointWrite() const;
   uint32_t GetIgnoreCount() const;
@@ -157,12 +157,15 @@ class Watchpoint : public 
std::enable_shared_from_this,
 private:
   friend class Target;
   friend class WatchpointList;
+  friend class StopInfoWatchpoint; // This needs to call UndoHitCount()
 
   void ResetHistoricValues() {
 m_old_value_sp.reset();
 m_new_value_sp.reset();
   }
 
+  void UndoHitCount() { m_hit_counter.Decrement(); }
+
   Target &m_target;
   bool m_enabled;   // Is this watchpoint enabled
   bool m_is_hardware;   // Is this a hardware watchpoint

diff  --git a/lldb/include/lldb/Target/StopInfo.h 
b/lldb/include/lldb/Target/StopInfo.h
index cdb906dcd7ede..9527a6ea553e3 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -17,7 +17,7 @@
 
 namespace lldb_private {
 
-class StopInfo {
+class StopInfo : public std::enable_shared_from_this {
   friend class Process::ProcessEventData;
   friend class ThreadPlanBase;
 

diff  --git a/lldb/include/lldb/Target/Thread.h 
b/lldb/include/lldb/Target/Thread.h
index f5f024434c8ee..efe82721a04e3 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -240,6 +240,7 @@ class Thread : public std::enable_shared_from_this,
   // this just calls through to the ThreadSpec's ThreadPassesBasicTests method.
   virtual bool MatchesSpec(const ThreadSpec *spec);
 
+  // Get the current public stop info, calculating it if necessary.
   lldb::StopInfoSP GetStopInfo();
 
   lldb::StopReason GetStopReason();
@@ -1121,7 +1122,7 @@ class Thread : public 
std::enable_shared_from_this,
   // "checkpointed and restored" stop info, so if it is still around it is
   // right even if you have not calculated this yourself, or if it disagrees
   // with what you might have calculated.
-  virtual lldb::StopInfoSP GetPrivateStopInfo();
+  virtual lldb::StopInfoSP GetPrivateStopInfo(bool calculate = true);
 
   // Calculate the stop info that will be shown to lldb clients.  For instance,
   // a "step out"

[Lldb-commits] [PATCH] D130674: Accurate watchpoint hit counts redux

2022-08-05 Thread Jim Ingham via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0948f1cf8177: Reapply the commits to enable accurate 
hit-count detection for watchpoints. (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130674

Files:
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Target/StopInfo.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Target/StopInfo.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Target/ThreadList.cpp
  
lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
  
lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py

Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
===
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py
@@ -12,10 +12,6 @@
 # Atomic sequences are not supported yet for MIPS in LLDB.
 @skipIf(triple='^mips')
 @add_test_categories(["watchpoint"])
-@skipIf(
-oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-bugnumber="rdar://93863107")
 
 def test(self):
 """Test watchpoint and a breakpoint in multiple threads."""
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
===
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
@@ -12,10 +12,6 @@
 # Atomic sequences are not supported yet for MIPS in LLDB.
 @skipIf(triple='^mips')
 @expectedFailureNetBSD
-@skipIf(
-oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-bugnumber="rdar://93863107")
 @add_test_categories(["watchpoint"])
 def test(self):
 """Test two threads that trigger a watchpoint and one signal thread. """
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
===
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
@@ -11,10 +11,6 @@
 
 # Atomic sequences are not supported yet for MIPS in LLDB.
 @skipIf(triple='^mips')
-@skipIf(
-oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-bugnumber="rdar://93863107")
 @add_test_categories(["watchpoint"])
 def test(self):
 """Test two threads that trigger a watchpoint and one (1 second delay) breakpoint thread. """
Index: lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
===
--- lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
+++ lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
@@ -11,10 +11,6 @@
 
 # Atomic sequences are not supported yet for MIPS in LLDB.
 @skipIf(triple='^mips')
-@skipIf(
-oslist=["ios", "watchos", "tvos", "bridgeos", "macosx"],
-archs=['arm64', 'arm64e', 'arm64_32', 'arm'],
-bugnumber="rdar://93863107")
 @add_test_categories(["watchpoint"])
 def test(self):
 """Test two threads that tr

[Lldb-commits] [PATCH] D131130: [lldb] Improve EXC_RESOURCE exception reason

2022-08-05 Thread Jim Ingham via Phabricator via lldb-commits

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

LGTM


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

https://reviews.llvm.org/D131130

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

[Lldb-commits] [PATCH] D131275: [lldb] Make Process and subclass constructors protected

2022-08-05 Thread Michał Górny via Phabricator via lldb-commits

mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste, jingham.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.

Make constructors of the Process and its subclasses class protected,
to prevent accidentally constructing Process on stack when it could be
afterwards accessed via a shared_ptr (since it uses
std::enable_shared_from_this<>).

The only place where a stack allocation was used were unittests,
and fixing them via declaring an explicit public constructor
in the respective mock classes is trivial.

Sponsored by: The FreeBSD Foundation


https://reviews.llvm.org/D131275

Files:
  lldb/include/lldb/Target/PostMortemProcess.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  lldb/unittests/Expression/DWARFExpressionTest.cpp
  lldb/unittests/Process/ProcessEventDataTest.cpp
  lldb/unittests/Target/ExecutionContextTest.cpp
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- lldb/unittests/Thread/ThreadTest.cpp
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -40,7 +40,8 @@
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
 return true;
Index: lldb/unittests/Target/ExecutionContextTest.cpp
===
--- lldb/unittests/Target/ExecutionContextTest.cpp
+++ lldb/unittests/Target/ExecutionContextTest.cpp
@@ -47,7 +47,8 @@
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
 return true;
Index: lldb/unittests/Process/ProcessEventDataTest.cpp
===
--- lldb/unittests/Process/ProcessEventDataTest.cpp
+++ lldb/unittests/Process/ProcessEventDataTest.cpp
@@ -42,7 +42,8 @@
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
 return true;
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -328,7 +328,9 @@
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit0, DW_OP_deref}), llvm::Failed());
 
   struct MockProcess : Process {
-using Process::Process;
+MockProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+: Process(target_sp, listener_sp) {}
+
 llvm::StringRef GetPluginName() override { return "mock process"; }
 bool CanDebug(lldb::TargetSP target,
   bool plugin_specified_by_name) override {
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -50,10 +50,12 @@
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
+protected:
   ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
   const ScriptedProcess::ScriptedProcessInfo &launch_info,
   Status &error);
 
+public:
   ~ScriptedProcess() override;
 
   bool CanDebug(lldb::TargetSP target_sp,
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -62,8 +62,8 @@
   ScriptedProcess::ScriptedProcessInfo scripted_process_info(
   target_sp->GetProcessLaunchInfo());
 
-  auto process_sp = std::make_shared(
-  target_sp, listener_sp, scripted_process_info, error);
+  auto process_sp = std::shared_ptr(new ScriptedProcess(
+  target_sp, listener_sp, scripted_process_info, error));
 
   if (error.Fail() || !process_sp || !process_sp->m_script_object_sp ||
   !process_sp->m_script_object_sp->IsValid()) {
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
==

[Lldb-commits] [PATCH] D113155: [lldb] Remove nested switches from ARMGetSupportedArchitectureAtIndex (NFC)

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

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



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:561
+  default:
+LLVM_FALLTHROUGH;
+  case ArchSpec::eCore_arm_arm64: {

fixathon wrote:
> This will default to the ArchSpec::eCore_arm_arm64 case if no matching 
> ArchSpec is specified. Double-checking if this by intent.
I'm sure it was intentional when I wrote it, but I can't figure out for which 
core this would actually make sense. If the test suite passes with returning an 
empty array here, that works for me. 



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:640
+  }
+  return {};
+}

fixathon wrote:
> Could we delete this unreachable return statement?  It's tripping up static 
> code checkers since all possible branches already have a return.
Yes, let's make it an `llvm_unreachable` (otherwise gcc will complain).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113155

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

[Lldb-commits] [PATCH] D131130: [lldb] Improve EXC_RESOURCE exception reason

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9c81b743e31a: [lldb] Improve EXC_RESOURCE exception reason 
(authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D131130?vs=449835&id=450335#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131130

Files:
  lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp


Index: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
===
--- lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -410,7 +410,8 @@
 
   switch (resource_type) {
   case RESOURCE_TYPE_CPU:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_CPU";
+exc_desc =
+"EXC_RESOURCE (RESOURCE_TYPE_CPU: CPU usage monitor tripped)";
 snprintf(code_desc_buf, sizeof(code_desc_buf), "%d%%",
  (int)EXC_RESOURCE_CPUMONITOR_DECODE_PERCENTAGE(m_exc_code));
 snprintf(subcode_desc_buf, sizeof(subcode_desc_buf), "%d%%",
@@ -418,7 +419,8 @@
  m_exc_subcode));
 break;
   case RESOURCE_TYPE_WAKEUPS:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_WAKEUPS";
+exc_desc = "EXC_RESOURCE (RESOURCE_TYPE_WAKEUPS: idle wakeups monitor "
+   "tripped)";
 snprintf(
 code_desc_buf, sizeof(code_desc_buf), "%d w/s",
 (int)EXC_RESOURCE_CPUMONITOR_DECODE_WAKEUPS_PERMITTED(m_exc_code));
@@ -427,11 +429,12 @@
  m_exc_subcode));
 break;
   case RESOURCE_TYPE_MEMORY:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_MEMORY";
+exc_desc = "EXC_RESOURCE (RESOURCE_TYPE_MEMORY: high watermark memory "
+   "limit exceeded)";
 snprintf(code_desc_buf, sizeof(code_desc_buf), "%d MB",
  (int)EXC_RESOURCE_HWM_DECODE_LIMIT(m_exc_code));
 subcode_desc = nullptr;
-subcode_label = "unused";
+subcode_label = nullptr;
 break;
 #if defined(RESOURCE_TYPE_IO)
   // RESOURCE_TYPE_IO is introduced in macOS SDK 10.12.
@@ -468,9 +471,9 @@
   }
 
   if (m_exc_data_count >= 2) {
-if (subcode_desc)
+if (subcode_label && subcode_desc)
   strm.Printf(", %s=%s", subcode_label, subcode_desc);
-else
+else if (subcode_label)
   strm.Printf(", %s=0x%" PRIx64, subcode_label, m_exc_subcode);
   }
 


Index: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
===
--- lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -410,7 +410,8 @@
 
   switch (resource_type) {
   case RESOURCE_TYPE_CPU:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_CPU";
+exc_desc =
+"EXC_RESOURCE (RESOURCE_TYPE_CPU: CPU usage monitor tripped)";
 snprintf(code_desc_buf, sizeof(code_desc_buf), "%d%%",
  (int)EXC_RESOURCE_CPUMONITOR_DECODE_PERCENTAGE(m_exc_code));
 snprintf(subcode_desc_buf, sizeof(subcode_desc_buf), "%d%%",
@@ -418,7 +419,8 @@
  m_exc_subcode));
 break;
   case RESOURCE_TYPE_WAKEUPS:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_WAKEUPS";
+exc_desc = "EXC_RESOURCE (RESOURCE_TYPE_WAKEUPS: idle wakeups monitor "
+   "tripped)";
 snprintf(
 code_desc_buf, sizeof(code_desc_buf), "%d w/s",
 (int)EXC_RESOURCE_CPUMONITOR_DECODE_WAKEUPS_PERMITTED(m_exc_code));
@@ -427,11 +429,12 @@
  m_exc_subcode));
 break;
   case RESOURCE_TYPE_MEMORY:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_MEMORY";
+exc_desc = "EXC_RESOURCE (RESOURCE_TYPE_MEMORY: high watermark memory "
+   "limit exceeded)";
 snprintf(code_desc_buf, sizeof(code_desc_buf), "%d MB",
  (int)EXC_RESOURCE_HWM_DECODE_LIMIT(m_exc_code));
 subcode_desc = nullptr;
-subcode_label = "unused";
+subcode_label = nullptr;
 break;
 #if defined(RESOURCE_TYPE_IO)
   // RESOURCE_TYPE_IO is introduced in macOS SDK 10.12.
@@ -468,9 +471,9 @@
   }
 
   if (m_exc_data_count >= 2) {
-if (subcode_desc)
+if (subcode_label && subcode_desc)
   strm.Printf(", %s=%s", subcode_label, subcode_desc);
-else
+else if (subcode_label)
   strm.Printf(", %s=0x%" PRIx64, subcode_label, m_exc_subcode);
   }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 9c81b74 - [lldb] Improve EXC_RESOURCE exception reason

2022-08-05 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2022-08-05T11:19:46-07:00
New Revision: 9c81b743e31a7dca288b37b6cf6cca3213bfd923

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

LOG: [lldb] Improve EXC_RESOURCE exception reason

Jason noted that the stop message we print for a memory high water mark
notification (EXC_RESOURCE) could be clearer. Currently, the stop
reason looks like this:

  * thread #3, queue = 'com.apple.CFNetwork.LoaderQ', stop reason =
EXC_RESOURCE RESOURCE_TYPE_MEMORY (limit=14 MB, unused=0x0)

It's hard to read the message because the exception and the type
(EXC_RESOURCE RESOURCE_TYPE_MEMORY) blend together. Additionally, the
"observed=0x0" should not be printed for memory limit exceptions.

I wanted to continue to include the resource type from
 while also explaining what it actually is. I used
the wording from the comments in the header. With this path, the stop
reason now looks like this:

  * thread #5, stop reason = EXC_RESOURCE (RESOURCE_TYPE_MEMORY: high
watermark memory limit exceeded) (limit=14 MB)

rdar://40466897

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

Added: 


Modified: 
lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp 
b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 4df210032153f..4623a50537eb6 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -410,7 +410,8 @@ const char *StopInfoMachException::GetDescription() {
 
   switch (resource_type) {
   case RESOURCE_TYPE_CPU:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_CPU";
+exc_desc =
+"EXC_RESOURCE (RESOURCE_TYPE_CPU: CPU usage monitor tripped)";
 snprintf(code_desc_buf, sizeof(code_desc_buf), "%d%%",
  (int)EXC_RESOURCE_CPUMONITOR_DECODE_PERCENTAGE(m_exc_code));
 snprintf(subcode_desc_buf, sizeof(subcode_desc_buf), "%d%%",
@@ -418,7 +419,8 @@ const char *StopInfoMachException::GetDescription() {
  m_exc_subcode));
 break;
   case RESOURCE_TYPE_WAKEUPS:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_WAKEUPS";
+exc_desc = "EXC_RESOURCE (RESOURCE_TYPE_WAKEUPS: idle wakeups monitor "
+   "tripped)";
 snprintf(
 code_desc_buf, sizeof(code_desc_buf), "%d w/s",
 (int)EXC_RESOURCE_CPUMONITOR_DECODE_WAKEUPS_PERMITTED(m_exc_code));
@@ -427,11 +429,12 @@ const char *StopInfoMachException::GetDescription() {
  m_exc_subcode));
 break;
   case RESOURCE_TYPE_MEMORY:
-exc_desc = "EXC_RESOURCE RESOURCE_TYPE_MEMORY";
+exc_desc = "EXC_RESOURCE (RESOURCE_TYPE_MEMORY: high watermark memory "
+   "limit exceeded)";
 snprintf(code_desc_buf, sizeof(code_desc_buf), "%d MB",
  (int)EXC_RESOURCE_HWM_DECODE_LIMIT(m_exc_code));
 subcode_desc = nullptr;
-subcode_label = "unused";
+subcode_label = nullptr;
 break;
 #if defined(RESOURCE_TYPE_IO)
   // RESOURCE_TYPE_IO is introduced in macOS SDK 10.12.
@@ -468,9 +471,9 @@ const char *StopInfoMachException::GetDescription() {
   }
 
   if (m_exc_data_count >= 2) {
-if (subcode_desc)
+if (subcode_label && subcode_desc)
   strm.Printf(", %s=%s", subcode_label, subcode_desc);
-else
+else if (subcode_label)
   strm.Printf(", %s=0x%" PRIx64, subcode_label, m_exc_subcode);
   }
 



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

[Lldb-commits] [PATCH] D113155: [lldb] Remove nested switches from ARMGetSupportedArchitectureAtIndex (NFC)

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

fixathon added a comment.

Added comments




Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:640
+  }
+  return {};
+}

JDevlieghere wrote:
> fixathon wrote:
> > Could we delete this unreachable return statement?  It's tripping up static 
> > code checkers since all possible branches already have a return.
> Yes, let's make it an `llvm_unreachable` (otherwise gcc will complain).
If we return {} in the **default** case, and delete this,  gcc should not 
complain since all possible code paths will have a return, is that right?
Alternatively, we could break in the **default** case and leave this return 
alone. I prefer the first approach for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113155

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

[Lldb-commits] [PATCH] D113155: [lldb] Remove nested switches from ARMGetSupportedArchitectureAtIndex (NFC)

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

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



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:640
+  }
+  return {};
+}

fixathon wrote:
> JDevlieghere wrote:
> > fixathon wrote:
> > > Could we delete this unreachable return statement?  It's tripping up 
> > > static code checkers since all possible branches already have a return.
> > Yes, let's make it an `llvm_unreachable` (otherwise gcc will complain).
> If we return {} in the **default** case, and delete this,  gcc should not 
> complain since all possible code paths will have a return, is that right?
> Alternatively, we could break in the **default** case and leave this return 
> alone. I prefer the first approach for consistency.
Yes, you're right, it's only a problem for gcc when you explicitly handle every 
case (see all the instances of `llvm_unreachable("Fully covered switch!");` in 
lldb/llvm). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113155

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

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

I wonder how much breakage there would be if you implemented a bunch of the 
dunder methods. Ex:

  class StopReason:
  def __int__(self) -> int: ...
  def __eq__(self, other: StopReason | int) -> bool: ...
  # ...

This would cover many cases, but not all. For example `5 == 
StopReason.whatever` would fail, even when `StopReason.whatever == 5` passes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

2022-08-05 Thread Tobias Hieta via Phabricator via lldb-commits

thieta updated this revision to Diff 450364.
thieta added a comment.

Fixed spelling error and added links to C++ standard libraries


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

Files:
  bolt/runtime/CMakeLists.txt
  clang/CMakeLists.txt
  lld/CMakeLists.txt
  lldb/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/cmake/modules/CheckCompilerVersion.cmake
  llvm/docs/CodingStandards.rst
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -47,6 +47,18 @@
 Update on required toolchains to build LLVM
 ---
 
+LLVM is now built with C++17 by default. This means C++17 can be used in
+the code base.
+
+The previous "soft" toolchain requirements have now been changed to "hard".
+This means that the the following versions are now required to build LLVM
+and there is no way to suppress this error.
+
+* GCC >= 7.1
+* Clang >= 5.0
+* Apple Clang >= 9.3
+* Visual Studio 2019 >= 16.7
+
 Changes to the LLVM IR
 --
 
Index: llvm/docs/CodingStandards.rst
===
--- llvm/docs/CodingStandards.rst
+++ llvm/docs/CodingStandards.rst
@@ -53,7 +53,7 @@
 C++ Standard Versions
 -
 
-Unless otherwise documented, LLVM subprojects are written using standard C++14
+Unless otherwise documented, LLVM subprojects are written using standard C++17
 code and avoid unnecessary vendor-specific extensions.
 
 Nevertheless, we restrict ourselves to features which are available in the
@@ -63,7 +63,13 @@
 Each toolchain provides a good reference for what it accepts:
 
 * Clang: https://clang.llvm.org/cxx_status.html
-* GCC: https://gcc.gnu.org/projects/cxx-status.html#cxx14
+
+  * libc++: https://libcxx.llvm.org/Status/Cxx17.html
+
+* GCC: https://gcc.gnu.org/projects/cxx-status.html#cxx17
+
+  * libstdc++: https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017
+
 * MSVC: https://msdn.microsoft.com/en-us/library/hh567368.aspx
 
 
Index: llvm/cmake/modules/CheckCompilerVersion.cmake
===
--- llvm/cmake/modules/CheckCompilerVersion.cmake
+++ llvm/cmake/modules/CheckCompilerVersion.cmake
@@ -4,21 +4,19 @@
 
 include(CheckCXXSourceCompiles)
 
-set(GCC_MIN 5.1)
+set(GCC_MIN 7.1)
 set(GCC_SOFT_ERROR 7.1)
-set(CLANG_MIN 3.5)
+set(CLANG_MIN 5.0)
 set(CLANG_SOFT_ERROR 5.0)
-set(APPLECLANG_MIN 6.0)
+set(APPLECLANG_MIN 9.3)
 set(APPLECLANG_SOFT_ERROR 9.3)
 
 # https://en.wikipedia.org/wiki/Microsoft_Visual_C#Internal_version_numbering
-# _MSC_VER == 1920 MSVC++ 14.20 Visual Studio 2019 Version 16.0
 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7
-set(MSVC_MIN 19.20)
+set(MSVC_MIN 19.27)
 set(MSVC_SOFT_ERROR 19.27)
 
-# Map the above GCC versions to dates: https://gcc.gnu.org/develop.html#timeline
-set(GCC_MIN_DATE 20150422)
+set(LIBSTDCXX_MIN 7)
 set(LIBSTDCXX_SOFT_ERROR 7)
 
 
@@ -76,22 +74,14 @@
 set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
 set(OLD_CMAKE_REQUIRED_LIBRARIES ${CMAKE_REQUIRED_LIBRARIES})
 set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++0x")
-# Test for libstdc++ version of at least 4.8 by checking for _ZNKSt17bad_function_call4whatEv.
-# Note: We should check _GLIBCXX_RELEASE when possible (i.e., for GCC 7.1 and up).
 check_cxx_source_compiles("
 #include 
 #if defined(__GLIBCXX__)
-#if __GLIBCXX__ < ${GCC_MIN_DATE}
+#if !defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE < ${LIBSTDCXX_MIN}
 #error Unsupported libstdc++ version
 #endif
 #endif
-#if defined(__GLIBCXX__)
-extern const char _ZNKSt17bad_function_call4whatEv[];
-const char *chk = _ZNKSt17bad_function_call4whatEv;
-#else
-const char *chk = \"\";
-#endif
-int main() { ++chk; return 0; }
+int main() { return 0; }
 "
   LLVM_LIBSTDCXX_MIN)
 if(NOT LLVM_LIBSTDCXX_MIN)
Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -58,7 +58,7 @@
 # Must go after project(..)
 include(GNUInstallDirs)
 
-set(CMAKE_CXX_STANDARD 14 CACHE STRING "C++ standard to conform to")
+set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard to conform to")
 set(CMAKE_CXX_STANDARD_REQUIRED YES)
 if (CYGWIN)
   # Cygwin is a bit stricter and lack things like 'strdup', 'stricmp', etc in
Index: lldb/CMakeLists.txt
===
--- lldb/CMakeLists.txt
+++ lldb/CMakeLists.txt
@@ -20,7 +20,7 @@
 if(LLDB_BUILT_STANDALONE)
   include(LLDBStandalone)
 
-  set(CMAKE_CXX_STANDARD 14 CACHE STRING "C++ standard to conform to")
+  set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard to conform to")
   set(CMAKE_CXX_STANDARD_REQUIRED YES)
   set(CMAKE_CXX_EXTENSIONS NO)

[Lldb-commits] [PATCH] D131294: [LLDB][NFC] Reliability fixes to TCPSocket code

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

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

Patch the following issues found by static code inspection:

- Unchecked return values from lib calls
- Passing potentially negative arg into a function that requires non-negative 
input
- Possible socket double-close


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131294

Files:
  lldb/source/Host/common/TCPSocket.cpp


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -174,7 +174,10 @@
   continue;
 }
 
-SetOptionNoDelay();
+if (-1 == SetOptionNoDelay()) {
+  Close();
+  continue;
+}
 
 error.Clear();
 return error;
@@ -200,15 +203,18 @@
   for (SocketAddress &address : addresses) {
 int fd = Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP,
   m_child_processes_inherit, error);
-if (error.Fail())
+if (error.Fail() || fd < 0)
   continue;
 
 // enable local address reuse
 int option_value = 1;
 set_socket_option_arg_type option_value_p =
 reinterpret_cast(&option_value);
-::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
- sizeof(option_value));
+if (-1 == ::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
+   sizeof(option_value))) {
+  CLOSE_SOCKET(fd);
+  continue;
+}
 
 SocketAddress listen_address = address;
 if(!listen_address.IsLocalhost())
@@ -255,8 +261,8 @@
 return error;
   }
 
-  int sock = -1;
-  int listen_sock = -1;
+  int sock = kInvalidSocketValue;
+  int listen_sock = kInvalidSocketValue;
   lldb_private::SocketAddress AcceptAddr;
   MainLoop accept_loop;
   std::vector handles;
@@ -288,7 +294,10 @@
 
 lldb_private::SocketAddress &AddrIn = m_listen_sockets[listen_sock];
 if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
-  CLOSE_SOCKET(sock);
+  if (kInvalidSocketValue != sock) {
+CLOSE_SOCKET(sock);
+sock = kInvalidSocketValue;
+  }
   llvm::errs() << llvm::formatv(
   "error: rejecting incoming connection from {0} (expecting {1})",
   AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -174,7 +174,10 @@
   continue;
 }
 
-SetOptionNoDelay();
+if (-1 == SetOptionNoDelay()) {
+  Close();
+  continue;
+}
 
 error.Clear();
 return error;
@@ -200,15 +203,18 @@
   for (SocketAddress &address : addresses) {
 int fd = Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP,
   m_child_processes_inherit, error);
-if (error.Fail())
+if (error.Fail() || fd < 0)
   continue;
 
 // enable local address reuse
 int option_value = 1;
 set_socket_option_arg_type option_value_p =
 reinterpret_cast(&option_value);
-::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
- sizeof(option_value));
+if (-1 == ::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
+   sizeof(option_value))) {
+  CLOSE_SOCKET(fd);
+  continue;
+}
 
 SocketAddress listen_address = address;
 if(!listen_address.IsLocalhost())
@@ -255,8 +261,8 @@
 return error;
   }
 
-  int sock = -1;
-  int listen_sock = -1;
+  int sock = kInvalidSocketValue;
+  int listen_sock = kInvalidSocketValue;
   lldb_private::SocketAddress AcceptAddr;
   MainLoop accept_loop;
   std::vector handles;
@@ -288,7 +294,10 @@
 
 lldb_private::SocketAddress &AddrIn = m_listen_sockets[listen_sock];
 if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
-  CLOSE_SOCKET(sock);
+  if (kInvalidSocketValue != sock) {
+CLOSE_SOCKET(sock);
+sock = kInvalidSocketValue;
+  }
   llvm::errs() << llvm::formatv(
   "error: rejecting incoming connection from {0} (expecting {1})",
   AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130942: [LLDB][DWARF] Set MSInheritanceAttr for record decl when it's using Microsoft compatible ABI.

2022-08-05 Thread Reid Kleckner via Phabricator via lldb-commits

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754
+  m_ast.getASTContext(),
+  
clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance));
+}

mstorsjo wrote:
> rnk wrote:
> > I'm concerned that this isn't really the right fix. Changing the 
> > inheritance model changes the size of the member pointer representation, so 
> > the consequences of getting the wrong one could affect expression 
> > evaluation results. Zequan suggested guessing the inheritance model from 
> > the class definition, but we really ought to write down the inheritance 
> > model in the DWARF when using the MS ABI. This probably needs its own DWARF 
> > attribute.
> FWIW, there’s two use cases at play here:
> 
> 1. The executable actually is using the itanium ABI, but is being 
> (mis)interpreted with the MSVC ABI. When this happens, we currently crash, 
> which is a terrible user experience. In this case, there’s probably no good 
> solution (we can’t really get it right at this point) - but pretty much 
> anything is better than crashing. Before D127048, we always interpreted 
> everything with the MSVC C++ ABI; now we probably are getting it right in a 
> majority of cases at least.
> 
> 2. People intentionally using dwarf debug into with MSVC ABI. Not very 
> common, but something that some people do use, as far as I know. Here we at 
> least have a chance of getting right.
I see, I thought we already knew we were in use case 2 not 1. Having this as a 
workaround to not crash seems fine, but we really ought to warn when LLDB 
encounters these conditions:
1. MS C++ ABI is in use
2. DWARF is in use
3. A C++ source file is in use

Using DWARF for C code in the MS ABI model is fine, no need to warn in that 
case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

[Lldb-commits] [PATCH] D130796: [LLDB][NativePDB] Switch to use DWARFLocationList.

2022-08-05 Thread Zequan Wu via Phabricator via lldb-commits

zequanwu updated this revision to Diff 450408.
zequanwu added a comment.

- Recurse into parent classes when filling the offset to size map.
- Previously the range parsed earlier is preferred, now it's the range parsed 
later is preferred, since it makes the code cleaner and it doesn't really 
matter as long as it's consistent. Although overlapping ranges are allowed in 
RangeDataVector, I'm still trying to keep the RangeMap contains non-overlapping 
ranges, since only whole value ranges are allowed to overlap with each other, 
and subfield ranges should not overlap with each other or whole value ranges. 
It's easier just to keep all ranges non-overlapping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130796

Files:
  lldb/include/lldb/Utility/RangeMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/test/Shell/SymbolFile/NativePDB/Inputs/local-variables-registers.lldbinit
  lldb/test/Shell/SymbolFile/NativePDB/inline_sites.test
  lldb/test/Shell/SymbolFile/NativePDB/local-variables-registers.s
  lldb/test/Shell/SymbolFile/NativePDB/subfield_register_simple_type.s

Index: lldb/test/Shell/SymbolFile/NativePDB/subfield_register_simple_type.s
===
--- lldb/test/Shell/SymbolFile/NativePDB/subfield_register_simple_type.s
+++ /dev/null
@@ -1,433 +0,0 @@
-# clang-format off
-# REQUIRES: lld, x86
-
-# RUN: %clang_cl --target=i386-windows-msvc -c /Fo%t.obj -- %s
-# RUN: lld-link /debug:full /nodefaultlib /entry:main %t.obj /out:%t.exe /base:0x40
-# RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
-# RUN: %p/Inputs/subfield_register_simple_type.lldbinit 2>&1 | FileCheck %s
-
-# This file is compiled from following source file:
-# clang-cl --target=i386-windows-msvc /Z7 /O1 -c /Fa a.cpp
-# __int64 __attribute__((optnone)) bar(__int64 x) { return x; };
-# __int64 foo(__int64 x) {
-#   return bar(x);
-# }
-#
-# int main(int argc, char** argv) {
-#   foo(argc);
-#   return 0;
-# }
-
-# FIXME: The following variable location have wrong register numbers due to
-# https://github.com/llvm/llvm-project/issues/53575. Fix them after resolving
-# the issue.
-
-# CHECK:  (lldb) image lookup -a 0x40102f -v
-# CHECK:  LineEntry: [0x00401026-0x00401039): C:\src\a.cpp:3
-# CHECK-NEXT: Variable: id = {{.*}}, name = "x", type = "int64_t", valid ranges = [0x0040102f-0x00401036), location = DW_OP_reg0 EAX, DW_OP_piece 0x4, DW_OP_reg2 EDX, DW_OP_piece 0x4, decl =
-
-	.text
-	.def	@feat.00;
-	.scl	3;
-	.type	0;
-	.endef
-	.globl	@feat.00
-.set @feat.00, 1
-	.intel_syntax noprefix
-	.file	"a.cpp"
-	.def	"?bar@@YA_J_J@Z";
-	.scl	2;
-	.type	32;
-	.endef
-	.section	.text,"xr",one_only,"?bar@@YA_J_J@Z"
-	.globl	"?bar@@YA_J_J@Z"# -- Begin function ?bar@@YA_J_J@Z
-	.p2align	4, 0x90
-"?bar@@YA_J_J@Z":   # @"?bar@@YA_J_J@Z"
-Lfunc_begin0:
-	.cv_func_id 0
-	.cv_file	1 "C:\\src\\a.cpp" "CB99424BC3DD1AB059A2DBC6841147F2" 1
-	.cv_loc	0 1 1 0 # a.cpp:1:0
-	.cv_fpo_proc	"?bar@@YA_J_J@Z" 8
-# %bb.0:# %entry
-	push	ebp
-	.cv_fpo_pushreg	ebp
-	mov	ebp, esp
-	.cv_fpo_setframe	ebp
-	and	esp, -8
-	.cv_fpo_stackalign	8
-	sub	esp, 8
-	.cv_fpo_stackalloc	8
-	.cv_fpo_endprologue
-	mov	eax, dword ptr [ebp + 8]
-	mov	ecx, dword ptr [ebp + 12]
-	mov	dword ptr [esp], eax
-	mov	dword ptr [esp + 4], ecx
-Ltmp0:
-	mov	eax, dword ptr [esp]
-	mov	edx, dword ptr [esp + 4]
-	mov	esp, ebp
-	pop	ebp
-	ret
-Ltmp1:
-	.cv_fpo_endproc
-Lfunc_end0:
-# -- End function
-	.def	"?foo@@YA_J_J@Z";
-	.scl	2;
-	.type	32;
-	.endef
-	.section	.text,"xr",one_only,"?foo@@YA_J_J@Z"
-	.globl	"?foo@@YA_J_J@Z"# -- Begin function ?foo@@YA_J_J@Z
-"?foo@@YA_J_J@Z":   # @"?foo@@YA_J_J@Z"
-Lfunc_begin1:
-	.cv_func_id 1
-	.cv_fpo_proc	"?foo@@YA_J_J@Z" 8
-# %bb.0:# %entry
-	#DEBUG_VALUE: foo:x <- [DW_OP_plus_uconst 4] [$esp+0]
-	.cv_loc	1 1 3 0 # a.cpp:3:0
-	jmp	"?bar@@YA_J_J@Z"# TAILCALL
-Ltmp2:
-	.cv_fpo_endproc
-Lfunc_end1:
-# -- End function
-	.def	_main;
-	.scl	2;
-	.type	32;
-	.endef
-	.section	.text,"xr",one_only,_main
-	.globl	_main   # -- Begin function main
-_main:  # @main
-Lfunc_begin2:
-	.cv_func_id 2
-	.cv_loc	2 1 6 0 # a.cpp:6:0
-	.cv_fpo_proc	_main 8
-# %bb.0:# %entry
-	#DEBUG_VALUE: main:argv <- [DW_OP_plus_uconst 8] [$esp+0]
-	#DEBUG_VALUE: main:argc <- [DW

[Lldb-commits] [PATCH] D131303: [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere created this revision.
JDevlieghere added a reviewer: jasonmolenda.
Herald added a project: All.
JDevlieghere requested review of this revision.

- Reduce indentation
- Extract caching of the DbgShellCommand and the dsymForUUID executable (or 
equivalent)
- Check the DBGShellCommands before falling back to /usr/local/bin/dsymForUUID
- Don't check `~rc/bin/dsymForUUID`
- Improve error reporting


https://reviews.llvm.org/D131303

Files:
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -491,190 +491,180 @@
   return success;
 }
 
-bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
-  Status &error, bool force_lookup) {
-  bool success = false;
-  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
-  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
-
-  // It's expensive to check for the DBGShellCommands defaults setting, only do
-  // it once per lldb run and cache the result.
-  static bool g_have_checked_for_dbgshell_command = false;
-  static const char *g_dbgshell_command = NULL;
-  if (!g_have_checked_for_dbgshell_command) {
-g_have_checked_for_dbgshell_command = true;
+// It's expensive to check for the DBGShellCommands defaults setting. Only do
+// it once per lldb run and cache the result.
+static llvm::StringRef GetDbgShellCommand() {
+  static std::once_flag g_once_flag;
+  static std::string g_dbgshell_command;
+  std::call_once(g_once_flag, [&]() {
 CFTypeRef defaults_setting = CFPreferencesCopyAppValue(
 CFSTR("DBGShellCommands"), CFSTR("com.apple.DebugSymbols"));
 if (defaults_setting &&
 CFGetTypeID(defaults_setting) == CFStringGetTypeID()) {
-  char cstr_buf[PATH_MAX];
-  if (CFStringGetCString((CFStringRef)defaults_setting, cstr_buf,
- sizeof(cstr_buf), kCFStringEncodingUTF8)) {
-g_dbgshell_command =
-strdup(cstr_buf); // this malloc'ed memory will never be freed
+  char buffer[PATH_MAX];
+  if (CFStringGetCString((CFStringRef)defaults_setting, buffer,
+ sizeof(buffer), kCFStringEncodingUTF8)) {
+g_dbgshell_command = buffer;
   }
 }
 if (defaults_setting) {
   CFRelease(defaults_setting);
 }
-  }
+  });
+  return g_dbgshell_command;
+}
+
+static FileSpec GetDsymForUUIDExecutable() {
+  static std::once_flag g_once_flag;
+  static FileSpec g_dsymForUUID_executable;
+  std::call_once(g_once_flag, [&]() {
+// Try LLDB_APPLE_DSYMFORUUID_EXECUTABLE
+if (const char *dsymForUUID_env =
+getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
+  g_dsymForUUID_executable = FileSpec(dsymForUUID_env);
+  FileSystem::Instance().Resolve(g_dsymForUUID_executable);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// Try the DBGShellCommands
+llvm::StringRef dbgshell_command = GetDbgShellCommand();
+if (!dbgshell_command.empty()) {
+  g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// Try dsymForUUID in /usr/local/bin
+{
+  g_dsymForUUID_executable =
+  FileSpec("/usr/local/bin/dsymForUUID", FileSpec::Style::native);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// We couldn't find the dsymForUUID binary.
+g_dsymForUUID_executable = {};
+  });
+  return g_dsymForUUID_executable;
+}
+
+bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
+  Status &error, bool force_lookup) {
+  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
+  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
 
-  // When g_dbgshell_command is NULL, the user has not enabled the use of an
+  llvm::StringRef dbgshell_command = GetDbgShellCommand();
+
+  // When dbgshell_command is empty, the user has not enabled the use of an
   // external program to find the symbols, don't run it for them.
-  if (!force_lookup && g_dbgshell_command == NULL) {
+  if (!force_lookup && !dbgshell_command.empty())
+return false;
+
+  // We need a UUID or valid (existing FileSpec.
+  if (!uuid_ptr &&
+  (!file_spec_ptr || !FileSystem::Instance().Exists(*file_spec_ptr)))
+return false;
+
+  // We need a dsymForUUID binary or an equivalent executable/script.
+  FileSpec dsymForUUID_exe_spec = GetDsymForUUIDExecutable();
+  if (!dsymForUUID_exe_spec)
+return false;
+
+  const std::string dsymForUUID_exe_path = dsymForUUID_exe_spec.GetPath();
+  const std::string uuid_str = uuid_ptr ? uuid_ptr->GetAsString() : "";
+  const std::string file_path_str =
+  file_spec_ptr ? file_spec_ptr-

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione updated this revision to Diff 450434.
kastiglione added a comment.

Fix bug; Add type annotations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

Files:
  lldb/packages/Python/lldbsuite/test/lldbutil.py

Index: lldb/packages/Python/lldbsuite/test/lldbutil.py
===
--- lldb/packages/Python/lldbsuite/test/lldbutil.py
+++ lldb/packages/Python/lldbsuite/test/lldbutil.py
@@ -14,6 +14,7 @@
 import re
 import sys
 import subprocess
+from typing import Dict
 
 # LLDB modules
 import lldb
@@ -205,144 +206,54 @@
 # Convert some enum value to its string counterpart
 # =
 
-def state_type_to_str(enum):
+def _enum_names(prefix: str) -> Dict[int, str]:
+"""Generate a mapping of enum value to name, for the enum prefix."""
+suffix_start = len(prefix)
+return {
+getattr(lldb, attr): attr[suffix_start:].lower()
+for attr in dir(lldb)
+if attr.startswith(prefix)
+}
+
+
+_STATE_NAMES = _enum_names(prefix="eState")
+
+def state_type_to_str(enum: int) -> str:
 """Returns the stateType string given an enum."""
-if enum == lldb.eStateInvalid:
-return "invalid"
-elif enum == lldb.eStateUnloaded:
-return "unloaded"
-elif enum == lldb.eStateConnected:
-return "connected"
-elif enum == lldb.eStateAttaching:
-return "attaching"
-elif enum == lldb.eStateLaunching:
-return "launching"
-elif enum == lldb.eStateStopped:
-return "stopped"
-elif enum == lldb.eStateRunning:
-return "running"
-elif enum == lldb.eStateStepping:
-return "stepping"
-elif enum == lldb.eStateCrashed:
-return "crashed"
-elif enum == lldb.eStateDetached:
-return "detached"
-elif enum == lldb.eStateExited:
-return "exited"
-elif enum == lldb.eStateSuspended:
-return "suspended"
-else:
-raise Exception("Unknown StateType enum: " + str(enum))
+name = _STATE_NAMES.get(enum)
+if name:
+return name
+raise Exception(f"Unknown StateType enum: {enum}")
 
 
-def stop_reason_to_str(enum):
+_STOP_REASON_NAMES = _enum_names(prefix="eStopReason")
+
+def stop_reason_to_str(enum: int) -> str:
 """Returns the stopReason string given an enum."""
-if enum == lldb.eStopReasonInvalid:
-return "invalid"
-elif enum == lldb.eStopReasonNone:
-return "none"
-elif enum == lldb.eStopReasonTrace:
-return "trace"
-elif enum == lldb.eStopReasonBreakpoint:
-return "breakpoint"
-elif enum == lldb.eStopReasonWatchpoint:
-return "watchpoint"
-elif enum == lldb.eStopReasonExec:
-return "exec"
-elif enum == lldb.eStopReasonFork:
-return "fork"
-elif enum == lldb.eStopReasonVFork:
-return "vfork"
-elif enum == lldb.eStopReasonVForkDone:
-return "vforkdone"
-elif enum == lldb.eStopReasonSignal:
-return "signal"
-elif enum == lldb.eStopReasonException:
-return "exception"
-elif enum == lldb.eStopReasonPlanComplete:
-return "plancomplete"
-elif enum == lldb.eStopReasonThreadExiting:
-return "threadexiting"
-elif enum == lldb.eStopReasonInstrumentation:
-return "instrumentation"
-elif enum == lldb.eStopReasonProcessorTrace:
-return "processortrace"
-else:
-raise Exception("Unknown StopReason enum")
+name = _STOP_REASON_NAMES.get(enum)
+if name:
+return name
+raise Exception(f"Unknown StopReason enum: {enum}")
 
 
-def symbol_type_to_str(enum):
+_SYMBOL_TYPE_NAMES = _enum_names(prefix="eSymbolType")
+
+def symbol_type_to_str(enum: int) -> str:
 """Returns the symbolType string given an enum."""
-if enum == lldb.eSymbolTypeInvalid:
-return "invalid"
-elif enum == lldb.eSymbolTypeAbsolute:
-return "absolute"
-elif enum == lldb.eSymbolTypeCode:
-return "code"
-elif enum == lldb.eSymbolTypeData:
-return "data"
-elif enum == lldb.eSymbolTypeTrampoline:
-return "trampoline"
-elif enum == lldb.eSymbolTypeRuntime:
-return "runtime"
-elif enum == lldb.eSymbolTypeException:
-return "exception"
-elif enum == lldb.eSymbolTypeSourceFile:
-return "sourcefile"
-elif enum == lldb.eSymbolTypeHeaderFile:
-return "headerfile"
-elif enum == lldb.eSymbolTypeObjectFile:
-return "objectfile"
-elif enum == lldb.eSymbolTypeCommonBlock:
-return "commonblock"
-elif enum == lldb.eSymbolTypeBlock:
-return "block"
-elif enum == lldb.eSymbolTypeLocal:
-return "local"
-elif enum == lldb.eSymbolTypeParam:
-return "param"
-elif enum == lldb.eSymbolTypeVariable:
-return "variable"
-elif enum == lldb.eSymbolTypeVariable

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione requested review of this revision.
kastiglione added a comment.

@JDevlieghere @mib what do you think of introducing type annotations as code is 
being touched?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

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

[Lldb-commits] [PATCH] D131303: [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

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

- Don't cache the value of `LLDB_APPLE_DSYMFORUUID_EXECUTABLE`


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

https://reviews.llvm.org/D131303

Files:
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -491,190 +491,184 @@
   return success;
 }
 
-bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
-  Status &error, bool force_lookup) {
-  bool success = false;
-  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
-  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
-
-  // It's expensive to check for the DBGShellCommands defaults setting, only do
-  // it once per lldb run and cache the result.
-  static bool g_have_checked_for_dbgshell_command = false;
-  static const char *g_dbgshell_command = NULL;
-  if (!g_have_checked_for_dbgshell_command) {
-g_have_checked_for_dbgshell_command = true;
+/// It's expensive to check for the DBGShellCommands defaults setting. Only do
+/// it once per lldb run and cache the result.
+static llvm::StringRef GetDbgShellCommand() {
+  static std::once_flag g_once_flag;
+  static std::string g_dbgshell_command;
+  std::call_once(g_once_flag, [&]() {
 CFTypeRef defaults_setting = CFPreferencesCopyAppValue(
 CFSTR("DBGShellCommands"), CFSTR("com.apple.DebugSymbols"));
 if (defaults_setting &&
 CFGetTypeID(defaults_setting) == CFStringGetTypeID()) {
-  char cstr_buf[PATH_MAX];
-  if (CFStringGetCString((CFStringRef)defaults_setting, cstr_buf,
- sizeof(cstr_buf), kCFStringEncodingUTF8)) {
-g_dbgshell_command =
-strdup(cstr_buf); // this malloc'ed memory will never be freed
+  char buffer[PATH_MAX];
+  if (CFStringGetCString((CFStringRef)defaults_setting, buffer,
+ sizeof(buffer), kCFStringEncodingUTF8)) {
+g_dbgshell_command = buffer;
   }
 }
 if (defaults_setting) {
   CFRelease(defaults_setting);
 }
+  });
+  return g_dbgshell_command;
+}
+
+/// Get the dsymForUUID executable and cache the result so we don't end up
+/// stat'ing the binary over and over.
+static FileSpec GetDsymForUUIDExecutable() {
+  // The LLDB_APPLE_DSYMFORUUID_EXECUTABLE environment variable is used by the
+  // test suite to override the dsymForUUID location. Because we must be able
+  // to change the value within a single test, don't bother resolving or
+  // caching it.
+  if (const char *dsymForUUID_env =
+  getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
+FileSpec dsymForUUID_executable(dsymForUUID_env);
+if (FileSystem::Instance().Exists(dsymForUUID_executable))
+  return dsymForUUID_executable;
   }
 
-  // When g_dbgshell_command is NULL, the user has not enabled the use of an
+  static std::once_flag g_once_flag;
+  static FileSpec g_dsymForUUID_executable;
+  std::call_once(g_once_flag, [&]() {
+// Try the DBGShellCommand.
+llvm::StringRef dbgshell_command = GetDbgShellCommand();
+if (!dbgshell_command.empty()) {
+  g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// Try dsymForUUID in /usr/local/bin
+{
+  g_dsymForUUID_executable =
+  FileSpec("/usr/local/bin/dsymForUUID", FileSpec::Style::native);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// We couldn't find the dsymForUUID binary.
+g_dsymForUUID_executable = {};
+  });
+  return g_dsymForUUID_executable;
+}
+
+bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
+  Status &error, bool force_lookup) {
+  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
+  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
+
+  llvm::StringRef dbgshell_command = GetDbgShellCommand();
+
+  // When dbgshell_command is empty, the user has not enabled the use of an
   // external program to find the symbols, don't run it for them.
-  if (!force_lookup && g_dbgshell_command == NULL) {
+  if (!force_lookup && !dbgshell_command.empty())
+return false;
+
+  // We need a UUID or valid (existing FileSpec.
+  if (!uuid_ptr &&
+  (!file_spec_ptr || !FileSystem::Instance().Exists(*file_spec_ptr)))
+return false;
+
+  // We need a dsymForUUID binary or an equivalent executable/script.
+  FileSpec dsymForUUID_exe_spec = GetDsymForUUIDExecutable();
+  if (!dsymForUUID_exe_spec)
+return false;
+
+  const std::string dsymForUUID_exe_path = dsymForUUID_exe_spec.GetPath();
+  const std::string uuid_str = uuid_

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

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

With lldb (& llvm) requiring Python 3.6+, use of the `six` module can be 
removed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131304

Files:
  lldb/bindings/interface/SBData.i
  lldb/bindings/python/python.swig
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/examples/summaries/synth.py
  lldb/packages/Python/lldbsuite/support/encoded_file.py
  lldb/packages/Python/lldbsuite/support/seven.py
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py
  lldb/packages/Python/lldbsuite/test/lldbplatform.py
  lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/test/API/api/listeners/TestListener.py
  lldb/test/API/commands/command/script/import/thepackage/TPunitA.py
  lldb/test/API/commands/command/script/import/thepackage/TPunitB.py
  lldb/test/API/commands/process/launch/TestProcessLaunch.py
  lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py
  lldb/test/API/functionalities/postmortem/wow64_minidump/TestWow64MiniDump.py
  lldb/test/API/python_api/frame/TestFrames.py
  lldb/test/API/terminal/TestSTTYBeforeAndAfter.py
  lldb/test/API/test_utils/base/TestBaseTest.py
  lldb/third_party/Python/module/progress/progress.py
  lldb/third_party/Python/module/unittest2/unittest2/case.py
  lldb/third_party/Python/module/unittest2/unittest2/main.py
  lldb/third_party/Python/module/unittest2/unittest2/result.py
  lldb/third_party/Python/module/unittest2/unittest2/suite.py
  lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
  
lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py

Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
@@ -1,5 +1,4 @@
 import unittest2
-import six
 
 from unittest2.test.support import LoggingResult
 
@@ -125,7 +124,7 @@
 def test_id(self):
 test = unittest2.FunctionTestCase(lambda: None)
 
-self.assertIsInstance(test.id(), six.string_types)
+self.assertIsInstance(test.id(), str)
 
 # "Returns a one-line description of the test, or None if no description
 # has been provided. The default implementation of this method returns
Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
@@ -1,7 +1,6 @@
 import difflib
 import pprint
 import re
-import six
 
 from copy import deepcopy
 
@@ -543,7 +542,7 @@
 def runTest(self):
 pass
 
-self.assertIsInstance(Foo().id(), six.string_types)
+self.assertIsInstance(Foo().id(), str)
 
 # "If result is omitted or None, a temporary result object is created
 # and used, but is not made available to the caller. As TestCase owns the
Index: lldb/third_party/Python/module/unittest2/unittest2/suite.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/suite.py
+++ lldb/third_party/Python/module/unittest2/unittest2/suite.py
@@ -3,7 +3,6 @@
 import sys
 import unittest
 from unittest2 import case, util
-import six
 
 __unittest = True
 
@@ -50,7 +49,7 @@
 self._tests.append(test)
 
 def addTests(self, tests):
-if isinstance(tests, six.string_types):
+if isinstance(tests, str):
 raise TypeError("tests must be an iterable of tests, not a string")
 for test in tests:
 self.addTest(test)
Index: lldb/third_party/Python/module/unittest2/unittest2/result.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/result.py
+++ lldb/third_party/Python/module/unittest2/unittest2/result.py
@@ -2,12 +2,11 @@
 
 imp

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added inline comments.



Comment at: lldb/packages/Python/lldbsuite/support/encoded_file.py:17-25
-def _encoded_read(old_read, encoding):
-def impl(size):
-result = old_read(size)
-# If this is Python 2 then we need to convert the resulting `unicode` 
back
-# into a `str` before returning
-if six.PY2:
-result = result.encode(encoding)

this function only deviated in the case of python 2, so I removed it.



Comment at: lldb/packages/Python/lldbsuite/support/seven.py:12
-else:
-def get_command_status_output(command):
-try:

this function was not used externally, so I inlined it into 
`get_command_output`.



Comment at: lldb/packages/Python/lldbsuite/support/seven.py:27
-
-cmp_ = lambda x, y: (x > y) - (x < y)
-

this was also not used.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2278-2281
+assert not isinstance(patterns, str), \
 "patterns must be a collection of strings"
-assert not isinstance(substrs, six.string_types), \
+assert not isinstance(substrs, str), \
 "substrs must be a collection of strings"

this function had a parameter named `str`, which shadowed `builtin.str`. As a 
fix, in this file I renamed all variables named `str` to `string`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131138: [lldb] Dynamically generate enum names in lldbutil

2022-08-05 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib added a comment.

In D131138#3703554 , @kastiglione 
wrote:

> @JDevlieghere @mib what do you think of introducing type annotations as code 
> is being touched?

Since we're basically building an API, it makes a lot of sense of add type 
annotations. I'm strongly in favor of doing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131138

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

I left `third_party/Python/module/six`, in case there are any lldb scripts that 
depend on the existence of `six`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131303: [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

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

- Fix typo
- Still resolve the LLDB_APPLE_DSYMFORUUID_EXECUTABLE in case it's a relative 
path


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

https://reviews.llvm.org/D131303

Files:
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp


Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -519,11 +519,11 @@
 static FileSpec GetDsymForUUIDExecutable() {
   // The LLDB_APPLE_DSYMFORUUID_EXECUTABLE environment variable is used by the
   // test suite to override the dsymForUUID location. Because we must be able
-  // to change the value within a single test, don't bother resolving or
-  // caching it.
+  // to change the value within a single test, don't bother caching it.
   if (const char *dsymForUUID_env =
   getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
 FileSpec dsymForUUID_executable(dsymForUUID_env);
+FileSystem::Instance().Resolve(dsymForUUID_executable);
 if (FileSystem::Instance().Exists(dsymForUUID_executable))
   return dsymForUUID_executable;
   }
@@ -535,6 +535,7 @@
 llvm::StringRef dbgshell_command = GetDbgShellCommand();
 if (!dbgshell_command.empty()) {
   g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  FileSystem::Instance().Resolve(g_dsymForUUID_executable);
   if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
 return;
 }
@@ -542,7 +543,7 @@
 // Try dsymForUUID in /usr/local/bin
 {
   g_dsymForUUID_executable =
-  FileSpec("/usr/local/bin/dsymForUUID", FileSpec::Style::native);
+  FileSpec("/usr/local/bin/dsymForUUID");
   if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
 return;
 }
@@ -562,7 +563,7 @@
 
   // When dbgshell_command is empty, the user has not enabled the use of an
   // external program to find the symbols, don't run it for them.
-  if (!force_lookup && !dbgshell_command.empty())
+  if (!force_lookup && dbgshell_command.empty())
 return false;
 
   // We need a UUID or valid (existing FileSpec.
@@ -635,7 +636,7 @@
 return false;
   }
 
-  if (CFGetTypeID(plist.get()) == CFDictionaryGetTypeID()) {
+  if (CFGetTypeID(plist.get()) != CFDictionaryGetTypeID()) {
 LLDB_LOGF(log, "'%s' failed: output plist is not a valid CFDictionary",
   command.GetData());
 return false;


Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -519,11 +519,11 @@
 static FileSpec GetDsymForUUIDExecutable() {
   // The LLDB_APPLE_DSYMFORUUID_EXECUTABLE environment variable is used by the
   // test suite to override the dsymForUUID location. Because we must be able
-  // to change the value within a single test, don't bother resolving or
-  // caching it.
+  // to change the value within a single test, don't bother caching it.
   if (const char *dsymForUUID_env =
   getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
 FileSpec dsymForUUID_executable(dsymForUUID_env);
+FileSystem::Instance().Resolve(dsymForUUID_executable);
 if (FileSystem::Instance().Exists(dsymForUUID_executable))
   return dsymForUUID_executable;
   }
@@ -535,6 +535,7 @@
 llvm::StringRef dbgshell_command = GetDbgShellCommand();
 if (!dbgshell_command.empty()) {
   g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  FileSystem::Instance().Resolve(g_dsymForUUID_executable);
   if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
 return;
 }
@@ -542,7 +543,7 @@
 // Try dsymForUUID in /usr/local/bin
 {
   g_dsymForUUID_executable =
-  FileSpec("/usr/local/bin/dsymForUUID", FileSpec::Style::native);
+  FileSpec("/usr/local/bin/dsymForUUID");
   if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
 return;
 }
@@ -562,7 +563,7 @@
 
   // When dbgshell_command is empty, the user has not enabled the use of an
   // external program to find the symbols, don't run it for them.
-  if (!force_lookup && !dbgshell_command.empty())
+  if (!force_lookup && dbgshell_command.empty())
 return false;
 
   // We need a UUID or valid (existing FileSpec.
@@ -635,7 +636,7 @@
 return false;
   }
 
-  if (CFGetTypeID(plist.get()) == CFDictionaryGetTypeID()) {
+  if (CFGetTypeID(plist.get()) != CFDictionaryGetTypeID()) {
 LLDB_LOGF(log, "'%s' failed: output plist is not a valid CFDictionary",
   command.GetData());
 return false;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131303: [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

2022-08-05 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere updated this revision to Diff 450442.

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

https://reviews.llvm.org/D131303

Files:
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -491,190 +491,185 @@
   return success;
 }
 
-bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
-  Status &error, bool force_lookup) {
-  bool success = false;
-  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
-  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
-
-  // It's expensive to check for the DBGShellCommands defaults setting, only do
-  // it once per lldb run and cache the result.
-  static bool g_have_checked_for_dbgshell_command = false;
-  static const char *g_dbgshell_command = NULL;
-  if (!g_have_checked_for_dbgshell_command) {
-g_have_checked_for_dbgshell_command = true;
+/// It's expensive to check for the DBGShellCommands defaults setting. Only do
+/// it once per lldb run and cache the result.
+static llvm::StringRef GetDbgShellCommand() {
+  static std::once_flag g_once_flag;
+  static std::string g_dbgshell_command;
+  std::call_once(g_once_flag, [&]() {
 CFTypeRef defaults_setting = CFPreferencesCopyAppValue(
 CFSTR("DBGShellCommands"), CFSTR("com.apple.DebugSymbols"));
 if (defaults_setting &&
 CFGetTypeID(defaults_setting) == CFStringGetTypeID()) {
-  char cstr_buf[PATH_MAX];
-  if (CFStringGetCString((CFStringRef)defaults_setting, cstr_buf,
- sizeof(cstr_buf), kCFStringEncodingUTF8)) {
-g_dbgshell_command =
-strdup(cstr_buf); // this malloc'ed memory will never be freed
+  char buffer[PATH_MAX];
+  if (CFStringGetCString((CFStringRef)defaults_setting, buffer,
+ sizeof(buffer), kCFStringEncodingUTF8)) {
+g_dbgshell_command = buffer;
   }
 }
 if (defaults_setting) {
   CFRelease(defaults_setting);
 }
+  });
+  return g_dbgshell_command;
+}
+
+/// Get the dsymForUUID executable and cache the result so we don't end up
+/// stat'ing the binary over and over.
+static FileSpec GetDsymForUUIDExecutable() {
+  // The LLDB_APPLE_DSYMFORUUID_EXECUTABLE environment variable is used by the
+  // test suite to override the dsymForUUID location. Because we must be able
+  // to change the value within a single test, don't bother caching it.
+  if (const char *dsymForUUID_env =
+  getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
+FileSpec dsymForUUID_executable(dsymForUUID_env);
+FileSystem::Instance().Resolve(dsymForUUID_executable);
+if (FileSystem::Instance().Exists(dsymForUUID_executable))
+  return dsymForUUID_executable;
   }
 
-  // When g_dbgshell_command is NULL, the user has not enabled the use of an
+  static std::once_flag g_once_flag;
+  static FileSpec g_dsymForUUID_executable;
+  std::call_once(g_once_flag, [&]() {
+// Try the DBGShellCommand.
+llvm::StringRef dbgshell_command = GetDbgShellCommand();
+if (!dbgshell_command.empty()) {
+  g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  FileSystem::Instance().Resolve(g_dsymForUUID_executable);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// Try dsymForUUID in /usr/local/bin
+{
+  g_dsymForUUID_executable =
+  FileSpec("/usr/local/bin/dsymForUUID");
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// We couldn't find the dsymForUUID binary.
+g_dsymForUUID_executable = {};
+  });
+  return g_dsymForUUID_executable;
+}
+
+bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
+  Status &error, bool force_lookup) {
+  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
+  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
+
+  llvm::StringRef dbgshell_command = GetDbgShellCommand();
+
+  // When dbgshell_command is empty, the user has not enabled the use of an
   // external program to find the symbols, don't run it for them.
-  if (!force_lookup && g_dbgshell_command == NULL) {
+  if (!force_lookup && dbgshell_command.empty())
+return false;
+
+  // We need a UUID or valid (existing FileSpec.
+  if (!uuid_ptr &&
+  (!file_spec_ptr || !FileSystem::Instance().Exists(*file_spec_ptr)))
+return false;
+
+  // We need a dsymForUUID binary or an equivalent executable/script.
+  FileSpec dsymForUUID_exe_spec = GetDsymForUUIDExecutable();
+  if (!dsymForUUID_exe_spec)
+return false;
+
+  const std::string dsymForUUID_exe_path = dsymForUUID_exe_spec.GetPath();
+  const std::string uuid_str = uuid_ptr ? uuid_ptr->GetAsString() : "";
+  const std::string file_pa

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Med Ismail Bennani via Phabricator via lldb-commits

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

Very cool! Thanks for taking care of this! LGTM with the 2 comments and 
assuming the test suite runs fine :)




Comment at: lldb/examples/python/scripted_process/scripted_process.py:5
 
-@six.add_metaclass(ABCMeta)
-class ScriptedProcess:
+class ScriptedProcess(metaclass=ABCMeta):
 

nit: no need to specify `metaclass=ABCMeta`, it can just be  
`ScriptedProcess(ABC)`



Comment at: lldb/examples/python/scripted_process/scripted_process.py:194
 
-@six.add_metaclass(ABCMeta)
-class ScriptedThread:
+class ScriptedThread(metaclass=ABCMeta):
 

ditto



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2278-2281
+assert not isinstance(patterns, str), \
 "patterns must be a collection of strings"
-assert not isinstance(substrs, six.string_types), \
+assert not isinstance(substrs, str), \
 "substrs must be a collection of strings"

kastiglione wrote:
> this function had a parameter named `str`, which shadowed `builtin.str`. As a 
> fix, in this file I renamed all variables named `str` to `string`.
nit: If you feel like it, may be you can split everything related to the 
`str`-> `string` refactor  into a separate since it's orthogonal to removing 
`six`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib added inline comments.



Comment at: lldb/examples/python/scripted_process/scripted_process.py:5
 
-@six.add_metaclass(ABCMeta)
-class ScriptedProcess:
+class ScriptedProcess(metaclass=ABCMeta):
 

mib wrote:
> nit: no need to specify `metaclass=ABCMeta`, it can just be  
> `ScriptedProcess(ABC)`
disregard this comment, that's actually the way to do it ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2278-2281
+assert not isinstance(patterns, str), \
 "patterns must be a collection of strings"
-assert not isinstance(substrs, six.string_types), \
+assert not isinstance(substrs, str), \
 "substrs must be a collection of strings"

mib wrote:
> kastiglione wrote:
> > this function had a parameter named `str`, which shadowed `builtin.str`. As 
> > a fix, in this file I renamed all variables named `str` to `string`.
> nit: If you feel like it, may be you can split everything related to the 
> `str`-> `string` refactor  into a separate since it's orthogonal to removing 
> `six`
I need to at least make the `str` -> `string` rename in the functions where the 
refactor involves `isinstance(..., str)`. I will remove the other renames, for 
another diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione updated this revision to Diff 450444.
kastiglione added a comment.

Restore some "str" variable names


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

Files:
  lldb/bindings/interface/SBData.i
  lldb/bindings/python/python.swig
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/examples/summaries/synth.py
  lldb/packages/Python/lldbsuite/support/encoded_file.py
  lldb/packages/Python/lldbsuite/support/seven.py
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py
  lldb/packages/Python/lldbsuite/test/lldbplatform.py
  lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/test/API/api/listeners/TestListener.py
  lldb/test/API/commands/command/script/import/thepackage/TPunitA.py
  lldb/test/API/commands/command/script/import/thepackage/TPunitB.py
  lldb/test/API/commands/process/launch/TestProcessLaunch.py
  lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py
  lldb/test/API/functionalities/postmortem/wow64_minidump/TestWow64MiniDump.py
  lldb/test/API/python_api/frame/TestFrames.py
  lldb/test/API/terminal/TestSTTYBeforeAndAfter.py
  lldb/test/API/test_utils/base/TestBaseTest.py
  lldb/third_party/Python/module/progress/progress.py
  lldb/third_party/Python/module/unittest2/unittest2/case.py
  lldb/third_party/Python/module/unittest2/unittest2/main.py
  lldb/third_party/Python/module/unittest2/unittest2/result.py
  lldb/third_party/Python/module/unittest2/unittest2/suite.py
  lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
  
lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py

Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
@@ -1,5 +1,4 @@
 import unittest2
-import six
 
 from unittest2.test.support import LoggingResult
 
@@ -125,7 +124,7 @@
 def test_id(self):
 test = unittest2.FunctionTestCase(lambda: None)
 
-self.assertIsInstance(test.id(), six.string_types)
+self.assertIsInstance(test.id(), str)
 
 # "Returns a one-line description of the test, or None if no description
 # has been provided. The default implementation of this method returns
Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
@@ -1,7 +1,6 @@
 import difflib
 import pprint
 import re
-import six
 
 from copy import deepcopy
 
@@ -543,7 +542,7 @@
 def runTest(self):
 pass
 
-self.assertIsInstance(Foo().id(), six.string_types)
+self.assertIsInstance(Foo().id(), str)
 
 # "If result is omitted or None, a temporary result object is created
 # and used, but is not made available to the caller. As TestCase owns the
Index: lldb/third_party/Python/module/unittest2/unittest2/suite.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/suite.py
+++ lldb/third_party/Python/module/unittest2/unittest2/suite.py
@@ -3,7 +3,6 @@
 import sys
 import unittest
 from unittest2 import case, util
-import six
 
 __unittest = True
 
@@ -50,7 +49,7 @@
 self._tests.append(test)
 
 def addTests(self, tests):
-if isinstance(tests, six.string_types):
+if isinstance(tests, str):
 raise TypeError("tests must be an iterable of tests, not a string")
 for test in tests:
 self.addTest(test)
Index: lldb/third_party/Python/module/unittest2/unittest2/result.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/result.py
+++ lldb/third_party/Python/module/unittest2/unittest2/result.py
@@ -2,12 +2,11 @@
 
 import use_lldb_suite
 
+import io
 import sys
 import traceback
 import unittest
 
-from six import StringIO as SixStringIO
-
 from unittest2 im

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

@mib the tests pass on my machine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131305: [lldb] Tidy some regex in crashlog.py (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione created this revision.
kastiglione added reviewers: JDevlieghere, mib.
Herald added a project: All.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

A spiritual follow up to D131032 . I noticed 
some regex could be simplified.

This does some of the following:

1. Removes unused capture groups
2. Uses non-capturing `(?:...)` groups where grouping is needed but capturing 
isn't
3. Removes trailing `.*`
4. Uses `\d` over `[0-9]`
5. Uses raw strings
6. Uses `{N,}` to indicate N-or-more

Also improves the call site of a `re.findall`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131305

Files:
  lldb/examples/python/crashlog.py


Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -592,26 +592,26 @@
 
 
 class TextCrashLogParser:
-parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
-thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
-thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
-thread_regex = re.compile('^Thread ([0-9]+)([^:]*):(.*)')
-app_backtrace_regex = re.compile('^Application Specific Backtrace 
([0-9]+)([^:]*):(.*)')
-version = r'(\(.+\)|(arm|x86_)[0-9a-z]+)\s+'
-frame_regex = re.compile(r'^([0-9]+)' r'\s'# id
- r'+(.+?)'r'\s+'   # img_name
- r'(' +version+ r')?'  # img_version
- r'(0x[0-9a-fA-F]{7}[0-9a-fA-F]+)' # addr
- r' +(.*)' # offs
+parent_process_regex = re.compile(r'^Parent Process:\s*(.*)\[(\d+)\]')
+thread_state_regex = re.compile(r'^Thread \d+ crashed with')
+thread_instrs_regex = re.compile(r'^Thread \d+ instruction stream')
+thread_regex = re.compile(r'^Thread (\d+).*:')
+app_backtrace_regex = re.compile(r'^Application Specific Backtrace 
(\d+).*:')
+version = r'\(.+\)|(?:arm|x86_)[0-9a-z]+'
+frame_regex = re.compile(r'^(\d+)\s+'  # id
+ r'(.+?)\s+'   # img_name
+ r'(?:' +version+ r'\s+)?' # img_version
+ r'(0x[0-9a-fA-F]{7,})'# addr
+ r' +(.*)' # offs
 )
-null_frame_regex = re.compile(r'^([0-9]+)\s+\?\?\?\s+(0{7}0+) +(.*)')
-image_regex_uuid = re.compile(r'(0x[0-9a-fA-F]+)'# img_lo
-  r'\s+' '-' r'\s+'  #   -
-  r'(0x[0-9a-fA-F]+)' r'\s+' # img_hi
-  r'[+]?(.+?)'r'\s+' # img_name
-  r'(' +version+ ')?'# img_version
-  r'(<([-0-9a-fA-F]+)>\s+)?' # img_uuid
-  r'(/.*)'   # img_path
+null_frame_regex = re.compile(r'^\d+\s+\?\?\?\s+0{7,} +')
+image_regex_uuid = re.compile(r'(0x[0-9a-fA-F]+)'  # img_lo
+  r'\s+-\s+'   #   -
+  r'(0x[0-9a-fA-F]+)\s+'   # img_hi
+  r'[+]?(.+?)\s+'  # img_name
+  r'(?:(' +version+ r')\s+)?'  # img_version
+  r'(?:<([-0-9a-fA-F]+)>\s+)?' # img_uuid
+  r'(/.*)' # img_path
  )
 
 
@@ -762,8 +762,8 @@
 return
 frame_match = self.frame_regex.search(line)
 if frame_match:
-(frame_id, frame_img_name, _, frame_img_version, _,
-frame_addr, frame_ofs) = frame_match.groups()
+(frame_id, frame_img_name, frame_addr,
+frame_ofs) = frame_match.groups()
 ident = frame_img_name
 self.thread.add_ident(ident)
 if ident not in self.crashlog.idents:
@@ -776,8 +776,8 @@
 def parse_images(self, line):
 image_match = self.image_regex_uuid.search(line)
 if image_match:
-(img_lo, img_hi, img_name, _, img_version, _,
-_, img_uuid, img_path) = image_match.groups()
+(img_lo, img_hi, img_name, img_version,
+img_uuid, img_path) = image_match.groups()
 image = self.crashlog.DarwinImage(int(img_lo, 0), int(img_hi, 0),
 img_name.strip(),
 img_version.strip()
@@ -790,13 +790,10 @@
 
 
 def parse_thread_registers(self, line):
-stripped_line = line.strip()
 # "r12: 0x7fff6b5939c8  r13: 0x0

[Lldb-commits] [PATCH] D131305: [lldb] Tidy some regex in crashlog.py (NFC)

2022-08-05 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

this includes changes in D131032  and so 
depends on it merging first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131305

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

[Lldb-commits] [PATCH] D131294: [LLDB][NFC] Reliability fixes to TCPSocket code

2022-08-05 Thread Will Hawkins via Phabricator via lldb-commits

hawkinsw added a comment.

Not that my opinion means much, but the changes look good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131294

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

[Lldb-commits] [lldb] bcac7b3 - [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread Slava Gurevich via lldb-commits


Author: Slava Gurevich
Date: 2022-08-05T18:33:18-07:00
New Revision: bcac7b3acb1972bdfabe3c84f51243e9a353e7fe

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

LOG:   [LLDB] Missing break in a switch statement alters the execution flow.

Looks like a typo from the past code changes.

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

Added: 


Modified: 
lldb/source/Plugins/Process/Utility/ARMUtils.h

Removed: 




diff  --git a/lldb/source/Plugins/Process/Utility/ARMUtils.h 
b/lldb/source/Plugins/Process/Utility/ARMUtils.h
index bbe4c9a35fa6..a7aaa5ac7a1f 100644
--- a/lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ b/lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@ static inline uint32_t DecodeImmShift(const uint32_t type, 
const uint32_t imm5,
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@ static inline uint32_t ARMExpandImm(uint32_t opcode) {
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@ static inline uint32_t ThumbExpandImm_C(uint32_t opcode, 
uint32_t carry_in,
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;



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

[Lldb-commits] [PATCH] D131308: [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

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

  Looks like a typo from the past code changes.
  
  Differential Revision: https://reviews.llvm.org/D131244


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131308

Files:
  lldb/source/Plugins/Process/Utility/ARMUtils.h


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

fixathon updated this revision to Diff 450460.
fixathon added a comment.

Addressed comments


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

https://reviews.llvm.org/D131244

Files:
  lldb/source/Plugins/Process/Utility/ARMUtils.h


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

fixathon updated this revision to Diff 450466.
fixathon added a comment.

Code update to address the comments

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131244

Files:
  lldb/source/Plugins/Process/Utility/ARMUtils.h


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;


Index: lldb/source/Plugins/Process/Utility/ARMUtils.h
===
--- lldb/source/Plugins/Process/Utility/ARMUtils.h
+++ lldb/source/Plugins/Process/Utility/ARMUtils.h
@@ -25,7 +25,8 @@
   ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+assert(0 && "Invalid shift type");
+break;
   case 0:
 shift_t = SRType_LSL;
 return imm5;
@@ -302,7 +303,7 @@
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
 uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
@@ -311,6 +312,8 @@
   if (bits(imm12, 11, 10) == 0) {
 switch (bits(imm12, 9, 8)) {
 default: // Keep static analyzer happy with a default case
+  break;
+
 case 0:
   imm32 = abcdefgh;
   break;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D113155: [lldb] Remove nested switches from ARMGetSupportedArchitectureAtIndex (NFC)

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

fixathon added inline comments.



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:640
+  }
+  return {};
+}

JDevlieghere wrote:
> fixathon wrote:
> > JDevlieghere wrote:
> > > fixathon wrote:
> > > > Could we delete this unreachable return statement?  It's tripping up 
> > > > static code checkers since all possible branches already have a return.
> > > Yes, let's make it an `llvm_unreachable` (otherwise gcc will complain).
> > If we return {} in the **default** case, and delete this,  gcc should not 
> > complain since all possible code paths will have a return, is that right?
> > Alternatively, we could break in the **default** case and leave this return 
> > alone. I prefer the first approach for consistency.
> Yes, you're right, it's only a problem for gcc when you explicitly handle 
> every case (see all the instances of `llvm_unreachable("Fully covered 
> switch!");` in lldb/llvm). 
I'll just add a break to the default case to avoid llvm_unreachable(). What's 
the best way to introduce this change? Can I link it to this diff somehow or a 
stand-alone new diff?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113155

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

[Lldb-commits] [PATCH] D130942: [LLDB][DWARF] Set MSInheritanceAttr for record decl when it's using Microsoft compatible ABI.

2022-08-05 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754
+  m_ast.getASTContext(),
+  
clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance));
+}

rnk wrote:
> mstorsjo wrote:
> > rnk wrote:
> > > I'm concerned that this isn't really the right fix. Changing the 
> > > inheritance model changes the size of the member pointer representation, 
> > > so the consequences of getting the wrong one could affect expression 
> > > evaluation results. Zequan suggested guessing the inheritance model from 
> > > the class definition, but we really ought to write down the inheritance 
> > > model in the DWARF when using the MS ABI. This probably needs its own 
> > > DWARF attribute.
> > FWIW, there’s two use cases at play here:
> > 
> > 1. The executable actually is using the itanium ABI, but is being 
> > (mis)interpreted with the MSVC ABI. When this happens, we currently crash, 
> > which is a terrible user experience. In this case, there’s probably no good 
> > solution (we can’t really get it right at this point) - but pretty much 
> > anything is better than crashing. Before D127048, we always interpreted 
> > everything with the MSVC C++ ABI; now we probably are getting it right in a 
> > majority of cases at least.
> > 
> > 2. People intentionally using dwarf debug into with MSVC ABI. Not very 
> > common, but something that some people do use, as far as I know. Here we at 
> > least have a chance of getting right.
> I see, I thought we already knew we were in use case 2 not 1. Having this as 
> a workaround to not crash seems fine, but we really ought to warn when LLDB 
> encounters these conditions:
> 1. MS C++ ABI is in use
> 2. DWARF is in use
> 3. A C++ source file is in use
> 
> Using DWARF for C code in the MS ABI model is fine, no need to warn in that 
> case.
Some kind of warning would be great, yes. Is MS ABI C++ in dwarf essentially 
broken by definition (unless we’d extend our dwarf generation)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

2022-08-05 Thread Slava Gurevich via Phabricator via lldb-commits

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

Current application of bitwise-OR to a binary mask always results in True, 
which seems
inconsistent with the intent of the statement, a likely typo.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131312

Files:
  lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp


Index: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
===
--- lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
+++ lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
@@ -63,7 +63,7 @@
   const lldb::addr_t one_cmpl64 = ~((lldb::addr_t)0);
   const lldb::addr_t one_cmpl32 = ~((uint32_t)0);
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
+  if ((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);
   } else {
 result.Printf("lbound = 0x%" PRIu64 ",", lbound);


Index: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
===
--- lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
+++ lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
@@ -63,7 +63,7 @@
   const lldb::addr_t one_cmpl64 = ~((lldb::addr_t)0);
   const lldb::addr_t one_cmpl32 = ~((uint32_t)0);
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
+  if ((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", value);
   } else {
 result.Printf("lbound = 0x%" PRIu64 ",", lbound);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

49 matches

Mail list logo