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

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz marked 3 inline comments as done.
sgraenitz added a comment.

Hi David, thanks for your notes. Please find responses inline.



================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp:42
+                                                    LanguageType language) {
+  if (language != eLanguageTypeObjC)
+    return nullptr;
----------------
theraven wrote:
> I'm not familiar with lldb internals, will this exclude ObjC++?
Good question. There is also `eLanguageTypeObjC_plus_plus`, but the plugin scanner e.g. seems to use `eLanguageTypeObjC` for both. I'd like to keep it like that until I actually start testing ObjC++.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp:47
+  const llvm::Triple &TT = process->GetTarget().GetArchitecture().GetTriple();
+  if (TT.getVendor() == llvm::Triple::VendorType::Apple)
+    return nullptr;
----------------
theraven wrote:
> I never finished the v2 ABI support for Mach-O, but v1 works on Apple platforms and v2 will eventually.  We definitely shouldn't be the default here, but it would be nice not to prevent it working on macOS.
For the moment it would be nice to stay away from potential conflicts, since they won't make reviews easier. We can still add this later on if there is demand.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp:131
+                                        ExecutionContext &exe_ctx) {
+  // TODO: This function is supposed to check whether an ObjC selector is
+  // present for an object. Might be implemented similar as in the Apple V2
----------------
theraven wrote:
> I'm not sure how this is meant to work.  I'd expect this to call one of the runtime's introspection functions.  I believe the Apple version has variants of these that run without acquiring locks, which can be used in the debugger.  We can add these quite easily if necessary.
The Apple V2 runtime has two ways to do it: https://github.com/llvm/llvm-project/blob/release/16.x/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp#L1179 I experimented with the one that doesn't require `void *gdb_class_getClass(void*)` from the runtime library, but the JIT kept on crashing. So, for this first version I opted for a minimal implementation that doesn't do any selector checks. Let's look at it in another review.


================
Comment at: lldb/test/Shell/Expr/objc-gnustep-print.m:68
+//
+// MEMBERS_OUTSIDE: (lldb) p t->_int
+// MEMBERS_OUTSIDE: (int) $0 = 0
----------------
theraven wrote:
> It's not clear from this test if it's correctly computing the offsets.  I suspect that it isn't, since I don't see any reference to the ivar offset variables.
What exactly would you expect to see in the output?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146154