Some API test failures are really opaque/could be improved

Subprojects LLDB
#1

Wondering if anyone else has encountered/dealt with debugging lldb test failures like the one shown at the end of this email (“AssertionError: 10 != 5” in “test.assertEqual(process.GetState(), lldb.eStateStopped)” while checking that a breakpoint was reached)

Is there anything that could be done to improve the debuggability of such failures? Logging standard output/error from the lldb process or any other logging it might have? At least for one of these I managed to figure it out by running lldb directly on the binary and finding that the binary couldn’t find libc++.so (that’s a story for another bug/email thread, looks like maybe lldb libc++ pretty printer tests are using the system installed libc++, not the just-built libc++ (& thus not running if there is no system installed libc++)). But my current failure like this seems a bit more inscrutible and I’m still looking into it.

Looks like dotest.py has some sense of logging (it has a --log-success option which says preserves the logs even on failure - though the output of dotest.py, at least for me, has no mention of logs, log files, or where they might be located, and looking at the source points to some sort of “.log” files… ah, found some)

So, yeah, there do seem to be some Failure.log, SkippedTest.log, etc - should dotest print something about the paths to those files when it exits with failure, maybe?

#2

I think ⚙ D111978 [lldb] Improve the error message in run_to_breakpoint_do_run ,
⚙ D111981 [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms and the other patches Pavel & me put
up today should improve this situation IIUC.

- Raphael

#3

I think https://reviews.llvm.org/D111978 ,
https://reviews.llvm.org/D111981 and the other patches Pavel & me put
up today should improve this situation IIUC.

Thanks Raphael - really appreciate you & looking into this!

With https://reviews.llvm.org/D111981 I still seem to not have that cxx dependency (building/running the test, then building libcxx, then running the test again goes from unsupported → failing) - didn’t seem to work for me?

The diagnostic improvement sounds good to me (pity about whatever limitations it has re: remote or windows execution gathering the stdout). I guess gathering the logs in the remote execution case has the same problem (that the log is made on the remote machine/non-trivial to retrieve?)?

& yeah, have you got any patches/ideas about how to make the libcxx tests use the just-built libcxx library (using LD_LIBRARY_PATH, rpath, etc)? For now, even with both these fixes I’ll just be seeing (consistent, which is nice) failures, rather than being able to run these tests successfully. I’ll either have to get used to ignoring certain failures, or disable the tests by not building libcxx in that build tree, which would also be unfortunate. (or maybe there’s some other workarounds?) Any idea how this works for other folks?

  • Dave
#4

I just saw in your review comment that this is using
LLVM_ENABLE_RUNTIMES and not LLVM_ENABLE_PROJECTS for libcxx, so the
failure just comes from us setting the wrong RPATH due to the
different runtimes library directory (at least from what I can see).
Would it be possible to put libcxx in LLVM_ENABLE_PROJECTS for now? I
think this shouldn't be too hard to fix (famous last words?).

#5

Actually the RPATH theory is wrong, but the LLVM_ENABLE_PROJECT
workaround *should* still work.

#6

Actually the RPATH theory is wrong, but the LLVM_ENABLE_PROJECT
workaround should still work.

I’ll give that a go (it’s running at the moment) though I guess this is inconsistent with the direction libcxx is moving in for building, re: https://groups.google.com/g/llvm-dev/c/tpuLxk_ipLw

#7

Yep, it does work with LLVM_ENABLE_PROJECT rather than LLVM_ENABLE_RUNTIME.

Specifically the test binary is linked with an rpath to the just-built lib directory that ensures the just-built libc++.so is found:

/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang main.o -g -O0 -fno-builtin -m64 -I/usr/local/google/home/blaikie/dev/llvm/src/lldb/packages/Python/lldbsuite/test/make/…/…/…/…/…/include -I/usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list -I/usr/local/google/home/blaikie/dev/llvm/src/lldb/packages/Python/lldbsuite/test/make -include /usr/local/google/home/blaikie/dev/llvm/src/lldb/packages/Python/lldbsuite/test/make/test_common.h -fno-limit-debug-info -gsplit-dwarf -stdlib=libc++ -Wl,-rpath,/usr/local/google/home/blaikie/dev/llvm/build/release/./lib --driver-mode=g++ -o “a.out”

Oh, actually it passes the same rpath when using LLVM_ENABLE_RUNTIME, but the libc++.so.1 is in a different place: ./lib/x86_64-unknown-linux-gnu/libc++.so.1

Looks like this rpath setting happens here: (changing this to a junk argument causes the test to fail to build as expected)
https://github.com/llvm/llvm-project/blob/618583565687f5a494066fc902a977f6057fc93e/lldb/packages/Python/lldbsuite/test/make/Makefile.rules#L400

And it gets the LLVM_LIBS_DIR from here: https://github.com/llvm/llvm-project/blob/207998c242c8c8a270ff22a5136da87338546725/lldb/test/API/lit.cfg.py#L163

So maybe we need to pass down the default target triple too, since that seems to be how libc++ is deciding where to put the library? ( https://github.com/llvm/llvm-project/blob/207998c242c8c8a270ff22a5136da87338546725/libcxx/CMakeLists.txt#L424 ) at least on non-apple :confused: (or maybe there’s some way to make the connection between the two less brittle - for libc++'s build to export some variable that lldb can use, or for LLVM to provide something for both to use?)

Yeah, applying this change does work for me, but wouldn’t work on Apple for instance (where libcxx doesn’t add the default target triple to the path):

$ git diff

diff --git lldb/test/API/lit.site.cfg.py.in lldb/test/API/lit.site.cfg.py.in

index 987078a53edb…e327429b7ff9 100644

— lldb/test/API/lit.site.cfg.py.in

+++ lldb/test/API/lit.site.cfg.py.in

@@ -3,7 +3,7 @@

config.llvm_src_root = “@LLVM_SOURCE_DIR@”

config.llvm_obj_root = “@LLVM_BINARY_DIR@”

config.llvm_tools_dir = “@LLVM_TOOLS_DIR@”

-config.llvm_libs_dir = “@LLVM_LIBS_DIR@”

+config.llvm_libs_dir = “@LLVM_LIBS_DIR@/@LLVM_DEFAULT_TARGET_TRIPLE@”

config.llvm_shlib_dir = “@SHLIBDIR@”

config.llvm_build_mode = “@LLVM_BUILD_MODE@”

config.lit_tools_dir = “@LLVM_LIT_TOOLS_DIR@”

Thoughts?

#8

+Louis Dionne - perhaps the libcxx and lldb folks would be interesting in finding a suitable way to address this issue, since currently either option (using libcxx in ENABLE_PROJECTS or using it in ENABLE_RUNTIMES) is incomplete - if I use ENABLE_RUNTIMES I get the libcxx testing run against the just-built clang and generally this is the “supported” configuration, but then some lldb tests fail because they can’t find libcxx.so.1 (on Linux) - and using ENABLE_PROJECTS means not using the just-built clang for libcxx tests (so missing the libcxx breakages caused by my array name change) but do use the just-built libcxx in lldb tests and find failures there…

#9

I believe the issue is probably not related so much to LLVM_ENABLE_PROJECTS vs LLVM_ENABLE_RUNTIMES, but rather to the fact that LLVM_ENABLE_RUNTIMES uses per-target runtime directories now (hasn’t always been the case), which basically means that libc++ ends up in <prefix>/lib/<target-triple>/libc++.so instead of <prefix>/lib/libc++.so.

I think you either want to specify the per-target library dir when running against libc++, or you want to disable that and use LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF when configuring the runtimes. In all cases, you want to be using LLVM_ENABLE_RUNTIMES and not LLVM_ENABLE_PROJECTS, since the latter is deprecated now.

Cheers,
Louis

#10

I believe the issue is probably not related so much to LLVM_ENABLE_PROJECTS vs LLVM_ENABLE_RUNTIMES, but rather to the fact that LLVM_ENABLE_RUNTIMES uses per-target runtime directories now (hasn’t always been the case), which basically means that libc++ ends up in <prefix>/lib/<target-triple>/libc++.so instead of <prefix>/lib/libc++.so.

Ish, yes. It’s a bug in LLVM_ENABLE_RUNTIMES that isn’t present in LLVM_ENABLE_PROJECTS, so for now if I want to run the lldb pretty printer tests for libc++ on Linux it seems the only way I can is by using the deprecated functionality of libc++ in LLVM_ENABLE_PROJECTS.

Consider this a bug report (looks like a bug in the lldb CMake configuration, not in libc++'s build itself, but something to figure out if Linux lldb devs are going to use libc++ +.ENABLE_RUNTIMES path) on that deprecation?

I think you either want to specify the per-target library dir when running against libc++, or you want to disable that and use LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF when configuring the runtimes. In all cases, you want to be using LLVM_ENABLE_RUNTIMES and not LLVM_ENABLE_PROJECTS, since the latter is deprecated now.

I didn’t enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR myself/in the root cmake config. It looks like it’s hardcoded(?) into the ENABLE_RUNTIMES sub-build? https://github.com/llvm/llvm-project/blob/e5fb79b31424267704e9d2d9674089fd7316453e/llvm/runtimes/CMakeLists.txt#L76 I’m not sure there’s any way to override that from the root? And in any case I’d have thought the defaults would need to/be intended to work correctly on supported platforms?

So something in lldb’s dir handling (maybe some general infrastructure in LLVM could use some improvement to provide an LLVM_RUNTIME_LIBS_DIR, or similar? that could then be used from other places - rather than libc++, for instance, creating that directory for itself based on LLVM_LIBS_DIR and LLVM_ENABLE_PER_TARGET_RUNTIME_DIR, etc) needs some fixes to support the current defaults/hardcoded modes on Linux?

#11

Is someone currently working on fixing this? FWIW, I think David's
change seems to go in the right direction (when I originally looked at
this I also ended up on the wrong rpath but I thought it was some
other code that set the wrong value. Didn't realize we have two places
where this happens). I think David's diff is better than we currently
have so maybe we should just turn this into a review?

libc++ pretty printer test dependencies
#12

I haven’t made any further progress on it - I think the actual git diff I posted, changing config.llvm_libs_dir wouldn’t quite be shippable as-is, because it’s only correct to add the “/@LLVM_DEFAULT_TARGET_TRIPLE@” if the runtimes were built with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR ON (which is the default on Linux, but not on MacOS) - so some extra conditionality is needed, but I’m not sure of the best/right place to implement that.

#13

@JDevlieghere - this seems to have more of the state I’d forgotten in the intervening 6 months…

So,

  • using LLVM_ENABLE_PROJECTS (deprecated)
  • using LLVM_ENABLE_RUNTIMES and ~somehow~ disabling LLVM_ENABLE_PER_TARGET_RUNTIME_DIR in the libc++ build (hardcoded ON in the LLVM_ENABLE_RUNTIMES subbuild here), - seems like even if we got that working, it’s still not ideal that the default configuration is broken/unsupported/has failures in it.
  • or (conditionally) adding /@LLVM_DEFAULT_TARGET_TRIPLE@ here

Presumably the last of these is the right answer, but I don’t know nearly enough cmake to make a reasonable go at making this work correctly for different platforms, and whether or not LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is enabled…

#14

Thanks. Coincidentally, I seem to be running into a similar issue myself. There was a change to libcxx that required us to update the formatter. Ismail landed a fix in ⚙ D128694 [lldb/Dataformatters] Adapt C++ std::string dataformatter for D128285 and now all my libcxx tests are failing, which means that unlike GreenDragon, I’m not testing the just-built libcxx.

#15

That makes it sound like we have an lldb that only works with one of the libcxx layouts, and not older ones. That doesn’t seem right, shouldn’t we support both versions?

Jim

#16

I wish we could do that but AFAIK, there is no way to know which version of libcxx (and thus which layout) we are using … IIUC, the version the user could be using on their system could be totally different from the one lldb builds against

#17

What about the fallback that Adrian suggested? Based on the test output, it looks like we know we can’t parse the data, so can we detect that and use the formatter for the old layout?

#18

I would say that (one of) our problems is that we don’t have a good way of checking that, after any given pretty printer change, it keeps formatting the things that it is supposed to format. Given the recent pace of libc++ layout changes, and if we want to stay in the one-formatter-to-format-them-all world, we should come up with a solution to that. My attempt for that is ⚙ D124155 [lldb] Add tests which simulate the various std::string layouts. It is far from perfect, but unless someone has a better idea, I think we should check that in (I am looking for some to click “accept”).

Also given the pace of the layout changes, I think we may need to put some limit on the number of layouts we support. We probably don’t need/want to support every possible layout that ever existed. Maybe we could limit ourselves to layouts that existed in the last N libc++ releases, and possibly (for folks not following releases) layouts that existed in the last M months

1 Like
#19

Can we tell by looking at the names of the ivars of the raw structure? If they change the layout in a way we need to know about, but keep all the same ivars, that won’t help, but that seems unlikely.

Jim

#20

Couldn’t they provide a layout description for each version that we could use ? That would be more efficient than blindly try to infer the layout ourselves.