[Lldb-commits] [PATCH] D156493: [lldb-vsocde] Adding support for the "disassemble" request.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

It's failing the test, but actually it looks like a cleanup step is failing. 
Whether that is also the timeout I don't know. I will restart the machine to 
rule out any lingering processes and get back to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156493

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

[Lldb-commits] [PATCH] D156804: [lldb] Bump SWIG minimum version to 4

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

We are on 4.01 (AArch64), 4.02 (Arm) and 4.1.1 (Windows on Arm), so we have no 
issues with this.


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

https://reviews.llvm.org/D156804

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

[Lldb-commits] [PATCH] D156970: Fix crash in lldb-vscode when missing function name

[Tom Yang via Phabricator via lldb-commits]

zhyty created this revision.
Herald added a project: All.
zhyty requested review of this revision.
Herald added subscribers: lldb-commits, wangpc.
Herald added a project: LLDB.

In cases where the PC has no function name, lldb-vscode crashes.

`lldb::SBFrame::GetDisplayFunctionName()` returns a `nullptr`, and when we
attempt to construct an `std::string`, it raises an exception.

Test plan:
This can be observed with creating a test file (credit to @clayborg for the
example):

  int main() {
typedef void (*FooCallback)();
FooCallback foo_callback = (FooCallback)0;
foo_callback(); // Crash at zero!
return 0;
  }

and attempting to debug the file through VSCode.

I add a test case in D156732  which should 
cover this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156970

Files:
  lldb/tools/lldb-vscode/JSONUtils.cpp


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -696,7 +696,11 @@
   int64_t frame_id = MakeVSCodeFrameID(frame);
   object.try_emplace("id", frame_id);
 
-  std::string frame_name = frame.GetDisplayFunctionName();
+  // `function_name` can be a nullptr, which throws an error when assigned to 
an
+  // `std::string`.
+  const char *function_name = frame.GetDisplayFunctionName();
+  std::string frame_name =
+  function_name == nullptr ? std::string() : function_name;
   if (frame_name.empty())
 frame_name = "";
   bool is_optimized = frame.GetFunction().GetIsOptimized();


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -696,7 +696,11 @@
   int64_t frame_id = MakeVSCodeFrameID(frame);
   object.try_emplace("id", frame_id);
 
-  std::string frame_name = frame.GetDisplayFunctionName();
+  // `function_name` can be a nullptr, which throws an error when assigned to an
+  // `std::string`.
+  const char *function_name = frame.GetDisplayFunctionName();
+  std::string frame_name =
+  function_name == nullptr ? std::string() : function_name;
   if (frame_name.empty())
 frame_name = "";
   bool is_optimized = frame.GetFunction().GetIsOptimized();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156732: Display PC instead of for stack trace in vscode

[Tom Yang via Phabricator via lldb-commits]

zhyty updated this revision to Diff 546738.
zhyty added a comment.

- added a test case
- rebased this commit on top of D156970 , 
which fixes a crash preventing me from testing this feature
- addressed other comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156732

Files:
  lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/Makefile
  
lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
  lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/a.out
  lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
  lldb/tools/lldb-vscode/JSONUtils.cpp


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -701,8 +701,12 @@
   const char *function_name = frame.GetDisplayFunctionName();
   std::string frame_name =
   function_name == nullptr ? std::string() : function_name;
-  if (frame_name.empty())
-frame_name = "";
+  if (frame_name.empty()) {
+// If the function name is unavailable, display the pc address as a 
16-digit
+// hex string, e.g. "0x00012345"
+llvm::raw_string_ostream os(frame_name);
+os << llvm::format_hex(frame.GetPC(), 18);
+  }
   bool is_optimized = frame.GetFunction().GetIsOptimized();
   if (is_optimized)
 frame_name += " [opt]";
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
@@ -0,0 +1,6 @@
+int main() {
+  typedef void (*FooCallback)();
+  FooCallback foo_callback = (FooCallback)0;
+  foo_callback(); // Crash at zero!
+  return 0;
+}
Index: 
lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
===
--- /dev/null
+++ 
lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
@@ -0,0 +1,26 @@
+"""
+Test lldb-vscode stack trace response
+"""
+
+
+import vscode
+from lldbsuite.test.decorators import *
+import os
+
+import lldbvscode_testcase
+from lldbsuite.test import lldbtest, lldbutil
+
+
+class 
TestVSCode_stackTraceMissingFunctionName(lldbvscode_testcase.VSCodeTestCaseBase):
+@skipIfWindows
+@skipIfRemote
+def test_missingFunctionName(self):
+"""
+Test that the stack frame without a function name is given its pc in 
the response.
+"""
+program = self.getBuildArtifact("a.out")
+self.build_and_launch(program)
+
+self.continue_to_next_stop()
+frame_without_function_name = self.get_stackFrames()[0]
+self.assertEquals(frame_without_function_name["name"], 
"0x")
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/Makefile
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/Makefile
@@ -0,0 +1,2 @@
+CXX_SOURCES := main.cpp
+include Makefile.rules


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -701,8 +701,12 @@
   const char *function_name = frame.GetDisplayFunctionName();
   std::string frame_name =
   function_name == nullptr ? std::string() : function_name;
-  if (frame_name.empty())
-frame_name = "";
+  if (frame_name.empty()) {
+// If the function name is unavailable, display the pc address as a 16-digit
+// hex string, e.g. "0x00012345"
+llvm::raw_string_ostream os(frame_name);
+os << llvm::format_hex(frame.GetPC(), 18);
+  }
   bool is_optimized = frame.GetFunction().GetIsOptimized();
   if (is_optimized)
 frame_name += " [opt]";
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
@@ -0,0 +1,6 @@
+int main() {
+  typedef void (*FooCallback)();
+  FooCallback foo_callback = (FooCallback)0;
+  foo_callback(); // Crash at zero!
+  return 0;
+}
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
@@ -0,0 +1,26 @@
+"""
+Test lldb-vscode stack trace response
+"""
+
+
+import vscode
+from lldbsuite.test.decorators import *
+import os
+
+import lldbvscode_

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/CodeOwners.rst:7-8
+particular part of LLDB are reviewed, either by themself or by someone else.
+They are also the gatekeepers for their part of LLDB, with the final word on
+what goes in or not.
+

This could be taken to mean every review must have approval from a code owner, 
on top of whatever other review has been done. Is that the intent? Someone 
coming from a project with strong maintainer rules (e.g. GDB, so I gather) may 
take it that way.



Comment at: lldb/CodeOwners.rst:214
+
+
+Tools

Is the extra space an RST thing, seems placed randomly.


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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/include/lldb/API/SBProcess.h:433
+
+  /// Clear the non-addressable bits of an \a addr value and return a
+  /// virtual address in memory.

"Clear bits from an addr value that are not used for addressing" is clearer to 
me.

Addressable makes me think I can craft an address to point to that bit, and 
I've been saying "non-address" but that term is also inside baseball, so 
something more wordy I think is better.



Comment at: lldb/include/lldb/API/SBProcess.h:445
+  lldb::addr_t FixDataAddress(lldb::addr_t addr);
+  lldb::addr_t FixAnyAddress(lldb::addr_t addr);
+

jasonmolenda wrote:
> clayborg wrote:
> > What does this function do? Does it try to auto detect code/data low/hi on 
> > its own assuming that specific regions are easily identifiable?
> We have the same method in Process.  Linux has separate masks for code & data 
> addresses, so we have FixCode and FixData methods, but sometimes you just 
> have an addr_t and you don't know the context (e.g. a register value that 
> you're testing to see if it points to a symbol so you can print that as 
> additional info to the user), and so in that case we have a 
> Process::FixAddress.
> 
> There is one armv7 abi that clears the 0th bit on FixCode address, and I 
> could see other ABIs doing similar things for the fixed nature of code 
> addresses.  But Data addresses are the most general, having to point to any 
> addressable byte.
> 
> Our Process/SBProcess say "FixCode if you know it's code, FixData if you know 
> it's data.  And if you don't know, FixData".  But I think having a 
> "FixAddress" method, making it clear that you don't know the origin of the 
> value, makes the intention a little clearer.  Even if it's just calling 
> FixData inside Process.
FixAnyAddress is also a marker for the, hopefully far from now, time, that lldb 
has to track address provenance. That's not an API concern though.

Should these functions also be one function that takes the enum value?

It's `FixAddress(addr, lldb.eMaskTypeData` instead of `FixDataAddress(addr)`, 
but it keeps the theme consistent between all the functions.



Comment at: lldb/include/lldb/API/SBProcess.h:401
 
+  /// Get the current address mask that may be applied to addresses
+  /// before reading from memory.  The bits which are not relevant/valid

jasonmolenda wrote:
> DavidSpickett wrote:
> > may -> should?
> > 
> > Though for API uses that only run on simple systems, calling this would 
> > just be unnecessary overhead.
> Probably the right call.  The masking is implemented in the ABI so there are 
> systems that don't have this concept and have no mask function, which is what 
> I was thinking of with "maybe".  
> 
> It makes me wonder if doing it in the ABI is correct; if we have an address 
> mask for the system, is it a part of the ABI?  On an ARMv8.3 ptrauth using 
> process, the ABI defines what things are signed and need to be cleared before 
> dereferencing (where we need to call FixAddress from within lldb), but we 
> solve this by calling FixAddress at all sites that any ABI might use; 
> ultimately they all need to resolve to an addressable address before reading, 
> so if one ABI signs the C++ vtable pointer and one does not, having both call 
> FixAddress on that is fine.
> 
> Of course I'm coming at this with an AArch64 perspective.  On AArch64 we need 
> to look at b55 to decide if the bits that are masked off are set to 0 or 1  
> (bits 56-63 possibly being TBI non-address bits), which is not a generic 
> behavior that would apply on other processors that might need to apply an 
> address mask.  So maybe it's an architecture specific method instead of an 
> ABI method, I should say.
I don't think it being in the ABI plugin is particularly deliberate, even if it 
is correct.

The ABI states something like the top byte is reserved for the OS and language 
runtimes, lldb is dealing with a consequence of that choice. So you could argue 
the ABI plugin should say "yes this should be fixed" and the fixing should 
happen elsewhere.

But right now those two things are so close together I don't think it matters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155905

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

[Lldb-commits] [PATCH] D156493: [lldb-vsocde] Adding support for the "disassemble" request.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> Also it would be very useful if the logs could be printed in a pretty form 
> like:

And I'll have a patch for this shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156493

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

[Lldb-commits] [PATCH] D156977: [lldb][lldb-vscode] Fix nullptr dereference when JSON is not an object

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: lldb-commits, wangpc.
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156977

Files:
  lldb/tools/lldb-vscode/VSCode.cpp


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -523,12 +523,13 @@
 }
 return PacketStatus::JSONMalformed;
   }
-  object = *json_value->getAsObject();
-  if (!json_value->getAsObject()) {
+  llvm::json::Object *object_ptr = json_value->getAsObject();
+  if (!object_ptr) {
 if (log)
   *log << "error: json packet isn't a object" << std::endl;
 return PacketStatus::JSONNotObject;
   }
+  object = *object_ptr;
   return PacketStatus::Success;
 }
 


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -523,12 +523,13 @@
 }
 return PacketStatus::JSONMalformed;
   }
-  object = *json_value->getAsObject();
-  if (!json_value->getAsObject()) {
+  llvm::json::Object *object_ptr = json_value->getAsObject();
+  if (!object_ptr) {
 if (log)
   *log << "error: json packet isn't a object" << std::endl;
 return PacketStatus::JSONNotObject;
   }
+  object = *object_ptr;
   return PacketStatus::Success;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156979: [lldb][lldb-vscode] Pretty print JSON to log files

[David Spickett via Phabricator via lldb-commits]

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

This makes anlysing test failures much more easy.

For SendJSON this is simple, just use llvm::format instead.

For GetNextObject/ReadJSON it's a bit more tricky.

- Print the "Content-Length:" line in ReadJSON, but not the json.
- Back in GetNextObject, if the JSON doesn't parse, it'll be printed as a 
normal string along with an error message.
- If we didn't error before, we have a JSON value so we pretty print it.
- Finally, if it isn't an object we'll log an error for that, not including the 
JSON.

Before:

  <--
  Content-Length: 81
  
  
{"command":"disconnect","request_seq":5,"seq":0,"success":true,"type":"response"}

After:

  <--
  Content-Length: 81
  
  {
"command": "disconnect",
"request_seq": 5,
"seq": 0,
"success": true,
"type": "response"
  }

There appear to be some responses that include strings that are themselves JSON,
and this won't pretty print those but I think it's still worth doing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156979

Files:
  lldb/tools/lldb-vscode/VSCode.cpp


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -89,12 +89,6 @@
   output.write_full(llvm::utostr(json_str.size()));
   output.write_full("\r\n\r\n");
   output.write_full(json_str);
-
-  if (log) {
-*log << "<-- " << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << json_str << std::endl;
-  }
 }
 
 // Serialize the JSON value into a string and send the JSON packet to
@@ -105,7 +99,14 @@
   strm << json;
   static std::mutex mutex;
   std::lock_guard locker(mutex);
-  SendJSON(strm.str());
+  std::string json_str = strm.str();
+  SendJSON(json_str);
+
+  if (log) {
+*log << "<-- " << std::endl
+ << "Content-Length: " << json_str.size() << "\r\n\r\n"
+ << llvm::formatv("{0:2}", json).str() << std::endl;
+  }
 }
 
 // Read a JSON packet from the "in" stream.
@@ -129,11 +130,8 @@
   if (!input.read_full(log.get(), length, json_str))
 return json_str;
 
-  if (log) {
-*log << "--> " << std::endl
- << "Content-Length: " << length << "\r\n\r\n"
- << json_str << std::endl;
-  }
+  if (log)
+*log << "--> " << std::endl << "Content-Length: " << length << "\r\n\r\n";
 
   return json_str;
 }
@@ -523,6 +521,11 @@
 }
 return PacketStatus::JSONMalformed;
   }
+
+  if (log) {
+*log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
+  }
+
   llvm::json::Object *object_ptr = json_value->getAsObject();
   if (!object_ptr) {
 if (log)


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -89,12 +89,6 @@
   output.write_full(llvm::utostr(json_str.size()));
   output.write_full("\r\n\r\n");
   output.write_full(json_str);
-
-  if (log) {
-*log << "<-- " << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << json_str << std::endl;
-  }
 }
 
 // Serialize the JSON value into a string and send the JSON packet to
@@ -105,7 +99,14 @@
   strm << json;
   static std::mutex mutex;
   std::lock_guard locker(mutex);
-  SendJSON(strm.str());
+  std::string json_str = strm.str();
+  SendJSON(json_str);
+
+  if (log) {
+*log << "<-- " << std::endl
+ << "Content-Length: " << json_str.size() << "\r\n\r\n"
+ << llvm::formatv("{0:2}", json).str() << std::endl;
+  }
 }
 
 // Read a JSON packet from the "in" stream.
@@ -129,11 +130,8 @@
   if (!input.read_full(log.get(), length, json_str))
 return json_str;
 
-  if (log) {
-*log << "--> " << std::endl
- << "Content-Length: " << length << "\r\n\r\n"
- << json_str << std::endl;
-  }
+  if (log)
+*log << "--> " << std::endl << "Content-Length: " << length << "\r\n\r\n";
 
   return json_str;
 }
@@ -523,6 +521,11 @@
 }
 return PacketStatus::JSONMalformed;
   }
