[Lldb-commits] [PATCH] D159127: [lldb][libc++] Adds chrono data formatters.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

@Mordante @Michael137  This seems to fail on older versions of compiler/libcxx 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/7247/ You can 
probably fix this by just requiring a minimum clang version in the tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159127

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

[Lldb-commits] [PATCH] D159127: [lldb][libc++] Adds chrono data formatters.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:996
+  TypeSummaryImplSP(new StringSummaryFormat(
+  eTypeOptionHideChildren | eTypeOptionHideValue, "${var.__rep_}")));
 }

Nice!



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py:23
+# clean slate for the next test case.
+def cleanup():
+self.runCmd("type format clear", check=False)

You probably copied & pasted this — this is no longer needed since every test 
function is now running in its own instance.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py:33
+
+# empty vectors (and storage pointers SHOULD BOTH BE NULL..)
+self.expect("frame variable ns", substrs=["ns = 0"])

This is not guaranteed behavior and is highly specific on the actual compiler. 
I would probably just remove this first sets of tests.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp:17
+
+  return 0; // break here
+}

can you change this to a function call like
```
std::cout()<<"break here\n";
```

It's not guaranteed that all compilers actually generate code for this line 
that results in a breakpoint opportunity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159127

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

[Lldb-commits] [PATCH] D158958: [LLDB][REPL] Change the default tab size

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> @aprantl, how should I proceed with the swift branch? Should I create a new 
> branch in https://github.com/apple/swift and share it here so it's available 
> for whoever does the merge?

Actually, I can just cherry-pick it myself, let's not worry about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158958

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

[Lldb-commits] [PATCH] D159031: [LLDB] Fix IOHandlerEditline::GetCurrentLines()

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Is this covered by any tests?




Comment at: lldb/source/Core/IOHandler.cpp:512
+StringList IOHandlerEditline::GetCurrentLines() const {
+#if LLDB_ENABLE_LIBEDIT
+  if (m_editline_up)

I think this would benefit from a comment explaining the difference?



Comment at: lldb/source/Expression/REPL.cpp:535
+  if (current_line_idx < current_lines.GetSize()) {
+for (uint32_t i = 0; i < current_line_idx; ++i) {
+  const char *line_cstr = current_lines.GetStringAtIndex(i);

`for (const char *line_cstr : current_lines)` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159031

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

[Lldb-commits] [PATCH] D158958: [LLDB][REPL] Change the default tab size

[Adrian Prantl via Phabricator via lldb-commits]

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

This should probably be a language-specific setting, but I see no problem in 
doing it this way right now, since there is practically only one language.
Please update any REPL tests on the Swift branch that need updating when 
cherry-picking though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158958

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

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Test LGTM, I'll defer to others for the dynamic loader plugin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158583

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

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: 
lldb/test/API/functionalities/dyld-multiple-rdebug/TestDyldWithMultupleRDebug.py:54
+self.assertIn("main",
+  thread.GetFrameAtIndex(0).GetDisplayFunctionName())
+process.Continue()

You should be able to shorten the test setup considerably by using 
lldbutil.run_to_name_breakpoint() and only manually setting the second 
breakpoint afterwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158583

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

[Lldb-commits] [PATCH] D158470: [lldb] Add support for recognizing swift mangled names

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Core/Mangled.cpp:61
 
+  // Swift's older style of mangling used "_T" as a mangling prefix. This can
+  // lead to false positives with other symbols that just so happen to start

fdeazeve wrote:
> JDevlieghere wrote:
> > aprantl wrote:
> > > Feel free to completely ignore this, it's a pointless micro optimization.
> > > 
> > > I'm curious if it would be more efficient to write this as 
> > > 
> > > ```
> > > switch (name[0]) {
> > >   case '?': return Mangled::eManglingSchemeMSVC;
> > >   case '_': 
> > >  switch(name[1]) {
> > > ...
> > >  }
> > >   case '$': 
> > >  switch(name[1]) {
> > > ...
> > >  }
> > > ```
> > > or if it would actually be slower than the chain of "vector" comparisons 
> > > we have right now?
> > I actually started writing a similar comment before discarding it. Even if 
> > this code is as hot as I expect it to be, I don't think it would outweigh 
> > the complexity and the potential for bugs. I really like how you can glance 
> > at the code and see the different mangling schemes and that's the first 
> > thing we'd lose. Anyway happy to be proven wrong too. 
> Honestly the optimizer is pretty good at doing those, look at the IR: 
> https://godbolt.org/z/PY3TeadbM
Sounds good, let's leave it!

Just trolling now: should we use some crazy #ifdef magic to flip the order 
based on the host platform, such the `?` comes first on windows and `$` comes 
first on Darwin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158470

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

[Lldb-commits] [PATCH] D158470: [lldb] Add support for recognizing swift mangled names

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Core/Mangled.cpp:61
 
+  // Swift's older style of mangling used "_T" as a mangling prefix. This can
+  // lead to false positives with other symbols that just so happen to start

Feel free to completely ignore this, it's a pointless micro optimization.

I'm curious if it would be more efficient to write this as 

```
switch (name[0]) {
  case '?': return Mangled::eManglingSchemeMSVC;
  case '_': 
 switch(name[1]) {
...
 }
  case '$': 
 switch(name[1]) {
...
 }
```
or if it would actually be slower than the chain of "vector" comparisons we 
have right now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158470

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

[Lldb-commits] [PATCH] D158043: [lldb][NFCI] Module constructor should take ConstString by value

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/Core/Module.h:127
   const FileSpec _spec, const ArchSpec ,
-  const ConstString *object_name = nullptr,
-  lldb::offset_t object_offset = 0,
+  const ConstString object_name, lldb::offset_t object_offset = 0,
   const llvm::sys::TimePoint<> _mod_time = 
llvm::sys::TimePoint<>());

If we're passing it by value, why the const qualifier?



Comment at: lldb/source/Core/Module.cpp:249
 
-  if (object_name)
-m_object_name = *object_name;
+  m_object_name = object_name;
 

why not `m_object_name(object_name)` above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158043

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

[Lldb-commits] [PATCH] D157512: [lldb][PlatformDarwin] Only parse SDK from debug-info if target language is Objective-C

[Adrian Prantl via Phabricator via lldb-commits]

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

Apart from Jim's comment (does this still work?) this seems like a reasonable 
heuristic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157512

___
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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> If it's straightforward, I think would be nice to have a unit test with two 
> threads modifying the same OptionValue. That way a TSan run would catch this 
> issue. If that's more work than expected then this is fine as is.

We might just want to set up a bot that runs the entire test suite under TSan, 
assuming that we can get it clean?


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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Interpreter/OptionValue.cpp:55
 const OptionValueBoolean *OptionValue::GetAsBoolean() const {
+  std::lock_guard lock(m_mutex);
   if (GetType() == OptionValue::eTypeBoolean)

If you are following @kastiglione 's suggestion from above (I'd be fine with 
choosing symmetry over performance) then this method (and the ones below) also 
doesn't need any locks, since it just calls other thread-safe methods.


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] D156020: [lldb][PlatformDarwin] Parse SDK path for module compilation from debug-info

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1363
+  XcodeSDK sdk;
+  for (unsigned i = 0; i < sym_file->GetNumCompileUnits(); ++i)
+if (auto cu_sp = sym_file->GetCompileUnitAtIndex(i))

Michael137 wrote:
> Only remaining question is whether we want to limit this to just Objective-C 
> and Swift. Going through each compile unit for C++ seems like a lot of work 
> for something that we won't use
> 
> @aprantl 
The work to detect the language and to get the SDK attribute is almost the 
same, both are stored in the top-level CU DIE.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156020

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

[Lldb-commits] [PATCH] D156279: [lldb] Increase the default timeouts when running under ASan

[Adrian Prantl via Phabricator via lldb-commits]

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

Nevermind, you answered my question in the description already!


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

https://reviews.llvm.org/D156279

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

[Lldb-commits] [PATCH] D156279: [lldb] Increase the default timeouts when running under ASan

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This LGTM, but I'm surprized you didn't have to delete any older code that 
tried to do the same thing. Did the thing I remember not survive the transition 
to tablegen or is this orthogonal?


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

https://reviews.llvm.org/D156279

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

[Lldb-commits] [PATCH] D156020: [lldb][PlatformDarwin] Parse SDK path for module compilation from debug-info

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/Target/Platform.h:479
+/// to an internal SDK
+bool found_internal_sdk = false;
+

These flags really only make sense in the context of an XcodeSDK, so why not 
just return an XcodeSDK or XcodeSDK::Info object here? Otherwise we'll probably 
introduce subtle bugs due to a lossy translation between the flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156020

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

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Adrian Prantl via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf09f0a6b1076: [lldb] Consider OP_addrx in 
DWARFExpression::Update_DW_OP_addr (authored by fdeazeve, committed by aprantl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155198

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/unittests/Expression/DWARFExpressionTest.cpp


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  // Subtract one to account for the opcode.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128();
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  encoder.AppendData(data_after_op);
+  m_data.SetData(encoder.GetDataBuffer());
+  return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+  break;
+offset += op_arg_size;
   }
   return false;
 }


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
   // the heap data so "m_data" will now correctly manage the heap data.
   m_data.SetData(encoder.GetDataBuffer());
   return true;
-} else {
-  const offset_t op_arg_size =
-  GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-  if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
-  offset += op_arg_size;
 }
