This is an archive of the discontinued LLVM Phabricator instance.

[lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime
ClosedPublic

Authored by sgraenitz on Mar 14 2023, 8:51 AM.

Details

Summary

This patch adds test infrastructure to utilize the GNUstep runtime in the LLDB test suite and adds coverage for features that already work on Linux. These seem accidental in parts, but it's a good early baseline. On Windows nothing works yet. Please find the repository for the GNUstep ObjC runtime here: https://github.com/gnustep/libobjc2

GNUstep support is disabled by default. CMake configuration involves two variables:

  • LLDB_TEST_OBJC_GNUSTEP=On enables GNUstep support in the test suite. It requires the libobjc2 shared library and headers to be found.
  • LLDB_TEST_OBJC_GNUSTEP=Off disables GNUstep support in the test suite and resets associated cache values if necessary (default).
  • LLDB_TEST_OBJC_GNUSTEP_DIR allows to pass a custom installation root.

Diff Detail

Event Timeline

sgraenitz created this revision.Mar 14 2023, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 8:51 AM
sgraenitz requested review of this revision.Mar 14 2023, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 8:51 AM

I am not in touch with any of the build bot maintainers for Windows/Linux LLDB. Do you think it's worth asking for mainline test support? They'd basically need to build and install libobjc2 once. It's a few hundred kilobytes.

For me the test expectedly fails on Windows 10 and passes on Ubuntu 22.04 LTS right now:

> bin/llvm-lit --filter=objc-gnustep -v tools/lldb/test
-- Testing: 1 of 1979 tests, 1 workers --
PASS: lldb-shell :: Expr/objc-gnustep-print.m (1 of 1)

Testing Time: 1.26s
  Excluded: 1583
  Passed  :    1
lldb/test/CMakeLists.txt
64

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 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)

lldb/test/CMakeLists.txt
57

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.

64

I think we generally use UPPERCASE names for cmake variables.

lldb/test/Shell/helper/build.py
62

How about "Include and link GNUstep libobjc2 (Windows and Linux only)" ? It is a bit clearer imo.

687

Why did these 2 need to move?

lldb/test/Shell/lit.cfg.py
27

There are a couple of .m files in the Shell dir already, I assume the related tests still pass for those?

I am not in touch with any of the build bot maintainers for Windows/Linux LLDB. Do you think it's worth asking for mainline test support? They'd basically need to build and install libobjc2 once. It's a few hundred kilobytes.

Myself and Omair both work on Linaro's bots. We run lldb checks on linux arm/aarch64 and Windows on Arm. We could look at installing it.

Pavel runs the x86 debian bot and I think the x86 Windows bot may have been retired, or at least is offline at the moment (https://lab.llvm.org/buildbot/#/builders/83)

theraven added inline comments.Mar 14 2023, 9:26 AM
lldb/test/CMakeLists.txt
64

This is somewhat complicated. If gnustep-config exists, the runtime supports installing itself into a location defined by the gnustep-make install, which supports NeXT, Apple, and FHS-style layouts. It's probably not worth trying to autodetect the general case.

sgraenitz marked 3 inline comments as done.

Improve help text for the build.py --objc-gnustep option

Myself and Omair both work on Linaro's bots. We run lldb checks on linux arm/aarch64 and Windows on Arm. We could look at installing it.

Right now, my focus is x86 Windows and Linux, but thanks for the offer! So, this would be https://lab.llvm.org/buildbot/#/builders/219 (Windows AArch64) and https://lab.llvm.org/buildbot/#/builders/17 (Linux AArch32)? Let me discuss this internally and come back to you soon.

Pavel runs the x86 debian bot and I think the x86 Windows bot may have been retired, or at least is offline at the moment (https://lab.llvm.org/buildbot/#/builders/83)

Yes, that would be cool. @labath What do you think?

Oh I still saw that one running a few days back! @stella.stamenova in the review you mention a second windows lldb bot, is it x86 as well? I don't see it in https://lab.llvm.org/buildbot/#/console

lldb/test/CMakeLists.txt
64

This is somewhat complicated.
It's probably not worth trying to autodetect the general case.

Configuring with cmake and running the install target gives me these paths. Do you mean I should remove them and just let the user set LLDB_TEST_OBJC_GNUSTEP_DIR?

If gnustep-config exists, the runtime supports installing itself into a location defined by the gnustep-make install, which supports NeXT, Apple, and FHS-style layouts.

Right now, I am assuming to find the shared library in the lib subdirectory and headers in include. Is that different in the other layouts? Do we need to support that?

I think we generally use UPPERCASE names for cmake variables.

I think the convention is: UPPERCASE for global variables, lowercase for local variables

lldb/test/Shell/helper/build.py
687

The flags wouldn't apply for previous source files otherwise. I opted to move them both in order to keep them close in the resulting command.

lldb/test/Shell/lit.cfg.py
27

Interesting, it's exactly one .m and one .mm outside Input directories right? Let me double-check that.

SymbolFile/DWARF/clang-ast-from-dwarf-objc-property.m
SymbolFile/DWARF/x86/module-ownership.mm

Oh I still saw that one running a few days back! @stella.stamenova in the review you mention a second windows lldb bot, is it x86 as well? I don't see it in https://lab.llvm.org/buildbot/#/console

The one I was referring to is https://lab.llvm.org/buildbot/#/builders/219.

Also, the one that was removed was not x86 either, it was x64.

bulbazord added inline comments.
lldb/test/Shell/helper/build.py
254

The assertion message here is not tremendously helpful. Maybe something like "--objc-gnustep specified without path to libobjc2. Use --objc-gnustep-dir to specify said path."

lldb/test/Shell/helper/toolchain.py
46

Why does {0} need quotes around it but the args above it don't?

sgraenitz marked 2 inline comments as done.

Polish assertion message in build.py

The one I was referring to is https://lab.llvm.org/buildbot/#/builders/219.

Ok, thanks for the update. Then we are lacking a Windows x86 build bot for LLDB now!

Also, the one that was removed was not x86 either, it was x64.

Yes, the naming can be confusing. I think the x86 we talked about mainly means 64-bit (often called x64 on Windows) and includes the 32-bit legacy. Like the X86 target backend in LLVM.

lldb/test/Shell/helper/build.py
254

None of the other asserts have messages at all, but ok :)

lldb/test/Shell/helper/toolchain.py
46

Good catch! That's due to spaces in Windows paths...

sgraenitz marked 2 inline comments as done.Mar 14 2023, 12:10 PM

Soo, @DavidSpickett

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)

