This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Provide target specific directories to libclang
ClosedPublic

Authored by kpdev42 on Sep 30 2021, 6:33 AM.

Details

Summary

On Linux some C++ and C include files reside in target specific directories, like /usr/include/x86_64-linux-gnu.
Patch adds them to libclang, so LLDB jitter has more chances to compile expression.

OS Laboratory. Huawei Russian Research Institute. Saint-Petersburg

Diff Detail

Event Timeline

kpdev42 created this revision.Sep 30 2021, 6:33 AM
kpdev42 requested review of this revision.Sep 30 2021, 6:33 AM
yakush added a subscriber: yakush.Sep 30 2021, 6:40 AM
granata.enrico resigned from this revision.Sep 30 2021, 9:34 AM

Thanks for the patch! Out of curiosity: What distribution is using target-specific include paths?

I can take a look at this tomorrow, but just from scrolling over I think the general direction of this patch seems fine, so I think I only have a few nits here and there.

Should we be testing if these directories exist before trying to use them? Might be nice to avoid compiler warnings if the compiler will emit warnings or errors if these directories don't exist.

LLDB also tests with compilers that were built, like when LLDB builds clang and uses that clang and clang++ that it built to run the test suite. If we had settings in LLDB that users could set, then the test suite would be able to use the include files for the compiler that is being used instead of always defaulting to the system headers.

Should we be testing if these directories exist before trying to use them? Might be nice to avoid compiler warnings if the compiler will emit warnings or errors if these directories don't exist.

I think testing if these directories exists may be a little bit redundant because clang ignores non-existent include paths passed with -I

LLDB also tests with compilers that were built, like when LLDB builds clang and uses that clang and clang++ that it built to run the test suite. If we had settings in LLDB that users could set, then the test suite would be able to use the include files for the compiler that is being used instead of always defaulting to the system headers.

Could you please clarify: "LLDB builds clang" - here you mean clang which was build with LLDB? And I would like to mention that starting from https://reviews.llvm.org/D89013 libcxx puts __config_site header to target specific folder

Should we be testing if these directories exist before trying to use them? Might be nice to avoid compiler warnings if the compiler will emit warnings or errors if these directories don't exist.

I think testing if these directories exists may be a little bit redundant because clang ignores non-existent include paths passed with -I

no worries then

LLDB also tests with compilers that were built, like when LLDB builds clang and uses that clang and clang++ that it built to run the test suite. If we had settings in LLDB that users could set, then the test suite would be able to use the include files for the compiler that is being used instead of always defaulting to the system headers.

Could you please clarify: "LLDB builds clang" - here you mean clang which was build with LLDB? And I would like to mention that starting from https://reviews.llvm.org/D89013 libcxx puts __config_site header to target specific folder

We often build the full clang compiler during LLDB builds and then use the clang we built as the compiler when running our test suite. So anything we can do to make sure what ever clang we use for building the test suite binaries has all of the headers that it was built with along with any support library headers (libcxx, etc) that would be great.

LLDB also tests with compilers that were built, like when LLDB builds clang and uses that clang and clang++ that it built to run the test suite. If we had settings in LLDB that users could set, then the test suite would be able to use the include files for the compiler that is being used instead of always defaulting to the system headers.

Could you please clarify: "LLDB builds clang" - here you mean clang which was build with LLDB? And I would like to mention that starting from https://reviews.llvm.org/D89013 libcxx puts __config_site header to target specific folder

We often build the full clang compiler during LLDB builds and then use the clang we built as the compiler when running our test suite. So anything we can do to make sure what ever clang we use for building the test suite binaries has all of the headers that it was built with along with any support library headers (libcxx, etc) that would be great.

That's fine, but patch is about *libclang* seeing target specific headers, not clang frontend. While clang frontend obtains system header paths through Toolchain-derived classes (lib/Driver/Toolchain), one has to *explicitly* set those paths when calling libclang. That's exactly what lldb does in CppModuleConfiguration.cpp when evaluating expression.

Looks good to me, but the expression parser isn't my main area of expertise. I would like to see someone with more knowledge in the expression parser to make the final ok. Raphael?

teemperor requested changes to this revision.Oct 28 2021, 8:20 AM

Apologies for the delay, this slipped out of my review queue by accident.

The patch looks in general pretty much ready, I just have a few nits here and there.

(Also just to clarify what this code does. It scans the support files of some (LLDB) modules and then returns a list of include paths that are needed to compile the correct libc++/libc of the target. I fixed up the class docs a bit to make this clearer).

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp
34–62

Could this just return a llvm::SmallVector<std::string, 2> ? Then you can just use it directly in the for-range below without any variable.

Some docs would be nice:

/// Returns the target-specific default include paths for libc.
46

Could this just return llvm::Optional<std::string>? Also the parameter should technically be path_to_file (this file uses LLDB's variable_naming_style instead of the usualNamingStyle, even though the existing code already has some code style issues where it deviated towards LLVM's style but oh well)

46
/// Returns the include path matching the given pattern for the given file path (or None if the path doesn't match the pattern).
80
// Check if this is a target-specific libc++ include directory.
83

.str() instead of .toStringRef(tmp) should also work.

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
46

Please add a comment that this is optional unlike the other members:

/// This is an optional path only required on some systems.
52

Same as above.

67–69
/// The triple (if valid) is used to search for target-specific include paths.
lldb/unittests/Expression/CppModuleConfigurationTest.cpp
75

Could you please duplicate this test and make the target-specific directory its own new test? E.g. TEST_F(CppModuleConfigurationTest, LinuxTargetSpecificInclude). Just to make sure we also still support the old behaviour.

144

Same as above: both the new and old behaviour should work so just copy this into a new test.

This revision now requires changes to proceed.Oct 28 2021, 8:20 AM
kpdev42 updated this revision to Diff 388118.Nov 18 2021, 12:06 AM

Addressed review comments

kpdev42 marked 7 inline comments as done.Nov 24 2021, 10:53 PM
teemperor accepted this revision.Nov 24 2021, 11:17 PM

LGTM, thanks for fixing this!

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp
86–93

This can be moved to the assignments below (and then the double parenthesis can disappear too).

This revision is now accepted and ready to land.Nov 24 2021, 11:17 PM
kpdev42 edited the summary of this revision. (Show Details)Nov 25 2021, 12:13 AM
This revision was automatically updated to reflect the committed changes.