from:"Martin Storsjö via Phabricator via lldb\-commits"

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

2023-08-30 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a4b3fdb8232: [lldb][windows] _wsopen_s does not accept bits 
other than `_S_IREAD | _S_IWRITE` (authored by yshui, committed by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D156817?vs=546133=554679#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156817

Files:
  lldb/source/Host/windows/FileSystem.cpp


Index: lldb/source/Host/windows/FileSystem.cpp
===
--- lldb/source/Host/windows/FileSystem.cpp
+++ lldb/source/Host/windows/FileSystem.cpp
@@ -101,6 +101,8 @@
   std::wstring wpath;
   if (!llvm::ConvertUTF8toWide(path, wpath))
 return -1;
+  // All other bits are rejected by _wsopen_s
+  mode = mode & (_S_IREAD | _S_IWRITE);
   int result;
   ::_wsopen_s(, wpath.c_str(), flags, _SH_DENYNO, mode);
   return result;


Index: lldb/source/Host/windows/FileSystem.cpp
===
--- lldb/source/Host/windows/FileSystem.cpp
+++ lldb/source/Host/windows/FileSystem.cpp
@@ -101,6 +101,8 @@
   std::wstring wpath;
   if (!llvm::ConvertUTF8toWide(path, wpath))
 return -1;
+  // All other bits are rejected by _wsopen_s
+  mode = mode & (_S_IREAD | _S_IWRITE);
   int result;
   ::_wsopen_s(, wpath.c_str(), flags, _SH_DENYNO, mode);
   return result;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2023-08-28 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

I noticed this review is accepted but not committed. If you don't have commit 
access, what's your preferred git identity, i.e. `Real Name `?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156817

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

[Lldb-commits] [PATCH] D158391: [lldb][debugserver] Fix build after libcxx removed generic char_traits implementation

2023-08-21 Thread Martin Storsjö via Phabricator via lldb-commits

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158391

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

[Lldb-commits] [PATCH] D157589: [lldb] Fix building with latest libc++

2023-08-10 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG68744ffbdd7d: [lldb] Fix building with latest libc++ 
(authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157589

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp


Index: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -631,7 +631,7 @@
   } else {
 // Zero-out any unreadable values.
 if (reg_info.byte_size > 0) {
-  std::basic_string zeros(reg_info.byte_size, '\0');
+  std::vector zeros(reg_info.byte_size, '\0');
   AppendHexValue(response, zeros.data(), zeros.size(), false);
 }
   }


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -631,7 +631,7 @@
   } else {
 // Zero-out any unreadable values.
 if (reg_info.byte_size > 0) {
-  std::basic_string zeros(reg_info.byte_size, '\0');
+  std::vector zeros(reg_info.byte_size, '\0');
   AppendHexValue(response, zeros.data(), zeros.size(), false);
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157589: [lldb] Fix building with latest libc++

2023-08-10 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo created this revision.
mstorsjo added reviewers: JDevlieghere, mib, jingham.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

Since https://reviews.llvm.org/D157058 in libc++,
the base template for char_traits has been removed - it is only
provided for char, wchar_t, char8_t, char16_t and char32_t.
(Thus, to use basic_string with a type other than those, we'd need
to supply suitable traits ourselves.)

For this particular use, a vector works just as well as basic_string.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157589

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp


Index: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -631,7 +631,7 @@
   } else {
 // Zero-out any unreadable values.
 if (reg_info.byte_size > 0) {
-  std::basic_string zeros(reg_info.byte_size, '\0');
+  std::vector zeros(reg_info.byte_size, '\0');
   AppendHexValue(response, zeros.data(), zeros.size(), false);
 }
   }


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -631,7 +631,7 @@
   } else {
 // Zero-out any unreadable values.
 if (reg_info.byte_size > 0) {
-  std::basic_string zeros(reg_info.byte_size, '\0');
+  std::vector zeros(reg_info.byte_size, '\0');
   AppendHexValue(response, zeros.data(), zeros.size(), false);
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

2023-05-25 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: libunwind/src/CMakeLists.txt:28-35
 
 # See add_asm_sources() in compiler-rt for explanation of this workaround.
 # CMake doesn't work correctly with assembly on AIX. Workaround by compiling
 # as C files as well.
 if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR
-   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17) OR
-   (${CMAKE_SYSTEM_NAME} MATCHES "AIX"))
+   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
   set_source_files_properties(${LIBUNWIND_ASM_SOURCES} PROPERTIES LANGUAGE C)

h-vetinari wrote:
> Shouldn't it be possible to remove that entire block (as it only fires for 
> CMake <3.20)?
The intention was to do such cleanups in a later patch, as mentioned in the 
original commit message in https://reviews.llvm.org/D144509. (I guess it would 
be good to bring the original commit message along with the reland attempts 
too.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151344

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

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

2023-05-17 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D144509#4349051 , @hans wrote:

> In D144509#4347562 , @glandium 
> wrote:
>
>> FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c 
>>  is 
>> causing problems on Windows compiler-rt for some reason I haven't identified 
>> yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is 
>> actually altering its behavior, which I wouldn't have expected...
>
> It is indeed unfortunate that this seems to come with such a fundamental 
> behavior change. (we already have 
> https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for 
> the issue you're seeing?
>
> Since the breakages are piling up, perhaps it's time to revert this.

Unfortunately, much of this is hard to sort out without actually trying to 
patch forward on main, since many of these issues don't show up easily in e.g. 
premerge testing or similar (the patch has been tried many times for months 
before in that form), so I'd appreciate if we'd give it a little more time to 
actually try to sort out the issues...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

2023-05-16 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D144509#4347562 , @glandium wrote:

> FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c 
>  is 
> causing problems on Windows compiler-rt for some reason I haven't identified 
> yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is 
> actually altering its behavior, which I wouldn't have expected...

See D150688  - I believe that might fix the 
issue you're seeing, as that one mentions compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D144078: [lldb] Fix a log format warning on Windows, don't assume uint64_t is a long type

2023-02-15 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8483b9b3038: [lldb] Fix a log format warning on Windows, 
dont assume uint64_t is a long type (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144078

Files:
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp


Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -636,7 +636,7 @@
 if ((inst & pat.type_mask) == pat.eigen &&
 (inst_type & pat.inst_type) != 0) {
   LLDB_LOGF(
-  log, "EmulateInstructionRISCV::%s: inst(%x at %lx) was decoded to 
%s",
+  log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was 
decoded to %s",
   __FUNCTION__, inst, m_addr, pat.name);
   auto decoded = is_rvc ? pat.decode(try_rvc) : pat.decode(inst);
   return DecodeResult{decoded, inst, is_rvc, pat};


Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -636,7 +636,7 @@
 if ((inst & pat.type_mask) == pat.eigen &&
 (inst_type & pat.inst_type) != 0) {
   LLDB_LOGF(
-  log, "EmulateInstructionRISCV::%s: inst(%x at %lx) was decoded to %s",
+  log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was decoded to %s",
   __FUNCTION__, inst, m_addr, pat.name);
   auto decoded = is_rvc ? pat.decode(try_rvc) : pat.decode(inst);
   return DecodeResult{decoded, inst, is_rvc, pat};
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D144078: [lldb] Fix a log format warning on Windows, don't assume uint64_t is a long type

2023-02-15 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo created this revision.
mstorsjo added reviewers: Emmmer, DavidSpickett, JDevlieghere, labath.
Herald added subscribers: luke, frasercrmck, luismarques, apazos, 
sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, 
MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, 
simoncook, johnrusso, rbar, asb.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added subscribers: pcwang-thead, MaskRay.
Herald added a project: LLDB.

On Windows, long is 32 bit, and uint64_t is long long.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144078

Files:
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp


Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -636,7 +636,7 @@
 if ((inst & pat.type_mask) == pat.eigen &&
 (inst_type & pat.inst_type) != 0) {
   LLDB_LOGF(
-  log, "EmulateInstructionRISCV::%s: inst(%x at %lx) was decoded to 
%s",
+  log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was 
decoded to %s",
   __FUNCTION__, inst, m_addr, pat.name);
   auto decoded = is_rvc ? pat.decode(try_rvc) : pat.decode(inst);
   return DecodeResult{decoded, inst, is_rvc, pat};


Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -636,7 +636,7 @@
 if ((inst & pat.type_mask) == pat.eigen &&
 (inst_type & pat.inst_type) != 0) {
   LLDB_LOGF(
-  log, "EmulateInstructionRISCV::%s: inst(%x at %lx) was decoded to %s",
+  log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was decoded to %s",
   __FUNCTION__, inst, m_addr, pat.name);
   auto decoded = is_rvc ? pat.decode(try_rvc) : pat.decode(inst);
   return DecodeResult{decoded, inst, is_rvc, pat};
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138259: Add the ability to see when a type in incomplete.

2022-11-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

This seems to have caused build errors with GCC:

  ../tools/lldb/source/Symbol/CompilerType.cpp: In member function ‘bool 
lldb_private::CompilerType::IsForcefullyCompleted() const’:
  ../tools/lldb/source/Symbol/CompilerType.cpp:99:25: error: base operand of 
‘->’ has non-pointer type ‘const TypeSystemWP’ {aka ‘const 
std::weak_ptr’}
 99 | return m_type_system->IsForcefullyCompleted(m_type);
| ^~

Any clues about what’s missing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138259

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

[Lldb-commits] [PATCH] D137873: [LLDB][Minidump] Set abi environment for windows.

2022-11-16 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a subscriber: alvinhochun.
mstorsjo added a comment.

In D137873#3930683 , @labath wrote:

> If that doesn't work then (besides knowing why), I'd like us try some kind of 
> a single-setting solution, as I don't think it makes sense to have two 
> different settings like this (happy to hear counterarguments to that)

Overall, the ideal is if the user usually needs to twiddle one single setting. 
For a majority of cases, picking the default format (like is done for the 
objectfile setting, and afaik this one does too even though I didn't quite 
clearly see the codepath for it) will be what you'd want: If you're working 
with mingw tools, debugging with a mingw built lldb, then your app that you're 
debugging also probably uses mingw ABIs. But it's clearly not implausible to 
want to debug executables from the other ecosystem, so being able to change it 
with a single setting is good too.

And for the objectfile setting, it's possible to set it even on a per-DLL 
basis. This may sound contrieved, but it's actually totally reasonable. While 
the C++ ABIs are incompatible, such DLLs can interoperate over C APIs - this is 
not an uncommon scenario. Plus, every mingw executable links against msvcrt.dll 
or ucrtbase.dll (provided by the system) which use the MSVC C++ ABIs internally 
(if you use the versions of them with debug info).

So the ability to set it per DLL for the objectfile part is totally warranted. 
How that fits in towards minidump reading, I don't really know though (since 
the whole minidump contains stuff for the whole process). In which cases does 
the C++ ABI form matter for the minidump btw - since the actual interpretation 
of things still would be based on per-DLL debug info (either as DWARF or PDB)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137873

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

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

2022-10-28 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D136572#3890835 , @mgorny wrote:

> In D136572#3889317 , @mstorsjo 
> wrote:
>
>> Also for the record, I'd love to get rid of this symlink based setup, if I 
>> could make cmake produce project files where caching works in the same way. 
>> The main requirement for that would be to have the whole cmake build started 
>> from the toplevel llvm-project directory (so that all source from all 
>> subprojects are subdirectories to this), instead of the llvm-project/llvm 
>> directory.
>
> To be honest, I also find it weird to start monorepo builds from within 
> `llvm` subdirectory. Perhaps adding a top-level `CMakeLists.txt` that would 
> work and wouldn't break people's workflows wouldn't be that hard, though.

This has been discussed briefly before - the main issue is that LLVM does some 
cmake trickery to recreate the installed directory structure (with executables 
in `$(pwd)/bin` etc) directly in the build folder, and if `llvm-project/llvm` 
is included from a toplevel `llvm-project/CMakeLists.txt`, this all ends up in 
`$(pwd)/llvm/bin` as things stand right now. See e.g. 
https://discourse.llvm.org/t/rfc-llvm-build-system-future-direction/53430/18. 
Hopefully it wouldn't be too hard to resolve - I had a brief look at it (just 
creating a stub CMakeLists.txt at the root and including the llvm subdirectory) 
a couple years ago, but didn't spend further time on looking into how things 
are set up and what it would take to fix it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136572

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

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

2022-10-27 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

Also for the record, I'd love to get rid of this symlink based setup, if I 
could make cmake produce project files where caching works in the same way. The 
main requirement for that would be to have the whole cmake build started from 
the toplevel llvm-project directory (so that all source from all subprojects 
are subdirectories to this), instead of the llvm-project/llvm directory.


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

https://reviews.llvm.org/D136572

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

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

2022-10-27 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo accepted this revision.
mstorsjo added a comment.

In D136572#3888630 , @mgorny wrote:

> @mstorsjo, could you check this version?

LGTM, this seems to work for my usecase (with lld,lldb,clang enabled this way). 
Thanks for the quick revert and fix!


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

https://reviews.llvm.org/D136572

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

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

2022-10-27 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D136572#3888177 , @mstorsjo wrote:

> This broke builds where clang/lld/lldb are symlinked into `llvm/tools` 
> instead of specified via `LLVM_ENABLE_PROJECTS` - since 
> `${CMAKE_CURRENT_SOURCE_DIR}/../cmake` doesn't find anything in that context.

This aspect, for finding the shared cmake directory, is handled elsewhere by 
the main LLVM build setting the `LLVM_COMMON_CMAKE_UTILS` variable, and if not 
found, it's assumed to be in `${CMAKE_CURRENT_SOURCE_DIR}/..`. See e.g. 
https://github.com/llvm/llvm-project/blob/llvmorg-15.0.0/lld/CMakeLists.txt#L198-L206.

Since this is invoked very early in the per-project cmakefiles, I guess we 
can't assume to have all those variables set up yet, but e.g. something like 
this fixes the issue for me:

  diff --git a/lld/CMakeLists.txt b/lld/CMakeLists.txt
  index 964bc3fd8b17..530f52b05bac 100644
  --- a/lld/CMakeLists.txt
  +++ b/lld/CMakeLists.txt
  @@ -1,6 +1,11 @@
   cmake_minimum_required(VERSION 3.13.4)
  -include(${CMAKE_CURRENT_SOURCE_DIR}/../cmake/Modules/CMakePolicy.cmake
  -  NO_POLICY_SCOPE)
  +if(DEFINED LLVM_COMMON_CMAKE_UTILS)
  +  include(${LLVM_COMMON_CMAKE_UTILS}/Modules/CMakePolicy.cmake
  +NO_POLICY_SCOPE)
  +else()
  +  include(${CMAKE_CURRENT_SOURCE_DIR}/../cmake/Modules/CMakePolicy.cmake
  +NO_POLICY_SCOPE)
  +endif()

   # If we are not building as a part of LLVM, build LLD as an
   # standalone project, using LLVM as an external library:

(And the same applied for all the other subprojects.) It's admittedly not very 
pretty though...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136572

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

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

2022-10-27 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.
Herald added subscribers: JDevlieghere, jdoerfert.

This broke builds where clang/lld/lldb are symlinked into `llvm/tools` instead 
of specified via `LLVM_ENABLE_PROJECTS` - since 
`${CMAKE_CURRENT_SOURCE_DIR}/../cmake` doesn't find anything in that context.

(The reason for wanting to stick to such a legacy setup, is that ccache caches 
can't be shared between workdirs for source files that are found outside of the 
base `llvm-project/llvm` directory - and I'm heavily reliant on ccache for 
keeping build times manageable in my dev workflow.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136572

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-10-21 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

Was the consensus that we'd drop this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581

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

[Lldb-commits] [PATCH] D134636: [lldb][Windows] Always call SetExecutableModule on debugger connected

2022-09-30 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe17e026959c: [lldb][Windows] Always call 
SetExecutableModule on debugger connected (authored by alvinhochun, committed 
by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134636

Files:
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  lldb/test/Shell/Target/Inputs/main.c
  lldb/test/Shell/Target/Inputs/shlib.c
  lldb/test/Shell/Target/dependent-modules-nodupe-windows.test


Index: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
===
--- /dev/null
+++ lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
@@ -0,0 +1,24 @@
+# REQUIRES: system-windows
+
+# Checks that dependent modules preloaded by LLDB are not duplicated when the
+# process actually loads the DLL.
+
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll \
+# RUN: %if windows-msvc %{-Wl,-implib:%t.shlib.lib%} \
+# RUN: %else %{-Wl,--out-implib=%t.shlib.lib%}
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.lib -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \
+# RUN:   -o "#after" -o "target modules list" %t.main.exe | FileCheck %s
+
+# CHECK-LABEL: #before
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: .shlib.dll
+
+# CHECK-LABEL: #after
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: ntdll.dll
+# CHECK-NEXT: kernel32.dll
+# CHECK:  .shlib.dll
+# CHECK-NOT:  .shlib.dll
Index: lldb/test/Shell/Target/Inputs/shlib.c
===
--- /dev/null
+++ lldb/test/Shell/Target/Inputs/shlib.c
@@ -0,0 +1 @@
+__declspec(dllexport) void exportFunc(void) {}
Index: lldb/test/Shell/Target/Inputs/main.c
===
--- /dev/null
+++ lldb/test/Shell/Target/Inputs/main.c
@@ -0,0 +1,2 @@
+__declspec(dllimport) void exportFunc(void);
+int main() { exportFunc(); }
Index: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -653,28 +653,26 @@
   LLDB_LOG(log, "Debugger connected to process {0}.  Image base = {1:x}",
debugger->GetProcess().GetProcessId(), image_base);
 
-  ModuleSP module = GetTarget().GetExecutableModule();
-  if (!module) {
-// During attach, we won't have the executable module, so find it now.
-const DWORD pid = debugger->GetProcess().GetProcessId();
-const std::string file_name = GetProcessExecutableName(pid);
-if (file_name.empty()) {
-  return;
-}
-
-FileSpec executable_file(file_name);
-FileSystem::Instance().Resolve(executable_file);
-ModuleSpec module_spec(executable_file);
-Status error;
-module =
-GetTarget().GetOrCreateModule(module_spec, true /* notify */, );
-if (!module) {
-  return;
-}
+  ModuleSP module;
+  // During attach, we won't have the executable module, so find it now.
+  const DWORD pid = debugger->GetProcess().GetProcessId();
+  const std::string file_name = GetProcessExecutableName(pid);
+  if (file_name.empty()) {
+return;
+  }
 
-GetTarget().SetExecutableModule(module, eLoadDependentsNo);
+  FileSpec executable_file(file_name);
+  FileSystem::Instance().Resolve(executable_file);
+  ModuleSpec module_spec(executable_file);
+  Status error;
+  module =
+  GetTarget().GetOrCreateModule(module_spec, true /* notify */, );
+  if (!module) {
+return;
   }
 
+  GetTarget().SetExecutableModule(module, eLoadDependentsNo);
+
   if (auto dyld = GetDynamicLoader())
 dyld->OnLoadModule(module, ModuleSpec(), image_base);
 


Index: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
===
--- /dev/null
+++ lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
@@ -0,0 +1,24 @@
+# REQUIRES: system-windows
+
+# Checks that dependent modules preloaded by LLDB are not duplicated when the
+# process actually loads the DLL.
+
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll \
+# RUN: %if windows-msvc %{-Wl,-implib:%t.shlib.lib%} \
+# RUN: %else %{-Wl,--out-implib=%t.shlib.lib%}
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.lib -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \
+# RUN:   -o "#after" -o "target modules list" %t.main.exe | FileCheck %s
+
+# CHECK-LABEL: #before
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: .shlib.dll
+
+#

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8a67a05e9334: [lldb][COFF] Map symbols without base+complex 
type as Data type (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -15,7 +15,7 @@
 # CHECK-NEXT: 4294967295 Code   0x000180001020
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
 # CHECK-NEXT: 4294967295   X Additional 0x000180003000
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
 # CHECK-NEXT: 4294967295 Data   0x000180003004
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid0x000180003008
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT: 4294967295 Data   0x000180003008
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -6,7 +6,7 @@
 
 # CHECK: Type File Address/Value {{.*}} SizeFlags   
Name
 # CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Data 0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
 # CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
absolute_symbol
 
 --- !COFF
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -379,6 +379,13 @@
   if (complex_type == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION) {
 return lldb::eSymbolTypeCode;
   }
+  const auto base_type = coff_symbol_type & 0xff;
+  if (base_type == llvm::COFF::IMAGE_SYM_TYPE_NULL &&
+  complex_type == llvm::COFF::IMAGE_SYM_DTYPE_NULL) {
+// Unknown type. LLD and GNU ld uses this for variables on MinGW, so
+// consider these symbols to be data to enable printing.
+return lldb::eSymbolTypeData;
+  }
   return lldb::eSymbolTypeInvalid;
 }
 


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -15,7 +15,7 @@
 # CHECK-NEXT: 4294967295 Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
 # CHECK-NEXT: 4294967295   X Additional 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
 # CHECK-NEXT: 4294967295 Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT: 4294967295 Data   0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -6,7 +6,7 @@
 
 # CHECK: Type File Address/Value {{.*}} SizeFlags   Name
 # CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Data 0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
 # CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} absolute_symbol
 
 --- !COFF
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -379,6 +379,13 @@
   if (complex_type == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION) {
 return lldb::eSymbolTypeCode;
   }
+  const auto base_type = coff_symbol_type & 0xff;
+  if (base_type == llvm::COFF::IMAGE_SYM_TYPE_NULL &&
+

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Add note to forwarder export symbols in symtab

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGacf7d081198f: [lldb][COFF] Add note to forwarder export 
symbols in symtab (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134518

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml

Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml
@@ -0,0 +1,51 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# CHECK:   UserID DSX Type File Address/Value {{.*}} SizeFlags   Name
+# CHECK-NEXT:  --
+# CHECK-NEXT:   1   X Data 0x{{[0-9a-f]+}}   0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} LoadLibrary (forwarded to kernel32.LoadLibrary)
+# CHECK-EMPTY:
+
+# Test file generated with:
+#   clang -O2 --target=x86_64-windows-msvc test.c -nostdlib -c -o test.obj
+#   lld-link -dll -out:test.dll -entry:entry -export:LoadLibrary=kernel32.LoadLibrary test.obj
+# test.c:
+#   void entry(void) {}
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:   6442450944
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:   IMAGE_SUBSYSTEM_WINDOWS_GUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+RelativeVirtualAddress: 8192
+Size:110
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  4096
+VirtualSize: 1
+SectionData: C3
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  8192
+VirtualSize: 110
+SectionData: 2820010001000100432047204B206578706F72742D666F727761726465722E632E746D702E646C6C0059204D204C6F61644C696272617279006B65726E656C33322E4C6F61644C69627261727900
+symbols: []
+...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -869,6 +869,26 @@
 llvm::cantFail(entry.getOrdinal(ordinal));
 symbol.SetID(ordinal);
 
+bool is_forwarder;
+llvm::cantFail(entry.isForwarder(is_forwarder));
+if (is_forwarder) {
+  // Forwarder exports are redirected by the loader transparently, but keep
+  // it in symtab and make a note using the symbol name.
+  llvm::StringRef forwarder_name;
+  if (auto err = entry.getForwardTo(forwarder_name)) {
+LLDB_LOG(log,
+ "ObjectFilePECOFF::AppendFromExportTable - failed to get "
+ "forwarder name of forwarder export '{0}': {1}",
+ sym_name, llvm::fmt_consume(std::move(err)));
+continue;
+  }
+  llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
+ " (forwarded to ", forwarder_name,
+ ")"};
+  symbol.GetMangled().SetDemangledName(ConstString(new_name.str()));
+  symbol.SetDemangledNameIsSynthesized(true);
+}
+
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
   LLDB_LOG(log,
@@ -886,9 +906,10 @@
 // An exported symbol may be either code or data. Guess by checking whether
 // the section containing the symbol is executable.
 symbol.SetType(lldb::eSymbolTypeData);
-if (auto section_sp = symbol.GetAddressRef().GetSection())
-  if (section_sp->GetPermissions() & ePermissionsExecutable)
-symbol.SetType(lldb::eSymbolTypeCode);
+if (!is_forwarder)
+  if (auto section_sp = symbol.GetAddressRef().GetSection())
+if (section_sp->GetPermissions() & ePermissionsExecutable)
+  symbol.SetType(lldb::eSymbolTypeCode);
 symbol.SetExternal(true);
 uint32_t idx = symtab.AddSymbol(symbol);
 export_list.push_back(std::make_pair(function_rva, idx));
___
lldb-commits

[Lldb-commits] [PATCH] D134517: [lldb][COFF] Load absolute symbols from COFF symbol table

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7ebff6ab26aa: [lldb][COFF] Load absolute symbols from COFF 
symbol table (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134517

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -4,9 +4,10 @@
 ## The .file symbol isn't checked, but is included to test that the symbol
 ## table iteration handles cases with a symbol with more than one aux symbol.
 
-# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
-# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Type File Address/Value {{.*}} SizeFlags   
Name
+# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
entry
+# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
absolute_symbol
 
 --- !COFF
 OptionalHeader:
@@ -123,4 +124,10 @@
 SimpleType:  IMAGE_SYM_TYPE_NULL
 ComplexType: IMAGE_SYM_DTYPE_NULL
 StorageClass:IMAGE_SYM_CLASS_EXTERNAL
+  - Name:absolute_symbol
+Value:   0xdeadbeef
+SectionNumber:   -1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
 ...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -830,6 +830,9 @@
 symbol.SetType(exported->GetType());
 }
   }
+} else if (section_number == llvm::COFF::IMAGE_SYM_ABSOLUTE) {
+  symbol.GetAddressRef() = Address(coff_sym_ref.getValue());
+  symbol.SetType(lldb::eSymbolTypeAbsolute);
 }
 symtab.AddSymbol(symbol);
   }


Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -4,9 +4,10 @@
 ## The .file symbol isn't checked, but is included to test that the symbol
 ## table iteration handles cases with a symbol with more than one aux symbol.
 
-# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
-# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Type File Address/Value {{.*}} SizeFlags   Name
+# CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} absolute_symbol
 
 --- !COFF
 OptionalHeader:
@@ -123,4 +124,10 @@
 SimpleType:  IMAGE_SYM_TYPE_NULL
 ComplexType: IMAGE_SYM_DTYPE_NULL
 StorageClass:IMAGE_SYM_CLASS_EXTERNAL
+  - Name:absolute_symbol
+Value:   0xdeadbeef
+SectionNumber:   -1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
 ...
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -830,6 +830,9 @@
 symbol.SetType(exported->GetType());
 }
   }
+} else if (section_number == llvm::COFF::IMAGE_SYM_ABSOLUTE) {
+  symbol.GetAddressRef() = Address(coff_sym_ref.getValue());
+  symbol.SetType(lldb::eSymbolTypeAbsolute);
 }
 symtab.AddSymbol(symbol);
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG20c2f94c3cc1: [lldb][COFF] Match symbols from COFF symbol 
table to export symbols (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml

Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -4,18 +4,18 @@
 # Checks that the symtab contains both symbols from the export table and the
 # COFF symbol table.
 
-# CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
+# CHECK:  UserID DSX Type   File Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
-# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT:  1   X Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code   0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data   0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code   0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295   X Additional 0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
+# CHECK-NEXT: 4294967295   X Additional 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
+# CHECK-NEXT: 4294967295 Invalid0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -268,10 +268,12 @@
 
 private:
   bool CreateBinary();
+  typedef std::vector> rva_symbol_list_t;
   void AppendFromCOFFSymbolTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
-  void AppendFromExportTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
+ lldb_private::Symtab ,
+ const rva_symbol_list_t _exports);
+  rva_symbol_list_t AppendFromExportTable(lldb_private::SectionList *sect_list,
+  lldb_private::Symtab );
 
   dos_header_t m_dos_header;
   coff_header_t m_coff_header;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -761,12 +761,18 @@
 
 void ObjectFilePECOFF::ParseSymtab(Symtab ) {
   SectionList *sect_list = GetSectionList();
-  AppendFromExportTable(sect_list, symtab);
-  AppendFromCOFFSymbolTable(sect_list, symtab);
+  rva_symbol_list_t sorted_exports = AppendFromExportTable(sect_list, symtab);
+  AppendFromCOFFSymbolTable(sect_list, symtab, sorted_exports);
 }
 
-void

[Lldb-commits] [PATCH] D134196: [lldb][COFF] Rewrite ParseSymtab to list both export and symbol tables

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbf0cda9ed278: [lldb][COFF] Rewrite ParseSymtab to list both 
export and symbol tables (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134196

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
  llvm/include/llvm/Object/COFF.h

Index: llvm/include/llvm/Object/COFF.h
===
--- llvm/include/llvm/Object/COFF.h
+++ llvm/include/llvm/Object/COFF.h
@@ -914,6 +914,10 @@
 
   uint32_t getStringTableSize() const { return StringTableSize; }
 
+  const export_directory_table_entry *getExportTable() const {
+return ExportDirectory;
+  }
+
   const coff_load_configuration32 *getLoadConfig32() const {
 assert(!is64());
 return reinterpret_cast(LoadConfig);
Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -0,0 +1,109 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# Checks that the symtab contains both symbols from the export table and the
+# COFF symbol table.
+
+# CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
+# CHECK-NEXT: --
+# CHECK-NEXT: 4294967295 D   Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT: 4294967295 D   Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 D   Code0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 D   Code0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
+# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 Invalid 0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
+# CHECK-NEXT: 4294967295 Invalid 0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-EMPTY:
+
+# Test file generated with:
+#   clang -O2 --target=x86_64-windows-msvc test.c -nostdlib -c -o test.obj
+#   lld-link -debug:symtab -dll -out:test.dll -entry:entry -export:exportFnAlias=aliasFunc -export:exportIntAlias=aliasInt test.obj
+# test.c:
+#   __declspec(dllexport) int exportInt;
+#   int aliasInt;
+#   int internalInt;
+#   void entry(void) {}
+#   __declspec(dllexport) void exportFunc(void) {}
+#   void aliasFunc(void) {}
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:   6442450944
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:   IMAGE_SUBSYSTEM_WINDOWS_GUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+RelativeVirtualAddress: 8192
+Size:156
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  4096
+VirtualSize: 33
+SectionData: C32E0F1F8400C32E0F1F8400C3
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+VirtualAddress:  8192
+VirtualSize: 156
+SectionData: 282001000400040042205220622073796D626F6C732D6578706F7274732E632E746D702E646C6C0020101010003004306A20782083208D200100020003006578706F7274466E416C696173006578706F727446756E63006578706F7274496E74006578706F7274496E74416C69617300
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+VirtualAddress:  12288
+VirtualSize: 12
+SectionData: ''
+symbols:
+  - Name:

[Lldb-commits] [PATCH] D134265: [lldb][COFF] Improve info of symbols from export table

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0870afc68e1a: [lldb][COFF] Improve info of symbols from 
export table (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134265

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -6,10 +6,10 @@
 
 # CHECK:  UserID DSX TypeFile Address/Value {{.*}} Size
Flags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT: 4294967295 D   Code0x0001800010200x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT: 4294967295 D   Code0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 D   Code0x0001800030000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 D   Code0x0001800030040x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportIntAlias
 # CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} entry
 # CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportFunc
 # CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} aliasFunc
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -826,6 +826,10 @@
 // Note: symbol name may be empty if it is only exported by ordinal.
 symbol.GetMangled().SetValue(ConstString(sym_name));
 
+uint32_t ordinal;
+llvm::cantFail(entry.getOrdinal(ordinal));
+symbol.SetID(ordinal);
+
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
   LLDB_LOG(log,
@@ -834,10 +838,19 @@
sym_name, llvm::fmt_consume(std::move(err)));
   continue;
 }
+// Skip the symbol if it doesn't look valid.
+if (function_rva == 0 && sym_name.empty())
+  continue;
 symbol.GetAddressRef() =
 Address(m_coff_header_opt.image_base + function_rva, sect_list);
-symbol.SetType(lldb::eSymbolTypeCode);
-symbol.SetDebug(true);
+
+// An exported symbol may be either code or data. Guess by checking whether
+// the section containing the symbol is executable.
+symbol.SetType(lldb::eSymbolTypeData);
+if (auto section_sp = symbol.GetAddressRef().GetSection())
+  if (section_sp->GetPermissions() & ePermissionsExecutable)
+symbol.SetType(lldb::eSymbolTypeCode);
+symbol.SetExternal(true);
 symtab.AddSymbol(symbol);
   }
 }


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -6,10 +6,10 @@
 
 # CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT: 4294967295 D   Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT: 4294967295 D   Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 D   Code0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 D   Code0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
 # CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
 # CHECK-NEXT: 4294967295 Code0x000180001010

[Lldb-commits] [PATCH] D134516: [lldb] Improve display of absolute symbol lookup

2022-09-27 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf4991bfa891e: [lldb] Improve display of absolute symbol 
lookup (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134516

Files:
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/test/Shell/SymbolFile/absolute-symbol.test


Index: lldb/test/Shell/SymbolFile/absolute-symbol.test
===
--- lldb/test/Shell/SymbolFile/absolute-symbol.test
+++ lldb/test/Shell/SymbolFile/absolute-symbol.test
@@ -1,6 +1,7 @@
 # RUN: yaml2obj %s -o %t.o
 # RUN: %lldb -b -o 'target modules lookup -s absolute_symbol' %t.o | FileCheck 
%s
 # CHECK: 1 symbols match 'absolute_symbol'
+# CHECK:   Name: absolute_symbol
 # CHECK:   Value: 0x12345678
 # Created from:
 #   .globl absolute_symbol
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -1549,12 +1549,16 @@
   strm.EOL();
 } else {
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());
+  strm.EOL();
   strm.Indent("Value: ");
   strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetRawValue());
   if (symbol->GetByteSizeIsValid()) {
 strm.Indent("Size: ");
 strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetByteSize());
   }
+  strm.IndentLess();
 }
   }
 }