+
+  if (log) {
+*log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
+  }
+
   llvm::json::Object *object_ptr = json_value->getAsObject();
   if (!object_ptr) {
 if (log)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156979: [lldb][lldb-vscode] Pretty print JSON to log files

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added reviewers: ashgti, wallace.
DavidSpickett added a comment.

The one disadvantage here is that pretty printing adds more characters, so 
before you could copy paste the single JSON line and (I presume) it would have 
the same number of characters.

If that's a required use case then instead I can do the printing in the testing 
wrapper in lldb, by just going line by line and anything beginning with `{`, 
attempt to parse as JSON. I just figured that this formatting may be useful 
outside of the lldb context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156979

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

[Lldb-commits] [PATCH] D156118: [lldb][AArch64] Add reading of TLS tpidr register from core files

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156118

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

[Lldb-commits] [PATCH] D156512: [lldb][AArch64] Save/restore TLS registers around expressions

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156512

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

[Lldb-commits] [PATCH] D156118: [lldb][AArch64] Add reading of TLS tpidr register from core files

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

Looks legit. Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156118

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

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM modulo nits found by others, thank you for this!


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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D156512: [lldb][AArch64] Save/restore TLS registers around expressions

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

This looks good. Just a nit the summary seems to be using a lot pronouns. Could 
you kindly improve summary a little and add a one liner about expr_func and how 
it tests save/restore for future readers of this log.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156512

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

[Lldb-commits] [lldb] 43ad521 - [lldb][AArch64] Add reading of TLS tpidr register from core files

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-08-03T12:06:06Z
New Revision: 43ad521f2fa9a50e6b30425a3b9fb3025b42587a

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

LOG: [lldb][AArch64] Add reading of TLS tpidr register from core files

7e229217f4215b519b886e7881bae4da3742a7d2 did live processes, this does
core files. Pretty simple, there is an NT_ARM_TLS note that contains
at least tpidr, and on systems with the Scalable Matrix Extension (SME), tpidr2
as well.

tpidr2 will be handled in future patches for SME support.

This NT_ARM_TLS note has always been present but it seems convenient to
handle it as "optional" inside of LLDB. We'll probably want the flexibility
when supporting tpidr2.

Normally the C library would set tpidr but all our test sources build
without it. So I've updated the neon test program to write to tpidr
and regenerated the corefile.

I've removed the LLDB_PTRACE_NT_ARM_TLS that was unused, we get
what we need from llvm's defs instead.

Reviewed By: omjavaid

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

Added: 


Modified: 
lldb/include/lldb/Host/linux/Ptrace.h
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.c
lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.core
llvm/docs/ReleaseNotes.rst

Removed: 




diff  --git a/lldb/include/lldb/Host/linux/Ptrace.h 
b/lldb/include/lldb/Host/linux/Ptrace.h
index 29489d1ae4862f..aabd3fd4fc5573 100644
--- a/lldb/include/lldb/Host/linux/Ptrace.h
+++ b/lldb/include/lldb/Host/linux/Ptrace.h
@@ -57,6 +57,4 @@ typedef int __ptrace_request;
 #define PTRACE_POKEMTETAGS 34
 #endif
 
-#define LLDB_PTRACE_NT_ARM_TLS 0x401 // ARM TLS register
-
 #endif // liblldb_Host_linux_Ptrace_h_

diff  --git 
a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
index 676e450c4846ad..d306c818e89f4a 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
@@ -47,6 +47,10 @@ bool RegisterContextPOSIX_arm64::IsPAuth(unsigned reg) const 
{
   return m_register_info_up->IsPAuthReg(reg);
 }
 
+bool RegisterContextPOSIX_arm64::IsTLS(unsigned reg) const {
+  return m_register_info_up->IsTLSReg(reg);
+}
+
 RegisterContextPOSIX_arm64::RegisterContextPOSIX_arm64(
 lldb_private::Thread &thread,
 std::unique_ptr register_info)

diff  --git a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
index 7c301599d3af6e..6a935274fc40d4 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
@@ -55,6 +55,7 @@ class RegisterContextPOSIX_arm64 : public 
lldb_private::RegisterContext {
 
   bool IsSVE(unsigned reg) const;
   bool IsPAuth(unsigned reg) const;
+  bool IsTLS(unsigned reg) const;
 
   bool IsSVEZ(unsigned reg) const { return m_register_info_up->IsSVEZReg(reg); 
}
   bool IsSVEP(unsigned reg) const { return m_register_info_up->IsSVEPReg(reg); 
}

diff  --git a/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
index 8505ebfa9eb971..5de7cfea14c1b2 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
@@ -119,6 +119,7 @@ class RegisterInfoPOSIX_arm64
   bool IsSSVEEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskSSVE); }
   bool IsPAuthEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskPAuth); 
}
   bool IsMTEEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskMTE); }
+  bool IsTLSEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskTLS); }
 
   bool IsSVEReg(unsigned reg) const;
   bool IsSVEZReg(unsigned reg) const;

diff  --git 
a/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp 
b/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
index 22a9996b1a6e93..38abd8f8f2b116 100644
--- a/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ b/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -34,6 

[Lldb-commits] [PATCH] D156118: [lldb][AArch64] Add reading of TLS tpidr register from core files

[David Spickett via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG43ad521f2fa9: [lldb][AArch64] Add reading of TLS tpidr 
register from core files (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156118

Files:
  lldb/include/lldb/Host/linux/Ptrace.h
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.core
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -135,6 +135,9 @@
 Changes to LLDB
 -
 
+* AArch64 Linux targets now provide access to the Thread Local Storage
+  register ``tpidr``.
+
 Changes to Sanitizers
 -
 * HWASan now defaults to detecting use-after-scope bugs.
Index: lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.c
===
--- lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.c
+++ lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.c
@@ -1,6 +1,8 @@
 // compile with -march=armv8-a+simd on compatible aarch64 compiler
 // linux-aarch64-neon.core was generated by: aarch64-linux-gnu-gcc-8
 // commandline: -march=armv8-a+simd -nostdlib -static -g linux-aarch64-neon.c
+#include 
+
 static void bar(char *boom) {
   char F = 'b';
   asm volatile("fmov d0,  #0.5\n\t");
@@ -14,6 +16,9 @@
   asm volatile("movi v8.16b, #0x11\n\t");
   asm volatile("movi v31.16b, #0x30\n\t");
 
+  uint64_t pattern = 0x1122334455667788;
+  asm volatile("msr tpidr_el0, %0" ::"r"(pattern));
+
   *boom = 47; // Frame bar
 }
 
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -304,10 +304,10 @@
 values = {}
 values["x1"] = "0x002f"
 values["w1"] = "0x002f"
-values["fp"] = "0x007fc5dd7f20"
-values["lr"] = "0x00400180"
-values["sp"] = "0x007fc5dd7f00"
-values["pc"] = "0x0040014c"
+values["fp"] = "0xdab7c770"
+values["lr"] = "0x0040019c"
+values["sp"] = "0xdab7c750"
+values["pc"] = "0x00400168"
 values[
 "v0"
 ] = "{0x00 0x00 0x00 0x00 0x00 0x00 0xe0 0x3f 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
@@ -366,6 +366,7 @@
 values["d31"] = "1.3980432860952889E-76"
 values["fpsr"] = "0x"
 values["fpcr"] = "0x"
+values["tpidr"] = "0x1122334455667788"
 
 for regname, value in values.items():
 self.expect(
Index: lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
+++ lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
@@ -123,6 +123,10 @@
 {llvm::Triple::Linux, llvm::Triple::aarch64, llvm::ELF::NT_ARM_PAC_MASK},
 };
 
+constexpr RegsetDesc AARCH64_TLS_Desc[] = {
+{llvm::Triple::Linux, llvm::Triple::aarch64, llvm::ELF::NT_ARM_TLS},
+};
+
 constexpr RegsetDesc PPC_VMX_Desc[] = {
 {llvm::Triple::FreeBSD, llvm::Triple::UnknownArch, llvm::ELF::NT_PPC_VMX},
 {llvm::Triple::Linux, llvm::Triple::UnknownArch, llvm::ELF::NT_PPC_VMX},
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -57,6 +57,7 @@
   lldb_private::DataExtractor m_fpr_data;
   lldb_private::DataExtractor m_sve_data;
   lldb_private::DataExtractor m_pac_data;
+  lldb_private::DataExtractor m_tls_data;
 
   SVEState m_sve_state;
   uint16_t m_sve_vector_length = 0;
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/

[Lldb-commits] [lldb] 6239227 - [lldb][AArch64] Save/restore TLS registers around expressions

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-08-03T12:33:49Z
New Revision: 6239227172cdc92f3bb72131333f50f83a6439cf

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

LOG: [lldb][AArch64] Save/restore TLS registers around expressions

Previously lldb was storing them but not restoring them. Meaning that this 
function:
```
void expr(uint64_t value) {
  __asm__ volatile("msr tpidr_el0, %0" ::"r"(value));
}
```
When run from lldb:
```
(lldb) expression expr()
```
Would leave tpidr as `value` instead of the original value of the register.

A check for this scenario has been added to TestAArch64LinuxTLSRegisters.py,
which covers tpidr and the SME excluisve tpidr2 register when it's present.

Reviewed By: omjavaid

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

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
lldb/test/API/linux/aarch64/tls_registers/main.c

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
index ae9fe7039a9204..490b4d619edb59 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -559,8 +559,10 @@ Status 
NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
 dst += GetFPRSize();
   }
 
-  if (GetRegisterInfo().IsMTEEnabled())
+  if (GetRegisterInfo().IsMTEEnabled()) {
 ::memcpy(dst, GetMTEControl(), GetMTEControlSize());
+dst += GetMTEControlSize();
+  }
 
   ::memcpy(dst, GetTLSBuffer(), GetTLSBufferSize());
 
@@ -671,8 +673,17 @@ Status 
NativeRegisterContextLinux_arm64::WriteAllRegisterValues(
 ::memcpy(GetMTEControl(), src, GetMTEControlSize());
 m_mte_ctrl_is_valid = true;
 error = WriteMTEControl();
+if (error.Fail())
+  return error;
+src += GetMTEControlSize();
   }
 
+  // There is always a TLS set. It changes size based on system properties, 
it's
+  // not something an expression can change.
+  ::memcpy(GetTLSBuffer(), src, GetTLSBufferSize());
+  m_tls_is_valid = true;
+  error = WriteTLS();
+
   return error;
 }
 

diff  --git 
a/lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py 
b/lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
index f1bc6b45a9d815..53a60a4e3d3b72 100644
--- a/lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
+++ b/lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
@@ -42,13 +42,20 @@ def setup(self, registers):
 substrs=["stopped", "stop reason = breakpoint"],
 )
 
-def check_tls_reg(self, registers):
-self.setup(registers)
-
+def check_registers(self, registers, values):
 regs = self.thread().GetSelectedFrame().GetRegisters()
 tls_regs = regs.GetFirstValueByName("Thread Local Storage Registers")
 self.assertTrue(tls_regs.IsValid(), "No TLS registers found.")
 
+for register in registers:
+tls_reg = tls_regs.GetChildMemberWithName(register)
+self.assertTrue(tls_reg.IsValid(), "{} register not found.".format(
+register))
+self.assertEqual(tls_reg.GetValueAsUnsigned(), values[register])
+
+def check_tls_reg(self, registers):
+self.setup(registers)
+
 # Since we can't predict what the value will be, the program has set
 # a target value for us to find.
 initial_values = {
@@ -56,11 +63,12 @@ def check_tls_reg(self, registers):
 "tpidr2": 0x8877665544332211,
 }
 
-for register in registers:
-tls_reg = tls_regs.GetChildMemberWithName(register)
-self.assertTrue(tls_reg.IsValid(), "{} register not found.".format(
-register))
-self.assertEqual(tls_reg.GetValueAsUnsigned(), 
initial_values[register])
+self.check_registers(registers, initial_values)
+
+# Their values should be restored if an expression modifies them.
+self.runCmd("expression expr_func()")
+
+self.check_registers(registers, initial_values)
 
 set_values = {
 "tpidr": 0x,
@@ -95,7 +103,7 @@ def test_tls_no_sme(self):
 @skipUnlessPlatform(["linux"])
 def test_tls_sme(self):
 if not self.isAArch64SME():
-self.skipTest("SME must present.")
+self.skipTest("SME must be present.")
 
 self.check_tls_reg(["tpidr", "tpidr2"])
 

diff  --git a/lldb/test/API/linux/aarch64/tls_registers/main.c 
b/lldb/test/API/linux/aarch64/tls_registers/main.c
index bd8d7b4d1b6

[Lldb-commits] [PATCH] D156512: [lldb][AArch64] Save/restore TLS registers around expressions

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6239227172cd: [lldb][AArch64] Save/restore TLS registers 
around expressions (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156512

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
  lldb/test/API/linux/aarch64/tls_registers/main.c

Index: lldb/test/API/linux/aarch64/tls_registers/main.c
===
--- lldb/test/API/linux/aarch64/tls_registers/main.c
+++ lldb/test/API/linux/aarch64/tls_registers/main.c
@@ -22,8 +22,18 @@
   __asm__ volatile("msr S3_3_C13_C0_5, %0" ::"r"(value));
 }
 
+bool use_tpidr2 = false;
+const uint64_t tpidr_pattern = 0x1122334455667788;
+const uint64_t tpidr2_pattern = 0x8877665544332211;
+
+void expr_func() {
+  set_tpidr(~tpidr_pattern);
+  if (use_tpidr2)
+set_tpidr2(~tpidr2_pattern);
+}
+
 int main(int argc, char *argv[]) {
-  bool use_tpidr2 = argc > 1;
+  use_tpidr2 = argc > 1;
 
   uint64_t original_tpidr = get_tpidr();
   // Accessing this on a core without it produces SIGILL. Only do this if
@@ -32,10 +42,8 @@
   if (use_tpidr2)
 original_tpidr2 = get_tpidr2();
 
-  uint64_t tpidr_pattern = 0x1122334455667788;
   set_tpidr(tpidr_pattern);
 
-  uint64_t tpidr2_pattern = 0x8877665544332211;
   if (use_tpidr2)
 set_tpidr2(tpidr2_pattern);
 
Index: lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
===
--- lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
+++ lldb/test/API/linux/aarch64/tls_registers/TestAArch64LinuxTLSRegisters.py
@@ -42,13 +42,20 @@
 substrs=["stopped", "stop reason = breakpoint"],
 )
 
-def check_tls_reg(self, registers):
-self.setup(registers)
-
+def check_registers(self, registers, values):
 regs = self.thread().GetSelectedFrame().GetRegisters()
 tls_regs = regs.GetFirstValueByName("Thread Local Storage Registers")
 self.assertTrue(tls_regs.IsValid(), "No TLS registers found.")
 
+for register in registers:
+tls_reg = tls_regs.GetChildMemberWithName(register)
+self.assertTrue(tls_reg.IsValid(), "{} register not found.".format(
+register))
+self.assertEqual(tls_reg.GetValueAsUnsigned(), values[register])
+
+def check_tls_reg(self, registers):
+self.setup(registers)
+
 # Since we can't predict what the value will be, the program has set
 # a target value for us to find.
 initial_values = {
@@ -56,11 +63,12 @@
 "tpidr2": 0x8877665544332211,
 }
 
-for register in registers:
-tls_reg = tls_regs.GetChildMemberWithName(register)
-self.assertTrue(tls_reg.IsValid(), "{} register not found.".format(
-register))
-self.assertEqual(tls_reg.GetValueAsUnsigned(), initial_values[register])
+self.check_registers(registers, initial_values)
+
+# Their values should be restored if an expression modifies them.
+self.runCmd("expression expr_func()")
+
+self.check_registers(registers, initial_values)
 
 set_values = {
 "tpidr": 0x,
@@ -95,7 +103,7 @@
 @skipUnlessPlatform(["linux"])
 def test_tls_sme(self):
 if not self.isAArch64SME():
-self.skipTest("SME must present.")
+self.skipTest("SME must be present.")
 
 self.check_tls_reg(["tpidr", "tpidr2"])
 
Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -559,8 +559,10 @@
 dst += GetFPRSize();
   }
 
-  if (GetRegisterInfo().IsMTEEnabled())
+  if (GetRegisterInfo().IsMTEEnabled()) {
 ::memcpy(dst, GetMTEControl(), GetMTEControlSize());
+dst += GetMTEControlSize();
+  }
 
   ::memcpy(dst, GetTLSBuffer(), GetTLSBufferSize());
 
@@ -671,8 +673,17 @@
 ::memcpy(GetMTEControl(), src, GetMTEControlSize());
 m_mte_ctrl_is_valid = true;
 error = WriteMTEControl();
+if (error.Fail())
+  return error;
+src += GetMTEControlSize();
   }
 
+  // There is always a TLS set. It changes size based on system properties, it's
+  // not something an expression can change.
+  ::memcpy(GetTLSBuffer(), src, GetTLSBufferSize());
+  m_tls_is_valid = true;
+  error = WriteTLS();
+
   return error;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinf

[Lldb-commits] [PATCH] D156997: [LLDB][CMake][NFC] Remove unused LLDB_LINKER_SUPPORTS_GROUPS variable

[J. Ryan Stinnett via Phabricator via lldb-commits]

jryans created this revision.
jryans added a reviewer: JDevlieghere.
Herald added a subscriber: ekilmer.
Herald added a project: All.
jryans added a project: LLDB.
jryans published this revision for review.
Herald added a subscriber: lldb-commits.

The `LLDB_LINKER_SUPPORTS_GROUPS` CMake variable was added back in 2019 as part
of https://reviews.llvm.org/D71306, but it appears to have been unused even
then. This removes the unused variable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156997

Files:
  lldb/cmake/modules/LLDBConfig.cmake


Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -17,12 +17,6 @@
 "`CMakeFiles'. Please delete them.")
 endif()
 