It would be great to get all the existing Objective C test coverage for free, but I guess it'd take a while to support all of the Apple runtime features. Literally all existing ObjC tests are API tests and when running locally, they are all UNSUPPORTED for me on Linux and Windows. I didn't manage to get them running quickly and thus, I went for a Shell test here. My impression was that API tests are LLDB legacy and new tests would usually be Shell. It might still be worth getting (some of) the API tests to work with GNUstep as well, but I might need some advice on how to do that.

Myself and Omair both work on Linaro's bots. We run lldb checks on linux arm/aarch64 and Windows on Arm. We could look at installing it.

I suspect lldb-arm-ubuntu means 32-bit ARM. This should work, but it's not a priority for us right now. We could still give it a try and see if it works out-of-the-box!

As a side-note: Could the AArch64 bot https://lab.llvm.org/buildbot/#/builders/219 pass LLVM_LIT_ARGS="-v" as well so the log tells us which tests actually ran? That would be great!

lldb/test/CMakeLists.txt
57
lldb/test/Shell/lit.cfg.py
27

These have been running ever since, because https://github.com/llvm/llvm-project/blob/release/16.x/lldb/test/Shell/SymbolFile/DWARF/lit.local.cfg Anyway, thanks for your note!

sgraenitz marked an inline comment as done.Mar 14 2023, 12:11 PM

As a side-note: Could the AArch64 bot https://lab.llvm.org/buildbot/#/builders/219 pass LLVM_LIT_ARGS="-v" as well so the log tells us which tests actually ran? That would be great!

Done in https://github.com/llvm/llvm-zorg/commit/571f67f111870c25699ca47cef020e7d13558d52, it'll be a few days before it takes effect.

DavidSpickett added a comment.EditedMar 15 2023, 4:12 AM

If I run the test without the runtime installed I get these run lines:

: 'RUN: at line 4';   '/usr/bin/python3.8' /home/david.spickett/llvm-project/lldb/test/Shell/helper/build.py --compiler=any --arch=32 --tools-dir=/home/david.spickett/build-llvm-arm/./bin --libs-dir=/home/david.spickett/build-llvm-arm/./lib --objc-gnustep-dir="gnustep_install_dir-NOTFOUND" /home/david.spickett/llvm-project/lldb/test/Shell/Expr/objc-gnustep-print.m --compiler=clang --objc-gnustep --output=/home/david.spickett/build-llvm-arm/tools/lldb/test/Shell/Expr/Output/objc-gnustep-print.m.tmp

Note the NOTFOUND. So cmake does set it to something, which means he feature is added, so the test is run but clearly it won't work. If you look around there are probably some examples of others handling this. Probably checking whether it's found, then assigning the path to some var.

I tested this on 32 bit Arm Linux and after a few bodges (see comment), the test itself works fine. I see the CI for the library cross compiles for this, so it makes sense.

