Page MenuHomePhabricator

[lldb] -stdlib=libc++ for linking with lldb lib also if LLVM_ENABLE_LIBCXX
Needs ReviewPublic

Authored by llunak on Aug 23 2020, 4:21 AM.

Details

Summary

Otherwise on Linux e.g. the api/command-return-object test uses libstdc++ for the test app and libc++ is used for the lldb library, and some objects such as std::shared_ptr are not binary compatible between the two libs, leading to "mysterious" crashes.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

llunak created this revision.Aug 23 2020, 4:21 AM
llunak requested review of this revision.Aug 23 2020, 4:21 AM
teemperor added a subscriber: teemperor.

Thanks for working on this! This looks good to me, but I wonder if doing this with a dedicated flag instead of an environment variable would be better. But I'll leave that to the others who have a better idea how the dotest flags should work.

Out of curiosity, will this also fix the TestPluginCommands.py ?

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

Maybe move this below the config.llvm_use_sanitizer line, as this is currently in the middle of the python/dotest.py-related config flags.

This looks good to me, but I wonder if doing this with a dedicated flag instead of an environment variable would be better. But I'll leave that to the others who have a better idea how the dotest flags should work.

Yeah, please make this a dedicated flag and store its value in the configuration. We've been slowly (but steadily) moving away from passing around things trough the environment.

I actually think tests relying on this should be converted to c++ unit tests. The logic to compile and link against liblldb takes up a considerable chunk of our test buildsystem. Why recreate that logic, when cmake knows how to do that already? TestPluginCommands might be more involved, but converting api/command-return-object should be trivial using the api unit test framework we already have (unittests/API)...

llunak updated this revision to Diff 287451.Aug 24 2020, 11:16 AM

Updated to not use environment variable.

llunak marked an inline comment as done.Aug 24 2020, 11:18 AM

Out of curiosity, will this also fix the TestPluginCommands.py ?

Yes.

The change to use the configuration looks good, but I think Pavel raised a good point. Which tests are affected by this? How much work would it be to convert them as suggested?

Which tests are affected by this?

I don't know. There are 5 hits for 'buildDriver' and 8 for '%include_SB_APIs%' under tests/, so at least 13, but I don't know if that's all.

How much work would it be to convert them as suggested?

I don't know either. I don't plan on doing that, at least not now. My patch is a fix and it's done. The suggestion is an improvement. surely it can be done after the fix.