-set(LLDB_LINKER_SUPPORTS_GROUPS OFF)
-if (LLVM_COMPILER_IS_GCC_COMPATIBLE AND NOT "${CMAKE_SYSTEM_NAME}" MATCHES 
"Darwin")
-  # The Darwin linker doesn't understand --start-group/--end-group.
-  set(LLDB_LINKER_SUPPORTS_GROUPS ON)
-endif()
-
 macro(add_optional_dependency variable description package found)
   cmake_parse_arguments(ARG
 "QUIET"


Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -17,12 +17,6 @@
 "`CMakeFiles'. Please delete them.")
 endif()
 
-set(LLDB_LINKER_SUPPORTS_GROUPS OFF)
-if (LLVM_COMPILER_IS_GCC_COMPATIBLE AND NOT "${CMAKE_SYSTEM_NAME}" MATCHES "Darwin")
-  # The Darwin linker doesn't understand --start-group/--end-group.
-  set(LLDB_LINKER_SUPPORTS_GROUPS ON)
-endif()
-
 macro(add_optional_dependency variable description package found)
   cmake_parse_arguments(ARG
 "QUIET"
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157000: [lldb][AArch64] Check SIMD save/restore in SVE SIMD test

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: ctetreau, kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

While doing some refactoring I forgot to carry over the copying in of
SIMD data in normal mode, but no tests failed.

Turns out, it's very easy for us to get the restore wrong because
even if you forget the memcopy, setting the buffer to valid may
just read the data you had before the expression evaluation.

So I've extended the SVE SIMD testing (which includes the plain SIMD mode)
to check expression save/restore. This is the only test that fails
if you forget to do `m_fpu_is_valid = true` so I take from that, that
prior to this it wasn't tested at all.

As a bonus, we now have coverage of the same thing for SVE and SSVE modes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157000

Files:
  
lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
  lldb/test/API/commands/register/register/aarch64_sve_simd_registers/main.c

Index: lldb/test/API/commands/register/register/aarch64_sve_simd_registers/main.c
===
--- lldb/test/API/commands/register/register/aarch64_sve_simd_registers/main.c
+++ lldb/test/API/commands/register/register/aarch64_sve_simd_registers/main.c
@@ -40,6 +40,45 @@
   WRITE_SIMD(31);
 }
 
