This is an archive of the discontinued LLVM Phabricator instance.

[clangd] NFC: Move stdlib headers handling to Clang
ClosedPublic

Authored by kbobyrev on Feb 7 2022, 4:21 AM.

Details

Summary

This will allow moving the IncludeCleaner library essentials to Clang
and decoupling them from the majority of clangd.

The patch itself just moves the code, it doesn't change existing
functionality.

Diff Detail

Event Timeline

kbobyrev created this revision.Feb 7 2022, 4:21 AM
kbobyrev requested review of this revision.Feb 7 2022, 4:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 7 2022, 4:21 AM
kbobyrev retitled this revision from [clangd] Move stdlibg headers handling to Clang to [clangd] NFC: Move stdlibg headers handling to Clang.Feb 7 2022, 4:21 AM

The moved *.inc files are generated, the generator needs to be moved (and updated?) too I think.

Is having clangd continue to include the .inc files textually intended to stay that way, or is it a FIXME?

This looks sensible to me, but as I wrote this code we should maybe get a second look (@kadircet?) that it makes sense to lift to clang::tooling.

Some positive experience: we've used it successfully in an internal clang-tidy check.

clang-tools-extra/clangd/unittests/HeadersTests.cpp
411–412

Recognizer test also needs to be moved

clang/include/clang/Tooling/Inclusions/StandardLibrary.h
2

This needs a copyright header and a file comment

8

namespace tooling { namespace stdlib {

14

what's the design around C vs C++?

e.g. are std::printf and ::printf distinct symbols, are C printf and C++ ::printf distinct symbols, similarly for headers.

(Fine if only C++ is implemented here, but the interface should probably say one way or the other)

In clangd we had a handle on all the callers, but here we have to worry about acquiring callers that e.g. can't easily provide language information.

clang/lib/Tooling/Inclusions/StandardLibrary.cpp
111

if D is std::chrono I think it just returns "chrono"?

(we should probably have a Recognizer test for this, sorry I left it out...)

kbobyrev updated this revision to Diff 406401.Feb 7 2022, 4:45 AM

Move include-mapping generators to clang and re-generate the files.

Regarding the include mapping generator, I think it would've been better if we had some sort of list directly from libc++ (as this is now being part of clang rather than just clangd), but having the current symbol mapping available for other tools too is definitely a useful improvement and implementation details can change later on.
I think we should have some "more public" documentation available around the limitations of current generator though, so that people are not surprised and aware of the caveats (like symbols might be missing, or in case of ambiguity they might be dropped, etc). Not sure where that belongs though, maybe header of the .inc file, or if we want it to be only used through the recognizer interface, maybe we can make the inc file "private" and document it there.

As for stdlib symbol/header/recognizer, I've got a couple questions around the functionality we are exposing:

  • (briefly mentioned above) should we just make raw symbol mappings an implementation detail of stdlib recognizer and have applications depend on it?
  • do we want headers/symbols to be created outside of the recognizer?
clang/include/clang/Tooling/Inclusions/StandardLibrary.h
16

can you clarify if Name can have quotes/angles ?

38

should scope have trailing :: ?

47

s/my/by

67

what about macros?

kbobyrev updated this revision to Diff 406741.Feb 8 2022, 2:24 AM
kbobyrev marked 9 inline comments as done.

Address review comments.

clang/include/clang/Tooling/Inclusions/StandardLibrary.h
38

This is consistent with the current behavior; we can probably change it later.

67

For now, I'm just moving the code without adding any new capabilities. The only change is in namespaceSymbols (to break the dependency on clangd helpers) that Sam pointed out.

kbobyrev edited the summary of this revision. (Show Details)Feb 8 2022, 2:25 AM

Regarding the include mapping generator, I think it would've been better if we had some sort of list directly from libc++ (as this is now being part of clang rather than just clangd), but having the current symbol mapping available for other tools too is definitely a useful improvement and implementation details can change later on.
I think we should have some "more public" documentation available around the limitations of current generator though, so that people are not surprised and aware of the caveats (like symbols might be missing, or in case of ambiguity they might be dropped, etc). Not sure where that belongs though, maybe header of the .inc file, or if we want it to be only used through the recognizer interface, maybe we can make the inc file "private" and document it there.

As for stdlib symbol/header/recognizer, I've got a couple questions around the functionality we are exposing:

  • (briefly mentioned above) should we just make raw symbol mappings an implementation detail of stdlib recognizer and have applications depend on it?
  • do we want headers/symbols to be created outside of the recognizer?

Right, I agree that using libc++ directly would be better, but for now this just makes the functionality public. We can change it and update as we want afterwards.

sammccall added inline comments.Feb 8 2022, 3:01 AM
clang/include/clang/Tooling/Inclusions/StandardLibrary.h
38

Please document the behavior that's being added here.

(In clangd, "scope" fairly consistently includes "::" - I don't think that convention exists here)

57

I'm not sure what "the callers have to act accordingly..." means - can you drop it or elaborate?

67

Please add a mention of macros to the docs.

clang/lib/Tooling/Inclusions/StandardLibrary.cpp
120

Does this do the right thing for std::__1::chrono where __1 is inline?

clang/unittests/Tooling/StandardLibraryTest.cpp
62

should there be symbols in this NS and a test for them?

kbobyrev updated this revision to Diff 406793.Feb 8 2022, 6:20 AM
kbobyrev marked 5 inline comments as done.

Address the comments.

kbobyrev updated this revision to Diff 406794.Feb 8 2022, 6:24 AM

Add a comment for the helper function.

kbobyrev retitled this revision from [clangd] NFC: Move stdlibg headers handling to Clang to [clangd] NFC: Move stdlib headers handling to Clang.Feb 8 2022, 6:29 AM
kbobyrev updated this revision to Diff 406800.Feb 8 2022, 6:40 AM

Fix the tests.

Oh, and also regarding the Python generator scripts: they don't seem to work with the latest archives :( 2018 mentioned in the docs (works perfectly) but 2019 crashes. Probably not very relevant if we switch to libc++ eventually.

kbobyrev updated this revision to Diff 406827.Feb 8 2022, 7:48 AM

Fix the test.

Oh, and also regarding the Python generator scripts: they don't seem to work with the latest archives :( 2018 mentioned in the docs (works perfectly) but 2019 crashes.

Yikes, we should fix that. Especially since the 2019 version is 3 years out of date already...

Probably not very relevant if we switch to libc++ eventually.

This seems pretty unlikely to me.

sammccall accepted this revision.Feb 9 2022, 1:43 AM
sammccall added inline comments.
clang/unittests/Tooling/StandardLibraryTest.cpp
51

maybe add a test for a C symbol?

This revision is now accepted and ready to land.Feb 9 2022, 1:43 AM
kbobyrev updated this revision to Diff 407093.Feb 9 2022, 2:02 AM
kbobyrev marked an inline comment as done.

Add a test for C Standard Library symbol.

Oh, and also regarding the Python generator scripts: they don't seem to work with the latest archives :( 2018 mentioned in the docs (works perfectly) but 2019 crashes.

Yikes, we should fix that. Especially since the 2019 version is 3 years out of date already...

Agreed! Though, the other problem is that it seems that nothing beyond 2019 version really exists. Which means that even if we support that one, it's unlikely to be up-to-date. For us to be relatively up-to-date we might need to ask someone working on cppreference for new archives and the format is likely to be different from 2019, too, so I don't know if we should fix the issue now vs. try to get newer archives and fix the issue after that.

Probably not very relevant if we switch to libc++ eventually.

This seems pretty unlikely to me.

This revision was landed with ongoing or failed builds.Feb 9 2022, 2:05 AM
This revision was automatically updated to reflect the committed changes.

After this commit I am seeing the link time error

ld.lld: error: undefined symbol: clang::DeclContext::isInlineNamespace() const
>>> referenced by StandardLibrary.cpp:118 (/home/pmw/projects/upstream-llvm/llvm-project/clang/lib/Tooling/Inclusions/StandardLibrary.cpp:118)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::tooling::stdlib::Recognizer::namespaceSymbols(clang::NamespaceDecl const*)::'lambda'()::operator()() const)

ld.lld: error: undefined symbol: clang::Decl::castFromDeclContext(clang::DeclContext const*)
>>> referenced by DeclBase.h:2562 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2562)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::Decl, false>::doit(clang::DeclContext*))
>>> referenced by DeclBase.h:2558 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2558)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::NamedDecl, false>::doit(clang::DeclContext const*))

If I revert this commit it goes away. I've tried a fresh cmake and build and get the same result.

After this commit I am seeing the link time error

ld.lld: error: undefined symbol: clang::DeclContext::isInlineNamespace() const
>>> referenced by StandardLibrary.cpp:118 (/home/pmw/projects/upstream-llvm/llvm-project/clang/lib/Tooling/Inclusions/StandardLibrary.cpp:118)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::tooling::stdlib::Recognizer::namespaceSymbols(clang::NamespaceDecl const*)::'lambda'()::operator()() const)

ld.lld: error: undefined symbol: clang::Decl::castFromDeclContext(clang::DeclContext const*)
>>> referenced by DeclBase.h:2562 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2562)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::Decl, false>::doit(clang::DeclContext*))
>>> referenced by DeclBase.h:2558 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2558)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::NamedDecl, false>::doit(clang::DeclContext const*))

If I revert this commit it goes away. I've tried a fresh cmake and build and get the same result.

Thanks for reporting! This should be fixed in https://github.com/llvm/llvm-project/commit/e3ba831937189ec61e28ced0b0bfc1e41a3510f8, your issue looks the same I found in a failing buildbot; could you please try again and let me know if it's still broken?

Yep, that did the trick. Many Thanks.

thakis added a subscriber: thakis.EditedOct 2 2022, 5:04 PM

This makes clang-format depend on clang's AST library (because Format depends on Tooling/Inclusions, and this makes Tooling/Inclusions depend on AST), which it shouldn't.

Could this move into a dedicated library, or in some library that Format doesn't depend on?

Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2022, 5:04 PM
thakis added a comment.Oct 2 2022, 5:05 PM

Or, alternatively, maybe Recognizer could stay in clangd (or move to a separate lib); looks like that's the only thing in StandardLibrary.h that needs clang/AST/Decl.h.

This makes clang-format depend on clang's AST library

Oops, definitely an unintended outcome.
Ping @kadircet, in case he missed this. He should know best who can fix this.

yeah sorry for the mess here. i agree that we should move StandardLibrary to its own library. i'll try to take a stab at this tomorrow, if no one does it before then.