This is an archive of the discontinued LLVM Phabricator instance.

[clang][ExtractAPI] Add support for C++ classes
ClosedPublic

Authored by evelez7 on Jun 22 2023, 8:06 AM.

Details

Summary

Add ExtractAPI support C++ classes, fields, methods, and various qualifiers and specifiers

Diff Detail

Event Timeline

evelez7 created this revision.Jun 22 2023, 8:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: ChuanqiXu. · View Herald Transcript
evelez7 requested review of this revision.Jun 22 2023, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 8:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
evelez7 updated this revision to Diff 533788.Jun 22 2023, 2:42 PM

Add access control serialization

evelez7 updated this revision to Diff 533799.Jun 22 2023, 3:14 PM

Do not include return types for constructors/destructors

evelez7 updated this revision to Diff 534060.Jun 23 2023, 1:18 PM

Fix duplicate visitation of CXXRecordDecls by overloading WalkUpFromCXXRecordDecl

evelez7 updated this revision to Diff 534141.Jun 23 2023, 6:34 PM

Add conversion function and overloaded operator support

dang added a comment.Jun 28 2023, 8:39 AM

Starting to come together nicely, but I think now would be a good time to write some tests. I am particularly interested in more complex situations like inheritance hierarchies.

clang/include/clang/ExtractAPI/API.h
25

Inste

637

Not sure this is correct, the semantic relationship is that structs and unions can be classes. This expresses that all struct records and unions are CXXClassRecords. It would be best to keep these entirely separate as classof is used by the type casting functions, e.g. llvm::dynamic_cast and we certainly don't want arbitrary struct or union record to be cast-able to CXXClassRecord.

776

mega nit: I suggest grouping together the has_function_signature trait and the has_access trait separately, i.e. moving the empty line from 770 to between 772 and 773

clang/include/clang/ExtractAPI/DeclarationFragments.h
185

Should this be API.h?

196

Instead of storing a reference to the string here, it might make sense to store the string itself, I am worried that for future usages in libclang the string references would go stale.

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
319–320

Is it to short circuit to only call VisitRecordDecl if so I think that it should call getDerivedExtractAPIVisitor().VisitRecordDecl

401

Is there a way of only looking at public bases here? Non-public inheritance is not part of the API surface of the class.

clang/lib/ExtractAPI/API.cpp
19

AccessControl should move into API.h so we don't need to pull in DeclarationFragments.h

44

This just pollutes the git history, would be best to remove this.

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
607

We still need all symbols to have an "accessLevel" specifier. Maybe the false_type overload of serializeAccessMixin could make "public" the default.

evelez7 updated this revision to Diff 535559.Jun 28 2023, 4:14 PM
evelez7 marked 6 inline comments as done.

Address some review feedback

Remove RK_Struct, RK_Union from CXXClassRecord::classof, use std::string for AccessControl, style and remove unused imports

evelez7 updated this revision to Diff 536071.Jun 29 2023, 5:44 PM

Add C++ class tests, fix exception spec fragments

  • Add tests for single and multiple inheritance, class methods, and functions with noexcept.
  • Exception spec fragments had a space included in it, moved it to separate fragment.

There aren't any variables or fields in the tests currently, waiting on D154038 because of no semicolons in their fragments.

clang/lib/ExtractAPI/API.cpp
19

This include doesn't need to be here, clangd auto-included it. Using FunctionSignature would've explicitly included it in the first place. DeclarationFragments.h is already included in API.h.

I was following FunctionSignature as far as where to declare and define. Moving AccessControl would mean including API.h in DeclarationFragments.h to allow DeclarationFragmentsBuilder::getAccessControl.

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
607

This is done on line 534. "public" is serialized if the returned optional is empty.

dang added a comment.Jul 31 2023, 6:28 AM

Looking pretty good, if you can address the last few bits of feedback I am happy to merge this.

clang/include/clang/ExtractAPI/API.h
770

Does CXXInstanceMethodRecord need one of these as well?

clang/include/clang/ExtractAPI/DeclarationFragments.h
25

Are these necessary?

187

For ergonomics would it make more sense to have a constructor that takes the string in directly and moves it to Access? I don't think there are any situation where you want to change the access string after the fact?

246

I assume the implementation of this moved because it was needed for C++ methods? Can you update the comment and maybe move the implementation to the bottom of the .h file?

clang/lib/ExtractAPI/DeclarationFragments.cpp
617

Please address this, it's pretty important imo.

clang/test/ExtractAPI/methods.cpp
14

I think these are redundant since you have // expected-no-diagnostics below and pass `--verify` to cc1.

evelez7 updated this revision to Diff 546317.EditedAug 1 2023, 10:07 PM
evelez7 marked 4 inline comments as done.

Address review feedback, add tests, and small refactors.

  • Add tests for destructors, constructors, and conversion methods.
  • Remove redundant check-nots in existing tests.
  • Properly record destructor names.
  • Add explicit qualifier to conversions.
  • Remove unused code from getFragmentsForSpecialCXXMethod.
  • Use appendSpace isntead of literals in keyword fragments.
  • Initialize AccessControl with access string instead of using setter.
clang/include/clang/ExtractAPI/API.h
770

CXXInstanceMethodRecord inherits from CXXMethodRecord which holds the signature for all methods.

evelez7 updated this revision to Diff 546323.Aug 1 2023, 10:36 PM

Add test for overloaded operator.

dang accepted this revision.Aug 2 2023, 9:04 AM

LGTM

