[lldb-server] Use Platform plugin corresponding to the host

[bulbazord]

Summary

In ee232506b870ce5282cc4da5ca493d41d361feb3 I moved UnixSignal
initialization from lldbTarget to the various platform plugins. This
inadvertently broke lldb-server because lldb-server doesn't use
Platform plugins. lldb-server still needs to be able to create a
UnixSignals object for the host platform so we can add the relevant
platform plugin to lldb-server to make sure we always have a
HostPlatform.

[bulbazord]

With this patch:

alex@ubuntu:~/llvm-project/build$ ./bin/lldb foo
(lldb) target create "foo"
Current executable set to '/home/alex/llvm-project/build/foo' (aarch64).
(lldb) r
Process 19623 launched: '/home/alex/llvm-project/build/foo' (aarch64)
Process 19623 stopped
* thread #1, name = 'foo', stop reason = signal SIGSEGV: invalid permissions for mapped object (fault address: 0xfffff7ff6000)
    frame #0: 0x0000aaaaaaaa0808 foo`crash() at foo.cpp:5:7
   2
   3    void crash() {
   4            int *ptr = static_cast<int *>(mmap(nullptr, sizeof(int), PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0));
-> 5            *ptr = 0x12345678;
   6            munmap(ptr, sizeof(int));
   7    }
   8
(lldb)

[bulbazord]

Adding a test

[rupprecht]

Thanks! This indeed fixes the issue I'm seeing.

[thakis]

Hello, https://github.com/llvm/llvm-project/commit/5fb4147f74ad90095f08f243e5f19877d09d11b9 broke the windows build (http://45.33.8.238/win/76494/step_4.txt) because it's now including system headers within namespace lldb. Please take a look and revert for now if it takes a while to fix.

