[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D94888#2511675 <https://reviews.llvm.org/D94888#2511675>, @rupprecht wrote:

> In D94888#2506140 <https://reviews.llvm.org/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.



================
Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:765-770
             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
----------------
rupprecht wrote:
> 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?
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94888