[Lldb-commits] [PATCH] D147300: [lldb] Fix build on older FreeBSD

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147300

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

[Lldb-commits] [PATCH] D147045: [lldb] Drop RegisterInfoInterface::GetDynamicRegisterInfo

[David Spickett via Phabricator via lldb-commits]

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

LGTM

I have no idea about the specific use case but less code is good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147045

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

[Lldb-commits] [PATCH] D146965: [lldb] Add support for MSP430 in LLDB.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> One concern I have is that there are no tests. I can understand that it may 
> be difficult to get automated tests running testing the debugging of an 
> architecture like MSP430 but there are things we can test to sanity test 
> MSP430 support

Two things come to mind:

- Core files (though it is embedded so is that even a thing?)
- Mocking an msp430 remote, as we do for example in Target XML tests.


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

https://reviews.llvm.org/D146965

___
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

[Michael Buch via Phabricator via lldb-commits]

Michael137 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:

aprantl wrote:
> Can you check if we already have a similar function in Dwarf.h in LLVM?
we do in fact :)

`dwarf::isCplusPlus`

The only remaining quirk is that we one of the language constants 
(`DW_LANG_Mips_Assembler`) doesn't match the DWARF constants. Which isn't 
really a problem for this particular function


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] D143062: [lldb] Allow evaluating expressions in C++20 mode

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:551
   case lldb::eLanguageTypeC_plus_plus_14:
 lang_opts.CPlusPlus11 = true;
 m_compiler->getHeaderSearchOpts().UseLibcxx = true;

aprantl wrote:
> Why does this not set C++14? Because it's effectively equivalent?
I think it's because we haven't done the work to make c++14 in the expression 
evaluator work, but the default clang language is c++14. So we downgrade it 
silently to c++11 here.

We should really fix that, but I think this broke some tests in the past when 
we tried enabling it (see `8cb75745412e4bc9592d2409cc6cfa4a2940d9e7`, 
https://reviews.llvm.org/D80308)


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] D143061: [lldb][Language] Add more language types

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/include/lldb/lldb-enumerations.h:492
+  eLanguageTypeAda2005 = 0x002e,
+  eLanguageTypeAda2012 = 0x002f,
+

aprantl wrote:
> aprantl wrote:
> > Michael137 wrote:
> > > aprantl wrote:
> > > > Would it make sense to generate this list from the macros in 
> > > > `llvm/include/llvm/BinaryFormat/Dwarf.def` with some clever application 
> > > > of the ## operator?
> > > The only thing that's stopping us from doing this is that the constants 
> > > for the vendor extensions are not consecutive with the rest of the 
> > > constants. So if one ever does try to use that language constant we'd run 
> > > into out-of-bounds accesses here and there
> > Do have arrays that are indexed by language? Or what would be an example of 
> > that?
> Also, we could still define eNumLanguagesTypes to be at the end of the 
> official block?
> Do have arrays that are indexed by language? Or what would be an example of 
> that?

Yup, e.g., `Language::GetNameForLanguageType` and more crucially the 
`LanguageSet` type gets used by things like 
`GetAllTypeSystemSupportedLanguagesForExpressions`


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] D143061: [lldb][Language] Add more language types

[Michael Buch via Phabricator via lldb-commits]

Michael137 marked an inline comment as not done.
Michael137 added inline comments.



Comment at: lldb/include/lldb/lldb-enumerations.h:492
+  eLanguageTypeAda2005 = 0x002e,
+  eLanguageTypeAda2012 = 0x002f,
+

Michael137 wrote:
> aprantl wrote:
> > aprantl wrote:
> > > Michael137 wrote:
> > > > aprantl wrote:
> > > > > Would it make sense to generate this list from the macros in 
> > > > > `llvm/include/llvm/BinaryFormat/Dwarf.def` with some clever 
> > > > > application of the ## operator?
> > > > The only thing that's stopping us from doing this is that the constants 
> > > > for the vendor extensions are not consecutive with the rest of the 
> > > > constants. So if one ever does try to use that language constant we'd 
> > > > run into out-of-bounds accesses here and there
> > > Do have arrays that are indexed by language? Or what would be an example 
> > > of that?
> > Also, we could still define eNumLanguagesTypes to be at the end of the 
> > official block?
> > Do have arrays that are indexed by language? Or what would be an example of 
> > that?
> 
> Yup, e.g., `Language::GetNameForLanguageType` and more crucially the 
> `LanguageSet` type gets used by things like 
> `GetAllTypeSystemSupportedLanguagesForExpressions`
> Also, we could still define eNumLanguagesTypes to be at the end of the 
> official block?