+void write_simd_regs_expr() {
+#define WRITE_SIMD(NUM)\
+  asm volatile("MOV v" #NUM ".d[0], %0\n\t"\
+   "MOV v" #NUM ".d[1], %0\n\t" ::"r"(NUM + 1))
+
+  WRITE_SIMD(0);
+  WRITE_SIMD(1);
+  WRITE_SIMD(2);
+  WRITE_SIMD(3);
+  WRITE_SIMD(4);
+  WRITE_SIMD(5);
+  WRITE_SIMD(6);
+  WRITE_SIMD(7);
+  WRITE_SIMD(8);
+  WRITE_SIMD(9);
+  WRITE_SIMD(10);
+  WRITE_SIMD(11);
+  WRITE_SIMD(12);
+  WRITE_SIMD(13);
+  WRITE_SIMD(14);
+  WRITE_SIMD(15);
+  WRITE_SIMD(16);
+  WRITE_SIMD(17);
+  WRITE_SIMD(18);
+  WRITE_SIMD(19);
+  WRITE_SIMD(20);
+  WRITE_SIMD(21);
+  WRITE_SIMD(22);
+  WRITE_SIMD(23);
+  WRITE_SIMD(24);
+  WRITE_SIMD(25);
+  WRITE_SIMD(26);
+  WRITE_SIMD(27);
+  WRITE_SIMD(28);
+  WRITE_SIMD(29);
+  WRITE_SIMD(30);
+  WRITE_SIMD(31);
+}
+
 unsigned verify_simd_regs() {
   uint64_t got_low = 0;
   uint64_t got_high = 0;
Index: lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
===
--- lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
+++ lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
@@ -1,6 +1,6 @@
 """
-Test that LLDB correctly reads and writes AArch64 SIMD registers in SVE,
-streaming SVE and normal SIMD modes.
+Test that LLDB correctly reads and writes and restores AArch64 SIMD registers
+in SVE, streaming SVE and normal SIMD modes.
 
 There are a few operating modes and we use different strategies for each:
 * Without SVE, in SIMD mode - read the SIMD regset.
@@ -46,6 +46,13 @@
 pad = " ".join(["0x00"] * 7)
 return "{{0x{:02x} {} 0x{:02x} {}}}".format(n, pad, n, pad)
 
+def check_simd_values(self, value_offset):
+# These are 128 bit registers, so getting them from the API as unsigned
+# values doesn't work. Check the command output instead.
+for i in range(32):
+self.expect("register read v{}".format(i),
+substrs=[self.make_simd_value(i+value_offset)])
+
 def sve_simd_registers_impl(self, mode):
 self.skip_if_needed(mode)
 
@@ -66,11 +73,9 @@
 substrs=["stop reason = breakpoint 1."],
 )
 
-# These are 128 bit registers, so getting them from the API as unsigned
-# values doesn't work. Check the command output instead.
-for i in range(32):
-self.expect("register read v{}".format(i),
-substrs=[self.make_simd_value(i)])
+self.check_simd_values(0)
+self.runCmd("expression write_simd_regs_expr()")
+self.check_simd_values(0)
 
 # Write a new set of values. The kernel will move the program back to
 # non-streaming mode here.
@@ -79,9 +84,7 @@
 i, self.make_simd_value(i+1)))
 
 # Should be visible within lldb.
-for i in range(32):
-self.expect("register read v{}".format(i),
-substrs=[self.make_simd_value(i+1)])
+self.check_simd_values(1)
 
 # The program should agree with lldb.
 self.expect("continue", substrs=["exited with status = 0"])
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156687: [lldb][AArch64] Add kind marker to ReadAll/WriteALLRegisterValues data

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 546842.
DavidSpickett added a comment.

Use uint8_t for kind, fix some places doing sizeof uint32_t not of the kind 
type.

Put back memcopy of fpr registers, which is now tested by the parent patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156687

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

Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -499,6 +499,30 @@
   return Status("Failed to write register value");
 }
 
+enum SavedRegistersKind : uint8_t {
+  GPR,
+  SVE, // Used for SVE and SSVE.
+  FPR,
+  MTE,
+  TLS,
+};
+
+static uint8_t *AddSavedRegistersKind(uint8_t *dst, SavedRegistersKind kind) {
+  *(reinterpret_cast(dst)) = kind;
+  return dst + sizeof(SavedRegistersKind);
+}
+
+static uint8_t *AddSavedRegistersData(uint8_t *dst, void *src, size_t size) {
+  ::memcpy(dst, src, size);
+  return dst + size;
+}
+
+static uint8_t *AddSavedRegisters(uint8_t *dst, enum SavedRegistersKind kind,
+  void *src, size_t size) {
+  dst = AddSavedRegistersKind(dst, kind);
+  return AddSavedRegistersData(dst, src, size);
+}
+
 Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
 lldb::WritableDataBufferSP &data_sp) {
   // AArch64 register data must contain GPRs and either FPR or SVE registers.
@@ -512,33 +536,33 @@
   // corresponds to register sets enabled by current register context.
 
   Status error;
-  uint32_t reg_data_byte_size = GetGPRBufferSize();
+  uint32_t reg_data_byte_size = sizeof(SavedRegistersKind) + GetGPRBufferSize();
   error = ReadGPR();
   if (error.Fail())
 return error;
 
   // If SVE is enabled we need not copy FPR separately.
   if (GetRegisterInfo().IsSVEEnabled() || GetRegisterInfo().IsSSVEEnabled()) {
-reg_data_byte_size += GetSVEBufferSize();
-// Also store the current SVE mode.
-reg_data_byte_size += sizeof(uint32_t);
+// Store mode and register data.
+reg_data_byte_size +=
+sizeof(SavedRegistersKind) + sizeof(m_sve_state) + GetSVEBufferSize();
 error = ReadAllSVE();
   } else {
-reg_data_byte_size += GetFPRSize();
+reg_data_byte_size += sizeof(SavedRegistersKind) + GetFPRSize();
 error = ReadFPR();
   }
   if (error.Fail())
 return error;
 
   if (GetRegisterInfo().IsMTEEnabled()) {
-reg_data_byte_size += GetMTEControlSize();
+reg_data_byte_size += sizeof(SavedRegistersKind) + GetMTEControlSize();
 error = ReadMTEControl();
 if (error.Fail())
   return error;
   }
 
   // tpidr is always present but tpidr2 depends on SME.
-  reg_data_byte_size += GetTLSBufferSize();
+  reg_data_byte_size += sizeof(SavedRegistersKind) + GetTLSBufferSize();
   error = ReadTLS();
   if (error.Fail())
 return error;
@@ -546,25 +570,26 @@
   data_sp.reset(new DataBufferHeap(reg_data_byte_size, 0));
   uint8_t *dst = data_sp->GetBytes();
 
-  ::memcpy(dst, GetGPRBuffer(), GetGPRBufferSize());
-  dst += GetGPRBufferSize();
+  dst = AddSavedRegisters(dst, SavedRegistersKind::GPR, GetGPRBuffer(),
+  GetGPRBufferSize());
 
   if (GetRegisterInfo().IsSVEEnabled() || GetRegisterInfo().IsSSVEEnabled()) {
-*dst = static_cast(m_sve_state);
+dst = AddSavedRegistersKind(dst, SavedRegistersKind::SVE);
+*(reinterpret_cast(dst)) = m_sve_state;
 dst += sizeof(m_sve_state);
-::memcpy(dst, GetSVEBuffer(), GetSVEBufferSize());
-dst += GetSVEBufferSize();
+dst = AddSavedRegistersData(dst, GetSVEBuffer(), GetSVEBufferSize());
   } else {
-::memcpy(dst, GetFPRBuffer(), GetFPRSize());
-dst += GetFPRSize();
+dst = AddSavedRegisters(dst, SavedRegistersKind::FPR, GetFPRBuffer(),
+GetFPRSize());
   }
 
   if (GetRegisterInfo().IsMTEEnabled()) {
-::memcpy(dst, GetMTEControl(), GetMTEControlSize());
-dst += GetMTEControlSize();
+dst = AddSavedRegisters(dst, SavedRegistersKind::MTE, GetMTEControl(),
+GetMTEControlSize());
   }
 
-  ::memcpy(dst, GetTLSBuffer(), GetTLSBufferSize());
+  dst = AddSavedRegisters(dst, SavedRegistersKind::TLS, GetTLSBuffer(),
+  GetTLSBufferSize());
 
   return error;
 }
@@ -599,7 +624,8 @@
 return error;
   }
 
-  uint64_t reg_data_min_size = GetGPRBufferSize() + GetFPRSize();
+  uint64_t reg_data_min_size =
+  GetGPRBufferSize() + GetFPRSize() + 2 * (sizeof(SavedRegistersKind));
   if (data_sp->GetByteSize() < reg_data_min_size) {
 error.SetErrorStringWithFormat(
 "NativeRegisterContextLinux_arm64::%s data_sp contained insufficient "
@@ -608,82 +634,84 @@
 return error;
   }
 
-  // Register 

[Lldb-commits] [PATCH] D156687: [lldb][AArch64] Add kind marker to ReadAll/WriteALLRegisterValues data

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Oh, and fixed the FIXME now that the TLS fixes have landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156687

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

[Lldb-commits] [PATCH] D157000: [lldb][AArch64] Check SIMD save/restore in SVE SIMD test

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

MTE control needs a test too, I'm working on that as its own change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157000

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

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/CodeOwners.rst:7-8
+particular part of LLDB are reviewed, either by themself or by someone else.
+They are also the gatekeepers for their part of LLDB, with the final word on
+what goes in or not.
+

DavidSpickett wrote:
> This could be taken to mean every review must have approval from a code 
> owner, on top of whatever other review has been done. Is that the intent? 
> Someone coming from a project with strong maintainer rules (e.g. GDB, so I 
> gather) may take it that way.
I copied this from the Clang `CodeOwners.rst` with the aim of being consistent, 
but I'm happy to tweak it. We  could qualify the last sentence with something 
like "when consensus cannot be reached" or if we think "gatekeeper" is too 
strong of a work maybe we can use "tie-breaker", though I like that the former 
implies a sense of duty. Happy to take suggestions!



Comment at: lldb/CodeOwners.rst:214
+
+
+Tools

DavidSpickett wrote:
> Is the extra space an RST thing, seems placed randomly.
Nope that's just me: I'll clear that up 👍


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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/CodeOwners.rst:7-8
+particular part of LLDB are reviewed, either by themself or by someone else.
+They are also the gatekeepers for their part of LLDB, with the final word on
+what goes in or not.
+

JDevlieghere wrote:
> DavidSpickett wrote:
> > This could be taken to mean every review must have approval from a code 
> > owner, on top of whatever other review has been done. Is that the intent? 
> > Someone coming from a project with strong maintainer rules (e.g. GDB, so I 
> > gather) may take it that way.
> I copied this from the Clang `CodeOwners.rst` with the aim of being 
> consistent, but I'm happy to tweak it. We  could qualify the last sentence 
> with something like "when consensus cannot be reached" or if we think 
> "gatekeeper" is too strong of a work maybe we can use "tie-breaker", though I 
> like that the former implies a sense of duty. Happy to take suggestions!
My understanding was that llvm in general didn't have this hard requirement for 
an owner to acknowledge every review.

So yeah:
"They are also the gatekeepers for their part of LLDB, with the final word on 
what goes in or not when consensus cannot be reached."

Sounds good to me.


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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D156977: [lldb][lldb-vscode] Fix nullptr dereference when JSON is not an object

[walter erquinigo via Phabricator via lldb-commits]

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

thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156977

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

[Lldb-commits] [PATCH] D156979: [lldb][lldb-vscode] Pretty print JSON to log files

[walter erquinigo via Phabricator via lldb-commits]

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

nice improvement for sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156979

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

[Lldb-commits] [lldb] bdeb35b - [lldb][lldb-vscode] Fix nullptr dereference when JSON is not an object

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-08-03T14:56:24Z
New Revision: bdeb35bda4381d3b416a92d797713b4b5a6a6c97

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

LOG: [lldb][lldb-vscode] Fix nullptr dereference when JSON is not an object

Reviewed By: wallace

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

Added: 


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

Removed: 




diff  --git a/lldb/tools/lldb-vscode/VSCode.cpp 
b/lldb/tools/lldb-vscode/VSCode.cpp
index b6bf67c7bcfcfc..cad67c4273dd53 100644
--- a/lldb/tools/lldb-vscode/VSCode.cpp
+++ b/lldb/tools/lldb-vscode/VSCode.cpp
@@ -523,12 +523,13 @@ PacketStatus VSCode::GetNextObject(llvm::json::Object 
&object) {
 }
 return PacketStatus::JSONMalformed;
   }
-  object = *json_value->getAsObject();
-  if (!json_value->getAsObject()) {
+  llvm::json::Object *object_ptr = json_value->getAsObject();
+  if (!object_ptr) {
 if (log)
   *log << "error: json packet isn't a object" << std::endl;
 return PacketStatus::JSONNotObject;
   }
+  object = *object_ptr;
   return PacketStatus::Success;
 }
 



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

[Lldb-commits] [PATCH] D156977: [lldb][lldb-vscode] Fix nullptr dereference when JSON is not an object

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbdeb35bda438: [lldb][lldb-vscode] Fix nullptr dereference 
when JSON is not an object (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156977

Files:
  lldb/tools/lldb-vscode/VSCode.cpp


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -523,12 +523,13 @@
 }
 return PacketStatus::JSONMalformed;
   }
-  object = *json_value->getAsObject();
-  if (!json_value->getAsObject()) {
+  llvm::json::Object *object_ptr = json_value->getAsObject();
+  if (!object_ptr) {
 if (log)
   *log << "error: json packet isn't a object" << std::endl;
 return PacketStatus::JSONNotObject;
   }
+  object = *object_ptr;
   return PacketStatus::Success;
 }
 


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -523,12 +523,13 @@
 }
 return PacketStatus::JSONMalformed;
   }
-  object = *json_value->getAsObject();
-  if (!json_value->getAsObject()) {
+  llvm::json::Object *object_ptr = json_value->getAsObject();
+  if (!object_ptr) {
 if (log)
   *log << "error: json packet isn't a object" << std::endl;
 return PacketStatus::JSONNotObject;
   }
+  object = *object_ptr;
   return PacketStatus::Success;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 165f45a - [lldb][lldb-vscode] Pretty print JSON to log files

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-08-03T15:09:12Z
New Revision: 165f45a877742a74988d63f36aee635c8e0a47da

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

LOG: [lldb][lldb-vscode] Pretty print JSON to log files

This makes anlysing test failures much more easy.

For SendJSON this is simple, just use llvm::format instead.

For GetNextObject/ReadJSON it's a bit more tricky.
* Print the "Content-Length:" line in ReadJSON, but not the json.
* Back in GetNextObject, if the JSON doesn't parse, it'll be
  printed as a normal string along with an error message.
* If we didn't error before, we have a JSON value so we pretty print it.
* Finally, if it isn't an object we'll log an error for that,
  not including the JSON.

Before:
```
<--
Content-Length: 81

{"command":"disconnect","request_seq":5,"seq":0,"success":true,"type":"response"}
```

After:
```
<--
Content-Length: 81

{
  "command": "disconnect",
  "request_seq": 5,
  "seq": 0,
  "success": true,
  "type": "response"
}
```

There appear to be some responses that include strings that are themselves JSON,
and this won't pretty print those but I think it's still worth doing.

Reviewed By: wallace

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

Added: 


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

Removed: 




diff  --git a/lldb/tools/lldb-vscode/VSCode.cpp 
b/lldb/tools/lldb-vscode/VSCode.cpp
index cad67c4273dd53..85e1f4b4ac06e4 100644
--- a/lldb/tools/lldb-vscode/VSCode.cpp
+++ b/lldb/tools/lldb-vscode/VSCode.cpp
@@ -89,12 +89,6 @@ void VSCode::SendJSON(const std::string &json_str) {
   output.write_full(llvm::utostr(json_str.size()));
   output.write_full("\r\n\r\n");
   output.write_full(json_str);
-
-  if (log) {
-*log << "<-- " << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << json_str << std::endl;
-  }
 }
 
 // Serialize the JSON value into a string and send the JSON packet to
@@ -105,7 +99,14 @@ void VSCode::SendJSON(const llvm::json::Value &json) {
   strm << json;
   static std::mutex mutex;
   std::lock_guard locker(mutex);
-  SendJSON(strm.str());
+  std::string json_str = strm.str();
+  SendJSON(json_str);
+
+  if (log) {
+*log << "<-- " << std::endl
+ << "Content-Length: " << json_str.size() << "\r\n\r\n"
+ << llvm::formatv("{0:2}", json).str() << std::endl;
+  }
 }
 
 // Read a JSON packet from the "in" stream.
@@ -129,11 +130,8 @@ std::string VSCode::ReadJSON() {
   if (!input.read_full(log.get(), length, json_str))
 return json_str;
 
-  if (log) {
-*log << "--> " << std::endl
- << "Content-Length: " << length << "\r\n\r\n"
- << json_str << std::endl;
-  }
+  if (log)
+*log << "--> " << std::endl << "Content-Length: " << length << "\r\n\r\n";
 
   return json_str;
 }
@@ -523,6 +521,11 @@ PacketStatus VSCode::GetNextObject(llvm::json::Object 
&object) {
 }
 return PacketStatus::JSONMalformed;
   }
+
+  if (log) {
+*log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
+  }
+
   llvm::json::Object *object_ptr = json_value->getAsObject();
   if (!object_ptr) {
 if (log)



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

[Lldb-commits] [PATCH] D156979: [lldb][lldb-vscode] Pretty print JSON to log files

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG165f45a87774: [lldb][lldb-vscode] Pretty print JSON to log 
files (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156979

Files:
  lldb/tools/lldb-vscode/VSCode.cpp


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -89,12 +89,6 @@
   output.write_full(llvm::utostr(json_str.size()));
   output.write_full("\r\n\r\n");
   output.write_full(json_str);
-
-  if (log) {
-*log << "<-- " << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << json_str << std::endl;
-  }
 }
 
 // Serialize the JSON value into a string and send the JSON packet to
@@ -105,7 +99,14 @@
   strm << json;
   static std::mutex mutex;
   std::lock_guard locker(mutex);
-  SendJSON(strm.str());
+  std::string json_str = strm.str();
+  SendJSON(json_str);
+
+  if (log) {
+*log << "<-- " << std::endl
+ << "Content-Length: " << json_str.size() << "\r\n\r\n"
+ << llvm::formatv("{0:2}", json).str() << std::endl;
+  }
 }
 
 // Read a JSON packet from the "in" stream.
@@ -129,11 +130,8 @@
   if (!input.read_full(log.get(), length, json_str))
 return json_str;
 
-  if (log) {
-*log << "--> " << std::endl
- << "Content-Length: " << length << "\r\n\r\n"
- << json_str << std::endl;
-  }
+  if (log)
+*log << "--> " << std::endl << "Content-Length: " << length << "\r\n\r\n";
 
   return json_str;
 }
@@ -523,6 +521,11 @@
 }
 return PacketStatus::JSONMalformed;
   }
+
+  if (log) {
+*log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
+  }
+
   llvm::json::Object *object_ptr = json_value->getAsObject();
   if (!object_ptr) {
 if (log)


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -89,12 +89,6 @@
   output.write_full(llvm::utostr(json_str.size()));
   output.write_full("\r\n\r\n");
   output.write_full(json_str);
-
-  if (log) {
-*log << "<-- " << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << json_str << std::endl;
-  }
 }
 
 // Serialize the JSON value into a string and send the JSON packet to
@@ -105,7 +99,14 @@
   strm << json;
   static std::mutex mutex;
   std::lock_guard locker(mutex);
-  SendJSON(strm.str());
+  std::string json_str = strm.str();
+  SendJSON(json_str);
+
+  if (log) {
+*log << "<-- " << std::endl
+ << "Content-Length: " << json_str.size() << "\r\n\r\n"
+ << llvm::formatv("{0:2}", json).str() << std::endl;
+  }
 }
 
 // Read a JSON packet from the "in" stream.
@@ -129,11 +130,8 @@
   if (!input.read_full(log.get(), length, json_str))
 return json_str;
 
-  if (log) {
-*log << "--> " << std::endl
- << "Content-Length: " << length << "\r\n\r\n"
- << json_str << std::endl;
-  }
+  if (log)
+*log << "--> " << std::endl << "Content-Length: " << length << "\r\n\r\n";
 
   return json_str;
 }
@@ -523,6 +521,11 @@
 }
 return PacketStatus::JSONMalformed;
   }
+
+  if (log) {
+*log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
+  }
+
   llvm::json::Object *object_ptr = json_value->getAsObject();
   if (!object_ptr) {
 if (log)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156270: [lldb][NFCI] Change logic to find clang resource dir in standalone builds

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

mgorny added subscribers: paperchalice, tstellar, mgorny.
mgorny added a comment.

I'm not sure if it was really intended but the new API is kinda horrible, at 
least for us in Gentoo.

Our install prefix is `/usr/lib/llvm/NN`, whereas clang resource dir is 
`/usr/lib/clang/NN`.

If I don't override `CLANG_RESOURCE_DIR`, it infers the wrong directory: 
`/usr/lib/llvm/18/lib64/cmake/clang/../../..//lib64/clang/18`.

To get the correct directory, I need to pass 
`-DCLANG_RESOURCE_DIR="../../../clang/${LLVM_MAJOR}"` which is absolutely 
counterintuitive (the path gets appended to 
`/usr/lib/llvm/18/lib64/cmake/clang/../../../bin`).

Not saying it's not workable but I'd hardly call that an improvement over the 
previous API.

CC @tstellar, @paperchalice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156270

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

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 546897.
JDevlieghere marked 3 inline comments as done.
JDevlieghere added a comment.

- Update wording
- Remove double newline


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

https://reviews.llvm.org/D156949

Files:
  lldb/CODE_OWNERS.txt
  lldb/CodeOwners.rst

Index: lldb/CodeOwners.rst
===
--- /dev/null
+++ lldb/CodeOwners.rst
@@ -0,0 +1,243 @@
+
+LLDB Code Owners
+
+
+This file is a list of the people responsible for ensuring that patches for a
+particular part of LLDB are reviewed, either by themself or by someone else.
+They are also the gatekeepers for their part of LLDB, with the final word on
+what goes in or not when consensus cannot be reached.
+
+.. contents::
+   :depth: 2
+   :local:
+
+Current Code Owners
+===
+The following people are the active code owners for the project. Please reach
+out to them for code reviews, questions about their area of expertise, or other
+assistance.
+
+All parts of LLDB not covered by someone else
+--
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+Components
+--
+These code owners are responsible for particular high-level components within
+LLDB.
+
+ABI
+~~~
+| Jason Molenda
+| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub), jasonmolenda (Discourse), jasonmolenda (Discord)
+
+| David Spickett
+| david.spickett\@linaro.org (email), DavidSpickett (Phabricator), DavidSpickett (GitHub), DavidSpickett (Discourse), davidspickett (Discord)
+
+
+Breakpoint
+~~
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+CMake & Build System
+
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+| Alex Langford
+| alangford\@apple.com (email), bulbazord (Phabricator), bulbazord (GitHub), bulbazord (Discourse), bulba_zord (Discord)
+
+Commands
+
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Expression Parser
+~
+| Michael Buch
+| michaelbuch12\@gmail.com (email), Michael137 (Phabricator), Michael137 (GitHub), Michael137 (Discourse)
+
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Interpreter
+~~~
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+| Greg Clayton
+| gclayton\@fb.com (email), clayborg (Phabricator), clayborg (GitHub), clayborg (Discourse)
+
+
+Lua
+~~~
+| Jonas Delvieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+Python
+~~
+| Med Ismail Bennani
+| ismail\@bennani.ma (email), mib (Phabricator), medismailben (GitHub), mib (Discourse), mib#8727 (Discord)
+
+Target/Process Control
+~~
+| Med Ismail Bennani
+| ismail\@bennani.ma (email), mib (Phabricator), medismailben (GitHub), mib (Discourse), mib#8727 (Discord)
+
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Test Suite
+~~
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+| Pavel Labath
+| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub), labath (Discourse)
+
+Trace
+~
+| Walter Erquinigo
+| a20012251\@gmail.com (email), wallace (Phabricator), walter-erquinigo (GitHub), wallace (Discourse), werquinigo (Discord)
+
+Unwinding
+~
+| Jason Molenda
+| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub), jasonmolenda (Discourse), jasonmolenda (Discord)
+
+Utility
+~~~
+| Pavel Labath
+| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub), labath (Discourse)
+
+ValueObject
+~~~
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Watchpoints
+~~~
+| Jason Molenda
+| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub), jasonmolenda (Discourse), jasonmolenda (Discord)
+
+File Formats
+
+The following people are responsible for decisions involving file and debug
+info formats.
+
+(PE)COFF
+
+| Saleem Abdulrasool
+| compnerd\@compnerd.org (email), compnerd (Phabricator), compnerd (GitHub), compnerd (Discourse), compnerd (Discord)
+
+Breakpad
+
+| Pavel Labath
+| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub), labath (Discourse)
+
+CTF
+~~~
+| Jonas 

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/CodeOwners.rst:7-8
+particular part of LLDB are reviewed, either by themself or by someone else.
+They are also the gatekeepers for their part of LLDB, with the final word on
+what goes in or not.
+

DavidSpickett wrote:
> JDevlieghere wrote:
> > DavidSpickett wrote:
> > > This could be taken to mean every review must have approval from a code 
> > > owner, on top of whatever other review has been done. Is that the intent? 
> > > Someone coming from a project with strong maintainer rules (e.g. GDB, so 
> > > I gather) may take it that way.
> > I copied this from the Clang `CodeOwners.rst` with the aim of being 
> > consistent, but I'm happy to tweak it. We  could qualify the last sentence 
> > with something like "when consensus cannot be reached" or if we think 
> > "gatekeeper" is too strong of a work maybe we can use "tie-breaker", though 
> > I like that the former implies a sense of duty. Happy to take suggestions!
> My understanding was that llvm in general didn't have this hard requirement 
> for an owner to acknowledge every review.
> 
> So yeah:
> "They are also the gatekeepers for their part of LLDB, with the final word on 
> what goes in or not when consensus cannot be reached."
> 
> Sounds good to me.
> My understanding was that llvm in general didn't have this hard requirement 
> for an owner to acknowledge every review.

Yup, that's my understanding as well!



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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D156817: [lldb][windows] _wsopen_s does not accept bits other than `_S_IREAD | _S_IWRITE`

[yshui via Phabricator via lldb-commits]

yshui added a comment.

In D156817#4553045 , @DavidSpickett 
wrote:

> What exactly are the other bits in the mode here, are we losing something 
> important potentially? I guess it can't be that important if Windows rejects 
> them.

I don't think so, group and other permission bits don't really make sense on 
Windows anyways, unless we use ACL or something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156817

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

[Lldb-commits] [PATCH] D156997: [LLDB][CMake][NFC] Remove unused LLDB_LINKER_SUPPORTS_GROUPS variable

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156997

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

[Lldb-commits] [PATCH] D156270: [lldb][NFCI] Change logic to find clang resource dir in standalone builds

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D156270#4557902 , @mgorny wrote:

> I'm not sure if it was really intended but the new API is kinda horrible, at 
> least for us in Gentoo.
>
> Our install prefix is `/usr/lib/llvm/NN`, whereas clang resource dir is 
> `/usr/lib/clang/NN`.
>
> If I don't override `CLANG_RESOURCE_DIR`, it infers the wrong directory:
> `/usr/lib/llvm/18/lib64/cmake/clang/../../..//lib64/clang/18`.
>
> To get the correct directory, I need to pass:
> `-DCLANG_RESOURCE_DIR="../../../clang/${LLVM_MAJOR}"`
> which is absolutely counterintuitive (the path gets appended to:
> `/usr/lib/llvm/18/lib64/cmake/clang/../../../bin`
> ).
>
> Not saying it's not workable but I'd hardly call that an improvement over the 
> previous API.
>
> CC @tstellar, @paperchalice.

To give some context for my change, downstream for swift there are further 
changes that involve finding and/or using the clang resource directory. I 
wanted to take advantage of the new functionality `get_clang_resource_dir` in 
order to replace the bespoke logic that existed before. Downstream, we had some 
additional logic to find it again in some cases (which was frustratingly 
different than what was here before).

If the API is unintuitive or doesn't serve you well, I'm of the opinion that we 
should change the API. It think it would be a lot easier for your use case if 
we could tell CMake the absolute path to the clang resource dir instead of 
having to craft some relative directory that works around the implementation 
instead of with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156270

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

[Lldb-commits] [PATCH] D156997: [LLDB][CMake][NFC] Remove unused LLDB_LINKER_SUPPORTS_GROUPS variable

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

LGTM. I don't see any actual uses of this on llvm.org. If somebody needs it 
they can re-add it and add their use case or they can maintain a patch 
downstream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156997

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

[Lldb-commits] [PATCH] D156817: [lldb][windows] _wsopen_s does not accept bits other than `_S_IREAD | _S_IWRITE`

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

I think this could be tested with an lldb-server test, but it would likely go 
in `lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py` which has 
every test marked as skipped on Windows. For reasons lost to time.

If you feel like trying to re-enable them, great, otherwise I'm ok for such a 
simple fix to go in as is. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156817

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

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[Tanya Lattner via Phabricator via lldb-commits]

tonic added a comment.






Comment at: lldb/CodeOwners.rst:7-8
+particular part of LLDB are reviewed, either by themself or by someone else.
+They are also the gatekeepers for their part of LLDB, with the final word on
+what goes in or not.
+

JDevlieghere wrote:
> DavidSpickett wrote:
> > JDevlieghere wrote:
> > > DavidSpickett wrote:
> > > > This could be taken to mean every review must have approval from a code 
> > > > owner, on top of whatever other review has been done. Is that the 
> > > > intent? Someone coming from a project with strong maintainer rules 
> > > > (e.g. GDB, so I gather) may take it that way.
> > > I copied this from the Clang `CodeOwners.rst` with the aim of being 
> > > consistent, but I'm happy to tweak it. We  could qualify the last 
> > > sentence with something like "when consensus cannot be reached" or if we 
> > > think "gatekeeper" is too strong of a work maybe we can use 
> > > "tie-breaker", though I like that the former implies a sense of duty. 
> > > Happy to take suggestions!
> > My understanding was that llvm in general didn't have this hard requirement 
> > for an owner to acknowledge every review.
> > 
> > So yeah:
> > "They are also the gatekeepers for their part of LLDB, with the final word 
> > on what goes in or not when consensus cannot be reached."
> > 
> > Sounds good to me.
> > My understanding was that llvm in general didn't have this hard requirement 
> > for an owner to acknowledge every review.
> 
> Yup, that's my understanding as well!
> 
I would not put policy regarding code owners in this document. The policy is 
already in the DeveloperPolicy for the project. You could reference back to 
that document if you want. 


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

https://reviews.llvm.org/D156949

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

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Jim Ingham via Phabricator via lldb-commits]

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

Just mutatis mutandis for the Foundation changes.  The current NSIndexSet test 
fails on Sonoma w/o this patch and exercise the different forms, so no new 
tests are required.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157022

Files:
  lldb/source/Plugins/Language/ObjC/Cocoa.cpp


Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -237,7 +237,8 @@
   if (!process_sp)
 return false;
 
-  ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
+  AppleObjCRuntime *runtime = llvm::dyn_cast_or_null(
+  ObjCLanguageRuntime::Get(*process_sp));
 
   if (!runtime)
 return false;
@@ -264,20 +265,61 @@
 
   do {
 if (class_name == "NSIndexSet" || class_name == "NSMutableIndexSet") {
+  // Foundation version 2000 added a bitmask if the index set fit in 64 
bits
+  // and a Tagged Pointer version if the bitmask is small enough to fit in
+  // the tagged pointer payload.  
+  // It also changed the layout (but not the size) of the set descriptor.
+
+  // First check whether this is a tagged pointer.  The bitmask will be in
+  // the payload of the tagged pointer.
+  uint64_t payload;
+  if (runtime->GetFoundationVersion() >= 2000  
+  && descriptor->GetTaggedPointerInfo(nullptr, nullptr, &payload)) {
+count = __builtin_popcountll(payload);
+break;
+  }
+  // The first 32 bits describe the index set in all cases:
   Status error;
   uint32_t mode = process_sp->ReadUnsignedIntegerFromMemory(
-  valobj_addr + ptr_size, 4, 0, error);
+valobj_addr + ptr_size, 4, 0, error);
   if (error.Fail())
 return false;
-  // this means the set is empty - count = 0
-  if ((mode & 1) == 1) {
-count = 0;
-break;
+  // Now check if the index is held in a bitmask in the object:
+  if (runtime->GetFoundationVersion() >= 2000) {
+// The first two bits are "isSingleRange" and "isBitfield".  If this is
+// a bitfield we handle it here, otherwise set mode appropriately and
+// the rest of the treatment is in common.
+if ((mode & 2) == 2) {
+  // This is the bitfield case.  The bitfield is a uint64_t:
+  count = process_sp->ReadUnsignedIntegerFromMemory(
+  valobj_addr + 2 * ptr_size, 8, 0, error);
+  if (error.Fail())
+return false;
+  // The bitfield is a 64 bit uint at the beginning of the data var.
+  uint64_t bitfield = process_sp->ReadUnsignedIntegerFromMemory(
+valobj_addr + 2 * ptr_size, 8, 0, error);
+  if (error.Fail())
+return false;
+  count = __builtin_popcountll(bitfield);
+  break;
+}
+// It wasn't a bitfield, so read the isSingleRange from its new loc:
+if ((mode & 1) == 1)
+  mode = 1; // this means the set only has one range
+else
+  mode = 2; // this means the set has multiple ranges
+  } else {
+// this means the set is empty - count = 0
+if ((mode & 1) == 1) {
+  count = 0;
+  break;
+}
+  
+if ((mode & 2) == 2)
+  mode = 1; // this means the set only has one range
+else
+  mode = 2; // this means the set has multiple ranges
   }
-  if ((mode & 2) == 2)
-mode = 1; // this means the set only has one range
-  else
-mode = 2; // this means the set has multiple ranges
   if (mode == 1) {
 count = process_sp->ReadUnsignedIntegerFromMemory(
 valobj_addr + 3 * ptr_size, ptr_size, 0, error);


Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -237,7 +237,8 @@
   if (!process_sp)
 return false;
 
-  ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
+  AppleObjCRuntime *runtime = llvm::dyn_cast_or_null(
+  ObjCLanguageRuntime::Get(*process_sp));
 
   if (!runtime)
 return false;
@@ -264,20 +265,61 @@
 
   do {
 if (class_name == "NSIndexSet" || class_name == "NSMutableIndexSet") {
+  // Foundation version 2000 added a bitmask if the index set fit in 64 bits
+  // and a Tagged Pointer version if the bitmask is small enough to fit in
+  // the tagged pointer payload.  
+  // It also changed the layout (but not the size) of the set descriptor.
+
+  // First check whether this is a tagged 

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:294
+  // This is the bitfield case.  The bitfield is a uint64_t:
+  count = process_sp->ReadUnsignedIntegerFromMemory(
+  valobj_addr + 2 * ptr_size, 8, 0, error);

Do we want to overwrite the `count` read previously ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

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

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:278
+  && descriptor->GetTaggedPointerInfo(nullptr, nullptr, &payload)) {
+count = __builtin_popcountll(payload);
+break;

I'm pretty sure `__builtin_popcount` and friends are GNU extensions not 
supported by MSVC. You'll probably need to abstract this out with C 
preprocessor macro guards.

Alternatively, I think llvm has its own `popcount` implementation with 
`llvm::popcount` in `include/llvm/ADT/bit.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

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

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 546976.
jingham marked an inline comment as done.
jingham added a comment.

Use llvm::popcount instead of using the builtin directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

Files:
  lldb/source/Plugins/Language/ObjC/Cocoa.cpp


Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -237,7 +237,8 @@
   if (!process_sp)
 return false;
 
-  ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
+  AppleObjCRuntime *runtime = llvm::dyn_cast_or_null(
+  ObjCLanguageRuntime::Get(*process_sp));
 
   if (!runtime)
 return false;
@@ -264,20 +265,61 @@
 
   do {
 if (class_name == "NSIndexSet" || class_name == "NSMutableIndexSet") {
+  // Foundation version 2000 added a bitmask if the index set fit in 64 
bits
+  // and a Tagged Pointer version if the bitmask is small enough to fit in
+  // the tagged pointer payload.  
+  // It also changed the layout (but not the size) of the set descriptor.
+
+  // First check whether this is a tagged pointer.  The bitmask will be in
+  // the payload of the tagged pointer.
+  uint64_t payload;
+  if (runtime->GetFoundationVersion() >= 2000  
+  && descriptor->GetTaggedPointerInfo(nullptr, nullptr, &payload)) {
+count = llvm::popcount(payload);
+break;
+  }
+  // The first 32 bits describe the index set in all cases:
   Status error;
   uint32_t mode = process_sp->ReadUnsignedIntegerFromMemory(
-  valobj_addr + ptr_size, 4, 0, error);
+valobj_addr + ptr_size, 4, 0, error);
   if (error.Fail())
 return false;
-  // this means the set is empty - count = 0
-  if ((mode & 1) == 1) {
-count = 0;
-break;
+  // Now check if the index is held in a bitmask in the object:
+  if (runtime->GetFoundationVersion() >= 2000) {
+// The first two bits are "isSingleRange" and "isBitfield".  If this is
+// a bitfield we handle it here, otherwise set mode appropriately and
+// the rest of the treatment is in common.
+if ((mode & 2) == 2) {
+  // This is the bitfield case.  The bitfield is a uint64_t:
+  count = process_sp->ReadUnsignedIntegerFromMemory(
+  valobj_addr + 2 * ptr_size, 8, 0, error);
+  if (error.Fail())
+return false;
+  // The bitfield is a 64 bit uint at the beginning of the data var.
+  uint64_t bitfield = process_sp->ReadUnsignedIntegerFromMemory(
+valobj_addr + 2 * ptr_size, 8, 0, error);
+  if (error.Fail())
+return false;
+  count = llvm::popcount(bitfield);
+  break;
+}
+// It wasn't a bitfield, so read the isSingleRange from its new loc:
+if ((mode & 1) == 1)
+  mode = 1; // this means the set only has one range
+else
+  mode = 2; // this means the set has multiple ranges
+  } else {
+// this means the set is empty - count = 0
+if ((mode & 1) == 1) {
+  count = 0;
+  break;
+}
+  
+if ((mode & 2) == 2)
+  mode = 1; // this means the set only has one range
+else
+  mode = 2; // this means the set has multiple ranges
   }
-  if ((mode & 2) == 2)
-mode = 1; // this means the set only has one range
-  else
-mode = 2; // this means the set has multiple ranges
   if (mode == 1) {
 count = process_sp->ReadUnsignedIntegerFromMemory(
 valobj_addr + 3 * ptr_size, ptr_size, 0, error);


Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -237,7 +237,8 @@
   if (!process_sp)
 return false;
 
-  ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
+  AppleObjCRuntime *runtime = llvm::dyn_cast_or_null(
+  ObjCLanguageRuntime::Get(*process_sp));
 
   if (!runtime)
 return false;
@@ -264,20 +265,61 @@
 
   do {
 if (class_name == "NSIndexSet" || class_name == "NSMutableIndexSet") {
+  // Foundation version 2000 added a bitmask if the index set fit in 64 bits
+  // and a Tagged Pointer version if the bitmask is small enough to fit in
+  // the tagged pointer payload.  
+  // It also changed the layout (but not the size) of the set descriptor.
+
+  // First check whether this is a tagged pointer.  The bitmask will be in
+  // the payload of the tagged pointer.
+  uint64_t payload;
+  if (runtime->GetFoundationVersion() >= 2000  
+  && descriptor->GetTaggedPointerInfo(nullptr, null

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:278
+  && descriptor->GetTaggedPointerInfo(nullptr, nullptr, &payload)) {
+count = __builtin_popcountll(payload);
+break;

bulbazord wrote:
> I'm pretty sure `__builtin_popcount` and friends are GNU extensions not 
> supported by MSVC. You'll probably need to abstract this out with C 
> preprocessor macro guards.
> 
> Alternatively, I think llvm has its own `popcount` implementation with 
> `llvm::popcount` in `include/llvm/ADT/bit.h`.
Good catch.  The llvm one is just a wrapper for the builtin when available and 
a by-hand version otherwise, so might as well use that one.



Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:294
+  // This is the bitfield case.  The bitfield is a uint64_t:
+  count = process_sp->ReadUnsignedIntegerFromMemory(
+  valobj_addr + 2 * ptr_size, 8, 0, error);

mib wrote:
> Do we want to overwrite the `count` read previously ?
This code was originally written by Sean, who loved to write any code with 
complex logic in the form:

do {
  /// Complex Logic here
} while (0);

so that he could exit the whole construct with a "break".  After we get the bit 
mask value from the tagged pointer we `break` which sends us immediately to 
line 343.  So that setting and this one are on non-intersecting code paths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

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

[Lldb-commits] [lldb] 9a3f0cd - Fix crash in lldb-vscode when missing function name

[Tom Yang via lldb-commits]

Author: Tom Yang
Date: 2023-08-03T12:56:37-07:00
New Revision: 9a3f0cd717f68ccf9e348bce2d76a2372482f4f2

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

LOG: Fix crash in lldb-vscode when missing function name

Summary:
In cases where the PC has no function name, lldb-vscode crashes.

`lldb::SBFrame::GetDisplayFunctionName()` returns a `nullptr`, and when we
attempt to construct an `std::string`, it raises an exception.

Test plan:
This can be observed with creating a test file (credit to @clayborg for
the example):
```
int main() {
  typedef void (*FooCallback)();
  FooCallback foo_callback = (FooCallback)0;
  foo_callback(); // Crash at zero!
  return 0;
}
```
and attempting to debug the file through VSCode.

I add a test case in D156732 which should cover this.

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

Added: 


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

Removed: 




diff  --git a/lldb/tools/lldb-vscode/JSONUtils.cpp 
b/lldb/tools/lldb-vscode/JSONUtils.cpp
index 5ece7c01346b73..6acb07296da3be 100644
--- a/lldb/tools/lldb-vscode/JSONUtils.cpp
+++ b/lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -696,7 +696,11 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame) {
   int64_t frame_id = MakeVSCodeFrameID(frame);
   object.try_emplace("id", frame_id);
 
-  std::string frame_name = frame.GetDisplayFunctionName();
+  // `function_name` can be a nullptr, which throws an error when assigned to 
an
+  // `std::string`.
+  const char *function_name = frame.GetDisplayFunctionName();
+  std::string frame_name =
+  function_name == nullptr ? std::string() : function_name;
   if (frame_name.empty())
 frame_name = "";
   bool is_optimized = frame.GetFunction().GetIsOptimized();



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

[Lldb-commits] [PATCH] D156970: Fix crash in lldb-vscode when missing function name

[Tom Yang via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a3f0cd717f6: Fix crash in lldb-vscode when missing function 
name (authored by zhyty).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156970

Files:
  lldb/tools/lldb-vscode/JSONUtils.cpp


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -696,7 +696,11 @@
   int64_t frame_id = MakeVSCodeFrameID(frame);
   object.try_emplace("id", frame_id);
 
-  std::string frame_name = frame.GetDisplayFunctionName();
+  // `function_name` can be a nullptr, which throws an error when assigned to 
an
+  // `std::string`.
+  const char *function_name = frame.GetDisplayFunctionName();
+  std::string frame_name =
+  function_name == nullptr ? std::string() : function_name;
   if (frame_name.empty())
 frame_name = "";
   bool is_optimized = frame.GetFunction().GetIsOptimized();


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -696,7 +696,11 @@
   int64_t frame_id = MakeVSCodeFrameID(frame);
   object.try_emplace("id", frame_id);
 
-  std::string frame_name = frame.GetDisplayFunctionName();
+  // `function_name` can be a nullptr, which throws an error when assigned to an
+  // `std::string`.
+  const char *function_name = frame.GetDisplayFunctionName();
+  std::string frame_name =
+  function_name == nullptr ? std::string() : function_name;
   if (frame_name.empty())
 frame_name = "";
   bool is_optimized = frame.GetFunction().GetIsOptimized();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156732: Display PC instead of for stack trace in vscode

[Tom Yang via Phabricator via lldb-commits]

zhyty updated this revision to Diff 546979.
zhyty added a comment.

remove a.out


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156732

Files:
  lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/Makefile
  
lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
  lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
  lldb/tools/lldb-vscode/JSONUtils.cpp


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -701,8 +701,12 @@
   const char *function_name = frame.GetDisplayFunctionName();
   std::string frame_name =
   function_name == nullptr ? std::string() : function_name;
-  if (frame_name.empty())
-frame_name = "";
+  if (frame_name.empty()) {
+// If the function name is unavailable, display the pc address as a 
16-digit
+// hex string, e.g. "0x00012345"
+llvm::raw_string_ostream os(frame_name);
+os << llvm::format_hex(frame.GetPC(), 18);
+  }
   bool is_optimized = frame.GetFunction().GetIsOptimized();
   if (is_optimized)
 frame_name += " [opt]";
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
@@ -0,0 +1,6 @@
+int main() {
+  typedef void (*FooCallback)();
+  FooCallback foo_callback = (FooCallback)0;
+  foo_callback(); // Crash at zero!
+  return 0;
+}
Index: 
lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
===
--- /dev/null
+++ 
lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
@@ -0,0 +1,26 @@
+"""
+Test lldb-vscode stack trace response
+"""
+
+
+import vscode
+from lldbsuite.test.decorators import *
+import os
+
+import lldbvscode_testcase
+from lldbsuite.test import lldbtest, lldbutil
+
+
+class 
TestVSCode_stackTraceMissingFunctionName(lldbvscode_testcase.VSCodeTestCaseBase):
+@skipIfWindows
+@skipIfRemote
+def test_missingFunctionName(self):
+"""
+Test that the stack frame without a function name is given its pc in 
the response.
+"""
+program = self.getBuildArtifact("a.out")
+self.build_and_launch(program)
+
+self.continue_to_next_stop()
+frame_without_function_name = self.get_stackFrames()[0]
+self.assertEquals(frame_without_function_name["name"], 
"0x")
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/Makefile
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/Makefile
@@ -0,0 +1,2 @@
+CXX_SOURCES := main.cpp
+include Makefile.rules


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -701,8 +701,12 @@
   const char *function_name = frame.GetDisplayFunctionName();
   std::string frame_name =
   function_name == nullptr ? std::string() : function_name;
-  if (frame_name.empty())
-frame_name = "";
+  if (frame_name.empty()) {
+// If the function name is unavailable, display the pc address as a 16-digit
+// hex string, e.g. "0x00012345"
+llvm::raw_string_ostream os(frame_name);
+os << llvm::format_hex(frame.GetPC(), 18);
+  }
   bool is_optimized = frame.GetFunction().GetIsOptimized();
   if (is_optimized)
 frame_name += " [opt]";
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/main.cpp
@@ -0,0 +1,6 @@
+int main() {
+  typedef void (*FooCallback)();
+  FooCallback foo_callback = (FooCallback)0;
+  foo_callback(); // Crash at zero!
+  return 0;
+}
Index: lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stackTraceMissingFunctionName/TestVSCode_stackTraceMissingFunctionName.py
@@ -0,0 +1,26 @@
+"""
+Test lldb-vscode stack trace response
+"""
+
+
+import vscode
+from lldbsuite.test.decorators import *
+import os
+
+import lldbvscode_testcase
+from lldbsuite.test import lldbtest, lldbutil
+
+
+class TestVSCode_stackTraceMissingFunctionName(lldbvscode_testcase.VSCodeTestCaseBase):
+@skipIfWindows
+@skipIfRemote
+def test_missingFunctionName(self):
+"""

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Jan Svoboda via Phabricator via lldb-commits]

jansvoboda11 created this revision.
jansvoboda11 added a reviewer: MaskRay.
Herald added subscribers: jhenderson, ormris, ributzka, steven_wu, hiraditya, 
arichardson, emaste.
Herald added a reviewer: JDevlieghere.
Herald added a reviewer: jhenderson.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added projects: clang, LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits.

All command-line tools using `llvm::opt` create an enum of option IDs and a 
table of `OptTable::Info` object. Most of the tools use the same ID 
(`OPT_##ID`), kind (`Option::KIND##Class`), group ID (`OPT_##GROUP`) and alias 
ID (`OPT_##ALIAS`). This patch extracts that common code into canonical macros. 
This results in fewer changes when tweaking the `OPTION` macros emitted by the 
TableGen backend.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157028

Files:
  clang/include/clang/Driver/Options.h
  clang/lib/Driver/DriverOptions.cpp
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  lld/ELF/Driver.h
  lldb/tools/driver/Driver.cpp
  lldb/tools/lldb-server/lldb-gdbserver.cpp
  lldb/tools/lldb-vscode/lldb-vscode.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
  llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llvm-cvtres/llvm-cvtres.cpp
  llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
  llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp
  llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp
  llvm/tools/llvm-dwp/llvm-dwp.cpp
  llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp
  llvm/tools/llvm-ifs/llvm-ifs.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/tools/llvm-ml/llvm-ml.cpp
  llvm/tools/llvm-mt/llvm-mt.cpp
  llvm/tools/llvm-nm/llvm-nm.cpp
  llvm/tools/llvm-rc/llvm-rc.cpp
  llvm/tools/llvm-readobj/llvm-readobj.cpp
  llvm/tools/llvm-size/llvm-size.cpp
  llvm/tools/llvm-strings/llvm-strings.cpp
  llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
  llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
  llvm/tools/sancov/sancov.cpp
  llvm/unittests/Option/OptionParsingTest.cpp

Index: llvm/unittests/Option/OptionParsingTest.cpp
===
--- llvm/unittests/Option/OptionParsingTest.cpp
+++ llvm/unittests/Option/OptionParsingTest.cpp
@@ -17,9 +17,7 @@
 
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
   LastOption
 #undef OPTION
@@ -47,10 +45,7 @@
 };
 
 static constexpr OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  {PREFIX, NAME,  HELPTEXT,METAVAR, OPT_##ID,  Option::KIND##Class,\
-   PARAM,  FLAGS, OPT_##GROUP, OPT_##ALIAS, ALIASARGS, VALUES},
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
Index: llvm/tools/sancov/sancov.cpp
===
--- llvm/tools/sancov/sancov.cpp
+++ llvm/tools/sancov/sancov.cpp
@@ -63,9 +63,7 @@
 using namespace llvm::opt;
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
@@ -78,13 +76,7 @@
 #undef PREFIX
 
 static constexpr opt::OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  {\
-  PREFIX,  NAME,  HELPTEXT,\
-  METAVAR, OPT_##ID,  opt::Option::KIND##Class,\
-  PARAM,   FLAGS, OPT_##GROUP, \
-  OPT_##ALIAS, ALIASARGS, VALUES},
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
Index: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
===
--- llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
+++ llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
@@ -28,9 +28,7 @@
 namespace {
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES) 

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I'll be away for a few days but I took a quick glance. This change looks 
reasonable. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Jan Svoboda via Phabricator via lldb-commits]

jansvoboda11 added a comment.

Here's an example of a patch that changes the `OPTION` macro: D157029 
. I wonder if we could have counterparts to 
`LLVM_MAKE_OPT_ID` and `LLVM_CONSTRUCT_OPT_INFO` that allow overriding the 
default `OPT_` prefix. That would make D157029 
 even smaller. WDYT @MaskRay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D156732: Display PC instead of for stack trace in vscode

[Greg Clayton via Phabricator via lldb-commits]

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

Looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156732

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

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Augusto Noronha via Phabricator via lldb-commits]

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

