This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add an extract-api driver option
ClosedPublic

Authored by zixuw on Jan 20 2022, 10:37 AM.

Details

Summary

This is the initial commit for the clang-extract-api RFC
https://lists.llvm.org/pipermail/cfe-dev/2021-September/068768.html
Add a new driver option -extract-api and associate it with a dummy
(for now) frontend action to set up the initial structure for
incremental works.

Diff Detail

Event Timeline

zixuw created this revision.Jan 20 2022, 10:37 AM
zixuw requested review of this revision.Jan 20 2022, 10:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 10:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zixuw added a reviewer: arphaman.
ributzka accepted this revision.Jan 21 2022, 10:12 AM

LGTM. Just a few minor nits.

clang/lib/Driver/ToolChains/Clang.cpp
7082

Is this change required for extract-api? I don't think the output file needs to be renamed, because there is only one output in this case.

clang/lib/Frontend/CompilerInvocation.cpp
2431

This looks like an unrelated change.

This revision is now accepted and ready to land.Jan 21 2022, 10:12 AM
zixuw added inline comments.Jan 21 2022, 10:42 AM
clang/lib/Frontend/CompilerInvocation.cpp
2431

Result of clang-format, I can restore this line

zixuw updated this revision to Diff 402074.Jan 21 2022, 12:36 PM
  • Remove unrelated whitespace change
  • Remove unnecessary code
zixuw marked 2 inline comments as done.Jan 21 2022, 12:37 PM
clang/include/clang/Driver/Types.def
103

Symbol graph files generally have the extension .symbols.json - is that something that should be reflected here?

zixuw added inline comments.Jan 21 2022, 3:35 PM
clang/include/clang/Driver/Types.def
103

Actually I don't know. Looking at other types in this file, I don't see any existing ones with "two level" suffixes and I don't know if it makes sense (or be any useful) to put that here. The output file name (including the suffix) is still whatever the user put after the -o option. From the comment at the beginning of this file, this suffix here is only used for temporary files created, if any would be during the extract-api pipeline.

zixuw added a comment.Jan 21 2022, 3:36 PM

Pre-merge check failure with clang-format complaints :(

zixuw added a comment.Jan 24 2022, 3:17 PM

Ping for any additional reviews/comments.
I must admit that I got really confused and lost in the layers of the driver and setting up the phases. I wasn't sure if I made enough (or too many?) changes to make this work. So any comments or advice are welcomed.

arphaman accepted this revision.Jan 25 2022, 9:39 PM
This revision was landed with ongoing or failed builds.Jan 26 2022, 11:31 AM
This revision was automatically updated to reflect the committed changes.
morehouse added inline comments.
clang/lib/Frontend/ExtractAPIConsumer.cpp
21

ASTContext *Context is unused, and causing buildbot failures: https://lab.llvm.org/buildbot/#/builders/77/builds/13645

/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/clang/lib/Frontend/ExtractAPIConsumer.cpp:20:15: error: private field 'Context' is not used [-Werror,-Wunused-private-field]
  ASTContext *Context;
              ^
1 error generated.

Please fix.

MaskRay added a subscriber: MaskRay.Feb 8 2022, 8:48 PM

Thanks for working on such tools but the patch order is not right.
You should implement the functionality first, and in the last, add the driver option.
The driver option is user-facing and the availability makes users think the functionality is ready when it actually isn't.

I am also a bit unsure whether the option name should use the single-dash -e* since -e is a short option taking a value, so a typo in -extract-api cannot be detected.
By convention a two-dash option should be used.
(Yes, I know -emit-llvm is a violation.)

zixuw added a comment.Feb 9 2022, 4:03 PM

Thanks for working on such tools but the patch order is not right.
You should implement the functionality first, and in the last, add the driver option.
The driver option is user-facing and the availability makes users think the functionality is ready when it actually isn't.

I see. I was working on the functionality at the same time with this driver option, but it took longer and more complicated than I expected so I decided to post this as an initial patch to keep things small and incremental. I do have another patch ready to post very shortly.

I am also a bit unsure whether the option name should use the single-dash -e* since -e is a short option taking a value, so a typo in -extract-api cannot be detected.
By convention a two-dash option should be used.
(Yes, I know -emit-llvm is a violation.)

That's an interesting find, I'll look into it.