lldb/test/Shell/helper/build.py
256

These need to be defined always, and set to either None or "". Otherwise what happens is if args.objc_gnustep is not set, these variables don't exist and later it will try to read objc_gnustep_inc and fail.

Or you could hasattr later but I think empty defaults is cleaner.

sgraenitz updated this revision to Diff 505528.Mar 15 2023, 9:12 AM
sgraenitz marked an inline comment as done.

Fix CMakeLists.txt and build.py for the case that GNUstep is not installed

If I run the test without the runtime installed I get these run lines [...]

Oh, excellent catch. Thanks! Now, with my last patch, given that:

> grep GNUSTEP CMakeCache.txt
LLDB_TEST_OBJC_GNUSTEP_DIR:PATH=

The test says:

> bin/llvm-lit --filter=gnustep -a tools/lldb/test
-- Testing: 1 of 1979 tests, 1 workers --
UNSUPPORTED: lldb-shell :: Expr/objc-gnustep-print.m (1 of 1)
Test requires the following unavailable features: objc-gnustep
sgraenitz updated this revision to Diff 505529.Mar 15 2023, 9:17 AM

Make the test more extensible

sgraenitz added inline comments.Mar 15 2023, 9:22 AM
lldb/test/CMakeLists.txt
64

@theraven I installed gnustep-config locally, but I don't see how to get the libobjc2 install dir from it. So far, I always built from source and (for me) this works well with the current CMake code. Should we keep it like this or can you give me advice on how to improve it? Thanks.

bulbazord added inline comments.Mar 15 2023, 11:52 AM
lldb/test/Shell/helper/toolchain.py
46

In that case, do we also need to add quotes around the other paths? Seems like we're assuming that on windows those directories cannot have spaces in them. (Not suggesting you need to do that here and now, just wondering for the future)

I think this is ready to land, but I will leave it here for a few more days to allow for further questions.

lldb/test/Shell/helper/toolchain.py
46

I'd like to keep it as proposed here: I added the quotes since the default install path for gnustep libobjc2 was under Program Files for me. The other paths refer to subfolders in the build directory and apparently it hasn't been a problem. I guess no-one selects a path with spaces to build LLDB. That makes sense to me.

I wouldn't propose to add precautionary quotes around these paths. If you wish to do that, please post a review and discuss it.

sgraenitz added a comment.EditedMar 23 2023, 1:01 PM

Is there any more feedback on this patch? After all, this adds test coverage for existing functionality. I also need it to test the new GNUstep runtime plugin from D146154 (which is ready to land).

No issues with the patch. If you want to get it enabled on Linaro's lldb bots, email linaro-toolchain@lists.linaro.org and one of us will set it up for you.