Thread sanitizer is catching data races in OptionValue, protect accesses
to OptionValue with a mutex.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157041

Files:
  lldb/include/lldb/Interpreter/OptionValue.h
  lldb/source/Interpreter/OptionValue.cpp

Index: lldb/source/Interpreter/OptionValue.cpp
===
--- lldb/source/Interpreter/OptionValue.cpp
+++ lldb/source/Interpreter/OptionValue.cpp
@@ -15,249 +15,310 @@
 using namespace lldb;
 using namespace lldb_private;
 
+
+OptionValue::OptionValue(const OptionValue &other) {
+  std::lock_guard lock(other.m_mutex);
+
+  m_parent_wp = other.m_parent_wp;
+  m_callback = other.m_callback;
+  m_value_was_set = other.m_value_was_set;
+
+}
+
+OptionValue& OptionValue::operator=(const OptionValue &other) {
+  std::scoped_lock lock(m_mutex, other.m_mutex);
+
+  m_parent_wp = other.m_parent_wp;
+  m_callback = other.m_callback;
+  m_value_was_set = other.m_value_was_set;
+
+  return *this;
+}
+
 Status OptionValue::SetSubValue(const ExecutionContext *exe_ctx,
 VarSetOperationType op, llvm::StringRef name,
 llvm::StringRef value) {
+  std::lock_guard lock(m_mutex);
   Status error;
   error.SetErrorString("SetSubValue is not supported");
   return error;
 }
 
 OptionValueBoolean *OptionValue::GetAsBoolean() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeBoolean)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueBoolean *OptionValue::GetAsBoolean() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeBoolean)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueChar *OptionValue::GetAsChar() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeChar)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueChar *OptionValue::GetAsChar() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeChar)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueFileSpec *OptionValue::GetAsFileSpec() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeFileSpec)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueFileSpec *OptionValue::GetAsFileSpec() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeFileSpec)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueFileSpecList *OptionValue::GetAsFileSpecList() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeFileSpecList)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueFileSpecList *OptionValue::GetAsFileSpecList() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeFileSpecList)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueArch *OptionValue::GetAsArch() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeArch)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueArch *OptionValue::GetAsArch() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeArch)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueArray *OptionValue::GetAsArray() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeArray)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueArray *OptionValue::GetAsArray() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeArray)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueArgs *OptionValue::GetAsArgs() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeArgs)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueArgs *OptionValue::GetAsArgs() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeArgs)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueDictionary *OptionValue::GetAsDictionary() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeDictionary)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueDictionary *OptionValue::GetAsDictionary() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeDictionary)
 return static_cast(this);
   return nullptr;
 }
 
 OptionValueEnumeration *OptionValue::GetAsEnumeration() {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeEnum)
 return static_cast(this);
   return nullptr;
 }
 
 const OptionValueEnumeration *OptionValue::GetAsEnumeration() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == O

