This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix detection of existing libcxx
ClosedPublic

Authored by fdeazeve on Sep 10 2022, 4:40 AM.

Details

Summary

The CMake variable LLDB_HAS_LIBCXX is passed to
llvm_canonicalize_cmake_booleans, which transforms TRUE/FALSE into
'1'/'0'. It also transforms undefined variables to '0'.

In particular, this means that the configuration script for LLDB API's
test always has _some_ value for the has_libcxx configuration:

config.has_libcxx = '@LLDB_HAS_LIBCXX@'

When deciding whether a libcxx exist, the testing scripts would only
check for the existence of has_libcxx, but not for its value. In other
words, because if ('0') is true in python we always think there is a
libcxx.

This was caught once D132940 was merged and most tests started to use
libcxx by default if has_libcxx is true. Prior to that, no failures
were seen because only tests are marked with
@add_test_categories(["libc++"]) would require a libcxx, and these
would be filtered out on builds without the libcxx target. Furthermore,
the MacOS bots always build libcxx.

We fix this by making has_libcxx a boolean (instead of a string) and
by checking its value in the test configuration.

Diff Detail

Event Timeline

fdeazeve created this revision.Sep 10 2022, 4:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2022, 4:40 AM
fdeazeve requested review of this revision.Sep 10 2022, 4:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2022, 4:40 AM
fdeazeve updated this revision to Diff 459348.Sep 11 2022, 4:08 AM
fdeazeve edited the summary of this revision. (Show Details)

Reworded commit message

JDevlieghere added inline comments.Sep 11 2022, 11:32 AM
lldb/test/API/lit.cfg.py
94

is_enabled maybe?

180–185

Are there other places where this helper would be useful? If not I think we can check the value directly. With the helper we lose a little bit of signal (if it returns false, was it not configured or set to false?). I don't think it's a big deal, but those two things combined make me favor inlining the check.

fdeazeve added inline comments.Sep 12 2022, 5:13 AM
lldb/test/API/lit.cfg.py
180–185

I agree that inlining is better here, but note that config.has_libcxx doesn't work, this is still going to return True: config.has_libcxx is a string, not a boolean, so anything != None -- in particular '0' -- will evaluate to true.

There are two ways to inline this:

1.is_configured('has_libcxx') and config.has_libcxx == '1'
2.is_configured('has_libcxx') == '1'

Actually, since has_libcxx comes from a canonicalized variable (in the sense described in the commit message), we could also just do:

3.config.has_libcxx == '1'

What do you think?

Related: in a separate patch I think we should change the implementation of is_configured to compare against None, so that it returns a boolean.

fdeazeve updated this revision to Diff 459441.Sep 12 2022, 6:43 AM

Addressed review comment

JDevlieghere added inline comments.Sep 12 2022, 8:20 AM
lldb/test/API/lit.cfg.py
180–185

Aha, if has_libcxx is a string then that's a bug. It should be 0 or 1 like config.shared_libs and config.lldb_enable_python. Seems like someone (me) unintentionally added quotes.

This should fix that:

-config.has_libcxx = '@LLDB_HAS_LIBCXX@'
+config.has_libcxx = @LLDB_HAS_LIBCXX@
fdeazeve updated this revision to Diff 459474.Sep 12 2022, 8:46 AM
fdeazeve edited the summary of this revision. (Show Details)

Addressed review comment

This revision is now accepted and ready to land.Sep 12 2022, 9:02 AM
This revision was automatically updated to reflect the committed changes.