This revision is now accepted and ready to land.Aug 2 2023, 9:04 AM
This revision was landed with ongoing or failed builds.Aug 2 2023, 10:19 AM
This revision was automatically updated to reflect the committed changes.
haowei added a subscriber: haowei.Aug 2 2023, 11:29 AM

Hi, we are seeing a test error on Clang :: ExtractAPI/constructor_destructor.cpp after this patch was landed. Error message:

Script:
--
: 'RUN: at line 1';   rm -rf /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp
: 'RUN: at line 2';   split-file /b/s/w/ir/x/w/llvm-llvm-project/clang/test/ExtractAPI/constructor_destructor.cpp /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp
: 'RUN: at line 3';   sed -e "s@INPUT_DIR@/b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp@g"  /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/reference.output.json.in >> /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/reference.output.json
: 'RUN: at line 5';   /b/s/w/ir/x/w/llvm_build/bin/clang++ -extract-api -target arm64-apple-macosx -x c++-header  /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/input.h -o /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output.json -Xclang -verify
: 'RUN: at line 9';   sed -e "s@\"generator\": \".*\"@\"generator\": \"?\"@g"  /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output.json >> /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output-normalized.json
: 'RUN: at line 11';   diff /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/reference.output.json /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output-normalized.json
--
Exit Code: 1

Command Output (stderr):
--
clang++: llvm/include/llvm/Support/JSON.h:317: llvm::json::Value::Value(std::string): Assertion `false && "Invalid UTF-8 in value used as JSON"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /b/s/w/ir/x/w/llvm_build/bin/clang++ -extract-api -target arm64-apple-macosx -x c++-header /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/input.h -o /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output.json -Xclang -verify
#0 0x000055a2ec85c458 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/b/s/w/ir/x/w/llvm_build/bin/clang+++0x83e5458)
clang++: error: clang frontend command failed with exit code 134 (use -v to see invocation)
Fuchsia clang version 18.0.0 (https://llvm.googlesource.com/llvm-project dad9de0ae5360b18c890985d212bec266bf8c122)
Target: arm64-apple-macosx
Thread model: posix
InstalledDir: /b/s/w/ir/x/w/llvm_build/bin
clang++: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: /b/s/w/ir/x/t/lit-tmp-0iloyqe3/input-5086dc.hh
clang++: note: diagnostic msg: /b/s/w/ir/x/t/lit-tmp-0iloyqe3/input-5086dc.sh
clang++: note: diagnostic msg: 

********************

--

Link to the builder: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8773854265208228753/overview

It looks like it triggers an assertion error in llvm's JSON library. Could you take a look please? If it takes some time to fix, could you revert your change please? Thanks.

This comment was removed by evelez7.
haowei added a comment.Aug 2 2023, 1:44 PM

Thanks for reverting. I wasn't doing any changes to this when you were asking though, but looking at build log, it looks like the assertion error disappeared after f4de606ef271fe362f03d30c53a850f9877ec238 was landed.

@haowei Thank you for letting me know! That patch does seem to fix it, unfortunately it didn't fail on my end. Thanks again.

haowei added a comment.Aug 2 2023, 3:43 PM

FYI, this patch fails the Windows bots in a different way. (you can see the error triggered in the pre-merge build in https://buildkite.com/llvm-project/premerge-checks/builds/161603#018909c4-fe90-4de6-91a3-d251dc5ada8f)

The error message from our builders are:

Script:
--
: 'RUN: at line 1';   rm -rf C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp
: 'RUN: at line 2';   split-file C:\b\s\w\ir\x\w\llvm-llvm-project\clang\test\ExtractAPI\class.cpp C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp
: 'RUN: at line 3';   sed -e "s@INPUT_DIR@C:/b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/class.cpp.tmp@g"  C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json.in >> C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json
: 'RUN: at line 5';   c:\b\s\w\ir\x\w\llvm_build\bin\clang.exe++ -extract-api -target arm64-apple-macosx -x c++-header  C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/input.h -o C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output.json -Xclang -verify
: 'RUN: at line 9';   sed -e "s@\"generator\": \".*\"@\"generator\": \"?\"@g"  C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output.json >> C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output-normalized.json
: 'RUN: at line 11';   diff C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output-normalized.json
--
Exit Code: 127

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "rm" "-rf" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp"
$ ":" "RUN: at line 2"
$ "split-file" "C:\b\s\w\ir\x\w\llvm-llvm-project\clang\test\ExtractAPI\class.cpp" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp"
$ ":" "RUN: at line 3"
$ "sed" "-e" "s@INPUT_DIR@C:/b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/class.cpp.tmp@g" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json.in"
$ ":" "RUN: at line 5"
$ "c:\b\s\w\ir\x\w\llvm_build\bin\clang.exe++" "-extract-api" "-target" "arm64-apple-macosx" "-x" "c++-header" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/input.h" "-o" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output.json" "-Xclang" "-verify"
# command stderr:
'c:\\b\\s\\w\\ir\\x\\w\\llvm_build\\bin\\clang.exe++': command not found
error: command failed with exit status: 127

--

Link to the failed task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8773853314698659521/overview

%clang++ is probably not the correct way to invoke clang++ as it was replaced as clang.exe++ on Windows

evelez7 reopened this revision.Aug 2 2023, 4:09 PM
This revision is now accepted and ready to land.Aug 2 2023, 4:09 PM
evelez7 updated this revision to Diff 546635.Aug 2 2023, 4:15 PM

Reintroduce fix from f4de606ef271 and use clang frontend for C++ tests.

evelez7 closed this revision.Aug 3 2023, 7:06 AM