[Lldb-commits] [PATCH] D156919: [lldb/crashlog] Make register output match lldb ordering in legacy mode

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 547010.
mib marked an inline comment as done.
mib added a comment.

Address feedback & reformat


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

https://reviews.llvm.org/D156919

Files:
  lldb/examples/python/crashlog.py

Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -71,6 +71,7 @@
 sys.exit(1)
 
 from lldb.utils import symbolication
+from lldb.plugins.scripted_process import INTEL64_GPR, ARM64_GPR
 
 
 def read_plist(s):
@@ -84,7 +85,7 @@
 class Thread:
 """Class that represents a thread in a darwin crash log"""
 
-def __init__(self, index, app_specific_backtrace):
+def __init__(self, index, app_specific_backtrace, arch):
 self.index = index
 self.id = index
 self.images = list()
@@ -96,8 +97,38 @@
 self.queue = None
 self.crashed = False
 self.app_specific_backtrace = app_specific_backtrace
+self.arch = arch
+
+def dump_registers(self, prefix=""):
+registers_info = None
+if self.arch:
+if "x86_64" == self.arch:
+registers_info = INTEL64_GPR
+elif "arm64" in self.arch:
+registers_info = ARM64_GPR
+else:
+print("unknown target architecture: %s" % self.arch)
+return
 
-def dump(self, prefix):
+for reg_info in registers_info:
+reg_name = None
+if reg_info["name"] in self.registers:
+reg_name = reg_info["name"]
+elif (
+"generic" in reg_info and reg_info["generic"] in self.registers
+):
+reg_name = reg_info["generic"]
+else:
+# Skip register
+continue
+
+reg = self.registers[reg_name]
+print("%s%-8s = %#16.16x" % (prefix, reg_name, reg))
+else:
+for reg in self.registers.keys():
+print("%s%-8s = %#16.16x" % (prefix, reg, self.registers[reg]))
+
+def dump(self, prefix=""):
 if self.app_specific_backtrace:
 print(
 "%Application Specific Backtrace[%u] %s"
@@ -111,8 +142,7 @@
 frame.dump(prefix + "")
 if self.registers:
 print("%s  Registers:" % (prefix))
-for reg in self.registers.keys():
-print("%s%-8s = %#16.16x" % (prefix, reg, self.registers[reg]))
+self.dump_registers(prefix)
 
 def dump_symbolicated(self, crash_log, options):
 this_thread_crashed = self.app_specific_backtrace
@@ -194,8 +224,7 @@
 print(frame)
 if self.registers:
 print()
-for reg in self.registers.keys():
-print("%-8s = %#16.16x" % (reg, self.registers[reg]))
+self.dump_registers()
 elif self.crashed:
 print()
 print("No thread state (register information) available")
@@ -655,7 +684,7 @@
 def parse_threads(self, json_threads):
 idx = 0
 for json_thread in json_threads:
-thread = self.crashlog.Thread(idx, False)
+thread = self.crashlog.Thread(idx, False, self.crashlog.process_arch)
 if "name" in json_thread:
 thread.name = json_thread["name"]
 thread.reason = json_thread["name"]
@@ -749,7 +778,7 @@
 
 def parse_app_specific_backtraces(self, json_app_specific_bts):
 for idx, backtrace in enumerate(json_app_specific_bts):
-thread = self.crashlog.Thread(idx, True)
+thread = self.crashlog.Thread(idx, True, self.crashlog.process_arch)
 thread.queue = "Application Specific Backtrace"
 if self.parse_asi_backtrace(thread, backtrace):
 self.crashlog.threads.append(thread)
@@ -1008,7 +1037,9 @@
 self.app_specific_backtrace = False
 self.parse_mode = CrashLogParseMode.THREAD
 thread_idx = int(thread_match.group(1))
-self.thread = self.crashlog.Thread(thread_idx, False)
+self.thread = self.crashlog.Thread(
+thread_idx, False, self.crashlog.process_arch
+)
 return
 return
 elif line.startswith("Binary Images:"):
@@ -1020,12 +1051,14 @@
 self.parse_mode = CrashLogParseMode.THREAD
 self.app_specific_backtrace = True
 idx = int(app_backtrace_match.group(1))
-s

[Lldb-commits] [PATCH] D157043: [lldb/crashlog] Make TextCrashLogParser more resilient to new lines

[Med Ismail Bennani via Phabricator via lldb-commits]

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

This patch changes the parsing logic for the legacy crash report format
to avoid interrupting the parsing if there are new lines in the middle
of a section.

To do, the parser starts to skip all consecutive empty lines. If the
number of lines skipped is greater than 1, the parser considers that it
reached a new setion of the report and should reset the parsing mode to
back to normal.

Otherwise, it tries to parse the next line in the current parsing mode.
If it succeeds, the parser will also skip that line since it has already
been parsed and continue the parsing.

rdar://107022595

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157043

Files:
  lldb/examples/python/crashlog.py

Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -899,8 +899,15 @@
 with open(self.path, "r", encoding="utf-8") as f:
 lines = f.read().splitlines()
 
-for line in lines:
+idx = 0
+lines_count = len(lines)
+while True:
+if idx >= lines_count:
+break
+
+line = lines[idx]
 line_len = len(line)
+
 if line_len == 0:
 if self.thread:
 if self.parse_mode == CrashLogParseMode.THREAD:
@@ -924,15 +931,36 @@
 self.crashlog.info_lines[-1]
 ):
 self.crashlog.info_lines.append(line)
+
+empty_lines = 0
+while (
+idx + empty_lines < lines_count
+and len(lines[idx + empty_lines]) == 0
+):
+empty_lines = empty_lines + 1
+
+if (
+empty_lines == 1
+and idx + empty_lines < lines_count - 1
+and self.parse_mode != CrashLogParseMode.NORMAL
+):
+# check if next line can be parsed with the current parse mode
+next_line_idx = idx + empty_lines
+if self.parsers[self.parse_mode](lines[next_line_idx]):
+# If that suceeded, skip the empty line and the next line.
+idx = next_line_idx + 1
+continue
 self.parse_mode = CrashLogParseMode.NORMAL
 else:
 self.parsers[self.parse_mode](line)
 
+idx = idx + 1
+
 return self.crashlog
 
 def parse_exception(self, line):
 if not line.startswith("Exception"):
-return
+return False
 if line.startswith("Exception Type:"):
 self.crashlog.thread_exception = line[15:].strip()
 exception_type_match = self.exception_type_regex.search(line)
@@ -950,7 +978,7 @@
 elif line.startswith("Exception Codes:"):
 self.crashlog.thread_exception_data = line[16:].strip()
 if "type" not in self.crashlog.exception:
-return
+return False
 exception_codes_match = self.exception_codes_regex.search(line)
 if exception_codes_match:
 self.crashlog.exception["codes"] = self.crashlog.thread_exception_data
@@ -961,10 +989,11 @@
 ]
 else:
 if "type" not in self.crashlog.exception:
-return
+return False
 exception_extra_match = self.exception_extra_regex.search(line)
 if exception_extra_match:
 self.crashlog.exception["message"] = exception_extra_match.group(1)
+return True
 
 def parse_normal(self, line):
 if line.startswith("Process:"):
@@ -1063,14 +1092,14 @@
 
 def parse_thread(self, line):
 if line.startswith("Thread"):
-return
+return False
 if self.null_frame_regex.search(line):
 print('warning: thread parser ignored null-frame: "%s"' % line)
-return
+return False
 frame_match = self.frame_regex.search(line)
 if not frame_match:
 print('error: frame regex failed for line: "%s"' % line)
-return
+return False
 
 frame_id = (
 frame_img_name
@@ -1137,6 +1166,8 @@
 self.crashlog.Frame(int(frame_id), int(frame_addr, 0), description)
 )
 
+return True
+
 def parse_images(self, line):
 image_match = self.image_regex_uuid.search(line)
 if image_match:
@@ -1170,17 +1201,21 @@
 
 self.images.append(image)
 self.crashlog.images.append(image)
+

[Lldb-commits] [PATCH] D157044: [lldb/crashlog] Fix sticky image parsing logic

[Med Ismail Bennani via Phabricator via lldb-commits]

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

Prior to this patch, when a user loaded multiple crash report in lldb,
they could get in a situation where all the targets would keep the same
architecture and executable path as the first one that we've created.

The reason behind this was that even if we created a new CrashLog
object, which is derived from a Symbolicator class that has a newly
constructoted image list as a default argument, because that default
argument is only created once when the function is defined, every CrashLog
object would share the same list.

That will cause use to append newly parsed  images to the same
Symbolicator image list accross multiple CrashLog objects.

To address this, this patch changes the default argument value for the
image parameter to `None` and only initialize it as an empty list when
no argument was passed.

This also removes the image list stored in each CrashLog parsers since
they shouldn't have any state and should be re-usable. So now, the only
source of truth is stored in the CrashLog object.

rdar://84984949

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157044

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/symbolication.py


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -501,7 +501,7 @@
 
 
 class Symbolicator:
-def __init__(self, debugger=None, target=None, images=list()):
+def __init__(self, debugger=None, target=None, images=None):
 """A class the represents the information needed to symbolicate
 addresses in a program.
 
@@ -510,7 +510,8 @@
 """
 self.debugger = debugger
 self.target = target
