Page MenuHomePhabricator

[clang-cl] Add -emit-ast to clang-cl driver
ClosedPublic

Authored by thieta on Jun 23 2022, 1:42 AM.

Details

Summary

Also make the output of -emit-ast end up where /o points.
The same with .plist files from the static analyzer.

These are changes needed to make it possible to do CTU static
analysing work with clang-cl.

Diff Detail

Event Timeline

thieta created this revision.Jun 23 2022, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 1:42 AM
thieta requested review of this revision.Jun 23 2022, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 1:42 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
thieta added inline comments.Jun 23 2022, 1:43 AM
clang/include/clang/Driver/Options.td
834

Whops. My editor strikes again and removes a trailing space. Let me know if I should edit this out of this commit.

clang/lib/Driver/Driver.cpp
5629

I do wonder if this limitation here is really wanted. It makes clang-cl behave a bit different from clang/clang++. I can also split this part out in it's own commit if we feel like it's necessary.

hans added a comment.Jun 23 2022, 4:08 AM

I'm unfamiliar with -emit-ast. Can you add some background on what this is for? What's CTU?

In any case this needs a test.

clang/include/clang/Driver/Options.td
834

Yes please. (Or land it separately.)

clang/lib/Driver/Driver.cpp
5629

What limitation are you referring to?

I'm unfamiliar with -emit-ast. Can you add some background on what this is for? What's CTU?

CTU is cross translation unit. In this case the clang-static-analyzer can do analysis over several files - see the official docs that recommend that you build the .ast files with -emit-ast:
https://clang.llvm.org/docs/analyzer/user-docs/CrossTranslationUnit.html#manual-ctu-analysis

In any case this needs a test.

I can definitely add tests. What do you think needs a test here? that -emit-ast works the same with clang and clang-cl? Do we generally test the same arguments for all drivers even if they are expected to do the same thing?

clang/lib/Driver/Driver.cpp
5629

For the clang-cl options /Fo and /o we limit the exactly what should be written out to that file with TY_Object, TY_LTO_BC etc. But for the -o option we just dump everything with a few exceptions: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L5534

I haven't analyzed this method that carefully - but it seems like for most users using /o should be more or less analogous to -o?

thieta updated this revision to Diff 439341.Jun 23 2022, 5:07 AM

Removed unintentional whitespace removal and landed that as a NFC

thieta marked an inline comment as done.Jun 23 2022, 5:07 AM
hans added a comment.Jun 23 2022, 5:21 AM

I'm unfamiliar with -emit-ast. Can you add some background on what this is for? What's CTU?

CTU is cross translation unit. In this case the clang-static-analyzer can do analysis over several files - see the official docs that recommend that you build the .ast files with -emit-ast:
https://clang.llvm.org/docs/analyzer/user-docs/CrossTranslationUnit.html#manual-ctu-analysis

Thanks! I wasn't aware of this.

In any case this needs a test.

I can definitely add tests. What do you think needs a test here? that -emit-ast works the same with clang and clang-cl? Do we generally test the same arguments for all drivers even if they are expected to do the same thing?

I think we should just check that clang-cl accepts the flag and passes the right thing to the -cc1 invocation (including that we get the output file name right).

clang/lib/Driver/Driver.cpp
5629

For /Fe, /Fo we need to match MSVC's behavior, so I think we do need these different cases. Note that we also allow /o for TY_Image in the if-else further down.

Perhaps TY_AST and Plist should have its own case?

thieta updated this revision to Diff 440178.Jun 27 2022, 5:20 AM

Added tests and moved the check for TY_Plist and TY_AST to it's own if statement block

hans accepted this revision.Jun 27 2022, 7:15 AM

It feels kind of weird to use the /Fo option for this that's for object files, but if that makes using the static analyzer more convenient that's okay I suppose.

clang/test/Driver/ast.c
30 ↗(On Diff #440178)

This needs a -- before %s otherwise clang-cl might interpret the filename as a flag, e.g. if it starts with /Users which is common on Mac.

This revision is now accepted and ready to land.Jun 27 2022, 7:15 AM
thieta updated this revision to Diff 440227.Jun 27 2022, 7:51 AM

Fixed missing -- in test file

Fixed the test issue. Regarding /Fo bit - if you want I can change it to only handle /o in this if statement. I don't mind either way.

clang/test/Driver/ast.c
30 ↗(On Diff #440178)

Fixed - will also fix the other tests in the same file as a NFC.

hans added a comment.Jun 27 2022, 8:10 AM

Regarding /Fo bit - if you want I can change it to only handle /o in this if statement. I don't mind either way.

Just doing /o seems better. Thanks.

clang/test/Driver/ast.c
30 ↗(On Diff #440178)

It's only an issue for clang-cl since it supports flags starting with slashes. I don't think there are other clang-cl tests in the file.

thieta updated this revision to Diff 440242.Jun 27 2022, 8:36 AM

Just handle /o for AST and plist files

thieta updated this revision to Diff 440243.Jun 27 2022, 8:37 AM

Updated commit message

thieta edited the summary of this revision. (Show Details)Jun 27 2022, 8:37 AM
hans accepted this revision.Jun 27 2022, 8:50 AM

lgtm

This revision was landed with ongoing or failed builds.Jun 28 2022, 12:11 AM
This revision was automatically updated to reflect the committed changes.