Index: lldb/test/Shell/SymbolFile/absolute-symbol.test
===
--- lldb/test/Shell/SymbolFile/absolute-symbol.test
+++ lldb/test/Shell/SymbolFile/absolute-symbol.test
@@ -1,6 +1,7 @@
 # RUN: yaml2obj %s -o %t.o
 # RUN: %lldb -b -o 'target modules lookup -s absolute_symbol' %t.o | FileCheck %s
 # CHECK: 1 symbols match 'absolute_symbol'
+# CHECK:   Name: absolute_symbol
 # CHECK:   Value: 0x12345678
 # Created from:
 #   .globl absolute_symbol
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -1549,12 +1549,16 @@
   strm.EOL();
 } else {
   strm.IndentMore();
+  strm.Indent("Name: ");
+  strm.PutCString(symbol->GetDisplayName().GetStringRef());
+  strm.EOL();
   strm.Indent("Value: ");
   strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetRawValue());
   if (symbol->GetByteSizeIsValid()) {
 strm.Indent("Size: ");
 strm.Printf("0x%16.16" PRIx64 "\n", symbol->GetByteSize());
   }
+  strm.IndentLess();
 }
   }
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134111: [lldb] Add newline in output of `target modules lookup`

2022-09-27 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa426753ef029: [lldb] Add newline in output of `target 
modules lookup` (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134111

Files:
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/test/Shell/Commands/Inputs/symbols.yaml
  lldb/test/Shell/Commands/command-target-modules-lookup.test


Index: lldb/test/Shell/Commands/command-target-modules-lookup.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-target-modules-lookup.test
@@ -0,0 +1,12 @@
+# RUN: yaml2obj %S/Inputs/symbols.yaml -o %t
+
+# RUN: %lldb %t -b -o "target modules lookup -A -r -s some" | FileCheck %s 
-DMODULE=%basename_t.tmp --implicit-check-not ignoreThisFunction
+# CHECK:  4 symbols match the regular expression 'some' in 
{{.*}}[[MODULE]]:
+# CHECK-NEXT: Address: [[MODULE]][0x] ([[MODULE]]..text + 0)
+# CHECK-NEXT: Summary: [[MODULE]]`someFunc(int, int, int)
+# CHECK-NEXT: Address: [[MODULE]][0x001c] ([[MODULE]]..text + 28)
+# CHECK-NEXT: Summary: [[MODULE]]`someFunc(char, int)
+# CHECK-NEXT: Address: [[MODULE]][0x0034] ([[MODULE]]..text + 52)
+# CHECK-NEXT: Summary: [[MODULE]]`someOtherFunc()
+# CHECK-NEXT: Address: [[MODULE]][0x0038] ([[MODULE]]..text + 56)
+# CHECK-NEXT: Summary: [[MODULE]]`someOtherFunc(double)
Index: lldb/test/Shell/Commands/Inputs/symbols.yaml
===
--- /dev/null
+++ lldb/test/Shell/Commands/Inputs/symbols.yaml
@@ -0,0 +1,48 @@
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_REL
+  Machine: EM_AARCH64
+  SectionHeaderStringTable: .strtab
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+AddressAlign:0x4
+  - Type:SectionHeaderTable
+Sections:
+  - Name:.text
+  - Name:.strtab
+  - Name:.symtab
+Symbols:
+  - Name:_Z8someFunciii
+Type:STT_FUNC
+Section: .text
+Binding: STB_GLOBAL
+Size:0x1C
+  - Name:_Z8someFuncci
+Type:STT_FUNC
+Section: .text
+Binding: STB_GLOBAL
+Value:   0x1C
+Size:0x18
+  - Name:_Z13someOtherFuncv
+Type:STT_FUNC
+Section: .text
+Binding: STB_GLOBAL
+Value:   0x34
+Size:0x4
+  - Name:_Z13someOtherFuncd
+Type:STT_FUNC
+Section: .text
+Binding: STB_GLOBAL
+Value:   0x38
+Size:0x10
+  - Name:_Z18ignoreThisFunctionv
+Type:STT_FUNC
+Section: .text
+Binding: STB_GLOBAL
+Value:   0x48
+Size:0x8
+...
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -1546,6 +1546,7 @@
   DumpAddress(
   interpreter.GetExecutionContext().GetBestExecutionContextScope(),
   symbol->GetAddressRef(), verbose, all_ranges, strm);
+  strm.EOL();
 } else {
   strm.IndentMore();
   strm.Indent("Value: ");


Index: lldb/test/Shell/Commands/command-target-modules-lookup.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-target-modules-lookup.test
@@ -0,0 +1,12 @@
+# RUN: yaml2obj %S/Inputs/symbols.yaml -o %t
+
+# RUN: %lldb %t -b -o "target modules lookup -A -r -s some" | FileCheck %s -DMODULE=%basename_t.tmp --implicit-check-not ignoreThisFunction
+# CHECK:  4 symbols match the regular expression 'some' in {{.*}}[[MODULE]]:
+# CHECK-NEXT: Address: [[MODULE]][0x] ([[MODULE]]..text + 0)
+# CHECK-NEXT: Summary: [[MODULE]]`someFunc(int, int, int)
+# CHECK-NEXT: Address: [[MODULE]][0x001c] ([[MODULE]]..text + 28)
+# CHECK-NEXT: Summary: [[MODULE]]`someFunc(char, int)
+# CHECK-NEXT: Address: [[MODULE]][0x0034] ([[MODULE]]..text + 52)
+# CHECK-NEXT: Summary: [[MODULE]]`someOtherFunc()
+# CHECK-NEXT: Address: [[MODULE]][0x0038] ([[MODULE]]..text + 56)
+# CHECK-NEXT: Summary: [[MODULE]]`someOtherFunc(double)
Index: lldb/test/Shell/Commands/Inputs/symbols.yaml
===
--- /dev/null
+++ lldb/test/Shell/Commands/Inputs/symbols.yaml
@@ -0,0 +1,48 @@
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D134037#3815240 , @DavidSpickett 
wrote:

>> I might add the AArch64 and Arm equivalent if I have some spare time.
>
> I looked into this and there's a detail that makes this work on x86 only. So 
> I'll leave it as is, it's getting tested somewhere, and that's what matters.
>
> If you're curious this is the spanner in the works:
>
>   size_t NativeProcessProtocol::GetSoftwareBreakpointPCOffset() {
> switch (GetArchitecture().GetMachine()) {
> case llvm::Triple::x86:
> case llvm::Triple::x86_64:
> case llvm::Triple::systemz:
>   // These architectures report increment the PC after breakpoint is hit.
>   return cantFail(GetSoftwareBreakpointTrapOpcode(0)).size();
>
> Meaning AArch64 doesn't. lldb on x86 can continue past a break in the program 
> because of this:
>
>   (lldb) run
>   Process 31032 launched: '/tmp/test.o' (x86_64)
>   Process 31032 stopped
>   * thread #1, name = 'test.o', stop reason = signal SIGTRAP
>   frame #0: 0x460f test.o`main at test.c:3
>  1int main() {
>  2__asm__ __volatile__("int3");
>   -> 3  return 0;
>  4}
>   (lldb) c
>   Process 31032 resuming
>   Process 31032 exited with status = 0 (0x)
>
> On AArch64 we just stick on the break:
>
>   (lldb) run
>   Process 2162869 launched: '/tmp/test.o' (aarch64)
>   Process 2162869 stopped
>   * thread #1, name = 'test.o', stop reason = signal SIGTRAP
>   frame #0: 0xa71c test.o`main at test.c:2:3
>  1int main() {
>   -> 2  asm volatile("brk #0xf000\n\t");
>  3  return 0;
>  4}
>   (lldb) c
>   Process 2162869 resuming
>   Process 2162869 stopped
>   * thread #1, name = 'test.o', stop reason = signal SIGTRAP
>   frame #0: 0xa71c test.o`main at test.c:2:3
>  1int main() {
>   -> 2  asm volatile("brk #0xf000\n\t");
>  3  return 0;
>  4}
>
> gdb has the same behaviour on AArch64 so I'll leave it as is until someone 
> complains.

Ah, yes, I remember hitting that issue occasionally. On the other hand, if it 
would be somewhat doable to fix it, it'd would be a great value add IMO - I've 
had to do acrobatics to set the program counter to the next instruction to 
resume running after such a breakpoint a couple times...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

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

[Lldb-commits] [PATCH] D134636: [lldb][Windows] Always call SetExecutableModule on debugger connected

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a reviewer: jasonmolenda.
mstorsjo added a subscriber: jasonmolenda.
mstorsjo added a comment.

The test runs fine on Windows, but I'm not familiar with these aspects of lldb 
to (yet) have a good opinion on it; adding @jasonmolenda to the review who 
commented on the previous one too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134636

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:7
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll \
+# RUN: %if windows-msvc %{-Wl,-implib:%t.shlib.lib} \
+# RUN: %else %{-Wl,--out-implib=%t.shlib.lib%}

This closing `}` is lacking the preceding `%`, i.e. it should be `%}`, like on 
the second row.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:7
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \

alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > mstorsjo wrote:
> > > > alvinhochun wrote:
> > > > > mstorsjo wrote:
> > > > > > mstorsjo wrote:
> > > > > > > Unfortunately, this aspect of the test doesn't work as is on 
> > > > > > > Windows.
> > > > > > > 
> > > > > > > By default (in msvc builds), clang invokes link.exe here, but 
> > > > > > > link.exe can't take `%t.shlib.dll` as input file to the linker, 
> > > > > > > as it requires an import library.
> > > > > > > 
> > > > > > > If we would assume that lld is available, we could run the 
> > > > > > > linking command with `-fuse-ld=lld` - however that on its own 
> > > > > > > isn't enough, as lld only allows linking against DLLs when run 
> > > > > > > with the `-lldmingw` flag. We can add `-fuse-ld=lld 
> > > > > > > -Wl,-lldmingw` here to fix that, but that would break testing in 
> > > > > > > mingw environments, as `-lldmingw` isn't a valid option on the 
> > > > > > > mingw lld driver.
> > > > > > > 
> > > > > > > Likewise, we could create a proper import library by adding 
> > > > > > > `-Wl,-implib:%t.shlib.lib` on the first command above, but that 
> > > > > > > doesn't work in mingw mode either, as it would have to be 
> > > > > > > `-Wl,--out-implib=%t.shlib.lib` instead.
> > > > > > > 
> > > > > > > In practice, running the lldb tests in mingw mode is not entirely 
> > > > > > > supported, while they do pass cleanly in MSVC mode (and there's a 
> > > > > > > buildbot testing this configuration) - but I would like to work 
> > > > > > > towards making things work better in the mingw configuration too.
> > > > > > > 
> > > > > > > There's a different substitution, `%build`, which invokes the 
> > > > > > > `lldb/test/Shell/helpers/build.py` script, which abstracts a 
> > > > > > > bunch of boilerplate details mostly relevant for windows targets 
> > > > > > > (like creating PDB debug info files); the easiest at this point 
> > > > > > > would probably be to extend that script with options for creating 
> > > > > > > import libraries.
> > > > > > > 
> > > > > > > For testing with mingw, I'm currently using this out of tree 
> > > > > > > patch for that script too:
> > > > > > > ```
> > > > > > > diff --git a/lldb/test/Shell/helper/build.py 
> > > > > > > b/lldb/test/Shell/helper/build.py
> > > > > > > index 96684b7b3e66..f138b00bee9e 100755
> > > > > > > --- a/lldb/test/Shell/helper/build.py
> > > > > > > +++ b/lldb/test/Shell/helper/build.py
> > > > > > > @@ -191,7 +191,10 @@ def find_toolchain(compiler, tools_dir):
> > > > > > >  if compiler == 'any':
> > > > > > >  priorities = []
> > > > > > >  if sys.platform == 'win32':
> > > > > > > -priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> > > > > > > +if 'gcc' in sys.version.lower():
> > > > > > > +priorities = ['clang', 'gcc', 'clang-cl', 'msvc']
> > > > > > > +else:
> > > > > > > +priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> > > > > > >  else:
> > > > > > >  priorities = ['clang', 'gcc', 'clang-cl']
> > > > > > >  for toolchain in priorities:
> > > > > > > ```
> > > > > > > (This is a bit hacky, since it uses the build type of the python 
> > > > > > > interpreter to switch the preference between clang-cl and clang.)
> > > > > > > 
> > > > > > > Alternatively, we could maybe add a separate substitution that 
> > > > > > > expands into either `-implib:` or `--out-implib=`, but I'm not 
> > > > > > > sure if we can guess which one will be the right one either, 
> > > > > > > while the build.py helper script probably can do better.
> > > > > > I forgot to mention; see https://reviews.llvm.org/D129455 for some 
> > > > > > earlier discussion about other aspects (symbol table vs dwarf vs 
> > > > > > PDB) for windows testcases in lldb.
> > > > > Re different options on msvc vs mingw, how about using the `%if` 
> > > > > substitution (https://www.llvm.org/docs/TestingGuide.html)? We can 
> > > > > use it to check for the `windows-msvc` feature to apply options 
> > > > > conditionally.
> > > > Oh, I guess that would work too - it does litter the individual tests 
> > > > with the conditions instead of hiding it in `build.py`, but probably is 
> > > > acceptable too.
> > > The commands should become this:
> > > 
> > > ```
> > > # RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll %if 
> > > windows-msvc %{-Wl,-implib:%t.shlib.lib}
> > > # RUN: %clang_host -g0 -O0 %S/Inputs/main.c -o %t.main.exe %if 
> > > windows-msvc %{%t.shlib.lib} %else %{%t.shlib.dll}
> > > ```
> > This form

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:7
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \

alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > mstorsjo wrote:
> > > > mstorsjo wrote:
> > > > > Unfortunately, this aspect of the test doesn't work as is on Windows.
> > > > > 
> > > > > By default (in msvc builds), clang invokes link.exe here, but 
> > > > > link.exe can't take `%t.shlib.dll` as input file to the linker, as it 
> > > > > requires an import library.
> > > > > 
> > > > > If we would assume that lld is available, we could run the linking 
> > > > > command with `-fuse-ld=lld` - however that on its own isn't enough, 
> > > > > as lld only allows linking against DLLs when run with the `-lldmingw` 
> > > > > flag. We can add `-fuse-ld=lld -Wl,-lldmingw` here to fix that, but 
> > > > > that would break testing in mingw environments, as `-lldmingw` isn't 
> > > > > a valid option on the mingw lld driver.
> > > > > 
> > > > > Likewise, we could create a proper import library by adding 
> > > > > `-Wl,-implib:%t.shlib.lib` on the first command above, but that 
> > > > > doesn't work in mingw mode either, as it would have to be 
> > > > > `-Wl,--out-implib=%t.shlib.lib` instead.
> > > > > 
> > > > > In practice, running the lldb tests in mingw mode is not entirely 
> > > > > supported, while they do pass cleanly in MSVC mode (and there's a 
> > > > > buildbot testing this configuration) - but I would like to work 
> > > > > towards making things work better in the mingw configuration too.
> > > > > 
> > > > > There's a different substitution, `%build`, which invokes the 
> > > > > `lldb/test/Shell/helpers/build.py` script, which abstracts a bunch of 
> > > > > boilerplate details mostly relevant for windows targets (like 
> > > > > creating PDB debug info files); the easiest at this point would 
> > > > > probably be to extend that script with options for creating import 
> > > > > libraries.
> > > > > 
> > > > > For testing with mingw, I'm currently using this out of tree patch 
> > > > > for that script too:
> > > > > ```
> > > > > diff --git a/lldb/test/Shell/helper/build.py 
> > > > > b/lldb/test/Shell/helper/build.py
> > > > > index 96684b7b3e66..f138b00bee9e 100755
> > > > > --- a/lldb/test/Shell/helper/build.py
> > > > > +++ b/lldb/test/Shell/helper/build.py
> > > > > @@ -191,7 +191,10 @@ def find_toolchain(compiler, tools_dir):
> > > > >  if compiler == 'any':
> > > > >  priorities = []
> > > > >  if sys.platform == 'win32':
> > > > > -priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> > > > > +if 'gcc' in sys.version.lower():
> > > > > +priorities = ['clang', 'gcc', 'clang-cl', 'msvc']
> > > > > +else:
> > > > > +priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> > > > >  else:
> > > > >  priorities = ['clang', 'gcc', 'clang-cl']
> > > > >  for toolchain in priorities:
> > > > > ```
> > > > > (This is a bit hacky, since it uses the build type of the python 
> > > > > interpreter to switch the preference between clang-cl and clang.)
> > > > > 
> > > > > Alternatively, we could maybe add a separate substitution that 
> > > > > expands into either `-implib:` or `--out-implib=`, but I'm not sure 
> > > > > if we can guess which one will be the right one either, while the 
> > > > > build.py helper script probably can do better.
> > > > I forgot to mention; see https://reviews.llvm.org/D129455 for some 
> > > > earlier discussion about other aspects (symbol table vs dwarf vs PDB) 
> > > > for windows testcases in lldb.
> > > Re different options on msvc vs mingw, how about using the `%if` 
> > > substitution (https://www.llvm.org/docs/TestingGuide.html)? We can use it 
> > > to check for the `windows-msvc` feature to apply options conditionally.
> > Oh, I guess that would work too - it does litter the individual tests with 
> > the conditions instead of hiding it in `build.py`, but probably is 
> > acceptable too.
> The commands should become this:
> 
> ```
> # RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll %if 
> windows-msvc %{-Wl,-implib:%t.shlib.lib}
> # RUN: %clang_host -g0 -O0 %S/Inputs/main.c -o %t.main.exe %if windows-msvc 
> %{%t.shlib.lib} %else %{%t.shlib.dll}
> ```
This form works, and keeps it to just one conditional:
```
# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll %if 
windows-msvc %{-Wl,-implib:%t.shlib.lib%} %else 
%{-Wl,--out-implib=%t.shlib.lib%}
# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.lib -o %t.main.exe
```


Repository:
  rG LLVM Github Monorepo

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:7
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \

alvinhochun wrote:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > Unfortunately, this aspect of the test doesn't work as is on Windows.
> > > 
> > > By default (in msvc builds), clang invokes link.exe here, but link.exe 
> > > can't take `%t.shlib.dll` as input file to the linker, as it requires an 
> > > import library.
> > > 
> > > If we would assume that lld is available, we could run the linking 
> > > command with `-fuse-ld=lld` - however that on its own isn't enough, as 
> > > lld only allows linking against DLLs when run with the `-lldmingw` flag. 
> > > We can add `-fuse-ld=lld -Wl,-lldmingw` here to fix that, but that would 
> > > break testing in mingw environments, as `-lldmingw` isn't a valid option 
> > > on the mingw lld driver.
> > > 
> > > Likewise, we could create a proper import library by adding 
> > > `-Wl,-implib:%t.shlib.lib` on the first command above, but that doesn't 
> > > work in mingw mode either, as it would have to be 
> > > `-Wl,--out-implib=%t.shlib.lib` instead.
> > > 
> > > In practice, running the lldb tests in mingw mode is not entirely 
> > > supported, while they do pass cleanly in MSVC mode (and there's a 
> > > buildbot testing this configuration) - but I would like to work towards 
> > > making things work better in the mingw configuration too.
> > > 
> > > There's a different substitution, `%build`, which invokes the 
> > > `lldb/test/Shell/helpers/build.py` script, which abstracts a bunch of 
> > > boilerplate details mostly relevant for windows targets (like creating 
> > > PDB debug info files); the easiest at this point would probably be to 
> > > extend that script with options for creating import libraries.
> > > 
> > > For testing with mingw, I'm currently using this out of tree patch for 
> > > that script too:
> > > ```
> > > diff --git a/lldb/test/Shell/helper/build.py 
> > > b/lldb/test/Shell/helper/build.py
> > > index 96684b7b3e66..f138b00bee9e 100755
> > > --- a/lldb/test/Shell/helper/build.py
> > > +++ b/lldb/test/Shell/helper/build.py
> > > @@ -191,7 +191,10 @@ def find_toolchain(compiler, tools_dir):
> > >  if compiler == 'any':
> > >  priorities = []
> > >  if sys.platform == 'win32':
> > > -priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> > > +if 'gcc' in sys.version.lower():
> > > +priorities = ['clang', 'gcc', 'clang-cl', 'msvc']
> > > +else:
> > > +priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> > >  else:
> > >  priorities = ['clang', 'gcc', 'clang-cl']
> > >  for toolchain in priorities:
> > > ```
> > > (This is a bit hacky, since it uses the build type of the python 
> > > interpreter to switch the preference between clang-cl and clang.)
> > > 
> > > Alternatively, we could maybe add a separate substitution that expands 
> > > into either `-implib:` or `--out-implib=`, but I'm not sure if we can 
> > > guess which one will be the right one either, while the build.py helper 
> > > script probably can do better.
> > I forgot to mention; see https://reviews.llvm.org/D129455 for some earlier 
> > discussion about other aspects (symbol table vs dwarf vs PDB) for windows 
> > testcases in lldb.
> Re different options on msvc vs mingw, how about using the `%if` substitution 
> (https://www.llvm.org/docs/TestingGuide.html)? We can use it to check for the 
> `windows-msvc` feature to apply options conditionally.
Oh, I guess that would work too - it does litter the individual tests with the 
conditions instead of hiding it in `build.py`, but probably is acceptable too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:7
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \

mstorsjo wrote:
> Unfortunately, this aspect of the test doesn't work as is on Windows.
> 
> By default (in msvc builds), clang invokes link.exe here, but link.exe can't 
> take `%t.shlib.dll` as input file to the linker, as it requires an import 
> library.
> 
> If we would assume that lld is available, we could run the linking command 
> with `-fuse-ld=lld` - however that on its own isn't enough, as lld only 
> allows linking against DLLs when run with the `-lldmingw` flag. We can add 
> `-fuse-ld=lld -Wl,-lldmingw` here to fix that, but that would break testing 
> in mingw environments, as `-lldmingw` isn't a valid option on the mingw lld 
> driver.
> 
> Likewise, we could create a proper import library by adding 
> `-Wl,-implib:%t.shlib.lib` on the first command above, but that doesn't work 
> in mingw mode either, as it would have to be `-Wl,--out-implib=%t.shlib.lib` 
> instead.
> 
> In practice, running the lldb tests in mingw mode is not entirely supported, 
> while they do pass cleanly in MSVC mode (and there's a buildbot testing this 
> configuration) - but I would like to work towards making things work better 
> in the mingw configuration too.
> 
> There's a different substitution, `%build`, which invokes the 
> `lldb/test/Shell/helpers/build.py` script, which abstracts a bunch of 
> boilerplate details mostly relevant for windows targets (like creating PDB 
> debug info files); the easiest at this point would probably be to extend that 
> script with options for creating import libraries.
> 
> For testing with mingw, I'm currently using this out of tree patch for that 
> script too:
> ```
> diff --git a/lldb/test/Shell/helper/build.py b/lldb/test/Shell/helper/build.py
> index 96684b7b3e66..f138b00bee9e 100755
> --- a/lldb/test/Shell/helper/build.py
> +++ b/lldb/test/Shell/helper/build.py
> @@ -191,7 +191,10 @@ def find_toolchain(compiler, tools_dir):
>  if compiler == 'any':
>  priorities = []
>  if sys.platform == 'win32':
> -priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
> +if 'gcc' in sys.version.lower():
> +priorities = ['clang', 'gcc', 'clang-cl', 'msvc']
> +else:
> +priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
>  else:
>  priorities = ['clang', 'gcc', 'clang-cl']
>  for toolchain in priorities:
> ```
> (This is a bit hacky, since it uses the build type of the python interpreter 
> to switch the preference between clang-cl and clang.)
> 
> Alternatively, we could maybe add a separate substitution that expands into 
> either `-implib:` or `--out-implib=`, but I'm not sure if we can guess which 
> one will be the right one either, while the build.py helper script probably 
> can do better.
I forgot to mention; see https://reviews.llvm.org/D129455 for some earlier 
discussion about other aspects (symbol table vs dwarf vs PDB) for windows 
testcases in lldb.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D134585#3814463 , @DavidSpickett 
wrote:

>> MSVC/link.exe doesn't write symbols into linked PE images at all.
>
> So by the time we get to a debugger, it's not an issue anyway.

Yep, most of these patches about symbol table handling doesn't make any 
difference for the MSVC ecosystem usage at all (although some of these patches 
fix other generic windows-specific bugs noticed too).

> Then this LGTM.

Thanks! Can you mark it formally approved too? :-)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:7
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \

Unfortunately, this aspect of the test doesn't work as is on Windows.

By default (in msvc builds), clang invokes link.exe here, but link.exe can't 
take `%t.shlib.dll` as input file to the linker, as it requires an import 
library.

If we would assume that lld is available, we could run the linking command with 
`-fuse-ld=lld` - however that on its own isn't enough, as lld only allows 
linking against DLLs when run with the `-lldmingw` flag. We can add 
`-fuse-ld=lld -Wl,-lldmingw` here to fix that, but that would break testing in 
mingw environments, as `-lldmingw` isn't a valid option on the mingw lld driver.

Likewise, we could create a proper import library by adding 
`-Wl,-implib:%t.shlib.lib` on the first command above, but that doesn't work in 
mingw mode either, as it would have to be `-Wl,--out-implib=%t.shlib.lib` 
instead.

In practice, running the lldb tests in mingw mode is not entirely supported, 
while they do pass cleanly in MSVC mode (and there's a buildbot testing this 
configuration) - but I would like to work towards making things work better in 
the mingw configuration too.

There's a different substitution, `%build`, which invokes the 
`lldb/test/Shell/helpers/build.py` script, which abstracts a bunch of 
boilerplate details mostly relevant for windows targets (like creating PDB 
debug info files); the easiest at this point would probably be to extend that 
script with options for creating import libraries.

For testing with mingw, I'm currently using this out of tree patch for that 
script too:
```
diff --git a/lldb/test/Shell/helper/build.py b/lldb/test/Shell/helper/build.py
index 96684b7b3e66..f138b00bee9e 100755
--- a/lldb/test/Shell/helper/build.py
+++ b/lldb/test/Shell/helper/build.py
@@ -191,7 +191,10 @@ def find_toolchain(compiler, tools_dir):
 if compiler == 'any':
 priorities = []
 if sys.platform == 'win32':
-priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
+if 'gcc' in sys.version.lower():
+priorities = ['clang', 'gcc', 'clang-cl', 'msvc']
+else:
+priorities = ['clang-cl', 'msvc', 'clang', 'gcc']
 else:
 priorities = ['clang', 'gcc', 'clang-cl']
 for toolchain in priorities:
```
(This is a bit hacky, since it uses the build type of the python interpreter to 
switch the preference between clang-cl and clang.)

Alternatively, we could maybe add a separate substitution that expands into 
either `-implib:` or `--out-implib=`, but I'm not sure if we can guess which 
one will be the right one either, while the build.py helper script probably can 
do better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D134585#3814458 , @DavidSpickett 
wrote:

> You're the expert on what the linker does and the code looks fine.
>
> Is it possible that msvc uses these `IMAGE_SYM_TYPE_NULL` in a different way 
> that could cause a problem? Worst that happens is we get a bunch of symbols 
> available in expressions that shouldn't be?

MSVC/link.exe doesn't write symbols into linked PE images at all. But at the 
object file stage, they do express data symbols in the same way (which is the 
default marking for symbols that have no other types set).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8a3597d73c8f: [lldb] Fix 
CommandInterpreter::DidProcessStopAbnormally() with multiple threads (authored 
by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D134037?vs=462454=462836#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

Files:
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
  lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,5 @@
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | 
FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2471,8 +2471,12 @@
 
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
-if (!stop_info)
-  return false;
+if (!stop_info) {
+  // If there's no stop_info, keep iterating through the other threads;
+  // it's enough that any thread has got a stop_info that indicates
+  // an abnormal stop, to consider the process to be stopped abnormally.
+  continue;
+}
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,5 @@
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2471,8 +2471,12 @@
 
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
-if (!stop_info)
-  return false;
+if (!stop_info) {
+  // If there's no stop_info, keep iterating through the other threads;
+  // it's enough that any thread has got a stop_info that indicates
+  // an abnormal stop, to consider the process to be stopped abnormally.
+  continue;
+}
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test:1
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)

DavidSpickett wrote:
> mstorsjo wrote:
> > DavidSpickett wrote:
> > > Isn't this fixing an issue that you saw on Windows as well?
> > Yes, that was the place where I observed the issue (as Windows apparently, 
> > under some circumstances, can have a couple of extra threads running that 
> > haven't been spawned by the code of the process itself), but it's easily 
> > reproducible on any platform as long as you make sure there's more than one 
> > thread running.
> > 
> > I copied the basis for the test from 
> > `Shell/Register/x86-multithreaded-read.test` - most of the tests there 
> > under `Shell/Register` seem to have such an XFAIL, not sure exactly why. 
> > (In this particular test, at least the `-pthread` linker flag might be a 
> > trivial platform specific difference that might break it, which would need 
> > to be abstracted somewhere.)
> Yeah I didn't notice the pthread flag, that could be it. Makes sense to me.
Actually, it turns out that this test does pass on Windows (I hadn't tested 
before, I had just inherited the XFAIL from the test I based it on) - as 
`-pthread` isn't a linker flag but a compiler driver flag, the msvc target in 
clang just does nothing about it. So I'll remove the XFAIL here before pushing 
it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

2022-09-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

This LGTM, but I'll leave it to some LLDB maintainer to formally approve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > mstorsjo wrote:
> > > > This condition had me puzzled for a moment, until I realized that 
> > > > you're synchronizing symbol type in the other direction here. Is it 
> > > > worth clarifying this in the comment?
> > > > 
> > > > (Also, I presume the test case doesn't really trigger this case?)
> > > Hmm, I can try to improve the comment.
> > > 
> > > The `aliasInt` symbol should trigger this case. Perhaps it will be 
> > > clearer if I update the previous patch to include these symbols, so the 
> > > difference in output can be seen in this patch.
> > Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
> > expected it to return code or data. I guess this is fine then. (Changing 
> > that function, or changing the logic here to look at section types is out 
> > of scope here anyway, and I don’t know if the difference between data and 
> > invalid matters?)
> Data vs Invalid does seem to have an effect on whether a symbol can be 
> printed as data or have its address taken. I can't print or dump any 
> static/globals when the type is Invalid. I think the DWARF debug info should 
> have at least some information about these symbols, but I have no idea how 
> that works.
> 
> I wonder if I should change `MapSymbolType` to return Data for anything that 
> is not code?
I think that might be a good idea - but that's clearly a separate change from 
this one too. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

No further objections/comments from me on this!




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

alvinhochun wrote:
> mstorsjo wrote:
> > This condition had me puzzled for a moment, until I realized that you're 
> > synchronizing symbol type in the other direction here. Is it worth 
> > clarifying this in the comment?
> > 
> > (Also, I presume the test case doesn't really trigger this case?)
> Hmm, I can try to improve the comment.
> 
> The `aliasInt` symbol should trigger this case. Perhaps it will be clearer if 
> I update the previous patch to include these symbols, so the difference in 
> output can be seen in this patch.
Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
expected it to return code or data. I guess this is fine then. (Changing that 
function, or changing the logic here to look at section types is out of scope 
here anyway, and I don’t know if the difference between data and invalid 
matters?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo updated this revision to Diff 462454.
mstorsjo added a comment.

Add a code comment about the code that is changed in the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

Files:
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
  lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,6 @@
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | 
FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2471,8 +2471,12 @@
 
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
-if (!stop_info)
-  return false;
+if (!stop_info) {
+  // If there's no stop_info, keep iterating through the other threads;
+  // it's enough that any thread has got a stop_info that indicates
+  // an abnormal stop, to consider the process to be stopped abnormally.
+  continue;
+}
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,6 @@
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2471,8 +2471,12 @@
 
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
-if (!stop_info)
-  return false;
+if (!stop_info) {
+  // If there's no stop_info, keep iterating through the other threads;
+  // it's enough that any thread has got a stop_info that indicates
+  // an abnormal stop, to consider the process to be stopped abnormally.
+  continue;
+}
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.

Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2443
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
+  continue;

DavidSpickett wrote:
> Please add a comment explaining why we walk all the threads.
> 
> (though in hindsight it does seem the obvious thing to do)
Sure, can do

Comment at: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test:1
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)

DavidSpickett wrote:
> Isn't this fixing an issue that you saw on Windows as well?
Yes, that was the place where I observed the issue (as Windows apparently, 
under some circumstances, can have a couple of extra threads running that 
haven't been spawned by the code of the process itself), but it's easily 
reproducible on any platform as long as you make sure there's more than one 
thread running.

I copied the basis for the test from 
`Shell/Register/x86-multithreaded-read.test` - most of the tests there under 
`Shell/Register` seem to have such an XFAIL, not sure exactly why. (In this 
particular test, at least the `-pthread` linker flag might be a trivial 
platform specific difference that might break it, which would need to be 
abstracted somewhere.)

Comment at: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp:5
+  asm volatile(
+"int3\n\t"
+  );

DavidSpickett wrote:
> This is an x86 breakpoint, right? Shame there is no `__builtin_break` (well, 
> msvc has one apparently but no help for this).
> 
> I might add the AArch64 and Arm equivalent if I have some spare time.
Yes, afaik this is a regular x86 programmatic breakpoint.

About the portability of breakpoints on ARM btw; the exact `udf #xx` code for a 
programmatic breakpoint is OS dependent, and does differ between Linux and 
Windows at least - see e.g. 
https://github.com/mingw-w64/mingw-w64/commit/38c9f0318c6509110706afdc52adc12d8cc34ead.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

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

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

This looks mostly reasonable to me, a couple discussion points only.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+if (symbol_type != lldb::eSymbolTypeInvalid)
+  exported->SetType(symbol_type);
+if (exported->GetMangled() == symbol.GetMangled()) {

As a curious question - since D134265 we get better quality for the symbol 
types set from the get go - what are the other cases you foresee where this 
will make a difference? I don't mind if there isn't any difference though, 
syncing the types from the symbol table which is more expressible probably is 
good anyway. Just wondering about the actual utility of it.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

This condition had me puzzled for a moment, until I realized that you're 
synchronizing symbol type in the other direction here. Is it worth clarifying 
this in the comment?

(Also, I presume the test case doesn't really trigger this case?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D134518#3811153 , @labath wrote:

> They may not be useful (at the moment), but if they're not actively causing 
> harm (e.g. stopping some other feature from functioning, or if we're badly 
> misrepresenting them in the symtab output), then I think we should keep them. 
> You never know -- maybe someone will find a use for them.

Hm, maybe, but they're just plain symbols even without an associated address? 
@alvinhochun If we keep them, do we need to adjust the code below to handle the 
fact that they're addressless?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134518

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

[Lldb-commits] [PATCH] D134196: [lldb][COFF] Rewrite ParseSymtab to list both export and symbol tables

2022-09-21 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

This looks good to me, nothing to add from my PoV. Nice to have this condensed 
to a small enough rewrite, with few enough functional changes to wrap one's 
head around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134196

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

[Lldb-commits] [PATCH] D134265: [lldb][COFF] Improve info of symbols from export table

2022-09-21 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

Looks good to me in general. Do any of the changes make a functional 
difference, or is it just improved (and less misleading) display to the user of 
the debugger?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134265

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

[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables

2022-09-19 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:787
+  Symbol *symbols = symtab.Extend(num_syms);
+  uint32_t i = 0;
+  for (const auto _ref : m_binary->symbols()) {

alvinhochun wrote:
> mstorsjo wrote:
> > If we mean to append to the symbol table here, shouldn't we start with `i = 
> > symtab.size()` or similar? Otherwise we'd start overwriting the existing 
> > symbols from the start of the table?
> I made `symtab.Extend` return a pointer to the first item of the appended 
> range, so counting from 0 does not overwrite existing symbols.
Ah, I see - ok, good then, otherwise I was wondering how this was working at 
all :-)



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:819
+  if (exported->GetType() != lldb::eSymbolTypeReExported &&
+  exported->GetAddressRef() == symbols[i].GetAddressRef()) {
+symbols[i].SetID(exported->GetID());

alvinhochun wrote:
> mstorsjo wrote:
> > What about the case when a symbol is exported with a different name than 
> > the local symbol? (This is doable with def files e.g. as `ExportedName = 
> > LocalName` iirc.) Is it worth to have a map of address -> exported symbol, 
> > to use instead of the raw name?
> Indeed it is a good idea to match symbols with different export names to 
> synchronize the symbol types. I probably should use a `vector export_name>>` sorted by address for lookup instead.
Is it even relevant to keep the name in the vector in that case? As you'd only 
be looking up on address anyway, and based on that you can get the symbol name 
from the pointed-at symbol anyway, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134133

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

[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables

2022-09-19 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

So previously, LLDB essentially used the COFF symbol table for executables, but 
only the list of exported symbols for DLLs, ignoring (or, reading and then 
overwriting) the symbol table for any DLL with exports? Then this certainly 
does look like a good direction.

Overall, I think this does look nice, but it's certainly hard to rewrite and 
wrap one's head around. When you've settled what you want to achieve, can you 
try to split it up into a short series of patches, so that the initial rewrite 
patch doesn't add a ton of extra functionality, but only covers the rewrite and 
appending instead of wiping symbols?




Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1558
   }
+  strm.IndentLess();
 }

Looks like this is a stray change unrelated to the rest (although it does seem 
correct).



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:787
+  Symbol *symbols = symtab.Extend(num_syms);
+  uint32_t i = 0;
+  for (const auto _ref : m_binary->symbols()) {

If we mean to append to the symbol table here, shouldn't we start with `i = 
symtab.size()` or similar? Otherwise we'd start overwriting the existing 
symbols from the start of the table?



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:819
+  if (exported->GetType() != lldb::eSymbolTypeReExported &&
+  exported->GetAddressRef() == symbols[i].GetAddressRef()) {
+symbols[i].SetID(exported->GetID());

What about the case when a symbol is exported with a different name than the 
local symbol? (This is doable with def files e.g. as `ExportedName = LocalName` 
iirc.) Is it worth to have a map of address -> exported symbol, to use instead 
of the raw name?



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:917
+  // actually be data, but this is relatively rare and we cannot tell
+  // from just the export table.
   symbols[i].SetType(lldb::eSymbolTypeCode);

If it's relevant, we can look up what section the exported address is in, and 
check if the section is executable or not.



Comment at: lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp:84
 
+  if (symtab->HasSymbolWithType(eSymbolTypeReExported, Symtab::eDebugAny,
+Symtab::eVisibilityAny)) {

For ease of review (when removing the WIP status), I think it'd be easier to 
wrap the head around, if new features like these were deferred to a separate 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134133

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

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

2022-09-17 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

FYI, this change caused a strange regression for reading PDB files in i386 
mode, when LLDB is built in mingw mode (not when it is built in MSVC mode), see 
https://github.com/llvm/llvm-project/issues/57799 for details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130796

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

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-16 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo updated this revision to Diff 460880.
mstorsjo added a comment.

Added a testcase. I'm not sure if this is the most accurate/correct place for 
the test, but the name is at least verbose enough to describe what it exercises.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

Files:
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
  lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,6 @@
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | 
FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2441,7 +2441,7 @@
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
-  return false;
+  continue;
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||


Index: lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
===
--- /dev/null
+++ lldb/test/Shell/Driver/Inputs/CommandOnCrashMultiThreaded.cpp
@@ -0,0 +1,13 @@
+#include 
+
+void t_func() {
+  asm volatile(
+"int3\n\t"
+  );
+}
+
+int main() {
+  std::thread t(t_func);
+  t.join();
+  return 0;
+}
Index: lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
===
--- /dev/null
+++ lldb/test/Shell/Driver/CommandOnCrashMultiThreaded.test
@@ -0,0 +1,6 @@
+# XFAIL: system-windows
+# REQUIRES: native && (target-x86 || target-x86_64)
+# RUN: %clangxx_host %p/Inputs/CommandOnCrashMultiThreaded.cpp -o %t -pthread
+# RUN: %lldb -b -o "process launch" -k "process continue" -k "exit" %t | FileCheck %s
+
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2441,7 +2441,7 @@
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
-  return false;
+  continue;
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-16 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D134037#3796251 , @jingham wrote:

> We never made any guarantee about the order threads would be listed in the 
> array returned by GetThreadList.  I'm not sure it would be worth trying to do 
> that, because often you don't know whether a thread's public StopInfo will be 
> valid even if the private stop info is.  For instance, if a thread hits a 
> thread specific breakpoint for a thread that isn't in the breakpoint's thread 
> specifier, then we reset the stop info back to an empty one because formally 
> that thread didn't hit our breakpoint...  So this change is clearly right.
>
> I don't suppose there's a way to write a test for this?

Actually, yes, it is testable fairly easily - by firing up a test example with 
2 threads and hitting a crash or breakpoint in either of them, and running it 
with a `-k` option. Before this fix, the test would still pass maybe half of 
the time, but now it should pass reliably.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134037

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

[Lldb-commits] [PATCH] D134037: [lldb] Fix CommandInterpreter::DidProcessStopAbnormally() with multiple threads

2022-09-16 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo created this revision.
mstorsjo added reviewers: labath, DavidSpickett.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

If a process has multiple threads, the thread with the stop
info might not be the first one in the thread list.

On Windows, under certain circumstances, processes seem to have
one or more extra threads that haven't been launched by the
executable itself, waiting in NtWaitForWorkViaWorkerFactory. If the
main (stopped) thread isn't the first one in the list (the order
seems nondeterministic), DidProcessStopAbnormally() would return
false prematurely, instead of inspecting later threads.

The main observable effect of DidProcessStopAbnormally() erroneously
returning false, is when running lldb with multiple "-o" parameters
to specify multiple commands to execute on the command line.

After an abnormal stop, lldb would stop executing "-o" parameters
and execute "-k" parameters instead - but due to this issue, it
would instead keep executing "-o" parameters as if there was no
abnormal stop. (If multiple parameters are specified via a script
file via the "-s" option, all of the commands in that file are
executed regardless of whether there's an abnormal stop inbetween.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134037

Files:
  lldb/source/Interpreter/CommandInterpreter.cpp


Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2441,7 +2441,7 @@
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
-  return false;
+  continue;
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||


Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2441,7 +2441,7 @@
   for (const auto _sp : process_sp->GetThreadList().Threads()) {
 StopInfoSP stop_info = thread_sp->GetStopInfo();
 if (!stop_info)
-  return false;
+  continue;
 
 const StopReason reason = stop_info->GetStopReason();
 if (reason == eStopReasonException ||
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133045: Partial fix for handling backticks in commands and aliases

2022-09-13 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

This seems to break the build with GCC (tested with the system compiler GCC 9 
on Ubuntu 20.04):

  ld.lld: error: undefined symbol: lldb_private::CommandInterpreter::g_argument
  >>> referenced by CommandAlias.cpp
  >>>   
CommandAlias.cpp.o:(lldb_private::CommandAlias::GetAliasExpansion(lldb_private::StreamString&)
 const (.localalias)) in archive lib/liblldbInterpreter.a

(with lots more similar errors)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133045

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

[Lldb-commits] [PATCH] D133002: [LLDB] Make API tests to run using MSYS tools

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

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

LGTM, this looks reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133002

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

[Lldb-commits] [PATCH] D132841: [lldb] Use the NativeSock type instead of plain 'int'

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG118038e878cf: [lldb] Use the NativeSock type instead of 
plain int (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132841

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


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -261,8 +261,8 @@
 return error;
   }
 
-  int sock = kInvalidSocketValue;
-  int listen_sock = kInvalidSocketValue;
+  NativeSocket sock = kInvalidSocketValue;
+  NativeSocket listen_sock = kInvalidSocketValue;
   lldb_private::SocketAddress AcceptAddr;
   MainLoop accept_loop;
   std::vector handles;


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -261,8 +261,8 @@
 return error;
   }
 
-  int sock = kInvalidSocketValue;
-  int listen_sock = kInvalidSocketValue;
+  NativeSocket sock = kInvalidSocketValue;
+  NativeSocket listen_sock = kInvalidSocketValue;
   lldb_private::SocketAddress AcceptAddr;
   MainLoop accept_loop;
   std::vector handles;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132841: [lldb] Use the NativeSock type instead of plain 'int'

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

mstorsjo created this revision.
mstorsjo added reviewers: labath, fixathon, jasonmolenda, clayborg, 
JDevlieghere, DavidSpickett.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

This fixes a warning when building for Windows:

  ../tools/lldb/source/Host/common/TCPSocket.cpp:297:16: warning: comparison of 
integers of different signs: 'int' and 'const NativeSocket' (aka 'const 
unsigned long long') [-Wsign-compare]
if (sock != kInvalidSocketValue) {
 ^  ~~~


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132841

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


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -261,8 +261,8 @@
 return error;
   }
 
-  int sock = kInvalidSocketValue;
-  int listen_sock = kInvalidSocketValue;
+  NativeSocket sock = kInvalidSocketValue;
+  NativeSocket listen_sock = kInvalidSocketValue;
   lldb_private::SocketAddress AcceptAddr;
   MainLoop accept_loop;
   std::vector handles;


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -261,8 +261,8 @@
 return error;
   }
 
-  int sock = kInvalidSocketValue;
-  int listen_sock = kInvalidSocketValue;
+  NativeSocket sock = kInvalidSocketValue;
+  NativeSocket listen_sock = kInvalidSocketValue;
   lldb_private::SocketAddress AcceptAddr;
   MainLoop accept_loop;
   std::vector handles;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131983: [LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

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

mstorsjo added a comment.

This commit caused warnings when building with GCC:

  ../tools/lldb/source/Commands/CommandObjectThread.cpp: In member function 
‘virtual lldb_private::Status 
CommandObjectThreadBacktrace::CommandOptions::SetOptionValue(uint32_t, 
llvm::StringRef, lldb_private::ExecutionContext*)’:
  ../tools/lldb/source/Commands/CommandObjectThread.cpp:66:61: warning: 
comparison of unsigned expression < 0 is always false [-Wtype-limits]
 66 | if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
| ^~~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

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

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

mstorsjo added a comment.

In D131159#3742370 , @labath wrote:

> In D131159#3742364 , @mgorny wrote:
>
>> In D131159#3742235 , @mstorsjo 
>> wrote:
>>
>>> (They're only used in asserts.) I guess we should add void casts to mark 
>>> the variables as used outside of asserts.
>>
>> Actually, there's a dedicated `UNUSED_IF_ASSERT_DISABLED` macro for that. 
>> Please use it instead.
>
> One of these days, I'm going to have to delete it. If void casts are good 
> enough for the rest of llvm, I don't see why should we be any different.

Ok, so is this discussion settled and I can go ahead and use void casts for 
these trivial fixups? (I usually just push such patches directly without 
passing via review.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

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

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

mstorsjo added a comment.

FYI, this patch added some new compilation warnings:

  [4055/7050] Building CXX object 
tools/lldb/source/Host/CMakeFiles/lldbHost.dir/windows/MainLoopWindows.cpp.obj
  llvm-project/lldb/source/Host/windows/MainLoopWindows.cpp:28:9: warning: 
unused variable 'result' [-Wunused-variable]
  int result = WSAEventSelect(fd, info.event, FD_READ | FD_ACCEPT | 
FD_CLOSE);
  ^
  llvm-project/lldb/source/Host/windows/MainLoopWindows.cpp:38:9: warning: 
unused variable 'result' [-Wunused-variable]
  int result = WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0);
  ^
  llvm-project/lldb/source/Host/windows/MainLoopWindows.cpp:86:8: warning: 
unused variable 'result' [-Wunused-variable]
BOOL result = WSACloseEvent(it->second.event);
 ^
  3 warnings generated.

(They're only used in asserts.) I guess we should add void casts to mark the 
variables as used outside of asserts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

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

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

mstorsjo added inline comments.



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

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

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

mstorsjo added a comment.

In D131159#3711024 , @mstorsjo wrote:

> In D131159#3709965 , @labath wrote:
>
>>> Can @alvinhochun take the patch for a spin? I think you've got more 
>>> realistic usecases of lldb to try it out in. (Do you want me to make a 
>>> build with the patch for you to try out?) @labath What's the best way to 
>>> exercise this code in actual use of lldb?
>>
>> I would appreciate that. I have tried to build it and run the test suite, 
>> but I didn't have a clean baseline, so I can't really say whether it works 
>> for everything. The main user of this code is the gdb-remote communication 
>> code, so debugging something via lldb-server (or running the lldb-server 
>> test suite) should be a good indicator of whether it works.
>
> I presume this is covered by running `ninja check-lldb`, or should I flip the 
> switch to prefer lldb-server on Windows before doing that? (I guess I can do 
> both.) I've got an environment where `check-lldb` mostly passes, I can check 
> that it doesn't regress it.

I tested out this patch with running `ninja check-lldb`. Out of the box, this 
patch doesn't regress anything - the tests still pass. (In my environment, 
there's a bunch of occasional stray failures already before, but by rerunning 
the tests a couple times, I can get a clean run.)

If I edit `ShouldUseLLDBServer()` in 
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp to always return 
true (before applying this patch), most tests still pass, but I have 11 tests 
that hang so that I end up having to kill lldb-server. With this patch on top, 
I still get the same set of 11 hanging tests, so I think that means that this 
patch doesn't regress anything with respect to that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

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

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

mstorsjo added a comment.

In D131159#3709965 , @labath wrote:

>> Can @alvinhochun take the patch for a spin? I think you've got more 
>> realistic usecases of lldb to try it out in. (Do you want me to make a build 
>> with the patch for you to try out?) @labath What's the best way to exercise 
>> this code in actual use of lldb?
>
> I would appreciate that. I have tried to build it and run the test suite, but 
> I didn't have a clean baseline, so I can't really say whether it works for 
> everything. The main user of this code is the gdb-remote communication code, 
> so debugging something via lldb-server (or running the lldb-server test 
> suite) should be a good indicator of whether it works.

I presume this is covered by running `ninja check-lldb`, or should I flip the 
switch to prefer lldb-server on Windows before doing that? (I guess I can do 
both.) I've got an environment where `check-lldb` mostly passes, I can check 
that it doesn't regress it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

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

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

mstorsjo added a comment.

In D130689#3700094 , @thakis wrote:

> In D130689#3696186 , @thieta wrote:
>
>> @nikic @thakis I fixed this issue in https://reviews.llvm.org/D131063 and it 
>> can be built with CXX_STANDARD=17 and OSX_DEPLOYMENT_TARGET=10.11.
>
> Thanks! We took this opportunity to bump our clang deployment target from 
> 10.7 to 10.12 (which makes a few other minor things easier too), so we don't 
> need this change any more. But it's a good change anyways :)

FWIW I ran into an issue when doing Universal macOS builds (by building for 
arm64 and x86_64 at the same time) where I now had to raise my deployment 
target to 10.14 - see https://github.com/llvm/llvm-project/issues/57017 for 
more details about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

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

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

mstorsjo added a subscriber: alvinhochun.
mstorsjo added a comment.

I don't really have experience with this API - is this pattern (creating and 
closing event objects for each poll run) the common way of using this API? Is 
there any potential performance risk compared with keeping event objects around 
(if that's possible?).

Can @alvinhochun take the patch for a spin? I think you've got more realistic 
usecases of lldb to try it out in. (Do you want me to make a build with the 
patch for you to try out?) @labath What's the best way to exercise this code in 
actual use of lldb?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

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

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

mstorsjo added inline comments.



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

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

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

mstorsjo added inline comments.

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

rnk wrote:
> I'm concerned that this isn't really the right fix. Changing the inheritance 
> model changes the size of the member pointer representation, so the 
> consequences of getting the wrong one could affect expression evaluation 
> results. Zequan suggested guessing the inheritance model from the class 
> definition, but we really ought to write down the inheritance model in the 
> DWARF when using the MS ABI. This probably needs its own DWARF attribute.
FWIW, there’s two use cases at play here:

1. The executable actually is using the itanium ABI, but is being 
(mis)interpreted with the MSVC ABI. When this happens, we currently crash, 
which is a terrible user experience. In this case, there’s probably no good 
solution (we can’t really get it right at this point) - but pretty much 
anything is better than crashing. Before D127048, we always interpreted 
everything with the MSVC C++ ABI; now we probably are getting it right in a 
majority of cases at least.

2. People intentionally using dwarf debug into with MSVC ABI. Not very common, 
but something that some people do use, as far as I know. Here we at least have 
a chance of getting right.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

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

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

mstorsjo added a comment.

This looks sensible to me, although it might be good if someone else more 
familiar with this code has a look too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

[Lldb-commits] [PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

2022-07-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D129377#3673237 , @mib wrote:

> In D129377#3673204 , @mstorsjo 
> wrote:
>
>> This broke building of Clang, when it's set up by symlinking 
>> `llvm-project/clang` into `llvm-project/llvm/tools`. In that case, cmake 
>> configure errors out like this:
>>
>>   -- Configuring done 
>>   CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
>> Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
>> path:
>>
>>   
>> "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/."
>>  
>>   
>> which is prefixed in the source directory.
>>
>> See e.g. 
>> https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source
>>  for some discussion on the issue.
>>
>> Can we revert this commit until this has been sorted out?
>
> @mstorsjo I reverted Chelsea's patch in b797834748f1 
> , since 
> she's offline now. Do you have a link to a bot failure that would help us 
> investigate the issue ?

So in short, I think this would work if you'd simply remove the newly added 
`target_include_directories(clangHandleCXX PUBLIC .)` in 
`clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt` and 
`target_include_directories(clangProtoToCXX PUBLIC .)` and 
`target_include_directories(clangLoopProtoToCXX PUBLIC .)` in 
`clang/tools/clang-fuzzer/proto-to-cxx/CMakeLists.txt`.

To reproduce the issue - instead of configuring llvm+clang+lldb by passing 
`-DLLVM_ENABLE_PROJECTS="clang;lldb"` make symlinks in 
`llvm-project/llvm/tools` pointing to `llvm-project/clang` and 
`llvm-project/lldb`. (I.e., `cd llvm-project/llvm/tools; ln -s ../../clang .; 
ln -s ../../lldb .`) This slightly affects the effective path at where CMake 
sees the clang files, which causes CMake to report the unexpected path overlap 
issue here.

I've tried to build all these fuzzers - I haven't really managed to build it 
all, but I think I'm fairly close. Anyway, I tried removing those 
`target_include_directories()` in that setup, and it didn't change anything, 
but maybe it only makes a difference if one gets further than what I got. 
(Changing `PUBLIC` into `PRIVATE` in those lines avoided the issue too.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129377

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

[Lldb-commits] [PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

2022-07-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D129377#3673237 , @mib wrote:

> In D129377#3673204 , @mstorsjo 
> wrote:
>
>> This broke building of Clang, when it's set up by symlinking 
>> `llvm-project/clang` into `llvm-project/llvm/tools`. In that case, cmake 
>> configure errors out like this:
>>
>>   -- Configuring done 
>>   CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
>> Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
>> path:
>>
>>   
>> "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/."
>>  
>>   
>> which is prefixed in the source directory.
>>
>> See e.g. 
>> https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source
>>  for some discussion on the issue.
>>
>> Can we revert this commit until this has been sorted out?
>
> @mstorsjo I reverted Chelsea's patch in b797834748f1 
> , since 
> she's offline now. Do you have a link to a bot failure that would help us 
> investigate the issue ?
>
> Thanks!

Thanks!

It’s not on a public buildbot but only in my local continuous builds 
unfortunately - but I’ll have look at the issue myself today - I was running 
out of time yesterday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129377

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

[Lldb-commits] [PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

2022-07-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

This broke building of Clang, when it's set up by symlinking 
`llvm-project/clang` into `llvm-project/llvm/tools`. In that case, cmake 
configure errors out like this:

  -- Configuring done 
  CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
path:
   
  
"/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/."
 
  
which is prefixed in the source directory.

See e.g. 
https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source
 for some discussion on the issue.

Can we revert this commit until this has been sorted out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129377

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

[Lldb-commits] [PATCH] D130309: [NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

2022-07-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D130309#3673045 , @clayborg wrote:

> Another fix for linux:
>
> commit 0bbce7a4c2d2bff622bdadd4323f93f5d90e6d24 
>  (HEAD 
> -> main, origin/main, origin/HEAD)
> Author: Greg Clayton 
> Date:   Fri Jul 22 13:59:06 2022 -0700
>
>   Fix buildbot breakage after https://reviews.llvm.org/D130309.

Thanks, now it finally built correctly for me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130309

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

[Lldb-commits] [PATCH] D130309: [NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

2022-07-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D130309#3672934 , @clayborg wrote:

> Submitted a fix for the buildbots:
>
> commit f959d815f4637890ebbacca379f1c38ab47e4e14 
>  (HEAD 
> -> main)
> Author: Greg Clayton 
> Date:   Fri Jul 22 13:24:04 2022 -0700
>
>   Fix buildbot breakage after https://reviews.llvm.org/D130309.

Still broken for me:

  ../tools/lldb/source/Host/linux/HostInfoLinux.cpp: In static member function 
‘static bool 
lldb_private::HostInfoLinux::ComputeSupportExeDirectory(lldb_private::FileSpec&)’:
  ../tools/lldb/source/Host/linux/HostInfoLinux.cpp:174:64: error: passing 
‘const lldb_private::ConstString’ as ‘this’ argument discards qualifiers 
[-fpermissive]
174 |   file_spec.GetDirectory() = GetProgramFileSpec().GetDirectory();
|^
  In file included from ../tools/lldb/include/lldb/Utility/ArchSpec.h:13,
   from ../tools/lldb/include/lldb/Host/HostInfoBase.h:12,
   from 
../tools/lldb/include/lldb/Host/posix/HostInfoPosix.h:12,
   from 
../tools/lldb/include/lldb/Host/linux/HostInfoLinux.h:12,
   from ../tools/lldb/source/Host/linux/HostInfoLinux.cpp:9:
  ../tools/lldb/include/lldb/Utility/ConstString.h:40:7: note:   in call to 
‘constexpr lldb_private::ConstString& 
lldb_private::ConstString::operator=(const lldb_private::ConstString&)’
 40 | class ConstString {
|   ^~~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130309

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

[Lldb-commits] [PATCH] D130309: [NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.

2022-07-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

I ran into this build break too - please try to fix or revert!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130309

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

[Lldb-commits] [PATCH] D129455: [lldb] Reduce the stack alignment requirements for the Windows x86_64 ABI

2022-07-11 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG66cdd6548ac5: [lldb] Reduce the stack alignment requirements 
for the Windows x86_64 ABI (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129455

Files:
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
  lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64-asm.s
  lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64.cpp
  lldb/test/Shell/Unwind/windows-unaligned-x86_64.test


Index: lldb/test/Shell/Unwind/windows-unaligned-x86_64.test
===
--- /dev/null
+++ lldb/test/Shell/Unwind/windows-unaligned-x86_64.test
@@ -0,0 +1,26 @@
+# Test unwinding through stack frames that aren't aligned to 16 bytes.
+# (In real world code, this happens when unwinding through
+# KiUserExceptionDispatcher and KiUserCallbackDispatcher in ntdll.dll.)
+
+# REQUIRES: target-x86_64, native, system-windows
+
+# RUN: %build %p/Inputs/windows-unaligned-x86_64.cpp 
%p/Inputs/windows-unaligned-x86_64-asm.s -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+# Future TODO: If %build could compile the source file in C mode, the symbol
+# name handling would be easier across msvc and mingw build configurations.
+# (In mingw mode, the extern C symbol "func" is printed as "::func" while
+# it's plain "func" in msvc mode. Without the extern C, it's "func(..." in
+# mingw mode, but "void __cdecl func(..." in msvc mode.)
+
+breakpoint set -n func
+# CHECK: Breakpoint 1: where = {{.*}}`{{(::)?}}func
+
+process launch
+# CHECK: stop reason = breakpoint 1.1
+
+thread backtrace
+# CHECK: frame #0: {{.*}}`{{(::)?}}func
+# CHECK: frame #1: {{.*}}`realign_stack
+# CHECK: frame #2: {{.*}}`call_func
+# CHECK: frame #3: {{.*}}`main
Index: lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64.cpp
===
--- /dev/null
+++ lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64.cpp
@@ -0,0 +1,8 @@
+extern "C" void call_func(void (*ptr)(int a), int a);
+
+extern "C" void func(int arg) { }
+
+int main(int argc, char **argv) {
+  call_func(func, 42);
+  return 0;
+}
Index: lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64-asm.s
===
--- /dev/null
+++ lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64-asm.s
@@ -0,0 +1,25 @@
+.globlcall_func
+.def  call_func;   .scl2;  .type   32; .endef
+.seh_proc call_func
+call_func:
+subq$32, %rsp
+.seh_stackalloc 32
+.seh_endprologue
+callrealign_stack
+addq$32, %rsp
+ret
+.seh_endproc
+
+.globlrealign_stack
+.def  realign_stack;   .scl2;  .type   32; .endef
+.seh_proc realign_stack
+realign_stack:
+subq$32, %rsp
+.seh_stackalloc 32
+.seh_endprologue
+movq%rcx, %rax
+movl%edx, %ecx
+call*%rax
+addq$32, %rsp
+ret
+.seh_endproc
Index: lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
===
--- lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
+++ lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
@@ -40,10 +40,15 @@
 
   bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
 
-  // In Windows_x86_64 ABI, stack will always be maintained 16-byte aligned
+  // In Windows_x86_64 ABI requires that the stack will be maintained 16-byte
+  // aligned.
+  // When ntdll invokes callbacks such as KiUserExceptionDispatcher or
+  // KiUserCallbackDispatcher, those functions won't have a properly 16-byte
+  // aligned stack - but tolerate unwinding through them by relaxing the
+  // requirement to 8 bytes.
   bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
- if (cfa & (16ull - 1ull))
-  return false; // Not 16 byte aligned
+if (cfa & (8ull - 1ull))
+  return false; // Not 8 byte aligned
 if (cfa == 0)
   return false; // Zero is not a valid stack address
 return true;


Index: lldb/test/Shell/Unwind/windows-unaligned-x86_64.test
===
--- /dev/null
+++ lldb/test/Shell/Unwind/windows-unaligned-x86_64.test
@@ -0,0 +1,26 @@
+# Test unwinding through stack frames that aren't aligned to 16 bytes.
+# (In real world code, this happens when unwinding through
+# KiUserExceptionDispatcher and KiUserCallbackDispatcher in ntdll.dll.)
+
+# REQUIRES: target-x86_64, native, system-windows
+
+# RUN: %build %p/Inputs/windows-unaligned-x86_64.cpp %p/Inputs/windows-unaligned-x86_64-asm.s -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+# Future TODO: If %build could compile the source file in C mode, the symbol
+# name handling

[Lldb-commits] [PATCH] D129455: [lldb] Reduce the stack alignment requirements for the Windows x86_64 ABI

2022-07-11 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D129455#3642015 , @mstorsjo wrote:

> In D129455#3641967 , @labath wrote:
>
>> What does `-Wl,-debug:symtab` actually produce? Would it make sense to make 
>> it a part of the `%clang_host` substitution (only for the msvc mode) to make 
>> the output of %clang_host more uniform? Or maybe achieve equivalence in some 
>> other way (add `-g1`, or similar, to the cmd line)?
>
> I think that might be a sensible path forward in general!

If we'd want to take that even further, we could consider to have `%clang_host` 
add options for generating DWARF debug info even in MSVC mode, and pass 
`-Wl,-debug:dwarf` to include it embedded in the exe like in mingw mode. (I've 
understood that some people even do this in proper production setups.) It's not 
what you'd normally have in MSVC mode, but could be useful for making testcases 
more portable, for cases where such details don't matter.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129455

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

[Lldb-commits] [PATCH] D129455: [lldb] Reduce the stack alignment requirements for the Windows x86_64 ABI

2022-07-11 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D129455#3641967 , @labath wrote:

> You say that the issue is the lack of symtab in the "msvc" mode. What makes 
> this test work then?

When invoked via the `%build` python script (lldb/test/Shell/helper/build.py), 
clang is invoked with extra options (`/Z7`) that generate codeview debug info, 
and then later the linker is invoked with extra options (`/DEBUG:FULL` and 
`'/PDB:' + self._pdb_file_name())`) to write that codeview debug info into a 
separate standalone PDB file.

In the MSVC ecosystem, executables/DLLs never have embedded debug info - it's 
always in a separate PDB file. Contrary to the GCC-compatible ecosystem, debug 
info is opt-in here, not opt-out (e.g. if linking with a unix linker, you get 
the symbol table and debug info included automatically unless you pass `-Wl,-s` 
or strip things separately afterwards). In the MSVC ecosystem, the only way to 
have a symbol table (without actual dwarf debug info) is with a PDB file.

In the mingw ecosystem, things behave as on unix - you get any embedded DWARF 
debug info included in the executable by default, and a symbol table - both 
which can be stripped out afterwards.

> Do some of the assembler directives (`.def`?) produce some sort of debug info 
> entries?

Those just set the symbol type to "function" - it could be omitted from this 
test for clarity.

> What determines the modes that clang is operating in? Is it the default 
> target triple?

Exactly.

Additionally, there's a couple more tooling differences that makes things a bit 
more complicated:

In the MSVC ecosystem, you normally would execute cl.exe (the MSVC compiler) or 
clang-cl (the cl.exe compatible clang frontend) - which has got an option 
syntax that is distinct and mostly incompatible from the gcc-compatible normal 
`clang` interface.

Despite this, you can also build code in MSVC mode with the regular 
gcc-compatible clang interface (either by passing e.g. 
`--target=x86_64-windows-msvc` or if such a triple is the deafult target 
triple). You can do most things in this setup too, but some MSVC-isms are 
tricky to achieve. This is what the `%clang_host` lit test macro does. Regular 
compiling, e.g. `%clang_host input.c -o executable -luser32` works fine for 
both MSVC and mingw mode.

In the MSVC ecosystem, you very rarely use the compiler driver to run linking - 
you'd normally execute `link.exe` or `lld-link` directly. If linking via the 
gcc-compatible clang interface, options passed to the linked with `-Wl,` need 
to be link.exe/lld-link options, which a unix-style linker obviously doesn't 
recognize.

> What does `-Wl,-debug:symtab` actually produce?

It produces an executable embedded symbol table, like in mingw mode. This is a 
lld specific option, MS's upstream link.exe doesn't support this option (it 
supports other parameters to `-debug:` but `symtab` is lld's invention). LLDB 
happily picks up and uses that (for mingw built executables it's the default 
case anyway).

> Would it make sense to make it a part of the `%clang_host` substitution (only 
> for the msvc mode) to make the output of %clang_host more uniform? Or maybe 
> achieve equivalence in some other way (add `-g1`, or similar, to the cmd 
> line)?

I think that might be a sensible path forward in general! But as noted, I'd 
rather not hold up this trivial patch with that. (Also I'm a bit more short on 
time at the moment than usual, but I'm trying to get low hanging fruit merged 
before the llvm 15 branch deadline.)

> as I think it would be very useful if we could figure out a way to make this 
> test (and others) cross-platform, so we don't have to have two versions of 
> all of them.

A bunch of tests might be doable crossplatform that way, but any tests that 
involve assembly might be tricky. On i386, arm and aarch64, Windows has got 
mostly the same calling conventions as other OSes, but on x86_64, it's 
radically different - arguments are passed in different registers than on 
x86_64 unix. Additionally, the caller is required to allocate shadow space on 
the stack for all arguments passed in registers (the extra `subq $32, %rsp`) so 
that the callee can dump them there if wanted. And finally, this particular 
testcase generates SEH unwind instructions with the `.seh_*` directives. (I 
didn't test whether the testcase would work without the SEH unwind instructions 
or not - I made it to look mostly like what a regular Windows function would 
look like.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129455

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

[Lldb-commits] [PATCH] D129455: [lldb] Reduce the stack alignment requirements for the Windows x86_64 ABI

2022-07-10 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo created this revision.
mstorsjo added reviewers: labath, jasonmolenda.
Herald added a subscriber: jsji.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

This fixes https://github.com/llvm/llvm-project/issues/56095.

@labath, FWIW while making the testcase, I realized why most tests on
Windows need the %build script instead of plain %clang_host.

For this testcase, I do need to have at least symbols available
(full debug info isn't needed). On unix-like systems, a command like
"clang source.c -o exec" will produce an executable with symbols.
With MSVC and clang in MSVC mode, the default linking command won't
produce any debug info at all, no symbols, nothing. We can fix it by
passing "-fuse-ld=lld -Wl,-debug:symtab", but that breaks mingw builds
(where symbols are kept by default).

The %build script handles all the options that are needed for creating
PDB debug info with clang-cl/lld-link, and produces symbols when building
in mingw mode too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129455

Files:
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
  lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64-asm.s
  lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64.cpp
  lldb/test/Shell/Unwind/windows-unaligned-x86_64.test


Index: lldb/test/Shell/Unwind/windows-unaligned-x86_64.test
===
--- /dev/null
+++ lldb/test/Shell/Unwind/windows-unaligned-x86_64.test
@@ -0,0 +1,26 @@
+# Test unwinding through stack frames that aren't aligned to 16 bytes.
+# (In real world code, this happens when unwinding through
+# KiUserExceptionDispatcher and KiUserCallbackDispatcher in ntdll.dll.)
+
+# REQUIRES: target-x86_64, native, system-windows
+
+# RUN: %build %p/Inputs/windows-unaligned-x86_64.cpp 
%p/Inputs/windows-unaligned-x86_64-asm.s -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+# Future TODO: If %build could compile the source file in C mode, the symbol
+# name handling would be easier across msvc and mingw build configurations.
+# (In mingw mode, the extern C symbol "func" is printed as "::func" while
+# it's plain "func" in msvc mode. Without the extern C, it's "func(..." in
+# mingw mode, but "void __cdecl func(..." in msvc mode.)
+
+breakpoint set -n func
+# CHECK: Breakpoint 1: where = {{.*}}`{{(::)?}}func
+
+process launch
+# CHECK: stop reason = breakpoint 1.1
+
+thread backtrace
+# CHECK: frame #0: {{.*}}`{{(::)?}}func
+# CHECK: frame #1: {{.*}}`realign_stack
+# CHECK: frame #2: {{.*}}`call_func
+# CHECK: frame #3: {{.*}}`main
Index: lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64.cpp
===
--- /dev/null
+++ lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64.cpp
@@ -0,0 +1,8 @@
+extern "C" void call_func(void (*ptr)(int a), int a);
+
+extern "C" void func(int arg) { }
+
+int main(int argc, char **argv) {
+  call_func(func, 42);
+  return 0;
+}
Index: lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64-asm.s
===
--- /dev/null
+++ lldb/test/Shell/Unwind/Inputs/windows-unaligned-x86_64-asm.s
@@ -0,0 +1,25 @@
+.globlcall_func
+.def  call_func;   .scl2;  .type   32; .endef
+.seh_proc call_func
+call_func:
+subq$32, %rsp
+.seh_stackalloc 32
+.seh_endprologue
+callrealign_stack
+addq$32, %rsp
+ret
+.seh_endproc
+
+.globlrealign_stack
+.def  realign_stack;   .scl2;  .type   32; .endef
+.seh_proc realign_stack
+realign_stack:
+subq$32, %rsp
+.seh_stackalloc 32
+.seh_endprologue
+movq%rcx, %rax
+movl%edx, %ecx
+call*%rax
+addq$32, %rsp
+ret
+.seh_endproc
Index: lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
===
--- lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
+++ lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
@@ -40,10 +40,15 @@
 
   bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
 
-  // In Windows_x86_64 ABI, stack will always be maintained 16-byte aligned
+  // In Windows_x86_64 ABI requires that the stack will be maintained 16-byte
+  // aligned.
+  // When ntdll invokes callbacks such as KiUserExceptionDispatcher or
+  // KiUserCallbackDispatcher, those functions won't have a properly 16-byte
+  // aligned stack - but tolerate unwinding through them by relaxing the
+  // requirement to 8 bytes.
   bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
- if (cfa & (16ull - 1ull))
-  return false; // Not 16 byte aligned
+if (cfa & (8ull - 1ull))
+  return false; // Not 8 byte aligned
 if (cfa == 0)
   return false; // Zero is not a valid stack

[Lldb-commits] [PATCH] D128617: [lldb] Stop passing both i386 and i686 in parallel as architectures on Windows

2022-07-06 Thread Martin Storsjö via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4270c9cd44f2: [lldb] Stop passing both i386 and i686 in 
parallel as architectures on Windows (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128617

Files:
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args ) {
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-specs.Append(module_spec);
 break;
   case MachineArmNt:
 spec.SetTriple("armv7-pc-windows");
Index: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
===
--- lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
+++ lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
@@ -137,8 +137,6 @@
   case PDB_Machine::x86:
 module_arch.SetTriple("i386-pc-windows");
 specs.Append(module_spec);
-module_arch.SetTriple("i686-pc-windows");
-specs.Append(module_spec);
 break;
   case PDB_Machine::ArmNT:
 module_arch.SetTriple("armv7-pc-windows");


Index: lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args ) {
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-specs.Append(module_spec);
 break;
   case MachineArmNt:
 spec.SetTriple("armv7-pc-windows");
Index: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
===
--- lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
+++ lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
@@ -137,8 +137,6 @@
   case PDB_Machine::x86:

[Lldb-commits] [PATCH] D128678: [LLDB] Add PDB/calling-conventions.test for Arm/Windows

2022-06-28 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

Thanks, this looks more complete and consistent to me now!

In D128678#3615531 , @labath wrote:

> It seems like this is not actually running the code. Can we make it such that 
> these tests are conditional on the appropriate llvm targets being enabled 
> (instead of the host being of a specific type)?

Well it does need being able to link executables for these targets, which you 
generally can't rely on I think? (I don't think the `%build` macro in general 
works as a cross compiler?)

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

https://reviews.llvm.org/D128678

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

2022-06-28 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D128268#3614661 , @labath wrote:

> In D128268#3611053 , @mstorsjo 
> wrote:
>
>> The odd thing about the second one is the added hardcoded 
>> `AddArch(ArchSpec("i686-pc-windows"));` and 
>> `AddArch(ArchSpec("i386-pc-windows"));` at the top of `PlatformWindows.cpp`.
>>
>> Anyway, it does seem to work fine in my quick test to get rid of this 
>> duality, so I'll post a patch that does that.
>
> I think those hardcoded lines are there precisely because of the duality 
> (i.e. wanting to support both i386 and i686 regardless of what the host layer 
> reports).

Both that, and because the i386+i686 fat binary arches from ObjectFile 
triggered the more elaborate arch validation, these had to be present to make 
it work. Anyway, after getting rid of the duality, those hardcoded ones don't 
seem to be needed either.

> If that is removed, maybe all of that can be changed to  the pattern used in 
> other platform plugins 
> .

That looks reasonable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128678: [LLDB] Add PDB/Calling-conentions.test for Arm/Windows

2022-06-27 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

There's consistent typos about the test name in the commit subject and 
description.




Comment at: lldb/test/Shell/SymbolFile/PDB/calling-conventions-arm.test:1
+REQUIRES: system-windows, lld, (target-arm || target-aarch64) 
+RUN: %build --compiler=clang-cl --arch=32 --nodefaultlib --output=%t.exe 
%S/Inputs/CallingConventionsTest.cpp

Hmm, I wasn't aware of the fact that you can do such `||` expressions in the 
`REQUIRES` line - I presume you've verified that this test actually does get 
picked up and executed?



Comment at: lldb/test/Shell/SymbolFile/PDB/calling-conventions-arm.test:2
+REQUIRES: system-windows, lld, (target-arm || target-aarch64) 
+RUN: %build --compiler=clang-cl --arch=32 --nodefaultlib --output=%t.exe 
%S/Inputs/CallingConventionsTest.cpp
+RUN: lldb-test symbols -dump-ast %t.exe | FileCheck %s

Here, there's probably no need to force it to 32 bit mode - unless we expect to 
have a similar test for the undecorated mangling in aarch64 and x86_64 mode too?


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

https://reviews.llvm.org/D128678

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

[Lldb-commits] [PATCH] D128668: [LLDB] Fix PDB/pointers.test for 32bit Arm/Windows

2022-06-27 Thread Martin Storsjö via Phabricator via lldb-commits

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



Comment at: lldb/test/Shell/SymbolFile/PDB/pointers.test:2
 REQUIRES: system-windows, msvc
 RUN: %build --compiler=clang-cl --mode=compile --arch=32 --nodefaultlib 
--output=%T/PointerTypeTest.cpp.obj %S/Inputs/PointerTypeTest.cpp
 RUN: %build --compiler=msvc --mode=link --arch=32 --nodefaultlib 
--output=%T/PointerTypeTest.cpp.exe %T/PointerTypeTest.cpp.obj

omjavaid wrote:
> mstorsjo wrote:
> > I wonder if it'd be better to hardcode this to build for `i386` (assuming 
> > the `%build` script allows that?) to keep it testing what it was originally 
> > made to test?
> Given we expect this to pass for Windows/Arm it will reduce coverage if made 
> to run only for x86.
Right, as we probably need to link, that would require more of the host 
environment than we normally require (for other tests where we don't link 
things, we can compile things for x86 even if we're running on arm, as long as 
the x86 support is available in the compiler).

Given the structure of the lldb tests I guess this is more in line with how 
things normally are done here, so I guess this is fine.


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

https://reviews.llvm.org/D128668

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

[Lldb-commits] [PATCH] D128668: [LLDB] Fix PDB/pointers.test for 32bit Arm/Windows

2022-06-27 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

I presume this is ok - if the main intent of this test is to test handling of 
pointers and the size thereof in PDBs.




Comment at: lldb/test/Shell/SymbolFile/PDB/pointers.test:2
 REQUIRES: system-windows, msvc
 RUN: %build --compiler=clang-cl --mode=compile --arch=32 --nodefaultlib 
--output=%T/PointerTypeTest.cpp.obj %S/Inputs/PointerTypeTest.cpp
 RUN: %build --compiler=msvc --mode=link --arch=32 --nodefaultlib 
--output=%T/PointerTypeTest.cpp.exe %T/PointerTypeTest.cpp.obj

I wonder if it'd be better to hardcode this to build for `i386` (assuming the 
`%build` script allows that?) to keep it testing what it was originally made to 
test?


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

https://reviews.llvm.org/D128668

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

[Lldb-commits] [PATCH] D128617: [lldb] Stop passing both i386 and i686 in parallel as architectures on Windows

2022-06-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo created this revision.
mstorsjo added reviewers: labath, zturner, DavidSpickett.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

When an object file returns multiple architectures, it is treated
as a fat binary - which really isn't the case of i386 vs i686 where
the object file actually has one architecture.

This allows getting rid of hardcoded architecture triples in
PlatformWindows.

The parallel i386 and i686 architecture strings stem from
5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 
 and
ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128617

Files:
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args ) {
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-specs.Append(module_spec);
 break;
   case MachineArmNt:
 spec.SetTriple("armv7-pc-windows");
Index: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
===
--- lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
+++ lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
@@ -137,8 +137,6 @@
   case PDB_Machine::x86:
 module_arch.SetTriple("i386-pc-windows");
 specs.Append(module_spec);
-module_arch.SetTriple("i686-pc-windows");
-specs.Append(module_spec);
 break;
   case PDB_Machine::ArmNT:
 module_arch.SetTriple("armv7-pc-windows");


Index: lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml
@@ -18,7 +18,7 @@
 # RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
 
 # CHECK-LABEL: image list --triple --basename
-# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+# CHECK-NEXT: i386-pc-windows-[[ABI]] [[FILENAME]]
 
 --- !COFF
 OptionalHeader:
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -124,11 +124,9 @@
 if (spec.IsValid())
   m_supported_architectures.push_back(spec);
   };
-  AddArch(ArchSpec("i686-pc-windows"));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKindDefault));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind32));
   AddArch(HostInfo::GetArchitecture(HostInfo::eArchKind64));
-  AddArch(ArchSpec("i386-pc-windows"));
 }
 
 Status PlatformWindows::ConnectRemote(Args ) {
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -337,9 +337,6 @@
 spec.SetTriple("i386-pc-windows");
 spec.GetTriple().setEnvironment(env);
 specs.Append(module_spec);
-spec.SetTriple("i686-pc-windows");
-spec.GetTriple().setEnvironment(env);
-

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

2022-06-26 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D128268#3608197 , @labath wrote:

> In D128268#3604555 , @mstorsjo 
> wrote:
>
>> I found that this duality was introduced in 
>> 5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 
>>  and 
>> ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 
>>  (CC @zturner), what do you make out of the 
>> reasonings in those commits?
>
> The first patch seems like it's just working around some mismatches in the 
> windows dynamic loader plugin. I think a better approach would be to have the 
> dynamic loader claim both architectures, though I don't think that is 
> necessary if we're just consistent about what we use. I don't see anything 
> wrong with the second patch (the darwin platform does something similar for 
> arm architectures, even though I'm not convinced it's necessary (the reason 
> it's necessary for darwin is because there they actually make a distinction 
> between armv6XX and armv7YY and treat those as different architectures).

The odd thing about the second one is the added hardcoded 
`AddArch(ArchSpec("i686-pc-windows"));` and 
`AddArch(ArchSpec("i386-pc-windows"));` at the top of `PlatformWindows.cpp`.

Anyway, it does seem to work fine in my quick test to get rid of this duality, 
so I'll post a patch that does that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

2022-06-24 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D128410#3608190 , @labath wrote:

> In D128410#3604927 , @alvinhochun 
> wrote:
>
>> It may be possible to stuff a pointer to an `EXCEPTION_RECORD` into another 
>> `EXCEPTION_RECORD` and use `RtlRaiseException` to generate the exception, 
>> but you'll have to test how it actually works.
>
> That would be pretty cool.

Yeah - I guess it's two separate kinds of testcases; this one would be more of 
a macro-testcase, "does this real-world case work - whichever way lldb happens 
to handle it" (nested exception or not?) while that would be more of a clinical 
unit test for specifically testing nested exceptions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a subscriber: zturner.
mstorsjo added a comment.

In D128268#3604081 , @labath wrote:

>> If we'd just set this to the baseline, i386, would that have any effect for 
>> how lldb e.g. is able to disassemble/interpret instructions that don't exist 
>> in the i386 baseline architecture?
>
> It should not have any effect (if it does, that's a separate fix). In the 
> disassembler, we explicitly enable the latest instruction set, and I can't 
> think of anything else that would be impacted by it.

Thanks - I did some cursory testing with removing the extra i686 everywhere, 
and at least on a quick test, it seems to work just fine (and requires a minor 
adjustment to only one testcase).

I found that this duality was introduced in 
5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 
 and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / 
D4658  (CC @zturner), what do you make out of 
the reasonings in those commits?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D128410#3604083 , @mstorsjo wrote:

> I'm not all that familiar with exactly what happens in these APIs here that 
> makes this a nested exception and what other non-GUI APIs would produce the 
> same effect. Maybe any API where Windows system code calls back to a user 
> provided callback? Does @stella.stamenova know?

FWIW, I tested with doing a similar crash in the callback from 
`EnumerateLoadedModules`, but that doesn't trigger the same case.

I tested running the lldb tests within docker, which is a headless windows 
environment, and this test does indeed fail there (it hangs).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

mstorsjo wrote:
> labath wrote:
> > Is there something reasonable we could assert here? The process exit status 
> > for instance?
> This in itself requires the `%lldb` command to succeed - the exit status for 
> each `RUN` line needs to be successful (unless a failure is expected and it 
> can be inverted with the `not` command). Or did you mean checking the process 
> exit code of the child process (that intentionally does crash)?
> 
> When this test case is run, it prints the following to the terminal:
> ```
> (lldb) run
> Process 3168 stopped
> * thread #1, stop reason = Exception 0xc41d encountered at address 
> 0x7ff68e6f1227
> frame #0: 0x7ff68e6f1227 wndproc_exception.cpp.tmp.exe
> ->  0x7ff68e6f1227: movl   $0x1, (%rax)
> 0x7ff68e6f122d: movq   $0x0, 0x50(%rsp)
> 0x7ff68e6f1236: jmp0x7ff68e6f1259
> 0x7ff68e6f123b: movq   0x48(%rsp), %r9
> Process 3168 launched: 
> 'C:\dev\llvm-project\llvm\build-msvc\tools\lldb\test\Shell\Process\Windows\Output\wndproc_exception.cpp.tmp.exe'
>  (x86_64)
> ```
> I guess we could check for `Exception 0xc41d encountered` maybe, although 
> I'm afraid of making the testcase unnecessarily brittle too.
If running with lldb-server enabled, it prints a different exception code, 
`0xc005` (which is `STATUS_ACCESS_VIOLATION`) which probably is the proper 
nested exception, while without lldb-server, it prints `0xc41d` 
(`STATUS_FATAL_USER_CALLBACK_EXCEPTION`).

So for the purpose of this testcase, just to make sure that it doesn't crash in 
this case, it'd be better to not specify exactly which exception code is to be 
returned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a subscriber: stella.stamenova.
mstorsjo added a comment.

In D128410#3604075 , @labath wrote:

> In D128410#3604066 , @mstorsjo 
> wrote:
>
>> For this testcase to work, it needs to be able to open a window - so it 
>> can't run entirely in headless mode. But I guess that a bunch of other tests 
>> in LLDB also already do that, since running the LLDB tests on Windows pops 
>> up a dozen of windows temporarily.
>
> I think those just come from the tests (or more like test runners) which 
> forget to suppress a console window from opening (see 
> LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE) -- this is probably the first "gui" 
> application. This isn't necessarily a showstopper, but I am surprised that it 
> is necessary. I'm hardly a windows expert, but I would expect that it should 
> be possible to generate an exception (even nested exception, which to me 
> sounds like the equivalent of getting a signal inside a signal handler) in a 
> "regular" text-mode application. Are you sure that is not possible?

I didn't say that this isn't necessarily possible - this is the reduced 
testcase that @alvinhochun provided in 
https://github.com/mstorsjo/llvm-mingw/issues/292#issuecomment-1160239522 (that 
I shrunk down further a bit).

I'm not all that familiar with exactly what happens in these APIs here that 
makes this a nested exception and what other non-GUI APIs would produce the 
same effect. Maybe any API where Windows system code calls back to a user 
provided callback? Does @stella.stamenova know?




Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

labath wrote:
> Is there something reasonable we could assert here? The process exit status 
> for instance?
This in itself requires the `%lldb` command to succeed - the exit status for 
each `RUN` line needs to be successful (unless a failure is expected and it can 
be inverted with the `not` command). Or did you mean checking the process exit 
code of the child process (that intentionally does crash)?

When this test case is run, it prints the following to the terminal:
```
(lldb) run
Process 3168 stopped
* thread #1, stop reason = Exception 0xc41d encountered at address 
0x7ff68e6f1227
frame #0: 0x7ff68e6f1227 wndproc_exception.cpp.tmp.exe
->  0x7ff68e6f1227: movl   $0x1, (%rax)
0x7ff68e6f122d: movq   $0x0, 0x50(%rsp)
0x7ff68e6f1236: jmp0x7ff68e6f1259
0x7ff68e6f123b: movq   0x48(%rsp), %r9
Process 3168 launched: 
'C:\dev\llvm-project\llvm\build-msvc\tools\lldb\test\Shell\Process\Windows\Output\wndproc_exception.cpp.tmp.exe'
 (x86_64)
```
I guess we could check for `Exception 0xc41d encountered` maybe, although 
I'm afraid of making the testcase unnecessarily brittle too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D128268#3603931 , @labath wrote:

>> As a separate path forward, one could also consider to stop returning
>> two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
>> for i386 files.
>
> I think that would be much more preferable. I think the current 
> implementation GetModuleSpecifications is based on a misconception about what 
> the multiple results of that function mean (a fat binary -- as you've 
> discovered).
>
> We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that 
> claims it can be run on armv5. Elf files also return just a single result for 
> i386 file.

If we'd just set this to the baseline, i386, would that have any effect for how 
lldb e.g. is able to disassemble/interpret instructions that don't exist in the 
i386 baseline architecture?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

For this testcase to work, it needs to be able to open a window - so it can't 
run entirely in headless mode. But I guess that a bunch of other tests in LLDB 
also already do that, since running the LLDB tests on Windows pops up a dozen 
of windows temporarily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo created this revision.
mstorsjo added reviewers: labath, alvinhochun, DavidSpickett.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

This adds a testcase for
4d123783957e547009e55346bf3a8ae43a88fa14 
 / D128201 
. Before that commit,
lldb would crash here, while now lldb manages to stop after the
exception.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128410

Files:
  lldb/test/Shell/Process/Windows/wndproc_exception.cpp


Index: lldb/test/Shell/Process/Windows/wndproc_exception.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/wndproc_exception.cpp
@@ -0,0 +1,54 @@
+// clang-format off
+
+// Check that lldb doesn't crash when there's a nested exception.
+
+// REQUIRES: system-windows
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+
+#include 
+
+static LRESULT CALLBACK WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam,
+   LPARAM lParam) {
+  switch (uMsg) {
+  case WM_DESTROY:
+PostQuitMessage(0);
+return 0;
+
+  case WM_PAINT:
+*(volatile int *)nullptr = 1;
+return 0;
+  }
+  return DefWindowProcA(hwnd, uMsg, wParam, lParam);
+}
+
+int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR 
pCmdLine,
+   int nCmdShow) {
+  const char CLASS_NAME[] = "Crash Window Class";
+
+  WNDCLASSA wc = {};
+
+  wc.lpfnWndProc = WindowProc;
+  wc.hInstance = hInstance;
+  wc.lpszClassName = CLASS_NAME;
+
+  RegisterClassA();
+
+  HWND hwnd = CreateWindowExA(0, CLASS_NAME, "Crash", WS_OVERLAPPEDWINDOW,
+  CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT,
+  CW_USEDEFAULT,
+  NULL, NULL, hInstance, NULL);
+
+  if (hwnd == NULL)
+return 0;
+
+  ShowWindow(hwnd, nCmdShow);
+
+  MSG msg = {};
+  while (GetMessageA(, NULL, 0, 0) > 0) {
+TranslateMessage();
+DispatchMessageA();
+  }
+
+  return 0;
+}


Index: lldb/test/Shell/Process/Windows/wndproc_exception.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/wndproc_exception.cpp
@@ -0,0 +1,54 @@
+// clang-format off
+
+// Check that lldb doesn't crash when there's a nested exception.
+
+// REQUIRES: system-windows
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+
+#include 
+
+static LRESULT CALLBACK WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam,
+   LPARAM lParam) {
+  switch (uMsg) {
+  case WM_DESTROY:
+PostQuitMessage(0);
+return 0;
+
+  case WM_PAINT:
+*(volatile int *)nullptr = 1;
+return 0;
+  }
+  return DefWindowProcA(hwnd, uMsg, wParam, lParam);
+}
+
+int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR pCmdLine,
+   int nCmdShow) {
+  const char CLASS_NAME[] = "Crash Window Class";
+
+  WNDCLASSA wc = {};
+
+  wc.lpfnWndProc = WindowProc;
+  wc.hInstance = hInstance;
+  wc.lpszClassName = CLASS_NAME;
+
+  RegisterClassA();
+
+  HWND hwnd = CreateWindowExA(0, CLASS_NAME, "Crash", WS_OVERLAPPEDWINDOW,
+  CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT,
+  CW_USEDEFAULT,
+  NULL, NULL, hInstance, NULL);
+
+  if (hwnd == NULL)
+return 0;
+
+  ShowWindow(hwnd, nCmdShow);
+
+  MSG msg = {};
+  while (GetMessageA(, NULL, 0, 0) > 0) {
+TranslateMessage();
+DispatchMessageA();
+  }
+
+  return 0;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

2022-06-23 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D128268#3603931 , @labath wrote:

>> As a separate path forward, one could also consider to stop returning
>> two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
>> for i386 files.
>
> I think that would be much more preferable. I think the current 
> implementation GetModuleSpecifications is based on a misconception about what 
> the multiple results of that function mean (a fat binary -- as you've 
> discovered).
>
> We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that 
> claims it can be run on armv5. Elf files also return just a single result for 
> i386 file.

Yep, that's probably a good path forward for the future - I was just weary of 
that possibly opening another huge can of worms. And regardless of that, I 
think the functional change of this patch, allowing considering various 
variations of triples that commonly occur on windows as compatible, is sensible 
on its own (such issues have caused unnecessary tweaking back and forth once or 
twice before too).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D126513: Add -b (--continue-to-breakpoint) option to the "process continue" command

2022-06-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added subscribers: stella.stamenova, mstorsjo.
mstorsjo added a comment.

I think this might have broken a bunch of testcases on the 
lldb-x64-windows-ninja buildbot too, e.g. 
https://lab.llvm.org/buildbot/#/builders/83/builds/20304 (CC @stella.stamenova)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126513

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

[Lldb-commits] [PATCH] D127436: [lldb] Resolve exe location for `target create`

2022-06-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D127436#3602224 , 
@stella.stamenova wrote:

> This broke the Windows LLDB bot:
>
> https://lab.llvm.org/buildbot/#/builders/83/builds/20295/steps/7/logs/stdio

The second fix attempt, in 
https://github.com/llvm/llvm-project/commit/a1ee0b947d46c9be1cc2ea8db21603bac84efb18,
 seems to have fixed this test now - however the latest test run seems to have 
some other failures: https://lab.llvm.org/buildbot/#/builders/83/builds/20304 
(Not sure if these are spurious failures or if they are other regressions that 
happened meanwhile)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127436

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

[Lldb-commits] [PATCH] D127436: [lldb] Resolve exe location for `target create`

2022-06-22 Thread Martin Storsjö via Phabricator via lldb-commits

mstorsjo added a comment.

In D127436#3602224 , 
@stella.stamenova wrote:

> This broke the Windows LLDB bot:
>
> https://lab.llvm.org/buildbot/#/builders/83/builds/20295/steps/7/logs/stdio

Yep, noted. It worked for me in my MSVC build configuration - I'll investigate 
a bit more and either push another attempt at fixing it, or revert, in a little 
while.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127436

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

1 2 3 4 5 >

1 - 100 of 424 matches

Mail list logo