-self.images = images  # a list of images to be used when symbolicating
+# a list of images to be used when symbolicating
+self.images = images if images else list()
 self.addr_mask = 0x
 
 @classmethod
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -531,8 +531,6 @@
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
-# List of DarwinImages sorted by their index.
-self.images = list()
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
 @abc.abstractmethod
@@ -627,7 +625,6 @@
 darwin_image.arch = json_image["arch"]
 if path == self.crashlog.process_path:
 self.crashlog.process_arch = darwin_image.arch
-self.images.append(darwin_image)
 self.crashlog.images.append(darwin_image)
 
 def parse_main_image(self, json_data):
@@ -654,7 +651,7 @@
 location = 0
 if "symbolLocation" in json_frame and 
json_frame["symbolLocation"]:
 location = int(json_frame["symbolLocation"])
-image = self.images[image_id]
+image = self.crashlog.images[image_id]
 image.symbols[symbol] = {
 "name": symbol,
 "type": "code",
@@ -1197,7 +1194,6 @@
 "address": symbol["address"] - int(img_lo, 0),
 }
 
-self.images.append(image)
 self.crashlog.images.append(image)
 return True
 else:


Index: lldb/examples/python/symbolication.py
===
--- lldb/examples/python/symbolication.py
+++ lldb/examples/python/symbolication.py
@@ -501,7 +501,7 @@
 
 
 class Symbolicator:
-def __init__(self, debugger=None, target=None, images=list()):
+def __init__(self, debugger=None, target=None, images=None):
 """A class the represents the information needed to symbolicate
 addresses in a program.
 
@@ -510,7 +510,8 @@
 """
 self.debugger = debugger
 self.target = target
-self.images = images  # a list of images to be used when symbolicating
+# a list of images to be used when symbolicating
+self.images = images if images else list()
 self.addr_mask = 0x
 
 @classmethod
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -531,8 +531,6 @@
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
-# List of DarwinImages sorte

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Jan Svoboda via Phabricator via lldb-commits]

jansvoboda11 updated this revision to Diff 547018.
jansvoboda11 added a comment.
Herald added a reviewer: alexander-shaposhnikov.

Consolidate all usages by extra `_WITH_ID_PREFIX` macros


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

Files:
  clang/include/clang/Driver/Options.h
  clang/lib/Driver/DriverOptions.cpp
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  lld/ELF/Driver.h
  lldb/tools/driver/Driver.cpp
  lldb/tools/lldb-server/lldb-gdbserver.cpp
  lldb/tools/lldb-vscode/lldb-vscode.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/ExecutionEngine/JITLink/COFFDirectiveParser.cpp
  llvm/lib/ExecutionEngine/JITLink/COFFDirectiveParser.h
  llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
  llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llvm-cvtres/llvm-cvtres.cpp
  llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
  llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp
  llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp
  llvm/tools/llvm-dwp/llvm-dwp.cpp
  llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp
  llvm/tools/llvm-ifs/llvm-ifs.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/tools/llvm-lipo/llvm-lipo.cpp
  llvm/tools/llvm-ml/llvm-ml.cpp
  llvm/tools/llvm-mt/llvm-mt.cpp
  llvm/tools/llvm-nm/llvm-nm.cpp
  llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
  llvm/tools/llvm-objdump/ObjdumpOptID.h
  llvm/tools/llvm-objdump/llvm-objdump.cpp
  llvm/tools/llvm-rc/llvm-rc.cpp
  llvm/tools/llvm-readobj/llvm-readobj.cpp
  llvm/tools/llvm-size/llvm-size.cpp
  llvm/tools/llvm-strings/llvm-strings.cpp
  llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
  llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
  llvm/tools/sancov/sancov.cpp
  llvm/unittests/Option/OptionParsingTest.cpp

Index: llvm/unittests/Option/OptionParsingTest.cpp
===
--- llvm/unittests/Option/OptionParsingTest.cpp
+++ llvm/unittests/Option/OptionParsingTest.cpp
@@ -17,9 +17,7 @@
 
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
   LastOption
 #undef OPTION
