[Lldb-commits] [PATCH] D146058: [lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

I am not familiar with Objective C, especially beyond Apple platforms. Are you aiming to run all the existing Objective C test cases with this different runtime or will it be mainly new tests?
(would be very neat if you got all the existing stuff for free)



================
Comment at: lldb/test/CMakeLists.txt:32
+  set(gnustep_info lib/libobjc.so)
+elseif (WIN32 AND EXISTS "C:/Program Files (x86)/libobjc/lib/objc.dll"
+              AND EXISTS "C:/Program Files (x86)/libobjc/include/objc/runtime.h")
----------------
Does this library work on Windows on Arm (AArch64 Windows)?

Fine not to handle that in this change, just curious. We (Linaro) do have an lldb bot for it, so it could be added at some point.


================
Comment at: lldb/test/CMakeLists.txt:39
+if (LLDB_TEST_OBJC_GNUSTEP_DIR)
+  message(STATUS "Found GNUstep ObjC runtime: ${LLDB_TEST_OBJC_GNUSTEP_DIR}/${gnustep_info}")
+endif()
----------------
sgraenitz wrote:
> Might be worth a `FindGNUstep.cmake` at some point? (Or is there one somewhere?)
> 
> Windows default install dir:
> ```
> -- Found GNUstep ObjC runtime: C:/Program Files (x86)/libobjc/lib/objc.dll
> ```
> 
> Linux default install dir:
> ```
> -- Found GNUstep ObjC runtime: /usr/local/lib/libobjc.so
> ```
I think we generally use UPPERCASE names for cmake variables.


================
Comment at: lldb/test/Shell/helper/build.py:62
+                    default=False,
+                    help='Windows and Linux include/link GNUstep libobjc2 for this build')
+
----------------
How about "Include and link GNUstep libobjc2 (Windows and Linux only)" ? It is a bit clearer imo.


================
Comment at: lldb/test/Shell/helper/build.py:687
+        args.extend(['-o', obj])
+        args.append(source)
+
----------------
Why did these 2 need to move?


================
Comment at: lldb/test/Shell/lit.cfg.py:27
 # by individual lit.local.cfg files in the test subdirectories.
-config.suffixes = ['.test', '.cpp', '.s']
+config.suffixes = ['.test', '.cpp', '.s', '.m']
 
----------------
There are a couple of .m files in the Shell dir already, I assume the related tests still pass for those?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146058