(Saying that here since I see no phab link on the bad commit. Uploading changes to phab gives you access to pre-commit testing on a bunch of platforms, so it's useful even for post-commit reviewed patches)

[bulbazord]

In D146668#4221223, @thakis wrote:

Hello, https://github.com/llvm/llvm-project/commit/5fb4147f74ad90095f08f243e5f19877d09d11b9 broke the windows build (http://45.33.8.238/win/76494/step_4.txt) because it's now including system headers within namespace lldb. Please take a look and revert for now if it takes a while to fix.

(Saying that here since I see no phab link on the bad commit. Uploading changes to phab gives you access to pre-commit testing on a bunch of platforms, so it's useful even for post-commit reviewed patches)

Thanks for letting me know and apologies for breaking the build. It looks like Benjamin Kramer fixed this in 9c6b69000d9307a36745004ee9300f70a5415160.

[labath]

I'm sorry, but I don't think this is a good change (which I guess means the previous one is not good either, but this one drives the point). I really don't want lldb-server to depend on a platform plugins . I have two reasons for that:

• lldb-server is always running on the "host" platform, so the abstraction is largely useless
• the platform plugins end up (transitively) depending on the whole world -- which includes tons of functionality that lldb-server does not need. With these changes the size of the lldb-server binary has increased by over 50% in my build (release+asserts on linux) -- from 40MB (which is already quite big for what lldb-server does) to 62MB (which is just ludicrous).

Of these, the second reason is the main one. If the platform plugin had a simple self-contained API, then using it in lldb-server would not be a problem. It might even be nice. However, we're very far from that state, and I don't think it makes sense to try to achieve it. We use it as a repository for os-specific logic in lldb, but here "lldb" really means the lldb client. lldb-server doesn't need 90% of the Platform functionality, and most of the other 10% (e.g. the file API) can be easily replaced by using the host equivalents directly. Before this patch set, unix signals belonged in those other 10 percent.

I think we need to revert these patches and figure out a different way to solve the issue at hand. The 50% increase is just too high. To be clear, I totally understand and support what you're trying to achieve. Having no outgoing plugin dependencies from the lldb core is a worthwhile endeavour, and in fact could help shrink lldb-server size as well (because it wants to depend on some -- and only some -- parts of lldb core). However, that won't be the case if we solve that by reattaching all of the dependencies to lldb-server.

From the lldb client perspective, there's nothing wrong with having a Platform::GetUnixSignals api. It fits in very well into the existing functionality. However, since this is also used by lldb-server, we should also have a non-platform way to access that data. One way to solve that would be to have some sort of a "platform lite" plugin, which would contain the signal API, and any other functionality that is useful for both lldb and lldb-server -- and the real platform plugin could subsume that.

However, I find that a bit overkill. The unix signals classes are very simple (and very repetitive), so it seems to me like the plugin machinery would just add a bunch of boilerplate (and institutionalize the repetition). I could be convinced otherwise (particularly, if we find some other platform-like functionality that could be placed into this plugin), but right now, I think it would be better to just de-pluginize the entire UnixSignals machinery. For example, we could just move it down to Utility so it can be accessed from anywhere. Later, with some refactoring, I think we could even merge the various ***Signals files into one, since all they should really differ is in the values of signal constants. Like, we probably would not want to have a different signal disposition settings for SIGINT on linux than say on FreeBSD, so the fact that these are repeated in every implementation is just a potential source of bugs.

And we can still have Platform::GetUnixSignals to wrap this, and call it from whereever it makes sense.

[bulbazord]

In D146668#4223456, @labath wrote:

I'm sorry, but I don't think this is a good change (which I guess means the previous one is not good either, but this one drives the point). I really don't want lldb-server to depend on a platform plugins . I have two reasons for that:

• lldb-server is always running on the "host" platform, so the abstraction is largely useless
• the platform plugins end up (transitively) depending on the whole world -- which includes tons of functionality that lldb-server does not need. With these changes the size of the lldb-server binary has increased by over 50% in my build (release+asserts on linux) -- from 40MB (which is already quite big for what lldb-server does) to 62MB (which is just ludicrous).

Of these, the second reason is the main one. If the platform plugin had a simple self-contained API, then using it in lldb-server would not be a problem. It might even be nice. However, we're very far from that state, and I don't think it makes sense to try to achieve it. We use it as a repository for os-specific logic in lldb, but here "lldb" really means the lldb client. lldb-server doesn't need 90% of the Platform functionality, and most of the other 10% (e.g. the file API) can be easily replaced by using the host equivalents directly. Before this patch set, unix signals belonged in those other 10 percent.

I think we need to revert these patches and figure out a different way to solve the issue at hand. The 50% increase is just too high. To be clear, I totally understand and support what you're trying to achieve. Having no outgoing plugin dependencies from the lldb core is a worthwhile endeavour, and in fact could help shrink lldb-server size as well (because it wants to depend on some -- and only some -- parts of lldb core). However, that won't be the case if we solve that by reattaching all of the dependencies to lldb-server.

From the lldb client perspective, there's nothing wrong with having a Platform::GetUnixSignals api. It fits in very well into the existing functionality. However, since this is also used by lldb-server, we should also have a non-platform way to access that data. One way to solve that would be to have some sort of a "platform lite" plugin, which would contain the signal API, and any other functionality that is useful for both lldb and lldb-server -- and the real platform plugin could subsume that.

However, I find that a bit overkill. The unix signals classes are very simple (and very repetitive), so it seems to me like the plugin machinery would just add a bunch of boilerplate (and institutionalize the repetition). I could be convinced otherwise (particularly, if we find some other platform-like functionality that could be placed into this plugin), but right now, I think it would be better to just de-pluginize the entire UnixSignals machinery. For example, we could just move it down to Utility so it can be accessed from anywhere. Later, with some refactoring, I think we could even merge the various ***Signals files into one, since all they should really differ is in the values of signal constants. Like, we probably would not want to have a different signal disposition settings for SIGINT on linux than say on FreeBSD, so the fact that these are repeated in every implementation is just a potential source of bugs.

And we can still have Platform::GetUnixSignals to wrap this, and call it from whereever it makes sense.

I'm alright with reverting these patches if it balloons lldb-server's size by 50%. I really appreciate you trying to work with me to find a path forward here. I really do not want to revert without some idea of how we could solve this dependency/layering issue.

I like the idea of a "platform lite" plugin, but I also agree that it is probably overkill right now. Let's keep that idea in our back pockets. Sometime today I will revert this patch (I'll re-commit the test in a follow-up as it is still useful) and its predecessor (D146263). I'll also do some investigation work and write something up on the LLVM discourse where we can have a discussion about the best way forward.

[labath]

Thank you for your understanding. Let me know if you need anything from me.

In the long-term I'd like to factor this signal code such that there is a single common code which maps all signals (or at least the most common ones) to their default disposition, description and what not, and then a bunch of os- (and, if needed arch-) specific bits that provide numbers for those signals. In the short term, I think we could just move these classes into Utility.

lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py

def inferior_crashing(self):
         """Inferior crashes upon launching; lldb should catch the event and stop."""
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         self.runCmd("run", RUN_SUCCEEDED)
         # The exact stop reason depends on the platform
         if self.platformIsDarwin():
             stop_reason = 'stop reason = EXC_BAD_ACCESS'
-        elif self.getPlatform() == "linux" or self.getPlatform() == "freebsd":
+        elif self.getPlatform() == "linux":
+            stop_reason = 'stop reason = signal SIGSEGV: address not mapped to object'
+        elif self.getPlatform() == "freebsd":
             stop_reason = 'stop reason = signal SIGSEGV'
         else:
             stop_reason = 'stop reason = invalid address'
         self.expect("thread list", STOPPED_DUE_TO_EXC_BAD_ACCESS,
                     substrs=['stopped',
                              stop_reason])
 
         # And it should report the correct line number.

lldb/tools/lldb-server/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS LLGSOptions.td)
 tablegen(LLVM LLGSOptions.inc -gen-opt-parser-defs)
 add_public_tablegen_target(LLGSOptionsTableGen)
 set_target_properties(LLGSOptionsTableGen PROPERTIES FOLDER "lldb misc")
 
 set(LLDB_PLUGINS)
 
 if(CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND LLDB_PLUGINS lldbPluginProcessLinux)