@@ -47,10 +45,7 @@
 };
 
 static constexpr OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  {PREFIX, NAME,  HELPTEXT,METAVAR, OPT_##ID,  Option::KIND##Class,\
-   PARAM,  FLAGS, OPT_##GROUP, OPT_##ALIAS, ALIASARGS, VALUES},
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
Index: llvm/tools/sancov/sancov.cpp
===
--- llvm/tools/sancov/sancov.cpp
+++ llvm/tools/sancov/sancov.cpp
@@ -63,9 +63,7 @@
 using namespace llvm::opt;
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
@@ -78,13 +76,7 @@
 #undef PREFIX
 
 static constexpr opt::OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  {\
-  PREFIX,  NAME,  HELPTEXT,\
-  METAVAR, OPT_##ID,  opt::Option::KIND##Class,\
-  PARAM,   FLAGS, OPT_##GROUP, \
-  OPT_##ALIAS, ALIASARGS, VALUES},
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
Index: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
===
--- llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
+++ llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
@@ -28,9 +28,7 @@
 namespace {
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
@@ -43,13 +41,7 @@
 #undef PREFIX
 
 static constexpr opt::OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HEL

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

It would be nice if we could clean up what `mode` is. There's a lot of bit-wise 
operations and setting it to 1 and 2 and it's not clear what each one might 
mean in each case... Some kind of enum or semantic meaning for each number 
would be nice. Either way, I think I'm on board with the logic in general. Just 
one question though.




Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:303
+return false;
+  count = llvm::popcount(bitfield);
+  break;

This clobbers the previously set `count` on line 294. What of that `count`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

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

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

A few questions:

- Do all of these need to be protected with a mutex? In your description you're 
saying TSan is detecting data races. What piece of data are you observing the 
data race on?
- Do we need a recursive mutex? I assume that these operations might call into 
each other, but if not it would be nice to just have it be a `std::mutex`.

I think your implementation is already correct, these questions are more geared 
towards performance (i.e. not locking more than we need to, using a more 
efficient mutex type, etc.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157041

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

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

> Do all of these need to be protected with a mutex? In your description you're 
> saying TSan is detecting data races. What piece of data are you observing the 
> data race on?

Only SetAsBoolean/GetAsBoolean are being caught when running our test suite at 
the moment.

> Do we need a recursive mutex? I assume that these operations might call into 
> each other, but if not it would be nice to just have it be a std::mutex.

If we want to protect all accesses then yes, since these functions call each 
other. It we decide to only lock against what's being caught by tsan then we 
can get by with a regular mutex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157041

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

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Interpreter/OptionValue.cpp:41
 llvm::StringRef value) {
+  std::lock_guard lock(m_mutex);
   Status error;

this one doesn't need a lock



Comment at: lldb/source/Interpreter/OptionValue.cpp:393
 
 std::optional OptionValue::GetFormatValue() const {
   if (const OptionValueFormat *option_value = GetAsFormat())

this one doesn't have a lock, should it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157041

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

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D157041#4559255 , @augusto2112 
wrote:

>> Do all of these need to be protected with a mutex? In your description 
>> you're saying TSan is detecting data races. What piece of data are you 
>> observing the data race on?
>
> Only SetAsBoolean/GetAsBoolean are being caught when running our test suite 
> at the moment.
>
>> Do we need a recursive mutex? I assume that these operations might call into 
>> each other, but if not it would be nice to just have it be a std::mutex.
>
> If we want to protect all accesses then yes, since these functions call each 
> other. It we decide to only lock against what's being caught by tsan then we 
> can get by with a regular mutex.

Only `SetAsBoolean` and `GetAsBoolean` are being caught right now, I see. The 
implementation of those methods really isn't any different (AFAICT) from the 
other methods so I would assume we would need to protect all the other `Get` 
and `Set` methods as well then, makes sense. I'd probably keep the recursive 
mutex for now then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157041

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

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Interpreter/OptionValue.cpp:49
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeBoolean)
 return static_cast(this);

do these GetAsX functions need to lock?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157041

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

[Lldb-commits] [PATCH] D157043: [lldb/crashlog] Make TextCrashLogParser more resilient to new lines

[Alex Langford via Phabricator via lldb-commits]

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

Looks alright to me. I think it would be interesting to replace a lot of the 
index juggling code with python Iterators that can only iterate over the lines 
that are not empty, but that's just a fun idea I had.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157043

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

[Lldb-commits] [PATCH] D157044: [lldb/crashlog] Fix sticky image parsing logic

[Alex Langford via Phabricator via lldb-commits]

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

Python default parameter values strike again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157044

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

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Jim Ingham via Phabricator via lldb-commits]

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

Remove redundant code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

Files:
  lldb/source/Plugins/Language/ObjC/Cocoa.cpp


Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -237,7 +237,8 @@
   if (!process_sp)
 return false;
 
-  ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
+  AppleObjCRuntime *runtime = llvm::dyn_cast_or_null(
+  ObjCLanguageRuntime::Get(*process_sp));
 
   if (!runtime)
 return false;
@@ -264,20 +265,56 @@
 
   do {
 if (class_name == "NSIndexSet" || class_name == "NSMutableIndexSet") {
+  // Foundation version 2000 added a bitmask if the index set fit in 64 
bits
+  // and a Tagged Pointer version if the bitmask is small enough to fit in
+  // the tagged pointer payload.  
+  // It also changed the layout (but not the size) of the set descriptor.
+
+  // First check whether this is a tagged pointer.  The bitmask will be in
+  // the payload of the tagged pointer.
+  uint64_t payload;
+  if (runtime->GetFoundationVersion() >= 2000  
+  && descriptor->GetTaggedPointerInfo(nullptr, nullptr, &payload)) {
+count = llvm::popcount(payload);
+break;
+  }
+  // The first 32 bits describe the index set in all cases:
   Status error;
   uint32_t mode = process_sp->ReadUnsignedIntegerFromMemory(
-  valobj_addr + ptr_size, 4, 0, error);
+valobj_addr + ptr_size, 4, 0, error);
   if (error.Fail())
 return false;
-  // this means the set is empty - count = 0
-  if ((mode & 1) == 1) {
-count = 0;
-break;
+  // Now check if the index is held in a bitmask in the object:
+  if (runtime->GetFoundationVersion() >= 2000) {
+// The first two bits are "isSingleRange" and "isBitfield".  If this is
+// a bitfield we handle it here, otherwise set mode appropriately and
+// the rest of the treatment is in common.
+if ((mode & 2) == 2) {
+  // The bitfield is a 64 bit uint at the beginning of the data var.
+  uint64_t bitfield = process_sp->ReadUnsignedIntegerFromMemory(
+valobj_addr + 2 * ptr_size, 8, 0, error);
+  if (error.Fail())
+return false;
+  count = llvm::popcount(bitfield);
+  break;
+}
+// It wasn't a bitfield, so read the isSingleRange from its new loc:
+if ((mode & 1) == 1)
+  mode = 1; // this means the set only has one range
+else
+  mode = 2; // this means the set has multiple ranges
+  } else {
+// this means the set is empty - count = 0
+if ((mode & 1) == 1) {
+  count = 0;
+  break;
+}
+  
+if ((mode & 2) == 2)
+  mode = 1; // this means the set only has one range
+else
+  mode = 2; // this means the set has multiple ranges
   }
-  if ((mode & 2) == 2)
-mode = 1; // this means the set only has one range
-  else
-mode = 2; // this means the set has multiple ranges
   if (mode == 1) {
 count = process_sp->ReadUnsignedIntegerFromMemory(
 valobj_addr + 3 * ptr_size, ptr_size, 0, error);


Index: lldb/source/Plugins/Language/ObjC/Cocoa.cpp
===
--- lldb/source/Plugins/Language/ObjC/Cocoa.cpp
+++ lldb/source/Plugins/Language/ObjC/Cocoa.cpp
@@ -237,7 +237,8 @@
   if (!process_sp)
 return false;
 
-  ObjCLanguageRuntime *runtime = ObjCLanguageRuntime::Get(*process_sp);
+  AppleObjCRuntime *runtime = llvm::dyn_cast_or_null(
+  ObjCLanguageRuntime::Get(*process_sp));
 
   if (!runtime)
 return false;
@@ -264,20 +265,56 @@
 
   do {
 if (class_name == "NSIndexSet" || class_name == "NSMutableIndexSet") {
+  // Foundation version 2000 added a bitmask if the index set fit in 64 bits
+  // and a Tagged Pointer version if the bitmask is small enough to fit in
+  // the tagged pointer payload.  
+  // It also changed the layout (but not the size) of the set descriptor.
+
+  // First check whether this is a tagged pointer.  The bitmask will be in
+  // the payload of the tagged pointer.
+  uint64_t payload;
+  if (runtime->GetFoundationVersion() >= 2000  
+  && descriptor->GetTaggedPointerInfo(nullptr, nullptr, &payload)) {
+count = llvm::popcount(payload);
+break;
+  }
+  // The first 32 bits describe the index set in all cases:
   Status error;
   uint32_t mode = process_sp->ReadUnsignedIntegerFromMemory(
-  valobj_addr + ptr_size, 4, 0, error);
+valobj_addr + ptr_size, 

[Lldb-commits] [PATCH] D157022: Fix the NSIndexSet formatter for macOS Sonoma

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:303
+return false;
+  count = llvm::popcount(bitfield);
+  break;

bulbazord wrote:
> This clobbers the previously set `count` on line 294. What of that `count`?
Oh, that's me saying it was bad to use a variable called `count` when it should 
be `bitfield`, adding the new one and then forgetting to delete the count 
version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157022

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

[Lldb-commits] [lldb] 08dc847 - [lldb][NFCI] Add SBTraceCursor.h to swig headers file

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-08-03T16:41:17-07:00
New Revision: 08dc847d55fbdfa7ff6cb8034633139df0199562

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

LOG: [lldb][NFCI] Add SBTraceCursor.h to swig headers file

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

Added: 


Modified: 
lldb/bindings/headers.swig

Removed: 




diff  --git a/lldb/bindings/headers.swig b/lldb/bindings/headers.swig
index eabf4e12241600..f7871765624dd3 100644
--- a/lldb/bindings/headers.swig
+++ b/lldb/bindings/headers.swig
@@ -63,6 +63,7 @@
 #include "lldb/API/SBThreadCollection.h"
 #include "lldb/API/SBThreadPlan.h"
 #include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceCursor.h"
 #include "lldb/API/SBType.h"
 #include "lldb/API/SBTypeCategory.h"
 #include "lldb/API/SBTypeEnumMember.h"



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

[Lldb-commits] [PATCH] D156934: [lldb][NFCI] Add SBTraceCursor.h to swig headers file

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG08dc847d55fb: [lldb][NFCI] Add SBTraceCursor.h to swig 
headers file (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156934

Files:
  lldb/bindings/headers.swig


Index: lldb/bindings/headers.swig
===
--- lldb/bindings/headers.swig
+++ lldb/bindings/headers.swig
@@ -63,6 +63,7 @@
 #include "lldb/API/SBThreadCollection.h"
 #include "lldb/API/SBThreadPlan.h"
 #include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceCursor.h"
 #include "lldb/API/SBType.h"
 #include "lldb/API/SBTypeCategory.h"
 #include "lldb/API/SBTypeEnumMember.h"


Index: lldb/bindings/headers.swig
===
--- lldb/bindings/headers.swig
+++ lldb/bindings/headers.swig
@@ -63,6 +63,7 @@
 #include "lldb/API/SBThreadCollection.h"
 #include "lldb/API/SBThreadPlan.h"
 #include "lldb/API/SBTrace.h"
+#include "lldb/API/SBTraceCursor.h"
 #include "lldb/API/SBType.h"
 #include "lldb/API/SBTypeCategory.h"
 #include "lldb/API/SBTypeEnumMember.h"
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157041: [lldb] Protect OptionValue accesses from data races

[Augusto Noronha via Phabricator via lldb-commits]

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



Comment at: lldb/source/Interpreter/OptionValue.cpp:49
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeBoolean)
 return static_cast(this);

kastiglione wrote:
> do these GetAsX functions need to lock?
Hmm, I think we can get away with just locking GetXValue and SetXValue, and 
replace the lock with a non recursive one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157041

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

[Lldb-commits] [PATCH] D157059: [lldb][PECOFF] Exclude alignment padding when reading section data

[Hiroshi Yamauchi via Phabricator via lldb-commits]

hjyamauchi created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
hjyamauchi requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

There can be zero padding bytes at a section end for file alignment in PECOFF. 
Exclude those padding bytes when reading the section data.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157059

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/source/Symbol/ObjectFile.cpp
  lldb/unittests/ObjectFile/PECOFF/CMakeLists.txt
  lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp
  llvm/lib/ObjectYAML/COFFYAML.cpp
  llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
  llvm/test/tools/yaml2obj/COFF/xrelocs.yaml

Index: llvm/test/tools/yaml2obj/COFF/xrelocs.yaml
===
--- llvm/test/tools/yaml2obj/COFF/xrelocs.yaml
+++ llvm/test/tools/yaml2obj/COFF/xrelocs.yaml
@@ -30,6 +30,7 @@
 # CHECK-YAML-NEXT: Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_NRELOC_OVFL, IMAGE_SCN_MEM_READ ]
 # CHECK-YAML-NEXT: Alignment:   16
 # CHECK-YAML-NEXT: SectionData: ''
+# CHECK-YAML-NEXT: SizeOfRawData:   16
 # CHECK-YAML-NEXT: Relocations:
 # CHECK-YAML-NEXT:   - VirtualAddress:  0
 # CHECK-YAML-NEXT: SymbolName:  foo
Index: llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
===
--- llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
+++ llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
@@ -1,5 +1,5 @@
 # RUN: not yaml2obj %s -o %t 2>&1 | FileCheck %s
-# CHECK: YAML:18:5: error: unknown key 'SizeOfRawData'
+# CHECK: YAML:14:5: error: StructuredData and SizeOfRawData can't be used together
 
 --- !COFF
 OptionalHeader:
Index: llvm/lib/ObjectYAML/COFFYAML.cpp
===
--- llvm/lib/ObjectYAML/COFFYAML.cpp
+++ llvm/lib/ObjectYAML/COFFYAML.cpp
@@ -689,11 +689,12 @@
 return;
   }
 
-  // Uninitialized sections, such as .bss, typically have no data, but the size
-  // is carried in SizeOfRawData, even though PointerToRawData is zero.
-  if (Sec.SectionData.binary_size() == 0 && Sec.StructuredData.empty() &&
-  NC->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA)
-IO.mapOptional("SizeOfRawData", Sec.Header.SizeOfRawData);
+  IO.mapOptional("SizeOfRawData", Sec.Header.SizeOfRawData, 0U);
+
+  if (!Sec.StructuredData.empty() && Sec.Header.SizeOfRawData) {
+IO.setError("StructuredData and SizeOfRawData can't be used together");
+return;
+  }
 
   IO.mapOptional("Relocations", Sec.Relocations);
 }
Index: lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp
===
--- /dev/null
+++ lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp
@@ -0,0 +1,78 @@
+//===-- TestSectionFileSize.cpp ---===//
+//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Symbol/CallFrameInfo.h"
+#include "lldb/Symbol/UnwindPlan.h"
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+class SectionSizeTest : public testing::Test {
+  SubsystemRAII subsystems;
+};
+
+TEST_F(SectionSizeTest, NoAlignmentPadding) {
+  llvm::Expected ExpectedFile = TestFile::fromYaml(
+  R"(
+--- !COFF
+OptionalHeader:
+  SectionAlignment: 4096
+  FileAlignment:   512
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:swiftast
+VirtualSize: 496
+SizeOfRawData:   512
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+SectionData: 
+
+symbols: []
+...
+)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+  ModuleSP module_sp = std::make_shared(ExpectedFile->moduleSpec());
+  ObjectFile *object_file = module_sp->GetObjectFile();
+  ASSERT_NE(object_file, nullptr);
+
+  SectionList *section_list = object_file->GetSectionList();
+  ASSERT_NE(sec

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Fangrui Song via Phabricator via lldb-commits]

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

In D157028#4558724 , @jansvoboda11 
wrote:

> Here's an example of a patch that changes the `OPTION` macro: D157029 
> . I wonder if we could have counterparts to 
> `LLVM_MAKE_OPT_ID` and `LLVM_CONSTRUCT_OPT_INFO` that allow overriding the 
> default `OPT_` prefix. That would make D157029 
>  even smaller. WDYT @MaskRay?

The `#define OPTION(...) LLVM_MAKE_OPT_ID_WITH_ID_PREFIX(COFF_OPT_, 
__VA_ARGS__),` use case looks good:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lld/ELF/Driver.h:28
   OPT_INVALID = 0,
-#define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11, _12) OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Options.inc"

lld/wasm lld/COFF lld/MachO are not updated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D157059: [lldb][PECOFF] Exclude alignment padding when reading section data

[Hiroshi Yamauchi via Phabricator via lldb-commits]

hjyamauchi updated this revision to Diff 547086.

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

https://reviews.llvm.org/D157059

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/source/Symbol/ObjectFile.cpp
  lldb/unittests/ObjectFile/PECOFF/CMakeLists.txt
  lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp
  llvm/lib/ObjectYAML/COFFYAML.cpp
  llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
  llvm/test/tools/yaml2obj/COFF/xrelocs.yaml

Index: llvm/test/tools/yaml2obj/COFF/xrelocs.yaml
===
--- llvm/test/tools/yaml2obj/COFF/xrelocs.yaml
+++ llvm/test/tools/yaml2obj/COFF/xrelocs.yaml
@@ -30,6 +30,7 @@
 # CHECK-YAML-NEXT: Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_NRELOC_OVFL, IMAGE_SCN_MEM_READ ]
 # CHECK-YAML-NEXT: Alignment:   16
 # CHECK-YAML-NEXT: SectionData: ''
+# CHECK-YAML-NEXT: SizeOfRawData:   16
 # CHECK-YAML-NEXT: Relocations:
 # CHECK-YAML-NEXT:   - VirtualAddress:  0
 # CHECK-YAML-NEXT: SymbolName:  foo
Index: llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
===
--- llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
+++ llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml
@@ -1,5 +1,5 @@
 # RUN: not yaml2obj %s -o %t 2>&1 | FileCheck %s
-# CHECK: YAML:18:5: error: unknown key 'SizeOfRawData'
+# CHECK: YAML:14:5: error: StructuredData and SizeOfRawData can't be used together
 
 --- !COFF
 OptionalHeader:
Index: llvm/lib/ObjectYAML/COFFYAML.cpp
===
--- llvm/lib/ObjectYAML/COFFYAML.cpp
+++ llvm/lib/ObjectYAML/COFFYAML.cpp
@@ -689,11 +689,12 @@
 return;
   }
 
-  // Uninitialized sections, such as .bss, typically have no data, but the size
-  // is carried in SizeOfRawData, even though PointerToRawData is zero.
-  if (Sec.SectionData.binary_size() == 0 && Sec.StructuredData.empty() &&
-  NC->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA)
-IO.mapOptional("SizeOfRawData", Sec.Header.SizeOfRawData);
+  IO.mapOptional("SizeOfRawData", Sec.Header.SizeOfRawData, 0U);
+
+  if (!Sec.StructuredData.empty() && Sec.Header.SizeOfRawData) {
+IO.setError("StructuredData and SizeOfRawData can't be used together");
+return;
+  }
 
   IO.mapOptional("Relocations", Sec.Relocations);
 }
Index: lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp
===
--- /dev/null
+++ lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp
@@ -0,0 +1,78 @@
+//===-- TestSectionFileSize.cpp ---===//
+//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Symbol/CallFrameInfo.h"
+#include "lldb/Symbol/UnwindPlan.h"
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+class SectionSizeTest : public testing::Test {
+  SubsystemRAII subsystems;
+};
+
+TEST_F(SectionSizeTest, NoAlignmentPadding) {
+  llvm::Expected ExpectedFile = TestFile::fromYaml(
+  R"(
+--- !COFF
+OptionalHeader:
+  SectionAlignment: 4096
+  FileAlignment:   512
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:swiftast
+VirtualSize: 496
+SizeOfRawData:   512
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+SectionData: 
+
+symbols: []
+...
+)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+  ModuleSP module_sp = std::make_shared(ExpectedFile->moduleSpec());
+  ObjectFile *object_file = module_sp->GetObjectFile();
+  ASSERT_NE(object_file, nullptr);
+
+  SectionList *section_list = object_file->GetSectionList();
+  ASSERT_NE(section_list, nullptr);
+
+  SectionSP swiftast_section;
+  size_t section_count = section_list->GetNumSections(0);
+  for (size_t i = 0; i < section_count; ++i) {
+SectionSP section_sp = section_list->GetSectionAtIndex(i);
+if (section_sp->GetName() == "swiftast") {
+  swiftast_section = s

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Jan Svoboda via Phabricator via lldb-commits]

jansvoboda11 updated this revision to Diff 547090.
jansvoboda11 added a comment.
Herald added subscribers: pmatos, asb, aheejin, sbc100.
Herald added a project: lld-macho.
Herald added a reviewer: lld-macho.

Convert missed LLD parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

Files:
  clang/include/clang/Driver/Options.h
  clang/lib/Driver/DriverOptions.cpp
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  lld/COFF/Driver.h
  lld/COFF/DriverUtils.cpp
  lld/ELF/Driver.h
  lld/ELF/DriverUtils.cpp
  lld/MachO/Driver.h
  lld/MinGW/Driver.cpp
  lld/wasm/Driver.cpp
  lldb/tools/driver/Driver.cpp
  lldb/tools/lldb-server/lldb-gdbserver.cpp
  lldb/tools/lldb-vscode/lldb-vscode.cpp
  llvm/include/llvm/Option/OptTable.h
  llvm/lib/ExecutionEngine/JITLink/COFFDirectiveParser.cpp
  llvm/lib/ExecutionEngine/JITLink/COFFDirectiveParser.h
  llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
  llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llvm-cvtres/llvm-cvtres.cpp
  llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
  llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp
  llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp
  llvm/tools/llvm-dwp/llvm-dwp.cpp
  llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp
  llvm/tools/llvm-ifs/llvm-ifs.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/tools/llvm-lipo/llvm-lipo.cpp
  llvm/tools/llvm-ml/llvm-ml.cpp
  llvm/tools/llvm-mt/llvm-mt.cpp
  llvm/tools/llvm-nm/llvm-nm.cpp
  llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
  llvm/tools/llvm-objdump/ObjdumpOptID.h
  llvm/tools/llvm-objdump/llvm-objdump.cpp
  llvm/tools/llvm-rc/llvm-rc.cpp
  llvm/tools/llvm-readobj/llvm-readobj.cpp
  llvm/tools/llvm-size/llvm-size.cpp
  llvm/tools/llvm-strings/llvm-strings.cpp
  llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
  llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
  llvm/tools/sancov/sancov.cpp
  llvm/unittests/Option/OptionParsingTest.cpp

Index: llvm/unittests/Option/OptionParsingTest.cpp
===
--- llvm/unittests/Option/OptionParsingTest.cpp
+++ llvm/unittests/Option/OptionParsingTest.cpp
@@ -17,9 +17,7 @@
 
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
   LastOption
 #undef OPTION
@@ -47,10 +45,7 @@
 };
 
 static constexpr OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  {PREFIX, NAME,  HELPTEXT,METAVAR, OPT_##ID,  Option::KIND##Class,\
-   PARAM,  FLAGS, OPT_##GROUP, OPT_##ALIAS, ALIASARGS, VALUES},
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
Index: llvm/tools/sancov/sancov.cpp
===
--- llvm/tools/sancov/sancov.cpp
+++ llvm/tools/sancov/sancov.cpp
@@ -63,9 +63,7 @@
 using namespace llvm::opt;
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
@@ -78,13 +76,7 @@
 #undef PREFIX
 
 static constexpr opt::OptTable::Info InfoTable[] = {
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  {\
-  PREFIX,  NAME,  HELPTEXT,\
-  METAVAR, OPT_##ID,  opt::Option::KIND##Class,\
-  PARAM,   FLAGS, OPT_##GROUP, \
-  OPT_##ALIAS, ALIASARGS, VALUES},
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
Index: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
===
--- llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
+++ llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp
@@ -28,9 +28,7 @@
 namespace {
 enum ID {
   OPT_INVALID = 0, // This is not an option ID.
-#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
-   HELPTEXT, METAVAR, VALUES)  \
-  OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Opts.inc"
 #undef OPTION
 };
@@ -4

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Jan Svoboda via Phabricator via lldb-commits]

jansvoboda11 added inline comments.



Comment at: lld/ELF/Driver.h:28
   OPT_INVALID = 0,
-#define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11, _12) OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Options.inc"

MaskRay wrote:
> lld/wasm lld/COFF lld/MachO are not updated?
You're right, I accidentally missed some LLD parts. Updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 547099.
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

Link to developer policy.


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

https://reviews.llvm.org/D156949

Files:
  lldb/CODE_OWNERS.txt
  lldb/CodeOwners.rst

Index: lldb/CodeOwners.rst
===
--- /dev/null
+++ lldb/CodeOwners.rst
@@ -0,0 +1,240 @@
+
+LLDB Code Owners
+
+
+This file is a list of the `code owners `_ for LLDB.
+
+.. contents::
+   :depth: 2
+   :local:
+
+Current Code Owners
+===
+The following people are the active code owners for the project. Please reach
+out to them for code reviews, questions about their area of expertise, or other
+assistance.
+
+All parts of LLDB not covered by someone else
+--
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+Components
+--
+These code owners are responsible for particular high-level components within
+LLDB.
+
+ABI
+~~~
+| Jason Molenda
+| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub), jasonmolenda (Discourse), jasonmolenda (Discord)
+
+| David Spickett
+| david.spickett\@linaro.org (email), DavidSpickett (Phabricator), DavidSpickett (GitHub), DavidSpickett (Discourse), davidspickett (Discord)
+
+
+Breakpoint
+~~
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+CMake & Build System
+
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+| Alex Langford
+| alangford\@apple.com (email), bulbazord (Phabricator), bulbazord (GitHub), bulbazord (Discourse), bulba_zord (Discord)
+
+Commands
+
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Expression Parser
+~
+| Michael Buch
+| michaelbuch12\@gmail.com (email), Michael137 (Phabricator), Michael137 (GitHub), Michael137 (Discourse)
+
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Interpreter
+~~~
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+| Greg Clayton
+| gclayton\@fb.com (email), clayborg (Phabricator), clayborg (GitHub), clayborg (Discourse)
+
+
+Lua
+~~~
+| Jonas Delvieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+Python
+~~
+| Med Ismail Bennani
+| ismail\@bennani.ma (email), mib (Phabricator), medismailben (GitHub), mib (Discourse), mib#8727 (Discord)
+
+Target/Process Control
+~~
+| Med Ismail Bennani
+| ismail\@bennani.ma (email), mib (Phabricator), medismailben (GitHub), mib (Discourse), mib#8727 (Discord)
+
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Test Suite
+~~
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+| Pavel Labath
+| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub), labath (Discourse)
+
+Trace
+~
+| Walter Erquinigo
+| a20012251\@gmail.com (email), wallace (Phabricator), walter-erquinigo (GitHub), wallace (Discourse), werquinigo (Discord)
+
+Unwinding
+~
+| Jason Molenda
+| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub), jasonmolenda (Discourse), jasonmolenda (Discord)
+
+Utility
+~~~
+| Pavel Labath
+| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub), labath (Discourse)
+
+ValueObject
+~~~
+| Jim Ingham
+| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub), jingham (Discourse)
+
+Watchpoints
+~~~
+| Jason Molenda
+| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub), jasonmolenda (Discourse), jasonmolenda (Discord)
+
+File Formats
+
+The following people are responsible for decisions involving file and debug
+info formats.
+
+(PE)COFF
+
+| Saleem Abdulrasool
+| compnerd\@compnerd.org (email), compnerd (Phabricator), compnerd (GitHub), compnerd (Discourse), compnerd (Discord)
+
+Breakpad
+
+| Pavel Labath
+| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub), labath (Discourse)
+
+CTF
+~~~
+| Jonas Devlieghere
+| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub), jdevlieghere (Discourse), jdevlieghere (Discord)
+
+DWARF
+~
+| Adrian Prantl
+| aprantl\@app

[Lldb-commits] [PATCH] D157059: [lldb][PECOFF] Exclude alignment padding when reading section data

[Saleem Abdulrasool via Phabricator via lldb-commits]

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

LGTM outside of the PE header check.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:1035
+  // llvm::object::COFFObjectFile::getSectionSize().
+  if (m_binary->getDOSHeader())
+return std::min(section->GetByteSize(), section->GetFileSize());

Can we use `m_binary->getPEHeader() || m_binary->getPE32Header()` here instead? 
 We are technically ensuring that this is a linked (PE) binary rather than an 
object file.  While a DOS header is present, it is not an absolute requirement 
(theoretically, it is practically never going to change).


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

https://reviews.llvm.org/D157059

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