Page MenuHomePhabricator

[lldb] Add -Wl,-rpath to make tests run with fresh built libc++
ClosedPublic

Authored by MaskRay on Jan 17 2021, 12:40 PM.

Details

Summary

On my Debian machine, system libc++/libc++abi is not installed (libc++1-9 libc++abi-9),
21 check-lldb-api tests fail because -stdlib=libc++ linked executables cannot
find runtime libc++.so.1 at runtime.

Use the -Wl,-rpath,$(LLVM_LIBS_DIR) mechanism in
packages/Python/lldbsuite/test/make/Makefile.rules (D58630 for NetBSD) to
allow such tests compile/link with fresh libc++ built beside lldb.
(A system libc++.so.1 is not guaranteed to match fresh libc++ header files.)

Some tweaks to the existing NetBSD rule when generalizing:

  • Drop -L$(LLVM_LIBS_DIR) since Clang driver adds it correctly.
  • Add -stdlib=libc++ only for USE_LIBCPP.

Also, drop -isystem /usr/include/c++/v1 introduced in D9426. It is not needed
by Clang driver. GCC using libc++ requires more setup.

I don't find any test needing -Wl,-rpath in test/Shell/helper/{build,toolchain}.py (D58630 for NetBSD added them).

Diff Detail

Event Timeline

MaskRay created this revision.Jan 17 2021, 12:40 PM
MaskRay requested review of this revision.Jan 17 2021, 12:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2021, 12:40 PM

You seem to be removing more than adding there.

(Works for me, FWIW - takes my local run down from 42 failures in check-lldb-api to 1 (a timeout))

Out of curiousity did you find a way to get lldb-test to print out the stdout/stderr of the underlying lldb process to see what error messages it was producing that demonstrated this failure? I was trying to figure out how to do that, and it seems like a good thing to be able to do for this issue or others down the way.

It looks like this is removing the ability to build libc++ tests with gcc (as it does not have the -stdlib option). While having that ability would be nice, I don't believe there's anyone currently using that configuration, so it shouldn't stand in the way of other things. But we should also update the python detection code then (in canRunLibcxxTests in packages/Python/lldbsuite/test/dotest.py -- I guess you just need to remove the if os.path.isdir("/usr/include/c++/v1"): blurb)

As for testing against the system libc++ with clang, I guess that should still work, as the extra rpath will be just ignored in that case...

Out of curiousity did you find a way to get lldb-test to print out the stdout/stderr of the underlying lldb process to see what error messages it was producing that demonstrated this failure? I was trying to figure out how to do that, and it seems like a good thing to be able to do for this issue or others down the way.

There's no underlying "lldb process" as these tests use lldb as a library through its python api. That said, adding -t (trace) to the lldb-dotest invocation will make it print more information about what the test is doing, though I am not entirely sure how much would it help in this case

It looks like this is removing the ability to build libc++ tests with gcc (as it does not have the -stdlib option). While having that ability would be nice, I don't believe there's anyone currently using that configuration, so it shouldn't stand in the way of other things. But we should also update the python detection code then (in canRunLibcxxTests in packages/Python/lldbsuite/test/dotest.py -- I guess you just need to remove the if os.path.isdir("/usr/include/c++/v1"): blurb)

As for testing against the system libc++ with clang, I guess that should still work, as the extra rpath will be just ignored in that case...

I do not know whether the following few lines D9426 were intentional.

			CXXFLAGS += -isystem /usr/include/c++/v1
			LDFLAGS += -lc++

For a proper setup, I think more stuff is needed. /usr/include/c++/v1 probably works for many Linux distributions but the choice doesn't look nice. When libc++ is built with lldb, it probably use the libc++ include directory instead. LDFLAGS will thus need a specific -L.

I think deleting the code until someone complains is fine? :)

You seem to be removing more than adding there.

Can you kindly test this on NetBSD? :)

It looks like this is removing the ability to build libc++ tests with gcc (as it does not have the -stdlib option). While having that ability would be nice, I don't believe there's anyone currently using that configuration, so it shouldn't stand in the way of other things. But we should also update the python detection code then (in canRunLibcxxTests in packages/Python/lldbsuite/test/dotest.py -- I guess you just need to remove the if os.path.isdir("/usr/include/c++/v1"): blurb)

As for testing against the system libc++ with clang, I guess that should still work, as the extra rpath will be just ignored in that case...

I do not know whether the following few lines D9426 were intentional.

I am pretty sure they were. This flag is needed to use (system) libc++ with gcc, and this was happening at a time when we were running the test suite with gcc, as it was still the default compiler for android. We weren't building libc++ in tree so this did kind of work (it looks like it doesn't work anymore -- the first problem seems to be the use of #include_next in libc++).

I think deleting the code until someone complains is fine? :)

Deleting it should be fine -- just finish the job and remove it from dotest.py too :)

MaskRay updated this revision to Diff 317599.Jan 19 2021, 9:14 AM

Delete if os.path.isdir("/usr/include/c++/v1"):

It looks like this is removing the ability to build libc++ tests with gcc (as it does not have the -stdlib option). While having that ability would be nice, I don't believe there's anyone currently using that configuration, so it shouldn't stand in the way of other things. But we should also update the python detection code then (in canRunLibcxxTests in packages/Python/lldbsuite/test/dotest.py -- I guess you just need to remove the if os.path.isdir("/usr/include/c++/v1"): blurb)

As for testing against the system libc++ with clang, I guess that should still work, as the extra rpath will be just ignored in that case...

I do not know whether the following few lines D9426 were intentional.

I am pretty sure they were. This flag is needed to use (system) libc++ with gcc, and this was happening at a time when we were running the test suite with gcc, as it was still the default compiler for android.

Is anyone running tests in this build mode (gcc + libc++) still? Should we keep this support around?

lldb/packages/Python/lldbsuite/test/dotest.py
765–770

I don't know if this is the right place to do it, but it would be good to also check somewhere that binaries built w/ libc++ actually run. That would help with the error you were seeing. Maybe something like this would work?

MaskRay updated this revision to Diff 318414.Jan 21 2021, 9:26 PM
MaskRay marked an inline comment as done.

Adopt rupprecht's suggestion

I am pretty sure they were. This flag is needed to use (system) libc++ with gcc, and this was happening at a time when we were running the test suite with gcc, as it was still the default compiler for android.

Is anyone running tests in this build mode (gcc + libc++) still? Should we keep this support around?

It doesn't seem to work right now, so I guess the answer to the first question is "no". I don't mind removing it, but I also want to be careful about baking in clang assumptions as one of the purposes of this test suite is to be able to validate that we are able to debug the output of different compilers.

lldb/packages/Python/lldbsuite/test/dotest.py
765–770

I'm not sure what does this help with? This compile command does not contain the -rpath flag, so, if the executable actually depended on libc++, it would not run. And if it does not depend on libc++ features, running it does not tell us anything new.

More generally, this is probably not the best place to implement fancy checks like this. It's a remnant of the time when folks were running dotest directly, and expecting it to just work. These days, it would be better to implement this kind of configuration in the lit config or some place like that. Among other benefits, it would mean that it gets run just once, instead of once for each test. But that is a topic for a different discussion...

MaskRay updated this revision to Diff 318546.Jan 22 2021, 9:13 AM

Drop fancy check in dotest.py (restore the previous behavior)

labath accepted this revision.Jan 24 2021, 11:56 AM

Let's give this a shot.

This revision is now accepted and ready to land.Jan 24 2021, 11:56 AM