Pavel runs the x86 debian bot and I think the x86 Windows bot may have been retired, or at least is offline at the moment (https://lab.llvm.org/buildbot/#/builders/83)

Yes, that would be cool. @labath What do you think?

What kind of setup is necessary to make that work? If it's not too complicated, I think we could make something work.

Speaking generally, my only concern with that is the that the bot doesn't remain broken (or even worse -- flaky!) for long stretches of time. I don't have time to debug objc failures, and reverting other people's changes because they break something that might be an "experimental" feature for quite some time could be problematic.

My impression was that API tests are LLDB legacy and new tests would usually be Shell. It might still be worth getting (some of) the API tests to work with GNUstep as well, but I might need some advice on how to do that.

The situation is definitely more complicated than that. If you're serious about objc support, I'd definitely recommend getting the existing tests working, as that's the best way to validate correctness and consistency (with the macos runtime). If you're adding some new tests, then they can be shell tests, particularly if they're gnustep-specific.

What kind of setup is necessary to make that work? If it's not too complicated, I think we could make something work.

Thanks, that'd be awesome!

The last official release is from 2020, so for the time being it seems best to build and install from TOT once -- @theraven please correct me if I am wrong:

> git clone https://github.com/gnustep/libobjc2
> cd libobjc2
> git submodule init && git submodule update
> CC=clang-15 CXX=clang++-15 cmake -Bbuild -GNinja -DTESTS=On .
> cd build
> ninja
...
> ctest
...
100% tests passed, 0 tests failed out of 198
> ninja install
...
-- Installing: /usr/local/lib/libobjc.so.4.6
-- Installing: /usr/local/lib/libobjc.so
...

Then apply this patch and re-run CMake. The new test should popup and pass:

> arc patch D146058
> cd build && cmake .
...
-- Found GNUstep ObjC runtime: /usr/local/lib/libobjc.so
...
> bin/llvm-lit --filter=objc-gnustep -vv tools/lldb/test
-- Testing: 1 of 1979 tests, 1 workers --
PASS: lldb-shell :: Expr/objc-gnustep-print.m (1 of 1)

Testing Time: 2.06s
  Excluded: 1583
  Passed  :    1

Speaking generally, my only concern with that is the that the bot doesn't remain broken (or even worse -- flaky!) for long stretches of time. I don't have time to debug objc failures, and reverting other people's changes because they break something that might be an "experimental" feature for quite some time could be problematic.

Absolutely. What do you think is the best way to prevent that?

Right now, the gnustep test would run whenever libobjc.so was found. A lit config reset like this would make it UNSUPPORTED (but it gets wiped with the next CMake run):

> grep objc_gnustep_dir build/tools/lldb/test/Shell/lit.site.cfg.py 
config.objc_gnustep_dir = ""

How can I change this to support the well-behaved bot? Add -DLLDB_TEST_OBJC_GNUSTEP=Off? Or should it be off by default and enabled on demand?

sgraenitz updated this revision to Diff 510732.Apr 4 2023, 3:17 AM

Split config in two parts: On/Off and DIR
Move standard find library logic into FindGNUstepObjC.cmake

sgraenitz edited the summary of this revision. (Show Details)Apr 4 2023, 3:35 AM

The modified proposal keeps GNUstep support off by default (even if installed). It must be enabled explicitly by passing -DLLDB_TEST_OBJC_GNUSTEP=On and it can be disabled easily by passing -DLLDB_TEST_OBJC_GNUSTEP=Off to CMake. A custom install location /path/to/lib/libobjc.so can be specified with -DLLDB_TEST_OBJC_GNUSTEP_DIR=/path/to. Since it involves some extra logic, I moved the standard bits into FindGNUstepObjC.cmake which should follow regular CMake naming conventions (GNUstepObjC_DIR, GNUstepObjC_FOUND, etc.). This might be the foundation for a future GNUstep standard package file.

This version aims to cause minimal friction while allowing to test with GNUstep for those who wish. Please find setup instructions in my previous comment. If you have any more questions, please let me know.

What kind of setup is necessary to make that work? If it's not too complicated, I think we could make something work.

Thanks, that'd be awesome!

The last official release is from 2020, so for the time being it seems best to build and install from TOT once -- @theraven please correct me if I am wrong:

> git clone https://github.com/gnustep/libobjc2
> cd libobjc2
> git submodule init && git submodule update
> CC=clang-15 CXX=clang++-15 cmake -Bbuild -GNinja -DTESTS=On .
> cd build
> ninja
...
> ctest
...
100% tests passed, 0 tests failed out of 198
> ninja install
...
-- Installing: /usr/local/lib/libobjc.so.4.6
-- Installing: /usr/local/lib/libobjc.so
...

Then apply this patch and re-run CMake. The new test should popup and pass:

I'm sorry for the delay. Building a package from source is slightly more that what I have time for right now. As you may have noticed, I don't have much time for LLDB work right now, and I'm trying to keep the buildbot a vanilla debian install so it's easy to reproduce its setup (both for myself and other developers).

How can I change this to support the well-behaved bot? Add -DLLDB_TEST_OBJC_GNUSTEP=Off? Or should it be off by default and enabled on demand?

That's not entirely what I was referring to. What I fear is the following situation. A random developer makes a random patch that happens to break gnustep support. That developer cannot debug that issue locally (cannot or doesn't know how to build gnustep from source), so someone has to help him figure out the problem. I don't want to be the person doing that. :)

That's not entirely what I was referring to. What I fear is the following situation. A random developer makes a random patch that happens to break gnustep support. That developer cannot debug that issue locally (cannot or doesn't know how to build gnustep from source), so someone has to help him figure out the problem. I don't want to be the person doing that. :)

Yes, that's reasonable. In my last patch, I already changed the default to Off.

lldb/test/CMakeLists.txt
54

See here, Off by default

sgraenitz updated this revision to Diff 518169.Apr 29 2023, 8:58 AM
sgraenitz edited the summary of this revision. (Show Details)

Rebase and double-check default config: GNUstep disabled, objc-gnustep-print test unsupported

sgraenitz added a reviewer: Restricted Project.May 14 2023, 8:02 AM

This patch adds test coverage for existing functionality. It was ready to land 6 weeks ago. There was plenty of opportunity to raise concerns and discuss details over the last 2 months of review. I assume the topic is just too niche to attract a lot of attention.

I am planning to land this patch tomorrow and D146154 soon after, since I have some time on Tuesday and Wednesday to cope with build bot reports. I am happy to answer any questions in the remaining time. Otherwise, don't hesitate to follow up post-commit.

sgraenitz updated this revision to Diff 522582.May 16 2023, 6:26 AM

Rebase and re-run pre-merge checks

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2023, 1:39 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.