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

[MaskRay]

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

[mgorny]

You seem to be removing more than adding there.

[dblaikie]

(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.

[labath]

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

[labath]

In D94888#2505698, @dblaikie wrote:

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

[MaskRay]

In D94888#2505992, @labath wrote:

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

[labath]

In D94888#2506000, @MaskRay wrote:

In D94888#2505992, @labath wrote:

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]

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

[rupprecht]

In D94888#2506140, @labath wrote:

In D94888#2506000, @MaskRay wrote:

In D94888#2505992, @labath wrote:

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]

Adopt rupprecht's suggestion

[labath]

In D94888#2511675, @rupprecht wrote:

In D94888#2506140, @labath wrote:

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]

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

[labath]

Let's give this a shot.

lldb/packages/Python/lldbsuite/test/dotest.py

def canRunLibcxxTests():
     from lldbsuite.test import lldbplatformutil
 
     platform = lldbplatformutil.getPlatform()
 
     if lldbplatformutil.target_is_android() or lldbplatformutil.platformIsDarwin():
         return True, "libc++ always present"
 
     if platform == "linux":
-        if os.path.isdir("/usr/include/c++/v1"):
-            return True, "Headers found, let's hope they work"
         with tempfile.NamedTemporaryFile() as f:
             cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", f.name, "-"]
             p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
             _, stderr = p.communicate("#include <algorithm>\nint main() {}")
             if not p.returncode:
                 return True, "Compiling with -stdlib=libc++ works"
             return False, "Compiling with -stdlib=libc++ fails with the error: %s" % stderr
 
     return False, "Don't know how to build with libc++ on %s" % platform
 
 def checkLibcxxSupport():
     result, reason = canRunLibcxxTests()
     if result:
         return # libc++ supported
     if "libc++" in configuration.categories_list:

lldb/packages/Python/lldbsuite/test/make/Makefile.rules

ifeq "$(MAKE_GMODULES)" "YES"
   CXXFLAGS += $(MANDATORY_MODULE_BUILD_CFLAGS)
 endif
 
 CFLAGS += $(CFLAGS_EXTRAS)
 CXXFLAGS += -std=c++11 $(CFLAGS) $(ARCH_CXXFLAGS) $(CXXFLAGS_EXTRAS)
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-  ifeq ($(OS),NetBSD)
-    LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-  endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
   ifneq (,$(filter YES,$(ENABLE_THREADS)))
     LDFLAGS += -pthread
   endif
 endif
 OBJECTS =
 EXE ?= a.out
 
ifeq (1,$(USE_LIBSTDCPP))
   ifneq (,$(findstring clang,$(CC)))
     CXXFLAGS += -stdlib=libstdc++ -DLLDB_USING_LIBSTDCPP
     LDFLAGS += -stdlib=libstdc++
   endif
 endif
 
 ifeq (1,$(USE_LIBCPP))
   CXXFLAGS += -DLLDB_USING_LIBCPP
-  ifeq "$(OS)" "Linux"
-    ifneq (,$(findstring clang,$(CC)))
-      CXXFLAGS += -stdlib=libc++
-      LDFLAGS += -stdlib=libc++
-    else
-      CXXFLAGS += -isystem /usr/include/c++/v1
-      LDFLAGS += -lc++
-    endif
-  else ifeq "$(OS)" "Android"
+  ifeq "$(OS)" "Android"
     # Nothing to do, this is already handled in
     # Android.rules.
   else
     CXXFLAGS += -stdlib=libc++
     LDFLAGS += -stdlib=libc++
   endif
+  ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+    ifneq (,$(LLVM_LIBS_DIR))
+      LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+    endif
+  endif
 endif
 
 #----------------------------------------------------------------------
 # Additional system libraries
 #----------------------------------------------------------------------
 ifeq (1,$(USE_LIBDL))
   ifeq (,$(filter $(OS), NetBSD Windows_NT))
     LDFLAGS += -ldl