+  if (CMAKE_SYSTEM_NAME MATCHES "Linux")
+    list(APPEND LLDB_PLUGINS lldbPluginPlatformLinux)
+  else()
+    list(APPEND LLDB_PLUGINS lldbPluginPlatformAndroid)
+  endif()
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessFreeBSD)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformFreeBSD)
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessNetBSD)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformNetBSD)
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "Darwin")
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileMachO)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformMacOSX)
 elseif(CMAKE_SYSTEM_NAME MATCHES "Windows")
   list(APPEND LLDB_PLUGINS lldbPluginObjectFilePECOFF)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformWindows)
 else()
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileELF)
 endif()
 
 if(APPLE_EMBEDDED)
   if(LLDB_CODESIGN_IDENTITY)
     # Use explicit LLDB identity
     set(LLVM_CODESIGNING_IDENTITY ${LLDB_CODESIGN_IDENTITY})

lldb/tools/lldb-server/SystemInitializerLLGS.cpp

 //===-- SystemInitializerLLGS.cpp -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SystemInitializerLLGS.h"
 
 #if defined(__APPLE__)
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 using HostObjectFile = ObjectFileMachO;
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+using HostPlatform = lldb_private::PlatformMacOSX;
 #elif defined(_WIN32)
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
 using HostObjectFile = ObjectFilePECOFF;
+#include "Plugins/Platform/Windows/PlatformWindows.h"
+using HostPlatform = lldb_private::PlatformWindows;
 #else
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 using HostObjectFile = ObjectFileELF;
+#if defined(__ANDROID__)
+#include "Plugins/Platform/Android/PlatformAndroid.h"
+using HostPlatform = lldb_private::platform_android::PlatformAndroid;
+#elif defined(__FreeBSD__)
+#include "Plugins/Platform/FreeBSD/PlatformFreeBSD.h"
+using HostPlatform = lldb_private::platform_freebsd::PlatformFreeBSD;
+#elif defined(__linux__)
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+using HostPlatform = lldb_private::platform_linux::PlatformLinux;
+#elif defined(__NetBSD__)
+#include "Plugins/Platform/NetBSD/PlatformNetBSD.h"
+using HostPlatform = lldb_private::platform_netbsd::PlatformNetBSD;
+#endif
 #endif
 
 #if defined(__arm64__) || defined(__aarch64__) || defined(_M_ARM64)
 #define LLDB_TARGET_ARM64
 #endif
 
 #if defined(__arm__) || defined(__arm) || defined(_ARM) || defined(_M_ARM) ||  \
     defined(LLDB_TARGET_ARM64)

 
 using namespace lldb_private;
 
 llvm::Error SystemInitializerLLGS::Initialize() {
   if (auto e = SystemInitializerCommon::Initialize())
     return e;
 
   HostObjectFile::Initialize();
+  HostPlatform::Initialize();
 
 #if defined(LLDB_TARGET_ARM) || defined(LLDB_TARGET_ARM64)
   EmulateInstructionARM::Initialize();
 #endif
 #if defined(LLDB_TARGET_LoongArch)
   EmulateInstructionLoongArch::Initialize();
 #endif
 #if defined(LLDB_TARGET_MIPS) || defined(LLDB_TARGET_MIPS64)
   EmulateInstructionMIPS::Initialize();
 #endif
 #if defined(LLDB_TARGET_MIPS64)
   EmulateInstructionMIPS64::Initialize();
 #endif
 #if defined(LLDB_TARGET_RISCV)
   EmulateInstructionRISCV::Initialize();
 #endif
 
   return llvm::Error::success();
 }
 
 void SystemInitializerLLGS::Terminate() {
   HostObjectFile::Terminate();
+  HostPlatform::Terminate();
 
 #if defined(LLDB_TARGET_ARM) || defined(LLDB_TARGET_ARM64)
   EmulateInstructionARM::Terminate();
 #endif
 #if defined(LLDB_TARGET_LoongArch)
   EmulateInstructionLoongArch::Terminate();
 #endif
 #if defined(LLDB_TARGET_MIPS) || defined(LLDB_TARGET_MIPS64)