That wouldn't quite work for the `LanguageSet` indexing because the vendor 
extension language codes are no consecutive to the other language codes. So the 
size of the `LanguageSet` would just balloon


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] D143061: [lldb][Language] Add more language types

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/include/lldb/lldb-enumerations.h:492
+  eLanguageTypeAda2005 = 0x002e,
+  eLanguageTypeAda2012 = 0x002f,
+

Michael137 wrote:
> Michael137 wrote:
> > aprantl wrote:
> > > aprantl wrote:
> > > > Michael137 wrote:
> > > > > aprantl wrote:
> > > > > > Would it make sense to generate this list from the macros in 
> > > > > > `llvm/include/llvm/BinaryFormat/Dwarf.def` with some clever 
> > > > > > application of the ## operator?
> > > > > The only thing that's stopping us from doing this is that the 
> > > > > constants for the vendor extensions are not consecutive with the rest 
> > > > > of the constants. So if one ever does try to use that language 
> > > > > constant we'd run into out-of-bounds accesses here and there
> > > > Do have arrays that are indexed by language? Or what would be an 
> > > > example of that?
> > > Also, we could still define eNumLanguagesTypes to be at the end of the 
> > > official block?
> > > Do have arrays that are indexed by language? Or what would be an example 
> > > of that?
> > 
> > Yup, e.g., `Language::GetNameForLanguageType` and more crucially the 
> > `LanguageSet` type gets used by things like 
> > `GetAllTypeSystemSupportedLanguagesForExpressions`
> > Also, we could still define eNumLanguagesTypes to be at the end of the 
> > official block?
> 
> That wouldn't quite work for the `LanguageSet` indexing because the vendor 
> extension language codes are no consecutive to the other language codes. So 
> the size of the `LanguageSet` would just balloon
Interestingly I found another language enum that does exactly what LLDB does to 
(in regards to the vendor extensions): `llvm/include/llvm-c/DebugInfo.h`


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

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added reviewers: aprantl, labath.
Herald added a project: All.
Michael137 updated this revision to Diff 510478.
Michael137 added a comment.
Michael137 updated this revision to Diff 510482.
Michael137 edited the summary of this revision.
Michael137 updated this revision to Diff 510489.
Michael137 published this revision for review.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- Add docs


Michael137 added a comment.

- Merge into single API


Michael137 added a comment.

- Adjust test-case. Add FIXME


**Summary**

In a program such as:

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

...LLDB would run into issues such as:

  (lldb) expr ::B::Foo f
  error: expression failed to parse:
  error: :1:6: no type named 'Foo' in namespace 'A::B'
  ::B::Foo f
  ~^

This is because the `SymbolFileDWARF::FindNamespace` implementation
will return *any* namespace it finds if the `parent_decl_ctx` provided
is empty. In `FindExternalVisibleDecls` we use this API to find the
namespace that symbol `B` refers to. If `A::B` happened to be the one
that `SymbolFileDWARF::FindNamespace` looked at first, we would try
to find `struct Foo` in `A::B`. Hence the error.

This patch proposes a new flag to `SymbolFileDWARF::FindNamespace`
allows us to only consider top-level namespaces in our search, which is what
`FindExternalVisibleDecls` is attempting anyway; it just never
accounted for multiple namespaces of the same name.

**Testing**

- Added API test-case


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147436

Files:
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolFileOnDemand.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Symbol/SymbolFileOnDemand.cpp
  lldb/test/API/commands/expression/inline-namespace/TestInlineNamespace.py
  lldb/test/API/lang/cpp/namespace/TestNamespace.py
  lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
  lldb/test/API/lang/cpp/namespace/main.cpp

Index: lldb/test/API/lang/cpp/namespace/main.cpp
===
--- lldb/test/API/lang/cpp/namespace/main.cpp
+++ lldb/test/API/lang/cpp/namespace/main.cpp
@@ -102,6 +102,18 @@
 return myfunc2(3) + j + i + a + 2 + anon_uint + a_uint + b_uint + y_uint; // Set break point at this line.
 }
 