+if (op == DW_OP_addrx) {
+  // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+  // read-only debug_addr table.
+  // Subtract one to account for the opcode.
+  llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+  // Read the addrx index to determine how many bytes it needs.
+  const lldb::offset_t old_offset = offset;
+  m_data.GetULEB128();
+  if (old_offset == offset)
+return false;
+  llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+  DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+  encoder.AppendData(data_before_op);
+  encoder.AppendU8(DW_OP_addr);
+  encoder.AppendAddress(file_addr);
+  

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

As far as I'm concerned, this doesn't look too controversial to me and it is 
unblocking one of the bots. I think it would be okay to tentatively land this, 
but be on the lookout and promptly react to any post-commit feedback from 
@clayborg.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155198

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

[Lldb-commits] [PATCH] D153054: [lldb][NFCI] TypeSystemClang::GetTypeForIdentifier should take a StringRef

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:76
+
+  compiler_type = 
scratch_ts_sp->GetTypeForIdentifier(g_lldb_autogen_nspair);
+

why not directly pass in `"__lldb_autogen_nspair"` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153054

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

[Lldb-commits] [PATCH] D152872: Add support for __debug_line_str in Mach-O

[Adrian Prantl via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG27fac4a72ae5: Add support for __debug_line_str in Mach-O 
(authored by aprantl).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152872

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c


Index: lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
@@ -0,0 +1,15 @@
+// Test that the file names in the __debug_line_str section can be decoded.
+
+// REQUIRES: system-darwin
+
+// RUN: %clang -target x86_64-apple-darwin %s -c -o %t.o -gdwarf-5
+// RUN: llvm-readobj --sections %t.o | FileCheck %s --check-prefix NAMES
+// RUN: xcrun %clang -target x86_64-apple-darwin -o %t.exe %t.o
+// RUN: %lldb %t.exe -b -o "target modules dump line-table %s" | FileCheck %s
+
+// NAMES: Name: __debug_line_str
+
+int main(int argc, char **argv) {
+  // CHECK: dwarf5-macho.c:[[@LINE+1]]
+  return 0;
+}
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1443,6 +1443,7 @@
   static ConstString g_sect_name_dwarf_debug_frame("__debug_frame");
   static ConstString g_sect_name_dwarf_debug_info("__debug_info");
   static ConstString g_sect_name_dwarf_debug_line("__debug_line");
+  static ConstString g_sect_name_dwarf_debug_line_str("__debug_line_str");
   static ConstString g_sect_name_dwarf_debug_loc("__debug_loc");
   static ConstString g_sect_name_dwarf_debug_loclists("__debug_loclists");
   static ConstString g_sect_name_dwarf_debug_macinfo("__debug_macinfo");
@@ -1472,6 +1473,8 @@
 return eSectionTypeDWARFDebugInfo;
   if (section_name == g_sect_name_dwarf_debug_line)
 return eSectionTypeDWARFDebugLine;
+  if (section_name == g_sect_name_dwarf_debug_line_str)
+return eSectionTypeDWARFDebugLineStr;
   if (section_name == g_sect_name_dwarf_debug_loc)
 return eSectionTypeDWARFDebugLoc;
   if (section_name == g_sect_name_dwarf_debug_loclists)


Index: lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
@@ -0,0 +1,15 @@
+// Test that the file names in the __debug_line_str section can be decoded.
+
+// REQUIRES: system-darwin
+
+// RUN: %clang -target x86_64-apple-darwin %s -c -o %t.o -gdwarf-5
+// RUN: llvm-readobj --sections %t.o | FileCheck %s --check-prefix NAMES
+// RUN: xcrun %clang -target x86_64-apple-darwin -o %t.exe %t.o
+// RUN: %lldb %t.exe -b -o "target modules dump line-table %s" | FileCheck %s
+
+// NAMES: Name: __debug_line_str
+
+int main(int argc, char **argv) {
+  // CHECK: dwarf5-macho.c:[[@LINE+1]]
+  return 0;
+}
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1443,6 +1443,7 @@
   static ConstString g_sect_name_dwarf_debug_frame("__debug_frame");
   static ConstString g_sect_name_dwarf_debug_info("__debug_info");
   static ConstString g_sect_name_dwarf_debug_line("__debug_line");
+  static ConstString g_sect_name_dwarf_debug_line_str("__debug_line_str");
   static ConstString g_sect_name_dwarf_debug_loc("__debug_loc");
   static ConstString g_sect_name_dwarf_debug_loclists("__debug_loclists");
   static ConstString g_sect_name_dwarf_debug_macinfo("__debug_macinfo");
@@ -1472,6 +1473,8 @@
 return eSectionTypeDWARFDebugInfo;
   if (section_name == g_sect_name_dwarf_debug_line)
 return eSectionTypeDWARFDebugLine;
+  if (section_name == g_sect_name_dwarf_debug_line_str)
+return eSectionTypeDWARFDebugLineStr;
   if (section_name == g_sect_name_dwarf_debug_loc)
 return eSectionTypeDWARFDebugLoc;
   if (section_name == g_sect_name_dwarf_debug_loclists)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152872: Add support for __debug_line_str in Mach-O

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: bulbazord, fdeazeve, rastogishubham, JDevlieghere.
Herald added a project: All.
aprantl requested review of this revision.

This patch resolves an issue that currently accounts for the vast majority of 
failures on the matrix bot.


https://reviews.llvm.org/D152872

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c


Index: lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
@@ -0,0 +1,15 @@
+// Test that the file names in the __debug_line_str section can be decoded.
+
+// REQUIRES: system-darwin
+
+// RUN: %clang -target x86_64-apple-darwin %s -c -o %t.o -gdwarf-5
+// RUN: llvm-readobj --sections %t.o | FileCheck %s --check-prefix NAMES
+// RUN: xcrun %clang -target x86_64-apple-darwin -o %t.exe %t.o
+// RUN: %lldb %t.exe -b -o "target modules dump line-table %s" | FileCheck %s
+
+// NAMES: Name: __debug_line_str
+
+int main(int argc, char **argv) {
+  // CHECK: dwarf5-macho.c:[[@LINE+1]]
+  return 0;
+}
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1443,6 +1443,7 @@
   static ConstString g_sect_name_dwarf_debug_frame("__debug_frame");
   static ConstString g_sect_name_dwarf_debug_info("__debug_info");
   static ConstString g_sect_name_dwarf_debug_line("__debug_line");
+  static ConstString g_sect_name_dwarf_debug_line_str("__debug_line_str");
   static ConstString g_sect_name_dwarf_debug_loc("__debug_loc");
   static ConstString g_sect_name_dwarf_debug_loclists("__debug_loclists");
   static ConstString g_sect_name_dwarf_debug_macinfo("__debug_macinfo");
@@ -1472,6 +1473,8 @@
 return eSectionTypeDWARFDebugInfo;
   if (section_name == g_sect_name_dwarf_debug_line)
 return eSectionTypeDWARFDebugLine;
+  if (section_name == g_sect_name_dwarf_debug_line_str)
+return eSectionTypeDWARFDebugLineStr;
   if (section_name == g_sect_name_dwarf_debug_loc)
 return eSectionTypeDWARFDebugLoc;
   if (section_name == g_sect_name_dwarf_debug_loclists)


Index: lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/dwarf5-macho.c
@@ -0,0 +1,15 @@
+// Test that the file names in the __debug_line_str section can be decoded.
+
+// REQUIRES: system-darwin
+
+// RUN: %clang -target x86_64-apple-darwin %s -c -o %t.o -gdwarf-5
+// RUN: llvm-readobj --sections %t.o | FileCheck %s --check-prefix NAMES
+// RUN: xcrun %clang -target x86_64-apple-darwin -o %t.exe %t.o
+// RUN: %lldb %t.exe -b -o "target modules dump line-table %s" | FileCheck %s
+
+// NAMES: Name: __debug_line_str
+
+int main(int argc, char **argv) {
+  // CHECK: dwarf5-macho.c:[[@LINE+1]]
+  return 0;
+}
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1443,6 +1443,7 @@
   static ConstString g_sect_name_dwarf_debug_frame("__debug_frame");
   static ConstString g_sect_name_dwarf_debug_info("__debug_info");
   static ConstString g_sect_name_dwarf_debug_line("__debug_line");
+  static ConstString g_sect_name_dwarf_debug_line_str("__debug_line_str");
   static ConstString g_sect_name_dwarf_debug_loc("__debug_loc");
   static ConstString g_sect_name_dwarf_debug_loclists("__debug_loclists");
   static ConstString g_sect_name_dwarf_debug_macinfo("__debug_macinfo");
@@ -1472,6 +1473,8 @@
 return eSectionTypeDWARFDebugInfo;
   if (section_name == g_sect_name_dwarf_debug_line)
 return eSectionTypeDWARFDebugLine;
+  if (section_name == g_sect_name_dwarf_debug_line_str)
+return eSectionTypeDWARFDebugLineStr;
   if (section_name == g_sect_name_dwarf_debug_loc)
 return eSectionTypeDWARFDebugLoc;
   if (section_name == g_sect_name_dwarf_debug_loclists)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152837: [lldb] Identify Swift-implemented ObjC classes

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Core/ValueObjectDynamicValue.cpp:169
+  if (known_type == lldb::eLanguageTypeObjC &&
+  UseSwiftRuntime(*m_parent, exe_ctx)) {
+runtime = process->GetLanguageRuntime(lldb::eLanguageTypeSwift);

This is a bit of a layering violation. Could the ObjC language runtime perform 
this check in `GetDynamicTypeAndAddress` and dispatch to the Swift runtime 
there?

If you do this, we need to ensure we can't get into an infinite loop between 
the runtimes.

Alternatively, we could add a virtual GetPreferredLanguageRuntime() method to 
LanguageRuntime that we call here, maybe?



Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h:89
 
+virtual bool IsSwift() const { return false; }
+

Can you add a Doxygen commen?
/// Determine whether this class is implemented in Swift.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152837

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

[Lldb-commits] [PATCH] D152590: Streamline expression parser error messages

[Adrian Prantl via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG133c3eaac0a5: Streamline expression parser error messages. 
(authored by aprantl).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152590

Files:
  lldb/source/Expression/UserExpression.cpp
  lldb/source/Interpreter/CommandReturnObject.cpp
  lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
  lldb/test/API/commands/expression/fixits/TestFixIts.py


Index: lldb/test/API/commands/expression/fixits/TestFixIts.py
===
--- lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -154,7 +154,6 @@
 multiple_runs_options.SetRetriesWithFixIts(0)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("using declaration resolved to type without 'typename'", 
errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 self.assertIn("using typename T::TypeDef", errmsg)
@@ -166,7 +165,6 @@
 multiple_runs_options.SetRetriesWithFixIts(1)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 # Both our fixed expressions should be in the suggested expression.
 self.assertIn("using typename T::TypeDef", errmsg)
Index: lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
===
--- lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
+++ lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
@@ -151,8 +151,7 @@
 value = frame.EvaluateExpression("struct Redef { float y; };", 
top_level_opts)
 self.assertFalse(value.GetError().Success())
 self.assertIn(
-"""
-error: :1:8: redefinition of 'Redef'
+"""error: :1:8: redefinition of 'Redef'
 1 | struct Redef { float y; };
   |^
 :1:8: previous definition is here
Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -101,7 +101,10 @@
   SetStatus(eReturnStatusFailed);
   if (in_string.empty())
 return;
-  error(GetErrorStream()) << in_string.rtrim() << '\n';
+  // Workaround to deal with already fully formatted compiler diagnostics.
+  llvm::StringRef msg(in_string.rtrim());
+  msg.consume_front("error: ");
+  error(GetErrorStream()) << msg << '\n';
 }
 
 void CommandReturnObject::SetError(const Status ,
Index: lldb/source/Expression/UserExpression.cpp
===
--- lldb/source/Expression/UserExpression.cpp
+++ lldb/source/Expression/UserExpression.cpp
@@ -330,11 +330,10 @@
   std::string msg;
   {
 llvm::raw_string_ostream os(msg);
-os << "expression failed to parse:\n";
 if (!diagnostic_manager.Diagnostics().empty())
   os << diagnostic_manager.GetString();
 else
-  os << "unknown error";
+  os << "expression failed to parse (no further compiler diagnostics)";
 if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
 !fixed_expression->empty())
   os << "\nfixed expression suggested:\n  " << *fixed_expression;


Index: lldb/test/API/commands/expression/fixits/TestFixIts.py
===
--- lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -154,7 +154,6 @@
 multiple_runs_options.SetRetriesWithFixIts(0)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("using declaration resolved to type without 'typename'", errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 self.assertIn("using typename T::TypeDef", errmsg)
@@ -166,7 +165,6 @@
 multiple_runs_options.SetRetriesWithFixIts(1)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 # Both our fixed expressions should be in the 

[Lldb-commits] [PATCH] D152590: Streamline expression parser error messages

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

pinging after the weekend.


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

https://reviews.llvm.org/D152590

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

[Lldb-commits] [PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Thanks for prototyping this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152708

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

[Lldb-commits] [PATCH] D152590: Streamline expression parser error messages

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: jingham, kastiglione, JDevlieghere.
Herald added a project: All.
aprantl requested review of this revision.

Currently the expression parser prints a mostly useless generic error before 
printing the compiler error:

  (lldb) p 1+x)
  error: expression failed to parse:
  error: :1:3: use of undeclared identifier 'x'
  1+x)
^

This is distracting and as far as I can tell only exists to work around the 
fact that the first "error: " is unconditionally injected by 
`CommandReturnObject`. The solution is not very elegant, but the result looks 
much better.

(Partially addresses rdar://110492710)


https://reviews.llvm.org/D152590

Files:
  lldb/source/Expression/UserExpression.cpp
  lldb/source/Interpreter/CommandReturnObject.cpp
  lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
  lldb/test/API/commands/expression/fixits/TestFixIts.py


Index: lldb/test/API/commands/expression/fixits/TestFixIts.py
===
--- lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -154,7 +154,6 @@
 multiple_runs_options.SetRetriesWithFixIts(0)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("using declaration resolved to type without 'typename'", 
errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 self.assertIn("using typename T::TypeDef", errmsg)
@@ -166,7 +165,6 @@
 multiple_runs_options.SetRetriesWithFixIts(1)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 # Both our fixed expressions should be in the suggested expression.
 self.assertIn("using typename T::TypeDef", errmsg)
Index: lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
===
--- lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
+++ lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
@@ -151,8 +151,7 @@
 value = frame.EvaluateExpression("struct Redef { float y; };", 
top_level_opts)
 self.assertFalse(value.GetError().Success())
 self.assertIn(
-"""
-error: :1:8: redefinition of 'Redef'
+"""error: :1:8: redefinition of 'Redef'
 1 | struct Redef { float y; };
   |^
 :1:8: previous definition is here
Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -101,7 +101,10 @@
   SetStatus(eReturnStatusFailed);
   if (in_string.empty())
 return;
-  error(GetErrorStream()) << in_string.rtrim() << '\n';
+  // Workaround to deal with already fully formatted compiler diagnostics.
+  llvm::StringRef msg(in_string.rtrim());
+  msg.consume_front("error: ");
+  error(GetErrorStream()) << msg << '\n';
 }
 
 void CommandReturnObject::SetError(const Status ,
Index: lldb/source/Expression/UserExpression.cpp
===
--- lldb/source/Expression/UserExpression.cpp
+++ lldb/source/Expression/UserExpression.cpp
@@ -330,11 +330,10 @@
   std::string msg;
   {
 llvm::raw_string_ostream os(msg);
-os << "expression failed to parse:\n";
 if (!diagnostic_manager.Diagnostics().empty())
   os << diagnostic_manager.GetString();
 else
-  os << "unknown error";
+  os << "expression failed to parse (no further compiler diagnostics)";
 if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
 !fixed_expression->empty())
   os << "\nfixed expression suggested:\n  " << *fixed_expression;


Index: lldb/test/API/commands/expression/fixits/TestFixIts.py
===
--- lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -154,7 +154,6 @@
 multiple_runs_options.SetRetriesWithFixIts(0)
 value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
 errmsg = value.GetError().GetCString()
-self.assertIn("expression failed to parse", errmsg)
 self.assertIn("using declaration resolved to type without 'typename'", errmsg)
 self.assertIn("fixed expression suggested:", errmsg)
 self.assertIn("using typename T::TypeDef", errmsg)
@@ -166,7 +165,6 @@
 multiple_runs_options.SetRetriesWithFixIts(1)
  

[Lldb-commits] [PATCH] D152560: Make TestStepNodebug more robust against codegen changes

[Adrian Prantl via Phabricator via lldb-commits]

aprantl abandoned this revision.
aprantl added a comment.

According to an offline conversation with @jingham this is still not the 
expected behavior from the stepping engine.


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

https://reviews.llvm.org/D152560

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

[Lldb-commits] [PATCH] D152560: Make TestStepNodebug more robust against codegen changes

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

  lldb) image dump line-table with-debug.c
  Line table for 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c
 in `a.out
  0x00013d4c: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:7
  0x00013d5c: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:8:7
  0x00013d60: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:9:42
  0x00013d68: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:9:18
  0x00013d7c: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:9:16
  0x00013d80: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:10:10
  0x00013d84: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:10:3
  0x00013d90: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:13
  0x00013da0: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:14:7
  0x00013da4: 
/Volumes/Data/llvm-project/lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c:15:58


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

https://reviews.llvm.org/D152560

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

[Lldb-commits] [PATCH] D152569: [lldb] Introduce a tool to quickly generate projects with an arbitrary number of sources

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

That looks useful!




Comment at: lldb/scripts/generate-project.py:175
+# Building Objects and their respective swiftmodule
+f.write(f"$(OBJDIR)/%.o: %.swift objdir\n")
+f.write("\t$(SWIFT_FE) -c -primary-file $< -I objs/ \\\n")

Would `"""` multiline literals be more readable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152569

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

[Lldb-commits] [PATCH] D152560: Make TestStepNodebug more robust against codegen changes

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: jingham, Michael137.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
aprantl requested review of this revision.

Depending on the compiler I've seen that for example the step-in command can do 
a column-step first before exiting out of the function:

  * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  frame #0: 0x00013d80 
a.out`called_from_nodebug_actual(some_value=5) at with-debug.
  c:10:10
 2
 3typedef int (*debug_callee) (int);
 4
 5extern int no_debug_caller (int, debug_callee);
 6
 7int called_from_nodebug_actual(int some_value) {
 8  int return_value = 0;
 9  return_value = printf("Length: %d.\n", some_value);
  -> 10 return return_value; // Stop here and step out of me
   ^
 11   }
 12   
 13   int called_from_nodebug(int some_value) {
 14 int intermediate_return_value = 0;
 15 intermediate_return_value = called_from_nodebug_actual(some_value);
 16 return intermediate_return_value;
 17   }
 18
  Process 58672 launched: 
'/Volumes/Data/llvm-project/_build.ninja.release/lldb-test-build.noindex/functionalities/step-avoids-no-debug/TestStepNoDebug.test_step_out_with_python_dsym/a.out'
 (arm64)
  (lldb) s
  Process 58672 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = step in
  frame #0: 0x00013d8c 
a.out`called_from_nodebug_actual(some_value=5) at with-debug.c:10:3
 2
 3typedef int (*debug_callee) (int);
 4
 5extern int no_debug_caller (int, debug_callee);
 6
 7int called_from_nodebug_actual(int some_value) {
 8  int return_value = 0;
 9  return_value = printf("Length: %d.\n", some_value);
  -> 10 return return_value; // Stop here and step out of me
^
 11   }
 12   


https://reviews.llvm.org/D152560

Files:
  lldb/test/API/functionalities/step-avoids-no-debug/TestStepNoDebug.py
  lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c
  lldb/test/API/functionalities/step-avoids-no-debug/without-debug.c

Index: lldb/test/API/functionalities/step-avoids-no-debug/without-debug.c
===
--- lldb/test/API/functionalities/step-avoids-no-debug/without-debug.c
+++ lldb/test/API/functionalities/step-avoids-no-debug/without-debug.c
@@ -1,16 +1,12 @@
-typedef int (*debug_callee) (int);
+typedef int (*debug_callee)(int);
 
-int
-no_debug_caller_intermediate(int input, debug_callee callee)
-{
+int no_debug_caller_intermediate(int input, debug_callee callee) {
   int return_value = 0;
   return_value = callee(input);
   return return_value;
 }
 
-int
-no_debug_caller (int input, debug_callee callee)
-{
+int no_debug_caller(int input, debug_callee callee) {
   int return_value = 0;
   return_value = no_debug_caller_intermediate (input, callee);
   return return_value;
Index: lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c
===
--- lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c
+++ lldb/test/API/functionalities/step-avoids-no-debug/with-debug.c
@@ -4,26 +4,20 @@
 
 extern int no_debug_caller (int, debug_callee);
 
-int
-called_from_nodebug_actual(int some_value)
-{
+int called_from_nodebug_actual(int some_value) {
   int return_value = 0;
-  return_value  = printf ("Length: %d.\n", some_value);
+  return_value = printf("Length: %d.\n", some_value);
   return return_value; // Stop here and step out of me
 }
 
-int
-called_from_nodebug(int some_value)
-{
+int called_from_nodebug(int some_value) {
   int intermediate_return_value = 0;
   intermediate_return_value = called_from_nodebug_actual(some_value);
   return intermediate_return_value;
 }
 
-int
-main()
-{
+int main() {
   int return_value = no_debug_caller(5, called_from_nodebug);
-  printf ("I got: %d.\n", return_value);
+  printf("I got: %d.\n", return_value);
   return 0;
 }
Index: lldb/test/API/functionalities/step-avoids-no-debug/TestStepNoDebug.py
===
--- lldb/test/API/functionalities/step-avoids-no-debug/TestStepNoDebug.py
+++ lldb/test/API/functionalities/step-avoids-no-debug/TestStepNoDebug.py
@@ -106,9 +106,9 @@
 self.thread = threads[0]
 
 def do_step_out_past_nodebug(self):
-# The first step out takes us to the called_from_nodebug frame, just to make sure setting
-# step-out-avoid-nodebug doesn't change the behavior in frames with
-# debug info.
+# The first step out takes us to the called_from_nodebug
+# frame, just to make sure setting step-out-avoid-nodebug
+# doesn't change the behavior in frames with debug info.
 self.thread.StepOut()
 self.hit_correct_line(
 

[Lldb-commits] [PATCH] D152476: [lldb] Remove lldb's DWARFAbbreviationDeclarationSet in favor of llvm's

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.

Nice!




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:61
 void DWARFDebugAbbrev::GetUnsupportedForms(
 std::set _forms) const {
+  for (const auto  : m_abbrevCollMap) {

at some point we could modernize this into
```
llvm::DenseSet DWARFDebugAbbrev::GetUnsupportedForms()
```



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:62
 std::set _forms) const {
-  for (const auto  : m_abbrevCollMap)
-pair.second.GetUnsupportedForms(invalid_forms);
+  for (const auto  : m_abbrevCollMap) {
+for (const auto  : pair.second) {

nit: LLVM coding style would probably elide the {}



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h:23
 
 typedef std::map
 DWARFAbbreviationDeclarationCollMap;

for a later commit: this should probably be a DenseMap for better performance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152476

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

[Lldb-commits] [PATCH] D152409: [lldb] Never print children if the max depth has been reached

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Would that be testable by implementing a python data formatter for a C struct 
that unconditionally returns an error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152409

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

[Lldb-commits] [PATCH] D151950: [lldb] Unconditionally increment depth when printing children

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.



Comment at: lldb/source/DataFormatters/ValueObjectPrinter.cpp:610
 
   if (child_sp.get()) {
 ValueObjectPrinter child_printer(

I would probably factor out:
```
if (does_consume_ptr_depth)
   --curr_ptr_depth;
```



Comment at: lldb/test/API/lang/cpp/frame-var-depth-and-elem-count/main.cpp:15
+  C *c = new C[5];
+  return 0; // break here
+}

Setting a breakpoint on the return is risky because there may not be any code 
associated with it. Something like
```
int puts(const char *s);

puts("break here");
```
is safer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151950

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, 
otherwise it seems to needlessly complicate things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152319: [lldb] Run crashlog inside lldb when invoked in interactive mode from the command line

[Adrian Prantl via Phabricator via lldb-commits]

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

This is super convenient! Thanks!


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

https://reviews.llvm.org/D152319

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

[Lldb-commits] [PATCH] D152315: [lldb][NFCI] Refactor TypeSystemClang::GetBasicTypeEnumeration

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:868
+lldb::BasicType TypeSystemClang::GetBasicTypeEnumeration(llvm::StringRef name) 
{
+  if (name.empty())
+return eBasicTypeInvalid;

bulbazord wrote:
> aprantl wrote:
> > Isn't this redundant?
> Technically yes. My reasoning is that it's faster to see if a StringRef is 
> empty than to perform a lookup in a hash map. 
True, but this adds extra code to the binary that in practice will 
(presumably?) never get executed. Or are we often looking up empty types? (It's 
not a big deal either way, but since we are being pedantic :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152315

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

[Lldb-commits] [PATCH] D152315: [lldb][NFCI] Refactor TypeSystemClang::GetBasicTypeEnumeration

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:868
+lldb::BasicType TypeSystemClang::GetBasicTypeEnumeration(llvm::StringRef name) 
{
+  if (name.empty())
+return eBasicTypeInvalid;

Isn't this redundant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152315

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

[Lldb-commits] [PATCH] D152225: [lldb] fix dangling reference in `ClangHost.cpp`

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Thanks!

FWIW, it seems like an unnecessary micro optimization to make both these 
variables static.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152225

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

[Lldb-commits] [PATCH] D152225: [lldb] fix dangling reference in `ClangHost.cpp`

[Adrian Prantl via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6c02e365711c: [lldb] fix dangling reference in 
`ClangHost.cpp` (authored by paperchalice, committed by aprantl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152225

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -52,7 +52,7 @@
   Log *log = GetLog(LLDBLog::Host);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
-  const std::string clang_resource_path =
+  static const std::string clang_resource_path =
   clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 
   static const llvm::StringRef kResourceDirSuffixes[] = {


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -52,7 +52,7 @@
   Log *log = GetLog(LLDBLog::Host);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
-  const std::string clang_resource_path =
+  static const std::string clang_resource_path =
   clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 
   static const llvm::StringRef kResourceDirSuffixes[] = {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151950: [lldb] Unconditionally increment depth when printing children

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Is ity possible to test this with a nested C++ type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151950

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

[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:889
+"Package unit with a non-zero abbreviation offset");
+
+  auto *unit_contrib = index_entry->getContribution();

I know this was also in the original code, so you may not know either: Should 
these error messages be limited to dwarf packages (.dwp) or should they be more 
generic?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:980
+  entry = index.getFromOffset(expected_header->GetOffset());
+if (entry) {
+  if (llvm::Error err = expected_header->ApplyIndexEntry(entry)) {

llvm coding style would delete most of these {} ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151919

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

[Lldb-commits] [PATCH] D151351: [lldb] Prevent dwim-print from showing kNoResult error

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:1
 //===-- CommandObjectDWIMPrint.cpp --*- C++ 
-*-===//
 //

nit: C++ marker is only needed in a .h file ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151351

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

[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

If we can come up with a counterexample where the heuristic in this patch is 
doing the wrong thin then I think emitting a DW_AT_LLVM_no_unique_address 
attribute sounds reasonable to me. But otherwise I don't see a problem with 
using a heuristic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143347

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

[Lldb-commits] [PATCH] D151588: Factor out xcrun (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

In D151588#4377128 , @bulbazord wrote:

> What is the motivation behind this change?
>
> Also, why did you factor out `--show-sdk-path` into an argument? I assume you 
> want to use `xcrun` for other purposes?

This is in preparation for https://reviews.llvm.org/D151588


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

https://reviews.llvm.org/D151588

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

[Lldb-commits] [PATCH] D151591: HostInfoMacOS: Add a utility function for finding an SDK-specific tool

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: bulbazord, JDevlieghere.
Herald added a project: All.
aprantl requested review of this revision.

This is an API needed by swift-lldb.


https://reviews.llvm.org/D151591

Files:
  lldb/include/lldb/Host/HostInfoBase.h
  lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
  lldb/unittests/Host/HostInfoTest.cpp

Index: lldb/unittests/Host/HostInfoTest.cpp
===
--- lldb/unittests/Host/HostInfoTest.cpp
+++ lldb/unittests/Host/HostInfoTest.cpp
@@ -72,6 +72,23 @@
   // This is expected to fail.
   EXPECT_TRUE(get_sdk("CeciNestPasUnOS.sdk", true).empty());
 }
+
+TEST_F(HostInfoTest, FindSDKTool) {
+  auto find_tool = [](std::string sdk, llvm::StringRef tool,
+  bool error = false) -> llvm::StringRef {
+auto sdk_path_or_err =
+HostInfo::FindSDKTool(XcodeSDK(std::move(sdk)), tool);
+if (!error) {
+  EXPECT_TRUE((bool)sdk_path_or_err);
+  return *sdk_path_or_err;
+}
+EXPECT_FALSE((bool)sdk_path_or_err);
+llvm::consumeError(sdk_path_or_err.takeError());
+return {};
+  };
+  EXPECT_FALSE(find_tool("MacOSX.sdk", "clang").empty());
+  EXPECT_TRUE(find_tool("MacOSX.sdk", "CeciNestPasUnOutil").empty());
+}
 #endif
 
 TEST(HostInfoTestInitialization, InitTwice) {
Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -522,36 +522,65 @@
   return path;
 }
 
-llvm::Expected HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
-  struct ErrorOrPath {
-std::string str;
-bool is_error;
-  };
-  static llvm::StringMap g_sdk_path;
-  static std::mutex g_sdk_path_mutex;
+namespace {
+struct ErrorOrPath {
+  std::string str;
+  bool is_error;
+};
+} // namespace
 
-  std::lock_guard guard(g_sdk_path_mutex);
+static llvm::Expected
+find_cached_path(llvm::StringMap , std::mutex ,
+ llvm::StringRef key,
+ std::function(void)> compute) {
+  std::lock_guard guard(mutex);
   LLDB_SCOPED_TIMER();
 
-  auto key = sdk.GetString();
-  auto it = g_sdk_path.find(key);
-  if (it != g_sdk_path.end()) {
+  auto it = cache.find(key);
+  if (it != cache.end()) {
 if (it->second.is_error)
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
  it->second.str);
-else
-  return it->second.str;
+return it->second.str;
   }
-  auto path_or_err = GetXcodeSDK(sdk);
+  auto path_or_err = compute();
   if (!path_or_err) {
 std::string error = toString(path_or_err.takeError());
-g_sdk_path.insert({key, {error, true}});
+cache.insert({key, {error, true}});
 return llvm::createStringError(llvm::inconvertibleErrorCode(), error);
   }
-  auto it_new = g_sdk_path.insert({key, {*path_or_err, false}});
+  auto it_new = cache.insert({key, {*path_or_err, false}});
   return it_new.first->second.str;
 }
 
+llvm::Expected HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+  static llvm::StringMap g_sdk_path;
+  static std::mutex g_sdk_path_mutex;
+  auto key = sdk.GetString();
+  return find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&](){
+return GetXcodeSDK(sdk);
+  });
+}
+
+llvm::Expected
+HostInfoMacOSX::FindSDKTool(XcodeSDK sdk, llvm::StringRef tool) {
+  static llvm::StringMap g_tool_path;
+  static std::mutex g_tool_path_mutex;
+  std::string key;
+  llvm::raw_string_ostream(key) << sdk.GetString() << ":" << tool;
+  return find_cached_path(
+  g_tool_path, g_tool_path_mutex, key,
+  [&]() -> llvm::Expected {
+std::string sdk_name = XcodeSDK::GetCanonicalName(sdk.Parse());
+if (sdk_name.empty())
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Unrecognized SDK type: " +
+ sdk.GetString());
+llvm::SmallVector find = {"-find", tool};
+return xcrun(sdk_name, find);
+  });
+}
+
 namespace {
 struct dyld_shared_cache_dylib_text_info {
   uint64_t version; // current version 1
Index: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
===
--- lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -12,6 +12,7 @@
 #include "lldb/Host/posix/HostInfoPosix.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/XcodeSDK.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/VersionTuple.h"
 #include 
 
@@ -30,8 +31,9 @@
   static FileSpec GetXcodeContentsDirectory();
   static FileSpec GetXcodeDeveloperDirectory();
 
-  /// Query xcrun to find an Xcode SDK directory.
   static llvm::Expected GetXcodeSDKPath(XcodeSDK sdk);
+  static llvm::Expected FindSDKTool(XcodeSDK sdk,
+

[Lldb-commits] [PATCH] D151588: Factor out xcrun (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 526194.

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

https://reviews.llvm.org/D151588

Files:
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -373,84 +373,86 @@
   return g_developer_directory;
 }
 
-llvm::Expected GetXcodeSDK(XcodeSDK sdk) {
-  XcodeSDK::Info info = sdk.Parse();
-  std::string sdk_name = XcodeSDK::GetCanonicalName(info);
-  if (sdk_name.empty())
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Unrecognized SDK type: " + sdk.GetString());
+static llvm::Expected
+xcrun(const std::string , llvm::ArrayRef arguments,
+  llvm::StringRef developer_dir = "") {
+  Args args;
+  if (!developer_dir.empty()) {
+args.AppendArgument("/usr/bin/env");
+args.AppendArgument("DEVELOPER_DIR=" + developer_dir.str());
+  }
+  args.AppendArgument("/usr/bin/xcrun");
+  args.AppendArgument("--sdk");
+  args.AppendArgument(sdk);
+  for (auto arg: arguments)
+args.AppendArgument(arg);
 
   Log *log = GetLog(LLDBLog::Host);
+  if (log) {
+std::string cmdstr;
+args.GetCommandString(cmdstr);
+log->Printf("GetXcodeSDK() running shell cmd '%s'", cmdstr.c_str());
+  }
 
-  auto xcrun = [](const std::string ,
-  llvm::StringRef developer_dir =
-  "") -> llvm::Expected {
-Args args;
-if (!developer_dir.empty()) {
-  args.AppendArgument("/usr/bin/env");
-  args.AppendArgument("DEVELOPER_DIR=" + developer_dir.str());
-}
-args.AppendArgument("/usr/bin/xcrun");
-args.AppendArgument("--show-sdk-path");
-args.AppendArgument("--sdk");
-args.AppendArgument(sdk);
-
-Log *log = GetLog(LLDBLog::Host);
-if (log) {
-  std::string cmdstr;
-  args.GetCommandString(cmdstr);
-  log->Printf("GetXcodeSDK() running shell cmd '%s'", cmdstr.c_str());
-}
+  int status = 0;
+  int signo = 0;
+  std::string output_str;
+  // The first time after Xcode was updated or freshly installed,
+  // xcrun can take surprisingly long to build up its database.
+  auto timeout = std::chrono::seconds(60);
+  bool run_in_shell = false;
+  lldb_private::Status error = Host::RunShellCommand(
+  args, FileSpec(), , , _str, timeout, run_in_shell);
+
+  // Check that xcrun returned something useful.
+  if (error.Fail()) {
+// Catastrophic error.
+LLDB_LOG(log, "xcrun failed to execute: %s", error.AsCString());
+return error.ToError();
+  }
+  if (status != 0) {
+// xcrun didn't find a matching SDK. Not an error, we'll try
+// different spellings.
+LLDB_LOG(log, "xcrun returned exit code %d", status);
+return "";
+  }
+  if (output_str.empty()) {
+LLDB_LOG(log, "xcrun returned no results");
+return "";
+  }
 
-int status = 0;
-int signo = 0;
-std::string output_str;
-// The first time after Xcode was updated or freshly installed,
-// xcrun can take surprisingly long to build up its database.
-auto timeout = std::chrono::seconds(60);
-bool run_in_shell = false;
-lldb_private::Status error = Host::RunShellCommand(
-args, FileSpec(), , , _str, timeout, run_in_shell);
-
-// Check that xcrun returned something useful.
-if (error.Fail()) {
-  // Catastrophic error.
-  LLDB_LOG(log, "xcrun failed to execute: %s", error.AsCString());
-  return error.ToError();
-}
-if (status != 0) {
-  // xcrun didn't find a matching SDK. Not an error, we'll try
-  // different spellings.
-  LLDB_LOG(log, "xcrun returned exit code %d", status);
-  return "";
-}
-if (output_str.empty()) {
-  LLDB_LOG(log, "xcrun returned no results");
-  return "";
-}
+  // Convert to a StringRef so we can manipulate the string without modifying
+  // the underlying data.
+  llvm::StringRef output(output_str);
 
-// Convert to a StringRef so we can manipulate the string without modifying
-// the underlying data.
-llvm::StringRef output(output_str);
+  // Remove any trailing newline characters.
+  output = output.rtrim();
 
-// Remove any trailing newline characters.
-output = output.rtrim();
+  // Strip any leading newline characters and everything before them.
+  const size_t last_newline = output.rfind('\n');
+  if (last_newline != llvm::StringRef::npos)
+output = output.substr(last_newline + 1);
 
-// Strip any leading newline characters and everything before them.
-const size_t last_newline = output.rfind('\n');
-if (last_newline != llvm::StringRef::npos)
-  output = output.substr(last_newline + 1);
+  return output.str();
+}
 
-return output.str();
-  };
+static llvm::Expected GetXcodeSDK(XcodeSDK sdk) {
+  XcodeSDK::Info info = sdk.Parse();
+  

[Lldb-commits] [PATCH] D151588: Factor out xcrun (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: JDevlieghere, bulbazord.
Herald added a project: All.
aprantl requested review of this revision.

https://reviews.llvm.org/D151588

Files:
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -373,84 +373,86 @@
   return g_developer_directory;
 }
 
-llvm::Expected GetXcodeSDK(XcodeSDK sdk) {
-  XcodeSDK::Info info = sdk.Parse();
-  std::string sdk_name = XcodeSDK::GetCanonicalName(info);
-  if (sdk_name.empty())
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Unrecognized SDK type: " + sdk.GetString());
+static llvm::Expected
+xcrun(const std::string , llvm::ArrayRef arguments,
+  llvm::StringRef developer_dir = "") {
+  Args args;
+  if (!developer_dir.empty()) {
+args.AppendArgument("/usr/bin/env");
+args.AppendArgument("DEVELOPER_DIR=" + developer_dir.str());
+  }
+  args.AppendArgument("/usr/bin/xcrun");
+  args.AppendArgument("--sdk");
+  args.AppendArgument(sdk);
+  for (auto arg: arguments)
+args.AppendArgument(arg);
 
   Log *log = GetLog(LLDBLog::Host);
+  if (log) {
+std::string cmdstr;
+args.GetCommandString(cmdstr);
+log->Printf("GetXcodeSDK() running shell cmd '%s'", cmdstr.c_str());
+  }
 
-  auto xcrun = [](const std::string ,
-  llvm::StringRef developer_dir =
-  "") -> llvm::Expected {
-Args args;
-if (!developer_dir.empty()) {
-  args.AppendArgument("/usr/bin/env");
-  args.AppendArgument("DEVELOPER_DIR=" + developer_dir.str());
-}
-args.AppendArgument("/usr/bin/xcrun");
-args.AppendArgument("--show-sdk-path");
-args.AppendArgument("--sdk");
-args.AppendArgument(sdk);
-
-Log *log = GetLog(LLDBLog::Host);
-if (log) {
-  std::string cmdstr;
-  args.GetCommandString(cmdstr);
-  log->Printf("GetXcodeSDK() running shell cmd '%s'", cmdstr.c_str());
-}
+  int status = 0;
+  int signo = 0;
+  std::string output_str;
+  // The first time after Xcode was updated or freshly installed,
+  // xcrun can take surprisingly long to build up its database.
+  auto timeout = std::chrono::seconds(60);
+  bool run_in_shell = false;
+  lldb_private::Status error = Host::RunShellCommand(
+  args, FileSpec(), , , _str, timeout, run_in_shell);
+
+  // Check that xcrun returned something useful.
+  if (error.Fail()) {
+// Catastrophic error.
+LLDB_LOG(log, "xcrun failed to execute: %s", error.AsCString());
+return error.ToError();
+  }
+  if (status != 0) {
+// xcrun didn't find a matching SDK. Not an error, we'll try
+// different spellings.
+LLDB_LOG(log, "xcrun returned exit code %d", status);
+return "";
+  }
+  if (output_str.empty()) {
+LLDB_LOG(log, "xcrun returned no results");
+return "";
+  }
 
-int status = 0;
-int signo = 0;
-std::string output_str;
-// The first time after Xcode was updated or freshly installed,
-// xcrun can take surprisingly long to build up its database.
-auto timeout = std::chrono::seconds(60);
-bool run_in_shell = false;
-lldb_private::Status error = Host::RunShellCommand(
-args, FileSpec(), , , _str, timeout, run_in_shell);
-
-// Check that xcrun returned something useful.
-if (error.Fail()) {
-  // Catastrophic error.
-  LLDB_LOG(log, "xcrun failed to execute: %s", error.AsCString());
-  return error.ToError();
-}
-if (status != 0) {
-  // xcrun didn't find a matching SDK. Not an error, we'll try
-  // different spellings.
-  LLDB_LOG(log, "xcrun returned exit code %d", status);
-  return "";
-}
-if (output_str.empty()) {
-  LLDB_LOG(log, "xcrun returned no results");
-  return "";
-}
+  // Convert to a StringRef so we can manipulate the string without modifying
+  // the underlying data.
+  llvm::StringRef output(output_str);
 
-// Convert to a StringRef so we can manipulate the string without modifying
-// the underlying data.
-llvm::StringRef output(output_str);
+  // Remove any trailing newline characters.
+  output = output.rtrim();
 
-// Remove any trailing newline characters.
-output = output.rtrim();
+  // Strip any leading newline characters and everything before them.
+  const size_t last_newline = output.rfind('\n');
+  if (last_newline != llvm::StringRef::npos)
+output = output.substr(last_newline + 1);
 
-// Strip any leading newline characters and everything before them.
-const size_t last_newline = output.rfind('\n');
-if (last_newline != llvm::StringRef::npos)
-  output = output.substr(last_newline + 1);
+  return output.str();
+}
 
-return output.str();
-  };
+static llvm::Expected GetXcodeSDK(XcodeSDK sdk) {
+  

[Lldb-commits] [PATCH] D151366: [lldb] Disable variable watchpoints when going out of scope

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> @aprantl Do you know if we can detect the end of the scope ? I'm not sure 
> it's possible currently ... But even if we could do it, that wouldn't cover 
> the following case:

When setting a variable watchpoint, you would have to store the scope the 
variable is in, and then ignore all hits that are (1) on the same thread but 
(2) where the scope is not any of the lexical scopes in any of the stack 
frames. Because you still want to find modifications in child frames or on 
other threads.


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

https://reviews.llvm.org/D151366

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

[Lldb-commits] [PATCH] D151366: [lldb] Disable variable watchpoints when going out of scope

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Just so I understand the limitations:
This works for

  int main() {
int val = 0;
// Break here
val++;
val++;
return 0;
  }

but not for

  int main() {
{
  int val = 0;
  // Break here
  val++;
  val++;
}
{
  int other = 42;
 printf("assuming that other and val aliases");
}
return 0;
  }

?

To be clear, I think this is still useful!




Comment at: lldb/include/lldb/Breakpoint/Watchpoint.h:93
 
+  struct WatchpointVariableContext {
+WatchpointVariableContext(lldb::watch_id_t watch_id,

Doxygen comment?



Comment at: lldb/include/lldb/Breakpoint/Watchpoint.h:109
+
+  /// Callback routine which disable the watchpoint set on a variable when it
+  /// goes out of scope.

s/which/to/



Comment at: lldb/source/Breakpoint/Watchpoint.cpp:86
 
+bool Watchpoint::SetupVariableWatchpointDisabler(StackFrameSP frame_sp) const {
+  ThreadSP thread_sp = frame_sp->GetThread();

bulbazord wrote:
> Should you also verify that the `frame_sp` you got isn't `nullptr`? If it'll 
> never be null, consider passing a `StackFrame &` instead.
I subjectively feel like I've received a crash log for every single unchecked 
dereference in the LLDB code so far ;-)



Comment at: lldb/source/Commands/CommandObjectWatchpoint.cpp:973
 } else {
   result.AppendErrorWithFormat(
   "Watchpoint creation failed (addr=0x%" PRIx64 ", size=%" PRIu64

Convert to early exit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151366

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

bulbazord wrote:
> aprantl wrote:
> > Using an llvm string stream to construct the string might be faster here.
> I tested this and found that `llvm::SmallString<128>` performs about as well 
> as `std::string` on average. `llvm::SmallString<64>` was on average slower 
> though.
> 
> I think I'm going to land this with std::string, we can revisit later if the 
> difference actually is measurable.
Maybe we were talking past each other: I was thinking about 
https://www.llvm.org/doxygen/classllvm_1_1raw__string__ostream.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:56
   if (m_idx_offset == UINT32_MAX) {
-DWARFAbbreviationDeclarationCollConstIter pos;
-DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
-for (pos = m_decls.begin(); pos != end; ++pos) {
-  if (pos->Code() == abbrCode)
-return &(*pos);
+for (const auto  : m_decls) {
+  if (decl.getCode() == abbrCode)

bulbazord wrote:
> aprantl wrote:
> > would std::find_if be shorter or would it look worse?
> ```
> for (const auto  : m_decls) {
>   if (decl.getCode() == abbrCode)
> return 
> }
> ```
> vs.
> ```
> auto pos = std::find_if(
>  m_decls.begin(), m_decls.end(),
>  [abbrCode](const auto ) { return decl.getCode() == abbrCode; });
> if (pos != m_decls.end())
>   return &*pos;
> ```
> 
> I think it would look worse, personally.
Agreed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

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

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Adrian Prantl via Phabricator via lldb-commits]

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

Overall seems good to me. Minor comments inline.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:27
  lldb::offset_t *offset_ptr) {
+  llvm::DataExtractor llvm_data = data.GetAsLLVM();
   const lldb::offset_t begin_offset = *offset_ptr;

Is this intentionally a copy or should this be a reference? I have no idea how 
heavyweight this object is...



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:56
   if (m_idx_offset == UINT32_MAX) {
-DWARFAbbreviationDeclarationCollConstIter pos;
-DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
-for (pos = m_decls.begin(); pos != end; ++pos) {
-  if (pos->Code() == abbrCode)
-return &(*pos);
+for (const auto  : m_decls) {
+  if (decl.getCode() == abbrCode)

would std::find_if be shorter or would it look worse?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h:18
 
+#include "llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h"
+

Nit: we usually do local includes first and then the stdlib at the bottom.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Adrian Prantl via Phabricator via lldb-commits]

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

One more comment inline but otherwise this is fine with me.




Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp:155
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
+

Using an llvm string stream to construct the string might be faster here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D150590: [lldb][DWARFASTParserClang][NFC] Extract condition for unnamed bitfield creation into helper function

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h:239
 
+  bool ShouldCreateUnnamedBitfield(
+  FieldInfo const _field_info, uint64_t last_field_end,

Doxygen comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150590

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

[Lldb-commits] [PATCH] D150383: [lldb] Correct elision of line zero in mixed disassembly

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/test/Shell/Commands/command-disassemble-mixed.test:12
+
+// CHECK-NOT: do_not_show

Can you add some positive check too? Otherwise this test succeeds even on an 
empty output..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150383

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

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I'm not opposed to using this implementation, but have you considered using 
something like the stdlib regex library to do the heavy lifting?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp:203
+.Case(".debug_pubnames", eSectionTypeDWARFDebugPubNames)
+.Case(".debug_pubtypes", eSectionTypeDWARFDebugPubTypes)
+.Case(".debug_str", eSectionTypeDWARFDebugStr)

Can these be correct? They seem too long.



Comment at: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h:16
+
+class ObjectFileCOFF : public lldb_private::ObjectFile {
+  std::unique_ptr m_object;

Can you add a short doxygen comment?



Comment at: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h:86
+  bool IsStripped() override {
+// FIXME(compnerd) see if there is a good way to identify a /Z7 v /Zi or 
/ZI
+// build.

We usually don't sign our FIXMEs :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Is there a way to compile something with clang and then load the object file 
into lldb-test so we could add a shell test for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Could you build this into a yaml2obj obj2yaml plugin, so we could test this?
Alternatively, you could check in a minimal binary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

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

[Lldb-commits] [PATCH] D149949: [lldb][TypeSystem] ForEach: Don't hold the TypeSystemMap lock across callback

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Do we have other ForEach methods that contain a similar footgun?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149949

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

[Lldb-commits] [PATCH] D149900: [lldb] Expose a const iterator for SymbolContextList

[Adrian Prantl via Phabricator via lldb-commits]

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

Please do more of this!
I haven't checked all the replacements for correctness but the direction is 
good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149900

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

[Lldb-commits] [PATCH] D149702: [DebugInfo] Add language code for the new Mojo language

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

You can also use a constant in the Vendor extension space, which might be more 
appropriate until the languages sees adoption by a wider audience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149702

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

[Lldb-commits] [PATCH] D149702: [DebugInfo] Add language code for the new Mojo language

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/lldb-enumerations.h:493
   eLanguageTypeAda2012 = 0x002f,
+  eLanguageTypeMojo = 0x0030,
 

bulbazord wrote:
> These values correspond to DWARF5's official language codes and `0x0030` is 
> technically already taken. LLDB just hasn't been updated yet. I don't think 
> this should necessarily block this patch but this value will need to be 
> changed at some point.
> 
> See: https://dwarfstd.org/languages.html
Registering a new language with the DWARF committee is a pretty quick process 
nowadays. Please do that before taking a constant in the reserved range.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149702

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

[Lldb-commits] [PATCH] D147370: [lldb] fixing #61727 fixing incorrect variable displaying with DW_OP_div

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I'll try adding a `# REQUIRES: lld`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147370

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

[Lldb-commits] [PATCH] D147370: [lldb] fixing #61727 fixing incorrect variable displaying with DW_OP_div

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This test is failing on Darwin: 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/lastFailedBuild/testReport/lldb-shell/SymbolFile_DWARF_x86/DW_OP_div_with_signed_s/

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/lastFailedBuild/

ld: unknown option: --hash-style=gnu
clang: error: linker command failed with exit code 1 (use -v to see invocation)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147370

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

[Lldb-commits] [PATCH] D148662: [lldb] Make the libcxx unique_ptr prettyprinter support custom deleters.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Thanks for the quick response!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148662

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

[Lldb-commits] [PATCH] D148662: [lldb] Make the libcxx unique_ptr prettyprinter support custom deleters.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Looks like this broke on the Darwin incremental bot:

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/54508/

Can you please take a look and revert in the mean time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148662

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

[Lldb-commits] [PATCH] D149214: [lldb] Speed up DebugAbbrev parsing

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Did you also measure the extra memory consumption? I would be surprised if this 
mattered, but we do parse a lot of DWARF DIEs...

Generally this seems fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149214

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

[Lldb-commits] [PATCH] D148846: [lldb] Let Mangled decide whether a name is mangled or not

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py:7
+
+
+class TestFunctionNameWithoutArgs(TestBase):

I guess this is a no debug info test?


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

https://reviews.llvm.org/D148846

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

[Lldb-commits] [PATCH] D148846: [lldb] Let Mangled decide whether a name is mangled or not

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py:1
+import os
+import lldb

why?



Comment at: lldb/test/API/macosx/format/main.c:2
+#include 
+#include 
+

why?


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

https://reviews.llvm.org/D148846

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

[Lldb-commits] [PATCH] D148846: [lldb] Let Mangled decide whether a name is mangled or not

[Adrian Prantl via Phabricator via lldb-commits]

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

It seems like we can quickly and unambiguously decide whether a name is mangled 
or not by looking at the first characters so that seems fine to me. Does 
Mangled try all Language plugins to decide?
This seems like a risky change, but I think it's going into the right direction.


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

https://reviews.llvm.org/D148846

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

[Lldb-commits] [PATCH] D148823: Make diagnostics API safer to use

[Adrian Prantl via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG737820e6d6e2: Make diagnostics API safer to use (authored by 
aprantl).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148823

Files:
  lldb/include/lldb/Expression/DiagnosticManager.h
  lldb/unittests/Expression/DiagnosticManagerTest.cpp


Index: lldb/unittests/Expression/DiagnosticManagerTest.cpp
===
--- lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -75,6 +75,9 @@
 TEST(DiagnosticManagerTest, GetStringNoDiags) {
   DiagnosticManager mgr;
   EXPECT_EQ("", mgr.GetString());
+  std::unique_ptr empty;
+  mgr.AddDiagnostic(std::move(empty));
+  EXPECT_EQ("", mgr.GetString());
 }
 
 TEST(DiagnosticManagerTest, GetStringBasic) {
Index: lldb/include/lldb/Expression/DiagnosticManager.h
===
--- lldb/include/lldb/Expression/DiagnosticManager.h
+++ lldb/include/lldb/Expression/DiagnosticManager.h
@@ -114,7 +114,8 @@
   }
 
   void AddDiagnostic(std::unique_ptr diagnostic) {
-m_diagnostics.push_back(std::move(diagnostic));
+if (diagnostic)
+  m_diagnostics.push_back(std::move(diagnostic));
   }
 
   size_t Printf(DiagnosticSeverity severity, const char *format, ...)


Index: lldb/unittests/Expression/DiagnosticManagerTest.cpp
===
--- lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -75,6 +75,9 @@
 TEST(DiagnosticManagerTest, GetStringNoDiags) {
   DiagnosticManager mgr;
   EXPECT_EQ("", mgr.GetString());
+  std::unique_ptr empty;
+  mgr.AddDiagnostic(std::move(empty));
+  EXPECT_EQ("", mgr.GetString());
 }
 
 TEST(DiagnosticManagerTest, GetStringBasic) {
Index: lldb/include/lldb/Expression/DiagnosticManager.h
===
--- lldb/include/lldb/Expression/DiagnosticManager.h
+++ lldb/include/lldb/Expression/DiagnosticManager.h
@@ -114,7 +114,8 @@
   }
 
   void AddDiagnostic(std::unique_ptr diagnostic) {
-m_diagnostics.push_back(std::move(diagnostic));
+if (diagnostic)
+  m_diagnostics.push_back(std::move(diagnostic));
   }
 
   size_t Printf(DiagnosticSeverity severity, const char *format, ...)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148823: Make diagnostics API safer to use

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: kastiglione, JDevlieghere, Michael137.
Herald added a project: All.
aprantl requested review of this revision.

I received a crash report in DiagnosticManager that was causes by a nullptr 
diagnostic having been added. The API allows passing in a null unique_ptr, but 
all the methods are written assuming that all pointers a dereferencable. This 
patch makes it impossible to add a null diagnostic.

rdar://107633615


https://reviews.llvm.org/D148823

Files:
  lldb/include/lldb/Expression/DiagnosticManager.h
  lldb/unittests/Expression/DiagnosticManagerTest.cpp


Index: lldb/unittests/Expression/DiagnosticManagerTest.cpp
===
--- lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -75,6 +75,9 @@
 TEST(DiagnosticManagerTest, GetStringNoDiags) {
   DiagnosticManager mgr;
   EXPECT_EQ("", mgr.GetString());
+  std::unique_ptr empty;
+  mgr.AddDiagnostic(std::move(empty));
+  EXPECT_EQ("", mgr.GetString());
 }
 
 TEST(DiagnosticManagerTest, GetStringBasic) {
Index: lldb/include/lldb/Expression/DiagnosticManager.h
===
--- lldb/include/lldb/Expression/DiagnosticManager.h
+++ lldb/include/lldb/Expression/DiagnosticManager.h
@@ -114,7 +114,8 @@
   }
 
   void AddDiagnostic(std::unique_ptr diagnostic) {
-m_diagnostics.push_back(std::move(diagnostic));
+if (diagnostic)
+  m_diagnostics.push_back(std::move(diagnostic));
   }
 
   size_t Printf(DiagnosticSeverity severity, const char *format, ...)


Index: lldb/unittests/Expression/DiagnosticManagerTest.cpp
===
--- lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -75,6 +75,9 @@
 TEST(DiagnosticManagerTest, GetStringNoDiags) {
   DiagnosticManager mgr;
   EXPECT_EQ("", mgr.GetString());
+  std::unique_ptr empty;
+  mgr.AddDiagnostic(std::move(empty));
+  EXPECT_EQ("", mgr.GetString());
 }
 
 TEST(DiagnosticManagerTest, GetStringBasic) {
Index: lldb/include/lldb/Expression/DiagnosticManager.h
===
--- lldb/include/lldb/Expression/DiagnosticManager.h
+++ lldb/include/lldb/Expression/DiagnosticManager.h
@@ -114,7 +114,8 @@
   }
 
   void AddDiagnostic(std::unique_ptr diagnostic) {
-m_diagnostics.push_back(std::move(diagnostic));
+if (diagnostic)
+  m_diagnostics.push_back(std::move(diagnostic));
   }
 
   size_t Printf(DiagnosticSeverity severity, const char *format, ...)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148175: [lldb] Add `operator StringRef` to ConstString

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.

Okay, then let the refactor fest begin!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148175

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

[Lldb-commits] [PATCH] D148175: [lldb] Add `operator StringRef` to ConstString

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I understand that the opposite direction is explicit because actual work is 
being done. In this direction, it shouldn't affect memory management (since 
ConstStrings live forever) or performance, so I think this is good (and very 
convenient!).
Does anyone else see a problem with this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148175

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

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/Symbol/Function.h:439
   /// The section offset based address for this function.
+  /// \param[in] generic_trampoline
+  /// If this function is a generic trampoline. A generic trampoline 

Is the "generic" qualifier necessary here? If we later add support for 
trampolines with a jump target maybe, but without that this seems to just 
create opportunity for confusion, particularly for Swift where the word 
"generic" has very different connotations.



Comment at: lldb/source/Target/ThreadPlanStepThroughGenericTrampoline.cpp:2
+//===-- ThreadPlanStepThroughGenericTrampoline.cpp
+//-===//
+//

formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

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

[Lldb-commits] [PATCH] D147436: [lldb][ClangExpression] Filter out non-root namespaces in FindNamespace

[Adrian Prantl via Phabricator via lldb-commits]

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

So the new bool flag effectively implements the leading `::` separator in the 
lookup. Seems reasonable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147436

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

[Lldb-commits] [PATCH] D147362: [lldb] Add Clang module import logging

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This looks great, and I think this should also be easy to test? I think we have 
many other tests that write out a logfile and FileCheck the result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147362

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

[Lldb-commits] [PATCH] D147385: [lldb] Add fixits for "frame variable"

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

That looks like a nice quality of life improvement! Personally I would lean 
towards replicating the expr behavior, though I don't know how much work that 
would be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147385

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

[Lldb-commits] [PATCH] D143061: [lldb][Language] Add more language types

[Adrian Prantl via Phabricator via lldb-commits]

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

Well, I guess we'll have to revisit this for DWARF 6 split of language names 
and versions anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143061

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

[Lldb-commits] [PATCH] D147436: [lldb][ClangExpression] Filter out non-root namespaces in FindNamespace

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Just out of curiosity:

  namespace A {
  namespace B {
  namespace C {
  struct Bar {};
  }
  }
  }
  
  namespace B {
  namespace C {
  struct Foo {};
  }
  }

Does this work?

  (lldb) expr C::Foo f




Comment at: lldb/include/lldb/Symbol/SymbolFile.h:333
 return CompilerDeclContext();
   }
 

This looks like a great opportunity to add a Doxygen comment in the base class!



Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:704
+found_namespace_decl = symbol_file->FindNamespace(
+name, namespace_decl, /* only root namespaces */ true);
 

Maybe comment why?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2337
+return !only_root_namespaces ||
+   die.GetParent().Tag() == dwarf::DW_TAG_compile_unit;
 

I find this condition with its double negative really hard to understand. Would 
it be easier to read as
```
if (!decl_ctx.IsValid()) {
if (only_root_namespaces)
  return die.GetParent().Tag() == dwarf::DW_TAG_compile_unit;
   return true;
}
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147436

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

[Lldb-commits] [PATCH] D147012: [lldb] Support Universal Mach-O binaries with a fat64 header

[Adrian Prantl via Phabricator via lldb-commits]

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

Thanks for checking!


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

https://reviews.llvm.org/D147012

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

[Lldb-commits] [PATCH] D143062: [lldb] Allow evaluating expressions in C++20 mode

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Target/Language.cpp:272
+  case eLanguageTypeC_plus_plus_17:
+  case eLanguageTypeC_plus_plus_20:
   case eLanguageTypeObjC_plus_plus:

Can you check if we already have a similar function in Dwarf.h in LLVM?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143062

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

[Lldb-commits] [PATCH] D147012: [lldb] Support Universal Mach-O binaries with a fat64 header

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: 
lldb/source/Plugins/ObjectContainer/Universal-Mach-O/ObjectContainerUniversalMachO.cpp:205
+const lldb::offset_t slice_file_offset =
+fat_arch.GetOffset() + file_offset;
+if (fat_arch.GetOffset() < file_size && file_size > slice_file_offset) 
{

Should this use AddOverflow?
https://llvm.org/doxygen/MathExtras_8h.html



Comment at: lldb/test/API/macosx/universal64/TestUniversal64.py:21
+# Make sure the binary and the dSYM are in the image list.
+self.expect("image list ", patterns=['fat.out', 'fat.out.dSYM'])
+

would it be easy to verify in the test or the makefile that the dsym indeed has 
a fat header?


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

https://reviews.llvm.org/D147012

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> breaks basically anything that examines debugger output

I'd be curious to learn about some example you have in mind here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Core/DumpRegisterValue.cpp:136-137
+  // See if we have made this type before and can reuse it.
+  CompilerType fields_type = ast->GetTypeForIdentifier(
+  ConstString(register_type_name.c_str()));
+

DavidSpickett wrote:
> aprantl wrote:
> > DavidSpickett wrote:
> > > aprantl wrote:
> > > > bulbazord wrote:
> > > > > This seems highly specific to C++... Let's try to find another way to 
> > > > > do this, ideally with `TypeSystem` instead of `TypeSystemClang` and 
> > > > > `clang::CXXRecordDecl`.
> > > > Are we saving a lot of code by going through the clang typesystem here, 
> > > > or would walking the bits and formatting the directly be roughly the 
> > > > same amount of code?
> > > Funny you should mention that.
> > > 
> > > I was initially doing that, but in the RFC it was suggested I use the 
> > > existing printers for C types 
> > > (https://discourse.llvm.org/t/rfc-showing-register-fields-in-lldb/64676/2).
> > >  There's also a hint that this would make assigning to specific fields 
> > > more easy (though that is a very far off goal).
> > > 
> > > This is what the first version looked like:
> > > ```
> > > (lldb) register read cpsr
> > > cpsr = 0x60001000
> > > | N | Z | C | V | TCO | DIT | UAO | PAN | SS | IL | SSBS | BTYPE | D | A 
> > > | I | F | nRW | EL | SP |
> > > | 0 | 1 | 1 | 0 |  0  |  0  |  0  |  0  | 0  | 0  |  1   |   0   | 0 | 0 
> > > | 0 | 0 |  0  | 0  | 0  |
> > > ```
> > > Of course it would be refined based on line length and field size etc 
> > > etc. A lot of the things the type printers already do, which is why using 
> > > them looked like it would save some effort.
> > > 
> > > Some of that extra formatting code will get written anyway because I want 
> > > to eventually add a register command that will show you the register 
> > > layout. So table formatting is needed either way. That would produce 
> > > something like:
> > > ```
> > > (lldb) register info cpsr
> > > | 31 | 30 | ...
> > > | N  |  Z | ...
> > > ```
> > > 
> > > So overall no there's nothing preventing me from walking the bits. 
> > > Assuming you already have code to format text tables nicely, it would be 
> > > about the same amount of code.
> > > 
> > > I need to check if we have any generic table formatting code, or could 
> > > extract some from somewhere.
> > Sorry I wasn't aware of the history :-)
> Do you have an idea of how much work it would be to make TypeSystem do the 
> same things TypeSystemClang does? Maybe more to the point, would I be trying 
> to make TypeSystem do something it just isn't designed to do.
Fundamentally, creating new types is something so tightly coupled to a specific 
type system that it may not be a good idea to expose this through the generic 
TypeSystem interface.

We could design a generic interface that looks like 
```
CompilerType ty = CreateRecordType({{"field1", CompilerType1}, {"field2", ...}})
```
but here you explicitly want to control packing and bitfields, and so you will 
end up with something that most likely only TypeSystemClang can fulfill anyway.

So the better solution may be along the lines of a register formatting plugin 
that depends on TypeSystemClang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

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

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Commands/CommandObjectRegister.cpp:229
+if (!DumpRegister(m_exe_ctx, strm, reg_ctx, reg_info,
+  /*print_flags=*/true, type_system))
   strm.Printf("%-12s = error: unavailable\n", reg_info->name);

Do we need to pass this in explicitly or could the implementation do the 
`Target::GetScratchTypeSystemForLanguage::GetForTarget(m_exe_ctx.GetTargetRef())`?
 That would avoid introducing a plugin dependence at least in this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

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

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Core/DumpRegisterValue.cpp:136-137
+  // See if we have made this type before and can reuse it.
+  CompilerType fields_type = ast->GetTypeForIdentifier(
+  ConstString(register_type_name.c_str()));
+

DavidSpickett wrote:
> aprantl wrote:
> > bulbazord wrote:
> > > This seems highly specific to C++... Let's try to find another way to do 
> > > this, ideally with `TypeSystem` instead of `TypeSystemClang` and 
> > > `clang::CXXRecordDecl`.
> > Are we saving a lot of code by going through the clang typesystem here, or 
> > would walking the bits and formatting the directly be roughly the same 
> > amount of code?
> Funny you should mention that.
> 
> I was initially doing that, but in the RFC it was suggested I use the 
> existing printers for C types 
> (https://discourse.llvm.org/t/rfc-showing-register-fields-in-lldb/64676/2). 
> There's also a hint that this would make assigning to specific fields more 
> easy (though that is a very far off goal).
> 
> This is what the first version looked like:
> ```
> (lldb) register read cpsr
> cpsr = 0x60001000
> | N | Z | C | V | TCO | DIT | UAO | PAN | SS | IL | SSBS | BTYPE | D | A | I 
> | F | nRW | EL | SP |
> | 0 | 1 | 1 | 0 |  0  |  0  |  0  |  0  | 0  | 0  |  1   |   0   | 0 | 0 | 0 
> | 0 |  0  | 0  | 0  |
> ```
> Of course it would be refined based on line length and field size etc etc. A 
> lot of the things the type printers already do, which is why using them 
> looked like it would save some effort.
> 
> Some of that extra formatting code will get written anyway because I want to 
> eventually add a register command that will show you the register layout. So 
> table formatting is needed either way. That would produce something like:
> ```
> (lldb) register info cpsr
> | 31 | 30 | ...
> | N  |  Z | ...
> ```
> 
> So overall no there's nothing preventing me from walking the bits. Assuming 
> you already have code to format text tables nicely, it would be about the 
> same amount of code.
> 
> I need to check if we have any generic table formatting code, or could 
> extract some from somewhere.
Sorry I wasn't aware of the history :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

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

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Core/DumpRegisterValue.cpp:136-137
+  // See if we have made this type before and can reuse it.
+  CompilerType fields_type = ast->GetTypeForIdentifier(
+  ConstString(register_type_name.c_str()));
+

bulbazord wrote:
> This seems highly specific to C++... Let's try to find another way to do 
> this, ideally with `TypeSystem` instead of `TypeSystemClang` and 
> `clang::CXXRecordDecl`.
Are we saving a lot of code by going through the clang typesystem here, or 
would walking the bits and formatting the directly be roughly the same amount 
of code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

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

[Lldb-commits] [PATCH] D146154: [lldb][gnustep] Add minimal GNUstepObjCRuntime plugin for LanguageTypeObjC on non-Apple platforms

[Adrian Prantl via Phabricator via lldb-commits]

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

Thanks for adding the check!




Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp:185
+  llvm::StringRef filename = module_file_spec.GetFilename().GetStringRef();
+  return filename.starts_with("libobjc.so") || filename == "objc.dll";
+}

Do you have access to the target triple here? Might want to only check for the 
strings based on the the OS?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154

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

[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2640
   if (!D || !D->isCompleteDefinition())
-return FwdDecl;
+return {FwdDecl, nullptr};
 

I'm curious if this works if we encounter a forward declaration, early exit 
here, then encounter a use of the type, and then the definition, since 
completeClass effectively also ignores the preferred name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145803

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

[Lldb-commits] [PATCH] D146265: [lldb] Introduce SymbolFile::ParseAllLanguages

[Adrian Prantl via Phabricator via lldb-commits]

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

You could add a test similar to 
lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp




Comment at: lldb/include/lldb/Symbol/SymbolFile.h:154
+  /// case this function should behave identically as ParseLanguage.
+  virtual llvm::SmallSet
+  ParseAllLanguages(CompileUnit _unit) {

I would increase the set size to 4. It's quite common to have C, C++, and ObjC 
in the same module.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146265

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

[Lldb-commits] [PATCH] D146320: [lldb] Test direct ivar access in objc++ (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

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

Otherwise LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146320

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

[Lldb-commits] [PATCH] D146320: [lldb] Test direct ivar access in objc++ (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/test/API/commands/frame/var/direct-ivar/objcpp/main.mm:6
+  void fun() {
+// check this
+  }

I would recommend using `printf("check this")` to make 100% sure that there is 
code on this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146320

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

[Lldb-commits] [PATCH] D146154: [lldb][gnustep] Add minimal GNUstepObjCRuntime plugin for LanguageTypeObjC on non-Apple platforms

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Here is an example of how the Swift Runtime plugin detects both itself and 
whether ObjC interop is enabled. Basically all I'm asking is to not 
accidentally break this mechanism.
https://github.com/apple/llvm-project/blob/7750ffa35111df6d38a5c73ab7e78ccebc9b43c3/lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp#L124
If you want to make 100% sure, you could checkout the Swift compiler on Linux 
(or I suppose Windows) from github.com/apple/swift run 
`swift/utils/update-checkout --clone` apply your patch to llvm-project and run 
`swift/utils/build-script --lldb --test -- --skip-build-benchmarks 
--skip-test-cmark --skip-test-swift` and make sure the lldb tests still pass 
after applying you patch. But if you check for the symbol that would be 
sufficient for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154

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

[Lldb-commits] [PATCH] D146154: [lldb][gnustep] Add minimal GNUstepObjCRuntime plugin for LanguageTypeObjC on non-Apple platforms

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

In D146154#4198730 , @sgraenitz wrote:

> In D146154#4197454 , @aprantl wrote:
>
>> One thing I just realized — we need to make sure that we don't accidentally 
>> create a GNUstep ObjC runtime in a Swift process that was built without ObjC 
>> support on Linux.
>
> Yes, thanks for bringing this up. The goal definitely is to avoid any 
> accidental conflicts with existing use cases that don't need or expect a 
> GNUstep runtime. I really want to get my focus to the Windows side and PDB 
> parsing. It's useful to have Linux working as well, so that we have a 
> testable feature set to match. Otherwise, we don't want to invest a lot of 
> effort here yet.
>
>> How can we ensure this works for both cases?
>
> Shouldn't the Swift processes report `eLanguageTypeSwift`? Then 
> `GNUstepObjCRuntime::CreateInstance()` rejects it reliably.

I'm not sure where eLanguageType being passed in here comes from. Generally for 
Swift processes with (Apple) Objective-C interoperability enabled, it is the 
expected case to have both a Swift and an Objective-C runtime in the same 
process.

>> I.e., can you detect based on the presence of a symbol or shared object that 
>> an GNUstep runtime is present?
>
> Are there existing cases that follow such an approach? Looking at the order 
> of events here, it appears that we have to wait for `ModulesDidLoad()` to 
> report the shared library before we can inspect its symbols. How would we 
> proceed if we want to create the language runtime first? I.e. here 
> https://github.com/llvm/llvm-project/blob/release/16.x/lldb/source/Target/Process.cpp#L5727-L5732
>
> The shared library has a GNUstep-specific EH personality for example, would 
> that do?
>
>   > llvm-nm libobjc2/build/libobjc.so | grep gnustep_objc
>   000264c0 T __gnustep_objc_personality_v0
>   00026500 T __gnustep_objcxx_personality_v0

I think that would be a great way to guard against false positives!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154

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

[Lldb-commits] [PATCH] D146154: [lldb][gnustep] Add minimal GNUstepObjCRuntime plugin for LanguageTypeObjC on non-Apple platforms

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I.e., can you detect based on the presence of a symbol or shared object that an 
GNUstep runtime is present?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154

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

[Lldb-commits] [PATCH] D146154: [lldb][gnustep] Add minimal GNUstepObjCRuntime plugin for LanguageTypeObjC on non-Apple platforms

[Adrian Prantl via Phabricator via lldb-commits]

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

One thing I just realized — we need to make sure that we don't accidentally 
create a GNUstep ObjC runtime in a Swift process that was built without ObjC 
support on Linux. How can we ensure this works for both cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154

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

[Lldb-commits] [PATCH] D146154: [lldb][gnustep] Add minimal GNUstepObjCRuntime plugin for LanguageTypeObjC on non-Apple platforms

[Adrian Prantl via Phabricator via lldb-commits]

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

In general I don't think I have a problem with adding this, especially since 
Clang also supports the runtime as a compilation target, and LLDB is tightly 
integrated with Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154

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