+namespace B {
+struct Bar {
+int x() { return 42; }
+};
+} // namespace B
+
+namespace A::B {
+struct Bar {
+int y() { return 137; }
+};
+} // namespace A::B
+
 int
 main (int argc, char const *argv[])
 {
@@ -112,5 +124,7 @@
 A::B::test_lookup_at_nested_ns_scope_after_using();
 test_lookup_before_using_directive();
 test_lookup_after_using_directive();
-return Foo::myfunc(12);
+::B::Bar bb;
+A::B::Bar ab;
+return Foo::myfunc(12) + bb.x() + ab.y();
 }
Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -159,8 +159,8 @@
 self.runToBkpt("continue")
 # Evaluate func(10) - should call A::func(int)
 self.expect_expr("func(10)", result_type="int", result_value="13")
-# Evaluate B::func() - should call B::func()
-self.expect_expr("B::func()", result_type="int", result_value="4")
+# FIXME: under C++ rules this should call A::B::func() since we're at namespace scope of 'A'.
+self.expect("expr B::func()", error=True, substrs=["no member named 'func' in namespace 'B'"])
 # Evaluate func() - should call A::func()
 self.expect_expr("func()", result_type="int", result_value="3")
 
@@ -187,14 +187,14 @@
 # Continue to BP_before_using_directive at global scope before using
 # declaration
 self.runToBkpt("continue")
-# Evaluate B::func() - should call B::func()
-self.expect_expr("B::func()", result_type="int", result_value="4")
+# Evaluate B::func() - should error out since there is no definition for it
+self.expect("expr B::func()", error=True, substrs=["no member named 'func' in namespace 'B'"])
 
 # Continue to BP_after_using_directive at global scope after using
 # declaration
 self.runToBkpt("continue")

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

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py:197
+# FIXME: under C++ rules this should call A::B::func() since we 
imported namespace 'A'
+self.expect("expr B::func()", error=True, substrs=["no member named 
'func' in namespace 'B'"])
 

I need to double check how exactly this used to work and whether there's a fix 
for it on top of the current patch. But not supporting this seems less 
problematic than choosing the wrong namespace


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] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

ping


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] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

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

If we want to export all lldb symbols (i.e
LLDB_EXPORT_ALL_SYMBOLS=ON), we need to use default visibility for all 
LLDB symbols even if a global `CMAKE_CXX_VISIBILITY_PRESET=hidden` is present.A


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility# for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden` is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility# for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden` is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 510550.
wallace added a comment.

nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 510551.
wallace added a comment.

another nit...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[River Riddle via Phabricator via lldb-commits]

rriddle added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:173
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")

Other places uses `if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")` to check for 
non-windows, should that check be used here as well? Not sure what's consistent 
for the lldb codebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:173
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")

rriddle wrote:
> Other places uses `if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")` to check for 
> non-windows, should that check be used here as well? Not sure what's 
> consistent for the lldb codebase.
I initially thought that it might be fine just to check the compiler id, but 
better be safer gating the target OS as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 510557.
wallace added a comment.

gate the target OS


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[River Riddle via Phabricator via lldb-commits]

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

LGTM, and makes sense to me given that downstream users often want a build of 
LLVM with `CMAKE_CXX_VISIBILITY_PRESET=hidden`, but lldb should still be able 
to build/link/work in the presence of that (the LLDB driver and tools currently 
fail to link if LLVM is built `CMAKE_CXX_VISIBILITY_PRESET=hidden`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jason Molenda via Phabricator via lldb-commits]

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

The Darwin xnu kernel has a global variable which has the number of bits used 
for addressing in ptrauth code.  The DynamicLoaderDarwinKernel already knows to 
look at xnu global variables to load kexts (solibs for a kernel); it's a 
natural place to find & use this information to set the number of bits 
correctly for the Process.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147462

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp


Index: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,45 @@
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  uint32_t wordsize =
+  m_process->GetTarget().GetArchitecture().GetAddressByteSize();
+  if (symbol && wordsize == 8) {
+if (symbol->GetByteSizeIsValid()) {
+  // Mark all bits as addressable so we don't strip any from our
+  // memory read below, with an incorrect default value.
+  uint32_t old_bits_value = m_process->GetVirtualAddressableBits();
+  // b55 is the sign extension bit with PAC, b56:63 are TBI,
+  // don't mark those as addressable.
+  m_process->SetVirtualAddressableBits((wordsize * 8) - 9);
+  addr_t sym_addr = symbol->GetLoadAddress(&m_process->GetTarget());
+  Status error;
+  // The definition of gT1Sz is in the xnu sources -- it is 8 bytes.
+  // We may be running on a stripped kernel binary and the size of
+  // the Symbol may be incorrect because neighboring symbols have
+  // been stripped out.  Hardcode it to 8 bytes here to sidestep
+  // this problem.
+  const size_t bytesize = 8; // size of gT1Sz value
+  uint64_t sym_value =
+  m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+  sym_addr, bytesize, 0, error, /*force_live_memory=*/true);
+  if (error.Success()) {
+// 64 - T1Sz is the highest bit used for auth.
+// The value we pass in to SetVirtualAddressableBits is
+// the number of bits used for addressing, so if
+// T1Sz is 25, then 64-25 == 39, bits 0..38 are used for
+// addressing, bits 39..63 are used for PAC/TBI or whatever.
+int virt_addr_bits = 64 - sym_value;
+m_process->SetVirtualAddressableBits(virt_addr_bits);
+  } else {
+if (old_bits_value != 0)
+  m_process->SetVirtualAddressableBits(old_bits_value);
+  }
+}
+  }
 } else {
   m_kernel.Clear();
 }


Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,45 @@
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  uint32_t wordsize =
+  m_process->GetTarget().GetArchitecture().GetAddressByteSize();
+  if (symbol && wordsize == 8) {
+if (symbol->GetByteSizeIsValid()) {
+  // Mark all bits as addressable so we don't strip any

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1087
+  if (symbol && wordsize == 8) {
+if (symbol->GetByteSizeIsValid()) {
+  // Mark all bits as addressable so we don't strip any from our

What does this check add? Should this be:
```
if (symbol && symbol->GetByteSize() == wordsize ==8)
```



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1090
+  // memory read below, with an incorrect default value.
+  uint32_t old_bits_value = m_process->GetVirtualAddressableBits();
+  // b55 is the sign extension bit with PAC, b56:63 are TBI,

Maybe make this `const` to make it clear nobody should modify this. 



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1093
+  // don't mark those as addressable.
+  m_process->SetVirtualAddressableBits((wordsize * 8) - 9);
+  addr_t sym_addr = symbol->GetLoadAddress(&m_process->GetTarget());

Instead of changing the addressable bits for the process, should we modify 
`GetLoadAddress` to explicitly skip the stripping instead? This is probably 
purely theoretical, but what if another (host) thread tried to read memory in 
the meantime? Changing global state like this can lead to subtle bugs. 



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1101
+  // this problem.
+  const size_t bytesize = 8; // size of gT1Sz value
+  uint64_t sym_value =

You can probably just reuse `wordsize` here?



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:
+// addressing, bits 39..63 are used for PAC/TBI or whatever.
+int virt_addr_bits = 64 - sym_value;
+m_process->SetVirtualAddressableBits(virt_addr_bits);

Why is this singed? This should be a `uint32_t`.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1114
+  } else {
+if (old_bits_value != 0)
+  m_process->SetVirtualAddressableBits(old_bits_value);

Is this actually necessary? Does that mean that you can't do 
`process->SetVirtualAddressableBits(process->GetVirtualAddressableBits())` in 
general?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147462

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

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1093
+  // don't mark those as addressable.
+  m_process->SetVirtualAddressableBits((wordsize * 8) - 9);
+  addr_t sym_addr = symbol->GetLoadAddress(&m_process->GetTarget());

JDevlieghere wrote:
> Instead of changing the addressable bits for the process, should we modify 
> `GetLoadAddress` to explicitly skip the stripping instead? This is probably 
> purely theoretical, but what if another (host) thread tried to read memory in 
> the meantime? Changing global state like this can lead to subtle bugs. 
Yes, I thought about touching the global state.  At this point we've just 
attached to the remote device / corefile, and are loading the kernel binary.  
There aren't other threads operating on this at this time.  We're also in a 
state where the number of addressable bits may default to a correct value, but 
is just as likely incorrect.  

As for a flag to do this, it's a bit tricky!  It's actually the 
Target::ReadUnsignedIntegerFromMemory() call, which takes an Address object, 
which ends up mutating the address while constructing the load address.  We 
could pipe a flag for ReadUnsignedIntegerFromMemory down a few layers to where 
that's happening, or add a flag to the Address object which indicates that it 
should not be mutated down the line.  Target::ReadMemory needs to take an 
Address object to fall back to using the backing file if possible (important if 
the corefile doesn't include the binary contents), another alternative is to 
switch to Process::ReadMemory whcih takes an `addr_t` but won't fall back to 
the on-disk file.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1114
+  } else {
+if (old_bits_value != 0)
+  m_process->SetVirtualAddressableBits(old_bits_value);

JDevlieghere wrote:
> Is this actually necessary? Does that mean that you can't do 
> `process->SetVirtualAddressableBits(process->GetVirtualAddressableBits())` in 
> general?
I didn't debug it through the layers, but setting the value to 0 to when the 
getter returns 0, did break this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147462

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

[Lldb-commits] [lldb] 8c3ae74 - [lldb][NFC] Remove outdated TODO message

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-04-03T13:08:54-07:00
New Revision: 8c3ae74ab147cad84f6bebc6d30a6e5a309f0d9f

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

LOG: [lldb][NFC] Remove outdated TODO message

This should have been removed in a52054cfa29d

Added: 


Modified: 
lldb/source/Expression/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Expression/CMakeLists.txt 
b/lldb/source/Expression/CMakeLists.txt
index 4194cfe02113f..edf67627374ca 100644
--- a/lldb/source/Expression/CMakeLists.txt
+++ b/lldb/source/Expression/CMakeLists.txt
@@ -1,4 +1,3 @@
-# TODO: Add property `NO_PLUGIN_DEPENDENCIES` to lldbExpression
 add_lldb_library(lldbExpression NO_PLUGIN_DEPENDENCIES
   DiagnosticManager.cpp
   DWARFExpression.cpp



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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:175
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()

Rather than changing the compile options directly, can we change the 
`CXX_VISIBILITY_PRESET` property?

```
set_target_properties((${name} PROPERTIES CXX_VISIBILITY_PRESET default)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 510586.
jasonmolenda added a comment.

Update patch to address some of Jonas' comments.  I was adding a check that the 
ptrsize of the architecture was 64-bit, but we don't work on 32-bit xnu kernels 
for a few years now; we can assume 64-bit safely.  I am still forcing the 
addressable bits value to 55 globally inside lldb while I read & set the 
correct value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147462

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp


Index: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,36 @@
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  if (symbol) {
+const uint32_t orig_bits_value = 
m_process->GetVirtualAddressableBits();
+// Mark all bits as addressable so we don't strip any from our
+// memory read below, with an incorrect default value.
+// b55 is the sign extension bit with PAC, b56:63 are TBI,
+// don't mark those as addressable.
+m_process->SetVirtualAddressableBits(55);
+Status error;
+// gT1Sz is 8 bytes.  We may run on a stripped kernel binary
+// where we can't get the size accurately.  Hardcode it.
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);
+if (error.Success()) {
+  // 64 - T1Sz is the highest bit used for auth.
+  // The value we pass in to SetVirtualAddressableBits is
+  // the number of bits used for addressing, so if
+  // T1Sz is 25, then 64-25 == 39, bits 0..38 are used for
+  // addressing, bits 39..63 are used for PAC/TBI or whatever.
+  uint32_t virt_addr_bits = 64 - sym_value;
+  m_process->SetVirtualAddressableBits(virt_addr_bits);
+} else {
+  m_process->SetVirtualAddressableBits(orig_bits_value);
+}
+  }
 } else {
   m_kernel.Clear();
 }


Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,36 @@
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  if (symbol) {
+const uint32_t orig_bits_value = m_process->GetVirtualAddressableBits();
+// Mark all bits as addressable so we don't strip any from our
+// memory read below, with an incorrect default value.
+// b55 is the sign extension bit with PAC, b56:63 are TBI,
+// don't mark those as addressable.
+m_process->SetVirtualAddressableBits(55);
+Status error;
+// gT1Sz is 8 bytes.  We may run on a stripped kernel binary
+// where we can't get the size accurately.  Hardcode it.
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);
+if (error.Success()) {
+  // 64 - T1Sz is the highest bit used for auth.
+  

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:175
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()

JDevlieghere wrote:
> Rather than changing the compile options directly, can we change the 
> `CXX_VISIBILITY_PRESET` property?
> 
> ```
> set_target_properties((${name} PROPERTIES CXX_VISIBILITY_PRESET default)
> ```
Thanks @JDevlieghere , that works and is cleaner


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 510588.
wallace added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM. I would still prefer to not change the global state, but after speaking 
to Jason offline, that might be better tackled when we unify the current 
Process/Target bifurcation.




Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1094-1097
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);

Why not use `symbol->GetByteSize()` too? Maybe add an 
`assert(symbol->GetByteSize() == 8)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147462

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

[Lldb-commits] [lldb] e538c6f - [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via lldb-commits]

Author: walter erquinigo
Date: 2023-04-03T15:41:02-05:00
New Revision: e538c6fc3048e1fb1a58da879275d6804186856e

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

LOG: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is 
provided.

If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we need 
to use default visibility for all LLDB libraries even if a global 
CMAKE_CXX_VISIBILITY_PRESET=hidden is present. In fact, there are cases in 
which a global llvm configuration wants CMAKE_CXX_VISIBILITY_PRESET as hidden 
but simultaneously LLDB_EXPORT_ALL_SYMBOLS=ON is also needed to be able to 
develop out-of-tree lldb plugins.

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

Added: 


Modified: 
lldb/cmake/modules/AddLLDB.cmake

Removed: 




diff  --git a/lldb/cmake/modules/AddLLDB.cmake 
b/lldb/cmake/modules/AddLLDB.cmake
index f2d96dfd68e00..d47a30f5e1092 100644
--- a/lldb/cmake/modules/AddLLDB.cmake
+++ b/lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@ function(add_lldb_library name)
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)



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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

[walter erquinigo via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe538c6fc3048: [LLDB] Ensure LLDB symbols are exported in 
LLDB_EXPORT_ALL_SYMBOLS is provided. (authored by wallace).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Yeah, I agree that messing with the global setting of addressable bits here is 
not ideal, but at this point in time the fact that we probably have an 
un-set/invalid value means it won't make things worse.




Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1094-1097
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);

JDevlieghere wrote:
> Why not use `symbol->GetByteSize()` too? Maybe add an 
> `assert(symbol->GetByteSize() == 8)`
We're trying to handle the case (it has happened in the past) where we have an 
inaccurate byte size because lldb is running on a stripped kernel binary, this 
is specifically working around the case where the symbol byte size is not 8.

(Symbols synthesize their size by looking at the next nearest symbol, so when 
you have a stripped binary you may have sizes that are larger than reality when 
some symbols have been stripped)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147462

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

[Lldb-commits] [lldb] 8b09271 - Using global variable in xnu kernel, set # of addressable bits

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-04-03T13:49:51-07:00
New Revision: 8b092714c304defb00876a01995af3b114dadc92

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

LOG: Using global variable in xnu kernel, set # of addressable bits

The kernel has a global variable with the TCR_EL1.T1SZ value,
from which was can calculate the number of addressable bits.
Find that symbol in DynamicLoaderDarwinKernel and set the bits
to that value for this Process.

Differential Revision: https://reviews.llvm.org/D147462
rdar://107445318

Added: 


Modified: 

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index ea2cb46af009f..05658d13001a2 100644
--- 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@ void 
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,36 @@ void 
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  if (symbol) {
+const uint32_t orig_bits_value = 
m_process->GetVirtualAddressableBits();
+// Mark all bits as addressable so we don't strip any from our
+// memory read below, with an incorrect default value.
+// b55 is the sign extension bit with PAC, b56:63 are TBI,
+// don't mark those as addressable.
+m_process->SetVirtualAddressableBits(55);
+Status error;
+// gT1Sz is 8 bytes.  We may run on a stripped kernel binary
+// where we can't get the size accurately.  Hardcode it.
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);
+if (error.Success()) {
+  // 64 - T1Sz is the highest bit used for auth.
+  // The value we pass in to SetVirtualAddressableBits is
+  // the number of bits used for addressing, so if
+  // T1Sz is 25, then 64-25 == 39, bits 0..38 are used for
+  // addressing, bits 39..63 are used for PAC/TBI or whatever.
+  uint32_t virt_addr_bits = 64 - sym_value;
+  m_process->SetVirtualAddressableBits(virt_addr_bits);
+} else {
+  m_process->SetVirtualAddressableBits(orig_bits_value);
+}
+  }
 } else {
   m_kernel.Clear();
 }



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

[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jason Molenda via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b092714c304: Using global variable in xnu kernel, set # of 
addressable bits (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147462

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp


Index: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,36 @@
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  if (symbol) {
+const uint32_t orig_bits_value = 
m_process->GetVirtualAddressableBits();
+// Mark all bits as addressable so we don't strip any from our
+// memory read below, with an incorrect default value.
+// b55 is the sign extension bit with PAC, b56:63 are TBI,
+// don't mark those as addressable.
+m_process->SetVirtualAddressableBits(55);
+Status error;
+// gT1Sz is 8 bytes.  We may run on a stripped kernel binary
+// where we can't get the size accurately.  Hardcode it.
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);
+if (error.Success()) {
+  // 64 - T1Sz is the highest bit used for auth.
+  // The value we pass in to SetVirtualAddressableBits is
+  // the number of bits used for addressing, so if
+  // T1Sz is 25, then 64-25 == 39, bits 0..38 are used for
+  // addressing, bits 39..63 are used for PAC/TBI or whatever.
+  uint32_t virt_addr_bits = 64 - sym_value;
+  m_process->SetVirtualAddressableBits(virt_addr_bits);
+} else {
+  m_process->SetVirtualAddressableBits(orig_bits_value);
+}
+  }
 } else {
   m_kernel.Clear();
 }


Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1068,6 +1068,7 @@
 
 if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
   static ConstString kext_summary_symbol("gLoadedKextSummaries");
+  static ConstString arm64_T1Sz_value("gT1Sz");
   const Symbol *symbol =
   m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
   kext_summary_symbol, eSymbolTypeData);
@@ -1076,6 +1077,36 @@
 // Update all image infos
 ReadAllKextSummaries();
   }
+  // If the kernel global with the T1Sz setting is available,
+  // update the target.process.virtual-addressable-bits to be correct.
+  symbol = m_kernel.GetModule()->FindFirstSymbolWithNameAndType(
+  arm64_T1Sz_value, eSymbolTypeData);
+  if (symbol) {
+const uint32_t orig_bits_value = m_process->GetVirtualAddressableBits();
+// Mark all bits as addressable so we don't strip any from our
+// memory read below, with an incorrect default value.
+// b55 is the sign extension bit with PAC, b56:63 are TBI,
+// don't mark those as addressable.
+m_process->SetVirtualAddressableBits(55);
+Status error;
+// gT1Sz is 8 bytes.  We may run on a stripped kernel binary
+// where we can't get the size accurately.  Hardcode it.
+const size_t sym_bytesize = 8; // size of gT1Sz value
+uint64_t sym_value =
+m_process->GetTarget().ReadUnsignedIntegerFromMemory(
+symbol->GetAddress(), sym_bytesize, 0, error);
+if (error.Success()) {
+  // 64 - T1Sz is the highest bit used for auth.
+  // The value we pass in to SetVirtualAddressableBits is
+  // the number of bits used for addressing, so if
+  // T1Sz is 25, then 64-25 == 39, bits 0..38 are used for
+  // addressi

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

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D147292#4238834 , @augusto2112 
wrote:

> In D147292#4238820 , @JDevlieghere 
> wrote:
>
>> There seems to be overlap in the code added in this patch and D146679 
>> . Does one supersede the other or is there 
>> a dependency? If it's the latter you should extract that into a separate 
>> patch and make it a parent revision of this and D146679 
>> .
>
> Yes, sorry, I'm abandoning the other one in favor of this one.

Be handy to mark this abandoned when you get a chance


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] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

> Be handy to mark this abandoned when you get a chance

Augusto had already abandoned it when he wrote that reply, maybe you're looking 
at a different patch or stale tab?


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] D147482: [lldb] Add an overload to SetModuleLoadAddress that takes an unsigned value

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Currently, `SBTarget::SetModuleLoadAddress` cannot accept the large slides 
needed to load images in high memory. This function should always have taken an 
unsigned as the slide as it immediately passes it to 
`Target::SetSectionLoadAddress` which takes an unsigned. This patch adds an 
overload and check that the slide is positive for the signed variant.

rdar://101355155


https://reviews.llvm.org/D147482

Files:
  lldb/include/lldb/API/SBTarget.h
  lldb/source/API/SBTarget.cpp
  lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py


Index: lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py
===
--- lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py
+++ lldb/test/API/functionalities/multiple-slides/TestMultipleSlides.py
@@ -36,6 +36,10 @@
 self.assertEqual(first_sym.GetStartAddress().GetLoadAddress(target), 
lldb.LLDB_INVALID_ADDRESS)
 self.assertEqual(second_sym.GetStartAddress().GetLoadAddress(target), 
lldb.LLDB_INVALID_ADDRESS)
 
+# Can't use a negative slide
+error = target.SetModuleLoadAddress(module, -1)
+self.assertTrue(error.Fail())
+self.assertEqual(error.GetCString(), "slide must be positive")
 
 # View the first element of `first` and `second` with
 # no slide applied, but with load address set.
@@ -43,7 +47,8 @@
 # In memory, we have something like
 #0x1000 - 0x17ff  first[]
 #0x1800 - 0x1fff  second[]
-target.SetModuleLoadAddress(module, 0)
+error = target.SetModuleLoadAddressUnsigned(module, 0)
+self.assertSuccess(error)
 self.expect("expression/d ((int*)&first)[0]", substrs=['= 5'])
 self.expect("expression/d ((int*)&second)[0]", substrs=['= 6'])
 self.assertEqual(first_sym.GetStartAddress().GetLoadAddress(target), 
@@ -60,7 +65,8 @@
 # but if the original entries are still present in lldb, 
 # the beginning address of second[] will get a load address
 # of 0x1800, instead of 0x17c0 (0x1800-64) as we need to get.
-target.SetModuleLoadAddress(module, first_size - 64)
+error = target.SetModuleLoadAddressUnsigned(module, first_size - 64)
+self.assertSuccess(error)
 self.expect("expression/d ((int*)&first)[0]", substrs=['= 5'])
 self.expect("expression/d ((int*)&second)[0]", substrs=['= 6'])
 
self.assertNotEqual(first_sym.GetStartAddress().GetLoadAddress(target), 
@@ -69,7 +75,8 @@
  second_sym.GetStartAddress().GetFileAddress())
 
 # Slide it back to the original vmaddr.
-target.SetModuleLoadAddress(module, 0)
+error = target.SetModuleLoadAddressUnsigned(module, 0)
+self.assertSuccess(error)
 self.expect("expression/d ((int*)&first)[0]", substrs=['= 5'])
 self.expect("expression/d ((int*)&second)[0]", substrs=['= 6'])
 self.assertEqual(first_sym.GetStartAddress().GetLoadAddress(target), 
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -2093,6 +2093,18 @@
int64_t slide_offset) {
   LLDB_INSTRUMENT_VA(this, module, slide_offset);
 
+  if (slide_offset < 0) {
+SBError sb_error;
+sb_error.SetErrorStringWithFormat("slide must be positive");
+return sb_error;
+  }
+
+  return SetModuleLoadAddressUnsigned(module, slide_offset);
+}
+
+SBError SBTarget::SetModuleLoadAddressUnsigned(lldb::SBModule module,
+   uint64_t slide_offset) {
+
   SBError sb_error;
 
   TargetSP target_sp(GetSP());
Index: lldb/include/lldb/API/SBTarget.h
===
--- lldb/include/lldb/API/SBTarget.h
+++ lldb/include/lldb/API/SBTarget.h
@@ -392,6 +392,27 @@
   lldb::SBError SetModuleLoadAddress(lldb::SBModule module,
  int64_t sections_offset);
 
+  /// Slide all file addresses for all module sections so that \a module
+  /// appears to loaded at these slide addresses.
+  ///
+  /// When you need all sections within a module to be loaded at a
+  /// rigid slide from the addresses found in the module object file,
+  /// this function will allow you to easily and quickly slide all
+  /// module sections.
+  ///
+  /// \param[in] module
+  /// The module to load.
+  ///
+  /// \param[in] sections_offset
+  /// An offset that will be applied to all section file addresses
+  /// (the virtual addresses found in the object file itself).
+  ///
+  /// \return
+  /// An error to indicate success, fail, and any reason for
+  /// failure.
+  lldb::SBEr

[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] 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] 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] 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] D147436: [lldb][ClangExpression] Filter out non-root namespaces in FindNamespace

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D147436#4241975 , @aprantl wrote:

> 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

Yup that does currently work. With this patch it wouldn't anymore


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] D147436: [lldb][ClangExpression] Filter out non-root namespaces in FindNamespace

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py:197
+# FIXME: under C++ rules this should call A::B::func() since we 
imported namespace 'A'
+self.expect("expr B::func()", error=True, substrs=["no member named 
'func' in namespace 'B'"])
 

Michael137 wrote:
> I need to double check how exactly this used to work and whether there's a 
> fix for it on top of the current patch. But not supporting this seems less 
> problematic than choosing the wrong namespace
I checked how expression evaluation currently works at namespace scope and it 
does rely on `FindExternalVisibleDecls` including all namespaces (not only 
top-level) in its search. We do the additional filtering for which function 
symbol to pick based on SymbolContext after the fact (in `LookupFunction`). 
This seems rather fragile but there is a decent amount of code to make this 
happen so I wouldn't want to just blindly stop supporting it.


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